Re: [PATCH v7 2/2] hw/acpi: Implement the SRAT GI affinity structure

[Jonathan Cameron]

On Tue, 27 Feb 2024 17:36:21 +0000
Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:

> On Tue, 27 Feb 2024 17:11:15 +0000
> Jonathan Cameron via <qemu-devel@nongnu.org> wrote:
> 
> > On Tue, 27 Feb 2024 08:37:15 +0000
> > Ankit Agrawal <ankita@nvidia.com> wrote:
> >   
> > > Thanks Jonathan for reviewing the change.
> > > 
> > > Comments inline.
> > >     
> > > >> The structure needs a PCI device handle [2] that consists of the 
> > > >> device BDF.
> > > >> The vfio-pci device corresponding to the acpi-generic-initiator object 
> > > >> is
> > > >> located to determine the BDF.
> > > >>
> > > >> [1] ACPI Spec 6.3, Section 5.2.16.6
> > > >> [2] ACPI Spec 6.3, Table 5.80
> > > >>
> > > >> Signed-off-by: Ankit Agrawal <ankita@nvidia.com>      
> > > >Hi Ankit,
> > > >
> > > > As the code stands the use of a list seems overkill.      
> > > 
> > > Yeah, I will try out your suggestion.
> > >     
> > > > Otherwise looks good to me.  I need Generic Ports support for CXL
> > > > stuff so will copy your approach for that as it's ended up nice
> > > > and simple.
> > > > 
> > > > Jonathan      
> > > 
> > > Nice, would be good to have uniform implementations.    
> > 
> > I've been messing around with this today.
> > 
> > They differ only very trivially.
> > 2 Options.
> > 1) Have acpi-generic-port inherit from acpi-generic-initiator.
> >    Works but implies a relationship that isn't really true.
> > 2) Add an abstract base class. I've called it acpi-generic-node
> >    and have bother acpi-generic-initiator and acpi-generic-port
> >    inherit from it.
> > 
> > The second feels more natural but is a tiny bit more code (a few
> > more empty init / finalize functions.
> > 
> > If we are going to end up with an abstract base 'object' it
> > will be cleaner to do this all as one series if you don't mind
> > carrying the generic port stuff as well? Or I can smash the
> > two series together and send out an updated version that hopefully
> > meets both our requirements (+ tests etc).
> > 
> > I'm just running tests against the CXL qos / generic port code
> > but assuming all goes well can share my additional changes
> > in next day or two.
> > 
> > Jonathan  
> 
> One more thing.  Right now we can't use Generic Initiators as
> HMAT initiators.  That also wants fixing given that's their
> normal usecase rather than what you are using them for so it
> should 'work'.

Something along the lines of this will do the job.


diff --git a/include/sysemu/numa.h b/include/sysemu/numa.h
index 4173ef2afa..825cfe86bc 100644
--- a/include/sysemu/numa.h
+++ b/include/sysemu/numa.h
@@ -41,6 +41,7 @@ struct NodeInfo {
     struct HostMemoryBackend *node_memdev;
     bool present;
     bool has_cpu;
+    bool has_gi;
     uint8_t lb_info_provided;
     uint16_t initiator;
     uint8_t distance[MAX_NODES];
diff --git a/hw/acpi/acpi-generic-initiator.c b/hw/acpi/acpi-generic-initiator.c
index 9179590a42..8a67300320 100644
--- a/hw/acpi/acpi-generic-initiator.c
+++ b/hw/acpi/acpi-generic-initiator.c
@@ -6,6 +6,7 @@
 #include "qemu/osdep.h"
 #include "hw/acpi/acpi-generic-initiator.h"
 #include "hw/pci/pci_device.h"
+#include "hw/boards.h"
 #include "qapi/error.h"
 #include "qapi/qapi-builtin-visit.h"
 #include "qapi/visitor.h"
@@ -58,6 +59,7 @@ static void acpi_generic_node_set_node(Object *obj, Visitor 
*v,
                                        const char *name, void *opaque,
                                        Error **errp)
 {
+    MachineState *ms = MACHINE(qdev_get_machine());
     AcpiGenericNode *gn = ACPI_GENERIC_NODE(obj);
     uint32_t value;
 
@@ -72,6 +74,10 @@ static void acpi_generic_node_set_node(Object *obj, Visitor 
*v,
     }
 
     gn->node = value;
+
+    if (object_dynamic_cast(obj, TYPE_ACPI_GENERIC_INITIATOR)) {
+        ms->numa_state->nodes[gn->node].has_gi = true;
+    }
 }
 
 static void acpi_generic_node_class_init(ObjectClass *oc, void *data)
diff --git a/hw/acpi/hmat.c b/hw/acpi/hmat.c
index b933ae3c06..9b1662b6b8 100644
--- a/hw/acpi/hmat.c
+++ b/hw/acpi/hmat.c
@@ -225,7 +225,7 @@ static void hmat_build_table_structs(GArray *table_data, 
NumaState *numa_state)
     }
 
     for (i = 0; i < numa_state->num_nodes; i++) {
-        if (numa_state->nodes[i].has_cpu) {
+        if (numa_state->nodes[i].has_cpu || numa_state->nodes[i].has_gi) {
             initiator_list[num_initiator++] = i;
         }
     }
diff --git a/hw/core/numa.c b/hw/core/numa.c
index f08956ddb0..58a32f1564 100644
--- a/hw/core/numa.c
+++ b/hw/core/numa.c
@@ -229,7 +229,8 @@ void parse_numa_hmat_lb(NumaState *numa_state, 
NumaHmatLBOptions *node,
                    node->target, numa_state->num_nodes);
         return;
     }
-    if (!numa_info[node->initiator].has_cpu) {
+    if (!numa_info[node->initiator].has_cpu &&
+        !numa_info[node->initiator].has_gi) {
         error_setg(errp, "Invalid initiator=%d, it isn't an "
                    "initiator proximity domain", node->initiator);
         return;

> 
> Jonathan
> 
> > 
> > 
> >   
>