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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

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