[PATCH-for-10.0 0/5] hw/arm/virt-acpi: Do not advertise disabled GIC ITS in MADT table

[PATCH-for-10.0 0/5] hw/arm/virt-acpi: Do not advertise disabled GIC ITS in MADT table

[Philippe Mathieu-Daudé]

GIC ITS can be disabled using '-M its=off'.
When that occurs, it shouldn't be advertised in ACPI tables.

Philippe Mathieu-Daudé (5):
  qtest/bios-tables-test: Add test for -M virt,its=off
  qtest/bios-tables-test: Whitelist aarch64/virt/APIC.its_off blob
  hw/arm/virt-acpi: Factor its_enabled() helper out
  hw/arm/virt-acpi: Do not advertise disabled GIC ITS
  qtest/bios-tables-test: Update aarch64/virt/APIC.its_off blob

 hw/arm/virt-acpi-build.c                  |  12 +++++++++---
 tests/qtest/bios-tables-test.c            |  22 ++++++++++++++++++++++
 tests/data/acpi/aarch64/virt/APIC.its_off | Bin 0 -> 164 bytes
 tests/data/acpi/aarch64/virt/FACP.its_off | Bin 0 -> 276 bytes
 tests/data/acpi/aarch64/virt/IORT.its_off | Bin 0 -> 236 bytes
 5 files changed, 31 insertions(+), 3 deletions(-)
 create mode 100644 tests/data/acpi/aarch64/virt/APIC.its_off
 create mode 100644 tests/data/acpi/aarch64/virt/FACP.its_off
 create mode 100644 tests/data/acpi/aarch64/virt/IORT.its_off

-- 
2.47.1

[PATCH-for-10.0 1/5] qtest/bios-tables-test: Add test for -M virt, its=off

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 tests/qtest/bios-tables-test.c            |  22 ++++++++++++++++++++++
 tests/data/acpi/aarch64/virt/APIC.its_off | Bin 0 -> 184 bytes
 tests/data/acpi/aarch64/virt/FACP.its_off | Bin 0 -> 276 bytes
 tests/data/acpi/aarch64/virt/IORT.its_off | Bin 0 -> 236 bytes
 4 files changed, 22 insertions(+)
 create mode 100644 tests/data/acpi/aarch64/virt/APIC.its_off
 create mode 100644 tests/data/acpi/aarch64/virt/FACP.its_off
 create mode 100644 tests/data/acpi/aarch64/virt/IORT.its_off

diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
index 0a333ec4353..55366bf4956 100644
--- a/tests/qtest/bios-tables-test.c
+++ b/tests/qtest/bios-tables-test.c
@@ -2146,6 +2146,26 @@ static void test_acpi_aarch64_virt_tcg_topology(void)
     free_test_data(&data);
 }
 
+static void test_acpi_aarch64_virt_tcg_its_off(void)
+{
+    test_data data = {
+        .machine = "virt",
+        .arch = "aarch64",
+        .variant = ".its_off",
+        .tcg_only = true,
+        .uefi_fl1 = "pc-bios/edk2-aarch64-code.fd",
+        .uefi_fl2 = "pc-bios/edk2-arm-vars.fd",
+        .cd = "tests/data/uefi-boot-images/bios-tables-test.aarch64.iso.qcow2",
+        .ram_start = 0x40000000ULL,
+        .scan_len = 128ULL * 1024 * 1024,
+    };
+
+    test_acpi_one("-cpu cortex-a57 "
+                  "-M virtualization=on,secure=off "
+                  "-M gic-version=max,its=off,iommu=smmuv3", &data);
+    free_test_data(&data);
+}
+
 static void test_acpi_q35_viot(void)
 {
     test_data data = {
@@ -2577,6 +2597,8 @@ int main(int argc, char *argv[])
                            test_acpi_aarch64_virt_tcg_acpi_hmat);
             qtest_add_func("acpi/virt/topology",
                            test_acpi_aarch64_virt_tcg_topology);
+            qtest_add_func("acpi/virt/its_off",
+                           test_acpi_aarch64_virt_tcg_its_off);
             qtest_add_func("acpi/virt/numamem",
                            test_acpi_aarch64_virt_tcg_numamem);
             qtest_add_func("acpi/virt/memhp", test_acpi_aarch64_virt_tcg_memhp);
diff --git a/tests/data/acpi/aarch64/virt/APIC.its_off b/tests/data/acpi/aarch64/virt/APIC.its_off
new file mode 100644
index 0000000000000000000000000000000000000000..c37d05d6e05805304f10afe73eb7cb9100fcccfa
GIT binary patch
literal 184
zcmZ<^@O0k6z`($=+{xeBBUr&HBEVSz2pEB4AU24G0Uik$i-7~iVgWL^17JJ`2AFzr
bgb+@aBn}xq0gwb2)Q)cq{30-g9B_L93G4|0

literal 0
HcmV?d00001

diff --git a/tests/data/acpi/aarch64/virt/FACP.its_off b/tests/data/acpi/aarch64/virt/FACP.its_off
new file mode 100644
index 0000000000000000000000000000000000000000..606dac3fe4b55c31fd68b25d3a4127eeef227434
GIT binary patch
literal 276
zcmZ>BbPf<<WME(uaq@Te2v%^42yj*a0-z8Bhz+8t3j|P&V`N}P6&N^PpsQ~v$aVnZ
CVg~^L

literal 0
HcmV?d00001

diff --git a/tests/data/acpi/aarch64/virt/IORT.its_off b/tests/data/acpi/aarch64/virt/IORT.its_off
new file mode 100644
index 0000000000000000000000000000000000000000..0fceb820d509e852ca0849baf568a8e93e426738
GIT binary patch
literal 236
zcmebD4+?q1z`(#9?&R<65v<@85#X!<1dKp25F11@1F-=RgMkDCNC*yK9F_<M77!bR
zUBI%eoFED&4;F$FSwK1)h;xBB2Py`m{{M%tVD>TjFfcO#g+N#Zh@s|zoCF3AP#UU@
R!2`+%Dg6Hr$N|zYvjDIZ5CH%H

literal 0
HcmV?d00001

-- 
2.47.1

[PATCH-for-10.0 2/5] qtest/bios-tables-test: Whitelist aarch64/virt/APIC.its_off blob

[Philippe Mathieu-Daudé]

Prepare for ACPI table change in aarch64/virt/APIC.its_off.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.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 dfb8523c8bf..bfc4d601243 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/aarch64/virt/APIC.its_off",
-- 
2.47.1

[PATCH-for-10.0 3/5] hw/arm/virt-acpi: Factor its_enabled() helper out

[Philippe Mathieu-Daudé]

GIC ITS is checked for the MADT and IORT tables.
Factor the checks out to the its_enabled() helper.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/arm/virt-acpi-build.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 3ac8f8e1786..fdc08b40883 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -208,6 +208,13 @@ static void acpi_dsdt_add_tpm(Aml *scope, VirtMachineState *vms)
 #define ROOT_COMPLEX_ENTRY_SIZE 36
 #define IORT_NODE_OFFSET 48
 
+static bool its_enabled(VirtMachineState *vms)
+{
+    VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
+
+    return its_class_name() && !vmc->no_its;
+}
+
 /*
  * Append an ID mapping entry as described by "Table 4 ID mapping format" in
  * "IO Remapping Table System Software on ARM Platforms", Chapter 3.
@@ -670,7 +677,6 @@ static void
 build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
 {
     int i;
-    VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
     const MemMapEntry *memmap = vms->memmap;
     AcpiTable table = { .sig = "APIC", .rev = 4, .oem_id = vms->oem_id,
                         .oem_table_id = vms->oem_table_id };
@@ -741,7 +747,7 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
                                           memmap[VIRT_HIGH_GIC_REDIST2].size);
         }
 
-        if (its_class_name() && !vmc->no_its) {
+        if (its_enabled(vms)) {
             /*
              * ACPI spec, Revision 6.0 Errata A
              * (original 6.0 definition has invalid Length)
@@ -973,7 +979,7 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
                           vms->oem_table_id);
     }
 
-    if (its_class_name() && !vmc->no_its) {
+    if (its_enabled(vms)) {
         acpi_add_table(table_offsets, tables_blob);
         build_iort(tables_blob, tables->linker, vms);
     }
-- 
2.47.1

[PATCH-for-10.0 4/5] hw/arm/virt-acpi: Do not advertise disabled GIC ITS

[Philippe Mathieu-Daudé]

GIC ITS can be disabled at runtime using '-M its=off',
which sets VirtMachineState::its = false. Check this
field to avoid advertising the ITS in the MADT table.

Reported-by: Udo Steinberg <udo@hypervisor.org>
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2886
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/arm/virt-acpi-build.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index fdc08b40883..b26f0ac8585 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -212,7 +212,7 @@ static bool its_enabled(VirtMachineState *vms)
 {
     VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
 
-    return its_class_name() && !vmc->no_its;
+    return its_class_name() && !vmc->no_its && vms->its;
 }
 
 /*
-- 
2.47.1

[PATCH-for-10.0 5/5] qtest/bios-tables-test: Update aarch64/virt/APIC.its_off blob

[Philippe Mathieu-Daudé]

Changes in the tables:

  @@ -1,32 +1,32 @@
   /*
    * Intel ACPI Component Architecture
    * AML/ASL+ Disassembler version 20240927 (64-bit version)
    * Copyright (c) 2000 - 2023 Intel Corporation
    *
    * Disassembly of tests/data/acpi/aarch64/virt/APIC.its_off
    *
    * ACPI Data Table [APIC]
    *
    * Format: [HexOffset DecimalOffset ByteLength]  FieldName : FieldValue (in hex)
    */

   [000h 0000 004h]                   Signature : "APIC"    [Multiple APIC Description Table (MADT)]
  -[004h 0004 004h]                Table Length : 000000B8
  +[004h 0004 004h]                Table Length : 000000A4
   [008h 0008 001h]                    Revision : 04
  -[009h 0009 001h]                    Checksum : A7
  +[009h 0009 001h]                    Checksum : EE
   [00Ah 0010 006h]                      Oem ID : "BOCHS "
   [010h 0016 008h]                Oem Table ID : "BXPC    "
   [018h 0024 004h]                Oem Revision : 00000001
   [01Ch 0028 004h]             Asl Compiler ID : "BXPC"
   [020h 0032 004h]       Asl Compiler Revision : 00000001

   [024h 0036 004h]          Local Apic Address : 00000000
   [028h 0040 004h]       Flags (decoded below) : 00000000
                            PC-AT Compatibility : 0

   [02Ch 0044 001h]               Subtable Type : 0C [Generic Interrupt Distributor]
   [02Dh 0045 001h]                      Length : 18
   [02Eh 0046 002h]                    Reserved : 0000
   [030h 0048 004h]       Local GIC Hardware ID : 00000000
   [034h 0052 008h]                Base Address : 0000000008000000
   [03Ch 0060 004h]              Interrupt Base : 00000000
  @@ -49,37 +49,29 @@
   [06Ch 0108 008h]    Virtual GIC Base Address : 0000000000000000
   [074h 0116 008h] Hypervisor GIC Base Address : 0000000000000000
   [07Ch 0124 004h]       Virtual GIC Interrupt : 00000019
   [080h 0128 008h]  Redistributor Base Address : 0000000000000000
   [088h 0136 008h]                   ARM MPIDR : 0000000000000000
   [090h 0144 001h]            Efficiency Class : 00
   [091h 0145 001h]                    Reserved : 00
   [092h 0146 002h]      SPE Overflow Interrupt : 0000
   [094h 0148 002h]              TRBE Interrupt : 100E

   [094h 0148 001h]               Subtable Type : 0E [Generic Interrupt Redistributor]
   [095h 0149 001h]                      Length : 10
   [097h 0151 002h]                    Reserved : 0000
   [098h 0152 008h]                Base Address : 00000000080A0000
   [0A0h 0160 004h]                      Length : 00F60000

  -[0A4h 0164 001h]               Subtable Type : 0F [Generic Interrupt Translator]
  -[0A5h 0165 001h]                      Length : 14
  -[0A7h 0167 002h]                    Reserved : 0000
  -[0A8h 0168 004h]              Translation ID : 00000000
  -[0ACh 0172 008h]                Base Address : 0000000008080000
  -[0B4h 0180 004h]                    Reserved : 00000000
  +Raw Table Data: Length 164 (0xA4)

  -Raw Table Data: Length 184 (0xB8)
  -
  -    0000: 41 50 49 43 B8 00 00 00 04 A7 42 4F 43 48 53 20  // APIC......BOCHS
  +    0000: 41 50 49 43 A4 00 00 00 04 EE 42 4F 43 48 53 20  // APIC......BOCHS
       0010: 42 58 50 43 20 20 20 20 01 00 00 00 42 58 50 43  // BXPC    ....BXPC
       0020: 01 00 00 00 00 00 00 00 00 00 00 00 0C 18 00 00  // ................
       0030: 00 00 00 00 00 00 00 08 00 00 00 00 00 00 00 00  // ................
       0040: 04 00 00 00 0B 50 00 00 00 00 00 00 00 00 00 00  // .....P..........
       0050: 01 00 00 00 00 00 00 00 17 00 00 00 00 00 00 00  // ................
       0060: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  // ................
       0070: 00 00 00 00 00 00 00 00 00 00 00 00 19 00 00 00  // ................
       0080: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  // ................
       0090: 00 00 00 00 0E 10 00 00 00 00 0A 08 00 00 00 00  // ................
  -    00A0: 00 00 F6 00 0F 14 00 00 00 00 00 00 00 00 08 08  // ................
  -    00B0: 00 00 00 00 00 00 00 00                          // ........
  +    00A0: 00 00 F6 00                                      // ....

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 tests/qtest/bios-tables-test-allowed-diff.h |   1 -
 tests/data/acpi/aarch64/virt/APIC.its_off   | Bin 184 -> 164 bytes
 2 files changed, 1 deletion(-)

diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
index bfc4d601243..dfb8523c8bf 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/aarch64/virt/APIC.its_off",
diff --git a/tests/data/acpi/aarch64/virt/APIC.its_off b/tests/data/acpi/aarch64/virt/APIC.its_off
index c37d05d6e05805304f10afe73eb7cb9100fcccfa..f24ac8fbff5261a52434abcfcff96dbdc7709de4 100644
GIT binary patch
delta 18
ZcmdnNxP+0*F~HM#2?GNI%e#qOvj8xy1yKM1

delta 39
jcmZ3&xPy_)F~HM#2Ll5G%kqg_vqbnsfJ`vp;DE6Jpmzmj

-- 
2.47.1

Re: [PATCH-for-10.0 1/5] qtest/bios-tables-test: Add test for -M virt,its=off

[Gustavo Romero]

Hi Phil,

On 3/31/25 19:12, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Please, put commit message (body) into the commits.

For example, the commit message here could quickly explain that the FACP table
changed because virtualization=on (due to PSCI conduit). I'm assuming
virtualization is set to on because gic-version=max and so GICv4 is selected for
testing. It also could be that  we want to exercise its=off when Arm Virtualization
Extensions are enabled, which is the common use case (I understand that ITS
can be used also with virtualization=off).

Finally, the commit message could mention at the end which struct
vanishes in APIC table and why IO remapping table is affected by
ITS on/off.

A good commit message always help in code spelunking :)


