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

[PATCH 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>


Mark Cave-Ayland (6):
  hw/acpi/viot: rename build_pci_range_node() to 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 bus 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 1/6] hw/acpi/viot: rename build_pci_range_node() to 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>
---
 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..2897aa8c88 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 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), pci_host_bridges,
                                    &pci_ranges);
 
     /* ACPI table header */
-- 
2.20.1

[PATCH 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>
---
 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 2897aa8c88..662124812f 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 pci_host_bridges(Object *obj, void *opaque)
 {
@@ -30,27 +55,8 @@ static int 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 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>
---
 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 662124812f..ce3b7b8c75 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 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 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++;
+            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), 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 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>
---
 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 5/6] hw/acpi/viot: sort VIOT ACPI table entries by PCI host bus 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>
---
 hw/acpi/viot.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/hw/acpi/viot.c b/hw/acpi/viot.c
index ce3b7b8c75..f5714b95bd 100644
--- a/hw/acpi/viot.c
+++ b/hw/acpi/viot.c
@@ -64,6 +64,20 @@ static int pci_host_bridges(Object *obj, void *opaque)
     return 0;
 }
 
+static int 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), 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 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>
---
 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 1/6] hw/acpi/viot: rename build_pci_range_node() to pci_host_bridges()

[Ani Sinha]

On Wed, May 18, 2022 at 4:38 PM Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> 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>
> ---
>  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..2897aa8c88 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 pci_host_bridges(Object *obj, void *opaque)

Please rename this as build_pci_host_bridges()

>  {
>      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), pci_host_bridges,
>                                     &pci_ranges);
>
>      /* ACPI table header */
> --
> 2.20.1
>

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

[Ani Sinha]

On Wed, May 18, 2022 at 4:38 PM Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> 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>


> ---
>  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 2897aa8c88..662124812f 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 pci_host_bridges(Object *obj, void *opaque)
>  {
> @@ -30,27 +55,8 @@ static int 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
>

Re: [PATCH 1/6] hw/acpi/viot: rename build_pci_range_node() to pci_host_bridges()

[Mark Cave-Ayland]

On 18/05/2022 12:36, Ani Sinha wrote:

> On Wed, May 18, 2022 at 4:38 PM Mark Cave-Ayland
> <mark.cave-ayland@ilande.co.uk> wrote:
>>
>> 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>
>> ---
>>   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..2897aa8c88 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 pci_host_bridges(Object *obj, void *opaque)
> 
> Please rename this as build_pci_host_bridges()

I'm not sure this makes sense? The naming here is deliberate since the whole aim of 
patches 1-3 is to remove the ACPI table build code out of this function so that its 
only purpose is to enumerate the PCI host bridges. This is similar to the approach 
already taken in hw/arm/virt-acpi-build.c in build_iort().

>>   {
>>       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), pci_host_bridges,
>>                                      &pci_ranges);
>>
>>       /* ACPI table header */
>> --
>> 2.20.1


ATB,

Mark.

Re: [PATCH 1/6] hw/acpi/viot: rename build_pci_range_node() to pci_host_bridges()

[Ani Sinha]

On Wed, May 18, 2022 at 5:57 PM Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> On 18/05/2022 12:36, Ani Sinha wrote:
>
> > On Wed, May 18, 2022 at 4:38 PM Mark Cave-Ayland
> > <mark.cave-ayland@ilande.co.uk> wrote:
> >>
> >> 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>
> >> ---
> >>   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..2897aa8c88 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 pci_host_bridges(Object *obj, void *opaque)
> >
> > Please rename this as build_pci_host_bridges()
>
> I'm not sure this makes sense?

How about enumerate_pci_host_bridges() then?

 The naming here is deliberate since the whole aim of
> patches 1-3 is to remove the ACPI table build code out of this function so that its
> only purpose is to enumerate the PCI host bridges. This is similar to the approach
> already taken in hw/arm/virt-acpi-build.c in build_iort().
>
> >>   {
> >>       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), pci_host_bridges,
> >>                                      &pci_ranges);
> >>
> >>       /* ACPI table header */
> >> --
> >> 2.20.1
>
>
> ATB,
>
> Mark.

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

[Ani Sinha]

On Wed, May 18, 2022 at 4:39 PM Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Acked-by: Ani Sinha <ani@anisinha.ca>

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

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

[Ani Sinha]

On Wed, May 18, 2022 at 4:39 PM Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> 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>
other than the nit below,
Reviewed-by: Ani Sinha <ani@anisinha.ca>

> ---
>  hw/acpi/viot.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
>
> diff --git a/hw/acpi/viot.c b/hw/acpi/viot.c
> index ce3b7b8c75..f5714b95bd 100644
> --- a/hw/acpi/viot.c
> +++ b/hw/acpi/viot.c
> @@ -64,6 +64,20 @@ static int pci_host_bridges(Object *obj, void *opaque)
>      return 0;
>  }
>
> +static int pci_host_range_compare(gconstpointer a, gconstpointer b)

