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

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

[Jonathan Cameron via]

Treating the HID as an integer caused it to get bit reversed
on big endian hosts running little endian guests.  Treat it
as a character array instead.

Fixes hw/acpi: Generic Port Affinity Structure Support
Tested-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

---
Richard ran the version posted in the thread on an s390 instance.
Thanks for the help!

Difference from version in thread:
- Instantiate i in the for loop.

Sending out now so Michael can decide whether to fold this in, or
drop the GP series for now from his pull request (in which case
I'll do an updated version with this and Markus' docs feedback
folded in.)

---
 include/hw/acpi/acpi_generic_initiator.h | 2 +-
 hw/acpi/acpi_generic_initiator.c         | 4 +++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/include/hw/acpi/acpi_generic_initiator.h b/include/hw/acpi/acpi_generic_initiator.h
index 1a899af30f..5baefda33a 100644
--- a/include/hw/acpi/acpi_generic_initiator.h
+++ b/include/hw/acpi/acpi_generic_initiator.h
@@ -61,7 +61,7 @@ typedef struct PCIDeviceHandle {
             uint16_t bdf;
         };
         struct {
-            uint64_t hid;
+            char hid[8];
             uint32_t uid;
         };
     };
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);
     }
-- 
2.39.2

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

[Michael S. Tsirkin]

On Wed, Jun 05, 2024 at 07:04:55PM +0100, Jonathan Cameron wrote:
> Treating the HID as an integer caused it to get bit reversed
> on big endian hosts running little endian guests.  Treat it
> as a character array instead.
> 
> Fixes hw/acpi: Generic Port Affinity Structure Support
> Tested-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> ---
> Richard ran the version posted in the thread on an s390 instance.
> Thanks for the help!
> 
> Difference from version in thread:
> - Instantiate i in the for loop.
> 
> Sending out now so Michael can decide whether to fold this in, or
> drop the GP series for now from his pull request (in which case
> I'll do an updated version with this and Markus' docs feedback
> folded in.)


Dropped for now.


> ---
>  include/hw/acpi/acpi_generic_initiator.h | 2 +-
>  hw/acpi/acpi_generic_initiator.c         | 4 +++-
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/include/hw/acpi/acpi_generic_initiator.h b/include/hw/acpi/acpi_generic_initiator.h
> index 1a899af30f..5baefda33a 100644
> --- a/include/hw/acpi/acpi_generic_initiator.h
> +++ b/include/hw/acpi/acpi_generic_initiator.h
> @@ -61,7 +61,7 @@ typedef struct PCIDeviceHandle {
>              uint16_t bdf;
>          };
>          struct {
> -            uint64_t hid;
> +            char hid[8];
>              uint32_t uid;
>          };
>      };

I think there is another issue:

+        memcpy(&dev_handle.hid, hid, sizeof(dev_handle.hid));

not nice since there is no check that 8 will hold all of
+        const char *hid = "ACPI0016";
and won't access buffer out of range.




> 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);
>      }
> -- 
> 2.39.2

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

[Jonathan Cameron via]

On Wed, 5 Jun 2024 19:38:18 -0400
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Wed, Jun 05, 2024 at 07:04:55PM +0100, Jonathan Cameron wrote:
> > Treating the HID as an integer caused it to get bit reversed
> > on big endian hosts running little endian guests.  Treat it
> > as a character array instead.
> > 
> > Fixes hw/acpi: Generic Port Affinity Structure Support
> > Tested-by: Richard Henderson <richard.henderson@linaro.org>
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > 
> > ---
> > Richard ran the version posted in the thread on an s390 instance.
> > Thanks for the help!
> > 
> > Difference from version in thread:
> > - Instantiate i in the for loop.
> > 
> > Sending out now so Michael can decide whether to fold this in, or
> > drop the GP series for now from his pull request (in which case
> > I'll do an updated version with this and Markus' docs feedback
> > folded in.)  
> 
> 
> Dropped for now.
> 
> 
> > ---
> >  include/hw/acpi/acpi_generic_initiator.h | 2 +-
> >  hw/acpi/acpi_generic_initiator.c         | 4 +++-
> >  2 files changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/hw/acpi/acpi_generic_initiator.h b/include/hw/acpi/acpi_generic_initiator.h
> > index 1a899af30f..5baefda33a 100644
> > --- a/include/hw/acpi/acpi_generic_initiator.h
> > +++ b/include/hw/acpi/acpi_generic_initiator.h
> > @@ -61,7 +61,7 @@ typedef struct PCIDeviceHandle {
> >              uint16_t bdf;
> >          };
> >          struct {
> > -            uint64_t hid;
> > +            char hid[8];
> >              uint32_t uid;
> >          };
> >      };  
> 
> I think there is another issue:
> 
> +        memcpy(&dev_handle.hid, hid, sizeof(dev_handle.hid));
> 
> not nice since there is no check that 8 will hold all of
> +        const char *hid = "ACPI0016";
> and won't access buffer out of range.
> 

