On Wed, May 25, 2022 at 06:32:26PM +0100, Mark Cave-Ayland wrote:
> I was working away at some improvements for PS2 devices when I noticed that one
> small change to the instantiation of a PS2 mouse device caused a regression in
> tests/qtest/bios-tables-test, specifically the /x86_64/acpi/q35/viot subtest.
>
> Closer examination of the failed test output showed the problem was that the
> order of the PCI host bridge entries had changed within the table causing the
> generated binary to fail to match the version in tests/data/acpi/q35/VIOT.viot.
>
> The error occurs because there is no guarantee in the order of PCI host bridges
> being returned from object_child_foreach_recursive() used within
> hw/acpi/viot.c's build_viot() function, so any change to the QOM tree can
> potentially change the generated ACPI VIOT table ordering and cause the
> regression tests to fail.
>
> Fortunately the solution is fairly easy: change build_viot() to build an array
> of PCI host bridges and then sort them first before generating the final ACPI
> VIOT table. I've chosen to sort the PCI host bridges based upon the min_bus
> number which seems to work okay here.
>
> The changes in this patchset were heavily inspired by the build_iort() function
> in hw/arm/virt-acpi-build.c which already does the right thing here. Patches 1-5
> make the required changes before patch 6 updates the VIOT binary to match the
> updated ACPI VIOT table ordering.
Thanks for the fix, looks good to me and I did some light testing
Reviewed-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>
> v3:
> - Rebase onto master
> - Add Reviewed-by tag from Ani in patch 1
> - Declare struct viot_pci_host_range as const in enumerate_pci_host_bridges() in patch 3
> - Add Reviewed-by tags for the series from Phil
>
> v2:
> - Rebase onto master
> - Rename pci_host_bridges() to enumerate_pci_host_bridges() in patch 1
> - Change return type of pci_host_range_compare() from int to gint in patch 5
> - Tweak subject line in patch 5: s/PCI host bus/PCI host bridge/
> - Add Acked-by and Reviewed-by tags from Ani
>
>
> Mark Cave-Ayland (6):
> hw/acpi/viot: rename build_pci_range_node() to
> enumerate_pci_host_bridges()
> hw/acpi/viot: move the individual PCI host bridge entry generation to
> a new function
> hw/acpi/viot: build array of PCI host bridges before generating VIOT
> ACPI table
> tests/acpi: virt: allow VIOT acpi table changes
> hw/acpi/viot: sort VIOT ACPI table entries by PCI host bridge min_bus
> tests/acpi: virt: update golden masters for VIOT
>
> hw/acpi/viot.c | 107 +++++++++++++++++++++-------------
> tests/data/acpi/q35/VIOT.viot | Bin 112 -> 112 bytes
> 2 files changed, 68 insertions(+), 39 deletions(-)
>
> --
> 2.20.1
>