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 12:51, Eric Auger wrote:
> 
> 
> On 6/17/25 5:12 PM, Gustavo Romero wrote:
>> 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?
> I think I won't bother and would simply talk about "ID mappings" which
> is the generic term used in the IORT spec.
>>
>>
>>>> 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! :)
> 
> Actually I checked your patch and effectively it does not produce any
> checkpatch error related to bios-tables-test rules so your patch is OK
> (yesterday I discovered with the ACPI PCI HP series that checkpatch
> points out infractions to bios-tables-test.c rules!). Since it results
> in less patches I think it is better. May be worth to clarify that
> directly in bios-tables-test.c though.

oh I didn't know it! wow, glad Option B passes the checkpatch scrutinity heh

Yes I think I can update the doc now I confirmed with Igor the details.

I'll cc Igor and you when submitting the doc improvement.

Thanks.


Cheers,
Gustavo

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