I think, in theory, that won't ever happen unless someone is using
an invalid ACPI ID as they 'must' be 8 chars (or a uint64_t which
would also be fine).  A bit of defensive programming seems
sensible though as there are known buggy real firmware's
that have invalid IDs so maybe one day someone will add one
of those to QEMU when we aren't paying attention.

I'll add a sanity check and treat such a value as an error.
It'll also act as documentation of the requirement.

if (strlen(hid) != sizeof(dev_handle.hid)) {
    error_printf("ACPI ID for generic port is not the expected 8 characters");
    exit(-1);	
}



> 
> 
> 
> > 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);
> >      }
> > -- 
> > 2.39.2  
> 
> 

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

[Igor Mammedov]

On Wed, 5 Jun 2024 19:04:55 +0100
Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:

> Treating the HID as an integer caused it to get bit reversed
> on big endian hosts running little endian guests.  Treat it
> as a character array instead.
> 
> Fixes hw/acpi: Generic Port Affinity Structure Support
> Tested-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> ---
> Richard ran the version posted in the thread on an s390 instance.
> Thanks for the help!
> 
> Difference from version in thread:
> - Instantiate i in the for loop.
> 
> Sending out now so Michael can decide whether to fold this in, or
> drop the GP series for now from his pull request (in which case
> I'll do an updated version with this and Markus' docs feedback
> folded in.)
> 
> ---
>  include/hw/acpi/acpi_generic_initiator.h | 2 +-
>  hw/acpi/acpi_generic_initiator.c         | 4 +++-
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/include/hw/acpi/acpi_generic_initiator.h b/include/hw/acpi/acpi_generic_initiator.h
> index 1a899af30f..5baefda33a 100644
> --- a/include/hw/acpi/acpi_generic_initiator.h
> +++ b/include/hw/acpi/acpi_generic_initiator.h
> @@ -61,7 +61,7 @@ typedef struct PCIDeviceHandle {
>              uint16_t bdf;
>          };
>          struct {
> -            uint64_t hid;
> +            char hid[8];
>              uint32_t uid;
>          };
>      };

not sure on top of what this patch applies but I have some generic comments wrt it

why PCIDeviceHandle is in header file? is there plan for it
being used outside of acpi_generic_initiator.c?


> 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

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 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?)

btw:
build_all_acpi_generic_initiators() name doesn't match what it's doing.
it composes only one initiator entry.

>      }

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

[Jonathan Cameron via]

On Thu, 6 Jun 2024 16:06:53 +0200
Igor Mammedov <imammedo@redhat.com> wrote:

> On Wed, 5 Jun 2024 19:04:55 +0100
> Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:
> 
> > Treating the HID as an integer caused it to get bit reversed
> > on big endian hosts running little endian guests.  Treat it
> > as a character array instead.
> > 
> > Fixes hw/acpi: Generic Port Affinity Structure Support
> > Tested-by: Richard Henderson <richard.henderson@linaro.org>
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > 
> > ---
> > Richard ran the version posted in the thread on an s390 instance.
> > Thanks for the help!
> > 
> > Difference from version in thread:
> > - Instantiate i in the for loop.
> > 
> > Sending out now so Michael can decide whether to fold this in, or
> > drop the GP series for now from his pull request (in which case
> > I'll do an updated version with this and Markus' docs feedback
> > folded in.)
> > 
> > ---
> >  include/hw/acpi/acpi_generic_initiator.h | 2 +-
> >  hw/acpi/acpi_generic_initiator.c         | 4 +++-
> >  2 files changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/hw/acpi/acpi_generic_initiator.h b/include/hw/acpi/acpi_generic_initiator.h
> > index 1a899af30f..5baefda33a 100644
> > --- a/include/hw/acpi/acpi_generic_initiator.h
> > +++ b/include/hw/acpi/acpi_generic_initiator.h
> > @@ -61,7 +61,7 @@ typedef struct PCIDeviceHandle {
> >              uint16_t bdf;
> >          };
> >          struct {
> > -            uint64_t hid;
> > +            char hid[8];
> >              uint32_t uid;
> >          };
> >      };  
> 
> not sure on top of what this patch applies but I have some generic comments wrt it

