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