On 6/17/25 6:01 PM, Gustavo Romero wrote:
> 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é <phi...@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.
>
> But I just dove into it because you suggested to use "RID" (aka ReqID,
> aka
> Requestor ID, ah, I "love" these variations in specs), so I thought, well
> RIDs are related to RC and StreamIDs related to SMMU, so, actually,
> you meant
> "while the ID mappings from" instead of "while the RID mappings"?
Yes I meant "while the ID mappings from". sorry for the misunderstanding.
Eric
>
>
> Cheers,
> Gustavo
>
>>>
>>>>> 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.rom...@linaro.org>
>>>>> Signed-off-by: Philippe Mathieu-Daudé <phi...@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.5ed03...@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.
>>
>> 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
>>>>
>>>
>>
>