https://lore.kernel.org/qemu-devel/20240524100507.32106-1-Jonathan.Cameron@huawei.com/

Comments are all on elements of the existing upstream code, but I'm touching it
anyway so will look at making the improvements you suggest as new precursors
to v3 given we are going around again anyway.

> 
> why PCIDeviceHandle is in header file? is there plan for it
> being used outside of acpi_generic_initiator.c?

I'll add a precursor patch to my series that moves
it and anything else that should be more local.  May well move
to being local in aml_build.c given your later comments with the
various fields passed in as parameters.

> 
> 
> > 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.

> 
> 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.

> 
> 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

> 
> >      }  
> 
> 

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

[Jonathan Cameron via]

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
> 
> >   
> > >      }    
> > 
> >   
> 
> 
> 

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

[Igor Mammedov]

On Thu, 6 Jun 2024 18:47:16 +0100
Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:

> On Thu, 6 Jun 2024 16:06:53 +0200
> Igor Mammedov <imammedo@redhat.com> wrote:
> 
> > On Wed, 5 Jun 2024 19:04:55 +0100
> > Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:
> > 
> > > Treating the HID as an integer caused it to get bit reversed
> > > on big endian hosts running little endian guests.  Treat it
> > > as a character array instead.
> > > 
> > > Fixes hw/acpi: Generic Port Affinity Structure Support
> > > Tested-by: Richard Henderson <richard.henderson@linaro.org>
> > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > 
> > > ---
> > > Richard ran the version posted in the thread on an s390 instance.
> > > Thanks for the help!
> > > 
> > > Difference from version in thread:
> > > - Instantiate i in the for loop.
> > > 
> > > Sending out now so Michael can decide whether to fold this in, or
> > > drop the GP series for now from his pull request (in which case
> > > I'll do an updated version with this and Markus' docs feedback
> > > folded in.)
> > > 
> > > ---
> > >  include/hw/acpi/acpi_generic_initiator.h | 2 +-
> > >  hw/acpi/acpi_generic_initiator.c         | 4 +++-
> > >  2 files changed, 4 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/include/hw/acpi/acpi_generic_initiator.h b/include/hw/acpi/acpi_generic_initiator.h
> > > index 1a899af30f..5baefda33a 100644
> > > --- a/include/hw/acpi/acpi_generic_initiator.h
> > > +++ b/include/hw/acpi/acpi_generic_initiator.h
> > > @@ -61,7 +61,7 @@ typedef struct PCIDeviceHandle {
> > >              uint16_t bdf;
> > >          };
> > >          struct {
> > > -            uint64_t hid;
> > > +            char hid[8];
> > >              uint32_t uid;
> > >          };
> > >      };  
> > 
> > not sure on top of what this patch applies but I have some generic comments wrt it
> 
> https://lore.kernel.org/qemu-devel/20240524100507.32106-1-Jonathan.Cameron@huawei.com/
> 
> Comments are all on elements of the existing upstream code, but I'm touching it
> anyway so will look at making the improvements you suggest as new precursors
> to v3 given we are going around again anyway.
> 
> > 
> > why PCIDeviceHandle is in header file? is there plan for it
> > being used outside of acpi_generic_initiator.c?
> 
> I'll add a precursor patch to my series that moves
> it and anything else that should be more local.  May well move
> to being local in aml_build.c given your later comments with the
> various fields passed in as parameters.
> 
> > 
> > 
> > > 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.

the point of moving handle packing to aml-build.c,
is to isolate primitives that likely could be reused later on elsewhere
and hide little endiannes from API user.
So shuch errors as you are fixing wouldn't be easy to introduce
(as long as API does it right)