> ---
>   tests/qtest/bios-tables-test.c            |  22 ++++++++++++++++++++++
>   tests/data/acpi/aarch64/virt/APIC.its_off | Bin 0 -> 184 bytes
>   tests/data/acpi/aarch64/virt/FACP.its_off | Bin 0 -> 276 bytes
>   tests/data/acpi/aarch64/virt/IORT.its_off | Bin 0 -> 236 bytes
>   4 files changed, 22 insertions(+)
>   create mode 100644 tests/data/acpi/aarch64/virt/APIC.its_off
>   create mode 100644 tests/data/acpi/aarch64/virt/FACP.its_off
>   create mode 100644 tests/data/acpi/aarch64/virt/IORT.its_off
> 
> diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
> index 0a333ec4353..55366bf4956 100644
> --- a/tests/qtest/bios-tables-test.c
> +++ b/tests/qtest/bios-tables-test.c
> @@ -2146,6 +2146,26 @@ static void test_acpi_aarch64_virt_tcg_topology(void)
>       free_test_data(&data);
>   }
>   
> +static void test_acpi_aarch64_virt_tcg_its_off(void)
> +{
> +    test_data data = {
> +        .machine = "virt",
> +        .arch = "aarch64",
> +        .variant = ".its_off",
> +        .tcg_only = true,
> +        .uefi_fl1 = "pc-bios/edk2-aarch64-code.fd",
> +        .uefi_fl2 = "pc-bios/edk2-arm-vars.fd",
> +        .cd = "tests/data/uefi-boot-images/bios-tables-test.aarch64.iso.qcow2",
> +        .ram_start = 0x40000000ULL,
> +        .scan_len = 128ULL * 1024 * 1024,
> +    };
> +
> +    test_acpi_one("-cpu cortex-a57 "
> +                  "-M virtualization=on,secure=off "
> +                  "-M gic-version=max,its=off,iommu=smmuv3", &data);
> +    free_test_data(&data);
> +}
> +
>   static void test_acpi_q35_viot(void)
>   {
>       test_data data = {
> @@ -2577,6 +2597,8 @@ int main(int argc, char *argv[])
>                              test_acpi_aarch64_virt_tcg_acpi_hmat);
>               qtest_add_func("acpi/virt/topology",
>                              test_acpi_aarch64_virt_tcg_topology);
> +            qtest_add_func("acpi/virt/its_off",
> +                           test_acpi_aarch64_virt_tcg_its_off);
>               qtest_add_func("acpi/virt/numamem",
>                              test_acpi_aarch64_virt_tcg_numamem);
>               qtest_add_func("acpi/virt/memhp", test_acpi_aarch64_virt_tcg_memhp);
> diff --git a/tests/data/acpi/aarch64/virt/APIC.its_off b/tests/data/acpi/aarch64/virt/APIC.its_off
> new file mode 100644
> index 0000000000000000000000000000000000000000..c37d05d6e05805304f10afe73eb7cb9100fcccfa
> GIT binary patch
> literal 184
> zcmZ<^@O0k6z`($=+{xeBBUr&HBEVSz2pEB4AU24G0Uik$i-7~iVgWL^17JJ`2AFzr
> bgb+@aBn}xq0gwb2)Q)cq{30-g9B_L93G4|0
> 
> literal 0
> HcmV?d00001
> 
> diff --git a/tests/data/acpi/aarch64/virt/FACP.its_off b/tests/data/acpi/aarch64/virt/FACP.its_off
> new file mode 100644
> index 0000000000000000000000000000000000000000..606dac3fe4b55c31fd68b25d3a4127eeef227434
> GIT binary patch
> literal 276
> zcmZ>BbPf<<WME(uaq@Te2v%^42yj*a0-z8Bhz+8t3j|P&V`N}P6&N^PpsQ~v$aVnZ
> CVg~^L
> 
> literal 0
> HcmV?d00001
> 
> diff --git a/tests/data/acpi/aarch64/virt/IORT.its_off b/tests/data/acpi/aarch64/virt/IORT.its_off
> new file mode 100644
> index 0000000000000000000000000000000000000000..0fceb820d509e852ca0849baf568a8e93e426738
> GIT binary patch
> literal 236
> zcmebD4+?q1z`(#9?&R<65v<@85#X!<1dKp25F11@1F-=RgMkDCNC*yK9F_<M77!bR
> zUBI%eoFED&4;F$FSwK1)h;xBB2Py`m{{M%tVD>TjFfcO#g+N#Zh@s|zoCF3AP#UU@
> R!2`+%Dg6Hr$N|zYvjDIZ5CH%H
> 
> literal 0
> HcmV?d00001
> 

I think the prescription for the acrobatics to update the ACPI expected
tables says the blobs here should be empty (blob files are added empty)
and at the same time they are listed in tests/qtest/bios-tables-test-allowed-diff.h:

  * 1. add empty files for new tables, if any, under tests/data/acpi
  * 2. list any changed files in tests/qtest/bios-tables-test-allowed-diff.h
  * 3. commit the above *before* making changes that affect the tables

(from tests/qtest/bios-tables-test.c header)

If that's correct, this patch should be merged with the following one (2/5) and
IORT.its_off and FACP.its_off should also be listed in
tests/qtest/bios-tables-test-allowed-diff.h so the empty blobs won't trigger
a test failure.

Then patch 5/5 should add the expected/updated blobs and drop the *.its_off from
bios-tables-test-allowed-diff.h. Patches 3/5 and 4/5 are sandwiched
between (1/5 + 2/5) and (5/5).

At least that's what I get from:

  * The resulting patchset/pull request then looks like this:
  * - patch 1: list changed files in tests/qtest/bios-tables-test-allowed-diff.h.
  * - patches 2 - n: real changes, may contain multiple patches.
  * - patch n + 1: update golden master binaries and empty tests/qtest/bios-tables-test-allowed-diff.h

Otherwise the change looks good.


Cheers,
Gustavo

Re: [PATCH-for-10.0 2/5] qtest/bios-tables-test: Whitelist aarch64/virt/APIC.its_off blob

[Gustavo Romero]

Hi Phil,

On 3/31/25 19:12, Philippe Mathieu-Daudé wrote:
> Prepare for ACPI table change in aarch64/virt/APIC.its_off.

The comment could be smth like:

Ignore APIC.its_off expected table (blob) for now until
we update it later, after fixing the code that generates
this table correctly.

?


> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.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 dfb8523c8bf..bfc4d601243 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/aarch64/virt/APIC.its_off",

I think this patch should be merged into 1/2, accordingly to my
comment in 1/5. FACP and IORT .its_off files should be added to the
list as well.

Btw, IMHO the name of this header is a tad misleading, because actually
"allowed-diff" means that "we allow the machine's table to be different
from the tables listed in this header", so it doesn't look like an
allowlist (whitelist), it works more like an ignore list?


Cheers,
Gustavo

Re: [PATCH-for-10.0 3/5] hw/arm/virt-acpi: Factor its_enabled() helper out

[Gustavo Romero]

Hi Phil,

On 3/31/25 19:12, Philippe Mathieu-Daudé wrote:
> GIC ITS is checked for the MADT and IORT tables.
> Factor the checks out to the its_enabled() helper.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/arm/virt-acpi-build.c | 12 +++++++++---
>   1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 3ac8f8e1786..fdc08b40883 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -208,6 +208,13 @@ static void acpi_dsdt_add_tpm(Aml *scope, VirtMachineState *vms)
>   #define ROOT_COMPLEX_ENTRY_SIZE 36
>   #define IORT_NODE_OFFSET 48
>   
> +static bool its_enabled(VirtMachineState *vms)
> +{
> +    VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
> +
> +    return its_class_name() && !vmc->no_its;
> +}
> +

Isn't its_class_name() always "true"?


Cheers,
Gustavo

>   /*
>    * Append an ID mapping entry as described by "Table 4 ID mapping format" in
>    * "IO Remapping Table System Software on ARM Platforms", Chapter 3.
> @@ -670,7 +677,6 @@ static void
>   build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>   {
>       int i;
> -    VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
>       const MemMapEntry *memmap = vms->memmap;
>       AcpiTable table = { .sig = "APIC", .rev = 4, .oem_id = vms->oem_id,
>                           .oem_table_id = vms->oem_table_id };
> @@ -741,7 +747,7 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>                                             memmap[VIRT_HIGH_GIC_REDIST2].size);
>           }
>   
> -        if (its_class_name() && !vmc->no_its) {
> +        if (its_enabled(vms)) {
>               /*
>                * ACPI spec, Revision 6.0 Errata A
>                * (original 6.0 definition has invalid Length)
> @@ -973,7 +979,7 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
>                             vms->oem_table_id);
>       }
>   
> -    if (its_class_name() && !vmc->no_its) {
> +    if (its_enabled(vms)) {
>           acpi_add_table(table_offsets, tables_blob);
>           build_iort(tables_blob, tables->linker, vms);
>       }

Re: [PATCH-for-10.0 4/5] hw/arm/virt-acpi: Do not advertise disabled GIC ITS

[Gustavo Romero]

Hi Phil,

On 3/31/25 19:12, Philippe Mathieu-Daudé wrote:
> GIC ITS can be disabled at runtime using '-M its=off',
> which sets VirtMachineState::its = false. Check this
> field to avoid advertising the ITS in the MADT table.
> 
> Reported-by: Udo Steinberg <udo@hypervisor.org>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2886
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/arm/virt-acpi-build.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index fdc08b40883..b26f0ac8585 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -212,7 +212,7 @@ static bool its_enabled(VirtMachineState *vms)
>   {
>       VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
>   
> -    return its_class_name() && !vmc->no_its;
> +    return its_class_name() && !vmc->no_its && vms->its;
>   }

Aside its_class_name() apparently being tautologic (please double check it):

Reviewed-by: Gustavo Romero <gustavo.romero@linaro.org>


Cheers,
Gustavo

Re: [PATCH-for-10.0 5/5] qtest/bios-tables-test: Update aarch64/virt/APIC.its_off blob

[Gustavo Romero]

Hi Phil,

On 3/31/25 19:12, Philippe Mathieu-Daudé wrote:
> Changes in the tables:
> 
>    @@ -1,32 +1,32 @@
>     /*
>      * Intel ACPI Component Architecture
>      * AML/ASL+ Disassembler version 20240927 (64-bit version)
>      * Copyright (c) 2000 - 2023 Intel Corporation
>      *
>      * Disassembly of tests/data/acpi/aarch64/virt/APIC.its_off
>      *
>      * ACPI Data Table [APIC]
>      *
>      * Format: [HexOffset DecimalOffset ByteLength]  FieldName : FieldValue (in hex)
>      */
> 
>     [000h 0000 004h]                   Signature : "APIC"    [Multiple APIC Description Table (MADT)]
>    -[004h 0004 004h]                Table Length : 000000B8
>    +[004h 0004 004h]                Table Length : 000000A4
>     [008h 0008 001h]                    Revision : 04
>    -[009h 0009 001h]                    Checksum : A7
>    +[009h 0009 001h]                    Checksum : EE
>     [00Ah 0010 006h]                      Oem ID : "BOCHS "
>     [010h 0016 008h]                Oem Table ID : "BXPC    "
>     [018h 0024 004h]                Oem Revision : 00000001
>     [01Ch 0028 004h]             Asl Compiler ID : "BXPC"
>     [020h 0032 004h]       Asl Compiler Revision : 00000001
> 
>     [024h 0036 004h]          Local Apic Address : 00000000
>     [028h 0040 004h]       Flags (decoded below) : 00000000
>                              PC-AT Compatibility : 0
> 
>     [02Ch 0044 001h]               Subtable Type : 0C [Generic Interrupt Distributor]
>     [02Dh 0045 001h]                      Length : 18
>     [02Eh 0046 002h]                    Reserved : 0000
>     [030h 0048 004h]       Local GIC Hardware ID : 00000000
>     [034h 0052 008h]                Base Address : 0000000008000000
>     [03Ch 0060 004h]              Interrupt Base : 00000000
>    @@ -49,37 +49,29 @@
>     [06Ch 0108 008h]    Virtual GIC Base Address : 0000000000000000
>     [074h 0116 008h] Hypervisor GIC Base Address : 0000000000000000
>     [07Ch 0124 004h]       Virtual GIC Interrupt : 00000019
>     [080h 0128 008h]  Redistributor Base Address : 0000000000000000
>     [088h 0136 008h]                   ARM MPIDR : 0000000000000000
>     [090h 0144 001h]            Efficiency Class : 00
>     [091h 0145 001h]                    Reserved : 00
>     [092h 0146 002h]      SPE Overflow Interrupt : 0000
>     [094h 0148 002h]              TRBE Interrupt : 100E
> 
>     [094h 0148 001h]               Subtable Type : 0E [Generic Interrupt Redistributor]
>     [095h 0149 001h]                      Length : 10
>     [097h 0151 002h]                    Reserved : 0000
>     [098h 0152 008h]                Base Address : 00000000080A0000
>     [0A0h 0160 004h]                      Length : 00F60000
> 
>    -[0A4h 0164 001h]               Subtable Type : 0F [Generic Interrupt Translator]
>    -[0A5h 0165 001h]                      Length : 14
>    -[0A7h 0167 002h]                    Reserved : 0000
>    -[0A8h 0168 004h]              Translation ID : 00000000
>    -[0ACh 0172 008h]                Base Address : 0000000008080000
>    -[0B4h 0180 004h]                    Reserved : 00000000
>    +Raw Table Data: Length 164 (0xA4)
> 
>    -Raw Table Data: Length 184 (0xB8)
>    -
>    -    0000: 41 50 49 43 B8 00 00 00 04 A7 42 4F 43 48 53 20  // APIC......BOCHS
>    +    0000: 41 50 49 43 A4 00 00 00 04 EE 42 4F 43 48 53 20  // APIC......BOCHS
>         0010: 42 58 50 43 20 20 20 20 01 00 00 00 42 58 50 43  // BXPC    ....BXPC
>         0020: 01 00 00 00 00 00 00 00 00 00 00 00 0C 18 00 00  // ................
>         0030: 00 00 00 00 00 00 00 08 00 00 00 00 00 00 00 00  // ................
>         0040: 04 00 00 00 0B 50 00 00 00 00 00 00 00 00 00 00  // .....P..........
>         0050: 01 00 00 00 00 00 00 00 17 00 00 00 00 00 00 00  // ................
>         0060: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  // ................
>         0070: 00 00 00 00 00 00 00 00 00 00 00 00 19 00 00 00  // ................
>         0080: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  // ................
>         0090: 00 00 00 00 0E 10 00 00 00 00 0A 08 00 00 00 00  // ................
>    -    00A0: 00 00 F6 00 0F 14 00 00 00 00 00 00 00 00 08 08  // ................
>    -    00B0: 00 00 00 00 00 00 00 00                          // ........
>    +    00A0: 00 00 F6 00                                      // ....
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   tests/qtest/bios-tables-test-allowed-diff.h |   1 -
>   tests/data/acpi/aarch64/virt/APIC.its_off   | Bin 184 -> 164 bytes
>   2 files changed, 1 deletion(-)
> 
> diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
> index bfc4d601243..dfb8523c8bf 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/aarch64/virt/APIC.its_off",
> diff --git a/tests/data/acpi/aarch64/virt/APIC.its_off b/tests/data/acpi/aarch64/virt/APIC.its_off
> index c37d05d6e05805304f10afe73eb7cb9100fcccfa..f24ac8fbff5261a52434abcfcff96dbdc7709de4 100644
> GIT binary patch
> delta 18
> ZcmdnNxP+0*F~HM#2?GNI%e#qOvj8xy1yKM1
> 
> delta 39
> jcmZ3&xPy_)F~HM#2Ll5G%kqg_vqbnsfJ`vp;DE6Jpmzmj

If the change affects other tables besides APIC (IORT and FACP in this series),
then I think that's the moment to update all the correct blobs (final versions)
and drop the list of blobs in bios-tables-test-allowed-diff.h. I also think it's
better to list all the table diffs in the commit message, not only the diff for
the APIC table.


Cheers,
Gustavo

Re: [PATCH-for-10.0 3/5] hw/arm/virt-acpi: Factor its_enabled() helper out

[Philippe Mathieu-Daudé]

On 2/4/25 08:43, Gustavo Romero wrote:
> Hi Phil,
> 
> On 3/31/25 19:12, Philippe Mathieu-Daudé wrote:
>> GIC ITS is checked for the MADT and IORT tables.
>> Factor the checks out to the its_enabled() helper.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   hw/arm/virt-acpi-build.c | 12 +++++++++---
>>   1 file changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>> index 3ac8f8e1786..fdc08b40883 100644
>> --- a/hw/arm/virt-acpi-build.c
>> +++ b/hw/arm/virt-acpi-build.c
>> @@ -208,6 +208,13 @@ static void acpi_dsdt_add_tpm(Aml *scope, 
>> VirtMachineState *vms)
>>   #define ROOT_COMPLEX_ENTRY_SIZE 36
>>   #define IORT_NODE_OFFSET 48
>> +static bool its_enabled(VirtMachineState *vms)
>> +{
>> +    VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
>> +
>> +    return its_class_name() && !vmc->no_its;
>> +}
>> +
> 
> Isn't its_class_name() always "true"?

The method signature is described as:

  /**
   * its_class_name:
   *
   * Return the ITS class name to use depending on whether
   * KVM acceleration  and KVM CAP_SIGNAL_MSI are supported
   *
   * Returns: class name to use or NULL
   */
  const char *its_class_name(void);

So I'd say no.

Indeed since commit cc5e719e2c8 ("kvm: require KVM_CAP_SIGNAL_MSI")
the single implementation doesn't return NULL anymore.

Paolo, can we update the signature and clean code path?

Anyhow Gustavo, while well noticed, this is pre-exising and unrelated
to the code movement in this patch.

> 
> 
> Cheers,
> Gustavo
> 
>>   /*
>>    * Append an ID mapping entry as described by "Table 4 ID mapping 
>> format" in
>>    * "IO Remapping Table System Software on ARM Platforms", Chapter 3.
>> @@ -670,7 +677,6 @@ static void
>>   build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState 
>> *vms)
>>   {
>>       int i;
>> -    VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
>>       const MemMapEntry *memmap = vms->memmap;
>>       AcpiTable table = { .sig = "APIC", .rev = 4, .oem_id = vms->oem_id,
>>                           .oem_table_id = vms->oem_table_id };
>> @@ -741,7 +747,7 @@ build_madt(GArray *table_data, BIOSLinker *linker, 
>> VirtMachineState *vms)
>>                                             
>> memmap[VIRT_HIGH_GIC_REDIST2].size);
>>           }
>> -        if (its_class_name() && !vmc->no_its) {
>> +        if (its_enabled(vms)) {
>>               /*
>>                * ACPI spec, Revision 6.0 Errata A
>>                * (original 6.0 definition has invalid Length)
>> @@ -973,7 +979,7 @@ void virt_acpi_build(VirtMachineState *vms, 
>> AcpiBuildTables *tables)
>>                             vms->oem_table_id);
>>       }
>> -    if (its_class_name() && !vmc->no_its) {
>> +    if (its_enabled(vms)) {
>>           acpi_add_table(table_offsets, tables_blob);
>>           build_iort(tables_blob, tables->linker, vms);
>>       }
> 

Re: [PATCH-for-10.0 1/5] qtest/bios-tables-test: Add test for -M virt,its=off

[Philippe Mathieu-Daudé]

On 2/4/25 08:41, Gustavo Romero wrote:
> Hi Phil,
> 
> On 3/31/25 19:12, Philippe Mathieu-Daudé wrote:
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> 
> Please, put commit message (body) into the commits.
> 
> For example, the commit message here could quickly explain that the FACP 
> table
> changed because virtualization=on (due to PSCI conduit). I'm assuming
> virtualization is set to on because gic-version=max and so GICv4 is 
> selected for
> testing. It also could be that  we want to exercise its=off when Arm 
> Virtualization
> Extensions are enabled, which is the common use case (I understand that ITS
> can be used also with virtualization=off).
> 
> Finally, the commit message could mention at the end which struct
> vanishes in APIC table and why IO remapping table is affected by
> ITS on/off.
> 
> A good commit message always help in code spelunking :)

I simply copied the reproducer from the issue, so I'll mention that.
(https://gitlab.com/qemu-project/qemu/-/issues/2886)

> 
> 
>> ---
>>   tests/qtest/bios-tables-test.c            |  22 ++++++++++++++++++++++
>>   tests/data/acpi/aarch64/virt/APIC.its_off | Bin 0 -> 184 bytes
>>   tests/data/acpi/aarch64/virt/FACP.its_off | Bin 0 -> 276 bytes
>>   tests/data/acpi/aarch64/virt/IORT.its_off | Bin 0 -> 236 bytes
>>   4 files changed, 22 insertions(+)
>>   create mode 100644 tests/data/acpi/aarch64/virt/APIC.its_off
>>   create mode 100644 tests/data/acpi/aarch64/virt/FACP.its_off
>>   create mode 100644 tests/data/acpi/aarch64/virt/IORT.its_off
>>
>> diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables- 
>> test.c
>> index 0a333ec4353..55366bf4956 100644
>> --- a/tests/qtest/bios-tables-test.c
>> +++ b/tests/qtest/bios-tables-test.c
>> @@ -2146,6 +2146,26 @@ static void 
>> test_acpi_aarch64_virt_tcg_topology(void)
>>       free_test_data(&data);
>>   }
>> +static void test_acpi_aarch64_virt_tcg_its_off(void)
>> +{
>> +    test_data data = {
>> +        .machine = "virt",
>> +        .arch = "aarch64",
>> +        .variant = ".its_off",
>> +        .tcg_only = true,
>> +        .uefi_fl1 = "pc-bios/edk2-aarch64-code.fd",
>> +        .uefi_fl2 = "pc-bios/edk2-arm-vars.fd",
>> +        .cd = "tests/data/uefi-boot-images/bios-tables- 
>> test.aarch64.iso.qcow2",
>> +        .ram_start = 0x40000000ULL,
>> +        .scan_len = 128ULL * 1024 * 1024,
>> +    };
>> +
>> +    test_acpi_one("-cpu cortex-a57 "
>> +                  "-M virtualization=on,secure=off "
>> +                  "-M gic-version=max,its=off,iommu=smmuv3", &data);
>> +    free_test_data(&data);
>> +}
>> +
>>   static void test_acpi_q35_viot(void)
>>   {
>>       test_data data = {
>> @@ -2577,6 +2597,8 @@ int main(int argc, char *argv[])
>>                              test_acpi_aarch64_virt_tcg_acpi_hmat);
>>               qtest_add_func("acpi/virt/topology",
>>                              test_acpi_aarch64_virt_tcg_topology);
>> +            qtest_add_func("acpi/virt/its_off",
>> +                           test_acpi_aarch64_virt_tcg_its_off);
>>               qtest_add_func("acpi/virt/numamem",
>>                              test_acpi_aarch64_virt_tcg_numamem);
>>               qtest_add_func("acpi/virt/memhp", 
>> test_acpi_aarch64_virt_tcg_memhp);
>> diff --git a/tests/data/acpi/aarch64/virt/APIC.its_off b/tests/data/ 
>> acpi/aarch64/virt/APIC.its_off
>> new file mode 100644
>> index 
>> 0000000000000000000000000000000000000000..c37d05d6e05805304f10afe73eb7cb9100fcccfa
>> GIT binary patch
>> literal 184
>> zcmZ<^@O0k6z`($=+{xeBBUr&HBEVSz2pEB4AU24G0Uik$i-7~iVgWL^17JJ`2AFzr
>> bgb+@aBn}xq0gwb2)Q)cq{30-g9B_L93G4|0
>>
>> literal 0
>> HcmV?d00001
>>
>> diff --git a/tests/data/acpi/aarch64/virt/FACP.its_off b/tests/data/ 
>> acpi/aarch64/virt/FACP.its_off
>> new file mode 100644
>> index 
>> 0000000000000000000000000000000000000000..606dac3fe4b55c31fd68b25d3a4127eeef227434
>> GIT binary patch
>> literal 276
>> zcmZ>BbPf<<WME(uaq@Te2v%^42yj*a0-z8Bhz+8t3j|P&V`N}P6&N^PpsQ~v$aVnZ
>> CVg~^L
>>
>> literal 0
>> HcmV?d00001
>>
>> diff --git a/tests/data/acpi/aarch64/virt/IORT.its_off b/tests/data/ 
>> acpi/aarch64/virt/IORT.its_off
>> new file mode 100644
>> index 
>> 0000000000000000000000000000000000000000..0fceb820d509e852ca0849baf568a8e93e426738
>> GIT binary patch
>> literal 236
>> zcmebD4+?q1z`(#9?&R<65v<@85#X!<1dKp25F11@1F-=RgMkDCNC*yK9F_<M77!bR
>> zUBI%eoFED&4;F$FSwK1)h;xBB2Py`m{{M%tVD>TjFfcO#g+N#Zh@s|zoCF3AP#UU@
>> R!2`+%Dg6Hr$N|zYvjDIZ5CH%H
>>
>> literal 0
>> HcmV?d00001
>>
> 
> I think the prescription for the acrobatics to update the ACPI expected
> tables says the blobs here should be empty (blob files are added empty)
> and at the same time they are listed in tests/qtest/bios-tables-test- 
> allowed-diff.h:
> 
>   * 1. add empty files for new tables, if any, under tests/data/acpi
>   * 2. list any changed files in tests/qtest/bios-tables-test-allowed- 
> diff.h
>   * 3. commit the above *before* making changes that affect the tables
> 
> (from tests/qtest/bios-tables-test.c header)
> 
> If that's correct, this patch should be merged with the following one 
> (2/5) and
> IORT.its_off and FACP.its_off should also be listed in
> tests/qtest/bios-tables-test-allowed-diff.h so the empty blobs won't 
> trigger
> a test failure.

I shouldn't have included the ACPI data in this patch but in the
following. IIUC, if no data/$TABLE.$variant, then the generic
data/$TABLE is used.

> 
> Then patch 5/5 should add the expected/updated blobs and drop the 
> *.its_off from
> bios-tables-test-allowed-diff.h. Patches 3/5 and 4/5 are sandwiched
> between (1/5 + 2/5) and (5/5).
> 
> At least that's what I get from:
> 
>   * The resulting patchset/pull request then looks like this:
>   * - patch 1: list changed files in tests/qtest/bios-tables-test- 
> allowed-diff.h.
>   * - patches 2 - n: real changes, may contain multiple patches.
>   * - patch n + 1: update golden master binaries and empty tests/qtest/ 
> bios-tables-test-allowed-diff.h
> 
> Otherwise the change looks good.
> 
> 
> Cheers,
> Gustavo

Re: [PATCH-for-10.0 2/5] qtest/bios-tables-test: Whitelist aarch64/virt/APIC.its_off blob

[Philippe Mathieu-Daudé]

On 2/4/25 08:43, Gustavo Romero wrote:
> Hi Phil,
> 
> On 3/31/25 19:12, Philippe Mathieu-Daudé wrote:
>> Prepare for ACPI table change in aarch64/virt/APIC.its_off.
> 
> The comment could be smth like:
> 
> Ignore APIC.its_off expected table (blob) for now until
> we update it later, after fixing the code that generates
> this table correctly.
> 
> ?
> 
> 
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.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 dfb8523c8bf..bfc4d601243 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/aarch64/virt/APIC.its_off",
> 
> I think this patch should be merged into 1/2, accordingly to my
> comment in 1/5. FACP and IORT .its_off files should be added to the
> list as well.

No, otherwise the test added in previous patch fails.

> 
> Btw, IMHO the name of this header is a tad misleading, because actually
> "allowed-diff" means that "we allow the machine's table to be different
> from the tables listed in this header", so it doesn't look like an
> allowlist (whitelist), it works more like an ignore list?
> 
> 
> Cheers,
> Gustavo

Re: [PATCH-for-10.0 5/5] qtest/bios-tables-test: Update aarch64/virt/APIC.its_off blob

[Philippe Mathieu-Daudé]

On 2/4/25 08:45, Gustavo Romero wrote:
> Hi Phil,
> 
> On 3/31/25 19:12, Philippe Mathieu-Daudé wrote:
>> Changes in the tables:
>>
>>    @@ -1,32 +1,32 @@
>>     /*
>>      * Intel ACPI Component Architecture
>>      * AML/ASL+ Disassembler version 20240927 (64-bit version)
>>      * Copyright (c) 2000 - 2023 Intel Corporation
>>      *
>>      * Disassembly of tests/data/acpi/aarch64/virt/APIC.its_off
>>      *
>>      * ACPI Data Table [APIC]
>>      *
>>      * Format: [HexOffset DecimalOffset ByteLength]  FieldName : 
>> FieldValue (in hex)
>>      */
>>
>>     [000h 0000 004h]                   Signature : "APIC"    [Multiple 
>> APIC Description Table (MADT)]
>>    -[004h 0004 004h]                Table Length : 000000B8
>>    +[004h 0004 004h]                Table Length : 000000A4
>>     [008h 0008 001h]                    Revision : 04
>>    -[009h 0009 001h]                    Checksum : A7
>>    +[009h 0009 001h]                    Checksum : EE
>>     [00Ah 0010 006h]                      Oem ID : "BOCHS "
>>     [010h 0016 008h]                Oem Table ID : "BXPC    "
>>     [018h 0024 004h]                Oem Revision : 00000001
>>     [01Ch 0028 004h]             Asl Compiler ID : "BXPC"
>>     [020h 0032 004h]       Asl Compiler Revision : 00000001
>>
>>     [024h 0036 004h]          Local Apic Address : 00000000
>>     [028h 0040 004h]       Flags (decoded below) : 00000000
>>                              PC-AT Compatibility : 0
>>
>>     [02Ch 0044 001h]               Subtable Type : 0C [Generic 
>> Interrupt Distributor]
>>     [02Dh 0045 001h]                      Length : 18
>>     [02Eh 0046 002h]                    Reserved : 0000
>>     [030h 0048 004h]       Local GIC Hardware ID : 00000000
>>     [034h 0052 008h]                Base Address : 0000000008000000
>>     [03Ch 0060 004h]              Interrupt Base : 00000000
>>    @@ -49,37 +49,29 @@
>>     [06Ch 0108 008h]    Virtual GIC Base Address : 0000000000000000
>>     [074h 0116 008h] Hypervisor GIC Base Address : 0000000000000000
>>     [07Ch 0124 004h]       Virtual GIC Interrupt : 00000019
>>     [080h 0128 008h]  Redistributor Base Address : 0000000000000000
>>     [088h 0136 008h]                   ARM MPIDR : 0000000000000000
>>     [090h 0144 001h]            Efficiency Class : 00
>>     [091h 0145 001h]                    Reserved : 00
>>     [092h 0146 002h]      SPE Overflow Interrupt : 0000
>>     [094h 0148 002h]              TRBE Interrupt : 100E
>>
>>     [094h 0148 001h]               Subtable Type : 0E [Generic 
>> Interrupt Redistributor]
>>     [095h 0149 001h]                      Length : 10
>>     [097h 0151 002h]                    Reserved : 0000
>>     [098h 0152 008h]                Base Address : 00000000080A0000
>>     [0A0h 0160 004h]                      Length : 00F60000
>>
>>    -[0A4h 0164 001h]               Subtable Type : 0F [Generic 
>> Interrupt Translator]
>>    -[0A5h 0165 001h]                      Length : 14
>>    -[0A7h 0167 002h]                    Reserved : 0000
>>    -[0A8h 0168 004h]              Translation ID : 00000000
>>    -[0ACh 0172 008h]                Base Address : 0000000008080000
>>    -[0B4h 0180 004h]                    Reserved : 00000000
>>    +Raw Table Data: Length 164 (0xA4)
>>
>>    -Raw Table Data: Length 184 (0xB8)
>>    -
>>    -    0000: 41 50 49 43 B8 00 00 00 04 A7 42 4F 43 48 53 20  // 
>> APIC......BOCHS
>>    +    0000: 41 50 49 43 A4 00 00 00 04 EE 42 4F 43 48 53 20  // 
>> APIC......BOCHS
>>         0010: 42 58 50 43 20 20 20 20 01 00 00 00 42 58 50 43  // 
>> BXPC    ....BXPC
>>         0020: 01 00 00 00 00 00 00 00 00 00 00 00 0C 18 00 
>> 00  // ................
>>         0030: 00 00 00 00 00 00 00 08 00 00 00 00 00 00 00 
>> 00  // ................
>>         0040: 04 00 00 00 0B 50 00 00 00 00 00 00 00 00 00 
>> 00  // .....P..........
>>         0050: 01 00 00 00 00 00 00 00 17 00 00 00 00 00 00 
>> 00  // ................
>>         0060: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
>> 00  // ................
>>         0070: 00 00 00 00 00 00 00 00 00 00 00 00 19 00 00 
>> 00  // ................
>>         0080: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
>> 00  // ................
>>         0090: 00 00 00 00 0E 10 00 00 00 00 0A 08 00 00 00 
>> 00  // ................
>>    -    00A0: 00 00 F6 00 0F 14 00 00 00 00 00 00 00 00 08 
>> 08  // ................
>>    -    00B0: 00 00 00 00 00 00 00 
>> 00                          // ........
>>    +    00A0: 00 00 F6 00                                      // ....
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   tests/qtest/bios-tables-test-allowed-diff.h |   1 -
>>   tests/data/acpi/aarch64/virt/APIC.its_off   | Bin 184 -> 164 bytes
>>   2 files changed, 1 deletion(-)
>>
>> diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/ 
>> qtest/bios-tables-test-allowed-diff.h
>> index bfc4d601243..dfb8523c8bf 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/aarch64/virt/APIC.its_off",
>> diff --git a/tests/data/acpi/aarch64/virt/APIC.its_off b/tests/data/ 
>> acpi/aarch64/virt/APIC.its_off
>> index 
>> c37d05d6e05805304f10afe73eb7cb9100fcccfa..f24ac8fbff5261a52434abcfcff96dbdc7709de4 100644
>> GIT binary patch
>> delta 18
>> ZcmdnNxP+0*F~HM#2?GNI%e#qOvj8xy1yKM1
>>
>> delta 39
>> jcmZ3&xPy_)F~HM#2Ll5G%kqg_vqbnsfJ`vp;DE6Jpmzmj
> 
> If the change affects other tables besides APIC (IORT and FACP in this 
> series),
> then I think that's the moment to update all the correct blobs (final 
> versions)
> and drop the list of blobs in bios-tables-test-allowed-diff.h. I also 
> think it's
> better to list all the table diffs in the commit message, not only the 
> diff for
> the APIC table.

