Discussion:
[gem5-dev] Review Request: MEM: Separate snoops and normal memory requests/responses
(too old to reply)
Andreas Hansson
2012-04-02 13:52:18 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/1118/
-----------------------------------------------------------

Review request for Default.


Description
-------

MEM: Separate snoops and normal memory requests/responses

This patch introduces port access methods that separates snoop
request/responses from normal memory request/responses. The
differentiation is made for functional, atomic and timing accesses and
builds on the introduction of master and slave ports.

Before the introduction of this patch, the packets belonging to the
different phases of the protocol (request -> [forwarded snoop request
-> snoop response]* -> response) all use the same port access
functions, even though the snoop packets flow in the opposite
direction to the normal packet. That is, a coherent master sends
normal request and receives responses, but receives snoop requests and
sends snoop responses (vice versa for the slave). These two distinct
phases now use different access functions, as described below.

Starting with the functional access, a master sends a request to a
slave through sendFunctional, and the request packet is turned into a
response before the call returns. In a system without cache coherence,
this is all that is needed from the functional interface. For the
cache-coherent scenario, a slave also sends snoop requests to coherent
masters through sendFunctionalSnoop, with responses returned within
the same packet pointer. This is currently used by the bus and caches,
and the LSQ of the O3 CPU. The send/recvFunctional and
send/recvFunctionalSnoop are moved from the Port super class to the
appropriate subclass.

Atomic accesses follow the same flow as functional accesses, with
request being sent from master to slave through sendAtomic. In the
case of cache-coherent ports, a slave can send snoop requests to a
master through sendAtomicSnoop. Just as for the functional access
methods, the atomic send and receive member functions are moved to the
appropriate subclasses.

The timing access methods are different from the functional and atomic
in that requests and responses are separated in time and
send/recvTiming are used for both directions. Hence, a master uses
sendTiming to send a request to a slave, and a slave uses sendTiming
to send a response back to a master, at a later point in time. Snoop
requests and responses travel in the opposite direction, similar to
what happens in functional and atomic accesses. With the introduction
of this patch, it is possible to determine the direction of packets in
the bus, and no longer necessary to look for both a master and a slave
port with the requested port id.

In contrast to the normal recvFunctional, recvAtomic and recvTiming
that are pure virtual functions, the recvFunctionalSnoop,
recvAtomicSnoop and recvTimingSnoop have a default implementation that
calls panic. This is to allow non-coherent master and slave ports to
not implement these functions.


Diffs
-----

src/sim/system.hh 97f06a79b6f5
src/mem/port.cc 97f06a79b6f5
src/mem/ruby/system/RubyPort.hh 97f06a79b6f5
src/mem/ruby/system/RubyPort.cc 97f06a79b6f5
src/mem/port.hh 97f06a79b6f5
src/mem/packet_queue.hh 97f06a79b6f5
src/mem/packet_queue.cc 97f06a79b6f5
src/mem/mport.cc 97f06a79b6f5
src/mem/mport.hh 97f06a79b6f5
src/mem/cache/cache.hh 97f06a79b6f5
src/mem/cache/cache_impl.hh 97f06a79b6f5
src/mem/cache/base.hh 97f06a79b6f5
src/mem/bus.cc 97f06a79b6f5
src/cpu/simple/timing.hh 97f06a79b6f5
src/cpu/simple/timing.cc 97f06a79b6f5
src/cpu/testers/directedtest/RubyDirectedTester.hh 97f06a79b6f5
src/cpu/testers/directedtest/RubyDirectedTester.cc 97f06a79b6f5
src/cpu/testers/memtest/memtest.hh 97f06a79b6f5
src/cpu/testers/memtest/memtest.cc 97f06a79b6f5
src/cpu/testers/networktest/networktest.hh 97f06a79b6f5
src/cpu/testers/networktest/networktest.cc 97f06a79b6f5
src/cpu/testers/rubytest/RubyTester.hh 97f06a79b6f5
src/cpu/testers/rubytest/RubyTester.cc 97f06a79b6f5
src/dev/io_device.hh 97f06a79b6f5
src/dev/io_device.cc 97f06a79b6f5
src/mem/bridge.hh 97f06a79b6f5
src/mem/bridge.cc 97f06a79b6f5
src/mem/bus.hh 97f06a79b6f5
src/cpu/base.cc 97f06a79b6f5
src/cpu/inorder/cpu.hh 97f06a79b6f5
src/cpu/inorder/cpu.cc 97f06a79b6f5
src/cpu/o3/cpu.hh 97f06a79b6f5
src/cpu/o3/cpu.cc 97f06a79b6f5
src/cpu/o3/lsq.hh 97f06a79b6f5
src/cpu/o3/lsq_impl.hh 97f06a79b6f5
src/cpu/simple/atomic.hh 97f06a79b6f5
src/arch/x86/pagetable_walker.hh 97f06a79b6f5
src/arch/x86/pagetable_walker.cc 97f06a79b6f5
src/cpu/base.hh 97f06a79b6f5

Diff: http://reviews.gem5.org/r/1118/diff/


Testing
-------

util/regress all passing (disregarding t1000 and eio)


Thanks,

Andreas Hansson
Ali Saidi
2012-04-03 16:43:48 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/1118/#review2414
-----------------------------------------------------------

Ship it!


Seems fine to me.


src/mem/bus.cc
<http://reviews.gem5.org/r/1118/#comment2809>

?



- Ali Saidi
Post by Andreas Hansson
-----------------------------------------------------------
http://reviews.gem5.org/r/1118/
-----------------------------------------------------------
(Updated April 2, 2012, 6:52 a.m.)
Review request for Default.
Description
-------
MEM: Separate snoops and normal memory requests/responses
This patch introduces port access methods that separates snoop
request/responses from normal memory request/responses. The
differentiation is made for functional, atomic and timing accesses and
builds on the introduction of master and slave ports.
Before the introduction of this patch, the packets belonging to the
different phases of the protocol (request -> [forwarded snoop request
-> snoop response]* -> response) all use the same port access
functions, even though the snoop packets flow in the opposite
direction to the normal packet. That is, a coherent master sends
normal request and receives responses, but receives snoop requests and
sends snoop responses (vice versa for the slave). These two distinct
phases now use different access functions, as described below.
Starting with the functional access, a master sends a request to a
slave through sendFunctional, and the request packet is turned into a
response before the call returns. In a system without cache coherence,
this is all that is needed from the functional interface. For the
cache-coherent scenario, a slave also sends snoop requests to coherent
masters through sendFunctionalSnoop, with responses returned within
the same packet pointer. This is currently used by the bus and caches,
and the LSQ of the O3 CPU. The send/recvFunctional and
send/recvFunctionalSnoop are moved from the Port super class to the
appropriate subclass.
Atomic accesses follow the same flow as functional accesses, with
request being sent from master to slave through sendAtomic. In the
case of cache-coherent ports, a slave can send snoop requests to a
master through sendAtomicSnoop. Just as for the functional access
methods, the atomic send and receive member functions are moved to the
appropriate subclasses.
The timing access methods are different from the functional and atomic
in that requests and responses are separated in time and
send/recvTiming are used for both directions. Hence, a master uses
sendTiming to send a request to a slave, and a slave uses sendTiming
to send a response back to a master, at a later point in time. Snoop
requests and responses travel in the opposite direction, similar to
what happens in functional and atomic accesses. With the introduction
of this patch, it is possible to determine the direction of packets in
the bus, and no longer necessary to look for both a master and a slave
port with the requested port id.
In contrast to the normal recvFunctional, recvAtomic and recvTiming
that are pure virtual functions, the recvFunctionalSnoop,
recvAtomicSnoop and recvTimingSnoop have a default implementation that
calls panic. This is to allow non-coherent master and slave ports to
not implement these functions.
Diffs
-----
src/sim/system.hh 97f06a79b6f5
src/mem/port.cc 97f06a79b6f5
src/mem/ruby/system/RubyPort.hh 97f06a79b6f5
src/mem/ruby/system/RubyPort.cc 97f06a79b6f5
src/mem/port.hh 97f06a79b6f5
src/mem/packet_queue.hh 97f06a79b6f5
src/mem/packet_queue.cc 97f06a79b6f5
src/mem/mport.cc 97f06a79b6f5
src/mem/mport.hh 97f06a79b6f5
src/mem/cache/cache.hh 97f06a79b6f5
src/mem/cache/cache_impl.hh 97f06a79b6f5
src/mem/cache/base.hh 97f06a79b6f5
src/mem/bus.cc 97f06a79b6f5
src/cpu/simple/timing.hh 97f06a79b6f5
src/cpu/simple/timing.cc 97f06a79b6f5
src/cpu/testers/directedtest/RubyDirectedTester.hh 97f06a79b6f5
src/cpu/testers/directedtest/RubyDirectedTester.cc 97f06a79b6f5
src/cpu/testers/memtest/memtest.hh 97f06a79b6f5
src/cpu/testers/memtest/memtest.cc 97f06a79b6f5
src/cpu/testers/networktest/networktest.hh 97f06a79b6f5
src/cpu/testers/networktest/networktest.cc 97f06a79b6f5
src/cpu/testers/rubytest/RubyTester.hh 97f06a79b6f5
src/cpu/testers/rubytest/RubyTester.cc 97f06a79b6f5
src/dev/io_device.hh 97f06a79b6f5
src/dev/io_device.cc 97f06a79b6f5
src/mem/bridge.hh 97f06a79b6f5
src/mem/bridge.cc 97f06a79b6f5
src/mem/bus.hh 97f06a79b6f5
src/cpu/base.cc 97f06a79b6f5
src/cpu/inorder/cpu.hh 97f06a79b6f5
src/cpu/inorder/cpu.cc 97f06a79b6f5
src/cpu/o3/cpu.hh 97f06a79b6f5
src/cpu/o3/cpu.cc 97f06a79b6f5
src/cpu/o3/lsq.hh 97f06a79b6f5
src/cpu/o3/lsq_impl.hh 97f06a79b6f5
src/cpu/simple/atomic.hh 97f06a79b6f5
src/arch/x86/pagetable_walker.hh 97f06a79b6f5
src/arch/x86/pagetable_walker.cc 97f06a79b6f5
src/cpu/base.hh 97f06a79b6f5
Diff: http://reviews.gem5.org/r/1118/diff/
Testing
-------
util/regress all passing (disregarding t1000 and eio)
Thanks,
Andreas Hansson
Andreas Hansson
2012-04-03 23:21:15 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/1118/
-----------------------------------------------------------

(Updated April 3, 2012, 4:21 p.m.)


Review request for Default.


Description
-------

MEM: Separate snoops and normal memory requests/responses

This patch introduces port access methods that separates snoop
request/responses from normal memory request/responses. The
differentiation is made for functional, atomic and timing accesses and
builds on the introduction of master and slave ports.

Before the introduction of this patch, the packets belonging to the
different phases of the protocol (request -> [forwarded snoop request
-> snoop response]* -> response) all use the same port access
functions, even though the snoop packets flow in the opposite
direction to the normal packet. That is, a coherent master sends
normal request and receives responses, but receives snoop requests and
sends snoop responses (vice versa for the slave). These two distinct
phases now use different access functions, as described below.

Starting with the functional access, a master sends a request to a
slave through sendFunctional, and the request packet is turned into a
response before the call returns. In a system without cache coherence,
this is all that is needed from the functional interface. For the
cache-coherent scenario, a slave also sends snoop requests to coherent
masters through sendFunctionalSnoop, with responses returned within
the same packet pointer. This is currently used by the bus and caches,
and the LSQ of the O3 CPU. The send/recvFunctional and
send/recvFunctionalSnoop are moved from the Port super class to the
appropriate subclass.

Atomic accesses follow the same flow as functional accesses, with
request being sent from master to slave through sendAtomic. In the
case of cache-coherent ports, a slave can send snoop requests to a
master through sendAtomicSnoop. Just as for the functional access
methods, the atomic send and receive member functions are moved to the
appropriate subclasses.

The timing access methods are different from the functional and atomic
in that requests and responses are separated in time and
send/recvTiming are used for both directions. Hence, a master uses
sendTiming to send a request to a slave, and a slave uses sendTiming
to send a response back to a master, at a later point in time. Snoop
requests and responses travel in the opposite direction, similar to
what happens in functional and atomic accesses. With the introduction
of this patch, it is possible to determine the direction of packets in
the bus, and no longer necessary to look for both a master and a slave
port with the requested port id.

In contrast to the normal recvFunctional, recvAtomic and recvTiming
that are pure virtual functions, the recvFunctionalSnoop,
recvAtomicSnoop and recvTimingSnoop have a default implementation that
calls panic. This is to allow non-coherent master and slave ports to
not implement these functions.


Diffs (updated)
-----

src/arch/x86/pagetable_walker.hh 570b44fe6e04
src/arch/x86/pagetable_walker.cc 570b44fe6e04
src/cpu/base.hh 570b44fe6e04
src/cpu/base.cc 570b44fe6e04
src/cpu/inorder/cpu.hh 570b44fe6e04
src/cpu/inorder/cpu.cc 570b44fe6e04
src/cpu/o3/cpu.hh 570b44fe6e04
src/cpu/o3/cpu.cc 570b44fe6e04
src/cpu/o3/lsq.hh 570b44fe6e04
src/cpu/o3/lsq_impl.hh 570b44fe6e04
src/cpu/simple/atomic.hh 570b44fe6e04
src/cpu/simple/timing.hh 570b44fe6e04
src/cpu/simple/timing.cc 570b44fe6e04
src/cpu/testers/directedtest/RubyDirectedTester.hh 570b44fe6e04
src/cpu/testers/directedtest/RubyDirectedTester.cc 570b44fe6e04
src/cpu/testers/memtest/memtest.hh 570b44fe6e04
src/cpu/testers/memtest/memtest.cc 570b44fe6e04
src/cpu/testers/networktest/networktest.hh 570b44fe6e04
src/cpu/testers/networktest/networktest.cc 570b44fe6e04
src/cpu/testers/rubytest/RubyTester.hh 570b44fe6e04
src/cpu/testers/rubytest/RubyTester.cc 570b44fe6e04
src/dev/io_device.hh 570b44fe6e04
src/dev/io_device.cc 570b44fe6e04
src/mem/bridge.hh 570b44fe6e04
src/mem/bridge.cc 570b44fe6e04
src/mem/bus.hh 570b44fe6e04
src/mem/bus.cc 570b44fe6e04
src/mem/cache/base.hh 570b44fe6e04
src/mem/cache/cache.hh 570b44fe6e04
src/mem/cache/cache_impl.hh 570b44fe6e04
src/mem/mport.hh 570b44fe6e04
src/mem/mport.cc 570b44fe6e04
src/mem/packet_queue.hh 570b44fe6e04
src/mem/packet_queue.cc 570b44fe6e04
src/mem/port.hh 570b44fe6e04
src/mem/port.cc 570b44fe6e04
src/mem/ruby/system/RubyPort.hh 570b44fe6e04
src/mem/ruby/system/RubyPort.cc 570b44fe6e04
src/sim/system.hh 570b44fe6e04

Diff: http://reviews.gem5.org/r/1118/diff/


Testing
-------

util/regress all passing (disregarding t1000 and eio)


Thanks,

Andreas Hansson
Nilay Vaish
2012-04-05 16:34:28 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/1118/#review2448
-----------------------------------------------------------



src/mem/ruby/system/RubyPort.cc
<http://reviews.gem5.org/r/1118/#comment2852>

Andreas, this packet is sent to invalidate loads in the LSQ. This packet was earlier handled in the LSQ's recvTiming() function. Since that code for snooping is being moved to recvTimingSnoop(), I think this code should change as well.