Also this API probably should take not packed BDF, i.e. something like this:
    build_append_srat_pci_device_handle(GArray *table_data, bus, dev, func)

Or a packed BDF as you suggest in the later email, but then API function wold have
to 'decode' that before putting numbers into table, which complicates things
and likely would pull in PCI deps to unpack BDF, which I'd rather avoid in
generic aml-build.c

> 
> > 
> > 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.
> 
> > 
> > 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
> 
> > 
> > >      }  
> > 
> > 
> 

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

[Jonathan Cameron via]

On Fri, 14 Jun 2024 12:57:25 +0200
Igor Mammedov <imammedo@redhat.com> wrote:

> On Thu, 6 Jun 2024 18:47:16 +0100
> Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
> 
> > On Thu, 6 Jun 2024 16:06:53 +0200
> > Igor Mammedov <imammedo@redhat.com> wrote:
> >   
> > > On Wed, 5 Jun 2024 19:04:55 +0100
> > > Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:
> > >   
> > > > Treating the HID as an integer caused it to get bit reversed
> > > > on big endian hosts running little endian guests.  Treat it
> > > > as a character array instead.
> > > > 
> > > > Fixes hw/acpi: Generic Port Affinity Structure Support
> > > > Tested-by: Richard Henderson <richard.henderson@linaro.org>
> > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > 
> > > > ---
> > > > Richard ran the version posted in the thread on an s390 instance.
> > > > Thanks for the help!
> > > > 
> > > > Difference from version in thread:
> > > > - Instantiate i in the for loop.
> > > > 
> > > > Sending out now so Michael can decide whether to fold this in, or
> > > > drop the GP series for now from his pull request (in which case
> > > > I'll do an updated version with this and Markus' docs feedback
> > > > folded in.)
> > > > 
> > > > ---
> > > >  include/hw/acpi/acpi_generic_initiator.h | 2 +-
> > > >  hw/acpi/acpi_generic_initiator.c         | 4 +++-
> > > >  2 files changed, 4 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/include/hw/acpi/acpi_generic_initiator.h b/include/hw/acpi/acpi_generic_initiator.h
> > > > index 1a899af30f..5baefda33a 100644
> > > > --- a/include/hw/acpi/acpi_generic_initiator.h
> > > > +++ b/include/hw/acpi/acpi_generic_initiator.h
> > > > @@ -61,7 +61,7 @@ typedef struct PCIDeviceHandle {
> > > >              uint16_t bdf;
> > > >          };
> > > >          struct {
> > > > -            uint64_t hid;
> > > > +            char hid[8];
> > > >              uint32_t uid;
> > > >          };
> > > >      };    
> > > 
> > > not sure on top of what this patch applies but I have some generic comments wrt it  
> > 
> > https://lore.kernel.org/qemu-devel/20240524100507.32106-1-Jonathan.Cameron@huawei.com/
> > 
> > Comments are all on elements of the existing upstream code, but I'm touching it
> > anyway so will look at making the improvements you suggest as new precursors
> > to v3 given we are going around again anyway.
> >   
> > > 
> > > why PCIDeviceHandle is in header file? is there plan for it
> > > being used outside of acpi_generic_initiator.c?  
> > 
> > I'll add a precursor patch to my series that moves
> > it and anything else that should be more local.  May well move
> > to being local in aml_build.c given your later comments with the
> > various fields passed in as parameters.
> >   
> > > 
> > >   
> > > > 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.  
> 
> the point of moving handle packing to aml-build.c,
> is to isolate primitives that likely could be reused later on elsewhere
> and hide little endiannes from API user.
> So shuch errors as you are fixing wouldn't be easy to introduce
> (as long as API does it right)
> 
> 
> Also this API probably should take not packed BDF, i.e. something like this:
>     build_append_srat_pci_device_handle(GArray *table_data, bus, dev, func)
> 
> Or a packed BDF as you suggest in the later email, but then API function wold have
> to 'decode' that before putting numbers into table, which complicates things
> and likely would pull in PCI deps to unpack BDF, which I'd rather avoid in
> generic aml-build.c

Ok. I can split it up. My motivation for the encoded version was that
the spec field is defined as a 2 byte field, but it is also broken out
in the description into byte 2 then various bits of byte 3 so we can construct
it that way instead. (were it defined as bits in the 16 bit field this would
make less sense).  Should still be obvious enough to anyone trying to
correlate the two.