nit: shouldn't this have a gint return type since we use gconstpointer
as arguments anyway?
https://docs.gtk.org/glib/callback.CompareFunc.html

> +{
> +    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), 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
>

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

[Ani Sinha]

On Wed, May 18, 2022 at 4:39 PM Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> 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>

> ---
>  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 662124812f..ce3b7b8c75 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 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 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++;
> +            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), 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
>

Re: [PATCH 1/6] hw/acpi/viot: rename build_pci_range_node() to pci_host_bridges()

[Mark Cave-Ayland]

On 19/05/2022 08:45, Ani Sinha wrote:

> On Wed, May 18, 2022 at 5:57 PM Mark Cave-Ayland
> <mark.cave-ayland@ilande.co.uk> wrote:
>>
>> On 18/05/2022 12:36, Ani Sinha wrote:
>>
>>> On Wed, May 18, 2022 at 4:38 PM Mark Cave-Ayland
>>> <mark.cave-ayland@ilande.co.uk> wrote:
>>>>
>>>> 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>
>>>> ---
>>>>    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..2897aa8c88 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 pci_host_bridges(Object *obj, void *opaque)
>>>
>>> Please rename this as build_pci_host_bridges()
>>
>> I'm not sure this makes sense?
> 
> How about enumerate_pci_host_bridges() then?

Sure, that works for me. I'll update this for v2.

>   The naming here is deliberate since the whole aim of
>> patches 1-3 is to remove the ACPI table build code out of this function so that its
>> only purpose is to enumerate the PCI host bridges. This is similar to the approach
>> already taken in hw/arm/virt-acpi-build.c in build_iort().
>>
>>>>    {
>>>>        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), pci_host_bridges,
>>>>                                       &pci_ranges);
>>>>
>>>>        /* ACPI table header */
>>>> --
>>>> 2.20.1


ATB,

Mark.

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

[Mark Cave-Ayland]

On 19/05/2022 08:50, Ani Sinha wrote:

> On Wed, May 18, 2022 at 4:39 PM Mark Cave-Ayland
> <mark.cave-ayland@ilande.co.uk> wrote:
>>
>> 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>
> other than the nit below,
> Reviewed-by: Ani Sinha <ani@anisinha.ca>
> 
>> ---
>>   hw/acpi/viot.c | 17 +++++++++++++++++
>>   1 file changed, 17 insertions(+)
>>
>> diff --git a/hw/acpi/viot.c b/hw/acpi/viot.c
>> index ce3b7b8c75..f5714b95bd 100644
>> --- a/hw/acpi/viot.c
>> +++ b/hw/acpi/viot.c
>> @@ -64,6 +64,20 @@ static int pci_host_bridges(Object *obj, void *opaque)
>>       return 0;
>>   }
>>
>> +static int pci_host_range_compare(gconstpointer a, gconstpointer b)
> 
> nit: shouldn't this have a gint return type since we use gconstpointer
> as arguments anyway?
> https://docs.gtk.org/glib/callback.CompareFunc.html

I guess so - int/gint seem to be fairly interchangeable, and there are examples of 
both in the QEMU codebase. The only reason it appears in the patch as int is because 
that was how it was in the reference code I used from iort_idmap_compare().

I'll change this to gint for v2.

>> +{
>> +    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), 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


ATB,

Mark.

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

[Philippe Mathieu-Daudé via]

On 18/5/22 13:08, Mark Cave-Ayland wrote:
> 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>
> ---
>   hw/acpi/viot.c | 42 ++++++++++++++++++++++++------------------
>   1 file changed, 24 insertions(+), 18 deletions(-)

> @@ -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 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 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++;
> +            struct viot_pci_host_range pci_host_range = {

Nitpicking, const?

> +                .min_bus = min_bus,
> +                .max_bus = max_bus,
> +            };
> +            g_array_append_val(pci_host_ranges, pci_host_range);
>           }
>       }

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

[Philippe Mathieu-Daudé via]

On 18/5/22 13:08, Mark Cave-Ayland wrote:

> Mark Cave-Ayland (6):
>    hw/acpi/viot: rename build_pci_range_node() to 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 bus 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(-)
> 

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

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

[Mark Cave-Ayland]

On 22/05/2022 23:11, Philippe Mathieu-Daudé via wrote:

> On 18/5/22 13:08, Mark Cave-Ayland wrote:
>> 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>
>> ---
>>   hw/acpi/viot.c | 42 ++++++++++++++++++++++++------------------
>>   1 file changed, 24 insertions(+), 18 deletions(-)
> 
>> @@ -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 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 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++;
>> +            struct viot_pci_host_range pci_host_range = {
> 
> Nitpicking, const?
> 
>> +                .min_bus = min_bus,
>> +                .max_bus = max_bus,
>> +            };
>> +            g_array_append_val(pci_host_ranges, pci_host_range);
>>           }
>>       }

Yes, that works here. I'll wait a day or so and see if anyone else has any further 
comments before posting a v3.


ATB,

Mark.