- Nilay Vaish
Post by Andreas Hansson
-----------------------------------------------------------
http://reviews.gem5.org/r/1118/
-----------------------------------------------------------
(Updated April 3, 2012, 4:21 p.m.)
Review request for Default.
Description
-------
MEM: Separate snoops and normal memory requests/responses
This patch introduces port access methods that separates snoop
request/responses from normal memory request/responses. The
differentiation is made for functional, atomic and timing accesses and
builds on the introduction of master and slave ports.
Before the introduction of this patch, the packets belonging to the
different phases of the protocol (request -> [forwarded snoop request
-> snoop response]* -> response) all use the same port access
functions, even though the snoop packets flow in the opposite
direction to the normal packet. That is, a coherent master sends
normal request and receives responses, but receives snoop requests and
sends snoop responses (vice versa for the slave). These two distinct
phases now use different access functions, as described below.
Starting with the functional access, a master sends a request to a
slave through sendFunctional, and the request packet is turned into a
response before the call returns. In a system without cache coherence,
this is all that is needed from the functional interface. For the
cache-coherent scenario, a slave also sends snoop requests to coherent
masters through sendFunctionalSnoop, with responses returned within
the same packet pointer. This is currently used by the bus and caches,
and the LSQ of the O3 CPU. The send/recvFunctional and
send/recvFunctionalSnoop are moved from the Port super class to the
appropriate subclass.
Atomic accesses follow the same flow as functional accesses, with
request being sent from master to slave through sendAtomic. In the
case of cache-coherent ports, a slave can send snoop requests to a
master through sendAtomicSnoop. Just as for the functional access
methods, the atomic send and receive member functions are moved to the
appropriate subclasses.
The timing access methods are different from the functional and atomic
in that requests and responses are separated in time and
send/recvTiming are used for both directions. Hence, a master uses
sendTiming to send a request to a slave, and a slave uses sendTiming
to send a response back to a master, at a later point in time. Snoop
requests and responses travel in the opposite direction, similar to
what happens in functional and atomic accesses. With the introduction
of this patch, it is possible to determine the direction of packets in
the bus, and no longer necessary to look for both a master and a slave
port with the requested port id.
In contrast to the normal recvFunctional, recvAtomic and recvTiming
that are pure virtual functions, the recvFunctionalSnoop,
recvAtomicSnoop and recvTimingSnoop have a default implementation that
calls panic. This is to allow non-coherent master and slave ports to
not implement these functions.
Diffs
-----
src/arch/x86/pagetable_walker.hh 570b44fe6e04
src/arch/x86/pagetable_walker.cc 570b44fe6e04
src/cpu/base.hh 570b44fe6e04
src/cpu/base.cc 570b44fe6e04
src/cpu/inorder/cpu.hh 570b44fe6e04
src/cpu/inorder/cpu.cc 570b44fe6e04
src/cpu/o3/cpu.hh 570b44fe6e04
src/cpu/o3/cpu.cc 570b44fe6e04
src/cpu/o3/lsq.hh 570b44fe6e04
src/cpu/o3/lsq_impl.hh 570b44fe6e04
src/cpu/simple/atomic.hh 570b44fe6e04
src/cpu/simple/timing.hh 570b44fe6e04
src/cpu/simple/timing.cc 570b44fe6e04
src/cpu/testers/directedtest/RubyDirectedTester.hh 570b44fe6e04
src/cpu/testers/directedtest/RubyDirectedTester.cc 570b44fe6e04
src/cpu/testers/memtest/memtest.hh 570b44fe6e04
src/cpu/testers/memtest/memtest.cc 570b44fe6e04
src/cpu/testers/networktest/networktest.hh 570b44fe6e04
src/cpu/testers/networktest/networktest.cc 570b44fe6e04
src/cpu/testers/rubytest/RubyTester.hh 570b44fe6e04
src/cpu/testers/rubytest/RubyTester.cc 570b44fe6e04
src/dev/io_device.hh 570b44fe6e04
src/dev/io_device.cc 570b44fe6e04
src/mem/bridge.hh 570b44fe6e04
src/mem/bridge.cc 570b44fe6e04
src/mem/bus.hh 570b44fe6e04
src/mem/bus.cc 570b44fe6e04
src/mem/cache/base.hh 570b44fe6e04
src/mem/cache/cache.hh 570b44fe6e04
src/mem/cache/cache_impl.hh 570b44fe6e04
src/mem/mport.hh 570b44fe6e04
src/mem/mport.cc 570b44fe6e04
src/mem/packet_queue.hh 570b44fe6e04
src/mem/packet_queue.cc 570b44fe6e04
src/mem/port.hh 570b44fe6e04
src/mem/port.cc 570b44fe6e04
src/mem/ruby/system/RubyPort.hh 570b44fe6e04
src/mem/ruby/system/RubyPort.cc 570b44fe6e04
src/sim/system.hh 570b44fe6e04
Diff: http://reviews.gem5.org/r/1118/diff/
Testing
-------
util/regress all passing (disregarding t1000 and eio)
Thanks,
Andreas Hansson
Andreas Hansson
2012-04-05 16:46:09 UTC
Permalink
Post by Andreas Hansson
src/mem/ruby/system/RubyPort.cc, line 708
<http://reviews.gem5.org/r/1118/diff/2/?file=25363#file25363line708>
Andreas, this packet is sent to invalidate loads in the LSQ. This packet was earlier handled in the LSQ's recvTiming() function. Since that code for snooping is being moved to recvTimingSnoop(), I think this code should change as well.
It seems this is not used in any regression...so I simply did not catch it. Fixed now and a new diff to come in a min or two.


- Andreas


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/1118/#review2448
-----------------------------------------------------------
Post by Andreas Hansson
-----------------------------------------------------------
http://reviews.gem5.org/r/1118/
-----------------------------------------------------------
(Updated April 3, 2012, 4:21 p.m.)
Review request for Default.
Description
-------
MEM: Separate snoops and normal memory requests/responses
This patch introduces port access methods that separates snoop
request/responses from normal memory request/responses. The
differentiation is made for functional, atomic and timing accesses and
builds on the introduction of master and slave ports.
Before the introduction of this patch, the packets belonging to the
different phases of the protocol (request -> [forwarded snoop request
-> snoop response]* -> response) all use the same port access
functions, even though the snoop packets flow in the opposite
direction to the normal packet. That is, a coherent master sends
normal request and receives responses, but receives snoop requests and
sends snoop responses (vice versa for the slave). These two distinct
phases now use different access functions, as described below.
Starting with the functional access, a master sends a request to a
slave through sendFunctional, and the request packet is turned into a
response before the call returns. In a system without cache coherence,
this is all that is needed from the functional interface. For the
cache-coherent scenario, a slave also sends snoop requests to coherent
masters through sendFunctionalSnoop, with responses returned within
the same packet pointer. This is currently used by the bus and caches,
and the LSQ of the O3 CPU. The send/recvFunctional and
send/recvFunctionalSnoop are moved from the Port super class to the
appropriate subclass.
Atomic accesses follow the same flow as functional accesses, with
request being sent from master to slave through sendAtomic. In the
case of cache-coherent ports, a slave can send snoop requests to a
master through sendAtomicSnoop. Just as for the functional access
methods, the atomic send and receive member functions are moved to the
appropriate subclasses.
The timing access methods are different from the functional and atomic
in that requests and responses are separated in time and
send/recvTiming are used for both directions. Hence, a master uses
sendTiming to send a request to a slave, and a slave uses sendTiming
to send a response back to a master, at a later point in time. Snoop
requests and responses travel in the opposite direction, similar to
what happens in functional and atomic accesses. With the introduction
of this patch, it is possible to determine the direction of packets in
the bus, and no longer necessary to look for both a master and a slave
port with the requested port id.
In contrast to the normal recvFunctional, recvAtomic and recvTiming
that are pure virtual functions, the recvFunctionalSnoop,
recvAtomicSnoop and recvTimingSnoop have a default implementation that
calls panic. This is to allow non-coherent master and slave ports to
not implement these functions.
Diffs
-----
src/arch/x86/pagetable_walker.hh 570b44fe6e04
src/arch/x86/pagetable_walker.cc 570b44fe6e04
src/cpu/base.hh 570b44fe6e04
src/cpu/base.cc 570b44fe6e04
src/cpu/inorder/cpu.hh 570b44fe6e04
src/cpu/inorder/cpu.cc 570b44fe6e04
src/cpu/o3/cpu.hh 570b44fe6e04
src/cpu/o3/cpu.cc 570b44fe6e04
src/cpu/o3/lsq.hh 570b44fe6e04
src/cpu/o3/lsq_impl.hh 570b44fe6e04
src/cpu/simple/atomic.hh 570b44fe6e04
src/cpu/simple/timing.hh 570b44fe6e04
src/cpu/simple/timing.cc 570b44fe6e04
src/cpu/testers/directedtest/RubyDirectedTester.hh 570b44fe6e04
src/cpu/testers/directedtest/RubyDirectedTester.cc 570b44fe6e04
src/cpu/testers/memtest/memtest.hh 570b44fe6e04
src/cpu/testers/memtest/memtest.cc 570b44fe6e04
src/cpu/testers/networktest/networktest.hh 570b44fe6e04
src/cpu/testers/networktest/networktest.cc 570b44fe6e04
src/cpu/testers/rubytest/RubyTester.hh 570b44fe6e04
src/cpu/testers/rubytest/RubyTester.cc 570b44fe6e04
src/dev/io_device.hh 570b44fe6e04
src/dev/io_device.cc 570b44fe6e04
src/mem/bridge.hh 570b44fe6e04
src/mem/bridge.cc 570b44fe6e04
src/mem/bus.hh 570b44fe6e04
src/mem/bus.cc 570b44fe6e04
src/mem/cache/base.hh 570b44fe6e04
src/mem/cache/cache.hh 570b44fe6e04
src/mem/cache/cache_impl.hh 570b44fe6e04
src/mem/mport.hh 570b44fe6e04
src/mem/mport.cc 570b44fe6e04
src/mem/packet_queue.hh 570b44fe6e04
src/mem/packet_queue.cc 570b44fe6e04
src/mem/port.hh 570b44fe6e04
src/mem/port.cc 570b44fe6e04
src/mem/ruby/system/RubyPort.hh 570b44fe6e04
src/mem/ruby/system/RubyPort.cc 570b44fe6e04
src/sim/system.hh 570b44fe6e04
Diff: http://reviews.gem5.org/r/1118/diff/
Testing
-------
util/regress all passing (disregarding t1000 and eio)
Thanks,
Andreas Hansson
Andreas Hansson
2012-04-05 18:06:07 UTC
Permalink
Post by Andreas Hansson
src/mem/ruby/system/RubyPort.cc, line 708
<http://reviews.gem5.org/r/1118/diff/2/?file=25363#file25363line708>
Andreas, this packet is sent to invalidate loads in the LSQ. This packet was earlier handled in the LSQ's recvTiming() function. Since that code for snooping is being moved to recvTimingSnoop(), I think this code should change as well.
It seems this is not used in any regression...so I simply did not catch it. Fixed now and a new diff to come in a min or two.
It would be great if you can give it a bash with your set-up and give it a "ship it" if things are fine.


- Andreas


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/1118/#review2448
-----------------------------------------------------------
Post by Andreas Hansson
-----------------------------------------------------------
http://reviews.gem5.org/r/1118/
-----------------------------------------------------------
(Updated April 5, 2012, 9:57 a.m.)
Review request for Default.
Description
-------
MEM: Separate snoops and normal memory requests/responses
This patch introduces port access methods that separates snoop
request/responses from normal memory request/responses. The
differentiation is made for functional, atomic and timing accesses and
builds on the introduction of master and slave ports.
Before the introduction of this patch, the packets belonging to the
different phases of the protocol (request -> [forwarded snoop request
-> snoop response]* -> response) all use the same port access
functions, even though the snoop packets flow in the opposite
direction to the normal packet. That is, a coherent master sends
normal request and receives responses, but receives snoop requests and
sends snoop responses (vice versa for the slave). These two distinct
phases now use different access functions, as described below.
Starting with the functional access, a master sends a request to a
slave through sendFunctional, and the request packet is turned into a
response before the call returns. In a system without cache coherence,
this is all that is needed from the functional interface. For the
cache-coherent scenario, a slave also sends snoop requests to coherent
masters through sendFunctionalSnoop, with responses returned within
the same packet pointer. This is currently used by the bus and caches,
and the LSQ of the O3 CPU. The send/recvFunctional and
send/recvFunctionalSnoop are moved from the Port super class to the
appropriate subclass.
Atomic accesses follow the same flow as functional accesses, with
request being sent from master to slave through sendAtomic. In the
case of cache-coherent ports, a slave can send snoop requests to a
master through sendAtomicSnoop. Just as for the functional access
methods, the atomic send and receive member functions are moved to the
appropriate subclasses.
The timing access methods are different from the functional and atomic
in that requests and responses are separated in time and
send/recvTiming are used for both directions. Hence, a master uses
sendTiming to send a request to a slave, and a slave uses sendTiming
to send a response back to a master, at a later point in time. Snoop
requests and responses travel in the opposite direction, similar to
what happens in functional and atomic accesses. With the introduction
of this patch, it is possible to determine the direction of packets in
the bus, and no longer necessary to look for both a master and a slave
port with the requested port id.
In contrast to the normal recvFunctional, recvAtomic and recvTiming
that are pure virtual functions, the recvFunctionalSnoop,
recvAtomicSnoop and recvTimingSnoop have a default implementation that
calls panic. This is to allow non-coherent master and slave ports to
not implement these functions.
Diffs
-----
src/arch/x86/pagetable_walker.hh 570b44fe6e04
src/arch/x86/pagetable_walker.cc 570b44fe6e04
src/cpu/base.hh 570b44fe6e04
src/cpu/base.cc 570b44fe6e04
src/cpu/inorder/cpu.hh 570b44fe6e04
src/cpu/inorder/cpu.cc 570b44fe6e04
src/cpu/o3/cpu.hh 570b44fe6e04
src/cpu/o3/cpu.cc 570b44fe6e04
src/cpu/o3/lsq.hh 570b44fe6e04
src/cpu/o3/lsq_impl.hh 570b44fe6e04
src/cpu/simple/atomic.hh 570b44fe6e04
src/cpu/simple/timing.hh 570b44fe6e04
src/cpu/simple/timing.cc 570b44fe6e04
src/cpu/testers/directedtest/RubyDirectedTester.hh 570b44fe6e04
src/cpu/testers/directedtest/RubyDirectedTester.cc 570b44fe6e04
src/cpu/testers/memtest/memtest.hh 570b44fe6e04
src/cpu/testers/memtest/memtest.cc 570b44fe6e04
src/cpu/testers/networktest/networktest.hh 570b44fe6e04
src/cpu/testers/networktest/networktest.cc 570b44fe6e04
src/cpu/testers/rubytest/RubyTester.hh 570b44fe6e04
src/cpu/testers/rubytest/RubyTester.cc 570b44fe6e04
src/dev/io_device.hh 570b44fe6e04
src/dev/io_device.cc 570b44fe6e04
src/mem/bridge.hh 570b44fe6e04
src/mem/bridge.cc 570b44fe6e04
src/mem/bus.hh 570b44fe6e04
src/mem/bus.cc 570b44fe6e04
src/mem/cache/base.hh 570b44fe6e04
src/mem/cache/cache.hh 570b44fe6e04
src/mem/cache/cache_impl.hh 570b44fe6e04
src/mem/mport.hh 570b44fe6e04
src/mem/mport.cc 570b44fe6e04
src/mem/packet_queue.hh 570b44fe6e04
src/mem/packet_queue.cc 570b44fe6e04
src/mem/port.hh 570b44fe6e04
src/mem/port.cc 570b44fe6e04
src/mem/ruby/system/RubyPort.hh 570b44fe6e04
src/mem/ruby/system/RubyPort.cc 570b44fe6e04
src/sim/system.hh 570b44fe6e04
Diff: http://reviews.gem5.org/r/1118/diff/
Testing
-------
util/regress all passing (disregarding t1000 and eio)
Thanks,
Andreas Hansson
Andreas Hansson
2012-04-11 09:00:08 UTC
Permalink
Post by Andreas Hansson
src/mem/ruby/system/RubyPort.cc, line 708
<http://reviews.gem5.org/r/1118/diff/2/?file=25363#file25363line708>
Andreas, this packet is sent to invalidate loads in the LSQ. This packet was earlier handled in the LSQ's recvTiming() function. Since that code for snooping is being moved to recvTimingSnoop(), I think this code should change as well.
It seems this is not used in any regression...so I simply did not catch it. Fixed now and a new diff to come in a min or two.
It would be great if you can give it a bash with your set-up and give it a "ship it" if things are fine.
Hi Nilay,

Any chance you managed to try this out?


- Andreas


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/1118/#review2448
-----------------------------------------------------------
Post by Andreas Hansson
-----------------------------------------------------------
http://reviews.gem5.org/r/1118/
-----------------------------------------------------------
(Updated April 10, 2012, 11:25 a.m.)
Review request for Default.
Description
-------
MEM: Separate snoops and normal memory requests/responses
This patch introduces port access methods that separates snoop
request/responses from normal memory request/responses. The
differentiation is made for functional, atomic and timing accesses and
builds on the introduction of master and slave ports.
Before the introduction of this patch, the packets belonging to the
different phases of the protocol (request -> [forwarded snoop request
-> snoop response]* -> response) all use the same port access
functions, even though the snoop packets flow in the opposite
direction to the normal packet. That is, a coherent master sends
normal request and receives responses, but receives snoop requests and
sends snoop responses (vice versa for the slave). These two distinct
phases now use different access functions, as described below.
Starting with the functional access, a master sends a request to a
slave through sendFunctional, and the request packet is turned into a
response before the call returns. In a system without cache coherence,
this is all that is needed from the functional interface. For the
cache-coherent scenario, a slave also sends snoop requests to coherent
masters through sendFunctionalSnoop, with responses returned within
the same packet pointer. This is currently used by the bus and caches,
and the LSQ of the O3 CPU. The send/recvFunctional and
send/recvFunctionalSnoop are moved from the Port super class to the
appropriate subclass.
Atomic accesses follow the same flow as functional accesses, with
request being sent from master to slave through sendAtomic. In the
case of cache-coherent ports, a slave can send snoop requests to a
master through sendAtomicSnoop. Just as for the functional access
methods, the atomic send and receive member functions are moved to the
appropriate subclasses.
The timing access methods are different from the functional and atomic
in that requests and responses are separated in time and
send/recvTiming are used for both directions. Hence, a master uses
sendTiming to send a request to a slave, and a slave uses sendTiming
to send a response back to a master, at a later point in time. Snoop
requests and responses travel in the opposite direction, similar to
what happens in functional and atomic accesses. With the introduction
of this patch, it is possible to determine the direction of packets in
the bus, and no longer necessary to look for both a master and a slave
port with the requested port id.
In contrast to the normal recvFunctional, recvAtomic and recvTiming
that are pure virtual functions, the recvFunctionalSnoop,
recvAtomicSnoop and recvTimingSnoop have a default implementation that
calls panic. This is to allow non-coherent master and slave ports to
not implement these functions.
Diffs
-----
src/arch/x86/pagetable_walker.hh 5534a564f6a0
src/arch/x86/pagetable_walker.cc 5534a564f6a0
src/cpu/base.hh 5534a564f6a0
src/cpu/base.cc 5534a564f6a0
src/cpu/inorder/cpu.hh 5534a564f6a0
src/cpu/inorder/cpu.cc 5534a564f6a0
src/cpu/o3/cpu.hh 5534a564f6a0
src/cpu/o3/cpu.cc 5534a564f6a0
src/cpu/o3/lsq.hh 5534a564f6a0
src/cpu/o3/lsq_impl.hh 5534a564f6a0
src/cpu/simple/atomic.hh 5534a564f6a0
src/cpu/simple/timing.hh 5534a564f6a0
src/cpu/simple/timing.cc 5534a564f6a0
src/cpu/testers/directedtest/RubyDirectedTester.hh 5534a564f6a0
src/cpu/testers/directedtest/RubyDirectedTester.cc 5534a564f6a0
src/cpu/testers/memtest/memtest.hh 5534a564f6a0
src/cpu/testers/memtest/memtest.cc 5534a564f6a0
src/cpu/testers/networktest/networktest.hh 5534a564f6a0
src/cpu/testers/networktest/networktest.cc 5534a564f6a0
src/cpu/testers/rubytest/RubyTester.hh 5534a564f6a0
src/cpu/testers/rubytest/RubyTester.cc 5534a564f6a0
src/dev/io_device.hh 5534a564f6a0
src/dev/io_device.cc 5534a564f6a0
src/mem/bridge.hh 5534a564f6a0
src/mem/bridge.cc 5534a564f6a0
src/mem/bus.hh 5534a564f6a0
src/mem/bus.cc 5534a564f6a0
src/mem/cache/base.hh 5534a564f6a0
src/mem/cache/cache.hh 5534a564f6a0
src/mem/cache/cache_impl.hh 5534a564f6a0
src/mem/mport.hh 5534a564f6a0
src/mem/mport.cc 5534a564f6a0
src/mem/packet_queue.hh 5534a564f6a0
src/mem/packet_queue.cc 5534a564f6a0
src/mem/port.hh 5534a564f6a0
src/mem/port.cc 5534a564f6a0
src/mem/ruby/system/RubyPort.hh 5534a564f6a0
src/mem/ruby/system/RubyPort.cc 5534a564f6a0
src/sim/system.hh 5534a564f6a0
Diff: http://reviews.gem5.org/r/1118/diff/
Testing
-------
util/regress all passing (disregarding t1000 and eio)
Thanks,
Andreas Hansson
Nilay Vaish
2012-04-11 09:20:38 UTC
Permalink
Post by Andreas Hansson
src/mem/ruby/system/RubyPort.cc, line 708
<http://reviews.gem5.org/r/1118/diff/2/?file=25363#file25363line708>
Andreas, this packet is sent to invalidate loads in the LSQ. This packet was earlier handled in the LSQ's recvTiming() function. Since that code for snooping is being moved to recvTimingSnoop(), I think this code should change as well.
It seems this is not used in any regression...so I simply did not catch it. Fixed now and a new diff to come in a min or two.
It would be great if you can give it a bash with your set-up and give it a "ship it" if things are fine.
Hi Nilay,
Any chance you managed to try this out?
Nope.


