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

[Igor Mammedov <imammedo@redhat.com>]

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.

>      }