diff mbox series

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

Message ID 20250331221239.87150-2-philmd@linaro.org
State New
Headers show
Series hw/arm/virt-acpi: Do not advertise disabled GIC ITS in MADT table | expand

Commit Message

Philippe Mathieu-Daudé March 31, 2025, 10:12 p.m. UTC 
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

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

Comments

Gustavo Romero April 2, 2025, 6:41 a.m. UTC | #1 
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
Philippe Mathieu-Daudé April 2, 2025, 10:30 a.m. UTC | #2 
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
Gustavo Romero April 3, 2025, 6:25 a.m. UTC | #3 
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
>
Philippe Mathieu-Daudé April 3, 2025, 12:47 p.m. UTC | #4 
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!
diff mbox series

Patch

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