- Nilay


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/1118/#review2448
-----------------------------------------------------------
Post by Andreas Hansson
-----------------------------------------------------------
http://reviews.gem5.org/r/1118/
-----------------------------------------------------------
(Updated April 10, 2012, 11:25 a.m.)
Review request for Default.
Description
-------
MEM: Separate snoops and normal memory requests/responses
This patch introduces port access methods that separates snoop
request/responses from normal memory request/responses. The
differentiation is made for functional, atomic and timing accesses and
builds on the introduction of master and slave ports.
Before the introduction of this patch, the packets belonging to the
different phases of the protocol (request -> [forwarded snoop request
-> snoop response]* -> response) all use the same port access
functions, even though the snoop packets flow in the opposite
direction to the normal packet. That is, a coherent master sends
normal request and receives responses, but receives snoop requests and
sends snoop responses (vice versa for the slave). These two distinct
phases now use different access functions, as described below.
Starting with the functional access, a master sends a request to a
slave through sendFunctional, and the request packet is turned into a
response before the call returns. In a system without cache coherence,
this is all that is needed from the functional interface. For the
cache-coherent scenario, a slave also sends snoop requests to coherent
masters through sendFunctionalSnoop, with responses returned within
the same packet pointer. This is currently used by the bus and caches,
and the LSQ of the O3 CPU. The send/recvFunctional and
send/recvFunctionalSnoop are moved from the Port super class to the
appropriate subclass.
Atomic accesses follow the same flow as functional accesses, with
request being sent from master to slave through sendAtomic. In the
case of cache-coherent ports, a slave can send snoop requests to a
master through sendAtomicSnoop. Just as for the functional access
methods, the atomic send and receive member functions are moved to the
appropriate subclasses.
The timing access methods are different from the functional and atomic
in that requests and responses are separated in time and
send/recvTiming are used for both directions. Hence, a master uses
sendTiming to send a request to a slave, and a slave uses sendTiming
to send a response back to a master, at a later point in time. Snoop
requests and responses travel in the opposite direction, similar to
what happens in functional and atomic accesses. With the introduction
of this patch, it is possible to determine the direction of packets in
the bus, and no longer necessary to look for both a master and a slave
port with the requested port id.
In contrast to the normal recvFunctional, recvAtomic and recvTiming
that are pure virtual functions, the recvFunctionalSnoop,
recvAtomicSnoop and recvTimingSnoop have a default implementation that
calls panic. This is to allow non-coherent master and slave ports to
not implement these functions.
Diffs
-----
src/arch/x86/pagetable_walker.hh 5534a564f6a0
src/arch/x86/pagetable_walker.cc 5534a564f6a0
src/cpu/base.hh 5534a564f6a0
src/cpu/base.cc 5534a564f6a0
src/cpu/inorder/cpu.hh 5534a564f6a0
src/cpu/inorder/cpu.cc 5534a564f6a0
src/cpu/o3/cpu.hh 5534a564f6a0
src/cpu/o3/cpu.cc 5534a564f6a0
src/cpu/o3/lsq.hh 5534a564f6a0
src/cpu/o3/lsq_impl.hh 5534a564f6a0
src/cpu/simple/atomic.hh 5534a564f6a0
src/cpu/simple/timing.hh 5534a564f6a0
src/cpu/simple/timing.cc 5534a564f6a0
src/cpu/testers/directedtest/RubyDirectedTester.hh 5534a564f6a0
src/cpu/testers/directedtest/RubyDirectedTester.cc 5534a564f6a0
src/cpu/testers/memtest/memtest.hh 5534a564f6a0
src/cpu/testers/memtest/memtest.cc 5534a564f6a0
src/cpu/testers/networktest/networktest.hh 5534a564f6a0
src/cpu/testers/networktest/networktest.cc 5534a564f6a0
src/cpu/testers/rubytest/RubyTester.hh 5534a564f6a0
src/cpu/testers/rubytest/RubyTester.cc 5534a564f6a0
src/dev/io_device.hh 5534a564f6a0
src/dev/io_device.cc 5534a564f6a0
src/mem/bridge.hh 5534a564f6a0
src/mem/bridge.cc 5534a564f6a0
src/mem/bus.hh 5534a564f6a0
src/mem/bus.cc 5534a564f6a0
src/mem/cache/base.hh 5534a564f6a0
src/mem/cache/cache.hh 5534a564f6a0
src/mem/cache/cache_impl.hh 5534a564f6a0
src/mem/mport.hh 5534a564f6a0
src/mem/mport.cc 5534a564f6a0
src/mem/packet_queue.hh 5534a564f6a0
src/mem/packet_queue.cc 5534a564f6a0
src/mem/port.hh 5534a564f6a0
src/mem/port.cc 5534a564f6a0
src/mem/ruby/system/RubyPort.hh 5534a564f6a0
src/mem/ruby/system/RubyPort.cc 5534a564f6a0
src/sim/system.hh 5534a564f6a0
Diff: http://reviews.gem5.org/r/1118/diff/
Testing
-------
util/regress all passing (disregarding t1000 and eio)
Thanks,
Andreas Hansson
Andreas Hansson
2012-04-11 10:43:14 UTC
Permalink
Post by Andreas Hansson
src/mem/ruby/system/RubyPort.cc, line 708
<http://reviews.gem5.org/r/1118/diff/2/?file=25363#file25363line708>
Andreas, this packet is sent to invalidate loads in the LSQ. This packet was earlier handled in the LSQ's recvTiming() function. Since that code for snooping is being moved to recvTimingSnoop(), I think this code should change as well.
It seems this is not used in any regression...so I simply did not catch it. Fixed now and a new diff to come in a min or two.
It would be great if you can give it a bash with your set-up and give it a "ship it" if things are fine.
Hi Nilay,
Any chance you managed to try this out?
Nope.
Is there I test I can run that will exercise this piece of code?


- Andreas


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/1118/#review2448
-----------------------------------------------------------
Post by Andreas Hansson
-----------------------------------------------------------
http://reviews.gem5.org/r/1118/
-----------------------------------------------------------
(Updated April 10, 2012, 11:25 a.m.)
Review request for Default.
Description
-------
MEM: Separate snoops and normal memory requests/responses
This patch introduces port access methods that separates snoop
request/responses from normal memory request/responses. The
differentiation is made for functional, atomic and timing accesses and
builds on the introduction of master and slave ports.
Before the introduction of this patch, the packets belonging to the
different phases of the protocol (request -> [forwarded snoop request
-> snoop response]* -> response) all use the same port access
functions, even though the snoop packets flow in the opposite
direction to the normal packet. That is, a coherent master sends
normal request and receives responses, but receives snoop requests and
sends snoop responses (vice versa for the slave). These two distinct
phases now use different access functions, as described below.
Starting with the functional access, a master sends a request to a
slave through sendFunctional, and the request packet is turned into a
response before the call returns. In a system without cache coherence,
this is all that is needed from the functional interface. For the
cache-coherent scenario, a slave also sends snoop requests to coherent
masters through sendFunctionalSnoop, with responses returned within
the same packet pointer. This is currently used by the bus and caches,
and the LSQ of the O3 CPU. The send/recvFunctional and
send/recvFunctionalSnoop are moved from the Port super class to the
appropriate subclass.
Atomic accesses follow the same flow as functional accesses, with
request being sent from master to slave through sendAtomic. In the
case of cache-coherent ports, a slave can send snoop requests to a
master through sendAtomicSnoop. Just as for the functional access
methods, the atomic send and receive member functions are moved to the
appropriate subclasses.
The timing access methods are different from the functional and atomic
in that requests and responses are separated in time and
send/recvTiming are used for both directions. Hence, a master uses
sendTiming to send a request to a slave, and a slave uses sendTiming
to send a response back to a master, at a later point in time. Snoop
requests and responses travel in the opposite direction, similar to
what happens in functional and atomic accesses. With the introduction
of this patch, it is possible to determine the direction of packets in
the bus, and no longer necessary to look for both a master and a slave
port with the requested port id.
In contrast to the normal recvFunctional, recvAtomic and recvTiming
that are pure virtual functions, the recvFunctionalSnoop,
recvAtomicSnoop and recvTimingSnoop have a default implementation that
calls panic. This is to allow non-coherent master and slave ports to
not implement these functions.
Diffs
-----
src/arch/x86/pagetable_walker.hh 5534a564f6a0
src/arch/x86/pagetable_walker.cc 5534a564f6a0
src/cpu/base.hh 5534a564f6a0
src/cpu/base.cc 5534a564f6a0
src/cpu/inorder/cpu.hh 5534a564f6a0
src/cpu/inorder/cpu.cc 5534a564f6a0
src/cpu/o3/cpu.hh 5534a564f6a0
src/cpu/o3/cpu.cc 5534a564f6a0
src/cpu/o3/lsq.hh 5534a564f6a0
src/cpu/o3/lsq_impl.hh 5534a564f6a0
src/cpu/simple/atomic.hh 5534a564f6a0
src/cpu/simple/timing.hh 5534a564f6a0
src/cpu/simple/timing.cc 5534a564f6a0
src/cpu/testers/directedtest/RubyDirectedTester.hh 5534a564f6a0
src/cpu/testers/directedtest/RubyDirectedTester.cc 5534a564f6a0
src/cpu/testers/memtest/memtest.hh 5534a564f6a0
src/cpu/testers/memtest/memtest.cc 5534a564f6a0
src/cpu/testers/networktest/networktest.hh 5534a564f6a0
src/cpu/testers/networktest/networktest.cc 5534a564f6a0
src/cpu/testers/rubytest/RubyTester.hh 5534a564f6a0
src/cpu/testers/rubytest/RubyTester.cc 5534a564f6a0
src/dev/io_device.hh 5534a564f6a0
src/dev/io_device.cc 5534a564f6a0
src/mem/bridge.hh 5534a564f6a0
src/mem/bridge.cc 5534a564f6a0
src/mem/bus.hh 5534a564f6a0
src/mem/bus.cc 5534a564f6a0
src/mem/cache/base.hh 5534a564f6a0
src/mem/cache/cache.hh 5534a564f6a0
src/mem/cache/cache_impl.hh 5534a564f6a0
src/mem/mport.hh 5534a564f6a0
src/mem/mport.cc 5534a564f6a0
src/mem/packet_queue.hh 5534a564f6a0
src/mem/packet_queue.cc 5534a564f6a0
src/mem/port.hh 5534a564f6a0
src/mem/port.cc 5534a564f6a0
src/mem/ruby/system/RubyPort.hh 5534a564f6a0
src/mem/ruby/system/RubyPort.cc 5534a564f6a0
src/sim/system.hh 5534a564f6a0
Diff: http://reviews.gem5.org/r/1118/diff/
Testing
-------
util/regress all passing (disregarding t1000 and eio)
Thanks,
Andreas Hansson
Nilay Vaish
2012-04-11 23:51:00 UTC
Permalink
Post by Andreas Hansson
src/mem/ruby/system/RubyPort.cc, line 708
<http://reviews.gem5.org/r/1118/diff/2/?file=25363#file25363line708>
Andreas, this packet is sent to invalidate loads in the LSQ. This packet was earlier handled in the LSQ's recvTiming() function. Since that code for snooping is being moved to recvTimingSnoop(), I think this code should change as well.
It seems this is not used in any regression...so I simply did not catch it. Fixed now and a new diff to come in a min or two.
It would be great if you can give it a bash with your set-up and give it a "ship it" if things are fine.
Hi Nilay,
Any chance you managed to try this out?
Nope.
Is there I test I can run that will exercise this piece of code?
There is none, I don't think any one uses ruby and o3 cpu currently.
I am planning to commit a full system regression test for o3 cpu,
ruby and x86 combination. This will have to wait since I am struggling
with some error that I see while booting. It seems, I might be
speaking too early, that SE/FS merge broke some thing some where.
It will take some time (2-3 days) before I can be definitive on this
issue. I will run your patch after that.


