* [PATCH-for-10.1 v4 0/8] hw/arm: GIC 'its=off' ACPI table fixes
@ 2025-06-16 13:18 Gustavo Romero
2025-06-16 13:18 ` [PATCH v4 1/8] hw/intc/gicv3_its: Do not check its_class_name() Gustavo Romero
` (8 more replies)
0 siblings, 9 replies; 26+ messages in thread
From: Gustavo Romero @ 2025-06-16 13:18 UTC (permalink / raw)
To: qemu-devel, eric.auger, philmd, mst
Cc: qemu-arm, alex.bennee, gustavo.romero, udo, ajones, peter.maydell,
imammedo, anisinha
Since v2:
- Fixed no_tcg_its inverted logic (rth)
Since v3:
- Fixed remappings in the IORT table when ITS is no present
- Rebased on master and resoled conflics, like no more "no_its"
flag in VirtMachineClass
- Dropped patch 1/9 because we actually want the instance flags,
not only the class flags, and the instance flags are the ones
to be used often when deciding about the presence/absence of a
machine feature, instead of the negated class flags ("no_*")
- Adapted the other patches that depended on 1/9
- Dropped patch 4/9 in favor of using the instance flag for
checking if ITS is on or off
- Simplified VM options for the new "its=off" test
v1: https://lists.gnu.org/archive/html/qemu-devel/2025-03/msg07080.html
v2: https://lists.gnu.org/archive/html/qemu-devel/2025-04/msg00495.html (Patches 6/14 -> 14/14 in the series)
v3: https://lists.gnu.org/archive/html/qemu-devel/2025-04/msg00567.html
Fix ACPI tables for '-M its=off' CLI option and resolve the issue:
https://gitlab.com/qemu-project/qemu/-/issues/2886
Cheers,
Gustavo
Gustavo Romero (7):
hw/intc/gicv3_its: Do not check its_class_name()
hw/arm/virt: Simplify logic for setting instance's 'tcg_its' variable
hw/arm/virt: Simplify create_its()
hw/arm/virt-acpi-build: Fix comment in build_iort
qtest/bios-tables-test: Add blobs for its=off test on aarch64
hw/arm/virt-acpi-build: Fix ACPI IORT and MADT tables when its=off
qtest/bios-tables-test: Update blobs for its=off test on aarch64
Philippe Mathieu-Daudé (1):
qtest/bios-tables-test: Add test for when ITS is off on aarch64
hw/arm/virt-acpi-build.c | 134 +++++++++++++---------
hw/arm/virt.c | 25 ++--
include/hw/intc/arm_gicv3_its_common.h | 2 +-
tests/data/acpi/aarch64/virt/APIC.its_off | Bin 0 -> 164 bytes
tests/data/acpi/aarch64/virt/IORT.its_off | Bin 0 -> 172 bytes
tests/qtest/bios-tables-test.c | 21 ++++
6 files changed, 113 insertions(+), 69 deletions(-)
create mode 100644 tests/data/acpi/aarch64/virt/APIC.its_off
create mode 100644 tests/data/acpi/aarch64/virt/IORT.its_off
--
2.34.1
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v4 1/8] hw/intc/gicv3_its: Do not check its_class_name()
2025-06-16 13:18 [PATCH-for-10.1 v4 0/8] hw/arm: GIC 'its=off' ACPI table fixes Gustavo Romero
@ 2025-06-16 13:18 ` Gustavo Romero
2025-06-16 13:33 ` Philippe Mathieu-Daudé
2025-06-16 13:18 ` [PATCH v4 2/8] hw/arm/virt: Simplify logic for setting instance's 'tcg_its' variable Gustavo Romero
` (7 subsequent siblings)
8 siblings, 1 reply; 26+ messages in thread
From: Gustavo Romero @ 2025-06-16 13:18 UTC (permalink / raw)
To: qemu-devel, eric.auger, philmd, mst
Cc: qemu-arm, alex.bennee, gustavo.romero, udo, ajones, peter.maydell,
imammedo, anisinha, Richard Henderson
Since commit cc5e719e2c8 ("kvm: require KVM_CAP_SIGNAL_MSI"), the single
implementation of its_class_name() no longer returns NULL (it now always
returns a valid char pointer). Hence, update the prototype docstring and
remove the tautological checks that use the its_class_name() returned
value.
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
---
hw/arm/virt-acpi-build.c | 32 +++++++++++---------------
include/hw/intc/arm_gicv3_its_common.h | 2 +-
2 files changed, 15 insertions(+), 19 deletions(-)
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 7e8e0f0298..9eee284c80 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -737,20 +737,18 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
memmap[VIRT_HIGH_GIC_REDIST2].size);
}
- if (its_class_name()) {
- /*
- * ACPI spec, Revision 6.0 Errata A
- * (original 6.0 definition has invalid Length)
- * 5.2.12.18 GIC ITS Structure
- */
- build_append_int_noprefix(table_data, 0xF, 1); /* Type */
- build_append_int_noprefix(table_data, 20, 1); /* Length */
- build_append_int_noprefix(table_data, 0, 2); /* Reserved */
- build_append_int_noprefix(table_data, 0, 4); /* GIC ITS ID */
- /* Physical Base Address */
- build_append_int_noprefix(table_data, memmap[VIRT_GIC_ITS].base, 8);
- build_append_int_noprefix(table_data, 0, 4); /* Reserved */
- }
+ /*
+ * ACPI spec, Revision 6.0 Errata A
+ * (original 6.0 definition has invalid Length)
+ * 5.2.12.18 GIC ITS Structure
+ */
+ build_append_int_noprefix(table_data, 0xF, 1); /* Type */
+ build_append_int_noprefix(table_data, 20, 1); /* Length */
+ build_append_int_noprefix(table_data, 0, 2); /* Reserved */
+ build_append_int_noprefix(table_data, 0, 4); /* GIC ITS ID */
+ /* Physical Base Address */
+ build_append_int_noprefix(table_data, memmap[VIRT_GIC_ITS].base, 8);
+ build_append_int_noprefix(table_data, 0, 4); /* Reserved */
} else {
const uint16_t spi_base = vms->irqmap[VIRT_GIC_V2M] + ARM_SPI_BASE;
@@ -969,10 +967,8 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
vms->oem_table_id);
}
- if (its_class_name()) {
- acpi_add_table(table_offsets, tables_blob);
- build_iort(tables_blob, tables->linker, vms);
- }
+ acpi_add_table(table_offsets, tables_blob);
+ build_iort(tables_blob, tables->linker, vms);
#ifdef CONFIG_TPM
if (tpm_get_version(tpm_find()) == TPM_VERSION_2_0) {
diff --git a/include/hw/intc/arm_gicv3_its_common.h b/include/hw/intc/arm_gicv3_its_common.h
index 7dc712b38d..3c7b543b01 100644
--- a/include/hw/intc/arm_gicv3_its_common.h
+++ b/include/hw/intc/arm_gicv3_its_common.h
@@ -128,7 +128,7 @@ struct GICv3ITSCommonClass {
* Return the ITS class name to use depending on whether KVM acceleration
* and KVM CAP_SIGNAL_MSI are supported
*
- * Returns: class name to use or NULL
+ * Returns: class name to use
*/
const char *its_class_name(void);
--
2.34.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v4 2/8] hw/arm/virt: Simplify logic for setting instance's 'tcg_its' variable
2025-06-16 13:18 [PATCH-for-10.1 v4 0/8] hw/arm: GIC 'its=off' ACPI table fixes Gustavo Romero
2025-06-16 13:18 ` [PATCH v4 1/8] hw/intc/gicv3_its: Do not check its_class_name() Gustavo Romero
@ 2025-06-16 13:18 ` Gustavo Romero
2025-06-17 9:46 ` Eric Auger
2025-06-16 13:18 ` [PATCH v4 3/8] hw/arm/virt: Simplify create_its() Gustavo Romero
` (6 subsequent siblings)
8 siblings, 1 reply; 26+ messages in thread
From: Gustavo Romero @ 2025-06-16 13:18 UTC (permalink / raw)
To: qemu-devel, eric.auger, philmd, mst
Cc: qemu-arm, alex.bennee, gustavo.romero, udo, ajones, peter.maydell,
imammedo, anisinha
Because 'tcg_its' in the machine instance is set based on the machine
class’s negated variable 'no_tcg_its', 'tcg_its' is the opposite of
'no_tcg_its' and hence the code in question can be simplified as:
tcg_its = !no_tcg_its.
Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
---
hw/arm/virt.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 9a6cd085a3..42a63a523e 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -3337,12 +3337,8 @@ static void virt_instance_init(Object *obj)
/* Default allows ITS instantiation */
vms->its = true;
-
- if (vmc->no_tcg_its) {
- vms->tcg_its = false;
- } else {
- vms->tcg_its = true;
- }
+ /* Allow ITS emulation if the machine version supports it. */
+ vms->tcg_its = !vmc->no_tcg_its;
/* Default disallows iommu instantiation */
vms->iommu = VIRT_IOMMU_NONE;
--
2.34.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v4 3/8] hw/arm/virt: Simplify create_its()
2025-06-16 13:18 [PATCH-for-10.1 v4 0/8] hw/arm: GIC 'its=off' ACPI table fixes Gustavo Romero
2025-06-16 13:18 ` [PATCH v4 1/8] hw/intc/gicv3_its: Do not check its_class_name() Gustavo Romero
2025-06-16 13:18 ` [PATCH v4 2/8] hw/arm/virt: Simplify logic for setting instance's 'tcg_its' variable Gustavo Romero
@ 2025-06-16 13:18 ` Gustavo Romero
2025-06-16 13:18 ` [PATCH v4 4/8] hw/arm/virt-acpi-build: Fix comment in build_iort Gustavo Romero
` (5 subsequent siblings)
8 siblings, 0 replies; 26+ messages in thread
From: Gustavo Romero @ 2025-06-16 13:18 UTC (permalink / raw)
To: qemu-devel, eric.auger, philmd, mst
Cc: qemu-arm, alex.bennee, gustavo.romero, udo, ajones, peter.maydell,
imammedo, anisinha, Richard Henderson
No need to strstr() check the class name when we can use
kvm_irqchip_in_kernel() to check if the ITS from the host can be used.
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Reviewed-by: Gustavo Romero <gustavo.romero@linaro.org>
---
hw/arm/virt.c | 17 +++++++----------
1 file changed, 7 insertions(+), 10 deletions(-)
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 42a63a523e..d2cd79ca50 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -705,21 +705,18 @@ static inline DeviceState *create_acpi_ged(VirtMachineState *vms)
static void create_its(VirtMachineState *vms)
{
- const char *itsclass = its_class_name();
DeviceState *dev;
- if (!strcmp(itsclass, "arm-gicv3-its")) {
- if (!vms->tcg_its) {
- itsclass = NULL;
- }
- }
-
- if (!itsclass) {
- /* Do nothing if not supported */
+ assert(vms->its);
+ if (!kvm_irqchip_in_kernel() && !vms->tcg_its) {
+ /*
+ * Do nothing if ITS is neither supported by the host nor emulated by
+ * the machine.
+ */
return;
}
- dev = qdev_new(itsclass);
+ dev = qdev_new(its_class_name());
object_property_set_link(OBJECT(dev), "parent-gicv3", OBJECT(vms->gic),
&error_abort);
--
2.34.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v4 4/8] hw/arm/virt-acpi-build: Fix comment in build_iort
2025-06-16 13:18 [PATCH-for-10.1 v4 0/8] hw/arm: GIC 'its=off' ACPI table fixes Gustavo Romero
` (2 preceding siblings ...)
2025-06-16 13:18 ` [PATCH v4 3/8] hw/arm/virt: Simplify create_its() Gustavo Romero
@ 2025-06-16 13:18 ` Gustavo Romero
2025-06-17 13:22 ` Eric Auger
2025-06-16 13:18 ` [PATCH v4 5/8] qtest/bios-tables-test: Add test for when ITS is off on aarch64 Gustavo Romero
` (4 subsequent siblings)
8 siblings, 1 reply; 26+ messages in thread
From: Gustavo Romero @ 2025-06-16 13:18 UTC (permalink / raw)
To: qemu-devel, eric.auger, philmd, mst
Cc: qemu-arm, alex.bennee, gustavo.romero, udo, ajones, peter.maydell,
imammedo, anisinha
The comment about the mapping from SMMU to ITS is incorrect and it reads
"RC -> ITS". The code in question actually maps SMMU -> ITS, so the
mapping in question is not direct. The direct mapping, i.e., RC -> ITS,
is handled a bit further down in the code, in the else block, and we
take the opportunity to update that as well, adding "RC -> ITS" to the
text so it's easier to see it's the direct map to the ITS Group node.
Signed-off-by Gustavo Romero <gustavo.romero@linaro.org>
---
hw/arm/virt-acpi-build.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 9eee284c80..6990bce3bb 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -407,23 +407,27 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
if (vms->iommu == VIRT_IOMMU_SMMUV3) {
AcpiIortIdMapping *range;
- /* translated RIDs connect to SMMUv3 node: RC -> SMMUv3 -> ITS */
+ /* Map RIDs (input) from RC to SMMUv3 nodes: RC -> SMMUv3. */
for (i = 0; i < smmu_idmaps->len; i++) {
range = &g_array_index(smmu_idmaps, AcpiIortIdMapping, i);
- /* output IORT node is the smmuv3 node */
+ /* Output IORT node is the SMMUv3 node. */
build_iort_id_mapping(table_data, range->input_base,
range->id_count, smmu_offset);
}
- /* bypassed RIDs connect to ITS group node directly: RC -> ITS */
+ /* Map DeviceIDs (input) from SMMUv3 to ITS Group nodes: SMMU -> ITS. */
for (i = 0; i < its_idmaps->len; i++) {
range = &g_array_index(its_idmaps, AcpiIortIdMapping, i);
- /* output IORT node is the ITS group node (the first node) */
+ /* Output IORT node is the ITS Group node (the first node). */
build_iort_id_mapping(table_data, range->input_base,
range->id_count, IORT_NODE_OFFSET);
}
} else {
- /* output IORT node is the ITS group node (the first node) */
+ /*
+ * Map bypassed RIDs (input) (don't go through the SMMU) to ITS Group
+ * nodes: RC -> ITS.
+ * Output IORT node is the ITS Group node (the first node).
+ */
build_iort_id_mapping(table_data, 0, 0x10000, IORT_NODE_OFFSET);
}
--
2.34.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v4 5/8] qtest/bios-tables-test: Add test for when ITS is off on aarch64
2025-06-16 13:18 [PATCH-for-10.1 v4 0/8] hw/arm: GIC 'its=off' ACPI table fixes Gustavo Romero
` (3 preceding siblings ...)
2025-06-16 13:18 ` [PATCH v4 4/8] hw/arm/virt-acpi-build: Fix comment in build_iort Gustavo Romero
@ 2025-06-16 13:18 ` Gustavo Romero
2025-06-17 13:34 ` Eric Auger
2025-06-16 13:18 ` [PATCH v4 6/8] qtest/bios-tables-test: Add blobs for its=off test " Gustavo Romero
` (3 subsequent siblings)
8 siblings, 1 reply; 26+ messages in thread
From: Gustavo Romero @ 2025-06-16 13:18 UTC (permalink / raw)
To: qemu-devel, eric.auger, philmd, mst
Cc: qemu-arm, alex.bennee, gustavo.romero, udo, ajones, peter.maydell,
imammedo, anisinha
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
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
@@ -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);
--
2.34.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v4 6/8] qtest/bios-tables-test: Add blobs for its=off test on aarch64
2025-06-16 13:18 [PATCH-for-10.1 v4 0/8] hw/arm: GIC 'its=off' ACPI table fixes Gustavo Romero
` (4 preceding siblings ...)
2025-06-16 13:18 ` [PATCH v4 5/8] qtest/bios-tables-test: Add test for when ITS is off on aarch64 Gustavo Romero
@ 2025-06-16 13:18 ` Gustavo Romero
2025-06-16 13:18 ` [PATCH v4 7/8] hw/arm/virt-acpi-build: Fix ACPI IORT and MADT tables when its=off Gustavo Romero
` (2 subsequent siblings)
8 siblings, 0 replies; 26+ messages in thread
From: Gustavo Romero @ 2025-06-16 13:18 UTC (permalink / raw)
To: qemu-devel, eric.auger, philmd, mst
Cc: qemu-arm, alex.bennee, gustavo.romero, udo, ajones, peter.maydell,
imammedo, anisinha
Add blobs for test_acpi_aarch64_virt_tcg_its_off(), which introduces a
new variant, .its_off, that requires variations of the MADT and IORT
tables.
MADT (aka APIC) diff:
+[000h 0000 4] Signature : "APIC" [Multiple APIC Description Table (MADT)]
+[004h 0004 4] Table Length : 000000B8
+[008h 0008 1] Revision : 04
+[009h 0009 1] Checksum : C1
+[00Ah 0010 6] Oem ID : "BOCHS "
+[010h 0016 8] Oem Table ID : "BXPC "
+[018h 0024 4] Oem Revision : 00000001
+[01Ch 0028 4] Asl Compiler ID : "BXPC"
+[020h 0032 4] Asl Compiler Revision : 00000001
+
+[024h 0036 4] Local Apic Address : 00000000
+[028h 0040 4] Flags (decoded below) : 00000000
+ PC-AT Compatibility : 0
+
+[02Ch 0044 1] Subtable Type : 0C [Generic Interrupt Distributor]
+[02Dh 0045 1] Length : 18
+[02Eh 0046 2] Reserved : 0000
+[030h 0048 4] Local GIC Hardware ID : 00000000
+[034h 0052 8] Base Address : 0000000008000000
+[03Ch 0060 4] Interrupt Base : 00000000
+[040h 0064 1] Version : 03
+[041h 0065 3] Reserved : 000000
+
+[044h 0068 1] Subtable Type : 0B [Generic Interrupt Controller]
+[045h 0069 1] Length : 50
+[046h 0070 2] Reserved : 0000
+[048h 0072 4] CPU Interface Number : 00000000
+[04Ch 0076 4] Processor UID : 00000000
+[050h 0080 4] Flags (decoded below) : 00000001
+ Processor Enabled : 1
+ Performance Interrupt Trigger Mode : 0
+ Virtual GIC Interrupt Trigger Mode : 0
+[054h 0084 4] Parking Protocol Version : 00000000
+[058h 0088 4] Performance Interrupt : 00000017
+[05Ch 0092 8] Parked Address : 0000000000000000
+[064h 0100 8] Base Address : 0000000000000000
+[06Ch 0108 8] Virtual GIC Base Address : 0000000000000000
+[074h 0116 8] Hypervisor GIC Base Address : 0000000000000000
+[07Ch 0124 4] Virtual GIC Interrupt : 00000000
+[080h 0128 8] Redistributor Base Address : 0000000000000000
+[088h 0136 8] ARM MPIDR : 0000000000000000
+[090h 0144 1] Efficiency Class : 00
+[091h 0145 1] Reserved : 00
+[092h 0146 2] SPE Overflow Interrupt : 0000
+
+[094h 0148 1] Subtable Type : 0E [Generic Interrupt Redistributor]
+[095h 0149 1] Length : 10
+[096h 0150 2] Reserved : 0000
+[098h 0152 8] Base Address : 00000000080A0000
+[0A0h 0160 4] Length : 00F60000
+
+[0A4h 0164 1] Subtable Type : 0F [Generic Interrupt Translator]
+[0A5h 0165 1] Length : 14
+[0A6h 0166 2] Reserved : 0000
+[0A8h 0168 4] Translation ID : 00000000
+[0ACh 0172 8] Base Address : 0000000008080000
+[0B4h 0180 4] Reserved : 00000000
IORT diff:
+[000h 0000 4] Signature : "IORT" [IO Remapping Table]
+[004h 0004 4] Table Length : 000000EC
+[008h 0008 1] Revision : 03
+[009h 0009 1] Checksum : 57
+[00Ah 0010 6] Oem ID : "BOCHS "
+[010h 0016 8] Oem Table ID : "BXPC "
+[018h 0024 4] Oem Revision : 00000001
+[01Ch 0028 4] Asl Compiler ID : "BXPC"
+[020h 0032 4] Asl Compiler Revision : 00000001
+
+[024h 0036 4] Node Count : 00000003
+[028h 0040 4] Node Offset : 00000030
+[02Ch 0044 4] Reserved : 00000000
+
+[030h 0048 1] Type : 00
+[031h 0049 2] Length : 0018
+[033h 0051 1] Revision : 01
+[034h 0052 4] Reserved : 00000000
+[038h 0056 4] Mapping Count : 00000000
+[03Ch 0060 4] Mapping Offset : 00000000
+
+[040h 0064 4] ItsCount : 00000001
+[044h 0068 4] Identifiers : 00000000
+
+[048h 0072 1] Type : 04
+[049h 0073 2] Length : 0058
+[04Bh 0075 1] Revision : 04
+[04Ch 0076 4] Reserved : 00000001
+[050h 0080 4] Mapping Count : 00000001
+[054h 0084 4] Mapping Offset : 00000044
+
+[058h 0088 8] Base Address : 0000000009050000
+[060h 0096 4] Flags (decoded below) : 00000001
+ COHACC Override : 1
+ HTTU Override : 0
+ Proximity Domain Valid : 0
+[064h 0100 4] Reserved : 00000000
+[068h 0104 8] VATOS Address : 0000000000000000
+[070h 0112 4] Model : 00000000
+[074h 0116 4] Event GSIV : 0000006A
+[078h 0120 4] PRI GSIV : 0000006B
+[07Ch 0124 4] GERR GSIV : 0000006D
+[080h 0128 4] Sync GSIV : 0000006C
+[084h 0132 4] Proximity Domain : 00000000
+[088h 0136 4] Device ID Mapping Index : 00000000
+
+[08Ch 0140 4] Input base : 00000000
+[090h 0144 4] ID Count : 0000FFFF
+[094h 0148 4] Output Base : 00000000
+[098h 0152 4] Output Reference : 00000030
+[09Ch 0156 4] Flags (decoded below) : 00000000
+ Single Mapping : 0
+
+[0A0h 0160 1] Type : 02
+[0A1h 0161 2] Length : 004C
+[0A3h 0163 1] Revision : 03
+[0A4h 0164 4] Reserved : 00000002
+[0A8h 0168 4] Mapping Count : 00000002
+[0ACh 0172 4] Mapping Offset : 00000024
+
+[0B0h 0176 8] Memory Properties : [IORT Memory Access Properties]
+[0B0h 0176 4] Cache Coherency : 00000001
+[0B4h 0180 1] Hints (decoded below) : 00
+ Transient : 0
+ Write Allocate : 0
+ Read Allocate : 0
+ Override : 0
+[0B5h 0181 2] Reserved : 0000
+[0B7h 0183 1] Memory Flags (decoded below) : 03
+ Coherency : 1
+ Device Attribute : 1
+[0B8h 0184 4] ATS Attribute : 00000000
+[0BCh 0188 4] PCI Segment Number : 00000000
+[0C0h 0192 1] Memory Size Limit : 40
+[0C1h 0193 3] Reserved : 000000
+
+[0C4h 0196 4] Input base : 00000000
+[0C8h 0200 4] ID Count : 000000FF
+[0CCh 0204 4] Output Base : 00000000
+[0D0h 0208 4] Output Reference : 00000048
+[0D4h 0212 4] Flags (decoded below) : 00000000
+ Single Mapping : 0
+
+[0D8h 0216 4] Input base : 00000100
+[0DCh 0220 4] ID Count : 0000FEFF
+[0E0h 0224 4] Output Base : 00000100
+[0E4h 0228 4] Output Reference : 00000030
+[0E8h 0232 4] Flags (decoded below) : 00000000
+ Single Mapping : 0
Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
---
tests/data/acpi/aarch64/virt/APIC.its_off | Bin 0 -> 184 bytes
tests/data/acpi/aarch64/virt/IORT.its_off | Bin 0 -> 236 bytes
tests/qtest/bios-tables-test-allowed-diff.h | 2 --
3 files changed, 2 deletions(-)
create mode 100644 tests/data/acpi/aarch64/virt/APIC.its_off
create mode 100644 tests/data/acpi/aarch64/virt/IORT.its_off
diff --git a/tests/data/acpi/aarch64/virt/APIC.its_off b/tests/data/acpi/aarch64/virt/APIC.its_off
new file mode 100644
index 0000000000000000000000000000000000000000..37d82e970b1331cb5b259f0bd2d3654bacb2d623
GIT binary patch
literal 184
zcmZ<^@O0k6z`($A(8=G~BUr&HBEVSz2pEB4AU24G0Uik$i-7~iVg@p}17JJ`2AFzr
Zgb>LrJ^_#xE~p*f82CkCMsUFG1ppOZ2>}2A
literal 0
HcmV?d00001
diff --git a/tests/data/acpi/aarch64/virt/IORT.its_off b/tests/data/acpi/aarch64/virt/IORT.its_off
new file mode 100644
index 0000000000000000000000000000000000000000..0fceb820d509e852ca0849baf568a8e93e426738
GIT binary patch
literal 236
zcmebD4+?q1z`(#9?&R<65v<@85#X!<1dKp25F11@1F-=RgMkDCNC*yK9F_<M77!bR
zUBI%eoFED&4;F$FSwK1)h;xBB2Py`m{{M%tVD>TjFfcO#g+N#Zh@s|zoCF3AP#UU@
R!2`+%Dg6Hr$N|zYvjDIZ5CH%H
literal 0
HcmV?d00001
diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
index a88198d5c2..dfb8523c8b 100644
--- a/tests/qtest/bios-tables-test-allowed-diff.h
+++ b/tests/qtest/bios-tables-test-allowed-diff.h
@@ -1,3 +1 @@
/* 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",
--
2.34.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v4 7/8] hw/arm/virt-acpi-build: Fix ACPI IORT and MADT tables when its=off
2025-06-16 13:18 [PATCH-for-10.1 v4 0/8] hw/arm: GIC 'its=off' ACPI table fixes Gustavo Romero
` (5 preceding siblings ...)
2025-06-16 13:18 ` [PATCH v4 6/8] qtest/bios-tables-test: Add blobs for its=off test " Gustavo Romero
@ 2025-06-16 13:18 ` Gustavo Romero
2025-06-17 14:04 ` Eric Auger
2025-06-16 13:18 ` [PATCH v4 8/8] qtest/bios-tables-test: Update blobs for its=off test on aarch64 Gustavo Romero
2025-06-17 9:35 ` [PATCH-for-10.1 v4 0/8] hw/arm: GIC 'its=off' ACPI table fixes Eric Auger
8 siblings, 1 reply; 26+ messages in thread
From: Gustavo Romero @ 2025-06-16 13:18 UTC (permalink / raw)
To: qemu-devel, eric.auger, philmd, mst
Cc: qemu-arm, alex.bennee, gustavo.romero, udo, ajones, peter.maydell,
imammedo, anisinha
Currently, the ITS Group nodes in the IORT table and the GIC ITS Struct
in the MADT table are always generated, even if GIC ITS is not available
on the machine.
This commit fixes it by not generating the ITS Group nodes, not mapping
any other node to them, and not advertising the GIC ITS in the MADT
table, when GIC ITS is not available on the machine.
Since the fix changes the MADT and IORT tables, add the blobs for the
"its=off" test to the allow list and update them in the next commit.
Reported-by: Udo Steinberg <udo@hypervisor.org>
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2886
Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
hw/arm/virt-acpi-build.c | 152 ++++++++++++--------
tests/qtest/bios-tables-test-allowed-diff.h | 2 +
2 files changed, 93 insertions(+), 61 deletions(-)
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 6990bce3bb..2240421980 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -295,32 +295,42 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
/* Sort the smmu idmap by input_base */
g_array_sort(smmu_idmaps, iort_idmap_compare);
- /*
- * Split the whole RIDs by mapping from RC to SMMU,
- * build the ID mapping from RC to ITS directly.
- */
- for (i = 0; i < smmu_idmaps->len; i++) {
- idmap = &g_array_index(smmu_idmaps, AcpiIortIdMapping, i);
+ nb_nodes = 2; /* RC and SMMUv3 */
+ rc_mapping_count = smmu_idmaps->len;
+
+ if (vms->its) {
+ /*
+ * Split the whole RIDs by mapping from RC to SMMU,
+ * build the ID mapping from RC to ITS directly.
+ */
+ for (i = 0; i < smmu_idmaps->len; i++) {
+ idmap = &g_array_index(smmu_idmaps, AcpiIortIdMapping, i);
+
+ if (next_range.input_base < idmap->input_base) {
+ next_range.id_count = idmap->input_base - next_range.input_base;
+ g_array_append_val(its_idmaps, next_range);
+ }
+
+ next_range.input_base = idmap->input_base + idmap->id_count;
+ }
- if (next_range.input_base < idmap->input_base) {
- next_range.id_count = idmap->input_base - next_range.input_base;
+ /* Append the last RC -> ITS ID mapping */
+ if (next_range.input_base < 0x10000) {
+ next_range.id_count = 0x10000 - next_range.input_base;
g_array_append_val(its_idmaps, next_range);
}
- next_range.input_base = idmap->input_base + idmap->id_count;
+ nb_nodes++; /* ITS */
+ rc_mapping_count += its_idmaps->len;
}
-
- /* Append the last RC -> ITS ID mapping */
- if (next_range.input_base < 0x10000) {
- next_range.id_count = 0x10000 - next_range.input_base;
- g_array_append_val(its_idmaps, next_range);
- }
-
- nb_nodes = 3; /* RC, ITS, SMMUv3 */
- rc_mapping_count = smmu_idmaps->len + its_idmaps->len;
} else {
- nb_nodes = 2; /* RC, ITS */
- rc_mapping_count = 1;
+ if (vms->its) {
+ nb_nodes = 2; /* RC and ITS */
+ rc_mapping_count = 1; /* Direct map to ITS */
+ } else {
+ nb_nodes = 1; /* RC only */
+ rc_mapping_count = 0; /* No output mapping */
+ }
}
/* Number of IORT Nodes */
build_append_int_noprefix(table_data, nb_nodes, 4);
@@ -329,31 +339,43 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
build_append_int_noprefix(table_data, IORT_NODE_OFFSET, 4);
build_append_int_noprefix(table_data, 0, 4); /* Reserved */
- /* Table 12 ITS Group Format */
- build_append_int_noprefix(table_data, 0 /* ITS Group */, 1); /* Type */
- node_size = 20 /* fixed header size */ + 4 /* 1 GIC ITS Identifier */;
- build_append_int_noprefix(table_data, node_size, 2); /* Length */
- build_append_int_noprefix(table_data, 1, 1); /* Revision */
- build_append_int_noprefix(table_data, id++, 4); /* Identifier */
- build_append_int_noprefix(table_data, 0, 4); /* Number of ID mappings */
- build_append_int_noprefix(table_data, 0, 4); /* Reference to ID Array */
- build_append_int_noprefix(table_data, 1, 4); /* Number of ITSs */
- /* GIC ITS Identifier Array */
- build_append_int_noprefix(table_data, 0 /* MADT translation_id */, 4);
+ if (vms->its) {
+ /* Table 12 ITS Group Format */
+ build_append_int_noprefix(table_data, 0 /* ITS Group */, 1); /* Type */
+ node_size = 20 /* fixed header size */ + 4 /* 1 GIC ITS Identifier */;
+ build_append_int_noprefix(table_data, node_size, 2); /* Length */
+ build_append_int_noprefix(table_data, 1, 1); /* Revision */
+ build_append_int_noprefix(table_data, id++, 4); /* Identifier */
+ build_append_int_noprefix(table_data, 0, 4); /* Number of ID mappings */
+ build_append_int_noprefix(table_data, 0, 4); /* Reference to ID Array */
+ build_append_int_noprefix(table_data, 1, 4); /* Number of ITSs */
+ /* GIC ITS Identifier Array */
+ build_append_int_noprefix(table_data, 0 /* MADT translation_id */, 4);
+ }
if (vms->iommu == VIRT_IOMMU_SMMUV3) {
int irq = vms->irqmap[VIRT_SMMU] + ARM_SPI_BASE;
-
+ int num_id_mappings, offset_to_id_array;
+
+ if (vms->its) {
+ num_id_mappings = 1; /* ITS Group node */
+ offset_to_id_array = SMMU_V3_ENTRY_SIZE; /* Just after the header */
+ } else {
+ num_id_mappings = 0; /* No ID mappings */
+ offset_to_id_array = 0; /* No ID mappings array */
+ }
smmu_offset = table_data->len - table.table_offset;
/* Table 9 SMMUv3 Format */
build_append_int_noprefix(table_data, 4 /* SMMUv3 */, 1); /* Type */
- node_size = SMMU_V3_ENTRY_SIZE + ID_MAPPING_ENTRY_SIZE;
+ node_size = SMMU_V3_ENTRY_SIZE +
+ (ID_MAPPING_ENTRY_SIZE * num_id_mappings);
build_append_int_noprefix(table_data, node_size, 2); /* Length */
build_append_int_noprefix(table_data, 4, 1); /* Revision */
build_append_int_noprefix(table_data, id++, 4); /* Identifier */
- build_append_int_noprefix(table_data, 1, 4); /* Number of ID mappings */
+ /* Number of ID mappings */
+ build_append_int_noprefix(table_data, num_id_mappings, 4);
/* Reference to ID Array */
- build_append_int_noprefix(table_data, SMMU_V3_ENTRY_SIZE, 4);
+ build_append_int_noprefix(table_data, offset_to_id_array, 4);
/* Base address */
build_append_int_noprefix(table_data, vms->memmap[VIRT_SMMU].base, 8);
/* Flags */
@@ -370,8 +392,10 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
/* DeviceID mapping index (ignored since interrupts are GSIV based) */
build_append_int_noprefix(table_data, 0, 4);
- /* output IORT node is the ITS group node (the first node) */
- build_iort_id_mapping(table_data, 0, 0x10000, IORT_NODE_OFFSET);
+ if (vms->its) {
+ /* Output IORT node is the ITS Group node (the first node). */
+ build_iort_id_mapping(table_data, 0, 0x10000, IORT_NODE_OFFSET);
+ }
}
/* Table 17 Root Complex Node */
@@ -415,20 +439,24 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
range->id_count, smmu_offset);
}
- /* Map DeviceIDs (input) from SMMUv3 to ITS Group nodes: SMMU -> ITS. */
- for (i = 0; i < its_idmaps->len; i++) {
- range = &g_array_index(its_idmaps, AcpiIortIdMapping, i);
- /* Output IORT node is the ITS Group node (the first node). */
- build_iort_id_mapping(table_data, range->input_base,
- range->id_count, IORT_NODE_OFFSET);
+ if (vms->its) {
+ /* Map DeviceIDs (input) from SMMUv3 to ITS Group nodes: SMMU -> ITS. */
+ for (i = 0; i < its_idmaps->len; i++) {
+ range = &g_array_index(its_idmaps, AcpiIortIdMapping, i);
+ /* Output IORT node is the ITS Group node (the first node). */
+ build_iort_id_mapping(table_data, range->input_base,
+ range->id_count, IORT_NODE_OFFSET);
+ }
}
} else {
- /*
- * Map bypassed RIDs (input) (don't go through the SMMU) to ITS Group
- * nodes: RC -> ITS.
- * Output IORT node is the ITS Group node (the first node).
- */
- build_iort_id_mapping(table_data, 0, 0x10000, IORT_NODE_OFFSET);
+ if (vms->its) {
+ /*
+ * Map bypassed RIDs (input) (don't go through the SMMU) to ITS Group
+ * nodes: RC -> ITS.
+ * Output IORT node is the ITS Group node (the first node).
+ */
+ build_iort_id_mapping(table_data, 0, 0x10000, IORT_NODE_OFFSET);
+ }
}
acpi_table_end(linker, &table);
@@ -741,18 +769,20 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
memmap[VIRT_HIGH_GIC_REDIST2].size);
}
- /*
- * ACPI spec, Revision 6.0 Errata A
- * (original 6.0 definition has invalid Length)
- * 5.2.12.18 GIC ITS Structure
- */
- build_append_int_noprefix(table_data, 0xF, 1); /* Type */
- build_append_int_noprefix(table_data, 20, 1); /* Length */
- build_append_int_noprefix(table_data, 0, 2); /* Reserved */
- build_append_int_noprefix(table_data, 0, 4); /* GIC ITS ID */
- /* Physical Base Address */
- build_append_int_noprefix(table_data, memmap[VIRT_GIC_ITS].base, 8);
- build_append_int_noprefix(table_data, 0, 4); /* Reserved */
+ if (vms->its) {
+ /*
+ * ACPI spec, Revision 6.0 Errata A
+ * (original 6.0 definition has invalid Length)
+ * 5.2.12.18 GIC ITS Structure
+ */
+ build_append_int_noprefix(table_data, 0xF, 1); /* Type */
+ build_append_int_noprefix(table_data, 20, 1); /* Length */
+ build_append_int_noprefix(table_data, 0, 2); /* Reserved */
+ build_append_int_noprefix(table_data, 0, 4); /* GIC ITS ID */
+ /* Physical Base Address */
+ build_append_int_noprefix(table_data, memmap[VIRT_GIC_ITS].base, 8);
+ build_append_int_noprefix(table_data, 0, 4); /* Reserved */
+ }
} else {
const uint16_t spi_base = vms->irqmap[VIRT_GIC_V2M] + ARM_SPI_BASE;
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",
--
2.34.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v4 8/8] qtest/bios-tables-test: Update blobs for its=off test on aarch64
2025-06-16 13:18 [PATCH-for-10.1 v4 0/8] hw/arm: GIC 'its=off' ACPI table fixes Gustavo Romero
` (6 preceding siblings ...)
2025-06-16 13:18 ` [PATCH v4 7/8] hw/arm/virt-acpi-build: Fix ACPI IORT and MADT tables when its=off Gustavo Romero
@ 2025-06-16 13:18 ` Gustavo Romero
2025-06-17 9:35 ` [PATCH-for-10.1 v4 0/8] hw/arm: GIC 'its=off' ACPI table fixes Eric Auger
8 siblings, 0 replies; 26+ messages in thread
From: Gustavo Romero @ 2025-06-16 13:18 UTC (permalink / raw)
To: qemu-devel, eric.auger, philmd, mst
Cc: qemu-arm, alex.bennee, gustavo.romero, udo, ajones, peter.maydell,
imammedo, anisinha
Update blobs for the its=off test on aarch64 after fix.
Basically, all structs related to ITS are gone in MADT and IORT
tables after the fix (previously ITS was not properly disabled
when "its=off" option was passed to the machine).
MADT diff:
[000h 0000 4] Signature : "APIC" [Multiple APIC Description Table (MADT)]
-[004h 0004 4] Table Length : 000000B8
+[004h 0004 4] Table Length : 000000A4
[008h 0008 1] Revision : 04
-[009h 0009 1] Checksum : C1
+[009h 0009 1] Checksum : 08
[00Ah 0010 6] Oem ID : "BOCHS "
[010h 0016 8] Oem Table ID : "BXPC "
[018h 0024 4] Oem Revision : 00000001
[01Ch 0028 4] Asl Compiler ID : "BXPC"
[020h 0032 4] Asl Compiler Revision : 00000001
[024h 0036 4] Local Apic Address : 00000000
[028h 0040 4] Flags (decoded below) : 00000000
PC-AT Compatibility : 0
[02Ch 0044 1] Subtable Type : 0C [Generic Interrupt Distributor]
[02Dh 0045 1] Length : 18
[02Eh 0046 2] Reserved : 0000
[030h 0048 4] Local GIC Hardware ID : 00000000
[034h 0052 8] Base Address : 0000000008000000
[03Ch 0060 4] Interrupt Base : 00000000
@@ -48,37 +48,29 @@
[064h 0100 8] Base Address : 0000000000000000
[06Ch 0108 8] Virtual GIC Base Address : 0000000000000000
[074h 0116 8] Hypervisor GIC Base Address : 0000000000000000
[07Ch 0124 4] Virtual GIC Interrupt : 00000000
[080h 0128 8] Redistributor Base Address : 0000000000000000
[088h 0136 8] ARM MPIDR : 0000000000000000
[090h 0144 1] Efficiency Class : 00
[091h 0145 1] Reserved : 00
[092h 0146 2] SPE Overflow Interrupt : 0000
[094h 0148 1] Subtable Type : 0E [Generic Interrupt Redistributor]
[095h 0149 1] Length : 10
[096h 0150 2] Reserved : 0000
[098h 0152 8] Base Address : 00000000080A0000
[0A0h 0160 4] Length : 00F60000
-[0A4h 0164 1] Subtable Type : 0F [Generic Interrupt Translator]
-[0A5h 0165 1] Length : 14
-[0A6h 0166 2] Reserved : 0000
-[0A8h 0168 4] Translation ID : 00000000
-[0ACh 0172 8] Base Address : 0000000008080000
-[0B4h 0180 4] Reserved : 00000000
IORT diff:
[000h 0000 4] Signature : "IORT" [IO Remapping Table]
-[004h 0004 4] Table Length : 000000EC
+[004h 0004 4] Table Length : 000000AC
[008h 0008 1] Revision : 03
-[009h 0009 1] Checksum : 57
+[009h 0009 1] Checksum : 97
[00Ah 0010 6] Oem ID : "BOCHS "
[010h 0016 8] Oem Table ID : "BXPC "
[018h 0024 4] Oem Revision : 00000001
[01Ch 0028 4] Asl Compiler ID : "BXPC"
[020h 0032 4] Asl Compiler Revision : 00000001
-[024h 0036 4] Node Count : 00000003
+[024h 0036 4] Node Count : 00000002
[028h 0040 4] Node Offset : 00000030
[02Ch 0044 4] Reserved : 00000000
-[030h 0048 1] Type : 00
-[031h 0049 2] Length : 0018
-[033h 0051 1] Revision : 01
+[030h 0048 1] Type : 04
+[031h 0049 2] Length : 0044
+[033h 0051 1] Revision : 04
[034h 0052 4] Reserved : 00000000
[038h 0056 4] Mapping Count : 00000000
[03Ch 0060 4] Mapping Offset : 00000000
-[040h 0064 4] ItsCount : 00000001
-[044h 0068 4] Identifiers : 00000000
-
-[048h 0072 1] Type : 04
-[049h 0073 2] Length : 0058
-[04Bh 0075 1] Revision : 04
-[04Ch 0076 4] Reserved : 00000001
-[050h 0080 4] Mapping Count : 00000001
-[054h 0084 4] Mapping Offset : 00000044
-
-[058h 0088 8] Base Address : 0000000009050000
-[060h 0096 4] Flags (decoded below) : 00000001
+[040h 0064 8] Base Address : 0000000009050000
+[048h 0072 4] Flags (decoded below) : 00000001
COHACC Override : 1
HTTU Override : 0
Proximity Domain Valid : 0
-[064h 0100 4] Reserved : 00000000
-[068h 0104 8] VATOS Address : 0000000000000000
-[070h 0112 4] Model : 00000000
-[074h 0116 4] Event GSIV : 0000006A
-[078h 0120 4] PRI GSIV : 0000006B
-[07Ch 0124 4] GERR GSIV : 0000006D
-[080h 0128 4] Sync GSIV : 0000006C
-[084h 0132 4] Proximity Domain : 00000000
-[088h 0136 4] Device ID Mapping Index : 00000000
-
-[08Ch 0140 4] Input base : 00000000
-[090h 0144 4] ID Count : 0000FFFF
-[094h 0148 4] Output Base : 00000000
-[098h 0152 4] Output Reference : 00000030
-[09Ch 0156 4] Flags (decoded below) : 00000000
- Single Mapping : 0
-
-[0A0h 0160 1] Type : 02
-[0A1h 0161 2] Length : 004C
-[0A3h 0163 1] Revision : 03
-[0A4h 0164 4] Reserved : 00000002
-[0A8h 0168 4] Mapping Count : 00000002
-[0ACh 0172 4] Mapping Offset : 00000024
-
-[0B0h 0176 8] Memory Properties : [IORT Memory Access Properties]
-[0B0h 0176 4] Cache Coherency : 00000001
-[0B4h 0180 1] Hints (decoded below) : 00
+[04Ch 0076 4] Reserved : 00000000
+[050h 0080 8] VATOS Address : 0000000000000000
+[058h 0088 4] Model : 00000000
+[05Ch 0092 4] Event GSIV : 0000006A
+[060h 0096 4] PRI GSIV : 0000006B
+[064h 0100 4] GERR GSIV : 0000006D
+[068h 0104 4] Sync GSIV : 0000006C
+[06Ch 0108 4] Proximity Domain : 00000000
+[070h 0112 4] Device ID Mapping Index : 00000000
+
+[074h 0116 1] Type : 02
+[075h 0117 2] Length : 0038
+[077h 0119 1] Revision : 03
+[078h 0120 4] Reserved : 00000001
+[07Ch 0124 4] Mapping Count : 00000001
+[080h 0128 4] Mapping Offset : 00000024
+
+[084h 0132 8] Memory Properties : [IORT Memory Access Properties]
+[084h 0132 4] Cache Coherency : 00000001
+[088h 0136 1] Hints (decoded below) : 00
Transient : 0
Write Allocate : 0
Read Allocate : 0
Override : 0
-[0B5h 0181 2] Reserved : 0000
-[0B7h 0183 1] Memory Flags (decoded below) : 03
+[089h 0137 2] Reserved : 0000
+[08Bh 0139 1] Memory Flags (decoded below) : 03
Coherency : 1
Device Attribute : 1
-[0B8h 0184 4] ATS Attribute : 00000000
-[0BCh 0188 4] PCI Segment Number : 00000000
-[0C0h 0192 1] Memory Size Limit : 40
-[0C1h 0193 3] Reserved : 000000
-
-[0C4h 0196 4] Input base : 00000000
-[0C8h 0200 4] ID Count : 000000FF
-[0CCh 0204 4] Output Base : 00000000
-[0D0h 0208 4] Output Reference : 00000048
-[0D4h 0212 4] Flags (decoded below) : 00000000
- Single Mapping : 0
-
-[0D8h 0216 4] Input base : 00000100
-[0DCh 0220 4] ID Count : 0000FEFF
-[0E0h 0224 4] Output Base : 00000100
-[0E4h 0228 4] Output Reference : 00000030
-[0E8h 0232 4] Flags (decoded below) : 00000000
+[08Ch 0140 4] ATS Attribute : 00000000
+[090h 0144 4] PCI Segment Number : 00000000
+[094h 0148 1] Memory Size Limit : 40
+[095h 0149 3] Reserved : 000000
+
+[098h 0152 4] Input base : 00000000
+[09Ch 0156 4] ID Count : 000000FF
+[0A0h 0160 4] Output Base : 00000000
+[0A4h 0164 4] Output Reference : 00000030
+[0A8h 0168 4] Flags (decoded below) : 00000000
Single Mapping : 0
Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
---
tests/data/acpi/aarch64/virt/APIC.its_off | Bin 184 -> 164 bytes
tests/data/acpi/aarch64/virt/IORT.its_off | Bin 236 -> 172 bytes
tests/qtest/bios-tables-test-allowed-diff.h | 2 --
3 files changed, 2 deletions(-)
diff --git a/tests/data/acpi/aarch64/virt/APIC.its_off b/tests/data/acpi/aarch64/virt/APIC.its_off
index 37d82e970b1331cb5b259f0bd2d3654bacb2d623..6130cb7d07103b326feb4dcd7034f85808bebadf 100644
GIT binary patch
delta 18
ZcmdnNxP+0*F~HM#2?GNI3&%vRSpY2+1Zw~Q
delta 39
jcmZ3&xPy_)F~HM#2Ll5G%fX3UvqbnsfJ`vp;DE6JqX7kf
diff --git a/tests/data/acpi/aarch64/virt/IORT.its_off b/tests/data/acpi/aarch64/virt/IORT.its_off
index 0fceb820d509e852ca0849baf568a8e93e426738..c10da4e61dd00e7eb062558a2735d49ca0b20620 100644
GIT binary patch
delta 69
zcmaFExQ3C-(?2L=4FdxM^Yn>aQj$zSmH`lh0E-I)3xowECx)7HGFdP%GXmL+6IZHp
Hz*GSMclZc%
literal 236
zcmebD4+?q1z`(#9?&R<65v<@85#X!<1dKp25F11@1F-=RgMkDCNC*yK9F_<M77!bR
zUBI%eoFED&4;F$FSwK1)h;xBB2Py`m{{M%tVD>TjFfcO#g+N#Zh@s|zoCF3AP#UU@
R!2`+%Dg6Hr$N|zYvjDIZ5CH%H
diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
index a88198d5c2..dfb8523c8b 100644
--- a/tests/qtest/bios-tables-test-allowed-diff.h
+++ b/tests/qtest/bios-tables-test-allowed-diff.h
@@ -1,3 +1 @@
/* 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",
--
2.34.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v4 1/8] hw/intc/gicv3_its: Do not check its_class_name()
2025-06-16 13:18 ` [PATCH v4 1/8] hw/intc/gicv3_its: Do not check its_class_name() Gustavo Romero
@ 2025-06-16 13:33 ` Philippe Mathieu-Daudé
2025-06-16 13:57 ` Gustavo Romero
0 siblings, 1 reply; 26+ messages in thread
From: Philippe Mathieu-Daudé @ 2025-06-16 13:33 UTC (permalink / raw)
To: Gustavo Romero, qemu-devel, eric.auger, mst
Cc: qemu-arm, alex.bennee, udo, ajones, peter.maydell, imammedo,
anisinha, Richard Henderson
Hi Gustavo, why reset authorship?
On 16/6/25 15:18, Gustavo Romero wrote:
> Since commit cc5e719e2c8 ("kvm: require KVM_CAP_SIGNAL_MSI"), the single
> implementation of its_class_name() no longer returns NULL (it now always
> returns a valid char pointer). Hence, update the prototype docstring and
> remove the tautological checks that use the its_class_name() returned
> value.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Reviewed-by: Eric Auger <eric.auger@redhat.com>
> ---
> hw/arm/virt-acpi-build.c | 32 +++++++++++---------------
> include/hw/intc/arm_gicv3_its_common.h | 2 +-
> 2 files changed, 15 insertions(+), 19 deletions(-)
>
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 7e8e0f0298..9eee284c80 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -737,20 +737,18 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> memmap[VIRT_HIGH_GIC_REDIST2].size);
> }
>
> - if (its_class_name()) {
> - /*
> - * ACPI spec, Revision 6.0 Errata A
> - * (original 6.0 definition has invalid Length)
> - * 5.2.12.18 GIC ITS Structure
> - */
> - build_append_int_noprefix(table_data, 0xF, 1); /* Type */
> - build_append_int_noprefix(table_data, 20, 1); /* Length */
> - build_append_int_noprefix(table_data, 0, 2); /* Reserved */
> - build_append_int_noprefix(table_data, 0, 4); /* GIC ITS ID */
> - /* Physical Base Address */
> - build_append_int_noprefix(table_data, memmap[VIRT_GIC_ITS].base, 8);
> - build_append_int_noprefix(table_data, 0, 4); /* Reserved */
> - }
> + /*
> + * ACPI spec, Revision 6.0 Errata A
> + * (original 6.0 definition has invalid Length)
> + * 5.2.12.18 GIC ITS Structure
> + */
> + build_append_int_noprefix(table_data, 0xF, 1); /* Type */
> + build_append_int_noprefix(table_data, 20, 1); /* Length */
> + build_append_int_noprefix(table_data, 0, 2); /* Reserved */
> + build_append_int_noprefix(table_data, 0, 4); /* GIC ITS ID */
> + /* Physical Base Address */
> + build_append_int_noprefix(table_data, memmap[VIRT_GIC_ITS].base, 8);
> + build_append_int_noprefix(table_data, 0, 4); /* Reserved */
> } else {
> const uint16_t spi_base = vms->irqmap[VIRT_GIC_V2M] + ARM_SPI_BASE;
>
> @@ -969,10 +967,8 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
> vms->oem_table_id);
> }
>
> - if (its_class_name()) {
> - acpi_add_table(table_offsets, tables_blob);
> - build_iort(tables_blob, tables->linker, vms);
> - }
> + acpi_add_table(table_offsets, tables_blob);
> + build_iort(tables_blob, tables->linker, vms);
>
> #ifdef CONFIG_TPM
> if (tpm_get_version(tpm_find()) == TPM_VERSION_2_0) {
> diff --git a/include/hw/intc/arm_gicv3_its_common.h b/include/hw/intc/arm_gicv3_its_common.h
> index 7dc712b38d..3c7b543b01 100644
> --- a/include/hw/intc/arm_gicv3_its_common.h
> +++ b/include/hw/intc/arm_gicv3_its_common.h
> @@ -128,7 +128,7 @@ struct GICv3ITSCommonClass {
> * Return the ITS class name to use depending on whether KVM acceleration
> * and KVM CAP_SIGNAL_MSI are supported
> *
> - * Returns: class name to use or NULL
> + * Returns: class name to use
> */
> const char *its_class_name(void);
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 1/8] hw/intc/gicv3_its: Do not check its_class_name()
2025-06-16 13:33 ` Philippe Mathieu-Daudé
@ 2025-06-16 13:57 ` Gustavo Romero
0 siblings, 0 replies; 26+ messages in thread
From: Gustavo Romero @ 2025-06-16 13:57 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, qemu-devel, eric.auger, mst
Cc: qemu-arm, alex.bennee, udo, ajones, peter.maydell, imammedo,
anisinha, Richard Henderson
Hi Phil,
On 6/16/25 10:33, Philippe Mathieu-Daudé wrote:
> Hi Gustavo, why reset authorship?
It was not intentional. hmm maybe it changed because I tweaked
the commit message and the code due to rebasing as well? Do you
know how to restore it?
Cheers,
Gustavo
> On 16/6/25 15:18, Gustavo Romero wrote:
>> Since commit cc5e719e2c8 ("kvm: require KVM_CAP_SIGNAL_MSI"), the single
>> implementation of its_class_name() no longer returns NULL (it now always
>> returns a valid char pointer). Hence, update the prototype docstring and
>> remove the tautological checks that use the its_class_name() returned
>> value.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>> Reviewed-by: Eric Auger <eric.auger@redhat.com>
>> ---
>> hw/arm/virt-acpi-build.c | 32 +++++++++++---------------
>> include/hw/intc/arm_gicv3_its_common.h | 2 +-
>> 2 files changed, 15 insertions(+), 19 deletions(-)
>>
>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>> index 7e8e0f0298..9eee284c80 100644
>> --- a/hw/arm/virt-acpi-build.c
>> +++ b/hw/arm/virt-acpi-build.c
>> @@ -737,20 +737,18 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>> memmap[VIRT_HIGH_GIC_REDIST2].size);
>> }
>> - if (its_class_name()) {
>> - /*
>> - * ACPI spec, Revision 6.0 Errata A
>> - * (original 6.0 definition has invalid Length)
>> - * 5.2.12.18 GIC ITS Structure
>> - */
>> - build_append_int_noprefix(table_data, 0xF, 1); /* Type */
>> - build_append_int_noprefix(table_data, 20, 1); /* Length */
>> - build_append_int_noprefix(table_data, 0, 2); /* Reserved */
>> - build_append_int_noprefix(table_data, 0, 4); /* GIC ITS ID */
>> - /* Physical Base Address */
>> - build_append_int_noprefix(table_data, memmap[VIRT_GIC_ITS].base, 8);
>> - build_append_int_noprefix(table_data, 0, 4); /* Reserved */
>> - }
>> + /*
>> + * ACPI spec, Revision 6.0 Errata A
>> + * (original 6.0 definition has invalid Length)
>> + * 5.2.12.18 GIC ITS Structure
>> + */
>> + build_append_int_noprefix(table_data, 0xF, 1); /* Type */
>> + build_append_int_noprefix(table_data, 20, 1); /* Length */
>> + build_append_int_noprefix(table_data, 0, 2); /* Reserved */
>> + build_append_int_noprefix(table_data, 0, 4); /* GIC ITS ID */
>> + /* Physical Base Address */
>> + build_append_int_noprefix(table_data, memmap[VIRT_GIC_ITS].base, 8);
>> + build_append_int_noprefix(table_data, 0, 4); /* Reserved */
>> } else {
>> const uint16_t spi_base = vms->irqmap[VIRT_GIC_V2M] + ARM_SPI_BASE;
>> @@ -969,10 +967,8 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
>> vms->oem_table_id);
>> }
>> - if (its_class_name()) {
>> - acpi_add_table(table_offsets, tables_blob);
>> - build_iort(tables_blob, tables->linker, vms);
>> - }
>> + acpi_add_table(table_offsets, tables_blob);
>> + build_iort(tables_blob, tables->linker, vms);
>> #ifdef CONFIG_TPM
>> if (tpm_get_version(tpm_find()) == TPM_VERSION_2_0) {
>> diff --git a/include/hw/intc/arm_gicv3_its_common.h b/include/hw/intc/arm_gicv3_its_common.h
>> index 7dc712b38d..3c7b543b01 100644
>> --- a/include/hw/intc/arm_gicv3_its_common.h
>> +++ b/include/hw/intc/arm_gicv3_its_common.h
>> @@ -128,7 +128,7 @@ struct GICv3ITSCommonClass {
>> * Return the ITS class name to use depending on whether KVM acceleration
>> * and KVM CAP_SIGNAL_MSI are supported
>> *
>> - * Returns: class name to use or NULL
>> + * Returns: class name to use
>> */
>> const char *its_class_name(void);
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH-for-10.1 v4 0/8] hw/arm: GIC 'its=off' ACPI table fixes
2025-06-16 13:18 [PATCH-for-10.1 v4 0/8] hw/arm: GIC 'its=off' ACPI table fixes Gustavo Romero
` (7 preceding siblings ...)
2025-06-16 13:18 ` [PATCH v4 8/8] qtest/bios-tables-test: Update blobs for its=off test on aarch64 Gustavo Romero
@ 2025-06-17 9:35 ` Eric Auger
2025-06-17 13:01 ` Gustavo Romero
8 siblings, 1 reply; 26+ messages in thread
From: Eric Auger @ 2025-06-17 9:35 UTC (permalink / raw)
To: Gustavo Romero, qemu-devel, philmd, mst,
Shameerali Kolothum Thodi
Cc: qemu-arm, alex.bennee, udo, ajones, peter.maydell, imammedo,
anisinha
Hi Gustavo,
On 6/16/25 3:18 PM, Gustavo Romero wrote:
> Since v2:
> - Fixed no_tcg_its inverted logic (rth)
>
> Since v3:
> - Fixed remappings in the IORT table when ITS is no present
> - Rebased on master and resoled conflics, like no more "no_its"
> flag in VirtMachineClass
> - Dropped patch 1/9 because we actually want the instance flags,
> not only the class flags, and the instance flags are the ones
> to be used often when deciding about the presence/absence of a
> machine feature, instead of the negated class flags ("no_*")
> - Adapted the other patches that depended on 1/9
> - Dropped patch 4/9 in favor of using the instance flag for
> checking if ITS is on or off
> - Simplified VM options for the new "its=off" test
>
> v1: https://lists.gnu.org/archive/html/qemu-devel/2025-03/msg07080.html
> v2: https://lists.gnu.org/archive/html/qemu-devel/2025-04/msg00495.html (Patches 6/14 -> 14/14 in the series)
> v3: https://lists.gnu.org/archive/html/qemu-devel/2025-04/msg00567.html
>
> Fix ACPI tables for '-M its=off' CLI option and resolve the issue:
>
> https://gitlab.com/qemu-project/qemu/-/issues/2886
One first comment is that this series will collide with Shameer's SMMU
multi instance series which has been lunder review for quite some time
(adding him in TO):
I think it may be more future proof if you could rebase on it - I know
it is a pain ;-( -. Or if sbdy objects for Shameer's series please raise
your voice now.
[PATCH v4 0/7] hw/arm/virt: Add support for user creatable SMMUv3 device <https://lore.kernel.org/all/20250613144449.60156-1-shameerali.kolothum.thodi@huawei.com/#r>
https://lore.kernel.org/all/20250613144449.60156-1-shameerali.kolothum.thodi@huawei.com/
Also I understood Shameer intended to write some new bios-tables-test.
Thanks
Eric
>
> Cheers,
> Gustavo
>
> Gustavo Romero (7):
> hw/intc/gicv3_its: Do not check its_class_name()
> hw/arm/virt: Simplify logic for setting instance's 'tcg_its' variable
> hw/arm/virt: Simplify create_its()
> hw/arm/virt-acpi-build: Fix comment in build_iort
> qtest/bios-tables-test: Add blobs for its=off test on aarch64
> hw/arm/virt-acpi-build: Fix ACPI IORT and MADT tables when its=off
> qtest/bios-tables-test: Update blobs for its=off test on aarch64
>
> Philippe Mathieu-Daudé (1):
> qtest/bios-tables-test: Add test for when ITS is off on aarch64
>
> hw/arm/virt-acpi-build.c | 134 +++++++++++++---------
> hw/arm/virt.c | 25 ++--
> include/hw/intc/arm_gicv3_its_common.h | 2 +-
> tests/data/acpi/aarch64/virt/APIC.its_off | Bin 0 -> 164 bytes
> tests/data/acpi/aarch64/virt/IORT.its_off | Bin 0 -> 172 bytes
> tests/qtest/bios-tables-test.c | 21 ++++
> 6 files changed, 113 insertions(+), 69 deletions(-)
> create mode 100644 tests/data/acpi/aarch64/virt/APIC.its_off
> create mode 100644 tests/data/acpi/aarch64/virt/IORT.its_off
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 2/8] hw/arm/virt: Simplify logic for setting instance's 'tcg_its' variable
2025-06-16 13:18 ` [PATCH v4 2/8] hw/arm/virt: Simplify logic for setting instance's 'tcg_its' variable Gustavo Romero
@ 2025-06-17 9:46 ` Eric Auger
0 siblings, 0 replies; 26+ messages in thread
From: Eric Auger @ 2025-06-17 9:46 UTC (permalink / raw)
To: Gustavo Romero, qemu-devel, philmd, mst
Cc: qemu-arm, alex.bennee, udo, ajones, peter.maydell, imammedo,
anisinha
Hi Gustavo,
On 6/16/25 3:18 PM, Gustavo Romero wrote:
> Because 'tcg_its' in the machine instance is set based on the machine
> class’s negated variable 'no_tcg_its', 'tcg_its' is the opposite of
> 'no_tcg_its' and hence the code in question can be simplified as:
> tcg_its = !no_tcg_its.
>
> Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
> ---
> hw/arm/virt.c | 8 ++------
> 1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 9a6cd085a3..42a63a523e 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -3337,12 +3337,8 @@ static void virt_instance_init(Object *obj)
>
> /* Default allows ITS instantiation */
> vms->its = true;
> -
> - if (vmc->no_tcg_its) {
> - vms->tcg_its = false;
> - } else {
> - vms->tcg_its = true;
> - }
> + /* Allow ITS emulation if the machine version supports it. */
remove "."?
besides
Reviewed-by: Eric Auger <eric.auger@redhat.com>
> + vms->tcg_its = !vmc->no_tcg_its;
>
> /* Default disallows iommu instantiation */
> vms->iommu = VIRT_IOMMU_NONE;
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH-for-10.1 v4 0/8] hw/arm: GIC 'its=off' ACPI table fixes
2025-06-17 9:35 ` [PATCH-for-10.1 v4 0/8] hw/arm: GIC 'its=off' ACPI table fixes Eric Auger
@ 2025-06-17 13:01 ` Gustavo Romero
2025-06-17 13:26 ` Eric Auger
0 siblings, 1 reply; 26+ messages in thread
From: Gustavo Romero @ 2025-06-17 13:01 UTC (permalink / raw)
To: eric.auger, qemu-devel, philmd, mst, Shameerali Kolothum Thodi
Cc: qemu-arm, alex.bennee, udo, ajones, peter.maydell, imammedo,
anisinha
Hi Eric,
Thanks a lot for doing a first pass on this series!
On 6/17/25 06:35, Eric Auger wrote:
> Hi Gustavo,
>
> On 6/16/25 3:18 PM, Gustavo Romero wrote:
>> Since v2:
>> - Fixed no_tcg_its inverted logic (rth)
>>
>> Since v3:
>> - Fixed remappings in the IORT table when ITS is no present
>> - Rebased on master and resoled conflics, like no more "no_its"
>> flag in VirtMachineClass
>> - Dropped patch 1/9 because we actually want the instance flags,
>> not only the class flags, and the instance flags are the ones
>> to be used often when deciding about the presence/absence of a
>> machine feature, instead of the negated class flags ("no_*")
>> - Adapted the other patches that depended on 1/9
>> - Dropped patch 4/9 in favor of using the instance flag for
>> checking if ITS is on or off
>> - Simplified VM options for the new "its=off" test
>>
>> v1: https://lists.gnu.org/archive/html/qemu-devel/2025-03/msg07080.html
>> v2: https://lists.gnu.org/archive/html/qemu-devel/2025-04/msg00495.html (Patches 6/14 -> 14/14 in the series)
>> v3: https://lists.gnu.org/archive/html/qemu-devel/2025-04/msg00567.html
>>
>> Fix ACPI tables for '-M its=off' CLI option and resolve the issue:
>>
>> https://gitlab.com/qemu-project/qemu/-/issues/2886
>
> One first comment is that this series will collide with Shameer's SMMU
> multi instance series which has been lunder review for quite some time
> (adding him in TO):
>
> I think it may be more future proof if you could rebase on it - I know
> it is a pain ;-( -. Or if sbdy objects for Shameer's series please raise
> your voice now.
>
> [PATCH v4 0/7] hw/arm/virt: Add support for user creatable SMMUv3 device <https://lore.kernel.org/all/20250613144449.60156-1-shameerali.kolothum.thodi@huawei.com/#r>
>
> https://lore.kernel.org/all/20250613144449.60156-1-shameerali.kolothum.thodi@huawei.com/
ayayay, life is never that easy! :)
Thanks for point that out. Sure, I can rebase it on Shameer's series, but also
I'd like to have this ITS fix for 10.1, so I think it's a matter of understanding
if Shameer's series will make the 10.1 release (thanks for asking the reviewers if they
have any current objection so we have an idea if it's close to get accepted
or not)?
Meanwhile, I'm pretty keen on if I'm correctly generating the IORT table pruned from ITS
(patch 7/8 in this series), like, are the remappings for the RC and SMMU nodes correct? That
would make me more comfortable to start working on a rebase.
> Also I understood Shameer intended to write some new bios-tables-test.
I see.
Cheers,
Gustavo
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 4/8] hw/arm/virt-acpi-build: Fix comment in build_iort
2025-06-16 13:18 ` [PATCH v4 4/8] hw/arm/virt-acpi-build: Fix comment in build_iort Gustavo Romero
@ 2025-06-17 13:22 ` Eric Auger
2025-06-19 17:07 ` Gustavo Romero
0 siblings, 1 reply; 26+ messages in thread
From: Eric Auger @ 2025-06-17 13:22 UTC (permalink / raw)
To: Gustavo Romero, qemu-devel, philmd, mst
Cc: qemu-arm, alex.bennee, udo, ajones, peter.maydell, imammedo,
anisinha
Hi Gustavo,
On 6/16/25 3:18 PM, Gustavo Romero wrote:
> The comment about the mapping from SMMU to ITS is incorrect and it reads
> "RC -> ITS". The code in question actually maps SMMU -> ITS, so the
> mapping in question is not direct. The direct mapping, i.e., RC -> ITS,
> is handled a bit further down in the code, in the else block, and we
> take the opportunity to update that as well, adding "RC -> ITS" to the
> text so it's easier to see it's the direct map to the ITS Group node.
>
> Signed-off-by Gustavo Romero <gustavo.romero@linaro.org>
> ---
> hw/arm/virt-acpi-build.c | 14 +++++++++-----
> 1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 9eee284c80..6990bce3bb 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -407,23 +407,27 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> if (vms->iommu == VIRT_IOMMU_SMMUV3) {
> AcpiIortIdMapping *range;
>
> - /* translated RIDs connect to SMMUv3 node: RC -> SMMUv3 -> ITS */
> + /* Map RIDs (input) from RC to SMMUv3 nodes: RC -> SMMUv3. */
yes this is what the code builds at this location.
> for (i = 0; i < smmu_idmaps->len; i++) {
> range = &g_array_index(smmu_idmaps, AcpiIortIdMapping, i);
> - /* output IORT node is the smmuv3 node */
> + /* Output IORT node is the SMMUv3 node. */
> build_iort_id_mapping(table_data, range->input_base,
> range->id_count, smmu_offset);
> }
>
> - /* bypassed RIDs connect to ITS group node directly: RC -> ITS */
> + /* Map DeviceIDs (input) from SMMUv3 to ITS Group nodes: SMMU -> ITS. */
But here I am confused. To me the its_idmaps matches the idmap between
RC and ITS. I understand this is built from holes left by bypassed
buses. See the build_iort() implementation. The comment at
/*
* Split the whole RIDs by mapping from RC to SMMU,
* build the ID mapping from RC to ITS directly.
*/
for (i = 0; i < smmu_idmaps->len; i++) {
idmap = &g_array_index(smmu_idmaps, AcpiIortIdMapping, i);
if (next_range.input_base < idmap->input_base) {
next_range.id_count = idmap->input_base -
next_range.input_base;
g_array_append_val(its_idmaps, next_range);
}
next_range.input_base = idmap->input_base + idmap->id_count;
}
is not crystal clear but it looks like filling the holes into its_idmap.
Besides there is another "Its group" instance to be replaced by ITS Group
Eric
> for (i = 0; i < its_idmaps->len; i++) {
> range = &g_array_index(its_idmaps, AcpiIortIdMapping, i);
> - /* output IORT node is the ITS group node (the first node) */
> + /* Output IORT node is the ITS Group node (the first node). */
> build_iort_id_mapping(table_data, range->input_base,
> range->id_count, IORT_NODE_OFFSET);
> }
> } else {
> - /* output IORT node is the ITS group node (the first node) */
> + /*
> + * Map bypassed RIDs (input) (don't go through the SMMU) to ITS Group
> + * nodes: RC -> ITS.
> + * Output IORT node is the ITS Group node (the first node).
> + */
> build_iort_id_mapping(table_data, 0, 0x10000, IORT_NODE_OFFSET);
> }
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH-for-10.1 v4 0/8] hw/arm: GIC 'its=off' ACPI table fixes
2025-06-17 13:01 ` Gustavo Romero
@ 2025-06-17 13:26 ` Eric Auger
2025-06-23 18:54 ` Gustavo Romero
0 siblings, 1 reply; 26+ messages in thread
From: Eric Auger @ 2025-06-17 13:26 UTC (permalink / raw)
To: Gustavo Romero, qemu-devel, philmd, mst,
Shameerali Kolothum Thodi
Cc: qemu-arm, alex.bennee, udo, ajones, peter.maydell, imammedo,
anisinha
Hi Gustavo,
On 6/17/25 3:01 PM, Gustavo Romero wrote:
> Hi Eric,
>
> Thanks a lot for doing a first pass on this series!
>
> On 6/17/25 06:35, Eric Auger wrote:
>> Hi Gustavo,
>>
>> On 6/16/25 3:18 PM, Gustavo Romero wrote:
>>> Since v2:
>>> - Fixed no_tcg_its inverted logic (rth)
>>>
>>> Since v3:
>>> - Fixed remappings in the IORT table when ITS is no present
>>> - Rebased on master and resoled conflics, like no more "no_its"
>>> flag in VirtMachineClass
>>> - Dropped patch 1/9 because we actually want the instance flags,
>>> not only the class flags, and the instance flags are the ones
>>> to be used often when deciding about the presence/absence of a
>>> machine feature, instead of the negated class flags ("no_*")
>>> - Adapted the other patches that depended on 1/9
>>> - Dropped patch 4/9 in favor of using the instance flag for
>>> checking if ITS is on or off
>>> - Simplified VM options for the new "its=off" test
>>>
>>> v1: https://lists.gnu.org/archive/html/qemu-devel/2025-03/msg07080.html
>>> v2:
>>> https://lists.gnu.org/archive/html/qemu-devel/2025-04/msg00495.html
>>> (Patches 6/14 -> 14/14 in the series)
>>> v3: https://lists.gnu.org/archive/html/qemu-devel/2025-04/msg00567.html
>>>
>>> Fix ACPI tables for '-M its=off' CLI option and resolve the issue:
>>>
>>> https://gitlab.com/qemu-project/qemu/-/issues/2886
>>
>> One first comment is that this series will collide with Shameer's SMMU
>> multi instance series which has been lunder review for quite some time
>> (adding him in TO):
>>
>> I think it may be more future proof if you could rebase on it - I know
>> it is a pain ;-( -. Or if sbdy objects for Shameer's series please raise
>> your voice now.
>>
>> [PATCH v4 0/7] hw/arm/virt: Add support for user creatable SMMUv3
>> device
>> <https://lore.kernel.org/all/20250613144449.60156-1-shameerali.kolothum.thodi@huawei.com/#r>
>>
>> https://lore.kernel.org/all/20250613144449.60156-1-shameerali.kolothum.thodi@huawei.com/
>>
>
> ayayay, life is never that easy! :)
>
> Thanks for point that out. Sure, I can rebase it on Shameer's series,
> but also
> I'd like to have this ITS fix for 10.1, so I think it's a matter of
> understanding
> if Shameer's series will make the 10.1 release (thanks for asking the
> reviewers if they
> have any current objection so we have an idea if it's close to get
> accepted
> or not)?
Peter was the most annoyed by the usage of -device arm-smmuv3 option
line. We'd better ask him.
On my end I don't see how we can achieve this more elegantly.
>
> Meanwhile, I'm pretty keen on if I'm correctly generating the IORT
> table pruned from ITS
> (patch 7/8 in this series), like, are the remappings for the RC and
> SMMU nodes correct? That
> would make me more comfortable to start working on a rebase.
sure looking at it...
Eric
>
>
>> Also I understood Shameer intended to write some new bios-tables-test.
>
> I see.
>
>
> Cheers,
> Gustavo
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 5/8] qtest/bios-tables-test: Add test for when ITS is off on aarch64
2025-06-16 13:18 ` [PATCH v4 5/8] qtest/bios-tables-test: Add test for when ITS is off on aarch64 Gustavo Romero
@ 2025-06-17 13:34 ` Eric Auger
2025-06-17 15:12 ` Gustavo Romero
0 siblings, 1 reply; 26+ messages in thread
From: Eric Auger @ 2025-06-17 13:34 UTC (permalink / raw)
To: Gustavo Romero, qemu-devel, philmd, mst
Cc: qemu-arm, alex.bennee, udo, ajones, peter.maydell, imammedo,
anisinha
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 7/8] hw/arm/virt-acpi-build: Fix ACPI IORT and MADT tables when its=off
2025-06-16 13:18 ` [PATCH v4 7/8] hw/arm/virt-acpi-build: Fix ACPI IORT and MADT tables when its=off Gustavo Romero
@ 2025-06-17 14:04 ` Eric Auger
0 siblings, 0 replies; 26+ messages in thread
From: Eric Auger @ 2025-06-17 14:04 UTC (permalink / raw)
To: Gustavo Romero, qemu-devel, philmd, mst
Cc: qemu-arm, alex.bennee, udo, ajones, peter.maydell, imammedo,
anisinha
Hi Gustavo,
On 6/16/25 3:18 PM, Gustavo Romero wrote:
> Currently, the ITS Group nodes in the IORT table and the GIC ITS Struct
> in the MADT table are always generated, even if GIC ITS is not available
> on the machine.
>
> This commit fixes it by not generating the ITS Group nodes, not mapping
> any other node to them, and not advertising the GIC ITS in the MADT
> table, when GIC ITS is not available on the machine.
>
> Since the fix changes the MADT and IORT tables, add the blobs for the
> "its=off" test to the allow list and update them in the next commit.
>
> Reported-by: Udo Steinberg <udo@hypervisor.org>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2886
> Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> hw/arm/virt-acpi-build.c | 152 ++++++++++++--------
> tests/qtest/bios-tables-test-allowed-diff.h | 2 +
> 2 files changed, 93 insertions(+), 61 deletions(-)
>
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 6990bce3bb..2240421980 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -295,32 +295,42 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> /* Sort the smmu idmap by input_base */
> g_array_sort(smmu_idmaps, iort_idmap_compare);
>
> - /*
> - * Split the whole RIDs by mapping from RC to SMMU,
> - * build the ID mapping from RC to ITS directly.
> - */
> - for (i = 0; i < smmu_idmaps->len; i++) {
> - idmap = &g_array_index(smmu_idmaps, AcpiIortIdMapping, i);
> + nb_nodes = 2; /* RC and SMMUv3 */
> + rc_mapping_count = smmu_idmaps->len;
> +
> + if (vms->its) {
> + /*
> + * Split the whole RIDs by mapping from RC to SMMU,
> + * build the ID mapping from RC to ITS directly.
> + */
> + for (i = 0; i < smmu_idmaps->len; i++) {
> + idmap = &g_array_index(smmu_idmaps, AcpiIortIdMapping, i);
> +
> + if (next_range.input_base < idmap->input_base) {
> + next_range.id_count = idmap->input_base - next_range.input_base;
> + g_array_append_val(its_idmaps, next_range);
> + }
> +
> + next_range.input_base = idmap->input_base + idmap->id_count;
> + }
I think I would create a helper that populates the its_idmap from the
smmu_idmaps. To me to would make the code more readable. Also we could
add a doc comment bout the helper explaining what it does.
>
> - if (next_range.input_base < idmap->input_base) {
> - next_range.id_count = idmap->input_base - next_range.input_base;
> + /* Append the last RC -> ITS ID mapping */
> + if (next_range.input_base < 0x10000) {
> + next_range.id_count = 0x10000 - next_range.input_base;
> g_array_append_val(its_idmaps, next_range);
> }
>
> - next_range.input_base = idmap->input_base + idmap->id_count;
> + nb_nodes++; /* ITS */
> + rc_mapping_count += its_idmaps->len;
> }
> -
> - /* Append the last RC -> ITS ID mapping */
> - if (next_range.input_base < 0x10000) {
> - next_range.id_count = 0x10000 - next_range.input_base;
> - g_array_append_val(its_idmaps, next_range);
> - }
> -
> - nb_nodes = 3; /* RC, ITS, SMMUv3 */
> - rc_mapping_count = smmu_idmaps->len + its_idmaps->len;
> } else {
> - nb_nodes = 2; /* RC, ITS */
> - rc_mapping_count = 1;
> + if (vms->its) {
> + nb_nodes = 2; /* RC and ITS */
> + rc_mapping_count = 1; /* Direct map to ITS */
> + } else {
> + nb_nodes = 1; /* RC only */
> + rc_mapping_count = 0; /* No output mapping */
> + }
> }
> /* Number of IORT Nodes */
> build_append_int_noprefix(table_data, nb_nodes, 4);
> @@ -329,31 +339,43 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> build_append_int_noprefix(table_data, IORT_NODE_OFFSET, 4);
> build_append_int_noprefix(table_data, 0, 4); /* Reserved */
>
> - /* Table 12 ITS Group Format */
> - build_append_int_noprefix(table_data, 0 /* ITS Group */, 1); /* Type */
> - node_size = 20 /* fixed header size */ + 4 /* 1 GIC ITS Identifier */;
> - build_append_int_noprefix(table_data, node_size, 2); /* Length */
> - build_append_int_noprefix(table_data, 1, 1); /* Revision */
> - build_append_int_noprefix(table_data, id++, 4); /* Identifier */
> - build_append_int_noprefix(table_data, 0, 4); /* Number of ID mappings */
> - build_append_int_noprefix(table_data, 0, 4); /* Reference to ID Array */
> - build_append_int_noprefix(table_data, 1, 4); /* Number of ITSs */
> - /* GIC ITS Identifier Array */
> - build_append_int_noprefix(table_data, 0 /* MADT translation_id */, 4);
> + if (vms->its) {
> + /* Table 12 ITS Group Format */
> + build_append_int_noprefix(table_data, 0 /* ITS Group */, 1); /* Type */
> + node_size = 20 /* fixed header size */ + 4 /* 1 GIC ITS Identifier */;
> + build_append_int_noprefix(table_data, node_size, 2); /* Length */
> + build_append_int_noprefix(table_data, 1, 1); /* Revision */
> + build_append_int_noprefix(table_data, id++, 4); /* Identifier */
> + build_append_int_noprefix(table_data, 0, 4); /* Number of ID mappings */
> + build_append_int_noprefix(table_data, 0, 4); /* Reference to ID Array */
> + build_append_int_noprefix(table_data, 1, 4); /* Number of ITSs */
> + /* GIC ITS Identifier Array */
> + build_append_int_noprefix(table_data, 0 /* MADT translation_id */, 4);
> + }
>
> if (vms->iommu == VIRT_IOMMU_SMMUV3) {
> int irq = vms->irqmap[VIRT_SMMU] + ARM_SPI_BASE;
> -
> + int num_id_mappings, offset_to_id_array;
> +
> + if (vms->its) {
> + num_id_mappings = 1; /* ITS Group node */
> + offset_to_id_array = SMMU_V3_ENTRY_SIZE; /* Just after the header */
> + } else {
> + num_id_mappings = 0; /* No ID mappings */
> + offset_to_id_array = 0; /* No ID mappings array */
> + }
> smmu_offset = table_data->len - table.table_offset;
> /* Table 9 SMMUv3 Format */
> build_append_int_noprefix(table_data, 4 /* SMMUv3 */, 1); /* Type */
> - node_size = SMMU_V3_ENTRY_SIZE + ID_MAPPING_ENTRY_SIZE;
> + node_size = SMMU_V3_ENTRY_SIZE +
> + (ID_MAPPING_ENTRY_SIZE * num_id_mappings);
> build_append_int_noprefix(table_data, node_size, 2); /* Length */
> build_append_int_noprefix(table_data, 4, 1); /* Revision */
> build_append_int_noprefix(table_data, id++, 4); /* Identifier */
> - build_append_int_noprefix(table_data, 1, 4); /* Number of ID mappings */
> + /* Number of ID mappings */
> + build_append_int_noprefix(table_data, num_id_mappings, 4);
> /* Reference to ID Array */
> - build_append_int_noprefix(table_data, SMMU_V3_ENTRY_SIZE, 4);
> + build_append_int_noprefix(table_data, offset_to_id_array, 4);
> /* Base address */
> build_append_int_noprefix(table_data, vms->memmap[VIRT_SMMU].base, 8);
> /* Flags */
> @@ -370,8 +392,10 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> /* DeviceID mapping index (ignored since interrupts are GSIV based) */
> build_append_int_noprefix(table_data, 0, 4);
>
> - /* output IORT node is the ITS group node (the first node) */
> - build_iort_id_mapping(table_data, 0, 0x10000, IORT_NODE_OFFSET);
> + if (vms->its) {
I would use num_id_mappings
> + /* Output IORT node is the ITS Group node (the first node). */
> + build_iort_id_mapping(table_data, 0, 0x10000, IORT_NODE_OFFSET);
> + }
> }
>
> /* Table 17 Root Complex Node */
> @@ -415,20 +439,24 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> range->id_count, smmu_offset);
> }
>
> - /* Map DeviceIDs (input) from SMMUv3 to ITS Group nodes: SMMU -> ITS. */
see my previous comment
> - for (i = 0; i < its_idmaps->len; i++) {
> - range = &g_array_index(its_idmaps, AcpiIortIdMapping, i);
> - /* Output IORT node is the ITS Group node (the first node). */
> - build_iort_id_mapping(table_data, range->input_base,
> - range->id_count, IORT_NODE_OFFSET);
> + if (vms->its) {
> + /* Map DeviceIDs (input) from SMMUv3 to ITS Group nodes: SMMU -> ITS. */
> + for (i = 0; i < its_idmaps->len; i++) {
> + range = &g_array_index(its_idmaps, AcpiIortIdMapping, i);
> + /* Output IORT node is the ITS Group node (the first node). */
> + build_iort_id_mapping(table_data, range->input_base,
> + range->id_count, IORT_NODE_OFFSET);
> + }
> }
> } else {
> - /*
> - * Map bypassed RIDs (input) (don't go through the SMMU) to ITS Group
> - * nodes: RC -> ITS.
> - * Output IORT node is the ITS Group node (the first node).
> - */
> - build_iort_id_mapping(table_data, 0, 0x10000, IORT_NODE_OFFSET);
> + if (vms->its) {
> + /*
> + * Map bypassed RIDs (input) (don't go through the SMMU) to ITS Group
> + * nodes: RC -> ITS.
> + * Output IORT node is the ITS Group node (the first node).
> + */
> + build_iort_id_mapping(table_data, 0, 0x10000, IORT_NODE_OFFSET);
> + }
> }
>
> acpi_table_end(linker, &table);
> @@ -741,18 +769,20 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> memmap[VIRT_HIGH_GIC_REDIST2].size);
> }
>
> - /*
> - * ACPI spec, Revision 6.0 Errata A
> - * (original 6.0 definition has invalid Length)
> - * 5.2.12.18 GIC ITS Structure
> - */
> - build_append_int_noprefix(table_data, 0xF, 1); /* Type */
> - build_append_int_noprefix(table_data, 20, 1); /* Length */
> - build_append_int_noprefix(table_data, 0, 2); /* Reserved */
> - build_append_int_noprefix(table_data, 0, 4); /* GIC ITS ID */
> - /* Physical Base Address */
> - build_append_int_noprefix(table_data, memmap[VIRT_GIC_ITS].base, 8);
> - build_append_int_noprefix(table_data, 0, 4); /* Reserved */
> + if (vms->its) {
> + /*
> + * ACPI spec, Revision 6.0 Errata A
> + * (original 6.0 definition has invalid Length)
> + * 5.2.12.18 GIC ITS Structure
> + */
> + build_append_int_noprefix(table_data, 0xF, 1); /* Type */
> + build_append_int_noprefix(table_data, 20, 1); /* Length */
> + build_append_int_noprefix(table_data, 0, 2); /* Reserved */
> + build_append_int_noprefix(table_data, 0, 4); /* GIC ITS ID */
> + /* Physical Base Address */
> + build_append_int_noprefix(table_data, memmap[VIRT_GIC_ITS].base, 8);
> + build_append_int_noprefix(table_data, 0, 4); /* Reserved */
> + }
> } else {
> const uint16_t spi_base = vms->irqmap[VIRT_GIC_V2M] + ARM_SPI_BASE;
>
> 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",
Thanks
Eric
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 5/8] qtest/bios-tables-test: Add test for when ITS is off on aarch64
2025-06-17 13:34 ` Eric Auger
@ 2025-06-17 15:12 ` Gustavo Romero
2025-06-17 15:51 ` Eric Auger
0 siblings, 1 reply; 26+ messages in thread
From: Gustavo Romero @ 2025-06-17 15:12 UTC (permalink / raw)
To: eric.auger, qemu-devel, philmd, mst
Cc: qemu-arm, alex.bennee, udo, ajones, peter.maydell, imammedo,
anisinha
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
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 5/8] qtest/bios-tables-test: Add test for when ITS is off on aarch64
2025-06-17 15:12 ` Gustavo Romero
@ 2025-06-17 15:51 ` Eric Auger
2025-06-17 16:01 ` Gustavo Romero
2025-06-17 16:06 ` Gustavo Romero
0 siblings, 2 replies; 26+ messages in thread
From: Eric Auger @ 2025-06-17 15:51 UTC (permalink / raw)
To: Gustavo Romero, qemu-devel, philmd, mst
Cc: qemu-arm, alex.bennee, udo, ajones, peter.maydell, imammedo,
anisinha
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
>>
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 5/8] qtest/bios-tables-test: Add test for when ITS is off on aarch64
2025-06-17 15:51 ` Eric Auger
@ 2025-06-17 16:01 ` Gustavo Romero
2025-06-17 17:01 ` Eric Auger
2025-06-17 16:06 ` Gustavo Romero
1 sibling, 1 reply; 26+ messages in thread
From: Gustavo Romero @ 2025-06-17 16:01 UTC (permalink / raw)
To: eric.auger, qemu-devel, philmd, mst
Cc: qemu-arm, alex.bennee, udo, ajones, peter.maydell, imammedo,
anisinha
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
>>>
>>
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 5/8] qtest/bios-tables-test: Add test for when ITS is off on aarch64
2025-06-17 15:51 ` Eric Auger
2025-06-17 16:01 ` Gustavo Romero
@ 2025-06-17 16:06 ` Gustavo Romero
1 sibling, 0 replies; 26+ messages in thread
From: Gustavo Romero @ 2025-06-17 16:06 UTC (permalink / raw)
To: eric.auger, qemu-devel, philmd, mst
Cc: qemu-arm, alex.bennee, udo, ajones, peter.maydell, imammedo,
anisinha
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
>>>
>>
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 5/8] qtest/bios-tables-test: Add test for when ITS is off on aarch64
2025-06-17 16:01 ` Gustavo Romero
@ 2025-06-17 17:01 ` Eric Auger
0 siblings, 0 replies; 26+ messages in thread
From: Eric Auger @ 2025-06-17 17:01 UTC (permalink / raw)
To: Gustavo Romero, qemu-devel, philmd, mst
Cc: qemu-arm, alex.bennee, udo, ajones, peter.maydell, imammedo,
anisinha
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
>>>>
>>>
>>
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 4/8] hw/arm/virt-acpi-build: Fix comment in build_iort
2025-06-17 13:22 ` Eric Auger
@ 2025-06-19 17:07 ` Gustavo Romero
2025-06-20 6:52 ` Eric Auger
0 siblings, 1 reply; 26+ messages in thread
From: Gustavo Romero @ 2025-06-19 17:07 UTC (permalink / raw)
To: eric.auger, qemu-devel, philmd, mst
Cc: qemu-arm, alex.bennee, udo, ajones, peter.maydell, imammedo,
anisinha
Hi Eric,
On 6/17/25 10:22, Eric Auger wrote:
> Hi Gustavo,
>
> On 6/16/25 3:18 PM, Gustavo Romero wrote:
>> The comment about the mapping from SMMU to ITS is incorrect and it reads
>> "RC -> ITS". The code in question actually maps SMMU -> ITS, so the
>> mapping in question is not direct. The direct mapping, i.e., RC -> ITS,
>> is handled a bit further down in the code, in the else block, and we
>> take the opportunity to update that as well, adding "RC -> ITS" to the
>> text so it's easier to see it's the direct map to the ITS Group node.
>>
>> Signed-off-by Gustavo Romero <gustavo.romero@linaro.org>
>> ---
>> hw/arm/virt-acpi-build.c | 14 +++++++++-----
>> 1 file changed, 9 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>> index 9eee284c80..6990bce3bb 100644
>> --- a/hw/arm/virt-acpi-build.c
>> +++ b/hw/arm/virt-acpi-build.c
>> @@ -407,23 +407,27 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>> if (vms->iommu == VIRT_IOMMU_SMMUV3) {
>> AcpiIortIdMapping *range;
>>
>> - /* translated RIDs connect to SMMUv3 node: RC -> SMMUv3 -> ITS */
>> + /* Map RIDs (input) from RC to SMMUv3 nodes: RC -> SMMUv3. */
> yes this is what the code builds at this location.
>> for (i = 0; i < smmu_idmaps->len; i++) {
>> range = &g_array_index(smmu_idmaps, AcpiIortIdMapping, i);
>> - /* output IORT node is the smmuv3 node */
>> + /* Output IORT node is the SMMUv3 node. */
>> build_iort_id_mapping(table_data, range->input_base,
>> range->id_count, smmu_offset);
>> }
>>
>> - /* bypassed RIDs connect to ITS group node directly: RC -> ITS */
>> + /* Map DeviceIDs (input) from SMMUv3 to ITS Group nodes: SMMU -> ITS. */
> But here I am confused. To me the its_idmaps matches the idmap between
> RC and ITS. I understand this is built from holes left by bypassed
> buses. See the build_iort() implementation. The comment at
ah, thanks! I see. Indeed, it's mapping the RC -> ITS, not the SMMU -> ITS.
I'll fix it in v5.
One thing that confused me, which I think is actually ok, is that the
output ID range from SMMU 0x000-0xFFFF) overlaps the output ID range
from the RC (e.g. 0x100-0xFFFF) because the SMMU output range is defined as
always taking the whole 16-bit range in:
[...]
366 build_append_int_noprefix(table_data, irq + 1, 4); /* PRI */
367 build_append_int_noprefix(table_data, irq + 3, 4); /* GERR */
368 build_append_int_noprefix(table_data, irq + 2, 4); /* Sync */
369 build_append_int_noprefix(table_data, 0, 4); /* Proximity domain */
370 /* DeviceID mapping index (ignored since interrupts are GSIV based) */
371 build_append_int_noprefix(table_data, 0, 4);
372
373 /* output IORT node is the ITS group node (the first node) */
374 build_iort_id_mapping(table_data, 0, 0x10000, IORT_NODE_OFFSET); <=========== HERE
375 }
376
377 /* Table 17 Root Complex Node */
I think that's ok since ITS can disambiguate the Device IDs from SMMU and from RC.
> /*
> * Split the whole RIDs by mapping from RC to SMMU,
> * build the ID mapping from RC to ITS directly.
> */
> for (i = 0; i < smmu_idmaps->len; i++) {
> idmap = &g_array_index(smmu_idmaps, AcpiIortIdMapping, i);
>
> if (next_range.input_base < idmap->input_base) {
> next_range.id_count = idmap->input_base -
> next_range.input_base;
> g_array_append_val(its_idmaps, next_range);
> }
>
> next_range.input_base = idmap->input_base + idmap->id_count;
> }
>
> is not crystal clear but it looks like filling the holes into its_idmap.
k, I'll create a helper as you suggested in 7/8 to populate the its_idmaps.
> Besides there is another "Its group" instance to be replaced by ITS Group
Pardon, what do you mean? Another text to be replaced or it's just a comment
about the "else" block when smmu is off?
Thanks!
Cheers,
Gustavo
> Eric
>
>> for (i = 0; i < its_idmaps->len; i++) {
>> range = &g_array_index(its_idmaps, AcpiIortIdMapping, i);
>> - /* output IORT node is the ITS group node (the first node) */
>> + /* Output IORT node is the ITS Group node (the first node). */
>> build_iort_id_mapping(table_data, range->input_base,
>> range->id_count, IORT_NODE_OFFSET);
>> }
>> } else {
>> - /* output IORT node is the ITS group node (the first node) */
>> + /*
>> + * Map bypassed RIDs (input) (don't go through the SMMU) to ITS Group
>> + * nodes: RC -> ITS.
>> + * Output IORT node is the ITS Group node (the first node).
>> + */
>> build_iort_id_mapping(table_data, 0, 0x10000, IORT_NODE_OFFSET);
>> }
>>
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 4/8] hw/arm/virt-acpi-build: Fix comment in build_iort
2025-06-19 17:07 ` Gustavo Romero
@ 2025-06-20 6:52 ` Eric Auger
0 siblings, 0 replies; 26+ messages in thread
From: Eric Auger @ 2025-06-20 6:52 UTC (permalink / raw)
To: Gustavo Romero, qemu-devel, philmd, mst
Cc: qemu-arm, alex.bennee, udo, ajones, peter.maydell, imammedo,
anisinha
Hi Gustavo,
On 6/19/25 7:07 PM, Gustavo Romero wrote:
> Hi Eric,
>
> On 6/17/25 10:22, Eric Auger wrote:
>> Hi Gustavo,
>>
>> On 6/16/25 3:18 PM, Gustavo Romero wrote:
>>> The comment about the mapping from SMMU to ITS is incorrect and it
>>> reads
>>> "RC -> ITS". The code in question actually maps SMMU -> ITS, so the
>>> mapping in question is not direct. The direct mapping, i.e., RC -> ITS,
>>> is handled a bit further down in the code, in the else block, and we
>>> take the opportunity to update that as well, adding "RC -> ITS" to the
>>> text so it's easier to see it's the direct map to the ITS Group node.
>>>
>>> Signed-off-by Gustavo Romero <gustavo.romero@linaro.org>
>>> ---
>>> hw/arm/virt-acpi-build.c | 14 +++++++++-----
>>> 1 file changed, 9 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>>> index 9eee284c80..6990bce3bb 100644
>>> --- a/hw/arm/virt-acpi-build.c
>>> +++ b/hw/arm/virt-acpi-build.c
>>> @@ -407,23 +407,27 @@ build_iort(GArray *table_data, BIOSLinker
>>> *linker, VirtMachineState *vms)
>>> if (vms->iommu == VIRT_IOMMU_SMMUV3) {
>>> AcpiIortIdMapping *range;
>>> - /* translated RIDs connect to SMMUv3 node: RC -> SMMUv3
>>> -> ITS */
>>> + /* Map RIDs (input) from RC to SMMUv3 nodes: RC -> SMMUv3. */
>> yes this is what the code builds at this location.
>>> for (i = 0; i < smmu_idmaps->len; i++) {
>>> range = &g_array_index(smmu_idmaps, AcpiIortIdMapping,
>>> i);
>>> - /* output IORT node is the smmuv3 node */
>>> + /* Output IORT node is the SMMUv3 node. */
>>> build_iort_id_mapping(table_data, range->input_base,
>>> range->id_count, smmu_offset);
>>> }
>>> - /* bypassed RIDs connect to ITS group node directly: RC
>>> -> ITS */
>>> + /* Map DeviceIDs (input) from SMMUv3 to ITS Group nodes:
>>> SMMU -> ITS. */
>> But here I am confused. To me the its_idmaps matches the idmap between
>> RC and ITS. I understand this is built from holes left by bypassed
>> buses. See the build_iort() implementation. The comment at
>
> ah, thanks! I see. Indeed, it's mapping the RC -> ITS, not the SMMU ->
> ITS.
> I'll fix it in v5.
>
> One thing that confused me, which I think is actually ok, is that the
> output ID range from SMMU 0x000-0xFFFF) overlaps the output ID range
> from the RC (e.g. 0x100-0xFFFF) because the SMMU output range is
> defined as
> always taking the whole 16-bit range in:
Agreed. However I think it is fine because those are ID mappings between
different input/output space pairs.
RC -> ITS
SMMU -> ITS
>
> [...]
> 366 build_append_int_noprefix(table_data, irq + 1, 4); /*
> PRI */
> 367 build_append_int_noprefix(table_data, irq + 3, 4); /*
> GERR */
> 368 build_append_int_noprefix(table_data, irq + 2, 4); /*
> Sync */
> 369 build_append_int_noprefix(table_data, 0, 4); /*
> Proximity domain */
> 370 /* DeviceID mapping index (ignored since interrupts are
> GSIV based) */
> 371 build_append_int_noprefix(table_data, 0, 4);
> 372
> 373 /* output IORT node is the ITS group node (the first
> node) */
> 374 build_iort_id_mapping(table_data, 0, 0x10000,
> IORT_NODE_OFFSET); <=========== HERE
> 375 }
> 376
> 377 /* Table 17 Root Complex Node */
>
>
> I think that's ok since ITS can disambiguate the Device IDs from SMMU
> and from RC.
>
>
>> /*
>> * Split the whole RIDs by mapping from RC to SMMU,
>> * build the ID mapping from RC to ITS directly.
>> */
>> for (i = 0; i < smmu_idmaps->len; i++) {
>> idmap = &g_array_index(smmu_idmaps, AcpiIortIdMapping, i);
>>
>> if (next_range.input_base < idmap->input_base) {
>> next_range.id_count = idmap->input_base -
>> next_range.input_base;
>> g_array_append_val(its_idmaps, next_range);
>> }
>>
>> next_range.input_base = idmap->input_base +
>> idmap->id_count;
>> }
>>
>> is not crystal clear but it looks like filling the holes into its_idmap.
>
> k, I'll create a helper as you suggested in 7/8 to populate the
> its_idmaps.
>
> > Besides there is another "Its group" instance to be replaced by ITS
> Group
>
> Pardon, what do you mean? Another text to be replaced or it's just a
> comment
> about the "else" block when smmu is off?
sorry, I meant another string to be replaced in the file
Cheers
Eric
>
> Thanks!
>
>
> Cheers,
> Gustavo
>
>> Eric
>>
>>> for (i = 0; i < its_idmaps->len; i++) {
>>> range = &g_array_index(its_idmaps, AcpiIortIdMapping, i);
>>> - /* output IORT node is the ITS group node (the first
>>> node) */
>>> + /* Output IORT node is the ITS Group node (the first
>>> node). */
>>> build_iort_id_mapping(table_data, range->input_base,
>>> range->id_count, IORT_NODE_OFFSET);
>>> }
>>> } else {
>>> - /* output IORT node is the ITS group node (the first node) */
>>> + /*
>>> + * Map bypassed RIDs (input) (don't go through the SMMU) to
>>> ITS Group
>>> + * nodes: RC -> ITS.
>>> + * Output IORT node is the ITS Group node (the first node).
>>> + */
>>> build_iort_id_mapping(table_data, 0, 0x10000,
>>> IORT_NODE_OFFSET);
>>> }
>>>
>>
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH-for-10.1 v4 0/8] hw/arm: GIC 'its=off' ACPI table fixes
2025-06-17 13:26 ` Eric Auger
@ 2025-06-23 18:54 ` Gustavo Romero
0 siblings, 0 replies; 26+ messages in thread
From: Gustavo Romero @ 2025-06-23 18:54 UTC (permalink / raw)
To: eric.auger, qemu-devel
Cc: qemu-arm, alex.bennee, udo, ajones, peter.maydell, imammedo,
anisinha, Philippe Mathieu-Daudé, Michael S . Tsirkin,
Shameerali Kolothum Thodi
Hi Eric,
On 6/17/25 10:26, Eric Auger wrote:
> Hi Gustavo,
>
> On 6/17/25 3:01 PM, Gustavo Romero wrote:
>> Hi Eric,
>>
>> Thanks a lot for doing a first pass on this series!
>>
>> On 6/17/25 06:35, Eric Auger wrote:
>>> Hi Gustavo,
>>>
>>> On 6/16/25 3:18 PM, Gustavo Romero wrote:
>>>> Since v2:
>>>> - Fixed no_tcg_its inverted logic (rth)
>>>>
>>>> Since v3:
>>>> - Fixed remappings in the IORT table when ITS is no present
>>>> - Rebased on master and resoled conflics, like no more "no_its"
>>>> flag in VirtMachineClass
>>>> - Dropped patch 1/9 because we actually want the instance flags,
>>>> not only the class flags, and the instance flags are the ones
>>>> to be used often when deciding about the presence/absence of a
>>>> machine feature, instead of the negated class flags ("no_*")
>>>> - Adapted the other patches that depended on 1/9
>>>> - Dropped patch 4/9 in favor of using the instance flag for
>>>> checking if ITS is on or off
>>>> - Simplified VM options for the new "its=off" test
>>>>
>>>> v1: https://lists.gnu.org/archive/html/qemu-devel/2025-03/msg07080.html
>>>> v2:
>>>> https://lists.gnu.org/archive/html/qemu-devel/2025-04/msg00495.html
>>>> (Patches 6/14 -> 14/14 in the series)
>>>> v3: https://lists.gnu.org/archive/html/qemu-devel/2025-04/msg00567.html
>>>>
>>>> Fix ACPI tables for '-M its=off' CLI option and resolve the issue:
>>>>
>>>> https://gitlab.com/qemu-project/qemu/-/issues/2886
>>>
>>> One first comment is that this series will collide with Shameer's SMMU
>>> multi instance series which has been lunder review for quite some time
>>> (adding him in TO):
>>>
>>> I think it may be more future proof if you could rebase on it - I know
>>> it is a pain ;-( -. Or if sbdy objects for Shameer's series please raise
>>> your voice now.
>>>
>>> [PATCH v4 0/7] hw/arm/virt: Add support for user creatable SMMUv3
>>> device
>>> <https://lore.kernel.org/all/20250613144449.60156-1-shameerali.kolothum.thodi@huawei.com/#r>
>>>
>>> https://lore.kernel.org/all/20250613144449.60156-1-shameerali.kolothum.thodi@huawei.com/
>>>
>>
>> ayayay, life is never that easy! :)
>>
>> Thanks for point that out. Sure, I can rebase it on Shameer's series,
>> but also
>> I'd like to have this ITS fix for 10.1, so I think it's a matter of
>> understanding
>> if Shameer's series will make the 10.1 release (thanks for asking the
>> reviewers if they
>> have any current objection so we have an idea if it's close to get
>> accepted
>> or not)?
> Peter was the most annoyed by the usage of -device arm-smmuv3 option
> line. We'd better ask him.
I've talked to Peter via IRC and he said that since this is a fix it could
be merged first than Shameer's SMMU series.
In any case, I'm wondering if you could review v5 of this series, which I
published earlier today, specially patch 8/9 "Fix ACPI IORT and MADT tables
when its=off", since it's the main patch in the series and currently it has
no R-bs. I've addressed all your comments on v4.
Thanks a lot!
Cheers,
Gustavo
[v5] https://lists.nongnu.org/archive/html/qemu-devel/2025-06/msg03793.html
> On my end I don't see how we can achieve this more elegantly.
>>
>> Meanwhile, I'm pretty keen on if I'm correctly generating the IORT
>> table pruned from ITS
>> (patch 7/8 in this series), like, are the remappings for the RC and
>> SMMU nodes correct? That
>> would make me more comfortable to start working on a rebase.
> sure looking at it...
>
> Eric
>>
>>
>>> Also I understood Shameer intended to write some new bios-tables-test.
>>
>> I see.
>>
>>
>> Cheers,
>> Gustavo
>>
>
^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2025-06-23 18:55 UTC | newest]
Thread overview: 26+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-06-16 13:18 [PATCH-for-10.1 v4 0/8] hw/arm: GIC 'its=off' ACPI table fixes Gustavo Romero
2025-06-16 13:18 ` [PATCH v4 1/8] hw/intc/gicv3_its: Do not check its_class_name() Gustavo Romero
2025-06-16 13:33 ` Philippe Mathieu-Daudé
2025-06-16 13:57 ` Gustavo Romero
2025-06-16 13:18 ` [PATCH v4 2/8] hw/arm/virt: Simplify logic for setting instance's 'tcg_its' variable Gustavo Romero
2025-06-17 9:46 ` Eric Auger
2025-06-16 13:18 ` [PATCH v4 3/8] hw/arm/virt: Simplify create_its() Gustavo Romero
2025-06-16 13:18 ` [PATCH v4 4/8] hw/arm/virt-acpi-build: Fix comment in build_iort Gustavo Romero
2025-06-17 13:22 ` Eric Auger
2025-06-19 17:07 ` Gustavo Romero
2025-06-20 6:52 ` Eric Auger
2025-06-16 13:18 ` [PATCH v4 5/8] qtest/bios-tables-test: Add test for when ITS is off on aarch64 Gustavo Romero
2025-06-17 13:34 ` Eric Auger
2025-06-17 15:12 ` Gustavo Romero
2025-06-17 15:51 ` Eric Auger
2025-06-17 16:01 ` Gustavo Romero
2025-06-17 17:01 ` Eric Auger
2025-06-17 16:06 ` Gustavo Romero
2025-06-16 13:18 ` [PATCH v4 6/8] qtest/bios-tables-test: Add blobs for its=off test " Gustavo Romero
2025-06-16 13:18 ` [PATCH v4 7/8] hw/arm/virt-acpi-build: Fix ACPI IORT and MADT tables when its=off Gustavo Romero
2025-06-17 14:04 ` Eric Auger
2025-06-16 13:18 ` [PATCH v4 8/8] qtest/bios-tables-test: Update blobs for its=off test on aarch64 Gustavo Romero
2025-06-17 9:35 ` [PATCH-for-10.1 v4 0/8] hw/arm: GIC 'its=off' ACPI table fixes Eric Auger
2025-06-17 13:01 ` Gustavo Romero
2025-06-17 13:26 ` Eric Auger
2025-06-23 18:54 ` Gustavo Romero
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).