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

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

[Mark Cave-Ayland]

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

-- 
2.20.1

[PATCH v3 1/6] hw/acpi/viot: rename build_pci_range_node() to enumerate_pci_host_bridges()

[Mark Cave-Ayland]

This is in preparation for separating out the VIOT ACPI table build from the
PCI host bridge numeration.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Ani Sinha <ani@anisinha.ca>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/acpi/viot.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/acpi/viot.c b/hw/acpi/viot.c
index c1af75206e..a41daded71 100644
--- a/hw/acpi/viot.c
+++ b/hw/acpi/viot.c
@@ -17,7 +17,7 @@ struct viot_pci_ranges {
 };
 
 /* Build PCI range for a given PCI host bridge */
-static int build_pci_range_node(Object *obj, void *opaque)
+static int enumerate_pci_host_bridges(Object *obj, void *opaque)
 {
     struct viot_pci_ranges *pci_ranges = opaque;
     GArray *blob = pci_ranges->blob;
@@ -78,7 +78,7 @@ void build_viot(MachineState *ms, GArray *table_data, BIOSLinker *linker,
     };
 
     /* Build the list of PCI ranges that this viommu manages */
-    object_child_foreach_recursive(OBJECT(ms), build_pci_range_node,
+    object_child_foreach_recursive(OBJECT(ms), enumerate_pci_host_bridges,
                                    &pci_ranges);
 
     /* ACPI table header */
-- 
2.20.1

[PATCH v3 2/6] hw/acpi/viot: move the individual PCI host bridge entry generation to a new function

[Mark Cave-Ayland]

Instead of generating each table entry inline, move the individual PCI host bridge
table entry generation to a separate build_pci_host_range() function.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Ani Sinha <ani@anisinha.ca>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/acpi/viot.c | 48 +++++++++++++++++++++++++++---------------------
 1 file changed, 27 insertions(+), 21 deletions(-)

diff --git a/hw/acpi/viot.c b/hw/acpi/viot.c
index a41daded71..5dafcbf5ef 100644
--- a/hw/acpi/viot.c
+++ b/hw/acpi/viot.c
@@ -16,6 +16,31 @@ struct viot_pci_ranges {
     uint16_t output_node;
 };
 
+static void build_pci_host_range(GArray *table_data, int min_bus, int max_bus,
+                                 uint16_t output_node)
+{
+    /* Type */
+    build_append_int_noprefix(table_data, 1 /* PCI range */, 1);
+    /* Reserved */
+    build_append_int_noprefix(table_data, 0, 1);
+    /* Length */
+    build_append_int_noprefix(table_data, 24, 2);
+    /* Endpoint start */
+    build_append_int_noprefix(table_data, PCI_BUILD_BDF(min_bus, 0), 4);
+    /* PCI Segment start */
+    build_append_int_noprefix(table_data, 0, 2);
+    /* PCI Segment end */
+    build_append_int_noprefix(table_data, 0, 2);
+    /* PCI BDF start */
+    build_append_int_noprefix(table_data, PCI_BUILD_BDF(min_bus, 0), 2);
+    /* PCI BDF end */
+    build_append_int_noprefix(table_data, PCI_BUILD_BDF(max_bus, 0xff), 2);
+    /* Output node */
+    build_append_int_noprefix(table_data, output_node, 2);
+    /* Reserved */
+    build_append_int_noprefix(table_data, 0, 6);
+}
+
 /* Build PCI range for a given PCI host bridge */
 static int enumerate_pci_host_bridges(Object *obj, void *opaque)
 {
@@ -30,27 +55,8 @@ static int enumerate_pci_host_bridges(Object *obj, void *opaque)
 
             pci_bus_range(bus, &min_bus, &max_bus);
 
-            /* Type */
-            build_append_int_noprefix(blob, 1 /* PCI range */, 1);
-            /* Reserved */
-            build_append_int_noprefix(blob, 0, 1);
-            /* Length */
-            build_append_int_noprefix(blob, 24, 2);
-            /* Endpoint start */
-            build_append_int_noprefix(blob, PCI_BUILD_BDF(min_bus, 0), 4);
-            /* PCI Segment start */
-            build_append_int_noprefix(blob, 0, 2);
-            /* PCI Segment end */
-            build_append_int_noprefix(blob, 0, 2);
-            /* PCI BDF start */
-            build_append_int_noprefix(blob, PCI_BUILD_BDF(min_bus, 0), 2);
-            /* PCI BDF end */
-            build_append_int_noprefix(blob, PCI_BUILD_BDF(max_bus, 0xff), 2);
-            /* Output node */
-            build_append_int_noprefix(blob, pci_ranges->output_node, 2);
-            /* Reserved */
-            build_append_int_noprefix(blob, 0, 6);
-
+            build_pci_host_range(blob, min_bus, max_bus,
+                                 pci_ranges->output_node);
             pci_ranges->count++;
         }
     }
-- 
2.20.1

[PATCH v3 3/6] hw/acpi/viot: build array of PCI host bridges before generating VIOT ACPI table

[Mark Cave-Ayland]

Perform the generation of the VIOT ACPI table in 2 separate passes: the first pass
enumerates all of the PCI host bridges and adds the min_bus and max_bus information
to an array.

Once this is done the VIOT table header is generated using the size of the array
to calculate the node count, which means it is no longer necessary to use a
sub-array to hold the PCI host bridge range information along with viommu_off.

Finally the PCI host bridge array is iterated again to add the required entries
to the final VIOT ACPI table.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Ani Sinha <ani@anisinha.ca>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/acpi/viot.c | 42 ++++++++++++++++++++++++------------------
 1 file changed, 24 insertions(+), 18 deletions(-)

diff --git a/hw/acpi/viot.c b/hw/acpi/viot.c
index 5dafcbf5ef..c32bbdd180 100644
--- a/hw/acpi/viot.c
+++ b/hw/acpi/viot.c
@@ -10,10 +10,9 @@
 #include "hw/pci/pci.h"
 #include "hw/pci/pci_host.h"
 
-struct viot_pci_ranges {
-    GArray *blob;
-    size_t count;
-    uint16_t output_node;
+struct viot_pci_host_range {
+    int min_bus;
+    int max_bus;
 };
 
 static void build_pci_host_range(GArray *table_data, int min_bus, int max_bus,
@@ -44,8 +43,7 @@ static void build_pci_host_range(GArray *table_data, int min_bus, int max_bus,
 /* Build PCI range for a given PCI host bridge */
 static int enumerate_pci_host_bridges(Object *obj, void *opaque)
 {
-    struct viot_pci_ranges *pci_ranges = opaque;
-    GArray *blob = pci_ranges->blob;
+    GArray *pci_host_ranges = opaque;
 
     if (object_dynamic_cast(obj, TYPE_PCI_HOST_BRIDGE)) {
         PCIBus *bus = PCI_HOST_BRIDGE(obj)->bus;
@@ -55,9 +53,11 @@ static int enumerate_pci_host_bridges(Object *obj, void *opaque)
 
             pci_bus_range(bus, &min_bus, &max_bus);
 
-            build_pci_host_range(blob, min_bus, max_bus,
-                                 pci_ranges->output_node);
-            pci_ranges->count++;
+            const struct viot_pci_host_range pci_host_range = {
+                .min_bus = min_bus,
+                .max_bus = max_bus,
+            };
+            g_array_append_val(pci_host_ranges, pci_host_range);
         }
     }
 
@@ -78,19 +78,19 @@ void build_viot(MachineState *ms, GArray *table_data, BIOSLinker *linker,
     int viommu_off = 48;
     AcpiTable table = { .sig = "VIOT", .rev = 0,
                         .oem_id = oem_id, .oem_table_id = oem_table_id };
-    struct viot_pci_ranges pci_ranges = {
-        .output_node = viommu_off,
-        .blob = g_array_new(false, true /* clear */, 1),
-    };
+    GArray *pci_host_ranges =  g_array_new(false, true,
+                                           sizeof(struct viot_pci_host_range));
+    struct viot_pci_host_range *pci_host_range;
+    int i;
 
     /* Build the list of PCI ranges that this viommu manages */
     object_child_foreach_recursive(OBJECT(ms), enumerate_pci_host_bridges,
-                                   &pci_ranges);
+                                   pci_host_ranges);
 
     /* ACPI table header */
     acpi_table_begin(&table, table_data);
     /* Node count */
-    build_append_int_noprefix(table_data, pci_ranges.count + 1, 2);
+    build_append_int_noprefix(table_data, pci_host_ranges->len + 1, 2);
     /* Node offset */
     build_append_int_noprefix(table_data, viommu_off, 2);
     /* Reserved */
@@ -111,9 +111,15 @@ void build_viot(MachineState *ms, GArray *table_data, BIOSLinker *linker,
     build_append_int_noprefix(table_data, 0, 8);
 
     /* PCI ranges found above */
-    g_array_append_vals(table_data, pci_ranges.blob->data,
-                        pci_ranges.blob->len);
-    g_array_free(pci_ranges.blob, true);
+    for (i = 0; i < pci_host_ranges->len; i++) {
+        pci_host_range = &g_array_index(pci_host_ranges,
+                                        struct viot_pci_host_range, i);
+
+        build_pci_host_range(table_data, pci_host_range->min_bus,
+                             pci_host_range->max_bus, viommu_off);
+    }
+
+    g_array_free(pci_host_ranges, true);
 
     acpi_table_end(linker, &table);
 }
-- 
2.20.1

[PATCH v3 4/6] tests/acpi: virt: allow VIOT acpi table changes

[Mark Cave-Ayland]

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Acked-by: Ani Sinha <ani@anisinha.ca>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 tests/qtest/bios-tables-test-allowed-diff.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
index dfb8523c8b..8367ffe1d4 100644
--- a/tests/qtest/bios-tables-test-allowed-diff.h
+++ b/tests/qtest/bios-tables-test-allowed-diff.h
@@ -1 +1,2 @@
 /* List of comma-separated changed AML files to ignore */
+"tests/data/acpi/virt/VIOT",
-- 
2.20.1

[PATCH v3 5/6] hw/acpi/viot: sort VIOT ACPI table entries by PCI host bridge min_bus

[Mark Cave-Ayland]

This ensures that the VIOT ACPI table output is always stable for a given PCI
topology by ensuring that entries are ordered according to min_bus.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Ani Sinha <ani@anisinha.ca>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/acpi/viot.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/hw/acpi/viot.c b/hw/acpi/viot.c
index c32bbdd180..4e0bf69067 100644
--- a/hw/acpi/viot.c
+++ b/hw/acpi/viot.c
@@ -64,6 +64,20 @@ static int enumerate_pci_host_bridges(Object *obj, void *opaque)
     return 0;
 }
 
+static gint pci_host_range_compare(gconstpointer a, gconstpointer b)
+{
+    struct viot_pci_host_range *range_a = (struct viot_pci_host_range *)a;
+    struct viot_pci_host_range *range_b = (struct viot_pci_host_range *)b;
+
+    if (range_a->min_bus < range_b->min_bus) {
+        return -1;
+    } else if (range_a->min_bus > range_b->min_bus) {
+        return 1;
+    } else {
+        return 0;
+    }
+}
+
 /*
  * Generate a VIOT table with one PCI-based virtio-iommu that manages PCI
  * endpoints.
@@ -87,6 +101,9 @@ void build_viot(MachineState *ms, GArray *table_data, BIOSLinker *linker,
     object_child_foreach_recursive(OBJECT(ms), enumerate_pci_host_bridges,
                                    pci_host_ranges);
 
+    /* Sort the pci host ranges by min_bus */
+    g_array_sort(pci_host_ranges, pci_host_range_compare);
+
     /* ACPI table header */
     acpi_table_begin(&table, table_data);
     /* Node count */
-- 
2.20.1

[PATCH v3 6/6] tests/acpi: virt: update golden masters for VIOT

[Mark Cave-Ayland]

Differences between disassembled ASL files for VIOT:

+++ /tmp/asl-V69GM1.dsl 2022-05-18 10:22:27.239796759 +0100
@@ -36,11 +36,11 @@
 [041h 0065   1]                     Reserved : 00
 [042h 0066   2]                       Length : 0018

-[044h 0068   4]               Endpoint start : 00003000
+[044h 0068   4]               Endpoint start : 00001000
 [048h 0072   2]            PCI Segment start : 0000
 [04Ah 0074   2]              PCI Segment end : 0000
-[04Ch 0076   2]                PCI BDF start : 3000
-[04Eh 0078   2]                  PCI BDF end : 30FF
+[04Ch 0076   2]                PCI BDF start : 1000
+[04Eh 0078   2]                  PCI BDF end : 10FF
 [050h 0080   2]                  Output node : 0030
 [052h 0082   6]                     Reserved : 000000000000

@@ -48,11 +48,11 @@
 [059h 0089   1]                     Reserved : 00
 [05Ah 0090   2]                       Length : 0018

-[05Ch 0092   4]               Endpoint start : 00001000
+[05Ch 0092   4]               Endpoint start : 00003000
 [060h 0096   2]            PCI Segment start : 0000
 [062h 0098   2]              PCI Segment end : 0000
-[064h 0100   2]                PCI BDF start : 1000
-[066h 0102   2]                  PCI BDF end : 10FF
+[064h 0100   2]                PCI BDF start : 3000
+[066h 0102   2]                  PCI BDF end : 30FF
 [068h 0104   2]                  Output node : 0030
 [06Ah 0106   6]                     Reserved : 000000000000

@@ -62,6 +62,6 @@
     0010: 42 58 50 43 20 20 20 20 01 00 00 00 42 58 50 43  // BXPC    ....BXPC
     0020: 01 00 00 00 03 00 30 00 00 00 00 00 00 00 00 00  // ......0.........
     0030: 03 00 10 00 00 00 10 00 00 00 00 00 00 00 00 00  // ................
-    0040: 01 00 18 00 00 30 00 00 00 00 00 00 00 30 FF 30  // .....0.......0.0
-    0050: 30 00 00 00 00 00 00 00 01 00 18 00 00 10 00 00  // 0...............
-    0060: 00 00 00 00 00 10 FF 10 30 00 00 00 00 00 00 00  // ........0.......
+    0040: 01 00 18 00 00 10 00 00 00 00 00 00 00 10 FF 10  // ................
+    0050: 30 00 00 00 00 00 00 00 01 00 18 00 00 30 00 00  // 0............0..
+    0060: 00 00 00 00 00 30 FF 30 30 00 00 00 00 00 00 00  // .....0.00.......

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 tests/data/acpi/q35/VIOT.viot               | Bin 112 -> 112 bytes
 tests/qtest/bios-tables-test-allowed-diff.h |   1 -
 2 files changed, 1 deletion(-)

diff --git a/tests/data/acpi/q35/VIOT.viot b/tests/data/acpi/q35/VIOT.viot
index 9b179266ccbf84f1c250ee646812d17e27987764..275c78fbe8e93190321d957c91c3f17551f865d4 100644
GIT binary patch
delta 10
RcmXRYnBY1wR(PU=1OOI`1E2r^

delta 10
RcmXRYnBY1wR(PU=1OOI`1E2r^

diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
index 8367ffe1d4..dfb8523c8b 100644
--- a/tests/qtest/bios-tables-test-allowed-diff.h
+++ b/tests/qtest/bios-tables-test-allowed-diff.h
@@ -1,2 +1 @@
 /* List of comma-separated changed AML files to ignore */
-"tests/data/acpi/virt/VIOT",
-- 
2.20.1

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
>