- Nilay


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/1118/#review2448
-----------------------------------------------------------
Post by Andreas Hansson
-----------------------------------------------------------
http://reviews.gem5.org/r/1118/
-----------------------------------------------------------
(Updated April 10, 2012, 11:25 a.m.)
Review request for Default.
Description
-------
MEM: Separate snoops and normal memory requests/responses
This patch introduces port access methods that separates snoop
request/responses from normal memory request/responses. The
differentiation is made for functional, atomic and timing accesses and
builds on the introduction of master and slave ports.
Before the introduction of this patch, the packets belonging to the
different phases of the protocol (request -> [forwarded snoop request
-> snoop response]* -> response) all use the same port access
functions, even though the snoop packets flow in the opposite
direction to the normal packet. That is, a coherent master sends
normal request and receives responses, but receives snoop requests and
sends snoop responses (vice versa for the slave). These two distinct
phases now use different access functions, as described below.
Starting with the functional access, a master sends a request to a
slave through sendFunctional, and the request packet is turned into a
response before the call returns. In a system without cache coherence,
this is all that is needed from the functional interface. For the
cache-coherent scenario, a slave also sends snoop requests to coherent
masters through sendFunctionalSnoop, with responses returned within
the same packet pointer. This is currently used by the bus and caches,
and the LSQ of the O3 CPU. The send/recvFunctional and
send/recvFunctionalSnoop are moved from the Port super class to the
appropriate subclass.
Atomic accesses follow the same flow as functional accesses, with
request being sent from master to slave through sendAtomic. In the
case of cache-coherent ports, a slave can send snoop requests to a
master through sendAtomicSnoop. Just as for the functional access
methods, the atomic send and receive member functions are moved to the
appropriate subclasses.
The timing access methods are different from the functional and atomic
in that requests and responses are separated in time and
send/recvTiming are used for both directions. Hence, a master uses
sendTiming to send a request to a slave, and a slave uses sendTiming
to send a response back to a master, at a later point in time. Snoop
requests and responses travel in the opposite direction, similar to
what happens in functional and atomic accesses. With the introduction
of this patch, it is possible to determine the direction of packets in
the bus, and no longer necessary to look for both a master and a slave
port with the requested port id.
In contrast to the normal recvFunctional, recvAtomic and recvTiming
that are pure virtual functions, the recvFunctionalSnoop,
recvAtomicSnoop and recvTimingSnoop have a default implementation that
calls panic. This is to allow non-coherent master and slave ports to
not implement these functions.
Diffs
-----
src/arch/x86/pagetable_walker.hh 5534a564f6a0
src/arch/x86/pagetable_walker.cc 5534a564f6a0
src/cpu/base.hh 5534a564f6a0
src/cpu/base.cc 5534a564f6a0
src/cpu/inorder/cpu.hh 5534a564f6a0
src/cpu/inorder/cpu.cc 5534a564f6a0
src/cpu/o3/cpu.hh 5534a564f6a0
src/cpu/o3/cpu.cc 5534a564f6a0
src/cpu/o3/lsq.hh 5534a564f6a0
src/cpu/o3/lsq_impl.hh 5534a564f6a0
src/cpu/simple/atomic.hh 5534a564f6a0
src/cpu/simple/timing.hh 5534a564f6a0
src/cpu/simple/timing.cc 5534a564f6a0
src/cpu/testers/directedtest/RubyDirectedTester.hh 5534a564f6a0
src/cpu/testers/directedtest/RubyDirectedTester.cc 5534a564f6a0
src/cpu/testers/memtest/memtest.hh 5534a564f6a0
src/cpu/testers/memtest/memtest.cc 5534a564f6a0
src/cpu/testers/networktest/networktest.hh 5534a564f6a0
src/cpu/testers/networktest/networktest.cc 5534a564f6a0
src/cpu/testers/rubytest/RubyTester.hh 5534a564f6a0
src/cpu/testers/rubytest/RubyTester.cc 5534a564f6a0
src/dev/io_device.hh 5534a564f6a0
src/dev/io_device.cc 5534a564f6a0
src/mem/bridge.hh 5534a564f6a0
src/mem/bridge.cc 5534a564f6a0
src/mem/bus.hh 5534a564f6a0
src/mem/bus.cc 5534a564f6a0
src/mem/cache/base.hh 5534a564f6a0
src/mem/cache/cache.hh 5534a564f6a0
src/mem/cache/cache_impl.hh 5534a564f6a0
src/mem/mport.hh 5534a564f6a0
src/mem/mport.cc 5534a564f6a0
src/mem/packet_queue.hh 5534a564f6a0
src/mem/packet_queue.cc 5534a564f6a0
src/mem/port.hh 5534a564f6a0
src/mem/port.cc 5534a564f6a0
src/mem/ruby/system/RubyPort.hh 5534a564f6a0
src/mem/ruby/system/RubyPort.cc 5534a564f6a0
src/sim/system.hh 5534a564f6a0
Diff: http://reviews.gem5.org/r/1118/diff/
Testing
-------
util/regress all passing (disregarding t1000 and eio)
Thanks,
Andreas Hansson
Steve Reinhardt
2012-04-12 00:07:09 UTC
Permalink
Is there an SE-mode O3/ruby/x86 regression we can add?
Post by Nilay Vaish
Post by Andreas Hansson
src/mem/ruby/system/RubyPort.cc, line 708
<http://reviews.gem5.org/r/1118/diff/2/?file=25363#file25363line708>
Andreas, this packet is sent to invalidate loads in the LSQ. This
packet was earlier handled in the LSQ's recvTiming() function. Since that
code for snooping is being moved to recvTimingSnoop(), I think this code
should change as well.
Post by Andreas Hansson
It seems this is not used in any regression...so I simply did not
catch it. Fixed now and a new diff to come in a min or two.
Post by Andreas Hansson
It would be great if you can give it a bash with your set-up and
give it a "ship it" if things are fine.
Post by Andreas Hansson
Hi Nilay,
Any chance you managed to try this out?
Nope.
Is there I test I can run that will exercise this piece of code?
There is none, I don't think any one uses ruby and o3 cpu currently.
I am planning to commit a full system regression test for o3 cpu,
ruby and x86 combination. This will have to wait since I am struggling
with some error that I see while booting. It seems, I might be
speaking too early, that SE/FS merge broke some thing some where.
It will take some time (2-3 days) before I can be definitive on this
issue. I will run your patch after that.
- Nilay
-----------------------------------------------------------
http://reviews.gem5.org/r/1118/#review2448
-----------------------------------------------------------
Post by Andreas Hansson
-----------------------------------------------------------
http://reviews.gem5.org/r/1118/
-----------------------------------------------------------
(Updated April 10, 2012, 11:25 a.m.)
Review request for Default.
Description
-------
MEM: Separate snoops and normal memory requests/responses
This patch introduces port access methods that separates snoop
request/responses from normal memory request/responses. The
differentiation is made for functional, atomic and timing accesses and
builds on the introduction of master and slave ports.
Before the introduction of this patch, the packets belonging to the
different phases of the protocol (request -> [forwarded snoop request
-> snoop response]* -> response) all use the same port access
functions, even though the snoop packets flow in the opposite
direction to the normal packet. That is, a coherent master sends
normal request and receives responses, but receives snoop requests and
sends snoop responses (vice versa for the slave). These two distinct
phases now use different access functions, as described below.
Starting with the functional access, a master sends a request to a
slave through sendFunctional, and the request packet is turned into a
response before the call returns. In a system without cache coherence,
this is all that is needed from the functional interface. For the
cache-coherent scenario, a slave also sends snoop requests to coherent
masters through sendFunctionalSnoop, with responses returned within
the same packet pointer. This is currently used by the bus and caches,
and the LSQ of the O3 CPU. The send/recvFunctional and
send/recvFunctionalSnoop are moved from the Port super class to the
appropriate subclass.
Atomic accesses follow the same flow as functional accesses, with
request being sent from master to slave through sendAtomic. In the
case of cache-coherent ports, a slave can send snoop requests to a
master through sendAtomicSnoop. Just as for the functional access
methods, the atomic send and receive member functions are moved to the
appropriate subclasses.
The timing access methods are different from the functional and atomic
in that requests and responses are separated in time and
send/recvTiming are used for both directions. Hence, a master uses
sendTiming to send a request to a slave, and a slave uses sendTiming
to send a response back to a master, at a later point in time. Snoop
requests and responses travel in the opposite direction, similar to
what happens in functional and atomic accesses. With the introduction
of this patch, it is possible to determine the direction of packets in
the bus, and no longer necessary to look for both a master and a slave
port with the requested port id.
In contrast to the normal recvFunctional, recvAtomic and recvTiming
that are pure virtual functions, the recvFunctionalSnoop,
recvAtomicSnoop and recvTimingSnoop have a default implementation that
calls panic. This is to allow non-coherent master and slave ports to
not implement these functions.
Diffs
-----
src/arch/x86/pagetable_walker.hh 5534a564f6a0
src/arch/x86/pagetable_walker.cc 5534a564f6a0
src/cpu/base.hh 5534a564f6a0
src/cpu/base.cc 5534a564f6a0
src/cpu/inorder/cpu.hh 5534a564f6a0
src/cpu/inorder/cpu.cc 5534a564f6a0
src/cpu/o3/cpu.hh 5534a564f6a0
src/cpu/o3/cpu.cc 5534a564f6a0
src/cpu/o3/lsq.hh 5534a564f6a0
src/cpu/o3/lsq_impl.hh 5534a564f6a0
src/cpu/simple/atomic.hh 5534a564f6a0
src/cpu/simple/timing.hh 5534a564f6a0
src/cpu/simple/timing.cc 5534a564f6a0
src/cpu/testers/directedtest/RubyDirectedTester.hh 5534a564f6a0
src/cpu/testers/directedtest/RubyDirectedTester.cc 5534a564f6a0
src/cpu/testers/memtest/memtest.hh 5534a564f6a0
src/cpu/testers/memtest/memtest.cc 5534a564f6a0
src/cpu/testers/networktest/networktest.hh 5534a564f6a0
src/cpu/testers/networktest/networktest.cc 5534a564f6a0
src/cpu/testers/rubytest/RubyTester.hh 5534a564f6a0
src/cpu/testers/rubytest/RubyTester.cc 5534a564f6a0
src/dev/io_device.hh 5534a564f6a0
src/dev/io_device.cc 5534a564f6a0
src/mem/bridge.hh 5534a564f6a0
src/mem/bridge.cc 5534a564f6a0
src/mem/bus.hh 5534a564f6a0
src/mem/bus.cc 5534a564f6a0
src/mem/cache/base.hh 5534a564f6a0
src/mem/cache/cache.hh 5534a564f6a0
src/mem/cache/cache_impl.hh 5534a564f6a0
src/mem/mport.hh 5534a564f6a0
src/mem/mport.cc 5534a564f6a0
src/mem/packet_queue.hh 5534a564f6a0
src/mem/packet_queue.cc 5534a564f6a0
src/mem/port.hh 5534a564f6a0
src/mem/port.cc 5534a564f6a0
src/mem/ruby/system/RubyPort.hh 5534a564f6a0
src/mem/ruby/system/RubyPort.cc 5534a564f6a0
src/sim/system.hh 5534a564f6a0
Diff: http://reviews.gem5.org/r/1118/diff/
Testing
-------
util/regress all passing (disregarding t1000 and eio)
Thanks,
Andreas Hansson
_______________________________________________
gem5-dev mailing list
http://m5sim.org/mailman/listinfo/gem5-dev
Andreas Hansson
2012-04-12 07:27:08 UTC
Permalink
A regression like that would eventually be useful, but right now I would be most appreciative if http://reviews.gem5.org/r/1118/ (and the pile of patches depending on it) was not in the critical path, and it also doesn't change any behaviour with respect to Ruby, it merely adds the snoop/non-snoop separation.

Thanks,

Andreas

From: Steve Reinhardt [mailto:stever-***@public.gmane.org]
Sent: 12 April 2012 01:07
To: gem5 Developer List
Cc: Nilay Vaish; Andreas Hansson
Subject: Re: [gem5-dev] Review Request: MEM: Separate snoops and normal memory requests/responses

Is there an SE-mode O3/ruby/x86 regression we can add?
Post by Andreas Hansson
src/mem/ruby/system/RubyPort.cc, line 708
<http://reviews.gem5.org/r/1118/diff/2/?file=25363#file25363line708>
Andreas, this packet is sent to invalidate loads in the LSQ. This packet was earlier handled in the LSQ's recvTiming() function. Since that code for snooping is being moved to recvTimingSnoop(), I think this code should change as well.
It seems this is not used in any regression...so I simply did not catch it. Fixed now and a new diff to come in a min or two.
It would be great if you can give it a bash with your set-up and give it a "ship it" if things are fine.
Hi Nilay,
Any chance you managed to try this out?
Nope.
Is there I test I can run that will exercise this piece of code?
There is none, I don't think any one uses ruby and o3 cpu currently.
I am planning to commit a full system regression test for o3 cpu,
ruby and x86 combination. This will have to wait since I am struggling
with some error that I see while booting. It seems, I might be
speaking too early, that SE/FS merge broke some thing some where.
It will take some time (2-3 days) before I can be definitive on this
issue. I will run your patch after that.


