mem-ruby: Fix Ruby handling of functional requests

This patch addresses multiple cases:

- When a controller has read/write permissions while others have read
  only permissions, the one with r/w permissions performs the read as
  the others may have stale data
- When controllers only have lines with stale or busy access permissions,
  a valid copy of the line may be in a message in transit in the network
  or in a message buffer (not seen by the controller yet). In this case,
  we forward the functional request accordingly.
- Sequencer messages should not accept functional reads
- Functional writes also update the packet data on the sequencer
  outstanding request lists and the cpu-side response queue.

Change-Id: I6b0656f1a2b81d41bdcf6c783dfa522a77393981
Signed-off-by: Tiago Mück <tiago.muck@arm.com>
Reviewed-on: https://gem5-review.googlesource.com/c/public/gem5/+/22022
Tested-by: Gem5 Cloud Project GCB service account <345032938727@cloudbuild.gserviceaccount.com>
Tested-by: kokoro <noreply+kokoro@google.com>
Reviewed-by: John Alsop <johnathan.alsop@amd.com>
Reviewed-by: Jason Lowe-Power <power.jg@gmail.com>
Maintainer: Jason Lowe-Power <power.jg@gmail.com>

src/mem/ruby/protocol/RubySlicc_Exports.sm

@@ -1,4 +1,16 @@
 /*
+ * Copyright (c) 2019 ARM Limited
+ * All rights reserved.
+ *
+ * The license below extends only to copyright in the software and shall
+ * not be construed as granting a license to any other intellectual
+ * property including but not limited to intellectual property relating
+ * to a hardware implementation of the functionality of the software
+ * licensed hereunder.  You may use the software subject to the license
+ * terms below provided that you ensure that this notice is replicated
+ * unmodified and in its entirety in all distributions of the software,
+ * modified or unmodified, in source code or in binary form.
+ *
  * Copyright (c) 1999-2012 Mark D. Hill and David A. Wood
  * Copyright (c) 2011 Advanced Micro Devices, Inc.
  * All rights reserved.
@@ -291,7 +303,7 @@ structure(SequencerMsg, desc="...", interface="Message") {
   MessageSizeType MessageSize, default="MessageSizeType_Request_Control";
 
   bool functionalRead(Packet *pkt) {
-    return testAndRead(PhysicalAddress, DataBlk, pkt);
+    return false;
   }
 
   bool functionalWrite(Packet *pkt) {

src/mem/ruby/slicc_interface/AbstractController.hh

@@ -62,6 +62,7 @@
 
 class Network;
 class GPUCoalescer;
+class DMASequencer;
 
 // used to communicate that an in_port peeked the wrong message type
 class RejectException: public std::exception
@@ -101,6 +102,7 @@ class AbstractController : public ClockedObject, public Consumer
 
     virtual void recordCacheTrace(int cntrl, CacheRecorder* tr) = 0;
     virtual Sequencer* getCPUSequencer() const = 0;
+    virtual DMASequencer* getDMASequencer() const = 0;
     virtual GPUCoalescer* getGPUCoalescer() const = 0;
 
     // This latency is used by the sequencer when enqueueing requests.

src/mem/ruby/slicc_interface/RubyRequest.cc

@@ -1,4 +1,16 @@
 /*
+ * Copyright (c) 2019 ARM Limited
+ * All rights reserved.
+ *
+ * The license below extends only to copyright in the software and shall
+ * not be construed as granting a license to any other intellectual
+ * property including but not limited to intellectual property relating
+ * to a hardware implementation of the functionality of the software
+ * licensed hereunder.  You may use the software subject to the license
+ * terms below provided that you ensure that this notice is replicated
+ * unmodified and in its entirety in all distributions of the software,
+ * modified or unmodified, in source code or in binary form.
+ *
  * Copyright (c) 2011 Mark D. Hill and David A. Wood
  * All rights reserved.
  *
@@ -30,6 +42,8 @@
 
 #include <iostream>
 
+#include "mem/ruby/slicc_interface/RubySlicc_Util.hh"
+
 using namespace std;
 
 void
@@ -67,6 +81,9 @@ RubyRequest::functionalWrite(Packet *pkt)
     // has to overwrite the data for the timing request, even if the
     // timing request has still not been ordered globally.
 
+    if (!data)
+      return false;
+
     Addr wBase = pkt->getAddr();
     Addr wTail = wBase + pkt->getSize();
     Addr mBase = m_PhysicalAddress;
@@ -81,5 +98,8 @@ RubyRequest::functionalWrite(Packet *pkt)
         data[i - mBase] = pktData[i - wBase];
     }
 
+    // also overwrite the WTData
+    testAndWrite(m_PhysicalAddress, m_WTData, pkt);
+
     return cBase < cTail;
 }

src/mem/ruby/system/RubyPort.cc

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2012-2013 ARM Limited
+ * Copyright (c) 2012-2013,2019 ARM Limited
  * All rights reserved.
  *
  * The license below extends only to copyright in the software and shall
@@ -408,6 +408,12 @@ RubyPort::MemSlavePort::recvFunctional(PacketPtr pkt)
 
         // turn packet around to go back to requester if response expected
         if (needsResponse) {
+            // The pkt is already turned into a reponse if the directory
+            // forwarded the request to the memory controller (see
+            // AbstractController::functionalMemoryWrite and
+            // AbstractMemory::functionalAccess)
+            if (!pkt->isResponse())
+                pkt->makeResponse();
             pkt->setFunctionalResponseStatus(accessSucceeded);
         }
 
@@ -620,3 +626,16 @@ RubyPort::PioMasterPort::recvRangeChange()
         r.pioSlavePort.sendRangeChange();
     }
 }
+
+
+int
+RubyPort::functionalWrite(Packet *func_pkt)
+{
+    int num_written = 0;
+    for (auto port : slave_ports) {
+        if (port->trySatisfyFunctional(func_pkt)) {
+            num_written += 1;
+        }
+    }
+    return num_written;
+}

src/mem/ruby/system/RubyPort.hh

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2012-2013 ARM Limited
+ * Copyright (c) 2012-2013,2019 ARM Limited
  * All rights reserved.
  *
  * The license below extends only to copyright in the software and shall
@@ -166,6 +166,8 @@ class RubyPort : public ClockedObject
 
     bool isCPUSequencer() { return m_isCPUSequencer; }
 
+    virtual int functionalWrite(Packet *func_pkt);
+
   protected:
     void trySendRetries();
     void ruby_hit_callback(PacketPtr pkt);

src/mem/ruby/system/RubySystem.cc

@@ -1,4 +1,16 @@
 /*
+ * Copyright (c) 2019 ARM Limited
+ * All rights reserved.
+ *
+ * The license below extends only to copyright in the software and shall
+ * not be construed as granting a license to any other intellectual
+ * property including but not limited to intellectual property relating
+ * to a hardware implementation of the functionality of the software
+ * licensed hereunder.  You may use the software subject to the license
+ * terms below provided that you ensure that this notice is replicated
+ * unmodified and in its entirety in all distributions of the software,
+ * modified or unmodified, in source code or in binary form.
+ *
  * Copyright (c) 1999-2011 Mark D. Hill and David A. Wood
  * All rights reserved.
  *
@@ -40,6 +52,8 @@
 #include "debug/RubySystem.hh"
 #include "mem/ruby/common/Address.hh"
 #include "mem/ruby/network/Network.hh"
+#include "mem/ruby/system/DMASequencer.hh"
+#include "mem/ruby/system/Sequencer.hh"
 #include "mem/simple_mem.hh"
 #include "sim/eventq.hh"
 #include "sim/simulate.hh"
@@ -409,25 +423,39 @@ RubySystem::functionalRead(PacketPtr pkt)
     unsigned int num_ro = 0;
     unsigned int num_rw = 0;
     unsigned int num_busy = 0;
+    unsigned int num_maybe_stale = 0;
     unsigned int num_backing_store = 0;
     unsigned int num_invalid = 0;
 
+    AbstractController *ctrl_ro = nullptr;
+    AbstractController *ctrl_rw = nullptr;
+    AbstractController *ctrl_backing_store = nullptr;
+
     // In this loop we count the number of controllers that have the given
     // address in read only, read write and busy states.
     for (unsigned int i = 0; i < num_controllers; ++i) {
         access_perm = m_abs_cntrl_vec[i]-> getAccessPermission(line_address);
-        if (access_perm == AccessPermission_Read_Only)
+        if (access_perm == AccessPermission_Read_Only){
             num_ro++;
-        else if (access_perm == AccessPermission_Read_Write)
+            if (ctrl_ro == nullptr) ctrl_ro = m_abs_cntrl_vec[i];
+        }
+        else if (access_perm == AccessPermission_Read_Write){
             num_rw++;
+            if (ctrl_rw == nullptr) ctrl_rw = m_abs_cntrl_vec[i];
+        }
         else if (access_perm == AccessPermission_Busy)
             num_busy++;
-        else if (access_perm == AccessPermission_Backing_Store)
+        else if (access_perm == AccessPermission_Maybe_Stale)
+            num_maybe_stale++;
+        else if (access_perm == AccessPermission_Backing_Store) {
             // See RubySlicc_Exports.sm for details, but Backing_Store is meant
             // to represent blocks in memory *for Broadcast/Snooping protocols*,
             // where memory has no idea whether it has an exclusive copy of data
             // or not.
             num_backing_store++;
+            if (ctrl_backing_store == nullptr)
+                ctrl_backing_store = m_abs_cntrl_vec[i];
+        }
         else if (access_perm == AccessPermission_Invalid ||
                  access_perm == AccessPermission_NotPresent)
             num_invalid++;
@@ -443,13 +471,8 @@ RubySystem::functionalRead(PacketPtr pkt)
     // it only if it's not in the cache hierarchy at all.
     if (num_invalid == (num_controllers - 1) && num_backing_store == 1) {
         DPRINTF(RubySystem, "only copy in Backing_Store memory, read from it\n");
-        for (unsigned int i = 0; i < num_controllers; ++i) {
-            access_perm = m_abs_cntrl_vec[i]->getAccessPermission(line_address);
-            if (access_perm == AccessPermission_Backing_Store) {
-                m_abs_cntrl_vec[i]->functionalRead(line_address, pkt);
-                return true;
-            }
-        }
+        ctrl_backing_store->functionalRead(line_address, pkt);
+        return true;
     } else if (num_ro > 0 || num_rw >= 1) {
         if (num_rw > 1) {
             // We iterate over the vector of abstract controllers, and return
@@ -462,19 +485,34 @@ RubySystem::functionalRead(PacketPtr pkt)
         // exists somewhere in the caching hierarchy, then you want to read any
         // valid RO or RW block.  In directory protocols, same thing, you want
         // to read any valid readable copy of the block.
-        DPRINTF(RubySystem, "num_busy = %d, num_ro = %d, num_rw = %d\n",
-                num_busy, num_ro, num_rw);
-        // In this loop, we try to figure which controller has a read only or
-        // a read write copy of the given address. Any valid copy would suffice
-        // for a functional read.
-        for (unsigned int i = 0;i < num_controllers;++i) {
-            access_perm = m_abs_cntrl_vec[i]->getAccessPermission(line_address);
-            if (access_perm == AccessPermission_Read_Only ||
-                access_perm == AccessPermission_Read_Write) {
-                m_abs_cntrl_vec[i]->functionalRead(line_address, pkt);
-                return true;
-            }
+        DPRINTF(RubySystem, "num_maybe_stale=%d, num_busy = %d, num_ro = %d, "
+                            "num_rw = %d\n",
+                num_maybe_stale, num_busy, num_ro, num_rw);
+        // Use the copy from the controller with read/write permission (if
+        // any), otherwise use get the first read only found
+        if (ctrl_rw) {
+            ctrl_rw->functionalRead(line_address, pkt);
+        } else {
+            assert(ctrl_ro);
+            ctrl_ro->functionalRead(line_address, pkt);
         }
+        return true;
+    } else if ((num_busy + num_maybe_stale) > 0) {
+        // No controller has a valid copy of the block, but a transient or
+        // stale state indicates a valid copy should be in transit in the
+        // network or in a message buffer waiting to be handled
+        DPRINTF(RubySystem, "Controllers functionalRead lookup "
+                            "(num_maybe_stale=%d, num_busy = %d)\n",
+                num_maybe_stale, num_busy);
+        for (unsigned int i = 0; i < num_controllers;++i) {
+            if (m_abs_cntrl_vec[i]->functionalReadBuffers(pkt))
+                return true;
+        }
+        DPRINTF(RubySystem, "Network functionalRead lookup "
+                            "(num_maybe_stale=%d, num_busy = %d)\n",
+                num_maybe_stale, num_busy);
+        if (m_network->functionalRead(pkt))
+            return true;
     }
 
     return false;
@@ -506,6 +544,17 @@ RubySystem::functionalWrite(PacketPtr pkt)
             num_functional_writes +=
                 m_abs_cntrl_vec[i]->functionalWrite(line_addr, pkt);
         }
+
+        // Also updates requests pending in any sequencer associated
+        // with the controller
+        if (m_abs_cntrl_vec[i]->getCPUSequencer()) {
+            num_functional_writes +=
+                m_abs_cntrl_vec[i]->getCPUSequencer()->functionalWrite(pkt);
+        }
+        if (m_abs_cntrl_vec[i]->getDMASequencer()) {
+            num_functional_writes +=
+                m_abs_cntrl_vec[i]->getDMASequencer()->functionalWrite(pkt);
+        }
     }
 
     num_functional_writes += m_network->functionalWrite(pkt);

src/mem/ruby/system/Sequencer.cc

@@ -1,4 +1,16 @@
 /*
+ * Copyright (c) 2019 ARM Limited
+ * All rights reserved.
+ *
+ * The license below extends only to copyright in the software and shall
+ * not be construed as granting a license to any other intellectual
+ * property including but not limited to intellectual property relating
+ * to a hardware implementation of the functionality of the software
+ * licensed hereunder.  You may use the software subject to the license
+ * terms below provided that you ensure that this notice is replicated
+ * unmodified and in its entirety in all distributions of the software,
+ * modified or unmodified, in source code or in binary form.
+ *
  * Copyright (c) 1999-2008 Mark D. Hill and David A. Wood
  * Copyright (c) 2013 Advanced Micro Devices, Inc.
  * All rights reserved.
@@ -112,6 +124,21 @@ Sequencer::wakeup()
     }
 }
 
+int
+Sequencer::functionalWrite(Packet *func_pkt)
+{
+    int num_written = RubyPort::functionalWrite(func_pkt);
+
+    for (const auto &table_entry : m_RequestTable) {
+        for (const auto& seq_req : table_entry.second) {
+            if (seq_req.functionalWrite(func_pkt))
+                ++num_written;
+        }
+    }
+
+    return num_written;
+}
+
 void Sequencer::resetStats()
 {
     m_outstandReqHist.reset();

src/mem/ruby/system/Sequencer.hh

@@ -1,4 +1,16 @@
 /*
+ * Copyright (c) 2019 ARM Limited
+ * All rights reserved.
+ *
+ * The license below extends only to copyright in the software and shall
+ * not be construed as granting a license to any other intellectual
+ * property including but not limited to intellectual property relating
+ * to a hardware implementation of the functionality of the software
+ * licensed hereunder.  You may use the software subject to the license
+ * terms below provided that you ensure that this notice is replicated
+ * unmodified and in its entirety in all distributions of the software,
+ * modified or unmodified, in source code or in binary form.
+ *
  * Copyright (c) 1999-2008 Mark D. Hill and David A. Wood
  * All rights reserved.
  *
@@ -52,6 +64,15 @@ struct SequencerRequest
                 : pkt(_pkt), m_type(_m_type), m_second_type(_m_second_type),
                   issue_time(_issue_time)
     {}
+
+    bool functionalWrite(Packet *func_pkt) const
+    {
+        // Follow-up on RubyRequest::functionalWrite
+        // This makes sure the hitCallback won't overrite the value we
+        // expect to find
+        assert(func_pkt->isWrite());
+        return func_pkt->trySatisfyFunctional(pkt);
+    }
 };
 
 std::ostream& operator<<(std::ostream& out, const SequencerRequest& obj);
@@ -103,6 +124,8 @@ class Sequencer : public RubyPort
     void invalidateSC(Addr address);
     int coreId() const { return m_coreId; }
 
+    virtual int functionalWrite(Packet *func_pkt) override;
+
     void recordRequestType(SequencerRequestType requestType);
     Stats::Histogram& getOutstandReqHist() { return m_outstandReqHist; }
 

src/mem/slicc/symbols/StateMachine.py

@@ -323,6 +323,7 @@ class $c_ident : public AbstractController
 
     void recordCacheTrace(int cntrl, CacheRecorder* tr);
     Sequencer* getCPUSequencer() const;
+    DMASequencer* getDMASequencer() const;
     GPUCoalescer* getGPUCoalescer() const;
 
     bool functionalReadBuffers(PacketPtr&);
@@ -702,6 +703,12 @@ $c_ident::init()
                 assert(param.pointer)
                 seq_ident = "m_%s_ptr" % param.ident
 
+        dma_seq_ident = "NULL"
+        for param in self.config_parameters:
+            if param.ident == "dma_sequencer":
+                assert(param.pointer)
+                dma_seq_ident = "m_%s_ptr" % param.ident
+
         coal_ident = "NULL"
         for param in self.config_parameters:
             if param.ident == "coalescer":
@@ -728,6 +735,28 @@ $c_ident::getCPUSequencer() const
 {
     return NULL;
 }
+''')
+
+        if dma_seq_ident != "NULL":
+            code('''
+DMASequencer*
+$c_ident::getDMASequencer() const
+{
+    if (NULL != $dma_seq_ident) {
+        return $dma_seq_ident;
+    } else {
+        return NULL;
+    }
+}
+''')
+        else:
+            code('''
+
+DMASequencer*
+$c_ident::getDMASequencer() const
+{
+    return NULL;
+}
 ''')
 
         if coal_ident != "NULL":