On Wed, 5 Jun 2024 19:04:55 +0100
Jonathan Cameron <jonathan.came...@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.hender...@linaro.org>
> Signed-off-by: Jonathan Cameron <jonathan.came...@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.
> }
