Hey Brad, It may not solve the multi-cache block problem, but it does get us closer to a working backing store ;).
Do you see a simple change to this patch to get multi-cache block DMA working? I don't have a program to test this with, so I'm not sure if I'm the right person to do it. Does it make sense to you to push this patch as is, then work on either a) another patch for multi-cache block support or b) a re-architecting of Ruby's backing store? Thanks, Jason On Tue Feb 10 2015 at 2:12:37 AM Andreas Hansson via gem5-dev < [email protected]> wrote: > Hi all, > > To me it sounds like the whole ruby philosophy of “two, or even three > views of the same data” might need an overhaul. > > Andreas > > On 10/02/2015 04:44, "Beckmann, Brad via gem5-dev" <[email protected]> > wrote: > > >Thanks Jason. I didn't notice your patch until after I sent out my > >email. It looks like we are encountering the same problem. > > > >I'm a bit concern that your patch doesn't modify > >DMASequencer::ackCallback(), thus it doesn't get us very close to > >correctly supporting multi-cache block dma. It seems like we need a > >fundamental different approach that separates updating the backing store > >from sending the response packet to the dma device. > > > >Brad > > > > > > > >-----Original Message----- > >From: gem5-dev [mailto:[email protected]] On Behalf Of Jason > >Power via gem5-dev > >Sent: Monday, February 09, 2015 6:06 PM > >To: gem5 Developer List; [email protected] > >Subject: Re: [gem5-dev] changeset in gem5: ruby: dma sequencer: remove > >RubyPort as paren... > > > >Hey Brad, > > > >I think this change is in my patch to fix the backing store. It's on > >reviewboard now. > >http://reviews.gem5.org/r/2627/ > > > >I'm not sure if that patch supports multi cache block DMA, but it's > >definitely a step in the right direction. > > > >There's also another patch for unaligned DMA. > >http://reviews.gem5.org/r/2653/. That patch was required for us to get > >our copy engine working. > > > > > > > >Let me know if you have any feedback or if it solves your problem. > > > >Cheers, > > > > > >Jason > > > >On Mon Feb 09 2015 at 7:03:54 PM Beckmann, Brad via gem5-dev < > >[email protected]> wrote: > > > >I should clarify. Is the simple way of supporting the backing store and > >DMA is adding those 6 lines back to the DMA sequencer? > > > >Also have you considered the case for multi-cache block DMA and updating > >the backing store? It appears that only the final cache block of a large > >multi-cache block write will even call > >DMASequencer::MemSlavePort::hitCallback(). How can we get the other DMA > >writes to update the backing store? > > > >Thanks, > > > >Brad > > > >-----Original Message----- > >From: gem5-dev [mailto:[email protected]] On Behalf Of Beckmann, > >Brad via gem5-dev > >Sent: Monday, February 09, 2015 4:58 PM > >To: gem5 Developer List; [email protected] > >Subject: Re: [gem5-dev] changeset in gem5: ruby: dma sequencer: remove > >RubyPort as paren... > > > >Hi Nilay, > > > >Did you consider this patch when you added the backing store back to Ruby? > >The following lines in "DMASequencer::MemSlavePort::hitCallback(PacketPtr > >pkt)" initially updated the backing store, but I believe they were > >removed in a later patch (7a3ad4b09ce4). > > > >+ if (accessPhysMem) { > >+ DMASequencer *seq = static_cast<DMASequencer *>(&owner); > >+ seq->system->getPhysMem().access(pkt); > >+ } else if (needsResponse) { > >+ pkt->makeResponse(); > >+ } > >+ > > > >Is it as simple as adding those 6 lines back into the DMA sequencer? Are > >there other issues we should consider? > > > >Thanks, > > > >Brad > > > >-----Original Message----- > >From: gem5-dev [mailto:[email protected]] On Behalf Of Nilay > >Vaish via gem5-dev > >Sent: Thursday, November 06, 2014 3:37 AM > >To: [email protected] > >Subject: [gem5-dev] changeset in gem5: ruby: dma sequencer: remove > >RubyPort as paren... > > > >changeset 30e3715c9405 in /z/repo/gem5 > >details: http://repo.gem5.org/gem5?cmd=changeset;node=30e3715c9405 > >description: > > ruby: dma sequencer: remove RubyPort as parent class > > As of now DMASequencer inherits from the RubyPort class. But the > >code in > > RubyPort class is heavily tailored for the CPU Sequencer. There > >are parts of > > the code that are not required at all for the DMA sequencer. > >Moreover, the > > next patch uses the dma sequencer for carrying out memory > >accesses for all the > > io devices. Hence, it is better to have a leaner dma sequencer. > > > >diffstat: > > > > src/mem/ruby/system/DMASequencer.cc | 195 > >+++++++++++++++++++++++++++++++++++- > > src/mem/ruby/system/DMASequencer.hh | 75 +++++++++++++- > > src/mem/ruby/system/Sequencer.py | 13 +- > > 3 files changed, 274 insertions(+), 9 deletions(-) > > > >diffs (truncated from 374 to 300 lines): > > > >diff -r ba51f8572571 -r 30e3715c9405 src/mem/ruby/system/DMASequencer.cc > >--- a/src/mem/ruby/system/DMASequencer.cc Mon Nov 03 10:14:42 2014 > >-0600 > >+++ b/src/mem/ruby/system/DMASequencer.cc Thu Nov 06 00:55:09 2014 > >-0600 > >@@ -28,26 +28,212 @@ > > > > #include <memory> > > > >+#include "debug/Config.hh" > >+#include "debug/Drain.hh" > > #include "debug/RubyDma.hh" > > #include "debug/RubyStats.hh" > > #include "mem/protocol/SequencerMsg.hh" > >-#include "mem/protocol/SequencerRequestType.hh" > > #include "mem/ruby/system/DMASequencer.hh" > > #include "mem/ruby/system/System.hh" > >+#include "sim/system.hh" > > > > DMASequencer::DMASequencer(const Params *p) > >- : RubyPort(p) > >+ : MemObject(p), m_version(p->version), m_controller(NULL), > >+ m_mandatory_q_ptr(NULL), m_usingRubyTester(p->using_ruby_tester), > >+ slave_port(csprintf("%s.slave", name()), this, access_phys_mem, > 0), > >+ drainManager(NULL), system(p->system), retry(false), > >+ access_phys_mem(p->access_phys_mem) > > { > >+ assert(m_version != -1); > > } > > > > void > > DMASequencer::init() > > { > >- RubyPort::init(); > >+ MemObject::init(); > >+ assert(m_controller != NULL); > >+ m_mandatory_q_ptr = m_controller->getMandatoryQueue(); > >+ m_mandatory_q_ptr->setSender(this); > > m_is_busy = false; > > m_data_block_mask = ~ (~0 << RubySystem::getBlockSizeBits()); } > > > >+BaseSlavePort & > >+DMASequencer::getSlavePort(const std::string &if_name, PortID idx) { > >+ // used by the CPUs to connect the caches to the interconnect, and > >+ // for the x86 case also the interrupt master > >+ if (if_name != "slave") { > >+ // pass it along to our super class > >+ return MemObject::getSlavePort(if_name, idx); > >+ } else { > >+ return slave_port; > >+ } > >+} > >+ > >+DMASequencer::MemSlavePort::MemSlavePort(const std::string &_name, > >+ DMASequencer *_port, bool _access_phys_mem, PortID id) > >+ : QueuedSlavePort(_name, _port, queue, id), queue(*_port, *this), > >+ access_phys_mem(_access_phys_mem) { > >+ DPRINTF(RubyDma, "Created slave memport on ruby sequencer %s\n", > >+_name); } > >+ > >+bool > >+DMASequencer::MemSlavePort::recvTimingReq(PacketPtr pkt) { > >+ DPRINTF(RubyDma, "Timing request for address %#x on port %d\n", > >+ pkt->getAddr(), id); > >+ DMASequencer *seq = static_cast<DMASequencer *>(&owner); > >+ > >+ if (pkt->memInhibitAsserted()) > >+ panic("DMASequencer should never see an inhibited request\n"); > >+ > >+ assert(isPhysMemAddress(pkt->getAddr())); > >+ assert(Address(pkt->getAddr()).getOffset() + pkt->getSize() <= > >+ RubySystem::getBlockSizeBytes()); > >+ > >+ // Submit the ruby request > >+ RequestStatus requestStatus = seq->makeRequest(pkt); > >+ > >+ // If the request successfully issued then we should return true. > >+ // Otherwise, we need to tell the port to retry at a later point > >+ // and return false. > >+ if (requestStatus == RequestStatus_Issued) { > >+ DPRINTF(RubyDma, "Request %s 0x%x issued\n", pkt->cmdString(), > >+ pkt->getAddr()); > >+ return true; > >+ } > >+ > >+ // Unless one is using the ruby tester, record the stalled M5 port > >for > >+ // later retry when the sequencer becomes free. > >+ if (!seq->m_usingRubyTester) { > >+ seq->retry = true; > >+ } > >+ > >+ DPRINTF(RubyDma, "Request for address %#x did not issued because > >%s\n", > >+ pkt->getAddr(), RequestStatus_to_string(requestStatus)); > >+ > >+ return false; > >+} > >+ > >+void > >+DMASequencer::ruby_hit_callback(PacketPtr pkt) { > >+ DPRINTF(RubyDma, "Hit callback for %s 0x%x\n", pkt->cmdString(), > >+ pkt->getAddr()); > >+ > >+ // The packet was destined for memory and has not yet been turned > >+ // into a response > >+ assert(system->isMemAddr(pkt->getAddr())); > >+ assert(pkt->isRequest()); > >+ slave_port.hitCallback(pkt); > >+ > >+ // If we had to stall the slave ports, wake it up because > >+ // the sequencer likely has free resources now. > >+ if (retry) { > >+ retry = false; > >+ DPRINTF(RubyDma,"Sequencer may now be free. SendRetry to port > >%s\n", > >+ slave_port.name()); > >+ slave_port.sendRetry(); > >+ } > >+ > >+ testDrainComplete(); > >+} > >+ > >+void > >+DMASequencer::testDrainComplete() > >+{ > >+ //If we weren't able to drain before, we might be able to now. > >+ if (drainManager != NULL) { > >+ unsigned int drainCount = outstandingCount(); > >+ DPRINTF(Drain, "Drain count: %u\n", drainCount); > >+ if (drainCount == 0) { > >+ DPRINTF(Drain, "DMASequencer done draining, signaling drain > >done\n"); > >+ drainManager->signalDrainDone(); > >+ // Clear the drain manager once we're done with it. > >+ drainManager = NULL; > >+ } > >+ } > >+} > >+ > >+unsigned int > >+DMASequencer::getChildDrainCount(DrainManager *dm) { > >+ int count = 0; > >+ count += slave_port.drain(dm); > >+ DPRINTF(Config, "count after slave port check %d\n", count); > >+ return count; > >+} > >+ > >+unsigned int > >+DMASequencer::drain(DrainManager *dm) > >+{ > >+ if (isDeadlockEventScheduled()) { > >+ descheduleDeadlockEvent(); > >+ } > >+ > >+ // If the DMASequencer is not empty, then it needs to clear all > >outstanding > >+ // requests before it should call drainManager->signalDrainDone() > >+ DPRINTF(Config, "outstanding count %d\n", outstandingCount()); > >+ bool need_drain = outstandingCount() > 0; > >+ > >+ // > >+ // Also, get the number of child ports that will also need to clear > >+ // their buffered requests before they call > >drainManager->signalDrainDone() > >+ // > >+ unsigned int child_drain_count = getChildDrainCount(dm); > >+ > >+ // Set status > >+ if (need_drain) { > >+ drainManager = dm; > >+ > >+ DPRINTF(Drain, "DMASequencer not drained\n"); > >+ setDrainState(Drainable::Draining); > >+ return child_drain_count + 1; > >+ } > >+ > >+ drainManager = NULL; > >+ setDrainState(Drainable::Drained); > >+ return child_drain_count; > >+} > >+ > >+void > >+DMASequencer::MemSlavePort::hitCallback(PacketPtr pkt) { > >+ bool needsResponse = pkt->needsResponse(); > >+ bool accessPhysMem = access_phys_mem; > >+ > >+ assert(!pkt->isLLSC()); > >+ assert(!pkt->isFlush()); > >+ > >+ DPRINTF(RubyDma, "Hit callback needs response %d\n", > >+ needsResponse); > >+ > >+ if (accessPhysMem) { > >+ DMASequencer *seq = static_cast<DMASequencer *>(&owner); > >+ seq->system->getPhysMem().access(pkt); > >+ } else if (needsResponse) { > >+ pkt->makeResponse(); > >+ } > >+ > >+ // turn packet around to go back to requester if response expected > >+ if (needsResponse) { > >+ DPRINTF(RubyDma, "Sending packet back over port\n"); > >+ // send next cycle > >+ schedTimingResp(pkt, curTick() + g_system_ptr->clockPeriod()); > >+ } else { > >+ delete pkt; > >+ } > >+ DPRINTF(RubyDma, "Hit callback done!\n"); } > >+ > >+bool > >+DMASequencer::MemSlavePort::isPhysMemAddress(Addr addr) const { > >+ DMASequencer *seq = static_cast<DMASequencer *>(&owner); > >+ return seq->system->isMemAddr(addr); } > >+ > > RequestStatus > > DMASequencer::makeRequest(PacketPtr pkt) { @@ -168,7 +354,8 @@ } > > > > void > >-DMASequencer::recordRequestType(DMASequencerRequestType requestType) { > >+DMASequencer::recordRequestType(DMASequencerRequestType requestType) { > > DPRINTF(RubyStats, "Recorded statistic: %s\n", > > DMASequencerRequestType_to_string(requestType)); > > } > >diff -r ba51f8572571 -r 30e3715c9405 src/mem/ruby/system/DMASequencer.hh > >--- a/src/mem/ruby/system/DMASequencer.hh Mon Nov 03 10:14:42 2014 > >-0600 > >+++ b/src/mem/ruby/system/DMASequencer.hh Thu Nov 06 00:55:09 2014 > >-0600 > >@@ -33,10 +33,16 @@ > > #include <memory> > > > > #include "mem/protocol/DMASequencerRequestType.hh" > >+#include "mem/protocol/RequestStatus.hh" > > #include "mem/ruby/common/DataBlock.hh" > >-#include "mem/ruby/system/RubyPort.hh" > >+#include "mem/ruby/network/MessageBuffer.hh" > >+#include "mem/ruby/system/System.hh" > >+#include "mem/mem_object.hh" > >+#include "mem/tport.hh" > > #include "params/DMASequencer.hh" > > > >+class AbstractController; > >+ > > struct DMARequest > > { > > uint64_t start_paddr; > >@@ -48,12 +54,45 @@ > > PacketPtr pkt; > > }; > > > >-class DMASequencer : public RubyPort > >+class DMASequencer : public MemObject > > { > > public: > > typedef DMASequencerParams Params; > > DMASequencer(const Params *); > > void init(); > >+ > >+ public: > >+ class MemSlavePort : public QueuedSlavePort > >+ { > >+ private: > >+ SlavePacketQueue queue; > >+ bool access_phys_mem; > >+ > >+ public: > >+ MemSlavePort(const std::string &_name, DMASequencer *_port, > >+ bool _access_phys_mem, PortID id); > >+ void hitCallback(PacketPtr pkt); > >+ void evictionCallback(const Address& address); > >+ > >+ protected: > >+ bool recvTimingReq(PacketPtr pkt); > >+ > >+ Tick recvAtomic(PacketPtr pkt) > >+ { panic("DMASequencer::MemSlavePort::recvAtomic() not > >+ implemented!\n"); } > >+ > >+ void recvFunctional(PacketPtr pkt) > >+ { panic("DMASequencer::MemSlavePort::recvFunctional() not > >+ implemented!\n"); } > >+ > >+ AddrRangeList getAddrRanges() const > >+ { AddrRangeList ranges; return ranges; } > >+ > >+ private: > >+ bool isPhysMemAddress(Addr addr) const; > >+ }; > >+ > >+ BaseSlavePort &getSlavePort(const std::string &if_name, > >+ PortID idx = InvalidPortID); > >+ > > /* external interface */ > > RequestStatus makeRequest(PacketPtr pkt); > > bool busy() { return m_is_busy;} > >@@ -61,6 +100,12 @@ > > bool isDeadlockEventScheduled() const { return false; } > > void descheduleDeadlockEvent() {} > >_______________________________________________ > >gem5-dev mailing list > >[email protected] > >http://m5sim.org/mailman/listinfo/gem5-dev > >_______________________________________________ > >gem5-dev mailing list > >[email protected] > >http://m5sim.org/mailman/listinfo/gem5-dev > >_______________________________________________ > >gem5-dev mailing list > >[email protected] > >http://m5sim.org/mailman/listinfo/gem5-dev > >_______________________________________________ > >gem5-dev mailing list > >[email protected] > >http://m5sim.org/mailman/listinfo/gem5-dev > >_______________________________________________ > >gem5-dev mailing list > >[email protected] > >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. > > ARM Limited, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, > Registered in England & Wales, Company No: 2557590 > ARM Holdings plc, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, > Registered in England & Wales, Company No: 2548782 > _______________________________________________ > gem5-dev mailing list > [email protected] > http://m5sim.org/mailman/listinfo/gem5-dev > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