Splitting the devfn bit up though is tricky as that doesn't really
have a separate meaning in PCI any more given ARI where it becomes an 8 bit
function ID. So probably makes more sense to keep that as devfn as it's
coming from pci->devfn in that form anyway and they both forms get encoded
into a byte anyway.

Jonathan


> 
> >   
> > > 
> > > 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.
> >   
> > > 
> > > 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
> >   
> > >   
> > > >      }    
> > > 
> > >   
> >   
> 
> 

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

[Igor Mammedov]

On Fri, 14 Jun 2024 15:08:35 +0100
Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:

> On Fri, 14 Jun 2024 12:57:25 +0200
> Igor Mammedov <imammedo@redhat.com> wrote:
> 
> > On Thu, 6 Jun 2024 18:47:16 +0100
> > Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
> >   
> > > On Thu, 6 Jun 2024 16:06:53 +0200
> > > Igor Mammedov <imammedo@redhat.com> wrote:
> > >     
> > > > On Wed, 5 Jun 2024 19:04:55 +0100
> > > > Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:
> > > >     
> > > > > Treating the HID as an integer caused it to get bit reversed
> > > > > on big endian hosts running little endian guests.  Treat it
> > > > > as a character array instead.
> > > > > 
> > > > > Fixes hw/acpi: Generic Port Affinity Structure Support
> > > > > Tested-by: Richard Henderson <richard.henderson@linaro.org>
> > > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > > 
> > > > > ---
> > > > > Richard ran the version posted in the thread on an s390 instance.
> > > > > Thanks for the help!
> > > > > 
> > > > > Difference from version in thread:
> > > > > - Instantiate i in the for loop.
> > > > > 
> > > > > Sending out now so Michael can decide whether to fold this in, or
> > > > > drop the GP series for now from his pull request (in which case
> > > > > I'll do an updated version with this and Markus' docs feedback
> > > > > folded in.)
> > > > > 
> > > > > ---
> > > > >  include/hw/acpi/acpi_generic_initiator.h | 2 +-
> > > > >  hw/acpi/acpi_generic_initiator.c         | 4 +++-
> > > > >  2 files changed, 4 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/include/hw/acpi/acpi_generic_initiator.h b/include/hw/acpi/acpi_generic_initiator.h
> > > > > index 1a899af30f..5baefda33a 100644
> > > > > --- a/include/hw/acpi/acpi_generic_initiator.h
> > > > > +++ b/include/hw/acpi/acpi_generic_initiator.h
> > > > > @@ -61,7 +61,7 @@ typedef struct PCIDeviceHandle {
> > > > >              uint16_t bdf;
> > > > >          };
> > > > >          struct {
> > > > > -            uint64_t hid;
> > > > > +            char hid[8];
> > > > >              uint32_t uid;
> > > > >          };
> > > > >      };      
> > > > 
> > > > not sure on top of what this patch applies but I have some generic comments wrt it    
> > > 
> > > https://lore.kernel.org/qemu-devel/20240524100507.32106-1-Jonathan.Cameron@huawei.com/
> > > 
> > > Comments are all on elements of the existing upstream code, but I'm touching it
> > > anyway so will look at making the improvements you suggest as new precursors
> > > to v3 given we are going around again anyway.
> > >     
> > > > 
> > > > why PCIDeviceHandle is in header file? is there plan for it
> > > > being used outside of acpi_generic_initiator.c?    
> > > 
> > > I'll add a precursor patch to my series that moves
> > > it and anything else that should be more local.  May well move
> > > to being local in aml_build.c given your later comments with the
> > > various fields passed in as parameters.
> > >     
> > > > 
> > > >     
> > > > > 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.    
> > 
> > the point of moving handle packing to aml-build.c,
> > is to isolate primitives that likely could be reused later on elsewhere
> > and hide little endiannes from API user.
> > So shuch errors as you are fixing wouldn't be easy to introduce
> > (as long as API does it right)
> > 
> > 
> > Also this API probably should take not packed BDF, i.e. something like this:
> >     build_append_srat_pci_device_handle(GArray *table_data, bus, dev, func)
> > 
> > Or a packed BDF as you suggest in the later email, but then API function wold have
> > to 'decode' that before putting numbers into table, which complicates things
> > and likely would pull in PCI deps to unpack BDF, which I'd rather avoid in
> > generic aml-build.c  
> 
> Ok. I can split it up. My motivation for the encoded version was that
> the spec field is defined as a 2 byte field, but it is also broken out
> in the description into byte 2 then various bits of byte 3 so we can construct
> it that way instead. (were it defined as bits in the 16 bit field this would
> make less sense).  Should still be obvious enough to anyone trying to
> correlate the two.
> 
> Splitting the devfn bit up though is tricky as that doesn't really
> have a separate meaning in PCI any more given ARI where it becomes an 8 bit
> function ID. So probably makes more sense to keep that as devfn as it's
> coming from pci->devfn in that form anyway and they both forms get encoded
> into a byte anyway.

