> On April 8, 2012, 9:22 p.m., Steve Reinhardt wrote: > > 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. > On April 8, 2012, 9:22 p.m., Steve Reinhardt wrote: > > 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? > On April 8, 2012, 9:22 p.m., Steve Reinhardt wrote: > > 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. > On April 8, 2012, 9:22 p.m., Steve Reinhardt wrote: > > 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. > On April 8, 2012, 9:22 p.m., Steve Reinhardt wrote: > > 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()). > > > > 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... > > 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 ----------------------------------------------------------- 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