When following the script steps with V=2, I only see diff change in the
MADT table.

Igor, am I missing something?

> 
> Cheers,
> Gustavo

Re: [PATCH-for-10.0 1/5] qtest/bios-tables-test: Add test for -M virt,its=off

[Gustavo Romero]

Hi Phil,

On 4/2/25 07:30, Philippe Mathieu-Daudé wrote:
> On 2/4/25 08:41, Gustavo Romero wrote:
>> Hi Phil,
>>
>> On 3/31/25 19:12, Philippe Mathieu-Daudé wrote:
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>
>> Please, put commit message (body) into the commits.
>>
>> For example, the commit message here could quickly explain that the FACP table
>> changed because virtualization=on (due to PSCI conduit). I'm assuming
>> virtualization is set to on because gic-version=max and so GICv4 is selected for
>> testing. It also could be that  we want to exercise its=off when Arm Virtualization
>> Extensions are enabled, which is the common use case (I understand that ITS
>> can be used also with virtualization=off).
>>
>> Finally, the commit message could mention at the end which struct
>> vanishes in APIC table and why IO remapping table is affected by
>> ITS on/off.
>>
>> A good commit message always help in code spelunking :)
> 
> I simply copied the reproducer from the issue, so I'll mention that.
> (https://gitlab.com/qemu-project/qemu/-/issues/2886)
> 
>>
>>
>>> ---
>>>   tests/qtest/bios-tables-test.c            |  22 ++++++++++++++++++++++
>>>   tests/data/acpi/aarch64/virt/APIC.its_off | Bin 0 -> 184 bytes
>>>   tests/data/acpi/aarch64/virt/FACP.its_off | Bin 0 -> 276 bytes
>>>   tests/data/acpi/aarch64/virt/IORT.its_off | Bin 0 -> 236 bytes
>>>   4 files changed, 22 insertions(+)
>>>   create mode 100644 tests/data/acpi/aarch64/virt/APIC.its_off
>>>   create mode 100644 tests/data/acpi/aarch64/virt/FACP.its_off
>>>   create mode 100644 tests/data/acpi/aarch64/virt/IORT.its_off
>>>
>>> diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables- test.c
>>> index 0a333ec4353..55366bf4956 100644
>>> --- a/tests/qtest/bios-tables-test.c
>>> +++ b/tests/qtest/bios-tables-test.c
>>> @@ -2146,6 +2146,26 @@ static void test_acpi_aarch64_virt_tcg_topology(void)
>>>       free_test_data(&data);
>>>   }
>>> +static void test_acpi_aarch64_virt_tcg_its_off(void)
>>> +{
>>> +    test_data data = {
>>> +        .machine = "virt",
>>> +        .arch = "aarch64",
>>> +        .variant = ".its_off",
>>> +        .tcg_only = true,
>>> +        .uefi_fl1 = "pc-bios/edk2-aarch64-code.fd",
>>> +        .uefi_fl2 = "pc-bios/edk2-arm-vars.fd",
>>> +        .cd = "tests/data/uefi-boot-images/bios-tables- test.aarch64.iso.qcow2",
>>> +        .ram_start = 0x40000000ULL,
>>> +        .scan_len = 128ULL * 1024 * 1024,
>>> +    };
>>> +
>>> +    test_acpi_one("-cpu cortex-a57 "
>>> +                  "-M virtualization=on,secure=off "
>>> +                  "-M gic-version=max,its=off,iommu=smmuv3", &data);
>>> +    free_test_data(&data);
>>> +}
>>> +
>>>   static void test_acpi_q35_viot(void)
>>>   {
>>>       test_data data = {
>>> @@ -2577,6 +2597,8 @@ int main(int argc, char *argv[])
>>>                              test_acpi_aarch64_virt_tcg_acpi_hmat);
>>>               qtest_add_func("acpi/virt/topology",
>>>                              test_acpi_aarch64_virt_tcg_topology);
>>> +            qtest_add_func("acpi/virt/its_off",
>>> +                           test_acpi_aarch64_virt_tcg_its_off);
>>>               qtest_add_func("acpi/virt/numamem",
>>>                              test_acpi_aarch64_virt_tcg_numamem);
>>>               qtest_add_func("acpi/virt/memhp", test_acpi_aarch64_virt_tcg_memhp);
>>> diff --git a/tests/data/acpi/aarch64/virt/APIC.its_off b/tests/data/ acpi/aarch64/virt/APIC.its_off
>>> new file mode 100644
>>> index 0000000000000000000000000000000000000000..c37d05d6e05805304f10afe73eb7cb9100fcccfa
>>> GIT binary patch
>>> literal 184
>>> zcmZ<^@O0k6z`($=+{xeBBUr&HBEVSz2pEB4AU24G0Uik$i-7~iVgWL^17JJ`2AFzr
>>> bgb+@aBn}xq0gwb2)Q)cq{30-g9B_L93G4|0
>>>
>>> literal 0
>>> HcmV?d00001
>>>
>>> diff --git a/tests/data/acpi/aarch64/virt/FACP.its_off b/tests/data/ acpi/aarch64/virt/FACP.its_off
>>> new file mode 100644
>>> index 0000000000000000000000000000000000000000..606dac3fe4b55c31fd68b25d3a4127eeef227434
>>> GIT binary patch
>>> literal 276
>>> zcmZ>BbPf<<WME(uaq@Te2v%^42yj*a0-z8Bhz+8t3j|P&V`N}P6&N^PpsQ~v$aVnZ
>>> CVg~^L
>>>
>>> literal 0
>>> HcmV?d00001
>>>
>>> diff --git a/tests/data/acpi/aarch64/virt/IORT.its_off b/tests/data/ acpi/aarch64/virt/IORT.its_off
>>> new file mode 100644
>>> index 0000000000000000000000000000000000000000..0fceb820d509e852ca0849baf568a8e93e426738
>>> GIT binary patch
>>> literal 236
>>> zcmebD4+?q1z`(#9?&R<65v<@85#X!<1dKp25F11@1F-=RgMkDCNC*yK9F_<M77!bR
>>> zUBI%eoFED&4;F$FSwK1)h;xBB2Py`m{{M%tVD>TjFfcO#g+N#Zh@s|zoCF3AP#UU@
>>> R!2`+%Dg6Hr$N|zYvjDIZ5CH%H
>>>
>>> literal 0
>>> HcmV?d00001
>>>
>>
>> I think the prescription for the acrobatics to update the ACPI expected
>> tables says the blobs here should be empty (blob files are added empty)
>> and at the same time they are listed in tests/qtest/bios-tables-test- allowed-diff.h:
>>
>>   * 1. add empty files for new tables, if any, under tests/data/acpi
>>   * 2. list any changed files in tests/qtest/bios-tables-test-allowed- diff.h
>>   * 3. commit the above *before* making changes that affect the tables
>>
>> (from tests/qtest/bios-tables-test.c header)
>>
>> If that's correct, this patch should be merged with the following one (2/5) and
>> IORT.its_off and FACP.its_off should also be listed in
>> tests/qtest/bios-tables-test-allowed-diff.h so the empty blobs won't trigger
>> a test failure.
> 
> I shouldn't have included the ACPI data in this patch but in the
> following. IIUC, if no data/$TABLE.$variant, then the generic
> data/$TABLE is used.

Yeah, it's correct that if no data/$TABLE.$variant is found then data/$TABLE is
used as a fallback. But my point actually was that in the first patch you should
create the blob .its_off variants for tables affected by the main change but
they must be empty, as per Step 1. in the prescription; and add them to the
"ignore list" (tests/qtest/bios-tables-test-allowed-diff.h) so they don't fail
the test, as per Step 2.

But on second thoughts I think Step 1. in prescription is confusing. Anyways,
what you're doing here is sensible.

Here (1/5), you're adding a new test, with new VM options. The new VM options
(different in comparison to the ¨baseline" data/$TABLE) cause changes to three
ACPI tables: APIC, FACP, and IORT, because:

- APIC: GICv2 is update to GICv4 due to gic-version=max + virtualization=on => GICv4
         and the addition of Subtable type 0xF for GIC ITS Structure (even tho its=off
         in the VM option, since that's the bug to be fixed down the road)

- FACP: Change of PSCI conduit due to virtualization=on option:
-                       Must use HVC for PSCI : 1
+                       Must use HVC for PSCI : 0 (use SMC instead)
because of logic in machvirt_init():
[...]
     } else if (vms->virt) { /* vms->virt is true is virtualization=on */
         vms->psci_conduit = QEMU_PSCI_CONDUIT_SMC;
     } else {
         vms->psci_conduit = QEMU_PSCI_CONDUIT_HVC;
     }

- IORT: A new node is added for SMMUv3 due to option iommu=smmuv3.


I think it's correct to add the blobs for .its_off to this patch, otherwise the test will
fail and, moreover, they are the results of the options being used for the new test.
As an alternative, you could add the *.its_off to tests/qtest/bios-tables-test-allowed-diff.h
so the differences would be ignored and the test passes, but really I think it makes
more sense as you're doing here.

The next patch (2/5) becomes, as you said in the commit message, a preparation for the
real changes (the fix), which will break temporarily the test, hence in 2/5 you add it
to the "ignore list", which is actually what Step 2. in the prescription recommends.

Thus:

Reviewed-by: Gustavo Romero <gustavo.romero@linaro.org>


Cheers,
Gustavo

>> Then patch 5/5 should add the expected/updated blobs and drop the *.its_off from
>> bios-tables-test-allowed-diff.h. Patches 3/5 and 4/5 are sandwiched
>> between (1/5 + 2/5) and (5/5).
>>
>> At least that's what I get from:
>>
>>   * The resulting patchset/pull request then looks like this:
>>   * - patch 1: list changed files in tests/qtest/bios-tables-test- allowed-diff.h.
>>   * - patches 2 - n: real changes, may contain multiple patches.
>>   * - patch n + 1: update golden master binaries and empty tests/qtest/ bios-tables-test-allowed-diff.h
>>
>> Otherwise the change looks good.
>>
>>
>> Cheers,
>> Gustavo
> 

Re: [PATCH-for-10.0 2/5] qtest/bios-tables-test: Whitelist aarch64/virt/APIC.its_off blob

[Gustavo Romero]

Hi Phil,

On 4/2/25 07:31, Philippe Mathieu-Daudé wrote:
> On 2/4/25 08:43, Gustavo Romero wrote:
>> Hi Phil,
>>
>> On 3/31/25 19:12, Philippe Mathieu-Daudé wrote:
>>> Prepare for ACPI table change in aarch64/virt/APIC.its_off.
>>
>> The comment could be smth like:
>>
>> Ignore APIC.its_off expected table (blob) for now until
>> we update it later, after fixing the code that generates
>> this table correctly.
>>
>> ?
>>
>>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.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 dfb8523c8bf..bfc4d601243 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/aarch64/virt/APIC.its_off",
>>
>> I think this patch should be merged into 1/2, accordingly to my
>> comment in 1/5. FACP and IORT .its_off files should be added to the
>> list as well.
> 
> No, otherwise the test added in previous patch fails.

I can't see how adding the tests to the list in
tests/qtest/bios-tables-test-allowed-diff.h can cause any failure.
The list in this header file works as a "ignore list", so even if
the .its_off blobs in 1/5 were empty (for instance) the test would
pass if they are in this list.

That said, as per my comments in 1/5, this preparation is correct
to me: the fix will cause changes to APIC table so the current
expected blob (committed) needs to be ignored until it gets updated
later, in 5/5.

Reviewed-by: Gustavo Romero <gustavo.romero@linaro.org>


Cheers,
Gustavo

Re: [PATCH-for-10.0 3/5] hw/arm/virt-acpi: Factor its_enabled() helper out

[Gustavo Romero]

Hi Phil,

On 4/2/25 07:27, Philippe Mathieu-Daudé wrote:
> On 2/4/25 08:43, Gustavo Romero wrote:
>> Hi Phil,
>>
>> On 3/31/25 19:12, Philippe Mathieu-Daudé wrote:
>>> GIC ITS is checked for the MADT and IORT tables.
>>> Factor the checks out to the its_enabled() helper.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>>   hw/arm/virt-acpi-build.c | 12 +++++++++---
>>>   1 file changed, 9 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>>> index 3ac8f8e1786..fdc08b40883 100644
>>> --- a/hw/arm/virt-acpi-build.c
>>> +++ b/hw/arm/virt-acpi-build.c
>>> @@ -208,6 +208,13 @@ static void acpi_dsdt_add_tpm(Aml *scope, VirtMachineState *vms)
>>>   #define ROOT_COMPLEX_ENTRY_SIZE 36
>>>   #define IORT_NODE_OFFSET 48
>>> +static bool its_enabled(VirtMachineState *vms)
>>> +{
>>> +    VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
>>> +
>>> +    return its_class_name() && !vmc->no_its;
>>> +}
>>> +
>>
>> Isn't its_class_name() always "true"?
> 
> The method signature is described as:
> 
>   /**
>    * its_class_name:
>    *
>    * Return the ITS class name to use depending on whether
>    * KVM acceleration  and KVM CAP_SIGNAL_MSI are supported
>    *
>    * Returns: class name to use or NULL
>    */
>   const char *its_class_name(void);
> 
> So I'd say no.
> 
> Indeed since commit cc5e719e2c8 ("kvm: require KVM_CAP_SIGNAL_MSI")
> the single implementation doesn't return NULL anymore.
> 
> Paolo, can we update the signature and clean code path?

Updating the signature won't solve the redundancy here. Using its_class_name()
for gating the generation of GIC ITS-related ACPI data is still moot.


> Anyhow Gustavo, while well noticed, this is pre-exising and unrelated
> to the code movement in this patch.

hmm I think the fix is kind simple: just remove its_class_name() from
the predicate in its_enabled(). Is that what you meant by "clean code path"
above?

This seems correct to me because we always have ITS present if
(vmc->no_its == false and vms->its == true). If TCG is used and option
its=on is given ITS is created in create_its(). If KVM accel is used
it's as in the commit message from Paolo you pointed out:

     ARM uses it to detect the presence of the ITS emulation in the kernel,
     introduced in Linux 4.8.  **Assume that it's there and possibly fail when
     realizing the arm-its-kvm device.**

So if kernel does not support in-kernel ITS kvm_arm_its_realize() will
bail out with "error creating in-kernel ITS".

It's up to you if you want to fix it in this series or not :)


>>>   /*
>>>    * Append an ID mapping entry as described by "Table 4 ID mapping format" in
>>>    * "IO Remapping Table System Software on ARM Platforms", Chapter 3.
>>> @@ -670,7 +677,6 @@ static void
>>>   build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>>>   {
>>>       int i;
>>> -    VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
>>>       const MemMapEntry *memmap = vms->memmap;
>>>       AcpiTable table = { .sig = "APIC", .rev = 4, .oem_id = vms->oem_id,
>>>                           .oem_table_id = vms->oem_table_id };
>>> @@ -741,7 +747,7 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>>> memmap[VIRT_HIGH_GIC_REDIST2].size);
>>>           }
>>> -        if (its_class_name() && !vmc->no_its) {
>>> +        if (its_enabled(vms)) {
>>>               /*
>>>                * ACPI spec, Revision 6.0 Errata A
>>>                * (original 6.0 definition has invalid Length)
>>> @@ -973,7 +979,7 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
>>>                             vms->oem_table_id);
>>>       }
>>> -    if (its_class_name() && !vmc->no_its) {
>>> +    if (its_enabled(vms)) {
>>>           acpi_add_table(table_offsets, tables_blob);
>>>           build_iort(tables_blob, tables->linker, vms);
>>>       }

I can't see how that's right. Gating IORT table generation entirely based
on the presence of ITS looks wrong because IORT table has data beyond GIC ITS,
like for SMMUv3 etc.. Maybe open an issue to investigate it later?


FWIW,

Reviewed-by: Gustavo Romero <gustavo.romero@linaro.org>


Cheers,
Gustavo

Re: [PATCH-for-10.0 5/5] qtest/bios-tables-test: Update aarch64/virt/APIC.its_off blob

[Gustavo Romero]

Hi Phil,

On 4/2/25 07:34, Philippe Mathieu-Daudé wrote:
> On 2/4/25 08:45, Gustavo Romero wrote:
>> Hi Phil,
>>
>> On 3/31/25 19:12, Philippe Mathieu-Daudé wrote:
>>> Changes in the tables:
>>>
>>>    @@ -1,32 +1,32 @@
>>>     /*
>>>      * Intel ACPI Component Architecture
>>>      * AML/ASL+ Disassembler version 20240927 (64-bit version)
>>>      * Copyright (c) 2000 - 2023 Intel Corporation
>>>      *
>>>      * Disassembly of tests/data/acpi/aarch64/virt/APIC.its_off
>>>      *
>>>      * ACPI Data Table [APIC]
>>>      *
>>>      * Format: [HexOffset DecimalOffset ByteLength]  FieldName : FieldValue (in hex)
>>>      */
>>>
>>>     [000h 0000 004h]                   Signature : "APIC"    [Multiple APIC Description Table (MADT)]
>>>    -[004h 0004 004h]                Table Length : 000000B8
>>>    +[004h 0004 004h]                Table Length : 000000A4
>>>     [008h 0008 001h]                    Revision : 04
>>>    -[009h 0009 001h]                    Checksum : A7
>>>    +[009h 0009 001h]                    Checksum : EE
>>>     [00Ah 0010 006h]                      Oem ID : "BOCHS "
>>>     [010h 0016 008h]                Oem Table ID : "BXPC    "
>>>     [018h 0024 004h]                Oem Revision : 00000001
>>>     [01Ch 0028 004h]             Asl Compiler ID : "BXPC"
>>>     [020h 0032 004h]       Asl Compiler Revision : 00000001
>>>
>>>     [024h 0036 004h]          Local Apic Address : 00000000
>>>     [028h 0040 004h]       Flags (decoded below) : 00000000
>>>                              PC-AT Compatibility : 0
>>>
>>>     [02Ch 0044 001h]               Subtable Type : 0C [Generic Interrupt Distributor]
>>>     [02Dh 0045 001h]                      Length : 18
>>>     [02Eh 0046 002h]                    Reserved : 0000
>>>     [030h 0048 004h]       Local GIC Hardware ID : 00000000
>>>     [034h 0052 008h]                Base Address : 0000000008000000
>>>     [03Ch 0060 004h]              Interrupt Base : 00000000
>>>    @@ -49,37 +49,29 @@
>>>     [06Ch 0108 008h]    Virtual GIC Base Address : 0000000000000000
>>>     [074h 0116 008h] Hypervisor GIC Base Address : 0000000000000000
>>>     [07Ch 0124 004h]       Virtual GIC Interrupt : 00000019
>>>     [080h 0128 008h]  Redistributor Base Address : 0000000000000000
>>>     [088h 0136 008h]                   ARM MPIDR : 0000000000000000
>>>     [090h 0144 001h]            Efficiency Class : 00
>>>     [091h 0145 001h]                    Reserved : 00
>>>     [092h 0146 002h]      SPE Overflow Interrupt : 0000
>>>     [094h 0148 002h]              TRBE Interrupt : 100E
>>>
>>>     [094h 0148 001h]               Subtable Type : 0E [Generic Interrupt Redistributor]
>>>     [095h 0149 001h]                      Length : 10
>>>     [097h 0151 002h]                    Reserved : 0000
>>>     [098h 0152 008h]                Base Address : 00000000080A0000
>>>     [0A0h 0160 004h]                      Length : 00F60000
>>>
>>>    -[0A4h 0164 001h]               Subtable Type : 0F [Generic Interrupt Translator]
>>>    -[0A5h 0165 001h]                      Length : 14
>>>    -[0A7h 0167 002h]                    Reserved : 0000
>>>    -[0A8h 0168 004h]              Translation ID : 00000000
>>>    -[0ACh 0172 008h]                Base Address : 0000000008080000
>>>    -[0B4h 0180 004h]                    Reserved : 00000000
>>>    +Raw Table Data: Length 164 (0xA4)
>>>
>>>    -Raw Table Data: Length 184 (0xB8)
>>>    -
>>>    -    0000: 41 50 49 43 B8 00 00 00 04 A7 42 4F 43 48 53 20  // APIC......BOCHS
>>>    +    0000: 41 50 49 43 A4 00 00 00 04 EE 42 4F 43 48 53 20  // APIC......BOCHS
>>>         0010: 42 58 50 43 20 20 20 20 01 00 00 00 42 58 50 43  // BXPC    ....BXPC
>>>         0020: 01 00 00 00 00 00 00 00 00 00 00 00 0C 18 00 00  // ................
>>>         0030: 00 00 00 00 00 00 00 08 00 00 00 00 00 00 00 00  // ................
>>>         0040: 04 00 00 00 0B 50 00 00 00 00 00 00 00 00 00 00  // .....P..........
>>>         0050: 01 00 00 00 00 00 00 00 17 00 00 00 00 00 00 00  // ................
>>>         0060: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  // ................
>>>         0070: 00 00 00 00 00 00 00 00 00 00 00 00 19 00 00 00  // ................
>>>         0080: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  // ................
>>>         0090: 00 00 00 00 0E 10 00 00 00 00 0A 08 00 00 00 00  // ................
>>>    -    00A0: 00 00 F6 00 0F 14 00 00 00 00 00 00 00 00 08 08  // ................
>>>    -    00B0: 00 00 00 00 00 00 00 00                          // ........
>>>    +    00A0: 00 00 F6 00                                      // ....
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>>   tests/qtest/bios-tables-test-allowed-diff.h |   1 -
>>>   tests/data/acpi/aarch64/virt/APIC.its_off   | Bin 184 -> 164 bytes
>>>   2 files changed, 1 deletion(-)
>>>
>>> diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/ qtest/bios-tables-test-allowed-diff.h
>>> index bfc4d601243..dfb8523c8bf 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/aarch64/virt/APIC.its_off",
>>> diff --git a/tests/data/acpi/aarch64/virt/APIC.its_off b/tests/data/ acpi/aarch64/virt/APIC.its_off
>>> index c37d05d6e05805304f10afe73eb7cb9100fcccfa..f24ac8fbff5261a52434abcfcff96dbdc7709de4 100644
>>> GIT binary patch
>>> delta 18
>>> ZcmdnNxP+0*F~HM#2?GNI%e#qOvj8xy1yKM1
>>>
>>> delta 39
>>> jcmZ3&xPy_)F~HM#2Ll5G%kqg_vqbnsfJ`vp;DE6Jpmzmj
>>
>> If the change affects other tables besides APIC (IORT and FACP in this series),
>> then I think that's the moment to update all the correct blobs (final versions)
>> and drop the list of blobs in bios-tables-test-allowed-diff.h. I also think it's
>> better to list all the table diffs in the commit message, not only the diff for
>> the APIC table.
> 
> When following the script steps with V=2, I only see diff change in the
> MADT table.

I was thinking of the case if empty blobs were added to 1/5. Indeed, FACP table does
not change, sorry for the confusion.

After the fix just APIC table changes, so the patch is correctly listing it in the
commit message and removing it from the "ignore list".

However, as I mentioned in 3/5, the IORT table is being gated as well by its_enabled(),
so after the fix no IORT table is generated by QEMU, hence tests/qtest/bios-tables-test.c
never sees/loads any IORT table to compare against IORT.its_off added in 1/5. There are
no test failures but IORT.its_off is a "dead blob". I think it should be removed in this
patch just like APIC table blob is updated, for consistence.


Cheers,
Gustavo
  
> Igor, am I missing something?
> 
>>
>> Cheers,
>> Gustavo
> 

Re: [PATCH-for-10.0 1/5] qtest/bios-tables-test: Add test for -M virt,its=off

[Philippe Mathieu-Daudé]

On 3/4/25 08:25, Gustavo Romero wrote:
> Hi Phil,
> 
> On 4/2/25 07:30, Philippe Mathieu-Daudé wrote:
>> On 2/4/25 08:41, Gustavo Romero wrote:
>>> Hi Phil,
>>>
>>> On 3/31/25 19:12, Philippe Mathieu-Daudé wrote:
>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>
>>> Please, put commit message (body) into the commits.
>>>
>>> For example, the commit message here could quickly explain that the 
>>> FACP table
>>> changed because virtualization=on (due to PSCI conduit). I'm assuming
>>> virtualization is set to on because gic-version=max and so GICv4 is 
>>> selected for
>>> testing. It also could be that  we want to exercise its=off when Arm 
>>> Virtualization
>>> Extensions are enabled, which is the common use case (I understand 
>>> that ITS
>>> can be used also with virtualization=off).
>>>
>>> Finally, the commit message could mention at the end which struct
>>> vanishes in APIC table and why IO remapping table is affected by
>>> ITS on/off.
>>>
>>> A good commit message always help in code spelunking :)
>>
>> I simply copied the reproducer from the issue, so I'll mention that.
>> (https://gitlab.com/qemu-project/qemu/-/issues/2886)
>>
>>>
>>>
>>>> ---
>>>>   tests/qtest/bios-tables-test.c            |  22 ++++++++++++++++++ 
>>>> ++++
>>>>   tests/data/acpi/aarch64/virt/APIC.its_off | Bin 0 -> 184 bytes
>>>>   tests/data/acpi/aarch64/virt/FACP.its_off | Bin 0 -> 276 bytes
>>>>   tests/data/acpi/aarch64/virt/IORT.its_off | Bin 0 -> 236 bytes
>>>>   4 files changed, 22 insertions(+)
>>>>   create mode 100644 tests/data/acpi/aarch64/virt/APIC.its_off
>>>>   create mode 100644 tests/data/acpi/aarch64/virt/FACP.its_off
>>>>   create mode 100644 tests/data/acpi/aarch64/virt/IORT.its_off
>>>>
>>>> diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios- 
>>>> tables- test.c
>>>> index 0a333ec4353..55366bf4956 100644
>>>> --- a/tests/qtest/bios-tables-test.c
>>>> +++ b/tests/qtest/bios-tables-test.c
>>>> @@ -2146,6 +2146,26 @@ static void 
>>>> test_acpi_aarch64_virt_tcg_topology(void)
>>>>       free_test_data(&data);
>>>>   }
>>>> +static void test_acpi_aarch64_virt_tcg_its_off(void)
>>>> +{
>>>> +    test_data data = {
>>>> +        .machine = "virt",
>>>> +        .arch = "aarch64",
>>>> +        .variant = ".its_off",
>>>> +        .tcg_only = true,
>>>> +        .uefi_fl1 = "pc-bios/edk2-aarch64-code.fd",
>>>> +        .uefi_fl2 = "pc-bios/edk2-arm-vars.fd",
>>>> +        .cd = "tests/data/uefi-boot-images/bios-tables- 
>>>> test.aarch64.iso.qcow2",
>>>> +        .ram_start = 0x40000000ULL,
>>>> +        .scan_len = 128ULL * 1024 * 1024,
>>>> +    };
>>>> +
>>>> +    test_acpi_one("-cpu cortex-a57 "
>>>> +                  "-M virtualization=on,secure=off "
>>>> +                  "-M gic-version=max,its=off,iommu=smmuv3", &data);
>>>> +    free_test_data(&data);
>>>> +}
>>>> +
>>>>   static void test_acpi_q35_viot(void)
>>>>   {
>>>>       test_data data = {
>>>> @@ -2577,6 +2597,8 @@ int main(int argc, char *argv[])
>>>>                              test_acpi_aarch64_virt_tcg_acpi_hmat);
>>>>               qtest_add_func("acpi/virt/topology",
>>>>                              test_acpi_aarch64_virt_tcg_topology);
>>>> +            qtest_add_func("acpi/virt/its_off",
>>>> +                           test_acpi_aarch64_virt_tcg_its_off);
>>>>               qtest_add_func("acpi/virt/numamem",
>>>>                              test_acpi_aarch64_virt_tcg_numamem);
>>>>               qtest_add_func("acpi/virt/memhp", 
>>>> test_acpi_aarch64_virt_tcg_memhp);
>>>> diff --git a/tests/data/acpi/aarch64/virt/APIC.its_off b/tests/data/ 
>>>> acpi/aarch64/virt/APIC.its_off
>>>> new file mode 100644
>>>> index 
>>>> 0000000000000000000000000000000000000000..c37d05d6e05805304f10afe73eb7cb9100fcccfa
>>>> GIT binary patch
>>>> literal 184
>>>> zcmZ<^@O0k6z`($=+{xeBBUr&HBEVSz2pEB4AU24G0Uik$i-7~iVgWL^17JJ`2AFzr
>>>> bgb+@aBn}xq0gwb2)Q)cq{30-g9B_L93G4|0
>>>>
>>>> literal 0
>>>> HcmV?d00001
>>>>
>>>> diff --git a/tests/data/acpi/aarch64/virt/FACP.its_off b/tests/data/ 
>>>> acpi/aarch64/virt/FACP.its_off
>>>> new file mode 100644
>>>> index 
>>>> 0000000000000000000000000000000000000000..606dac3fe4b55c31fd68b25d3a4127eeef227434
>>>> GIT binary patch
>>>> literal 276
>>>> zcmZ>BbPf<<WME(uaq@Te2v%^42yj*a0-z8Bhz+8t3j|P&V`N}P6&N^PpsQ~v$aVnZ
>>>> CVg~^L
>>>>
>>>> literal 0
>>>> HcmV?d00001
>>>>
>>>> diff --git a/tests/data/acpi/aarch64/virt/IORT.its_off b/tests/data/ 
>>>> acpi/aarch64/virt/IORT.its_off
>>>> new file mode 100644
>>>> index 
>>>> 0000000000000000000000000000000000000000..0fceb820d509e852ca0849baf568a8e93e426738
>>>> GIT binary patch
>>>> literal 236
>>>> zcmebD4+?q1z`(#9?&R<65v<@85#X!<1dKp25F11@1F-=RgMkDCNC*yK9F_<M77!bR
>>>> zUBI%eoFED&4;F$FSwK1)h;xBB2Py`m{{M%tVD>TjFfcO#g+N#Zh@s|zoCF3AP#UU@
>>>> R!2`+%Dg6Hr$N|zYvjDIZ5CH%H
>>>>
>>>> literal 0
>>>> HcmV?d00001
>>>>
>>>
>>> I think the prescription for the acrobatics to update the ACPI expected
>>> tables says the blobs here should be empty (blob files are added empty)
>>> and at the same time they are listed in tests/qtest/bios-tables-test- 
>>> allowed-diff.h:
>>>
>>>   * 1. add empty files for new tables, if any, under tests/data/acpi
>>>   * 2. list any changed files in tests/qtest/bios-tables-test- 
>>> allowed- diff.h
>>>   * 3. commit the above *before* making changes that affect the tables
>>>
>>> (from tests/qtest/bios-tables-test.c header)
>>>
>>> If that's correct, this patch should be merged with the following one 
>>> (2/5) and
>>> IORT.its_off and FACP.its_off should also be listed in
>>> tests/qtest/bios-tables-test-allowed-diff.h so the empty blobs won't 
>>> trigger
>>> a test failure.
>>
>> I shouldn't have included the ACPI data in this patch but in the
>> following. IIUC, if no data/$TABLE.$variant, then the generic
>> data/$TABLE is used.
> 
> Yeah, it's correct that if no data/$TABLE.$variant is found then data/ 
> $TABLE is
> used as a fallback. But my point actually was that in the first patch 
> you should
> create the blob .its_off variants for tables affected by the main change 
> but
> they must be empty, as per Step 1. in the prescription; and add them to the
> "ignore list" (tests/qtest/bios-tables-test-allowed-diff.h) so they 
> don't fail
> the test, as per Step 2.

Without data files:

stderr:
acpi-test: Warning! FACP binary file mismatch. Actual 
[aml:/tmp/aml-379632], Expected [aml:tests/data/acpi/aarch64/virt/FACP].
See source file tests/qtest/bios-tables-test.c for instructions on how 
to update expected files.
acpi-test: Warning! FACP mismatch. Actual [asl:/tmp/asl-2XA732.dsl, 
aml:/tmp/aml-379632], Expected [asl:/tmp/asl-LO1632.dsl, 
aml:tests/data/acpi/aarch64/virt/FACP].
acpi-test: Warning! APIC binary file mismatch. Actual 
[aml:/tmp/aml-ZCA732], Expected [aml:tests/data/acpi/aarch64/virt/APIC].
See source file tests/qtest/bios-tables-test.c for instructions on how 
to update expected files.
acpi-test: Warning! APIC mismatch. Actual [asl:/tmp/asl-0XH832.dsl, 
aml:/tmp/aml-ZCA732], Expected [asl:/tmp/asl-YBK832.dsl, 
aml:tests/data/acpi/aarch64/virt/APIC].
acpi-test: Warning! IORT binary file mismatch. Actual 
[aml:/tmp/aml-GX9632], Expected [aml:tests/data/acpi/aarch64/virt/IORT].
See source file tests/qtest/bios-tables-test.c for instructions on how 
to update expected files.
acpi-test: Warning! IORT mismatch. Actual [asl:/tmp/asl-SPM832.dsl, 
aml:/tmp/aml-GX9632], Expected [asl:/tmp/asl-4SP832.dsl, 
aml:tests/data/acpi/aarch64/virt/IORT].
**
ERROR:../../tests/qtest/bios-tables-test.c:554:test_acpi_asl: assertion 
failed: (all_tables_match)

With empty data files:

stderr:
Warning! zero length expected file 
'tests/data/acpi/aarch64/virt/FACP.its_off'
Warning! zero length expected file 
'tests/data/acpi/aarch64/virt/APIC.its_off'
Warning! zero length expected file 
'tests/data/acpi/aarch64/virt/IORT.its_off'
Warning! zero length expected file 
'tests/data/acpi/aarch64/virt/FACP.its_off'
Warning! zero length expected file 
'tests/data/acpi/aarch64/virt/APIC.its_off'
Warning! zero length expected file 
'tests/data/acpi/aarch64/virt/IORT.its_off'
acpi-test: Warning!  binary file mismatch. Actual [aml:/tmp/aml-T6YE42], 
Expected [aml:tests/data/acpi/aarch64/virt/FACP.its_off].
See source file tests/qtest/bios-tables-test.c for instructions on how 
to update expected files.
acpi-test: Warning!  mismatch. Actual [asl:/tmp/asl-FG0E42.dsl, 
aml:/tmp/aml-T6YE42], Expected [asl:/tmp/asl-SE2E42.dsl, 
aml:tests/data/acpi/aarch64/virt/FACP.its_off].
acpi-test: Warning!  binary file mismatch. Actual [aml:/tmp/aml-0DZE42], 
Expected [aml:tests/data/acpi/aarch64/virt/APIC.its_off].
See source file tests/qtest/bios-tables-test.c for instructions on how 
to update expected files.
acpi-test: Warning!  mismatch. Actual [asl:/tmp/asl-3G3E42.dsl, 
aml:/tmp/aml-0DZE42], Expected [asl:/tmp/asl-RV4E42.dsl, 
aml:tests/data/acpi/aarch64/virt/APIC.its_off].
acpi-test: Warning!  binary file mismatch. Actual [aml:/tmp/aml-YH0E42], 
Expected [aml:tests/data/acpi/aarch64/virt/IORT.its_off].
See source file tests/qtest/bios-tables-test.c for instructions on how 
to update expected files.
acpi-test: Warning!  mismatch. Actual [asl:/tmp/asl-BX4E42.dsl, 
aml:/tmp/aml-YH0E42], Expected [asl:/tmp/asl-OL7E42.dsl, 
aml:tests/data/acpi/aarch64/virt/IORT.its_off].
**
ERROR:../../tests/qtest/bios-tables-test.c:554:test_acpi_asl: assertion 
failed: (all_tables_match)

> But on second thoughts I think Step 1. in prescription is confusing. 

I'll try to improve that.

> Anyways,
> what you're doing here is sensible.
> 
> Here (1/5), you're adding a new test, with new VM options. The new VM 
> options
> (different in comparison to the ¨baseline" data/$TABLE) cause changes to 
> three
> ACPI tables: APIC, FACP, and IORT, because:
> 
> - APIC: GICv2 is update to GICv4 due to gic-version=max + 
> virtualization=on => GICv4
>          and the addition of Subtable type 0xF for GIC ITS Structure 
> (even tho its=off
>          in the VM option, since that's the bug to be fixed down the road)
> 
> - FACP: Change of PSCI conduit due to virtualization=on option:
> -                       Must use HVC for PSCI : 1
> +                       Must use HVC for PSCI : 0 (use SMC instead)
> because of logic in machvirt_init():
> [...]
>      } else if (vms->virt) { /* vms->virt is true is virtualization=on */
>          vms->psci_conduit = QEMU_PSCI_CONDUIT_SMC;
>      } else {
>          vms->psci_conduit = QEMU_PSCI_CONDUIT_HVC;
>      }
> 
> - IORT: A new node is added for SMMUv3 due to option iommu=smmuv3.
> 
> 
> I think it's correct to add the blobs for .its_off to this patch, 
> otherwise the test will
> fail and, moreover, they are the results of the options being used for 
> the new test.
> As an alternative, you could add the *.its_off to tests/qtest/bios- 
> tables-test-allowed-diff.h
> so the differences would be ignored and the test passes, but really I 
> think it makes
> more sense as you're doing here.
> 
> The next patch (2/5) becomes, as you said in the commit message, a 
> preparation for the
> real changes (the fix), which will break temporarily the test, hence in 
> 2/5 you add it
> to the "ignore list", which is actually what Step 2. in the prescription 
> recommends.
> 
> Thus:
> 
> Reviewed-by: Gustavo Romero <gustavo.romero@linaro.org>

Thanks!

Re: [PATCH-for-10.0 0/5] hw/arm/virt-acpi: Do not advertise disabled GIC ITS in MADT table

[Michael S. Tsirkin]

On Tue, Apr 01, 2025 at 12:12:34AM +0200, Philippe Mathieu-Daudé wrote:
> GIC ITS can be disabled using '-M its=off'.
> When that occurs, it shouldn't be advertised in ACPI tables.

I doubt this is 10.0 material. How common is its=off that we
urgently need to fix it?

> Philippe Mathieu-Daudé (5):
>   qtest/bios-tables-test: Add test for -M virt,its=off
>   qtest/bios-tables-test: Whitelist aarch64/virt/APIC.its_off blob
>   hw/arm/virt-acpi: Factor its_enabled() helper out
>   hw/arm/virt-acpi: Do not advertise disabled GIC ITS
>   qtest/bios-tables-test: Update aarch64/virt/APIC.its_off blob
> 
>  hw/arm/virt-acpi-build.c                  |  12 +++++++++---
>  tests/qtest/bios-tables-test.c            |  22 ++++++++++++++++++++++
>  tests/data/acpi/aarch64/virt/APIC.its_off | Bin 0 -> 164 bytes
>  tests/data/acpi/aarch64/virt/FACP.its_off | Bin 0 -> 276 bytes
>  tests/data/acpi/aarch64/virt/IORT.its_off | Bin 0 -> 236 bytes
>  5 files changed, 31 insertions(+), 3 deletions(-)
>  create mode 100644 tests/data/acpi/aarch64/virt/APIC.its_off
>  create mode 100644 tests/data/acpi/aarch64/virt/FACP.its_off
>  create mode 100644 tests/data/acpi/aarch64/virt/IORT.its_off
> 
> -- 
> 2.47.1

Re: [PATCH-for-10.0 1/5] qtest/bios-tables-test: Add test for -M virt,its=off

[Gustavo Romero]

Hi Phil,

On 4/3/25 09:47, Philippe Mathieu-Daudé wrote:
> On 3/4/25 08:25, Gustavo Romero wrote:
>> Hi Phil,
>>
>> On 4/2/25 07:30, Philippe Mathieu-Daudé wrote:
>>> On 2/4/25 08:41, Gustavo Romero wrote:
>>>> Hi Phil,
>>>>
>>>> On 3/31/25 19:12, Philippe Mathieu-Daudé wrote:
>>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>>
>>>> Please, put commit message (body) into the commits.
>>>>
>>>> For example, the commit message here could quickly explain that the FACP table
>>>> changed because virtualization=on (due to PSCI conduit). I'm assuming
>>>> virtualization is set to on because gic-version=max and so GICv4 is selected for
>>>> testing. It also could be that  we want to exercise its=off when Arm Virtualization
>>>> Extensions are enabled, which is the common use case (I understand that ITS
>>>> can be used also with virtualization=off).
>>>>
>>>> Finally, the commit message could mention at the end which struct
>>>> vanishes in APIC table and why IO remapping table is affected by
>>>> ITS on/off.
>>>>
>>>> A good commit message always help in code spelunking :)
>>>
>>> I simply copied the reproducer from the issue, so I'll mention that.
>>> (https://gitlab.com/qemu-project/qemu/-/issues/2886)
>>>
>>>>
>>>>
>>>>> ---
>>>>>   tests/qtest/bios-tables-test.c            |  22 ++++++++++++++++++ ++++
>>>>>   tests/data/acpi/aarch64/virt/APIC.its_off | Bin 0 -> 184 bytes
>>>>>   tests/data/acpi/aarch64/virt/FACP.its_off | Bin 0 -> 276 bytes
>>>>>   tests/data/acpi/aarch64/virt/IORT.its_off | Bin 0 -> 236 bytes
>>>>>   4 files changed, 22 insertions(+)
>>>>>   create mode 100644 tests/data/acpi/aarch64/virt/APIC.its_off
>>>>>   create mode 100644 tests/data/acpi/aarch64/virt/FACP.its_off
>>>>>   create mode 100644 tests/data/acpi/aarch64/virt/IORT.its_off
>>>>>
>>>>> diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios- tables- test.c
>>>>> index 0a333ec4353..55366bf4956 100644
>>>>> --- a/tests/qtest/bios-tables-test.c
>>>>> +++ b/tests/qtest/bios-tables-test.c
>>>>> @@ -2146,6 +2146,26 @@ static void test_acpi_aarch64_virt_tcg_topology(void)
>>>>>       free_test_data(&data);
>>>>>   }
>>>>> +static void test_acpi_aarch64_virt_tcg_its_off(void)
>>>>> +{
>>>>> +    test_data data = {
>>>>> +        .machine = "virt",
>>>>> +        .arch = "aarch64",
>>>>> +        .variant = ".its_off",
>>>>> +        .tcg_only = true,
>>>>> +        .uefi_fl1 = "pc-bios/edk2-aarch64-code.fd",
>>>>> +        .uefi_fl2 = "pc-bios/edk2-arm-vars.fd",
>>>>> +        .cd = "tests/data/uefi-boot-images/bios-tables- test.aarch64.iso.qcow2",
>>>>> +        .ram_start = 0x40000000ULL,
>>>>> +        .scan_len = 128ULL * 1024 * 1024,
>>>>> +    };
>>>>> +
>>>>> +    test_acpi_one("-cpu cortex-a57 "
>>>>> +                  "-M virtualization=on,secure=off "
>>>>> +                  "-M gic-version=max,its=off,iommu=smmuv3", &data);
>>>>> +    free_test_data(&data);
>>>>> +}
>>>>> +
>>>>>   static void test_acpi_q35_viot(void)
>>>>>   {
>>>>>       test_data data = {
>>>>> @@ -2577,6 +2597,8 @@ int main(int argc, char *argv[])
>>>>>                              test_acpi_aarch64_virt_tcg_acpi_hmat);
>>>>>               qtest_add_func("acpi/virt/topology",
>>>>>                              test_acpi_aarch64_virt_tcg_topology);
>>>>> +            qtest_add_func("acpi/virt/its_off",
>>>>> +                           test_acpi_aarch64_virt_tcg_its_off);
>>>>>               qtest_add_func("acpi/virt/numamem",
>>>>>                              test_acpi_aarch64_virt_tcg_numamem);
>>>>>               qtest_add_func("acpi/virt/memhp", test_acpi_aarch64_virt_tcg_memhp);
>>>>> diff --git a/tests/data/acpi/aarch64/virt/APIC.its_off b/tests/data/ acpi/aarch64/virt/APIC.its_off
>>>>> new file mode 100644
>>>>> index 0000000000000000000000000000000000000000..c37d05d6e05805304f10afe73eb7cb9100fcccfa
>>>>> GIT binary patch
>>>>> literal 184
>>>>> zcmZ<^@O0k6z`($=+{xeBBUr&HBEVSz2pEB4AU24G0Uik$i-7~iVgWL^17JJ`2AFzr
>>>>> bgb+@aBn}xq0gwb2)Q)cq{30-g9B_L93G4|0
>>>>>
>>>>> literal 0
>>>>> HcmV?d00001
>>>>>
>>>>> diff --git a/tests/data/acpi/aarch64/virt/FACP.its_off b/tests/data/ acpi/aarch64/virt/FACP.its_off
>>>>> new file mode 100644
>>>>> index 0000000000000000000000000000000000000000..606dac3fe4b55c31fd68b25d3a4127eeef227434
>>>>> GIT binary patch
>>>>> literal 276
>>>>> zcmZ>BbPf<<WME(uaq@Te2v%^42yj*a0-z8Bhz+8t3j|P&V`N}P6&N^PpsQ~v$aVnZ
>>>>> CVg~^L
>>>>>
>>>>> literal 0
>>>>> HcmV?d00001
>>>>>
>>>>> diff --git a/tests/data/acpi/aarch64/virt/IORT.its_off b/tests/data/ acpi/aarch64/virt/IORT.its_off
>>>>> new file mode 100644
>>>>> index 0000000000000000000000000000000000000000..0fceb820d509e852ca0849baf568a8e93e426738
>>>>> GIT binary patch
>>>>> literal 236
>>>>> zcmebD4+?q1z`(#9?&R<65v<@85#X!<1dKp25F11@1F-=RgMkDCNC*yK9F_<M77!bR
>>>>> zUBI%eoFED&4;F$FSwK1)h;xBB2Py`m{{M%tVD>TjFfcO#g+N#Zh@s|zoCF3AP#UU@
>>>>> R!2`+%Dg6Hr$N|zYvjDIZ5CH%H
>>>>>
>>>>> literal 0
>>>>> HcmV?d00001
>>>>>
>>>>
>>>> I think the prescription for the acrobatics to update the ACPI expected
>>>> tables says the blobs here should be empty (blob files are added empty)
>>>> and at the same time they are listed in tests/qtest/bios-tables-test- allowed-diff.h:
>>>>
>>>>   * 1. add empty files for new tables, if any, under tests/data/acpi
>>>>   * 2. list any changed files in tests/qtest/bios-tables-test- allowed- diff.h
>>>>   * 3. commit the above *before* making changes that affect the tables
>>>>
>>>> (from tests/qtest/bios-tables-test.c header)
>>>>
>>>> If that's correct, this patch should be merged with the following one (2/5) and
>>>> IORT.its_off and FACP.its_off should also be listed in
>>>> tests/qtest/bios-tables-test-allowed-diff.h so the empty blobs won't trigger
>>>> a test failure.
>>>
>>> I shouldn't have included the ACPI data in this patch but in the
>>> following. IIUC, if no data/$TABLE.$variant, then the generic
>>> data/$TABLE is used.
>>
>> Yeah, it's correct that if no data/$TABLE.$variant is found then data/ $TABLE is
>> used as a fallback. But my point actually was that in the first patch you should
>> create the blob .its_off variants for tables affected by the main change but
>> they must be empty, as per Step 1. in the prescription; and add them to the
>> "ignore list" (tests/qtest/bios-tables-test-allowed-diff.h) so they don't fail
>> the test, as per Step 2.
> 
> Without data files:
> 
> stderr:
> acpi-test: Warning! FACP binary file mismatch. Actual [aml:/tmp/aml-379632], Expected [aml:tests/data/acpi/aarch64/virt/FACP].
> See source file tests/qtest/bios-tables-test.c for instructions on how to update expected files.
> acpi-test: Warning! FACP mismatch. Actual [asl:/tmp/asl-2XA732.dsl, aml:/tmp/aml-379632], Expected [asl:/tmp/asl-LO1632.dsl, aml:tests/data/acpi/aarch64/virt/FACP].
> acpi-test: Warning! APIC binary file mismatch. Actual [aml:/tmp/aml-ZCA732], Expected [aml:tests/data/acpi/aarch64/virt/APIC].
> See source file tests/qtest/bios-tables-test.c for instructions on how to update expected files.
> acpi-test: Warning! APIC mismatch. Actual [asl:/tmp/asl-0XH832.dsl, aml:/tmp/aml-ZCA732], Expected [asl:/tmp/asl-YBK832.dsl, aml:tests/data/acpi/aarch64/virt/APIC].
> acpi-test: Warning! IORT binary file mismatch. Actual [aml:/tmp/aml-GX9632], Expected [aml:tests/data/acpi/aarch64/virt/IORT].
> See source file tests/qtest/bios-tables-test.c for instructions on how to update expected files.
> acpi-test: Warning! IORT mismatch. Actual [asl:/tmp/asl-SPM832.dsl, aml:/tmp/aml-GX9632], Expected [asl:/tmp/asl-4SP832.dsl, aml:tests/data/acpi/aarch64/virt/IORT].
> **
> ERROR:../../tests/qtest/bios-tables-test.c:554:test_acpi_asl: assertion failed: (all_tables_match)

This is expected because if variant blobs are missing then data/$TABLE ones are used as the
blobs to be compared against the VM tables. Adding the data/$TABLE.$variant to the
"ignore list" (tests/qtest/bios-tables-test-allowed-diff.h) has no effect and so the test fails.


> With empty data files:
> 
> stderr:
> Warning! zero length expected file 'tests/data/acpi/aarch64/virt/FACP.its_off'
> Warning! zero length expected file 'tests/data/acpi/aarch64/virt/APIC.its_off'
> Warning! zero length expected file 'tests/data/acpi/aarch64/virt/IORT.its_off'
> Warning! zero length expected file 'tests/data/acpi/aarch64/virt/FACP.its_off'
> Warning! zero length expected file 'tests/data/acpi/aarch64/virt/APIC.its_off'
> Warning! zero length expected file 'tests/data/acpi/aarch64/virt/IORT.its_off'
> acpi-test: Warning!  binary file mismatch. Actual [aml:/tmp/aml-T6YE42], Expected [aml:tests/data/acpi/aarch64/virt/FACP.its_off].
> See source file tests/qtest/bios-tables-test.c for instructions on how to update expected files.
> acpi-test: Warning!  mismatch. Actual [asl:/tmp/asl-FG0E42.dsl, aml:/tmp/aml-T6YE42], Expected [asl:/tmp/asl-SE2E42.dsl, aml:tests/data/acpi/aarch64/virt/FACP.its_off].
> acpi-test: Warning!  binary file mismatch. Actual [aml:/tmp/aml-0DZE42], Expected [aml:tests/data/acpi/aarch64/virt/APIC.its_off].
> See source file tests/qtest/bios-tables-test.c for instructions on how to update expected files.
> acpi-test: Warning!  mismatch. Actual [asl:/tmp/asl-3G3E42.dsl, aml:/tmp/aml-0DZE42], Expected [asl:/tmp/asl-RV4E42.dsl, aml:tests/data/acpi/aarch64/virt/APIC.its_off].
> acpi-test: Warning!  binary file mismatch. Actual [aml:/tmp/aml-YH0E42], Expected [aml:tests/data/acpi/aarch64/virt/IORT.its_off].
> See source file tests/qtest/bios-tables-test.c for instructions on how to update expected files.
> acpi-test: Warning!  mismatch. Actual [asl:/tmp/asl-BX4E42.dsl, aml:/tmp/aml-YH0E42], Expected [asl:/tmp/asl-OL7E42.dsl, aml:tests/data/acpi/aarch64/virt/IORT.its_off].
> **
> ERROR:../../tests/qtest/bios-tables-test.c:554:test_acpi_asl: assertion failed: (all_tables_match)

But are you adding the data/$TABLE.variant (*.its_off) blobs to "ignore list"? If you add them to
tests/qtest/bios-tables-test-allowed-diff.h then the test pass (the warnings happen but not
assert is hit, as expected):

gromero@gromero0:/mnt/git/qemu_/build$ git log -1
commit c429ecebbea4633e1eaed15a16fb675629ffa03f (HEAD)
Author: Philippe Mathieu-Daudé <philmd@linaro.org>
Date:   Tue Apr 1 00:12:35 2025 +0200

     qtest/bios-tables-test: Add test for -M virt,its=off
     
     Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
gromero@gromero0:/mnt/git/qemu_$ find -name \*.its_off -ls # empty .its_off blobs
   4083505      0 -rw-rw-r--   1 gromero  gromero         0 Apr  3 23:29 ./tests/data/acpi/aarch64/virt/FACP.its_off
   4083315      0 -rw-rw-r--   1 gromero  gromero         0 Apr  3 23:29 ./tests/data/acpi/aarch64/virt/APIC.its_off
   4084089      0 -rw-rw-r--   1 gromero  gromero         0 Apr  3 23:29 ./tests/data/acpi/aarch64/virt/IORT.its_off
gromero@gromero0:/mnt/git/qemu_$ git diff # .its_off blobs added to the "ignore list"
diff --git a/tests/data/acpi/aarch64/virt/APIC.its_off b/tests/data/acpi/aarch64/virt/APIC.its_off
index c37d05d6e0..e69de29bb2 100644
Binary files a/tests/data/acpi/aarch64/virt/APIC.its_off and b/tests/data/acpi/aarch64/virt/APIC.its_off differ
diff --git a/tests/data/acpi/aarch64/virt/FACP.its_off b/tests/data/acpi/aarch64/virt/FACP.its_off
index 606dac3fe4..e69de29bb2 100644
Binary files a/tests/data/acpi/aarch64/virt/FACP.its_off and b/tests/data/acpi/aarch64/virt/FACP.its_off differ
diff --git a/tests/data/acpi/aarch64/virt/IORT.its_off b/tests/data/acpi/aarch64/virt/IORT.its_off
index 0fceb820d5..e69de29bb2 100644
Binary files a/tests/data/acpi/aarch64/virt/IORT.its_off and b/tests/data/acpi/aarch64/virt/IORT.its_off differ
diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
index dfb8523c8b..23591a55df 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/aarch64/virt/FACP.its_off", "tests/data/acpi/aarch64/virt/APIC.its_off", "tests/data/acpi/aarch64/virt/IORT.its_off",
gromero@gromero0:/mnt/git/qemu_$ cd build
gromero@gromero0:/mnt/git/qemu_/build$ make -j 32 check V=2 |& tail -10


Ok:                 320
Expected Fail:      0
Fail:               0
Unexpected Pass:    0
Skipped:            11
Timeout:            0

Full log written to /mnt/git/qemu_/build/meson-logs/testlog.txt
gromero@gromero0:/mnt/git/qemu_/build$ grep "assertion failed" /mnt/git/qemu_/build/meson-logs/testlog.txt
gromero@gromero0:/mnt/git/qemu_/build$


This is what I understand steps 1. and 2. in the prescription are saying.

That said, for you series I think your approach makes sense: you add the correct blobs
in the same patch you introduce the new test, so the test _does not_ fail with the "ignore list" empty.


Cheers,
Gustavo
  
>> But on second thoughts I think Step 1. in prescription is confusing. 
> 
> I'll try to improve that.
> 
>> Anyways,
>> what you're doing here is sensible.
>>
>> Here (1/5), you're adding a new test, with new VM options. The new VM options
>> (different in comparison to the ¨baseline" data/$TABLE) cause changes to three
>> ACPI tables: APIC, FACP, and IORT, because:
>>
>> - APIC: GICv2 is update to GICv4 due to gic-version=max + virtualization=on => GICv4
>>          and the addition of Subtable type 0xF for GIC ITS Structure (even tho its=off
>>          in the VM option, since that's the bug to be fixed down the road)
>>
>> - FACP: Change of PSCI conduit due to virtualization=on option:
>> -                       Must use HVC for PSCI : 1
>> +                       Must use HVC for PSCI : 0 (use SMC instead)
>> because of logic in machvirt_init():
>> [...]
>>      } else if (vms->virt) { /* vms->virt is true is virtualization=on */
>>          vms->psci_conduit = QEMU_PSCI_CONDUIT_SMC;
>>      } else {
>>          vms->psci_conduit = QEMU_PSCI_CONDUIT_HVC;
>>      }
>>
>> - IORT: A new node is added for SMMUv3 due to option iommu=smmuv3.
>>
>>
>> I think it's correct to add the blobs for .its_off to this patch, otherwise the test will
>> fail and, moreover, they are the results of the options being used for the new test.
>> As an alternative, you could add the *.its_off to tests/qtest/bios- tables-test-allowed-diff.h
>> so the differences would be ignored and the test passes, but really I think it makes
>> more sense as you're doing here.
>>
>> The next patch (2/5) becomes, as you said in the commit message, a preparation for the
>> real changes (the fix), which will break temporarily the test, hence in 2/5 you add it
>> to the "ignore list", which is actually what Step 2. in the prescription recommends.
>>
>> Thus:
>>
>> Reviewed-by: Gustavo Romero <gustavo.romero@linaro.org>
> 
> Thanks!
>