ack, 
as long as acpi API user doesn't have to worry about byte order it should be fine

> 
> Jonathan
> 
> 
> >   
> > >     
> > > > 
> > > > 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.
> > >     
> > > > 
> > > > 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
> > >     
> > > >     
> > > > >      }      
> > > > 
> > > >     
> > >     
> > 
> >   
> 

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

[Jonathan Cameron via]

On Fri, 14 Jun 2024 15:08:35 +0100
Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:

> On Fri, 14 Jun 2024 12:57:25 +0200
> Igor Mammedov <imammedo@redhat.com> wrote:
> 
> > On Thu, 6 Jun 2024 18:47:16 +0100
> > Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
> >   
> > > On Thu, 6 Jun 2024 16:06:53 +0200
> > > Igor Mammedov <imammedo@redhat.com> wrote:
> > >     
> > > > On Wed, 5 Jun 2024 19:04:55 +0100
> > > > Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:
> > > >     
> > > > > Treating the HID as an integer caused it to get bit reversed
> > > > > on big endian hosts running little endian guests.  Treat it
> > > > > as a character array instead.
> > > > > 
> > > > > Fixes hw/acpi: Generic Port Affinity Structure Support
> > > > > Tested-by: Richard Henderson <richard.henderson@linaro.org>
> > > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > > 
> > > > > ---
> > > > > Richard ran the version posted in the thread on an s390 instance.
> > > > > Thanks for the help!
> > > > > 
> > > > > Difference from version in thread:
> > > > > - Instantiate i in the for loop.
> > > > > 
> > > > > Sending out now so Michael can decide whether to fold this in, or
> > > > > drop the GP series for now from his pull request (in which case
> > > > > I'll do an updated version with this and Markus' docs feedback
> > > > > folded in.)
> > > > > 
> > > > > ---
> > > > >  include/hw/acpi/acpi_generic_initiator.h | 2 +-
> > > > >  hw/acpi/acpi_generic_initiator.c         | 4 +++-
> > > > >  2 files changed, 4 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/include/hw/acpi/acpi_generic_initiator.h b/include/hw/acpi/acpi_generic_initiator.h
> > > > > index 1a899af30f..5baefda33a 100644
> > > > > --- a/include/hw/acpi/acpi_generic_initiator.h
> > > > > +++ b/include/hw/acpi/acpi_generic_initiator.h
> > > > > @@ -61,7 +61,7 @@ typedef struct PCIDeviceHandle {
> > > > >              uint16_t bdf;
> > > > >          };
> > > > >          struct {
> > > > > -            uint64_t hid;
> > > > > +            char hid[8];
> > > > >              uint32_t uid;
> > > > >          };
> > > > >      };      
> > > > 
> > > > not sure on top of what this patch applies but I have some generic comments wrt it    
> > > 
> > > https://lore.kernel.org/qemu-devel/20240524100507.32106-1-Jonathan.Cameron@huawei.com/
> > > 
> > > Comments are all on elements of the existing upstream code, but I'm touching it
> > > anyway so will look at making the improvements you suggest as new precursors
> > > to v3 given we are going around again anyway.
> > >     
> > > > 
> > > > why PCIDeviceHandle is in header file? is there plan for it
> > > > being used outside of acpi_generic_initiator.c?    
> > > 
> > > I'll add a precursor patch to my series that moves
> > > it and anything else that should be more local.  May well move
> > > to being local in aml_build.c given your later comments with the
> > > various fields passed in as parameters.
> > >     
> > > > 
> > > >     
> > > > > 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.    
> > 
> > the point of moving handle packing to aml-build.c,
> > is to isolate primitives that likely could be reused later on elsewhere
> > and hide little endiannes from API user.
> > So shuch errors as you are fixing wouldn't be easy to introduce
> > (as long as API does it right)
> > 
> > 
> > Also this API probably should take not packed BDF, i.e. something like this:
> >     build_append_srat_pci_device_handle(GArray *table_data, bus, dev, func)
> > 
> > Or a packed BDF as you suggest in the later email, but then API function wold have
> > to 'decode' that before putting numbers into table, which complicates things
> > and likely would pull in PCI deps to unpack BDF, which I'd rather avoid in
> > generic aml-build.c  
> 
> Ok. I can split it up. My motivation for the encoded version was that
> the spec field is defined as a 2 byte field, but it is also broken out
> in the description into byte 2 then various bits of byte 3 so we can construct
> it that way instead. (were it defined as bits in the 16 bit field this would
> make less sense).  Should still be obvious enough to anyone trying to
> correlate the two.
> 
> Splitting the devfn bit up though is tricky as that doesn't really
> have a separate meaning in PCI any more given ARI where it becomes an 8 bit
> function ID. So probably makes more sense to keep that as devfn as it's
> coming from pci->devfn in that form anyway and they both forms get encoded
> into a byte anyway.

