| Message ID | 20250616131824.425315-6-gustavo.romero@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | hw/arm: GIC 'its=off' ACPI table fixes | expand |
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 ..." > 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 > @@ -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. > + .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
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 >
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. 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 >> >
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. 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"? 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.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. > > 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 >>> >> >
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 >>> >> >
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é <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. > > 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.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. >> >> 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 >>>> >>> >> >
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 @@ -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", + .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);