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.
>
>
>>> 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
>>
>
