----------------------------------------------------------- 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 On April 5, 2012, 9:57 a.m., Andreas Hansson wrote: > > ----------------------------------------------------------- > 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 > ----- > > 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 > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
