Re: [PATCH qemu ] hw/acpi: Fix big endian host creation of Generic Port Affinity Structures

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

Hi Igor,

Some code snippets below to try and see if I'm on the correct track
for what you had in mind.

> >   
> > > diff --git a/hw/acpi/acpi_generic_initiator.c b/hw/acpi/acpi_generic_initiator.c
> > > index 78b80dcf08..f064753b67 100644
> > > --- a/hw/acpi/acpi_generic_initiator.c
> > > +++ b/hw/acpi/acpi_generic_initiator.c
> > > @@ -151,7 +151,9 @@ build_srat_generic_node_affinity(GArray *table_data, int node,
> > >          build_append_int_noprefix(table_data, 0, 12);
> > >      } else {
> > >          /* Device Handle - ACPI */
> > > -        build_append_int_noprefix(table_data, handle->hid, 8);
> > > +        for (int i = 0; i < sizeof(handle->hid); i++) {
> > > +            build_append_int_noprefix(table_data, handle->hid[i], 1);
> > > +        }
> > >          build_append_int_noprefix(table_data, handle->uid, 4);
> > >          build_append_int_noprefix(table_data, 0, 4);    
> > 
> > instead of open codding structure
> > 
> > it might be better to introduce helper in aml_build.c
> > something like 
> >   /* proper reference to spec as we do for other ACPI primitives */
> >   build_append_srat_acpi_device_handle(GArray *table_data, char* hid, unit32_t uid)
> >       assert(strlen(hid) ...
> >       for() {
> >             build_append_byte()
> >       }          
> >       ...
> > 
> > the same applies to "Device Handle - PCI" structure  
> 
> I'll look at moving that stuff and the affinity structure creation
> code themselves in there. I think they ended up in this file because
> of the other infrastructure needed to create these nodes and it
> will have felt natural to keep this together.
> 
> Putting it in aml_build.c will put it with similar code though
> which makes sense to me.

This all works out fine, though there is less reason to keep a
ACPI_GENERIC_NODE base under GENERIC_PORT and GENERIC_INITIATOR
so I may drop that and just have a small amount of code duplication.

> 
> > 
> > Also get rid of PCI deps in acpi_generic_initiator.c 
> > move build_all_acpi_generic_initiators/build_srat_generic_pci_initiator into
> > hw/acpi/pci.c  
> 
> Today it's used only for PCI devices, but that's partly an artifact
> of how we get to the root complex via the bus below it.
> 
> Spec wise, it's just as applicable to platform devices etc, but maybe
> we can move it to pci.c for now and move it out again if it gains other
> users. Or leave it in acpi_generic_initiator.c but have all the aml
> stuff in aml_build.c as you suggest. 
> 
> > file if it has to access PCI code/structures directly
> > (which I'm not convinced it should, can we get/expose what it needs as QOM properties?)  
> 
> Maybe. I'll see what I can come up with.  This feels involved
> however so I'm more doubtful about this as a precursor.

This is a little messy and tricky to get the right level of generic.
For the bdf, were you thinking something along the lines of the following?

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 324c1302d2..75366491b7 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -67,6 +67,19 @@ static char *pcibus_get_fw_dev_path(DeviceState *dev);
 static void pcibus_reset_hold(Object *obj, ResetType type);
 static bool pcie_has_upstream_port(PCIDevice *dev);

+static void prop_pci_bdf_get(Object *obj, Visitor *v, const char *name,
+                             void *opaque, Error **errp)
+{
+    uint16_t bdf = pci_get_bdf(PCI_DEVICE(obj));
+
+    visit_type_uint16(v, name, &bdf, errp);
+}
+
+static const PropertyInfo prop_pci_bdf = {
+    .name = "bdf",
+    .get = prop_pci_bdf_get,
+};
+
 static Property pci_props[] = {
     DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, -1),
     DEFINE_PROP_STRING("romfile", PCIDevice, romfile),
@@ -85,6 +98,7 @@ static Property pci_props[] = {
                     QEMU_PCIE_ERR_UNC_MASK_BITNR, true),
     DEFINE_PROP_BIT("x-pcie-ari-nextfn-1", PCIDevice, cap_present,
                     QEMU_PCIE_ARI_NEXTFN_1_BITNR, false),
+    { .name = "bdf", .info = &prop_pci_bdf },
     DEFINE_PROP_END_OF_LIST()
 };


The other case is where I need to get the ACPI UID associate with a
root complex. Now that has to be matched to the appropriate HID and so
far the only one of those is ACPI0016 which is the HID for
TYPE_PXB_CXL_DEV. That happens to the bus number of the
TYPE_PXB_CXL_BUS but that connection should probably not be explicit
outside of the PXB specific code.

I can add a property like: 

diff --git a/hw/pci-bridge/pci_expander_bridge.c b/hw/pci-bridge/pci_expander_bridge.c
index f5431443b9..1c51f3f5b6 100644
--- a/hw/pci-bridge/pci_expander_bridge.c
+++ b/hw/pci-bridge/pci_expander_bridge.c
@@ -92,6 +92,21 @@ static void pxb_bus_class_init(ObjectClass *class, void *data)
     pbc->numa_node = pxb_bus_numa_node;
 }

+static void prop_pxb_cxl_uid_get(Object *obj, Visitor *v, const char *name,
+                             void *opaque, Error **errp)
+{
+    uint32_t uid = pci_bus_num(PCI_BUS(obj));
+
+    visit_type_uint32(v, name, &uid, errp);
+}
+
+static void pxb_cxl_bus_class_init(ObjectClass *class, void *data)
+{
+    pxb_bus_class_init(class, data);
+    object_class_property_add(class, "acpi_uid", "uint32",
+                              prop_pxb_cxl_uid_get, NULL, NULL, NULL);
+}
+
 static const TypeInfo pxb_bus_info = {
     .name          = TYPE_PXB_BUS,
     .parent        = TYPE_PCI_BUS,
@@ -110,7 +125,7 @@ static const TypeInfo pxb_cxl_bus_info = {
     .name          = TYPE_PXB_CXL_BUS,
     .parent        = TYPE_CXL_BUS,
     .instance_size = sizeof(PXBBus),
-    .class_init    = pxb_bus_class_init,
+    .class_init    = pxb_cxl_bus_class_init,
 };

 static const char *pxb_host_root_bus_path(PCIHostState *host_bridge,

and query it when setting up the generic port with

        const char *hid = "ACPI0016";
        uint32_t uid;

        if (gn->node >= ms->numa_state->num_nodes) {
            error_printf("%s: node %d is invalid.\n",
                         TYPE_ACPI_GENERIC_PORT, gn->node);
            exit(1);
        }

        o = object_resolve_path_type(gn->pci_dev, TYPE_PXB_CXL_BUS, NULL);
        if (!o) {
            error_printf("%s: device must be a CXL host bridge.\n",
                         TYPE_ACPI_GENERIC_PORT);
            exit(1);
        }

        uid = object_property_get_uint(o, "acpi_uid", &error_fatal);
        build_srat_acpi_generic_port(table_data, gn->node, hid, uid);

        return 0;

Thanks,

Jonathan

> 
> > 
> > btw:
> > build_all_acpi_generic_initiators() name doesn't match what it's doing.
> > it composes only one initiator entry.  
> 
> I'll look at tidying up all the relevant naming.
> 
> Jonathan
> 
> >   
> > >      }    
> > 
> >   
> 
> 
>