Hi Nilay, I remember the discussion, I simply did not realise it was also a conclusion.
I will close that patch for now then, and await an update. Andreas On 24/02/2014 12:14, "Nilay Vaish" <[email protected]> wrote: >We talked about it before as well. I am quoting my self from a >previously >sent email: > >-------------------------------------------------------------------------- >-- >"If we decide on introducing bridges between ruby port and piobus, then >we >will have one bridge whose master port is connected to ruby and slave >port >is connected to the piobus. The address range that this bridge caters to >is not known to the ruby port initially. It becomes known when the slave >side of the interrupt controller informs the ruby port. But the bridge >needs to inform the address range to the piobus when its init() function >is called. I tried to think of ways to modify the bridge so that it >informs the piobus only when its master port has received the address >range. Ultimately I concluded that this might not be feasible since some >instantiations of the bridge class rely on the default address range >provided to the bridge. > >As I mentioned above, once all packets start flowing through ruby's >network, the point at which they enter/exit ruby network and the piobus >would be connected via a bridge, whose address range I am guessing would >be the entire 64-bit space". >-------------------------------------------------------------------------- >- > >As explained above, the bridge solution used in 2153 will not work. As >of >now, I am relying on storing the src port pointer in the sender state of >the packet. I think once these pio packets start flowing through ruby's >network, we will not need the port pointer (like the regular memory >packets). > >-- >Nilay > > >On Mon, 24 Feb 2014, Andreas Hansson wrote: > >> Hi Nilay, >> >> I??m a bit surprised this patch was pushed without: >> http://reviews.gem5.org/r/2153/ >> >> How did you resolve the responsibility of saving and restoring the >>src/dst >> fields? >> >> Andreas >> >> >> On 24/02/2014 01:16, "Andreas Hansson" <[email protected]> wrote: >> >>> changeset bc3126a05a7f in /z/repo/gem5 >>> details: http://repo.gem5.org/gem5?cmd=changeset;node=bc3126a05a7f >>> description: >>> ruby: Simplify RubyPort flow control and routing >>> >>> This patch simplfies the retry logic in the RubyPort, avoiding >>> redundant attributes, and enforcing more stringent checks on the >>> interactions with the normal ports. The patch also simplifies the >>> routing done by the RubyPort, using the port identifiers instead >>>of a >>> heavy-weight sender state. >>> >>> The patch also fixes a bug in the sending of responses from PIO >>> ports. Previously these responses bypassed the queue in the >>>queued >>> port, and ignored the return value, potentially leading to >>>response >>> packets being lost. >>> >>> Committed by: Nilay Vaish <[email protected]> >>> >>> diffstat: >>> >>> src/mem/qport.hh | 4 +- >>> src/mem/ruby/system/RubyPort.cc | 94 >>> +++++++++++++++++++-------------------- >>> src/mem/ruby/system/RubyPort.hh | 61 ++++++++++--------------- >>> src/mem/ruby/system/Sequencer.cc | 9 +-- >>> 4 files changed, 74 insertions(+), 94 deletions(-) >>> >>> diffs (truncated from 359 to 300 lines): >>> >>> diff -r eca928d8a4ab -r bc3126a05a7f src/mem/qport.hh >>> --- a/src/mem/qport.hh Sun Feb 23 19:16:15 2014 -0600 >>> +++ b/src/mem/qport.hh Sun Feb 23 19:16:16 2014 -0600 >>> @@ -78,8 +78,8 @@ >>> * QueuePort constructor. >>> */ >>> QueuedSlavePort(const std::string& name, MemObject* owner, >>> - SlavePacketQueue &queue) : >>> - SlavePort(name, owner), queue(queue) >>> + SlavePacketQueue &queue, PortID id = >>>InvalidPortID) : >>> + SlavePort(name, owner, id), queue(queue) >>> { } >>> >>> virtual ~QueuedSlavePort() { } >>> diff -r eca928d8a4ab -r bc3126a05a7f src/mem/ruby/system/RubyPort.cc >>> --- a/src/mem/ruby/system/RubyPort.cc Sun Feb 23 19:16:15 2014 -0600 >>> +++ b/src/mem/ruby/system/RubyPort.cc Sun Feb 23 19:16:16 2014 -0600 >>> @@ -1,5 +1,5 @@ >>> /* >>> - * Copyright (c) 2012 ARM Limited >>> + * Copyright (c) 2012-2013 ARM Limited >>> * All rights reserved. >>> * >>> * The license below extends only to copyright in the software and >>>shall >>> @@ -52,16 +52,17 @@ >>> : MemObject(p), m_version(p->version), m_controller(NULL), >>> m_mandatory_q_ptr(NULL), >>> pio_port(csprintf("%s-pio-port", name()), this), >>> - m_usingRubyTester(p->using_ruby_tester), m_request_cnt(0), >>> + m_usingRubyTester(p->using_ruby_tester), >>> drainManager(NULL), ruby_system(p->ruby_system), >>>system(p->system), >>> - waitingOnSequencer(false), access_phys_mem(p->access_phys_mem) >>> + access_phys_mem(p->access_phys_mem) >>> { >>> assert(m_version != -1); >>> >>> // create the slave ports based on the number of connected ports >>> for (size_t i = 0; i < p->port_slave_connection_count; ++i) { >>> slave_ports.push_back(new M5Port(csprintf("%s-slave%d", name(), >>> i), >>> - this, ruby_system, >>> access_phys_mem)); >>> + this, ruby_system, >>> + access_phys_mem, i)); >>> } >>> >>> // create the master ports based on the number of connected ports >>> @@ -119,16 +120,17 @@ >>> >>> RubyPort::PioPort::PioPort(const std::string &_name, >>> RubyPort *_port) >>> - : QueuedMasterPort(_name, _port, queue), queue(*_port, *this) >>> + : QueuedMasterPort(_name, _port, queue), queue(*_port, *this), >>> + ruby_port(_port) >>> { >>> DPRINTF(RubyPort, "creating master port on ruby sequencer %s\n", >>> _name); >>> } >>> >>> RubyPort::M5Port::M5Port(const std::string &_name, RubyPort *_port, >>> - RubySystem *_system, bool _access_phys_mem) >>> - : QueuedSlavePort(_name, _port, queue), queue(*_port, *this), >>> + RubySystem *_system, bool _access_phys_mem, >>> PortID id) >>> + : QueuedSlavePort(_name, _port, queue, id), queue(*_port, *this), >>> ruby_port(_port), ruby_system(_system), >>> - _onRetryList(false), access_phys_mem(_access_phys_mem) >>> + access_phys_mem(_access_phys_mem) >>> { >>> DPRINTF(RubyPort, "creating slave port on ruby sequencer %s\n", >>> _name); >>> } >>> @@ -140,22 +142,23 @@ >>> return 0; >>> } >>> >>> +bool >>> +RubyPort::recvTimingResp(PacketPtr pkt, PortID master_port_id) >>> +{ >>> + // got a response from a device >>> + assert(pkt->isResponse()); >>> >>> -bool >>> -RubyPort::PioPort::recvTimingResp(PacketPtr pkt) >>> -{ >>> // In FS mode, ruby memory will receive pio responses from devices >>> // and it must forward these responses back to the particular CPU. >>> - DPRINTF(RubyPort, "Pio response for address %#x\n", >>>pkt->getAddr()); >>> + DPRINTF(RubyPort, "Pio response for address %#x, going to %d\n", >>> + pkt->getAddr(), pkt->getDest()); >>> >>> - // First we must retrieve the request port from the sender State >>> - RubyPort::SenderState *senderState = >>> - safe_cast<RubyPort::SenderState *>(pkt->popSenderState()); >>> - M5Port *port = senderState->port; >>> - assert(port != NULL); >>> - delete senderState; >>> + // Retrieve the port from the destination field >>> + assert(pkt->getDest() < slave_ports.size()); >>> >>> - port->sendTimingResp(pkt); >>> + // attempt to send the response in the next cycle >>> + slave_ports[pkt->getDest()]->schedTimingResp(pkt, curTick() + >>> + >>> g_system_ptr->clockPeriod()); >>> >>> return true; >>> } >>> @@ -164,16 +167,14 @@ >>> RubyPort::M5Port::recvTimingReq(PacketPtr pkt) >>> { >>> DPRINTF(RubyPort, >>> - "Timing access caught for address %#x\n", pkt->getAddr()); >>> - >>> - //dsm: based on SimpleTimingPort::recvTimingReq(pkt); >>> + "Timing access for address %#x on port %d\n", >>>pkt->getAddr(), >>> + id); >>> >>> if (pkt->memInhibitAsserted()) >>> panic("RubyPort should never see an inhibited request\n"); >>> >>> - // Save the port in the sender state object to be used later to >>> - // route the response >>> - pkt->pushSenderState(new SenderState(this)); >>> + // Save the port id to be used later to route the response >>> + pkt->setSrc(id); >>> >>> // Check for pio requests and directly send them to the dedicated >>> // pio port. >>> @@ -196,10 +197,11 @@ >>> RequestStatus requestStatus = ruby_port->makeRequest(pkt); >>> >>> // If the request successfully issued then we should return true. >>> - // Otherwise, we need to delete the senderStatus we just created >>>and >>> return >>> - // false. >>> + // Otherwise, we need to tell the port to retry at a later point >>> + // and return false. >>> if (requestStatus == RequestStatus_Issued) { >>> - DPRINTF(RubyPort, "Request %#x issued\n", pkt->getAddr()); >>> + DPRINTF(RubyPort, "Request %s 0x%x issued\n", >>>pkt->cmdString(), >>> + pkt->getAddr()); >>> return true; >>> } >>> >>> @@ -215,9 +217,6 @@ >>> "Request for address %#x did not issue because %s\n", >>> pkt->getAddr(), RequestStatus_to_string(requestStatus)); >>> >>> - SenderState* senderState = >>>safe_cast<SenderState*>(pkt->senderState); >>> - pkt->senderState = senderState->predecessor; >>> - delete senderState; >>> return false; >>> } >>> >>> @@ -284,23 +283,25 @@ >>> void >>> RubyPort::ruby_hit_callback(PacketPtr pkt) >>> { >>> - // Retrieve the request port from the sender State >>> - RubyPort::SenderState *senderState = >>> - safe_cast<RubyPort::SenderState *>(pkt->senderState); >>> - M5Port *port = senderState->port; >>> - assert(port != NULL); >>> + DPRINTF(RubyPort, "Hit callback for %s 0x%x\n", pkt->cmdString(), >>> + pkt->getAddr()); >>> >>> - // pop the sender state from the packet >>> - pkt->senderState = senderState->predecessor; >>> - delete senderState; >>> + // The packet was destined for memory and has not yet been turned >>> + // into a response >>> + assert(system->isMemAddr(pkt->getAddr())); >>> + assert(pkt->isRequest()); >>> >>> - port->hitCallback(pkt); >>> + // As it has not yet been turned around, the source field tells us >>> + // which port it came from. >>> + assert(pkt->getSrc() < slave_ports.size()); >>> + >>> + slave_ports[pkt->getSrc()]->hitCallback(pkt); >>> >>> // >>> // If we had to stall the M5Ports, wake them up because the >>>sequencer >>> // likely has free resources now. >>> // >>> - if (waitingOnSequencer) { >>> + if (!retryList.empty()) { >>> // >>> // Record the current list of ports to retry on a temporary >>>list >>> before >>> // calling sendRetry on those ports. sendRetry will cause an >>> @@ -308,17 +309,14 @@ >>> // list. Therefore we want to clear the retryList before >>>calling >>> // sendRetry. >>> // >>> - std::list<M5Port*> curRetryList(retryList); >>> + std::vector<M5Port*> curRetryList(retryList); >>> >>> retryList.clear(); >>> - waitingOnSequencer = false; >>> - >>> - for (std::list<M5Port*>::iterator i = curRetryList.begin(); >>> - i != curRetryList.end(); ++i) { >>> + >>> + for (auto i = curRetryList.begin(); i != curRetryList.end(); >>> ++i) { >>> DPRINTF(RubyPort, >>> "Sequencer may now be free. SendRetry to port >>>%s\n", >>> (*i)->name()); >>> - (*i)->onRetryList(false); >>> (*i)->sendRetry(); >>> } >>> } >>> @@ -471,7 +469,7 @@ >>> } >>> >>> bool >>> -RubyPort::M5Port::isPhysMemAddress(Addr addr) >>> +RubyPort::M5Port::isPhysMemAddress(Addr addr) const >>> { >>> return ruby_port->system->isMemAddr(addr); >>> } >>> diff -r eca928d8a4ab -r bc3126a05a7f src/mem/ruby/system/RubyPort.hh >>> --- a/src/mem/ruby/system/RubyPort.hh Sun Feb 23 19:16:15 2014 -0600 >>> +++ b/src/mem/ruby/system/RubyPort.hh Sun Feb 23 19:16:16 2014 -0600 >>> @@ -1,5 +1,5 @@ >>> /* >>> - * Copyright (c) 2012 ARM Limited >>> + * Copyright (c) 2012-2013 ARM Limited >>> * All rights reserved. >>> * >>> * The license below extends only to copyright in the software and >>>shall >>> @@ -65,54 +65,37 @@ >>> SlavePacketQueue queue; >>> RubyPort *ruby_port; >>> RubySystem* ruby_system; >>> - bool _onRetryList; >>> bool access_phys_mem; >>> >>> public: >>> M5Port(const std::string &_name, RubyPort *_port, >>> - RubySystem*_system, bool _access_phys_mem); >>> + RubySystem*_system, bool _access_phys_mem, PortID id); >>> void hitCallback(PacketPtr pkt); >>> void evictionCallback(const Address& address); >>> - >>> - bool onRetryList() >>> - { return _onRetryList; } >>> - >>> - void onRetryList(bool newVal) >>> - { _onRetryList = newVal; } >>> >>> protected: >>> - virtual bool recvTimingReq(PacketPtr pkt); >>> - virtual Tick recvAtomic(PacketPtr pkt); >>> - virtual void recvFunctional(PacketPtr pkt); >>> - virtual AddrRangeList getAddrRanges() const; >>> + bool recvTimingReq(PacketPtr pkt); >>> + Tick recvAtomic(PacketPtr pkt); >>> + void recvFunctional(PacketPtr pkt); >>> + AddrRangeList getAddrRanges() const; >>> >>> private: >>> - bool isPhysMemAddress(Addr addr); >>> + bool isPhysMemAddress(Addr addr) const; >>> }; >>> >>> - friend class M5Port; >>> - >>> class PioPort : public QueuedMasterPort >>> { >>> private: >>> >>> MasterPacketQueue queue; >>> + RubyPort *ruby_port; >>> >>> public: >>> PioPort(const std::string &_name, RubyPort *_port); >>> >>> protected: >>> - virtual bool recvTimingResp(PacketPtr pkt); >>> - }; >>> - >>> - friend class PioPort; >>> - >>> - struct SenderState : public Packet::SenderState >>> - { >>> - M5Port* port; >>> - >>> - SenderState(M5Port* _port) : port(_port) >>> - {} >>> + bool recvTimingResp(PacketPtr pkt) >>> + { return ruby_port->recvTimingResp(pkt, id); } >>> }; >>> >>> typedef RubyPortParams Params; >>> @@ -145,6 +128,16 @@ >>> void testDrainComplete(); >>> void ruby_eviction_callback(const Address& address); >>> >>> + /** >>> + * Called by the PIO port when receiving a timing response. >>> + * >>> + * @param pkt Response packet >>> + * @param master_port_id Port id of the PIO port >>> + * >>> + * @return Whether successfully sent >>> + */ >>> _______________________________________________ >>> 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 >> -- 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
