Re: [PATCH v3 0/6] hw/acpi/viot: generate stable VIOT ACPI tables

[Jean-Philippe Brucker]

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
>