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