mbox series

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

Message ID 20220525173232.31429-1-mark.cave-ayland@ilande.co.uk (mailing list archive)
Headers show
Series hw/acpi/viot: generate stable VIOT ACPI tables | expand

Message

Mark Cave-Ayland May 25, 2022, 5:32 p.m. UTC
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.

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

Comments

Jean-Philippe Brucker May 26, 2022, 2:23 p.m. UTC | #1
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
>