I guess it was inevitable (and perhaps you had this thought).  QEMU ordering
of BDF has the bytes swapped wrt to the ACPI ordering.  With them split the
test was failing.  I'll also tweak the test to ensure we have a non 0 bus
number (by inserting a root port rather than having the generic initiator
on the root bridge bus 0).

Ankit.  Can you confirm if you were seeing these reversed?

https://elixir.bootlin.com/qemu/v9.0.1/source/hw/acpi/acpi_generic_initiator.c#L134
uses PCI_BUIILD_BDF() which has bus << 8
The ACPI write is little endian, so bus ends up in byte 3 but should be in byte 2.

Jonathan

> 
> Jonathan
> 
> 
> >   
> > >     
> > > > 
> > > > 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.
> > >     
> > > > 
> > > > 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
> > >     
> > > >     
> > > > >      }      
> > > > 
> > > >     
> > >     
> > 
> >   
> 
> 

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

[Igor Mammedov]

On Mon, 17 Jun 2024 13:05:15 +0100
Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:

> On Fri, 14 Jun 2024 15:08:35 +0100
> Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
> 
> > On Fri, 14 Jun 2024 12:57:25 +0200
> > Igor Mammedov <imammedo@redhat.com> wrote:
> >   
> > > On Thu, 6 Jun 2024 18:47:16 +0100
> > > Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
> > >     
> > > > On Thu, 6 Jun 2024 16:06:53 +0200
> > > > Igor Mammedov <imammedo@redhat.com> wrote:
> > > >       
> > > > > On Wed, 5 Jun 2024 19:04:55 +0100
> > > > > Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:
> > > > >       
> > > > > > Treating the HID as an integer caused it to get bit reversed
> > > > > > on big endian hosts running little endian guests.  Treat it
> > > > > > as a character array instead.
> > > > > > 
> > > > > > Fixes hw/acpi: Generic Port Affinity Structure Support
> > > > > > Tested-by: Richard Henderson <richard.henderson@linaro.org>
> > > > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > > > 
> > > > > > ---
> > > > > > Richard ran the version posted in the thread on an s390 instance.
> > > > > > Thanks for the help!
> > > > > > 
> > > > > > Difference from version in thread:
> > > > > > - Instantiate i in the for loop.
> > > > > > 
> > > > > > Sending out now so Michael can decide whether to fold this in, or
> > > > > > drop the GP series for now from his pull request (in which case
> > > > > > I'll do an updated version with this and Markus' docs feedback
> > > > > > folded in.)
> > > > > > 
> > > > > > ---
> > > > > >  include/hw/acpi/acpi_generic_initiator.h | 2 +-
> > > > > >  hw/acpi/acpi_generic_initiator.c         | 4 +++-
> > > > > >  2 files changed, 4 insertions(+), 2 deletions(-)
> > > > > > 
> > > > > > diff --git a/include/hw/acpi/acpi_generic_initiator.h b/include/hw/acpi/acpi_generic_initiator.h
> > > > > > index 1a899af30f..5baefda33a 100644
> > > > > > --- a/include/hw/acpi/acpi_generic_initiator.h
> > > > > > +++ b/include/hw/acpi/acpi_generic_initiator.h
> > > > > > @@ -61,7 +61,7 @@ typedef struct PCIDeviceHandle {
> > > > > >              uint16_t bdf;
> > > > > >          };
> > > > > >          struct {
> > > > > > -            uint64_t hid;
> > > > > > +            char hid[8];
> > > > > >              uint32_t uid;
> > > > > >          };
> > > > > >      };        
> > > > > 
> > > > > not sure on top of what this patch applies but I have some generic comments wrt it      
> > > > 
> > > > https://lore.kernel.org/qemu-devel/20240524100507.32106-1-Jonathan.Cameron@huawei.com/
> > > > 
> > > > Comments are all on elements of the existing upstream code, but I'm touching it
> > > > anyway so will look at making the improvements you suggest as new precursors
> > > > to v3 given we are going around again anyway.
> > > >       
> > > > > 
> > > > > why PCIDeviceHandle is in header file? is there plan for it
> > > > > being used outside of acpi_generic_initiator.c?      
> > > > 
> > > > I'll add a precursor patch to my series that moves
> > > > it and anything else that should be more local.  May well move
> > > > to being local in aml_build.c given your later comments with the
> > > > various fields passed in as parameters.
> > > >       
> > > > > 
> > > > >       
> > > > > > 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.      
> > > 
> > > the point of moving handle packing to aml-build.c,
> > > is to isolate primitives that likely could be reused later on elsewhere
> > > and hide little endiannes from API user.
> > > So shuch errors as you are fixing wouldn't be easy to introduce
> > > (as long as API does it right)
> > > 
> > > 
> > > Also this API probably should take not packed BDF, i.e. something like this:
> > >     build_append_srat_pci_device_handle(GArray *table_data, bus, dev, func)
> > > 
> > > Or a packed BDF as you suggest in the later email, but then API function wold have
> > > to 'decode' that before putting numbers into table, which complicates things
> > > and likely would pull in PCI deps to unpack BDF, which I'd rather avoid in
> > > generic aml-build.c    
> > 
> > Ok. I can split it up. My motivation for the encoded version was that
> > the spec field is defined as a 2 byte field, but it is also broken out
> > in the description into byte 2 then various bits of byte 3 so we can construct
> > it that way instead. (were it defined as bits in the 16 bit field this would
> > make less sense).  Should still be obvious enough to anyone trying to
> > correlate the two.
> > 
> > Splitting the devfn bit up though is tricky as that doesn't really
> > have a separate meaning in PCI any more given ARI where it becomes an 8 bit
> > function ID. So probably makes more sense to keep that as devfn as it's
> > coming from pci->devfn in that form anyway and they both forms get encoded
> > into a byte anyway.  
> 
> I guess it was inevitable (and perhaps you had this thought).  QEMU ordering
> of BDF has the bytes swapped wrt to the ACPI ordering.  With them split the
> test was failing.  I'll also tweak the test to ensure we have a non 0 bus
> number (by inserting a root port rather than having the generic initiator
> on the root bridge bus 0).

that's why I asked for split BDF (into parts) for acpi API to provide endian
agonstic interface. Internally acpi functions can convert/pack arguments as
necessary. And it would be easier to validate/review by just comparing
with spec description.

> 
> Ankit.  Can you confirm if you were seeing these reversed?
> 
> https://elixir.bootlin.com/qemu/v9.0.1/source/hw/acpi/acpi_generic_initiator.c#L134
> uses PCI_BUIILD_BDF() which has bus << 8
> The ACPI write is little endian, so bus ends up in byte 3 but should be in byte 2.
> 
> Jonathan
> 
> > 
> > Jonathan
> > 
> >   
> > >     
> > > >       
> > > > > 
> > > > > 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.
> > > >       
> > > > > 
> > > > > 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
> > > >       
> > > > >       
> > > > > >      }        
> > > > > 
> > > > >       
> > > >       
> > > 
> > >     
> > 
> >   
>