Re: [PATCH v4 5/8] qtest/bios-tables-test: Add test for when ITS is off on aarch64

[Gustavo Romero <gustavo.romero@linaro.org>]

Hi Eric,

On 6/17/25 10:34, Eric Auger wrote:
> Hi Gustavo,
> 
> On 6/16/25 3:18 PM, Gustavo Romero wrote:
>> From: Philippe Mathieu-Daudé <philmd@linaro.org>
>>
>> Arm64 GIC ITS (Interrupt Translation Service) is an optional piece of
>> hardware introduced in GICv3 and, being optional, it can be disabled
>> in QEMU aarch64 VMs that support it using machine option "its=off",
>> like, for instance: "-M virt,its=off".
>>
>> In ACPI, the ITS is advertised, if present, in the MADT (aka APIC)
>> table and the remappings from the Root Complex (RC) and from the SMMU
> I would rephrase "and the remappings" by "while the RID mappings from ..."

hmm true. Do you think it would be even better to say something like:

"while the RID and StreamID mappings from the RC and from the SMMU nodes
to the ITS Group nodes are described in the IORT table."?

I'm saying that because I understand the map from RC to ITS is from
a RID to a DeviceID, while map from the SMMU to ITS is from a StreamID to
a DeviceID, hence say "while the RID and StreamID". Does it make sense?


>> nodes to the ITS Group nodes are described in the IORT table.
>>
>> This new test verifies that when the "its=off" option is passed to the
>> machine the ITS-related data is correctly pruned from the ACPI tables.
>>
>> The new blobs for this test will be added in a following commit.
>>
>> Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   tests/qtest/bios-tables-test-allowed-diff.h |  2 ++
>>   tests/qtest/bios-tables-test.c              | 21 +++++++++++++++++++++
>>   2 files changed, 23 insertions(+)
>>
>> diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
>> index dfb8523c8b..a88198d5c2 100644
>> --- a/tests/qtest/bios-tables-test-allowed-diff.h
>> +++ b/tests/qtest/bios-tables-test-allowed-diff.h
> I still fail to understand whether empty tables + update if the
> 
> bios-tables-test-allowed-diff.h need to be done prior to adding the new test.
> 
>   * How to add or update the tests or commit changes that affect ACPI tables:
>   * Contributor:
>   * 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

I think the best reference I have to it is the reply from Igor to me here:

https://lore.kernel.org/qemu-devel/20250506173640.5ed03a16@imammedo.users.ipa.redhat.com/

I understand there are two possibilities when adding a new test:

1) Like in the steps above, 1., 2., and 3., which are taken from the bios-tables-test.c.

That gives option A:

A Patch 1: New empty files uuder tests/data/acpi + list of them in tests/qtest/bios-tables-test-allowed-diff.h
A Patch 2: New test (since the blobs are wrong but we added them in Patch 1 to allow list, there is no fail in test
A Patch 3: Update blobs (actually you are adding the real blobs, or updating from empty to real one)

or (what I'm doing here), option B:

B Patch 1: (A Patch 1) + (A Patch 2)
B Patch 2: Like (A Patch 3), i.e., just update the blobs (add the real ones)

This is the sequence Igor confirmed it's ok:

> - Patch 1     : Add the new test, add the empty blobs *.suffix files, whitelist such a blobs
> - Patch 2     : Update the blobs in Patch 1 with the ones that make the new test pass and remove them from the whitelist

Also, Igor says it's ok to add to the allow list the blobs that change at the same time
we add test that changes the very same blobs even when updating an existing test (not adding a
new one, which is slight variation):

> - Patch 3     : Add the APIC.suffix blob to the whitelist (the table that changes due to the fix)
> - Patch 4 - n : Fix(es)

"3 is not binary so it can be folded into 4 or be a separate patch (either way works for me)"
  
The important thingy is to follow the rules:

1) Don't make a commit which fails the tests
2) Don't fold a blob with the commit that changes the blob

That's my current understanding about it.

Let me know if that makes sense to you. We need to reach a consensus on this, confusing as
these acrobatics may be! :)


Cheers,
Gustavo

>> @@ -1 +1,3 @@
>>   /* List of comma-separated changed AML files to ignore */
>> +"tests/data/acpi/aarch64/virt/APIC.its_off",
>> +"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 0b2bdf9d0d..4201ec1131 100644
>> --- a/tests/qtest/bios-tables-test.c
>> +++ b/tests/qtest/bios-tables-test.c
>> @@ -2146,6 +2146,25 @@ 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",
> you have a checkpatch error here.

ouch, thanks, will fix in v5.


Cheers,
Gustavo

>> +        .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 gic-version=3,iommu=smmuv3,its=off", &data);
>> +    free_test_data(&data);
>> +}
>> +
>>   static void test_acpi_q35_viot(void)
>>   {
>>       test_data data = {
>> @@ -2577,6 +2596,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);
> Thanks
> 
> Eric
>