- Nilay


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/1118/#review2448
-----------------------------------------------------------
Post by Andreas Hansson
-----------------------------------------------------------
http://reviews.gem5.org/r/1118/
-----------------------------------------------------------
(Updated April 10, 2012, 11:25 a.m.)
Review request for Default.
Description
-------
MEM: Separate snoops and normal memory requests/responses
This patch introduces port access methods that separates snoop
request/responses from normal memory request/responses. The
differentiation is made for functional, atomic and timing accesses and
builds on the introduction of master and slave ports.
Before the introduction of this patch, the packets belonging to the
different phases of the protocol (request -> [forwarded snoop request
-> snoop response]* -> response) all use the same port access
functions, even though the snoop packets flow in the opposite
direction to the normal packet. That is, a coherent master sends
normal request and receives responses, but receives snoop requests and
sends snoop responses (vice versa for the slave). These two distinct
phases now use different access functions, as described below.
Starting with the functional access, a master sends a request to a
slave through sendFunctional, and the request packet is turned into a
response before the call returns. In a system without cache coherence,
this is all that is needed from the functional interface. For the
cache-coherent scenario, a slave also sends snoop requests to coherent
masters through sendFunctionalSnoop, with responses returned within
the same packet pointer. This is currently used by the bus and caches,
and the LSQ of the O3 CPU. The send/recvFunctional and
send/recvFunctionalSnoop are moved from the Port super class to the
appropriate subclass.
Atomic accesses follow the same flow as functional accesses, with
request being sent from master to slave through sendAtomic. In the
case of cache-coherent ports, a slave can send snoop requests to a
master through sendAtomicSnoop. Just as for the functional access
methods, the atomic send and receive member functions are moved to the
appropriate subclasses.
The timing access methods are different from the functional and atomic
in that requests and responses are separated in time and
send/recvTiming are used for both directions. Hence, a master uses
sendTiming to send a request to a slave, and a slave uses sendTiming
to send a response back to a master, at a later point in time. Snoop
requests and responses travel in the opposite direction, similar to
what happens in functional and atomic accesses. With the introduction
of this patch, it is possible to determine the direction of packets in
the bus, and no longer necessary to look for both a master and a slave
port with the requested port id.
In contrast to the normal recvFunctional, recvAtomic and recvTiming
that are pure virtual functions, the recvFunctionalSnoop,
recvAtomicSnoop and recvTimingSnoop have a default implementation that
calls panic. This is to allow non-coherent master and slave ports to
not implement these functions.
Diffs
-----
src/arch/x86/pagetable_walker.hh 5534a564f6a0
src/arch/x86/pagetable_walker.cc 5534a564f6a0
src/cpu/base.hh 5534a564f6a0
src/cpu/base.cc 5534a564f6a0
src/cpu/inorder/cpu.hh 5534a564f6a0
src/cpu/inorder/cpu.cc 5534a564f6a0
src/cpu/o3/cpu.hh 5534a564f6a0
src/cpu/o3/cpu.cc 5534a564f6a0
src/cpu/o3/lsq.hh 5534a564f6a0
src/cpu/o3/lsq_impl.hh 5534a564f6a0
src/cpu/simple/atomic.hh 5534a564f6a0
src/cpu/simple/timing.hh 5534a564f6a0
src/cpu/simple/timing.cc 5534a564f6a0
src/cpu/testers/directedtest/RubyDirectedTester.hh 5534a564f6a0
src/cpu/testers/directedtest/RubyDirectedTester.cc 5534a564f6a0
src/cpu/testers/memtest/memtest.hh 5534a564f6a0
src/cpu/testers/memtest/memtest.cc 5534a564f6a0
src/cpu/testers/networktest/networktest.hh 5534a564f6a0
src/cpu/testers/networktest/networktest.cc 5534a564f6a0
src/cpu/testers/rubytest/RubyTester.hh 5534a564f6a0
src/cpu/testers/rubytest/RubyTester.cc 5534a564f6a0
src/dev/io_device.hh 5534a564f6a0
src/dev/io_device.cc 5534a564f6a0
src/mem/bridge.hh 5534a564f6a0
src/mem/bridge.cc 5534a564f6a0
src/mem/bus.hh 5534a564f6a0
src/mem/bus.cc 5534a564f6a0
src/mem/cache/base.hh 5534a564f6a0
src/mem/cache/cache.hh 5534a564f6a0
src/mem/cache/cache_impl.hh 5534a564f6a0
src/mem/mport.hh 5534a564f6a0
src/mem/mport.cc 5534a564f6a0
src/mem/packet_queue.hh 5534a564f6a0
src/mem/packet_queue.cc 5534a564f6a0
src/mem/port.hh 5534a564f6a0
src/mem/port.cc 5534a564f6a0
src/mem/ruby/system/RubyPort.hh 5534a564f6a0
src/mem/ruby/system/RubyPort.cc 5534a564f6a0
src/sim/system.hh 5534a564f6a0
Diff: http://reviews.gem5.org/r/1118/diff/
Testing
-------
util/regress all passing (disregarding t1000 and eio)
Thanks,
Andreas Hansson
_______________________________________________
gem5-dev mailing list
gem5-dev-1Gs4CP2/***@public.gmane.org<mailto:gem5-dev-1Gs4CP2/***@public.gmane.org>
http://m5sim.org/mailman/listinfo/gem5-dev


-- IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
Andreas Hansson
2012-04-05 16:57:57 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/1118/
-----------------------------------------------------------

(Updated April 5, 2012, 9:57 a.m.)


Review request for Default.


Description
-------

MEM: Separate snoops and normal memory requests/responses

This patch introduces port access methods that separates snoop
request/responses from normal memory request/responses. The
differentiation is made for functional, atomic and timing accesses and
builds on the introduction of master and slave ports.

Before the introduction of this patch, the packets belonging to the
different phases of the protocol (request -> [forwarded snoop request
-> snoop response]* -> response) all use the same port access
functions, even though the snoop packets flow in the opposite
direction to the normal packet. That is, a coherent master sends
normal request and receives responses, but receives snoop requests and
sends snoop responses (vice versa for the slave). These two distinct
phases now use different access functions, as described below.

Starting with the functional access, a master sends a request to a
slave through sendFunctional, and the request packet is turned into a
response before the call returns. In a system without cache coherence,
this is all that is needed from the functional interface. For the
cache-coherent scenario, a slave also sends snoop requests to coherent
masters through sendFunctionalSnoop, with responses returned within
the same packet pointer. This is currently used by the bus and caches,
and the LSQ of the O3 CPU. The send/recvFunctional and
send/recvFunctionalSnoop are moved from the Port super class to the
appropriate subclass.

Atomic accesses follow the same flow as functional accesses, with
request being sent from master to slave through sendAtomic. In the
case of cache-coherent ports, a slave can send snoop requests to a
master through sendAtomicSnoop. Just as for the functional access
methods, the atomic send and receive member functions are moved to the
appropriate subclasses.

The timing access methods are different from the functional and atomic
in that requests and responses are separated in time and
send/recvTiming are used for both directions. Hence, a master uses
sendTiming to send a request to a slave, and a slave uses sendTiming
to send a response back to a master, at a later point in time. Snoop
requests and responses travel in the opposite direction, similar to
what happens in functional and atomic accesses. With the introduction
of this patch, it is possible to determine the direction of packets in
the bus, and no longer necessary to look for both a master and a slave
port with the requested port id.

In contrast to the normal recvFunctional, recvAtomic and recvTiming
that are pure virtual functions, the recvFunctionalSnoop,
recvAtomicSnoop and recvTimingSnoop have a default implementation that
calls panic. This is to allow non-coherent master and slave ports to
not implement these functions.


Diffs (updated)
-----

src/arch/x86/pagetable_walker.hh 570b44fe6e04
src/arch/x86/pagetable_walker.cc 570b44fe6e04
src/cpu/base.hh 570b44fe6e04
src/cpu/base.cc 570b44fe6e04
src/cpu/inorder/cpu.hh 570b44fe6e04
src/cpu/inorder/cpu.cc 570b44fe6e04
src/cpu/o3/cpu.hh 570b44fe6e04
src/cpu/o3/cpu.cc 570b44fe6e04
src/cpu/o3/lsq.hh 570b44fe6e04
src/cpu/o3/lsq_impl.hh 570b44fe6e04
src/cpu/simple/atomic.hh 570b44fe6e04
src/cpu/simple/timing.hh 570b44fe6e04
src/cpu/simple/timing.cc 570b44fe6e04
src/cpu/testers/directedtest/RubyDirectedTester.hh 570b44fe6e04
src/cpu/testers/directedtest/RubyDirectedTester.cc 570b44fe6e04
src/cpu/testers/memtest/memtest.hh 570b44fe6e04
src/cpu/testers/memtest/memtest.cc 570b44fe6e04
src/cpu/testers/networktest/networktest.hh 570b44fe6e04
src/cpu/testers/networktest/networktest.cc 570b44fe6e04
src/cpu/testers/rubytest/RubyTester.hh 570b44fe6e04
src/cpu/testers/rubytest/RubyTester.cc 570b44fe6e04
src/dev/io_device.hh 570b44fe6e04
src/dev/io_device.cc 570b44fe6e04
src/mem/bridge.hh 570b44fe6e04
src/mem/bridge.cc 570b44fe6e04
src/mem/bus.hh 570b44fe6e04
src/mem/bus.cc 570b44fe6e04
src/mem/cache/base.hh 570b44fe6e04
src/mem/cache/cache.hh 570b44fe6e04
src/mem/cache/cache_impl.hh 570b44fe6e04
src/mem/mport.hh 570b44fe6e04
src/mem/mport.cc 570b44fe6e04
src/mem/packet_queue.hh 570b44fe6e04
src/mem/packet_queue.cc 570b44fe6e04
src/mem/port.hh 570b44fe6e04
src/mem/port.cc 570b44fe6e04
src/mem/ruby/system/RubyPort.hh 570b44fe6e04
src/mem/ruby/system/RubyPort.cc 570b44fe6e04
src/sim/system.hh 570b44fe6e04

Diff: http://reviews.gem5.org/r/1118/diff/


Testing
-------

util/regress all passing (disregarding t1000 and eio)


Thanks,

Andreas Hansson
Steve Reinhardt
2012-04-09 04:22:44 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/1118/#review2512
-----------------------------------------------------------


Overall looks good. I like your descriptions in the commit message... can you incorporate the bulk of that in a comment somewhere, or put it on the wiki? No need for the "before" part (that's useful for the changeset, but not as part of the stable documentation), but the middle few paragraphs that describe how things work need to be included somewhere where they're easier for future generations to find.


src/cpu/base.cc
<http://reviews.gem5.org/r/1118/#comment2919>

Do we still need this if the default implementation already panics?



src/mem/bus.cc
<http://reviews.gem5.org/r/1118/#comment2920>

This master/slave thing (where the bus masters connect to BusSlavePorts and slaves connect to BusMasterPorts) still confuses me... at the very least, let's be clear in the comments that that's what is happening. Also, I would say there's no such thing as a "default master". There's a default slave device, which (by our twisted logic) has to connect to a master port, but that doesn't make it a "default master".

I think these comments really refer more to changes that happened in the previous patch, but I was asleep at the wheel then or something.



src/mem/bus.cc
<http://reviews.gem5.org/r/1118/#comment2921>

again, "default master" just seems wrong



src/mem/bus.cc
<http://reviews.gem5.org/r/1118/#comment2922>

This is unnecessary... just add a Port * param to Bus::recvTiming, and change the recvTiming call in BusSlavePort and BusMasterPort to call bus->revTiming(pkt, this).




src/mem/bus.cc
<http://reviews.gem5.org/r/1118/#comment2923>

typo in comment



src/mem/bus.cc
<http://reviews.gem5.org/r/1118/#comment2925>

There seems to be quite a bit of duplicated code here with recvTiming() (and similarly recvAtomicSnoop() appears to duplicate a lot of code with recvAtomic()).

Can we leave recvTiming and recvAtomic as integrated functions, but add an isSnoop flag to distinguish snoops from non-snoops? E.g., the BusMasterPort and BusSlavePort methods would look like this:

recvTiming(PacketPtr pkt) {
bus->recvTiming(pkt, this, false);
}

recvTimingSnoop(PacketPtr pkt) {
bus->recvTiming(pkt, this, true);
}

I'd think this would still allow cleanly distinguishing the snoop and non-snoop cases while avoiding so much code duplication. But I'm just eyeballing it...



- Steve Reinhardt
Post by Andreas Hansson
-----------------------------------------------------------
http://reviews.gem5.org/r/1118/
-----------------------------------------------------------
(Updated April 5, 2012, 9:57 a.m.)
Review request for Default.
Description
-------
MEM: Separate snoops and normal memory requests/responses
This patch introduces port access methods that separates snoop
request/responses from normal memory request/responses. The
differentiation is made for functional, atomic and timing accesses and
builds on the introduction of master and slave ports.
Before the introduction of this patch, the packets belonging to the
different phases of the protocol (request -> [forwarded snoop request
-> snoop response]* -> response) all use the same port access
functions, even though the snoop packets flow in the opposite
direction to the normal packet. That is, a coherent master sends
normal request and receives responses, but receives snoop requests and
sends snoop responses (vice versa for the slave). These two distinct
phases now use different access functions, as described below.
Starting with the functional access, a master sends a request to a
slave through sendFunctional, and the request packet is turned into a
response before the call returns. In a system without cache coherence,
this is all that is needed from the functional interface. For the
cache-coherent scenario, a slave also sends snoop requests to coherent
masters through sendFunctionalSnoop, with responses returned within
the same packet pointer. This is currently used by the bus and caches,
and the LSQ of the O3 CPU. The send/recvFunctional and
send/recvFunctionalSnoop are moved from the Port super class to the
appropriate subclass.
Atomic accesses follow the same flow as functional accesses, with
request being sent from master to slave through sendAtomic. In the
case of cache-coherent ports, a slave can send snoop requests to a
master through sendAtomicSnoop. Just as for the functional access
methods, the atomic send and receive member functions are moved to the
appropriate subclasses.
The timing access methods are different from the functional and atomic
in that requests and responses are separated in time and
send/recvTiming are used for both directions. Hence, a master uses
sendTiming to send a request to a slave, and a slave uses sendTiming
to send a response back to a master, at a later point in time. Snoop
requests and responses travel in the opposite direction, similar to
what happens in functional and atomic accesses. With the introduction
of this patch, it is possible to determine the direction of packets in
the bus, and no longer necessary to look for both a master and a slave
port with the requested port id.
In contrast to the normal recvFunctional, recvAtomic and recvTiming
that are pure virtual functions, the recvFunctionalSnoop,
recvAtomicSnoop and recvTimingSnoop have a default implementation that
calls panic. This is to allow non-coherent master and slave ports to
not implement these functions.
Diffs
-----
src/arch/x86/pagetable_walker.hh 570b44fe6e04
src/arch/x86/pagetable_walker.cc 570b44fe6e04
src/cpu/base.hh 570b44fe6e04
src/cpu/base.cc 570b44fe6e04
src/cpu/inorder/cpu.hh 570b44fe6e04
src/cpu/inorder/cpu.cc 570b44fe6e04
src/cpu/o3/cpu.hh 570b44fe6e04
src/cpu/o3/cpu.cc 570b44fe6e04
src/cpu/o3/lsq.hh 570b44fe6e04
src/cpu/o3/lsq_impl.hh 570b44fe6e04
src/cpu/simple/atomic.hh 570b44fe6e04
src/cpu/simple/timing.hh 570b44fe6e04
src/cpu/simple/timing.cc 570b44fe6e04
src/cpu/testers/directedtest/RubyDirectedTester.hh 570b44fe6e04
src/cpu/testers/directedtest/RubyDirectedTester.cc 570b44fe6e04
src/cpu/testers/memtest/memtest.hh 570b44fe6e04
src/cpu/testers/memtest/memtest.cc 570b44fe6e04
src/cpu/testers/networktest/networktest.hh 570b44fe6e04
src/cpu/testers/networktest/networktest.cc 570b44fe6e04
src/cpu/testers/rubytest/RubyTester.hh 570b44fe6e04
src/cpu/testers/rubytest/RubyTester.cc 570b44fe6e04
src/dev/io_device.hh 570b44fe6e04
src/dev/io_device.cc 570b44fe6e04
src/mem/bridge.hh 570b44fe6e04
src/mem/bridge.cc 570b44fe6e04
src/mem/bus.hh 570b44fe6e04
src/mem/bus.cc 570b44fe6e04
src/mem/cache/base.hh 570b44fe6e04
src/mem/cache/cache.hh 570b44fe6e04
src/mem/cache/cache_impl.hh 570b44fe6e04
src/mem/mport.hh 570b44fe6e04
src/mem/mport.cc 570b44fe6e04
src/mem/packet_queue.hh 570b44fe6e04
src/mem/packet_queue.cc 570b44fe6e04
src/mem/port.hh 570b44fe6e04
src/mem/port.cc 570b44fe6e04
src/mem/ruby/system/RubyPort.hh 570b44fe6e04
src/mem/ruby/system/RubyPort.cc 570b44fe6e04
src/sim/system.hh 570b44fe6e04
Diff: http://reviews.gem5.org/r/1118/diff/
Testing
-------
util/regress all passing (disregarding t1000 and eio)
Thanks,
Andreas Hansson
Andreas Hansson
2012-04-09 11:10:51 UTC
Permalink
Post by Andreas Hansson
Post by Steve Reinhardt
Overall looks good. I like your descriptions in the commit message... can you incorporate the bulk of that in a comment somewhere, or put it on the wiki? No need for the "before" part (that's useful for the changeset, but not as part of the stable documentation), but the middle few paragraphs that describe how things work need to be included somewhere where they're easier for future generations to find.
Thanks!

I have a document ready for updating the wiki, and this indeed part of it. Overall, we have prepared a decent amount of documentation that will go on the wiki as the functionality is committed, and this is probably a good time to start putting things in place.
Post by Andreas Hansson
Post by Steve Reinhardt
src/cpu/base.cc, line 548
<http://reviews.gem5.org/r/1118/diff/3/?file=25539#file25539line548>
Do we still need this if the default implementation already panics?
It can indeed be removed, currently it is there for symmetry with the other accessors of the BaseCPU port. Sounds reasonable?
Post by Andreas Hansson
Post by Steve Reinhardt
src/mem/bus.cc, line 74
<http://reviews.gem5.org/r/1118/diff/3/?file=25562#file25562line74>
This master/slave thing (where the bus masters connect to BusSlavePorts and slaves connect to BusMasterPorts) still confuses me... at the very least, let's be clear in the comments that that's what is happening. Also, I would say there's no such thing as a "default master". There's a default slave device, which (by our twisted logic) has to connect to a master port, but that doesn't make it a "default master".
I think these comments really refer more to changes that happened in the previous patch, but I was asleep at the wheel then or something.
I'm happy to add more descriptions and explanations, and could also refer to all existing documentation for TLM-2, and/or any protocol like AMBA, OCP, VCI, etc. just to have more background info that uses the same terminology.

The notion of master and slave and that a master always connects to a slave will be on the Wiki as well. The bus has a master port that is the default port...hence default master from the perspective of the bus. The master port that connects to the default slave.
Post by Andreas Hansson
Post by Steve Reinhardt
src/mem/bus.cc, line 216
<http://reviews.gem5.org/r/1118/diff/3/?file=25562#file25562line216>
This is unnecessary... just add a Port * param to Bus::recvTiming, and change the recvTiming call in BusSlavePort and BusMasterPort to call bus->revTiming(pkt, this).
A few patches down the line, the bus port will disappear, and the bus itself will implement the MasterInterface and SlaveInterface that has the recv functions with two parameters, a packet pointer, and a port id. Thus, for this patch it would be much prefers to keep it the way it is if that's ok with you.
Post by Andreas Hansson
Post by Steve Reinhardt
src/mem/bus.cc, line 352
<http://reviews.gem5.org/r/1118/diff/3/?file=25562#file25562line352>
There seems to be quite a bit of duplicated code here with recvTiming() (and similarly recvAtomicSnoop() appears to duplicate a lot of code with recvAtomic()).
recvTiming(PacketPtr pkt) {
bus->recvTiming(pkt, this, false);
}
recvTimingSnoop(PacketPtr pkt) {
bus->recvTiming(pkt, this, true);
}
I'd think this would still allow cleanly distinguishing the snoop and non-snoop cases while avoiding so much code duplication. But I'm just eyeballing it...
I can break out additional code into new member functions rather. The next few patches will introduce the master and slave interfaces, and in doing so also split the recvTiming and recvTimingSnoop into a request and response part that then get moved from the Port class to the appropriate master/slave subclass. The end result is a non-ambiguous interface that is unique to each port. The concept of the parameter would make this impossible, as it by construction would have to be shared between both.

I'll factor out more of the code in members.


- Andreas


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/1118/#review2512
-----------------------------------------------------------
Post by Andreas Hansson
-----------------------------------------------------------
http://reviews.gem5.org/r/1118/
-----------------------------------------------------------
(Updated April 5, 2012, 9:57 a.m.)
Review request for Default.
Description
-------
MEM: Separate snoops and normal memory requests/responses
This patch introduces port access methods that separates snoop
request/responses from normal memory request/responses. The
differentiation is made for functional, atomic and timing accesses and
builds on the introduction of master and slave ports.
Before the introduction of this patch, the packets belonging to the
different phases of the protocol (request -> [forwarded snoop request
-> snoop response]* -> response) all use the same port access
functions, even though the snoop packets flow in the opposite
direction to the normal packet. That is, a coherent master sends
normal request and receives responses, but receives snoop requests and
sends snoop responses (vice versa for the slave). These two distinct
phases now use different access functions, as described below.
Starting with the functional access, a master sends a request to a
slave through sendFunctional, and the request packet is turned into a
response before the call returns. In a system without cache coherence,
this is all that is needed from the functional interface. For the
cache-coherent scenario, a slave also sends snoop requests to coherent
masters through sendFunctionalSnoop, with responses returned within
the same packet pointer. This is currently used by the bus and caches,
and the LSQ of the O3 CPU. The send/recvFunctional and
send/recvFunctionalSnoop are moved from the Port super class to the
appropriate subclass.
Atomic accesses follow the same flow as functional accesses, with
request being sent from master to slave through sendAtomic. In the
case of cache-coherent ports, a slave can send snoop requests to a
master through sendAtomicSnoop. Just as for the functional access
methods, the atomic send and receive member functions are moved to the
appropriate subclasses.
The timing access methods are different from the functional and atomic
in that requests and responses are separated in time and
send/recvTiming are used for both directions. Hence, a master uses
sendTiming to send a request to a slave, and a slave uses sendTiming
to send a response back to a master, at a later point in time. Snoop
requests and responses travel in the opposite direction, similar to
what happens in functional and atomic accesses. With the introduction
of this patch, it is possible to determine the direction of packets in
the bus, and no longer necessary to look for both a master and a slave
port with the requested port id.
In contrast to the normal recvFunctional, recvAtomic and recvTiming
that are pure virtual functions, the recvFunctionalSnoop,
recvAtomicSnoop and recvTimingSnoop have a default implementation that
calls panic. This is to allow non-coherent master and slave ports to
not implement these functions.
Diffs
-----
src/arch/x86/pagetable_walker.hh 570b44fe6e04
src/arch/x86/pagetable_walker.cc 570b44fe6e04
src/cpu/base.hh 570b44fe6e04
src/cpu/base.cc 570b44fe6e04
src/cpu/inorder/cpu.hh 570b44fe6e04
src/cpu/inorder/cpu.cc 570b44fe6e04
src/cpu/o3/cpu.hh 570b44fe6e04
src/cpu/o3/cpu.cc 570b44fe6e04
src/cpu/o3/lsq.hh 570b44fe6e04
src/cpu/o3/lsq_impl.hh 570b44fe6e04
src/cpu/simple/atomic.hh 570b44fe6e04
src/cpu/simple/timing.hh 570b44fe6e04
src/cpu/simple/timing.cc 570b44fe6e04
src/cpu/testers/directedtest/RubyDirectedTester.hh 570b44fe6e04
src/cpu/testers/directedtest/RubyDirectedTester.cc 570b44fe6e04
src/cpu/testers/memtest/memtest.hh 570b44fe6e04
src/cpu/testers/memtest/memtest.cc 570b44fe6e04
src/cpu/testers/networktest/networktest.hh 570b44fe6e04
src/cpu/testers/networktest/networktest.cc 570b44fe6e04
src/cpu/testers/rubytest/RubyTester.hh 570b44fe6e04
src/cpu/testers/rubytest/RubyTester.cc 570b44fe6e04
src/dev/io_device.hh 570b44fe6e04
src/dev/io_device.cc 570b44fe6e04
src/mem/bridge.hh 570b44fe6e04
src/mem/bridge.cc 570b44fe6e04
src/mem/bus.hh 570b44fe6e04
src/mem/bus.cc 570b44fe6e04
src/mem/cache/base.hh 570b44fe6e04
src/mem/cache/cache.hh 570b44fe6e04
src/mem/cache/cache_impl.hh 570b44fe6e04
src/mem/mport.hh 570b44fe6e04
src/mem/mport.cc 570b44fe6e04
src/mem/packet_queue.hh 570b44fe6e04
src/mem/packet_queue.cc 570b44fe6e04
src/mem/port.hh 570b44fe6e04
src/mem/port.cc 570b44fe6e04
src/mem/ruby/system/RubyPort.hh 570b44fe6e04
src/mem/ruby/system/RubyPort.cc 570b44fe6e04
src/sim/system.hh 570b44fe6e04
Diff: http://reviews.gem5.org/r/1118/diff/
Testing
-------
util/regress all passing (disregarding t1000 and eio)
Thanks,
Andreas Hansson
Steve Reinhardt
2012-04-10 16:03:27 UTC
Permalink
Post by Andreas Hansson
Post by Steve Reinhardt
src/cpu/base.cc, line 548
<http://reviews.gem5.org/r/1118/diff/3/?file=25539#file25539line548>
Do we still need this if the default implementation already panics?
It can indeed be removed, currently it is there for symmetry with the other accessors of the BaseCPU port. Sounds reasonable?
Not a big deal either way... it just appears that a number of no-op/panic handlers are being removed, presumably because they merely override default versions that do the same thing, and this stuck out as an exception so I thought it may have been overlooked.
Post by Andreas Hansson
Post by Steve Reinhardt
src/mem/bus.cc, line 352
<http://reviews.gem5.org/r/1118/diff/3/?file=25562#file25562line352>
There seems to be quite a bit of duplicated code here with recvTiming() (and similarly recvAtomicSnoop() appears to duplicate a lot of code with recvAtomic()).
recvTiming(PacketPtr pkt) {
bus->recvTiming(pkt, this, false);
}
recvTimingSnoop(PacketPtr pkt) {
bus->recvTiming(pkt, this, true);
}
I'd think this would still allow cleanly distinguishing the snoop and non-snoop cases while avoiding so much code duplication. But I'm just eyeballing it...
I can break out additional code into new member functions rather. The next few patches will introduce the master and slave interfaces, and in doing so also split the recvTiming and recvTimingSnoop into a request and response part that then get moved from the Port class to the appropriate master/slave subclass. The end result is a non-ambiguous interface that is unique to each port. The concept of the parameter would make this impossible, as it by construction would have to be shared between both.
I'll factor out more of the code in members.
Thanks for the update, but I'm still not thrilled that, for example, recvAtomic() and recvAtomicSnoop() are over thirty lines each of which approximately two are different. It sounds like the direction you're headed is not actually going to solve this, but rather make it impossible (or more difficult) to solve. I guess I can withhold judgment awhile longer, but on the face of it this seems like a step backwards.
Post by Andreas Hansson
Post by Steve Reinhardt
src/mem/bus.cc, line 216
<http://reviews.gem5.org/r/1118/diff/3/?file=25562#file25562line216>
This is unnecessary... just add a Port * param to Bus::recvTiming, and change the recvTiming call in BusSlavePort and BusMasterPort to call bus->revTiming(pkt, this).
A few patches down the line, the bus port will disappear, and the bus itself will implement the MasterInterface and SlaveInterface that has the recv functions with two parameters, a packet pointer, and a port id. Thus, for this patch it would be much prefers to keep it the way it is if that's ok with you.
OK, if this isn't the final destination, I'll wait until we get there to see if I'm happy ;-).
Post by Andreas Hansson
Post by Steve Reinhardt
src/mem/bus.cc, line 74
<http://reviews.gem5.org/r/1118/diff/3/?file=25562#file25562line74>
This master/slave thing (where the bus masters connect to BusSlavePorts and slaves connect to BusMasterPorts) still confuses me... at the very least, let's be clear in the comments that that's what is happening. Also, I would say there's no such thing as a "default master". There's a default slave device, which (by our twisted logic) has to connect to a master port, but that doesn't make it a "default master".
I think these comments really refer more to changes that happened in the previous patch, but I was asleep at the wheel then or something.
I'm happy to add more descriptions and explanations, and could also refer to all existing documentation for TLM-2, and/or any protocol like AMBA, OCP, VCI, etc. just to have more background info that uses the same terminology.
The notion of master and slave and that a master always connects to a slave will be on the Wiki as well. The bus has a master port that is the default port...hence default master from the perspective of the bus. The master port that connects to the default slave.
There are two parts: understanding it (which only needs to happen once) and getting used to it (which may never happen). The former is important, but I'm past that; it's the latter that bugs me.

It's like when you give a talk and you want to point out something on your slides, and you have to remember that what's on your left as you face the audience is on the audience's right, unless you're looking at your slides on your laptop and not on the screen, in which case your left is the same as the audience's left. The concept is straightforward, and it's easy to figure out when you stop and think about it, but it doesn't readily become intuitive, as you can tell by the number of people that mess it up.

So mostly what I'm talking about here is just being generous and redundant with casual reminders in the comments that when we're accessing master ports it is so that we can talk to slave devices, and vice versa. I see that you've already done some of that, which is great.

As far as "default master" specifically, I figured out why it bugged me so much... I see that you've already addressed this too, but I'll explain anyway just in case it helps keep you from thinking I'm crazy. (Or maybe it will further convince you that I am crazy, but I'll take that risk.)

Basically when you use "master" or "slave" as a stand-alone noun, to me that implicitly refers to a device. So "default master" sounds like "default master device", which makes no sense. Saying that the port that's used to talk to the default slave is the "default master port" is conceptually OK (even if it takes getting used to as discussed above), but abbreviating "default master port" as "default master" is where it breaks for me.

I think it's also true that any use of "master" or "slave" as an adjective modifying anything other than "port" inclines me to think of the device and not the port, and is at best ambiguous. Thus I would recommend staying away from terms like "master id" and "slave id". It looks like "master id" is not used, and "slave id" is used only in the forward*() functions, where of course it identifies the master device to exclude ;-).


- Steve


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/1118/#review2512
-----------------------------------------------------------
Post by Andreas Hansson
-----------------------------------------------------------
http://reviews.gem5.org/r/1118/
-----------------------------------------------------------
(Updated April 9, 2012, 8:44 a.m.)
Review request for Default.
Description
-------
MEM: Separate snoops and normal memory requests/responses
This patch introduces port access methods that separates snoop
request/responses from normal memory request/responses. The
differentiation is made for functional, atomic and timing accesses and
builds on the introduction of master and slave ports.
Before the introduction of this patch, the packets belonging to the
different phases of the protocol (request -> [forwarded snoop request
-> snoop response]* -> response) all use the same port access
functions, even though the snoop packets flow in the opposite
direction to the normal packet. That is, a coherent master sends
normal request and receives responses, but receives snoop requests and
sends snoop responses (vice versa for the slave). These two distinct
phases now use different access functions, as described below.
Starting with the functional access, a master sends a request to a
slave through sendFunctional, and the request packet is turned into a
response before the call returns. In a system without cache coherence,
this is all that is needed from the functional interface. For the
cache-coherent scenario, a slave also sends snoop requests to coherent
masters through sendFunctionalSnoop, with responses returned within
the same packet pointer. This is currently used by the bus and caches,
and the LSQ of the O3 CPU. The send/recvFunctional and
send/recvFunctionalSnoop are moved from the Port super class to the
appropriate subclass.
Atomic accesses follow the same flow as functional accesses, with
request being sent from master to slave through sendAtomic. In the
case of cache-coherent ports, a slave can send snoop requests to a
master through sendAtomicSnoop. Just as for the functional access
methods, the atomic send and receive member functions are moved to the
appropriate subclasses.
The timing access methods are different from the functional and atomic
in that requests and responses are separated in time and
send/recvTiming are used for both directions. Hence, a master uses
sendTiming to send a request to a slave, and a slave uses sendTiming
to send a response back to a master, at a later point in time. Snoop
requests and responses travel in the opposite direction, similar to
what happens in functional and atomic accesses. With the introduction
of this patch, it is possible to determine the direction of packets in
the bus, and no longer necessary to look for both a master and a slave
port with the requested port id.
In contrast to the normal recvFunctional, recvAtomic and recvTiming
that are pure virtual functions, the recvFunctionalSnoop,
recvAtomicSnoop and recvTimingSnoop have a default implementation that
calls panic. This is to allow non-coherent master and slave ports to
not implement these functions.
Diffs
-----
src/arch/x86/pagetable_walker.hh a47fd7c2d44e
src/arch/x86/pagetable_walker.cc a47fd7c2d44e
src/cpu/base.hh a47fd7c2d44e
src/cpu/base.cc a47fd7c2d44e
src/cpu/inorder/cpu.hh a47fd7c2d44e
src/cpu/inorder/cpu.cc a47fd7c2d44e
src/cpu/o3/cpu.hh a47fd7c2d44e
src/cpu/o3/cpu.cc a47fd7c2d44e
src/cpu/o3/lsq.hh a47fd7c2d44e
src/cpu/o3/lsq_impl.hh a47fd7c2d44e
src/cpu/simple/atomic.hh a47fd7c2d44e
src/cpu/simple/timing.hh a47fd7c2d44e
src/cpu/simple/timing.cc a47fd7c2d44e
src/cpu/testers/directedtest/RubyDirectedTester.hh a47fd7c2d44e
src/cpu/testers/directedtest/RubyDirectedTester.cc a47fd7c2d44e
src/cpu/testers/memtest/memtest.hh a47fd7c2d44e
src/cpu/testers/memtest/memtest.cc a47fd7c2d44e
src/cpu/testers/networktest/networktest.hh a47fd7c2d44e
src/cpu/testers/networktest/networktest.cc a47fd7c2d44e
src/cpu/testers/rubytest/RubyTester.hh a47fd7c2d44e
src/cpu/testers/rubytest/RubyTester.cc a47fd7c2d44e
src/dev/io_device.hh a47fd7c2d44e
src/dev/io_device.cc a47fd7c2d44e
src/mem/bridge.hh a47fd7c2d44e
src/mem/bridge.cc a47fd7c2d44e
src/mem/bus.hh a47fd7c2d44e
src/mem/bus.cc a47fd7c2d44e
src/mem/cache/base.hh a47fd7c2d44e
src/mem/cache/cache.hh a47fd7c2d44e
src/mem/cache/cache_impl.hh a47fd7c2d44e
src/mem/mport.hh a47fd7c2d44e
src/mem/mport.cc a47fd7c2d44e
src/mem/packet_queue.hh a47fd7c2d44e
src/mem/packet_queue.cc a47fd7c2d44e
src/mem/port.hh a47fd7c2d44e
src/mem/port.cc a47fd7c2d44e
src/mem/ruby/system/RubyPort.hh a47fd7c2d44e
src/mem/ruby/system/RubyPort.cc a47fd7c2d44e
src/sim/system.hh a47fd7c2d44e
Diff: http://reviews.gem5.org/r/1118/diff/
Testing
-------
util/regress all passing (disregarding t1000 and eio)
Thanks,
Andreas Hansson
Andreas Hansson
2012-04-10 16:43:53 UTC
Permalink
Post by Andreas Hansson
Post by Steve Reinhardt
src/mem/bus.cc, line 74
<http://reviews.gem5.org/r/1118/diff/3/?file=25562#file25562line74>
This master/slave thing (where the bus masters connect to BusSlavePorts and slaves connect to BusMasterPorts) still confuses me... at the very least, let's be clear in the comments that that's what is happening. Also, I would say there's no such thing as a "default master". There's a default slave device, which (by our twisted logic) has to connect to a master port, but that doesn't make it a "default master".
I think these comments really refer more to changes that happened in the previous patch, but I was asleep at the wheel then or something.
I'm happy to add more descriptions and explanations, and could also refer to all existing documentation for TLM-2, and/or any protocol like AMBA, OCP, VCI, etc. just to have more background info that uses the same terminology.
The notion of master and slave and that a master always connects to a slave will be on the Wiki as well. The bus has a master port that is the default port...hence default master from the perspective of the bus. The master port that connects to the default slave.
There are two parts: understanding it (which only needs to happen once) and getting used to it (which may never happen). The former is important, but I'm past that; it's the latter that bugs me.
It's like when you give a talk and you want to point out something on your slides, and you have to remember that what's on your left as you face the audience is on the audience's right, unless you're looking at your slides on your laptop and not on the screen, in which case your left is the same as the audience's left. The concept is straightforward, and it's easy to figure out when you stop and think about it, but it doesn't readily become intuitive, as you can tell by the number of people that mess it up.
So mostly what I'm talking about here is just being generous and redundant with casual reminders in the comments that when we're accessing master ports it is so that we can talk to slave devices, and vice versa. I see that you've already done some of that, which is great.
As far as "default master" specifically, I figured out why it bugged me so much... I see that you've already addressed this too, but I'll explain anyway just in case it helps keep you from thinking I'm crazy. (Or maybe it will further convince you that I am crazy, but I'll take that risk.)
Basically when you use "master" or "slave" as a stand-alone noun, to me that implicitly refers to a device. So "default master" sounds like "default master device", which makes no sense. Saying that the port that's used to talk to the default slave is the "default master port" is conceptually OK (even if it takes getting used to as discussed above), but abbreviating "default master port" as "default master" is where it breaks for me.
I think it's also true that any use of "master" or "slave" as an adjective modifying anything other than "port" inclines me to think of the device and not the port, and is at best ambiguous. Thus I would recommend staying away from terms like "master id" and "slave id". It looks like "master id" is not used, and "slave id" is used only in the forward*() functions, where of course it identifies the master device to exclude ;-).
I agree with your comments, and will be even better at clarifying "master port" and "slave port" when talking about ports. Consider the excluded_slave_id parameter changed to a slightly lengthier excluded_slave_port_id :)

In general, a "master" should refer to a module with only "master ports", and slave to a module with only "slave ports". If you have e.g. a bridge or a DmaDevice, it becomes more difficult as it has both master ports and slave ports. I will make sure I'm consistent with this terminology in this and future patches.

...and don't worry, I don't think you're crazy.
Post by Andreas Hansson
Post by Steve Reinhardt
src/mem/bus.cc, line 352
<http://reviews.gem5.org/r/1118/diff/3/?file=25562#file25562line352>
There seems to be quite a bit of duplicated code here with recvTiming() (and similarly recvAtomicSnoop() appears to duplicate a lot of code with recvAtomic()).
recvTiming(PacketPtr pkt) {
bus->recvTiming(pkt, this, false);
}
recvTimingSnoop(PacketPtr pkt) {
bus->recvTiming(pkt, this, true);
}
I'd think this would still allow cleanly distinguishing the snoop and non-snoop cases while avoiding so much code duplication. But I'm just eyeballing it...
I can break out additional code into new member functions rather. The next few patches will introduce the master and slave interfaces, and in doing so also split the recvTiming and recvTimingSnoop into a request and response part that then get moved from the Port class to the appropriate master/slave subclass. The end result is a non-ambiguous interface that is unique to each port. The concept of the parameter would make this impossible, as it by construction would have to be shared between both.
I'll factor out more of the code in members.
Thanks for the update, but I'm still not thrilled that, for example, recvAtomic() and recvAtomicSnoop() are over thirty lines each of which approximately two are different. It sounds like the direction you're headed is not actually going to solve this, but rather make it impossible (or more difficult) to solve. I guess I can withhold judgment awhile longer, but on the face of it this seems like a step backwards.
I'll simplify recvAtomicSnoop similar to what is done for recvTimingSnoop and post a new version of the patch. I hope that is more in line with what you were hoping for. If there is more I can do please let me know.

The direction I'm heading in is to separate out the different protocol phases into separate functions, with very clear semantics (they are there at the moment, just hidden in a complex control flow). The assert statements that currently "guard" these assumptions can eventually go away once all modules are implementing these interfaces (as exemplified by removing the broadcast flag).


- Andreas


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/1118/#review2512
-----------------------------------------------------------
Post by Andreas Hansson
-----------------------------------------------------------
http://reviews.gem5.org/r/1118/
-----------------------------------------------------------
(Updated April 9, 2012, 8:44 a.m.)
Review request for Default.
Description
-------
MEM: Separate snoops and normal memory requests/responses
This patch introduces port access methods that separates snoop
request/responses from normal memory request/responses. The
differentiation is made for functional, atomic and timing accesses and
builds on the introduction of master and slave ports.
Before the introduction of this patch, the packets belonging to the
different phases of the protocol (request -> [forwarded snoop request
-> snoop response]* -> response) all use the same port access
functions, even though the snoop packets flow in the opposite
direction to the normal packet. That is, a coherent master sends
normal request and receives responses, but receives snoop requests and
sends snoop responses (vice versa for the slave). These two distinct
phases now use different access functions, as described below.
Starting with the functional access, a master sends a request to a
slave through sendFunctional, and the request packet is turned into a
response before the call returns. In a system without cache coherence,
this is all that is needed from the functional interface. For the
cache-coherent scenario, a slave also sends snoop requests to coherent
masters through sendFunctionalSnoop, with responses returned within
the same packet pointer. This is currently used by the bus and caches,
and the LSQ of the O3 CPU. The send/recvFunctional and
send/recvFunctionalSnoop are moved from the Port super class to the
appropriate subclass.
Atomic accesses follow the same flow as functional accesses, with
request being sent from master to slave through sendAtomic. In the
case of cache-coherent ports, a slave can send snoop requests to a
master through sendAtomicSnoop. Just as for the functional access
methods, the atomic send and receive member functions are moved to the
appropriate subclasses.
The timing access methods are different from the functional and atomic
in that requests and responses are separated in time and
send/recvTiming are used for both directions. Hence, a master uses
sendTiming to send a request to a slave, and a slave uses sendTiming
to send a response back to a master, at a later point in time. Snoop
requests and responses travel in the opposite direction, similar to
what happens in functional and atomic accesses. With the introduction
of this patch, it is possible to determine the direction of packets in
the bus, and no longer necessary to look for both a master and a slave
port with the requested port id.
In contrast to the normal recvFunctional, recvAtomic and recvTiming
that are pure virtual functions, the recvFunctionalSnoop,
recvAtomicSnoop and recvTimingSnoop have a default implementation that
calls panic. This is to allow non-coherent master and slave ports to
not implement these functions.
Diffs
-----
src/arch/x86/pagetable_walker.hh a47fd7c2d44e
src/arch/x86/pagetable_walker.cc a47fd7c2d44e
src/cpu/base.hh a47fd7c2d44e
src/cpu/base.cc a47fd7c2d44e
src/cpu/inorder/cpu.hh a47fd7c2d44e
src/cpu/inorder/cpu.cc a47fd7c2d44e
src/cpu/o3/cpu.hh a47fd7c2d44e
src/cpu/o3/cpu.cc a47fd7c2d44e
src/cpu/o3/lsq.hh a47fd7c2d44e
src/cpu/o3/lsq_impl.hh a47fd7c2d44e
src/cpu/simple/atomic.hh a47fd7c2d44e
src/cpu/simple/timing.hh a47fd7c2d44e
src/cpu/simple/timing.cc a47fd7c2d44e
src/cpu/testers/directedtest/RubyDirectedTester.hh a47fd7c2d44e
src/cpu/testers/directedtest/RubyDirectedTester.cc a47fd7c2d44e
src/cpu/testers/memtest/memtest.hh a47fd7c2d44e
src/cpu/testers/memtest/memtest.cc a47fd7c2d44e
src/cpu/testers/networktest/networktest.hh a47fd7c2d44e
src/cpu/testers/networktest/networktest.cc a47fd7c2d44e
src/cpu/testers/rubytest/RubyTester.hh a47fd7c2d44e
src/cpu/testers/rubytest/RubyTester.cc a47fd7c2d44e
src/dev/io_device.hh a47fd7c2d44e
src/dev/io_device.cc a47fd7c2d44e
src/mem/bridge.hh a47fd7c2d44e
src/mem/bridge.cc a47fd7c2d44e
src/mem/bus.hh a47fd7c2d44e
src/mem/bus.cc a47fd7c2d44e
src/mem/cache/base.hh a47fd7c2d44e
src/mem/cache/cache.hh a47fd7c2d44e
src/mem/cache/cache_impl.hh a47fd7c2d44e
src/mem/mport.hh a47fd7c2d44e
src/mem/mport.cc a47fd7c2d44e
src/mem/packet_queue.hh a47fd7c2d44e
src/mem/packet_queue.cc a47fd7c2d44e
src/mem/port.hh a47fd7c2d44e
src/mem/port.cc a47fd7c2d44e
src/mem/ruby/system/RubyPort.hh a47fd7c2d44e
src/mem/ruby/system/RubyPort.cc a47fd7c2d44e
src/sim/system.hh a47fd7c2d44e
Diff: http://reviews.gem5.org/r/1118/diff/
Testing
-------
util/regress all passing (disregarding t1000 and eio)
Thanks,
Andreas Hansson
Andreas Hansson
2012-04-11 13:53:13 UTC
Permalink
Post by Andreas Hansson
Post by Steve Reinhardt
Overall looks good. I like your descriptions in the commit message... can you incorporate the bulk of that in a comment somewhere, or put it on the wiki? No need for the "before" part (that's useful for the changeset, but not as part of the stable documentation), but the middle few paragraphs that describe how things work need to be included somewhere where they're easier for future generations to find.
Thanks!
I have a document ready for updating the wiki, and this indeed part of it. Overall, we have prepared a decent amount of documentation that will go on the wiki as the functionality is committed, and this is probably a good time to start putting things in place.
Is there anything else I can do, or are you happy with the current version Steve?

The code duplication should be at a very minimum with the latest diff.


- Andreas


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/1118/#review2512
-----------------------------------------------------------
Post by Andreas Hansson
-----------------------------------------------------------
http://reviews.gem5.org/r/1118/
-----------------------------------------------------------
(Updated April 10, 2012, 11:25 a.m.)
Review request for Default.
Description
-------
MEM: Separate snoops and normal memory requests/responses
This patch introduces port access methods that separates snoop
request/responses from normal memory request/responses. The
differentiation is made for functional, atomic and timing accesses and
builds on the introduction of master and slave ports.
Before the introduction of this patch, the packets belonging to the
different phases of the protocol (request -> [forwarded snoop request
-> snoop response]* -> response) all use the same port access
functions, even though the snoop packets flow in the opposite
direction to the normal packet. That is, a coherent master sends
normal request and receives responses, but receives snoop requests and
sends snoop responses (vice versa for the slave). These two distinct
phases now use different access functions, as described below.
Starting with the functional access, a master sends a request to a
slave through sendFunctional, and the request packet is turned into a
response before the call returns. In a system without cache coherence,
this is all that is needed from the functional interface. For the
cache-coherent scenario, a slave also sends snoop requests to coherent
masters through sendFunctionalSnoop, with responses returned within
the same packet pointer. This is currently used by the bus and caches,
and the LSQ of the O3 CPU. The send/recvFunctional and
send/recvFunctionalSnoop are moved from the Port super class to the
appropriate subclass.
Atomic accesses follow the same flow as functional accesses, with
request being sent from master to slave through sendAtomic. In the
case of cache-coherent ports, a slave can send snoop requests to a
master through sendAtomicSnoop. Just as for the functional access
methods, the atomic send and receive member functions are moved to the
appropriate subclasses.
The timing access methods are different from the functional and atomic
in that requests and responses are separated in time and
send/recvTiming are used for both directions. Hence, a master uses
sendTiming to send a request to a slave, and a slave uses sendTiming
to send a response back to a master, at a later point in time. Snoop
requests and responses travel in the opposite direction, similar to
what happens in functional and atomic accesses. With the introduction
of this patch, it is possible to determine the direction of packets in
the bus, and no longer necessary to look for both a master and a slave
port with the requested port id.
In contrast to the normal recvFunctional, recvAtomic and recvTiming
that are pure virtual functions, the recvFunctionalSnoop,
recvAtomicSnoop and recvTimingSnoop have a default implementation that
calls panic. This is to allow non-coherent master and slave ports to
not implement these functions.
Diffs
-----
src/arch/x86/pagetable_walker.hh 5534a564f6a0
src/arch/x86/pagetable_walker.cc 5534a564f6a0
src/cpu/base.hh 5534a564f6a0
src/cpu/base.cc 5534a564f6a0
src/cpu/inorder/cpu.hh 5534a564f6a0
src/cpu/inorder/cpu.cc 5534a564f6a0
src/cpu/o3/cpu.hh 5534a564f6a0
src/cpu/o3/cpu.cc 5534a564f6a0
src/cpu/o3/lsq.hh 5534a564f6a0
src/cpu/o3/lsq_impl.hh 5534a564f6a0
src/cpu/simple/atomic.hh 5534a564f6a0
src/cpu/simple/timing.hh 5534a564f6a0
src/cpu/simple/timing.cc 5534a564f6a0
src/cpu/testers/directedtest/RubyDirectedTester.hh 5534a564f6a0
src/cpu/testers/directedtest/RubyDirectedTester.cc 5534a564f6a0
src/cpu/testers/memtest/memtest.hh 5534a564f6a0
src/cpu/testers/memtest/memtest.cc 5534a564f6a0
src/cpu/testers/networktest/networktest.hh 5534a564f6a0
src/cpu/testers/networktest/networktest.cc 5534a564f6a0
src/cpu/testers/rubytest/RubyTester.hh 5534a564f6a0
src/cpu/testers/rubytest/RubyTester.cc 5534a564f6a0
src/dev/io_device.hh 5534a564f6a0
src/dev/io_device.cc 5534a564f6a0
src/mem/bridge.hh 5534a564f6a0
src/mem/bridge.cc 5534a564f6a0
src/mem/bus.hh 5534a564f6a0
src/mem/bus.cc 5534a564f6a0
src/mem/cache/base.hh 5534a564f6a0
src/mem/cache/cache.hh 5534a564f6a0
src/mem/cache/cache_impl.hh 5534a564f6a0
src/mem/mport.hh 5534a564f6a0
src/mem/mport.cc 5534a564f6a0
src/mem/packet_queue.hh 5534a564f6a0
src/mem/packet_queue.cc 5534a564f6a0
src/mem/port.hh 5534a564f6a0
src/mem/port.cc 5534a564f6a0
src/mem/ruby/system/RubyPort.hh 5534a564f6a0
src/mem/ruby/system/RubyPort.cc 5534a564f6a0
src/sim/system.hh 5534a564f6a0
Diff: http://reviews.gem5.org/r/1118/diff/
Testing
-------
util/regress all passing (disregarding t1000 and eio)
Thanks,
Andreas Hansson
Andreas Hansson
2012-04-09 15:44:03 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/1118/
-----------------------------------------------------------

(Updated April 9, 2012, 8:44 a.m.)


Review request for Default.


Description
-------

MEM: Separate snoops and normal memory requests/responses

This patch introduces port access methods that separates snoop
request/responses from normal memory request/responses. The
differentiation is made for functional, atomic and timing accesses and
builds on the introduction of master and slave ports.

Before the introduction of this patch, the packets belonging to the
different phases of the protocol (request -> [forwarded snoop request
-> snoop response]* -> response) all use the same port access
functions, even though the snoop packets flow in the opposite
direction to the normal packet. That is, a coherent master sends
normal request and receives responses, but receives snoop requests and
sends snoop responses (vice versa for the slave). These two distinct
phases now use different access functions, as described below.

Starting with the functional access, a master sends a request to a
slave through sendFunctional, and the request packet is turned into a
response before the call returns. In a system without cache coherence,
this is all that is needed from the functional interface. For the
cache-coherent scenario, a slave also sends snoop requests to coherent
masters through sendFunctionalSnoop, with responses returned within
the same packet pointer. This is currently used by the bus and caches,
and the LSQ of the O3 CPU. The send/recvFunctional and
send/recvFunctionalSnoop are moved from the Port super class to the
appropriate subclass.

Atomic accesses follow the same flow as functional accesses, with
request being sent from master to slave through sendAtomic. In the
case of cache-coherent ports, a slave can send snoop requests to a
master through sendAtomicSnoop. Just as for the functional access
methods, the atomic send and receive member functions are moved to the
appropriate subclasses.

The timing access methods are different from the functional and atomic
in that requests and responses are separated in time and
send/recvTiming are used for both directions. Hence, a master uses
sendTiming to send a request to a slave, and a slave uses sendTiming
to send a response back to a master, at a later point in time. Snoop
requests and responses travel in the opposite direction, similar to
what happens in functional and atomic accesses. With the introduction
of this patch, it is possible to determine the direction of packets in
the bus, and no longer necessary to look for both a master and a slave
port with the requested port id.

In contrast to the normal recvFunctional, recvAtomic and recvTiming
that are pure virtual functions, the recvFunctionalSnoop,
recvAtomicSnoop and recvTimingSnoop have a default implementation that
calls panic. This is to allow non-coherent master and slave ports to
not implement these functions.


Diffs (updated)
-----

src/arch/x86/pagetable_walker.hh a47fd7c2d44e
src/arch/x86/pagetable_walker.cc a47fd7c2d44e
src/cpu/base.hh a47fd7c2d44e
src/cpu/base.cc a47fd7c2d44e
src/cpu/inorder/cpu.hh a47fd7c2d44e
src/cpu/inorder/cpu.cc a47fd7c2d44e
src/cpu/o3/cpu.hh a47fd7c2d44e
src/cpu/o3/cpu.cc a47fd7c2d44e
src/cpu/o3/lsq.hh a47fd7c2d44e
src/cpu/o3/lsq_impl.hh a47fd7c2d44e
src/cpu/simple/atomic.hh a47fd7c2d44e
src/cpu/simple/timing.hh a47fd7c2d44e
src/cpu/simple/timing.cc a47fd7c2d44e
src/cpu/testers/directedtest/RubyDirectedTester.hh a47fd7c2d44e
src/cpu/testers/directedtest/RubyDirectedTester.cc a47fd7c2d44e
src/cpu/testers/memtest/memtest.hh a47fd7c2d44e
src/cpu/testers/memtest/memtest.cc a47fd7c2d44e
src/cpu/testers/networktest/networktest.hh a47fd7c2d44e
src/cpu/testers/networktest/networktest.cc a47fd7c2d44e
src/cpu/testers/rubytest/RubyTester.hh a47fd7c2d44e
src/cpu/testers/rubytest/RubyTester.cc a47fd7c2d44e
src/dev/io_device.hh a47fd7c2d44e
src/dev/io_device.cc a47fd7c2d44e
src/mem/bridge.hh a47fd7c2d44e
src/mem/bridge.cc a47fd7c2d44e
src/mem/bus.hh a47fd7c2d44e
src/mem/bus.cc a47fd7c2d44e
src/mem/cache/base.hh a47fd7c2d44e
src/mem/cache/cache.hh a47fd7c2d44e
src/mem/cache/cache_impl.hh a47fd7c2d44e
src/mem/mport.hh a47fd7c2d44e
src/mem/mport.cc a47fd7c2d44e
src/mem/packet_queue.hh a47fd7c2d44e
src/mem/packet_queue.cc a47fd7c2d44e
src/mem/port.hh a47fd7c2d44e
src/mem/port.cc a47fd7c2d44e
src/mem/ruby/system/RubyPort.hh a47fd7c2d44e
src/mem/ruby/system/RubyPort.cc a47fd7c2d44e
src/sim/system.hh a47fd7c2d44e

Diff: http://reviews.gem5.org/r/1118/diff/


Testing
-------

util/regress all passing (disregarding t1000 and eio)


Thanks,

Andreas Hansson
Andreas Hansson
2012-04-10 17:45:36 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/1118/
-----------------------------------------------------------

(Updated April 10, 2012, 10:45 a.m.)


Review request for Default.


Description
-------

MEM: Separate snoops and normal memory requests/responses

This patch introduces port access methods that separates snoop
request/responses from normal memory request/responses. The
differentiation is made for functional, atomic and timing accesses and
builds on the introduction of master and slave ports.

Before the introduction of this patch, the packets belonging to the
different phases of the protocol (request -> [forwarded snoop request
-> snoop response]* -> response) all use the same port access
functions, even though the snoop packets flow in the opposite
direction to the normal packet. That is, a coherent master sends
normal request and receives responses, but receives snoop requests and
sends snoop responses (vice versa for the slave). These two distinct
phases now use different access functions, as described below.

Starting with the functional access, a master sends a request to a
slave through sendFunctional, and the request packet is turned into a
response before the call returns. In a system without cache coherence,
this is all that is needed from the functional interface. For the
cache-coherent scenario, a slave also sends snoop requests to coherent
masters through sendFunctionalSnoop, with responses returned within
the same packet pointer. This is currently used by the bus and caches,
and the LSQ of the O3 CPU. The send/recvFunctional and
send/recvFunctionalSnoop are moved from the Port super class to the
appropriate subclass.

Atomic accesses follow the same flow as functional accesses, with
request being sent from master to slave through sendAtomic. In the
case of cache-coherent ports, a slave can send snoop requests to a
master through sendAtomicSnoop. Just as for the functional access
methods, the atomic send and receive member functions are moved to the
appropriate subclasses.

The timing access methods are different from the functional and atomic
in that requests and responses are separated in time and
send/recvTiming are used for both directions. Hence, a master uses
sendTiming to send a request to a slave, and a slave uses sendTiming
to send a response back to a master, at a later point in time. Snoop
requests and responses travel in the opposite direction, similar to
what happens in functional and atomic accesses. With the introduction
of this patch, it is possible to determine the direction of packets in
the bus, and no longer necessary to look for both a master and a slave
port with the requested port id.

In contrast to the normal recvFunctional, recvAtomic and recvTiming
that are pure virtual functions, the recvFunctionalSnoop,
recvAtomicSnoop and recvTimingSnoop have a default implementation that
calls panic. This is to allow non-coherent master and slave ports to
not implement these functions.


Diffs (updated)
-----

src/arch/x86/pagetable_walker.hh 5534a564f6a0
src/arch/x86/pagetable_walker.cc 5534a564f6a0
src/cpu/base.hh 5534a564f6a0
src/cpu/base.cc 5534a564f6a0
src/cpu/inorder/cpu.hh 5534a564f6a0
src/cpu/inorder/cpu.cc 5534a564f6a0
src/cpu/o3/cpu.hh 5534a564f6a0
src/cpu/o3/cpu.cc 5534a564f6a0
src/cpu/o3/lsq.hh 5534a564f6a0
src/cpu/o3/lsq_impl.hh 5534a564f6a0
src/cpu/simple/atomic.hh 5534a564f6a0
src/cpu/simple/timing.hh 5534a564f6a0
src/cpu/simple/timing.cc 5534a564f6a0
src/cpu/testers/directedtest/RubyDirectedTester.hh 5534a564f6a0
src/cpu/testers/directedtest/RubyDirectedTester.cc 5534a564f6a0
src/cpu/testers/memtest/memtest.hh 5534a564f6a0
src/cpu/testers/memtest/memtest.cc 5534a564f6a0
src/cpu/testers/networktest/networktest.hh 5534a564f6a0
src/cpu/testers/networktest/networktest.cc 5534a564f6a0
src/cpu/testers/rubytest/RubyTester.hh 5534a564f6a0
src/cpu/testers/rubytest/RubyTester.cc 5534a564f6a0
src/dev/io_device.hh 5534a564f6a0
src/dev/io_device.cc 5534a564f6a0
src/mem/bridge.hh 5534a564f6a0
src/mem/bridge.cc 5534a564f6a0
src/mem/bus.hh 5534a564f6a0
src/mem/bus.cc 5534a564f6a0
src/mem/cache/base.hh 5534a564f6a0
src/mem/cache/cache.hh 5534a564f6a0
src/mem/cache/cache_impl.hh 5534a564f6a0
src/mem/mport.hh 5534a564f6a0
src/mem/mport.cc 5534a564f6a0
src/mem/packet_queue.hh 5534a564f6a0
src/mem/packet_queue.cc 5534a564f6a0
src/mem/port.hh 5534a564f6a0
src/mem/port.cc 5534a564f6a0
src/mem/ruby/system/RubyPort.hh 5534a564f6a0
src/mem/ruby/system/RubyPort.cc 5534a564f6a0
src/sim/system.hh 5534a564f6a0

Diff: http://reviews.gem5.org/r/1118/diff/


Testing
-------

util/regress all passing (disregarding t1000 and eio)


Thanks,

Andreas Hansson
Andreas Hansson
2012-04-10 18:12:34 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/1118/
-----------------------------------------------------------

(Updated April 10, 2012, 11:12 a.m.)


Review request for Default.


Description
-------

MEM: Separate snoops and normal memory requests/responses

This patch introduces port access methods that separates snoop
request/responses from normal memory request/responses. The
differentiation is made for functional, atomic and timing accesses and
builds on the introduction of master and slave ports.

Before the introduction of this patch, the packets belonging to the
different phases of the protocol (request -> [forwarded snoop request
-> snoop response]* -> response) all use the same port access
functions, even though the snoop packets flow in the opposite
direction to the normal packet. That is, a coherent master sends
normal request and receives responses, but receives snoop requests and
sends snoop responses (vice versa for the slave). These two distinct
phases now use different access functions, as described below.

Starting with the functional access, a master sends a request to a
slave through sendFunctional, and the request packet is turned into a
response before the call returns. In a system without cache coherence,
this is all that is needed from the functional interface. For the
cache-coherent scenario, a slave also sends snoop requests to coherent
masters through sendFunctionalSnoop, with responses returned within
the same packet pointer. This is currently used by the bus and caches,
and the LSQ of the O3 CPU. The send/recvFunctional and
send/recvFunctionalSnoop are moved from the Port super class to the
appropriate subclass.

Atomic accesses follow the same flow as functional accesses, with
request being sent from master to slave through sendAtomic. In the
case of cache-coherent ports, a slave can send snoop requests to a
master through sendAtomicSnoop. Just as for the functional access
methods, the atomic send and receive member functions are moved to the
appropriate subclasses.

The timing access methods are different from the functional and atomic
in that requests and responses are separated in time and
send/recvTiming are used for both directions. Hence, a master uses
sendTiming to send a request to a slave, and a slave uses sendTiming
to send a response back to a master, at a later point in time. Snoop
requests and responses travel in the opposite direction, similar to
what happens in functional and atomic accesses. With the introduction
of this patch, it is possible to determine the direction of packets in
the bus, and no longer necessary to look for both a master and a slave
port with the requested port id.

In contrast to the normal recvFunctional, recvAtomic and recvTiming
that are pure virtual functions, the recvFunctionalSnoop,
recvAtomicSnoop and recvTimingSnoop have a default implementation that
calls panic. This is to allow non-coherent master and slave ports to
not implement these functions.


Diffs (updated)
-----

src/arch/x86/pagetable_walker.hh 5534a564f6a0
src/arch/x86/pagetable_walker.cc 5534a564f6a0
src/cpu/base.hh 5534a564f6a0
src/cpu/base.cc 5534a564f6a0
src/cpu/inorder/cpu.hh 5534a564f6a0
src/cpu/inorder/cpu.cc 5534a564f6a0
src/cpu/o3/cpu.hh 5534a564f6a0
src/cpu/o3/cpu.cc 5534a564f6a0
src/cpu/o3/lsq.hh 5534a564f6a0
src/cpu/o3/lsq_impl.hh 5534a564f6a0
src/cpu/simple/atomic.hh 5534a564f6a0
src/cpu/simple/timing.hh 5534a564f6a0
src/cpu/simple/timing.cc 5534a564f6a0
src/cpu/testers/directedtest/RubyDirectedTester.hh 5534a564f6a0
src/cpu/testers/directedtest/RubyDirectedTester.cc 5534a564f6a0
src/cpu/testers/memtest/memtest.hh 5534a564f6a0
src/cpu/testers/memtest/memtest.cc 5534a564f6a0
src/cpu/testers/networktest/networktest.hh 5534a564f6a0
src/cpu/testers/networktest/networktest.cc 5534a564f6a0
src/cpu/testers/rubytest/RubyTester.hh 5534a564f6a0
src/cpu/testers/rubytest/RubyTester.cc 5534a564f6a0
src/dev/io_device.hh 5534a564f6a0
src/dev/io_device.cc 5534a564f6a0
src/mem/bridge.hh 5534a564f6a0
src/mem/bridge.cc 5534a564f6a0
src/mem/bus.hh 5534a564f6a0
src/mem/bus.cc 5534a564f6a0
src/mem/cache/base.hh 5534a564f6a0
src/mem/cache/cache.hh 5534a564f6a0
src/mem/cache/cache_impl.hh 5534a564f6a0
src/mem/mport.hh 5534a564f6a0
src/mem/mport.cc 5534a564f6a0
src/mem/packet_queue.hh 5534a564f6a0
src/mem/packet_queue.cc 5534a564f6a0
src/mem/port.hh 5534a564f6a0
src/mem/port.cc 5534a564f6a0
src/mem/ruby/system/RubyPort.hh 5534a564f6a0
src/mem/ruby/system/RubyPort.cc 5534a564f6a0
src/sim/system.hh 5534a564f6a0

Diff: http://reviews.gem5.org/r/1118/diff/


Testing
-------

util/regress all passing (disregarding t1000 and eio)


Thanks,

Andreas Hansson
Andreas Hansson
2012-04-10 18:25:11 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/1118/
-----------------------------------------------------------

(Updated April 10, 2012, 11:25 a.m.)


Review request for Default.


Description
-------

MEM: Separate snoops and normal memory requests/responses

This patch introduces port access methods that separates snoop
request/responses from normal memory request/responses. The
differentiation is made for functional, atomic and timing accesses and
builds on the introduction of master and slave ports.

Before the introduction of this patch, the packets belonging to the
different phases of the protocol (request -> [forwarded snoop request
-> snoop response]* -> response) all use the same port access
functions, even though the snoop packets flow in the opposite
direction to the normal packet. That is, a coherent master sends
normal request and receives responses, but receives snoop requests and
sends snoop responses (vice versa for the slave). These two distinct
phases now use different access functions, as described below.

Starting with the functional access, a master sends a request to a
slave through sendFunctional, and the request packet is turned into a
response before the call returns. In a system without cache coherence,
this is all that is needed from the functional interface. For the
cache-coherent scenario, a slave also sends snoop requests to coherent
masters through sendFunctionalSnoop, with responses returned within
the same packet pointer. This is currently used by the bus and caches,
and the LSQ of the O3 CPU. The send/recvFunctional and
send/recvFunctionalSnoop are moved from the Port super class to the
appropriate subclass.

Atomic accesses follow the same flow as functional accesses, with
request being sent from master to slave through sendAtomic. In the
case of cache-coherent ports, a slave can send snoop requests to a
master through sendAtomicSnoop. Just as for the functional access
methods, the atomic send and receive member functions are moved to the
appropriate subclasses.

The timing access methods are different from the functional and atomic
in that requests and responses are separated in time and
send/recvTiming are used for both directions. Hence, a master uses
sendTiming to send a request to a slave, and a slave uses sendTiming
to send a response back to a master, at a later point in time. Snoop
requests and responses travel in the opposite direction, similar to
what happens in functional and atomic accesses. With the introduction
of this patch, it is possible to determine the direction of packets in
the bus, and no longer necessary to look for both a master and a slave
port with the requested port id.

In contrast to the normal recvFunctional, recvAtomic and recvTiming
that are pure virtual functions, the recvFunctionalSnoop,
recvAtomicSnoop and recvTimingSnoop have a default implementation that
calls panic. This is to allow non-coherent master and slave ports to
not implement these functions.


Diffs (updated)
-----

src/arch/x86/pagetable_walker.hh 5534a564f6a0
src/arch/x86/pagetable_walker.cc 5534a564f6a0
src/cpu/base.hh 5534a564f6a0
src/cpu/base.cc 5534a564f6a0
src/cpu/inorder/cpu.hh 5534a564f6a0
src/cpu/inorder/cpu.cc 5534a564f6a0
src/cpu/o3/cpu.hh 5534a564f6a0
src/cpu/o3/cpu.cc 5534a564f6a0
src/cpu/o3/lsq.hh 5534a564f6a0
src/cpu/o3/lsq_impl.hh 5534a564f6a0
src/cpu/simple/atomic.hh 5534a564f6a0
src/cpu/simple/timing.hh 5534a564f6a0
src/cpu/simple/timing.cc 5534a564f6a0
src/cpu/testers/directedtest/RubyDirectedTester.hh 5534a564f6a0
src/cpu/testers/directedtest/RubyDirectedTester.cc 5534a564f6a0
src/cpu/testers/memtest/memtest.hh 5534a564f6a0
src/cpu/testers/memtest/memtest.cc 5534a564f6a0
src/cpu/testers/networktest/networktest.hh 5534a564f6a0
src/cpu/testers/networktest/networktest.cc 5534a564f6a0
src/cpu/testers/rubytest/RubyTester.hh 5534a564f6a0
src/cpu/testers/rubytest/RubyTester.cc 5534a564f6a0
src/dev/io_device.hh 5534a564f6a0
src/dev/io_device.cc 5534a564f6a0
src/mem/bridge.hh 5534a564f6a0
src/mem/bridge.cc 5534a564f6a0
src/mem/bus.hh 5534a564f6a0
src/mem/bus.cc 5534a564f6a0
src/mem/cache/base.hh 5534a564f6a0
src/mem/cache/cache.hh 5534a564f6a0
src/mem/cache/cache_impl.hh 5534a564f6a0
src/mem/mport.hh 5534a564f6a0
src/mem/mport.cc 5534a564f6a0
src/mem/packet_queue.hh 5534a564f6a0
src/mem/packet_queue.cc 5534a564f6a0
src/mem/port.hh 5534a564f6a0
src/mem/port.cc 5534a564f6a0
src/mem/ruby/system/RubyPort.hh 5534a564f6a0
src/mem/ruby/system/RubyPort.cc 5534a564f6a0
src/sim/system.hh 5534a564f6a0

Diff: http://reviews.gem5.org/r/1118/diff/


Testing
-------

util/regress all passing (disregarding t1000 and eio)


Thanks,

Andreas Hansson
Andreas Hansson
2012-04-12 20:08:43 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/1118/
-----------------------------------------------------------

(Updated April 12, 2012, 1:08 p.m.)


Review request for Default.


Description
-------

MEM: Separate snoops and normal memory requests/responses

This patch introduces port access methods that separates snoop
request/responses from normal memory request/responses. The
differentiation is made for functional, atomic and timing accesses and
builds on the introduction of master and slave ports.

Before the introduction of this patch, the packets belonging to the
different phases of the protocol (request -> [forwarded snoop request
-> snoop response]* -> response) all use the same port access
functions, even though the snoop packets flow in the opposite
direction to the normal packet. That is, a coherent master sends
normal request and receives responses, but receives snoop requests and
sends snoop responses (vice versa for the slave). These two distinct
phases now use different access functions, as described below.

Starting with the functional access, a master sends a request to a
slave through sendFunctional, and the request packet is turned into a
response before the call returns. In a system without cache coherence,
this is all that is needed from the functional interface. For the
cache-coherent scenario, a slave also sends snoop requests to coherent
masters through sendFunctionalSnoop, with responses returned within
the same packet pointer. This is currently used by the bus and caches,
and the LSQ of the O3 CPU. The send/recvFunctional and
send/recvFunctionalSnoop are moved from the Port super class to the
appropriate subclass.

Atomic accesses follow the same flow as functional accesses, with
request being sent from master to slave through sendAtomic. In the
case of cache-coherent ports, a slave can send snoop requests to a
master through sendAtomicSnoop. Just as for the functional access
methods, the atomic send and receive member functions are moved to the
appropriate subclasses.

The timing access methods are different from the functional and atomic
in that requests and responses are separated in time and
send/recvTiming are used for both directions. Hence, a master uses
sendTiming to send a request to a slave, and a slave uses sendTiming
to send a response back to a master, at a later point in time. Snoop
requests and responses travel in the opposite direction, similar to
what happens in functional and atomic accesses. With the introduction
of this patch, it is possible to determine the direction of packets in
the bus, and no longer necessary to look for both a master and a slave
port with the requested port id.

In contrast to the normal recvFunctional, recvAtomic and recvTiming
that are pure virtual functions, the recvFunctionalSnoop,
recvAtomicSnoop and recvTimingSnoop have a default implementation that
calls panic. This is to allow non-coherent master and slave ports to
not implement these functions.


Diffs (updated)
-----

src/arch/x86/pagetable_walker.hh d062cc7a8bdf
src/arch/x86/pagetable_walker.cc d062cc7a8bdf
src/cpu/base.hh d062cc7a8bdf
src/cpu/base.cc d062cc7a8bdf
src/cpu/inorder/cpu.hh d062cc7a8bdf
src/cpu/inorder/cpu.cc d062cc7a8bdf
src/cpu/o3/cpu.hh d062cc7a8bdf
src/cpu/o3/cpu.cc d062cc7a8bdf
src/cpu/o3/lsq.hh d062cc7a8bdf
src/cpu/o3/lsq_impl.hh d062cc7a8bdf
src/cpu/simple/atomic.hh d062cc7a8bdf
src/cpu/simple/timing.hh d062cc7a8bdf
src/cpu/simple/timing.cc d062cc7a8bdf
src/cpu/testers/directedtest/RubyDirectedTester.hh d062cc7a8bdf
src/cpu/testers/directedtest/RubyDirectedTester.cc d062cc7a8bdf
src/cpu/testers/memtest/memtest.hh d062cc7a8bdf
src/cpu/testers/memtest/memtest.cc d062cc7a8bdf
src/cpu/testers/networktest/networktest.hh d062cc7a8bdf
src/cpu/testers/networktest/networktest.cc d062cc7a8bdf
src/cpu/testers/rubytest/RubyTester.hh d062cc7a8bdf
src/cpu/testers/rubytest/RubyTester.cc d062cc7a8bdf
src/dev/io_device.hh d062cc7a8bdf
src/dev/io_device.cc d062cc7a8bdf
src/mem/bridge.hh d062cc7a8bdf
src/mem/bridge.cc d062cc7a8bdf
src/mem/bus.hh d062cc7a8bdf
src/mem/bus.cc d062cc7a8bdf
src/mem/cache/base.hh d062cc7a8bdf
src/mem/cache/cache.hh d062cc7a8bdf
src/mem/cache/cache_impl.hh d062cc7a8bdf
src/mem/mport.hh d062cc7a8bdf
src/mem/mport.cc d062cc7a8bdf
src/mem/packet_queue.hh d062cc7a8bdf
src/mem/packet_queue.cc d062cc7a8bdf
src/mem/port.hh d062cc7a8bdf
src/mem/port.cc d062cc7a8bdf
src/mem/ruby/system/RubyPort.hh d062cc7a8bdf
src/mem/ruby/system/RubyPort.cc d062cc7a8bdf
src/sim/system.hh d062cc7a8bdf

Diff: http://reviews.gem5.org/r/1118/diff/


Testing
-------

util/regress all passing (disregarding t1000 and eio)


Thanks,

Andreas Hansson
Steve Reinhardt
2012-04-13 21:58:01 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/1118/#review2540
-----------------------------------------------------------

Ship it!


Looks good! Thanks for all the changes.

- Steve Reinhardt
Post by Andreas Hansson
-----------------------------------------------------------
http://reviews.gem5.org/r/1118/
-----------------------------------------------------------
(Updated April 12, 2012, 1:08 p.m.)
Review request for Default.
Description
-------
MEM: Separate snoops and normal memory requests/responses
This patch introduces port access methods that separates snoop
request/responses from normal memory request/responses. The
differentiation is made for functional, atomic and timing accesses and
builds on the introduction of master and slave ports.
Before the introduction of this patch, the packets belonging to the
different phases of the protocol (request -> [forwarded snoop request
-> snoop response]* -> response) all use the same port access
functions, even though the snoop packets flow in the opposite
direction to the normal packet. That is, a coherent master sends
normal request and receives responses, but receives snoop requests and
sends snoop responses (vice versa for the slave). These two distinct
phases now use different access functions, as described below.
Starting with the functional access, a master sends a request to a
slave through sendFunctional, and the request packet is turned into a
response before the call returns. In a system without cache coherence,
this is all that is needed from the functional interface. For the
cache-coherent scenario, a slave also sends snoop requests to coherent
masters through sendFunctionalSnoop, with responses returned within
the same packet pointer. This is currently used by the bus and caches,
and the LSQ of the O3 CPU. The send/recvFunctional and
send/recvFunctionalSnoop are moved from the Port super class to the
appropriate subclass.
Atomic accesses follow the same flow as functional accesses, with
request being sent from master to slave through sendAtomic. In the
case of cache-coherent ports, a slave can send snoop requests to a
master through sendAtomicSnoop. Just as for the functional access
methods, the atomic send and receive member functions are moved to the
appropriate subclasses.
The timing access methods are different from the functional and atomic
in that requests and responses are separated in time and
send/recvTiming are used for both directions. Hence, a master uses
sendTiming to send a request to a slave, and a slave uses sendTiming
to send a response back to a master, at a later point in time. Snoop
requests and responses travel in the opposite direction, similar to
what happens in functional and atomic accesses. With the introduction
of this patch, it is possible to determine the direction of packets in
the bus, and no longer necessary to look for both a master and a slave
port with the requested port id.
In contrast to the normal recvFunctional, recvAtomic and recvTiming
that are pure virtual functions, the recvFunctionalSnoop,
recvAtomicSnoop and recvTimingSnoop have a default implementation that
calls panic. This is to allow non-coherent master and slave ports to
not implement these functions.
Diffs
-----
src/arch/x86/pagetable_walker.hh d062cc7a8bdf
src/arch/x86/pagetable_walker.cc d062cc7a8bdf
src/cpu/base.hh d062cc7a8bdf
src/cpu/base.cc d062cc7a8bdf
src/cpu/inorder/cpu.hh d062cc7a8bdf
src/cpu/inorder/cpu.cc d062cc7a8bdf
src/cpu/o3/cpu.hh d062cc7a8bdf
src/cpu/o3/cpu.cc d062cc7a8bdf
src/cpu/o3/lsq.hh d062cc7a8bdf
src/cpu/o3/lsq_impl.hh d062cc7a8bdf
src/cpu/simple/atomic.hh d062cc7a8bdf
src/cpu/simple/timing.hh d062cc7a8bdf
src/cpu/simple/timing.cc d062cc7a8bdf
src/cpu/testers/directedtest/RubyDirectedTester.hh d062cc7a8bdf
src/cpu/testers/directedtest/RubyDirectedTester.cc d062cc7a8bdf
src/cpu/testers/memtest/memtest.hh d062cc7a8bdf
src/cpu/testers/memtest/memtest.cc d062cc7a8bdf
src/cpu/testers/networktest/networktest.hh d062cc7a8bdf
src/cpu/testers/networktest/networktest.cc d062cc7a8bdf
src/cpu/testers/rubytest/RubyTester.hh d062cc7a8bdf
src/cpu/testers/rubytest/RubyTester.cc d062cc7a8bdf
src/dev/io_device.hh d062cc7a8bdf
src/dev/io_device.cc d062cc7a8bdf
src/mem/bridge.hh d062cc7a8bdf
src/mem/bridge.cc d062cc7a8bdf
src/mem/bus.hh d062cc7a8bdf
src/mem/bus.cc d062cc7a8bdf
src/mem/cache/base.hh d062cc7a8bdf
src/mem/cache/cache.hh d062cc7a8bdf
src/mem/cache/cache_impl.hh d062cc7a8bdf
src/mem/mport.hh d062cc7a8bdf
src/mem/mport.cc d062cc7a8bdf
src/mem/packet_queue.hh d062cc7a8bdf
src/mem/packet_queue.cc d062cc7a8bdf
src/mem/port.hh d062cc7a8bdf
src/mem/port.cc d062cc7a8bdf
src/mem/ruby/system/RubyPort.hh d062cc7a8bdf
src/mem/ruby/system/RubyPort.cc d062cc7a8bdf
src/sim/system.hh d062cc7a8bdf
Diff: http://reviews.gem5.org/r/1118/diff/
Testing
-------
util/regress all passing (disregarding t1000 and eio)
Thanks,
Andreas Hansson
Loading...