* [PATCH-for-10.1 v3 0/9] hw/arm: GIC ITS=off ACPI tables fixes
@ 2025-04-03 20:40 Philippe Mathieu-Daudé
2025-04-03 20:40 ` [PATCH-for-10.1 v3 1/9] hw/arm/virt: Remove pointless VirtMachineState::tcg_its field Philippe Mathieu-Daudé
` (9 more replies)
0 siblings, 10 replies; 30+ messages in thread
From: Philippe Mathieu-Daudé @ 2025-04-03 20:40 UTC (permalink / raw)
To: qemu-devel
Cc: Andrew Jones, Alex Bennée, Michael S. Tsirkin, qemu-arm,
Udo Steinberg, Shannon Zhao, Gustavo Romero, Igor Mammedov,
Peter Maydell, Ani Sinha, Philippe Mathieu-Daudé
Since v2:
- Fixed no_tcg_its inverted logic (rth)
Fix ACPI tables for '-M its=off' CLI option.
Regards,
Phil.
Philippe Mathieu-Daudé (9):
hw/arm/virt: Remove pointless VirtMachineState::tcg_its field
hw/intc/gicv3_its: Do not check its_class_name() for NULL
hw/arm/virt: Simplify create_its()
hw/arm/virt-acpi: Factor its_enabled() helper out
qtest/bios-tables-test: Add test for -M virt,its=off
qtest/bios-tables-test: Whitelist aarch64/virt 'its_off' variant blobs
hw/arm/virt-acpi: Always build IORT table (even with GIC ITS disabled)
hw/arm/virt-acpi: Do not advertise disabled GIC ITS
qtest/bios-tables-test: Update aarch64/virt 'its_off' variant blobs
include/hw/arm/virt.h | 1 -
include/hw/intc/arm_gicv3_its_common.h | 2 +-
hw/arm/virt-acpi-build.c | 48 +++++++++++++---------
hw/arm/virt.c | 23 ++++-------
tests/qtest/bios-tables-test.c | 22 ++++++++++
tests/data/acpi/aarch64/virt/APIC.its_off | Bin 0 -> 164 bytes
tests/data/acpi/aarch64/virt/FACP.its_off | Bin 0 -> 276 bytes
tests/data/acpi/aarch64/virt/IORT.its_off | Bin 0 -> 212 bytes
8 files changed, 59 insertions(+), 37 deletions(-)
create mode 100644 tests/data/acpi/aarch64/virt/APIC.its_off
create mode 100644 tests/data/acpi/aarch64/virt/FACP.its_off
create mode 100644 tests/data/acpi/aarch64/virt/IORT.its_off
--
2.47.1
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH-for-10.1 v3 1/9] hw/arm/virt: Remove pointless VirtMachineState::tcg_its field
2025-04-03 20:40 [PATCH-for-10.1 v3 0/9] hw/arm: GIC ITS=off ACPI tables fixes Philippe Mathieu-Daudé
@ 2025-04-03 20:40 ` Philippe Mathieu-Daudé
2025-04-07 11:57 ` Eric Auger
2025-04-03 20:40 ` [PATCH-for-10.1 v3 2/9] hw/intc/gicv3_its: Do not check its_class_name() for NULL Philippe Mathieu-Daudé
` (8 subsequent siblings)
9 siblings, 1 reply; 30+ messages in thread
From: Philippe Mathieu-Daudé @ 2025-04-03 20:40 UTC (permalink / raw)
To: qemu-devel
Cc: Andrew Jones, Alex Bennée, Michael S. Tsirkin, qemu-arm,
Udo Steinberg, Shannon Zhao, Gustavo Romero, Igor Mammedov,
Peter Maydell, Ani Sinha, Philippe Mathieu-Daudé
VirtMachineState::tcg_its has the negated logic value of
VirtMachineClass::no_tcg_its. Directly use the latter,
removing the former.
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
include/hw/arm/virt.h | 1 -
hw/arm/virt.c | 13 +++++--------
2 files changed, 5 insertions(+), 9 deletions(-)
diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
index c8e94e6aedc..17c160429ea 100644
--- a/include/hw/arm/virt.h
+++ b/include/hw/arm/virt.h
@@ -150,7 +150,6 @@ struct VirtMachineState {
bool highmem_mmio;
bool highmem_redists;
bool its;
- bool tcg_its;
bool virt;
bool ras;
bool mte;
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index a96452f17a4..177e9e0eadb 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -710,11 +710,12 @@ static inline DeviceState *create_acpi_ged(VirtMachineState *vms)
static void create_its(VirtMachineState *vms)
{
+ VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
const char *itsclass = its_class_name();
DeviceState *dev;
if (!strcmp(itsclass, "arm-gicv3-its")) {
- if (!vms->tcg_its) {
+ if (vmc->no_tcg_its) {
itsclass = NULL;
}
}
@@ -831,7 +832,9 @@ static void create_gic(VirtMachineState *vms, MemoryRegion *mem)
redist_region_count);
if (!kvm_irqchip_in_kernel()) {
- if (vms->tcg_its) {
+ VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
+
+ if (!vmc->no_tcg_its) {
object_property_set_link(OBJECT(vms->gic), "sysmem",
OBJECT(mem), &error_fatal);
qdev_prop_set_bit(vms->gic, "has-lpi", true);
@@ -3357,12 +3360,6 @@ static void virt_instance_init(Object *obj)
} else {
/* Default allows ITS instantiation */
vms->its = true;
-
- if (vmc->no_tcg_its) {
- vms->tcg_its = false;
- } else {
- vms->tcg_its = true;
- }
}
/* Default disallows iommu instantiation */
--
2.47.1
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH-for-10.1 v3 2/9] hw/intc/gicv3_its: Do not check its_class_name() for NULL
2025-04-03 20:40 [PATCH-for-10.1 v3 0/9] hw/arm: GIC ITS=off ACPI tables fixes Philippe Mathieu-Daudé
2025-04-03 20:40 ` [PATCH-for-10.1 v3 1/9] hw/arm/virt: Remove pointless VirtMachineState::tcg_its field Philippe Mathieu-Daudé
@ 2025-04-03 20:40 ` Philippe Mathieu-Daudé
2025-04-07 11:57 ` Eric Auger
2025-04-03 20:40 ` [PATCH-for-10.1 v3 3/9] hw/arm/virt: Simplify create_its() Philippe Mathieu-Daudé
` (7 subsequent siblings)
9 siblings, 1 reply; 30+ messages in thread
From: Philippe Mathieu-Daudé @ 2025-04-03 20:40 UTC (permalink / raw)
To: qemu-devel
Cc: Andrew Jones, Alex Bennée, Michael S. Tsirkin, qemu-arm,
Udo Steinberg, Shannon Zhao, Gustavo Romero, Igor Mammedov,
Peter Maydell, Ani Sinha, Philippe Mathieu-Daudé,
Richard Henderson
Since commit cc5e719e2c8 ("kvm: require KVM_CAP_SIGNAL_MSI"),
its_class_name() single implementation doesn't return NULL
anymore. Update the prototype docstring, and remove the
pointless checks.
Reported-by: Gustavo Romero <gustavo.romero@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
include/hw/intc/arm_gicv3_its_common.h | 2 +-
hw/arm/virt-acpi-build.c | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/include/hw/intc/arm_gicv3_its_common.h b/include/hw/intc/arm_gicv3_its_common.h
index 7dc712b38d2..3c7b543b018 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);
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 3ac8f8e1786..9b7fc99f170 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -741,7 +741,7 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
memmap[VIRT_HIGH_GIC_REDIST2].size);
}
- if (its_class_name() && !vmc->no_its) {
+ if (!vmc->no_its) {
/*
* ACPI spec, Revision 6.0 Errata A
* (original 6.0 definition has invalid Length)
@@ -973,7 +973,7 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
vms->oem_table_id);
}
- if (its_class_name() && !vmc->no_its) {
+ if (!vmc->no_its) {
acpi_add_table(table_offsets, tables_blob);
build_iort(tables_blob, tables->linker, vms);
}
--
2.47.1
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH-for-10.1 v3 3/9] hw/arm/virt: Simplify create_its()
2025-04-03 20:40 [PATCH-for-10.1 v3 0/9] hw/arm: GIC ITS=off ACPI tables fixes Philippe Mathieu-Daudé
2025-04-03 20:40 ` [PATCH-for-10.1 v3 1/9] hw/arm/virt: Remove pointless VirtMachineState::tcg_its field Philippe Mathieu-Daudé
2025-04-03 20:40 ` [PATCH-for-10.1 v3 2/9] hw/intc/gicv3_its: Do not check its_class_name() for NULL Philippe Mathieu-Daudé
@ 2025-04-03 20:40 ` Philippe Mathieu-Daudé
2025-04-04 2:59 ` Gustavo Romero
2025-04-07 12:05 ` Eric Auger
2025-04-03 20:40 ` [PATCH-for-10.1 v3 4/9] hw/arm/virt-acpi: Factor its_enabled() helper out Philippe Mathieu-Daudé
` (6 subsequent siblings)
9 siblings, 2 replies; 30+ messages in thread
From: Philippe Mathieu-Daudé @ 2025-04-03 20:40 UTC (permalink / raw)
To: qemu-devel
Cc: Andrew Jones, Alex Bennée, Michael S. Tsirkin, qemu-arm,
Udo Steinberg, Shannon Zhao, Gustavo Romero, Igor Mammedov,
Peter Maydell, Ani Sinha, Philippe Mathieu-Daudé,
Richard Henderson
No need to strstr() check the class name when we can
use kvm_irqchip_in_kernel().
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
hw/arm/virt.c | 12 +++---------
1 file changed, 3 insertions(+), 9 deletions(-)
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 177e9e0eadb..326986deb16 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -711,21 +711,15 @@ static inline DeviceState *create_acpi_ged(VirtMachineState *vms)
static void create_its(VirtMachineState *vms)
{
VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
- const char *itsclass = its_class_name();
DeviceState *dev;
- if (!strcmp(itsclass, "arm-gicv3-its")) {
- if (vmc->no_tcg_its) {
- itsclass = NULL;
- }
- }
-
- if (!itsclass) {
+ assert(!vmc->no_its);
+ if (!kvm_irqchip_in_kernel() && vmc->no_tcg_its) {
/* Do nothing if not supported */
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.47.1
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH-for-10.1 v3 4/9] hw/arm/virt-acpi: Factor its_enabled() helper out
2025-04-03 20:40 [PATCH-for-10.1 v3 0/9] hw/arm: GIC ITS=off ACPI tables fixes Philippe Mathieu-Daudé
` (2 preceding siblings ...)
2025-04-03 20:40 ` [PATCH-for-10.1 v3 3/9] hw/arm/virt: Simplify create_its() Philippe Mathieu-Daudé
@ 2025-04-03 20:40 ` Philippe Mathieu-Daudé
2025-04-07 12:08 ` Eric Auger
2025-04-03 20:40 ` [PATCH-for-10.1 v3 5/9] qtest/bios-tables-test: Add test for -M virt, its=off Philippe Mathieu-Daudé
` (5 subsequent siblings)
9 siblings, 1 reply; 30+ messages in thread
From: Philippe Mathieu-Daudé @ 2025-04-03 20:40 UTC (permalink / raw)
To: qemu-devel
Cc: Andrew Jones, Alex Bennée, Michael S. Tsirkin, qemu-arm,
Udo Steinberg, Shannon Zhao, Gustavo Romero, Igor Mammedov,
Peter Maydell, Ani Sinha, Philippe Mathieu-Daudé,
Richard Henderson
GIC ITS is checked for the MADT and IORT tables.
Factor the checks out to the its_enabled() helper.
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Gustavo Romero <gustavo.romero@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
hw/arm/virt-acpi-build.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 9b7fc99f170..1c389ef5cdb 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -208,6 +208,13 @@ static void acpi_dsdt_add_tpm(Aml *scope, VirtMachineState *vms)
#define ROOT_COMPLEX_ENTRY_SIZE 36
#define IORT_NODE_OFFSET 48
+static bool its_enabled(VirtMachineState *vms)
+{
+ VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
+
+ return !vmc->no_its;
+}
+
/*
* Append an ID mapping entry as described by "Table 4 ID mapping format" in
* "IO Remapping Table System Software on ARM Platforms", Chapter 3.
@@ -670,7 +677,6 @@ static void
build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
{
int i;
- VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
const MemMapEntry *memmap = vms->memmap;
AcpiTable table = { .sig = "APIC", .rev = 4, .oem_id = vms->oem_id,
.oem_table_id = vms->oem_table_id };
@@ -741,7 +747,7 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
memmap[VIRT_HIGH_GIC_REDIST2].size);
}
- if (!vmc->no_its) {
+ if (its_enabled(vms)) {
/*
* ACPI spec, Revision 6.0 Errata A
* (original 6.0 definition has invalid Length)
@@ -973,7 +979,7 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
vms->oem_table_id);
}
- if (!vmc->no_its) {
+ if (its_enabled(vms)) {
acpi_add_table(table_offsets, tables_blob);
build_iort(tables_blob, tables->linker, vms);
}
--
2.47.1
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH-for-10.1 v3 5/9] qtest/bios-tables-test: Add test for -M virt, its=off
2025-04-03 20:40 [PATCH-for-10.1 v3 0/9] hw/arm: GIC ITS=off ACPI tables fixes Philippe Mathieu-Daudé
` (3 preceding siblings ...)
2025-04-03 20:40 ` [PATCH-for-10.1 v3 4/9] hw/arm/virt-acpi: Factor its_enabled() helper out Philippe Mathieu-Daudé
@ 2025-04-03 20:40 ` Philippe Mathieu-Daudé
2025-04-04 3:00 ` [PATCH-for-10.1 v3 5/9] qtest/bios-tables-test: Add test for -M virt,its=off Gustavo Romero
2025-04-03 20:40 ` [PATCH-for-10.1 v3 6/9] qtest/bios-tables-test: Whitelist aarch64/virt 'its_off' variant blobs Philippe Mathieu-Daudé
` (4 subsequent siblings)
9 siblings, 1 reply; 30+ messages in thread
From: Philippe Mathieu-Daudé @ 2025-04-03 20:40 UTC (permalink / raw)
To: qemu-devel
Cc: Andrew Jones, Alex Bennée, Michael S. Tsirkin, qemu-arm,
Udo Steinberg, Shannon Zhao, Gustavo Romero, Igor Mammedov,
Peter Maydell, Ani Sinha, Philippe Mathieu-Daudé
Add the use case reported as issue #2886 [*]. The test
passes while it shouldn't. We are going to fix that in
the following commits.
[*] https://gitlab.com/qemu-project/qemu/-/issues/2886
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
tests/qtest/bios-tables-test.c | 21 +++++++++++++++++++++
1 file changed, 21 insertions(+)
diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
index 0a333ec4353..baaf199e01c 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",
+ .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 virtualization=on,secure=off "
+ "-M gic-version=max,its=off,iommu=smmuv3", &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.47.1
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH-for-10.1 v3 6/9] qtest/bios-tables-test: Whitelist aarch64/virt 'its_off' variant blobs
2025-04-03 20:40 [PATCH-for-10.1 v3 0/9] hw/arm: GIC ITS=off ACPI tables fixes Philippe Mathieu-Daudé
` (4 preceding siblings ...)
2025-04-03 20:40 ` [PATCH-for-10.1 v3 5/9] qtest/bios-tables-test: Add test for -M virt, its=off Philippe Mathieu-Daudé
@ 2025-04-03 20:40 ` Philippe Mathieu-Daudé
2025-04-04 3:01 ` Gustavo Romero
2025-04-03 20:40 ` [PATCH-for-10.1 v3 7/9] hw/arm/virt-acpi: Always build IORT table (even with GIC ITS disabled) Philippe Mathieu-Daudé
` (3 subsequent siblings)
9 siblings, 1 reply; 30+ messages in thread
From: Philippe Mathieu-Daudé @ 2025-04-03 20:40 UTC (permalink / raw)
To: qemu-devel
Cc: Andrew Jones, Alex Bennée, Michael S. Tsirkin, qemu-arm,
Udo Steinberg, Shannon Zhao, Gustavo Romero, Igor Mammedov,
Peter Maydell, Ani Sinha, Philippe Mathieu-Daudé
We are going to fix the test_acpi_aarch64_virt_tcg_its_off()
test. In preparation, copy the ACPI tables which will be
altered as 'its_off' variants, and whitelist them.
Reviewed-by: Gustavo Romero <gustavo.romero@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
tests/qtest/bios-tables-test-allowed-diff.h | 3 +++
tests/qtest/bios-tables-test.c | 1 +
tests/data/acpi/aarch64/virt/APIC.its_off | Bin 0 -> 184 bytes
tests/data/acpi/aarch64/virt/FACP.its_off | Bin 0 -> 276 bytes
tests/data/acpi/aarch64/virt/IORT.its_off | Bin 0 -> 236 bytes
5 files changed, 4 insertions(+)
create mode 100644 tests/data/acpi/aarch64/virt/APIC.its_off
create mode 100644 tests/data/acpi/aarch64/virt/FACP.its_off
create mode 100644 tests/data/acpi/aarch64/virt/IORT.its_off
diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
index dfb8523c8bf..3421dd5adf3 100644
--- a/tests/qtest/bios-tables-test-allowed-diff.h
+++ b/tests/qtest/bios-tables-test-allowed-diff.h
@@ -1 +1,4 @@
/* List of comma-separated changed AML files to ignore */
+"tests/data/acpi/aarch64/virt/APIC.its_off",
+"tests/data/acpi/aarch64/virt/FACP.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 baaf199e01c..55366bf4956 100644
--- a/tests/qtest/bios-tables-test.c
+++ b/tests/qtest/bios-tables-test.c
@@ -2151,6 +2151,7 @@ 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",
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..c37d05d6e05805304f10afe73eb7cb9100fcccfa
GIT binary patch
literal 184
zcmZ<^@O0k6z`($=+{xeBBUr&HBEVSz2pEB4AU24G0Uik$i-7~iVgWL^17JJ`2AFzr
bgb+@aBn}xq0gwb2)Q)cq{30-g9B_L93G4|0
literal 0
HcmV?d00001
diff --git a/tests/data/acpi/aarch64/virt/FACP.its_off b/tests/data/acpi/aarch64/virt/FACP.its_off
new file mode 100644
index 0000000000000000000000000000000000000000..606dac3fe4b55c31fd68b25d3a4127eeef227434
GIT binary patch
literal 276
zcmZ>BbPf<<WME(uaq@Te2v%^42yj*a0-z8Bhz+8t3j|P&V`N}P6&N^PpsQ~v$aVnZ
CVg~^L
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
--
2.47.1
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH-for-10.1 v3 7/9] hw/arm/virt-acpi: Always build IORT table (even with GIC ITS disabled)
2025-04-03 20:40 [PATCH-for-10.1 v3 0/9] hw/arm: GIC ITS=off ACPI tables fixes Philippe Mathieu-Daudé
` (5 preceding siblings ...)
2025-04-03 20:40 ` [PATCH-for-10.1 v3 6/9] qtest/bios-tables-test: Whitelist aarch64/virt 'its_off' variant blobs Philippe Mathieu-Daudé
@ 2025-04-03 20:40 ` Philippe Mathieu-Daudé
2025-04-04 3:02 ` Gustavo Romero
2025-04-07 13:30 ` Eric Auger
2025-04-03 20:40 ` [PATCH-for-10.1 v3 8/9] hw/arm/virt-acpi: Do not advertise disabled GIC ITS Philippe Mathieu-Daudé
` (2 subsequent siblings)
9 siblings, 2 replies; 30+ messages in thread
From: Philippe Mathieu-Daudé @ 2025-04-03 20:40 UTC (permalink / raw)
To: qemu-devel
Cc: Andrew Jones, Alex Bennée, Michael S. Tsirkin, qemu-arm,
Udo Steinberg, Shannon Zhao, Gustavo Romero, Igor Mammedov,
Peter Maydell, Ani Sinha, Philippe Mathieu-Daudé
Citing Gustavo [*]:
Gating IORT table generation entirely based on the presence
of ITS looks wrong because IORT table has data beyond GIC ITS,
like for SMMUv3 etc..
[*] https://lore.kernel.org/qemu-devel/bae6e29a-7290-47d2-8caf-14702ee091f0@linaro.org/
Reported-by: Gustavo Romero <gustavo.romero@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
hw/arm/virt-acpi-build.c | 38 +++++++++++++++++++++-----------------
1 file changed, 21 insertions(+), 17 deletions(-)
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 1c389ef5cdb..e7e27951cb9 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -287,6 +287,7 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
uint32_t id = 0;
GArray *smmu_idmaps = g_array_new(false, true, sizeof(AcpiIortIdMapping));
GArray *its_idmaps = g_array_new(false, true, sizeof(AcpiIortIdMapping));
+ bool has_its = its_enabled(vms);
AcpiTable table = { .sig = "IORT", .rev = 3, .oem_id = vms->oem_id,
.oem_table_id = vms->oem_table_id };
@@ -325,9 +326,12 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
nb_nodes = 3; /* RC, ITS, SMMUv3 */
rc_mapping_count = smmu_idmaps->len + its_idmaps->len;
- } else {
+ } else if (has_its) {
nb_nodes = 2; /* RC, ITS */
rc_mapping_count = 1;
+ } else {
+ nb_nodes = 1; /* RC */
+ rc_mapping_count = 1;
}
/* Number of IORT Nodes */
build_append_int_noprefix(table_data, nb_nodes, 4);
@@ -336,17 +340,19 @@ 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 (has_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;
@@ -429,7 +435,7 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
build_iort_id_mapping(table_data, range->input_base,
range->id_count, IORT_NODE_OFFSET);
}
- } else {
+ } else if (has_its) {
/* output IORT node is the ITS group node (the first node) */
build_iort_id_mapping(table_data, 0, 0x10000, IORT_NODE_OFFSET);
}
@@ -979,10 +985,8 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
vms->oem_table_id);
}
- if (its_enabled(vms)) {
- 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) {
--
2.47.1
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH-for-10.1 v3 8/9] hw/arm/virt-acpi: Do not advertise disabled GIC ITS
2025-04-03 20:40 [PATCH-for-10.1 v3 0/9] hw/arm: GIC ITS=off ACPI tables fixes Philippe Mathieu-Daudé
` (6 preceding siblings ...)
2025-04-03 20:40 ` [PATCH-for-10.1 v3 7/9] hw/arm/virt-acpi: Always build IORT table (even with GIC ITS disabled) Philippe Mathieu-Daudé
@ 2025-04-03 20:40 ` Philippe Mathieu-Daudé
2025-04-03 20:40 ` [PATCH-for-10.1 v3 9/9] qtest/bios-tables-test: Update aarch64/virt 'its_off' variant blobs Philippe Mathieu-Daudé
2025-04-15 8:19 ` [PATCH-for-10.1 v3 0/9] hw/arm: GIC ITS=off ACPI tables fixes Philippe Mathieu-Daudé
9 siblings, 0 replies; 30+ messages in thread
From: Philippe Mathieu-Daudé @ 2025-04-03 20:40 UTC (permalink / raw)
To: qemu-devel
Cc: Andrew Jones, Alex Bennée, Michael S. Tsirkin, qemu-arm,
Udo Steinberg, Shannon Zhao, Gustavo Romero, Igor Mammedov,
Peter Maydell, Ani Sinha, Philippe Mathieu-Daudé
GIC ITS can be disabled at runtime using '-M its=off',
which sets VirtMachineState::its = false. Check this
field to avoid advertising the ITS in the MADT table.
Reported-by: Udo Steinberg <udo@hypervisor.org>
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2886
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Gustavo Romero <gustavo.romero@linaro.org>
---
hw/arm/virt-acpi-build.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index e7e27951cb9..38a9e6fe0c5 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -212,7 +212,7 @@ static bool its_enabled(VirtMachineState *vms)
{
VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
- return !vmc->no_its;
+ return !vmc->no_its && vms->its;
}
/*
--
2.47.1
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH-for-10.1 v3 9/9] qtest/bios-tables-test: Update aarch64/virt 'its_off' variant blobs
2025-04-03 20:40 [PATCH-for-10.1 v3 0/9] hw/arm: GIC ITS=off ACPI tables fixes Philippe Mathieu-Daudé
` (7 preceding siblings ...)
2025-04-03 20:40 ` [PATCH-for-10.1 v3 8/9] hw/arm/virt-acpi: Do not advertise disabled GIC ITS Philippe Mathieu-Daudé
@ 2025-04-03 20:40 ` Philippe Mathieu-Daudé
2025-04-04 3:04 ` Gustavo Romero
2025-04-15 8:19 ` [PATCH-for-10.1 v3 0/9] hw/arm: GIC ITS=off ACPI tables fixes Philippe Mathieu-Daudé
9 siblings, 1 reply; 30+ messages in thread
From: Philippe Mathieu-Daudé @ 2025-04-03 20:40 UTC (permalink / raw)
To: qemu-devel
Cc: Andrew Jones, Alex Bennée, Michael S. Tsirkin, qemu-arm,
Udo Steinberg, Shannon Zhao, Gustavo Romero, Igor Mammedov,
Peter Maydell, Ani Sinha, Philippe Mathieu-Daudé
Commit the blobs generated by tests/data/acpi/rebuild-expected-aml.sh.
Changes in the tables:
@@ -1,32 +1,32 @@
/*
* Intel ACPI Component Architecture
* AML/ASL+ Disassembler version 20240927 (64-bit version)
* Copyright (c) 2000 - 2023 Intel Corporation
*
* Disassembly of tests/data/acpi/aarch64/virt/APIC.its_off
*
* ACPI Data Table [APIC]
*
* Format: [HexOffset DecimalOffset ByteLength] FieldName : FieldValue (in hex)
*/
[000h 0000 004h] Signature : "APIC" [Multiple APIC Description Table (MADT)]
-[004h 0004 004h] Table Length : 000000B8
+[004h 0004 004h] Table Length : 000000A4
[008h 0008 001h] Revision : 04
-[009h 0009 001h] Checksum : A7
+[009h 0009 001h] Checksum : EE
[00Ah 0010 006h] Oem ID : "BOCHS "
[010h 0016 008h] Oem Table ID : "BXPC "
[018h 0024 004h] Oem Revision : 00000001
[01Ch 0028 004h] Asl Compiler ID : "BXPC"
[020h 0032 004h] Asl Compiler Revision : 00000001
[024h 0036 004h] Local Apic Address : 00000000
[028h 0040 004h] Flags (decoded below) : 00000000
PC-AT Compatibility : 0
[02Ch 0044 001h] Subtable Type : 0C [Generic Interrupt Distributor]
[02Dh 0045 001h] Length : 18
[02Eh 0046 002h] Reserved : 0000
[030h 0048 004h] Local GIC Hardware ID : 00000000
[034h 0052 008h] Base Address : 0000000008000000
[03Ch 0060 004h] Interrupt Base : 00000000
@@ -49,37 +49,29 @@
[06Ch 0108 008h] Virtual GIC Base Address : 0000000000000000
[074h 0116 008h] Hypervisor GIC Base Address : 0000000000000000
[07Ch 0124 004h] Virtual GIC Interrupt : 00000019
[080h 0128 008h] Redistributor Base Address : 0000000000000000
[088h 0136 008h] ARM MPIDR : 0000000000000000
[090h 0144 001h] Efficiency Class : 00
[091h 0145 001h] Reserved : 00
[092h 0146 002h] SPE Overflow Interrupt : 0000
[094h 0148 002h] TRBE Interrupt : 100E
[094h 0148 001h] Subtable Type : 0E [Generic Interrupt Redistributor]
[095h 0149 001h] Length : 10
[097h 0151 002h] Reserved : 0000
[098h 0152 008h] Base Address : 00000000080A0000
[0A0h 0160 004h] Length : 00F60000
-[0A4h 0164 001h] Subtable Type : 0F [Generic Interrupt Translator]
-[0A5h 0165 001h] Length : 14
-[0A7h 0167 002h] Reserved : 0000
-[0A8h 0168 004h] Translation ID : 00000000
-[0ACh 0172 008h] Base Address : 0000000008080000
-[0B4h 0180 004h] Reserved : 00000000
+Raw Table Data: Length 164 (0xA4)
-Raw Table Data: Length 184 (0xB8)
-
- 0000: 41 50 49 43 B8 00 00 00 04 A7 42 4F 43 48 53 20 // APIC......BOCHS
+ 0000: 41 50 49 43 A4 00 00 00 04 EE 42 4F 43 48 53 20 // APIC......BOCHS
0010: 42 58 50 43 20 20 20 20 01 00 00 00 42 58 50 43 // BXPC ....BXPC
0020: 01 00 00 00 00 00 00 00 00 00 00 00 0C 18 00 00 // ................
0030: 00 00 00 00 00 00 00 08 00 00 00 00 00 00 00 00 // ................
0040: 04 00 00 00 0B 50 00 00 00 00 00 00 00 00 00 00 // .....P..........
0050: 01 00 00 00 00 00 00 00 17 00 00 00 00 00 00 00 // ................
0060: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 // ................
0070: 00 00 00 00 00 00 00 00 00 00 00 00 19 00 00 00 // ................
0080: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 // ................
0090: 00 00 00 00 0E 10 00 00 00 00 0A 08 00 00 00 00 // ................
- 00A0: 00 00 F6 00 0F 14 00 00 00 00 00 00 00 00 08 08 // ................
- 00B0: 00 00 00 00 00 00 00 00 // ........
+ 00A0: 00 00 F6 00 // ....
/*
* Intel ACPI Component Architecture
* AML/ASL+ Disassembler version 20240927 (64-bit version)
* Copyright (c) 2000 - 2023 Intel Corporation
*
* Disassembly of tests/data/acpi/aarch64/virt/FACP
*
* ACPI Data Table [FACP]
*
* Format: [HexOffset DecimalOffset ByteLength] FieldName : FieldValue (in hex)
*/
[000h 0000 004h] Signature : "FACP" [Fixed ACPI Description Table (FADT)]
[004h 0004 004h] Table Length : 00000114
[008h 0008 001h] Revision : 06
-[009h 0009 001h] Checksum : 12
+[009h 0009 001h] Checksum : 14
[00Ah 0010 006h] Oem ID : "BOCHS "
[010h 0016 008h] Oem Table ID : "BXPC "
[018h 0024 004h] Oem Revision : 00000001
[01Ch 0028 004h] Asl Compiler ID : "BXPC"
[020h 0032 004h] Asl Compiler Revision : 00000001
[024h 0036 004h] FACS Address : 00000000
[028h 0040 004h] DSDT Address : 00000000
[02Ch 0044 001h] Model : 00
[02Dh 0045 001h] PM Profile : 00 [Unspecified]
[02Eh 0046 002h] SCI Interrupt : 0000
[030h 0048 004h] SMI Command Port : 00000000
[034h 0052 001h] ACPI Enable Value : 00
[035h 0053 001h] ACPI Disable Value : 00
[036h 0054 001h] S4BIOS Command : 00
[037h 0055 001h] P-State Control : 00
@@ -82,35 +82,35 @@
Use Platform Timer (V4) : 0
RTC_STS valid on S4 wake (V4) : 0
Remote Power-on capable (V4) : 0
Use APIC Cluster Model (V4) : 0
Use APIC Physical Destination Mode (V4) : 0
Hardware Reduced (V5) : 1
Low Power S0 Idle (V5) : 0
[074h 0116 00Ch] Reset Register : [Generic Address Structure]
[074h 0116 001h] Space ID : 00 [SystemMemory]
[075h 0117 001h] Bit Width : 00
[076h 0118 001h] Bit Offset : 00
[077h 0119 001h] Encoded Access Width : 00 [Undefined/Legacy]
[078h 0120 008h] Address : 0000000000000000
[080h 0128 001h] Value to cause reset : 00
-[081h 0129 002h] ARM Flags (decoded below) : 0003
+[081h 0129 002h] ARM Flags (decoded below) : 0001
PSCI Compliant : 1
- Must use HVC for PSCI : 1
+ Must use HVC for PSCI : 0
[083h 0131 001h] FADT Minor Revision : 03
[084h 0132 008h] FACS Address : 0000000000000000
[08Ch 0140 008h] DSDT Address : 0000000000000000
[094h 0148 00Ch] PM1A Event Block : [Generic Address Structure]
[094h 0148 001h] Space ID : 00 [SystemMemory]
[095h 0149 001h] Bit Width : 00
[096h 0150 001h] Bit Offset : 00
[097h 0151 001h] Encoded Access Width : 00 [Undefined/Legacy]
[098h 0152 008h] Address : 0000000000000000
[0A0h 0160 00Ch] PM1B Event Block : [Generic Address Structure]
[0A0h 0160 001h] Space ID : 00 [SystemMemory]
[0A1h 0161 001h] Bit Width : 00
[0A2h 0162 001h] Bit Offset : 00
[0A3h 0163 001h] Encoded Access Width : 00 [Undefined/Legacy]
@@ -164,34 +164,34 @@
[0F5h 0245 001h] Bit Width : 00
[0F6h 0246 001h] Bit Offset : 00
[0F7h 0247 001h] Encoded Access Width : 00 [Undefined/Legacy]
[0F8h 0248 008h] Address : 0000000000000000
[100h 0256 00Ch] Sleep Status Register : [Generic Address Structure]
[100h 0256 001h] Space ID : 00 [SystemMemory]
[101h 0257 001h] Bit Width : 00
[102h 0258 001h] Bit Offset : 00
[103h 0259 001h] Encoded Access Width : 00 [Undefined/Legacy]
[104h 0260 008h] Address : 0000000000000000
[10Ch 0268 008h] Hypervisor ID : 00000000554D4551
Raw Table Data: Length 276 (0x114)
- 0000: 46 41 43 50 14 01 00 00 06 12 42 4F 43 48 53 20 // FACP......BOCHS
+ 0000: 46 41 43 50 14 01 00 00 06 14 42 4F 43 48 53 20 // FACP......BOCHS
0010: 42 58 50 43 20 20 20 20 01 00 00 00 42 58 50 43 // BXPC ....BXPC
0020: 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 // ................
0030: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 // ................
0040: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 // ................
0050: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 // ................
0060: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 // ................
0070: 00 00 10 00 00 00 00 00 00 00 00 00 00 00 00 00 // ................
- 0080: 00 03 00 03 00 00 00 00 00 00 00 00 00 00 00 00 // ................
+ 0080: 00 01 00 03 00 00 00 00 00 00 00 00 00 00 00 00 // ................
0090: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 // ................
00A0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 // ................
00B0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 // ................
00C0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 // ................
00D0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 // ................
00E0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 // ................
00F0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 // ................
0100: 00 00 00 00 00 00 00 00 00 00 00 00 51 45 4D 55 // ............QEMU
0110: 00 00 00 00 // ....
@@ -1,122 +1,111 @@
/*
* Intel ACPI Component Architecture
* AML/ASL+ Disassembler version 20240927 (64-bit version)
* Copyright (c) 2000 - 2023 Intel Corporation
*
* Disassembly of tests/data/acpi/aarch64/virt/IORT.its_off
*
* ACPI Data Table [IORT]
*
* Format: [HexOffset DecimalOffset ByteLength] FieldName : FieldValue (in hex)
*/
[000h 0000 004h] Signature : "IORT" [IO Remapping Table]
-[004h 0004 004h] Table Length : 000000EC
+[004h 0004 004h] Table Length : 000000D4
[008h 0008 001h] Revision : 03
-[009h 0009 001h] Checksum : 57
+[009h 0009 001h] Checksum : A3
[00Ah 0010 006h] Oem ID : "BOCHS "
[010h 0016 008h] Oem Table ID : "BXPC "
[018h 0024 004h] Oem Revision : 00000001
[01Ch 0028 004h] Asl Compiler ID : "BXPC"
[020h 0032 004h] Asl Compiler Revision : 00000001
[024h 0036 004h] Node Count : 00000003
[028h 0040 004h] Node Offset : 00000030
[02Ch 0044 004h] Reserved : 00000000
-[030h 0048 001h] Type : 00
-[031h 0049 002h] Length : 0018
-[033h 0051 001h] Revision : 01
+[030h 0048 001h] Type : 04
+[031h 0049 002h] Length : 0058
+[033h 0051 001h] Revision : 04
[034h 0052 004h] Identifier : 00000000
-[038h 0056 004h] Mapping Count : 00000000
-[03Ch 0060 004h] Mapping Offset : 00000000
+[038h 0056 004h] Mapping Count : 00000001
+[03Ch 0060 004h] Mapping Offset : 00000044
-[040h 0064 004h] ItsCount : 00000001
-[044h 0068 004h] Identifiers : 00000000
-
-[048h 0072 001h] Type : 04
-[049h 0073 002h] Length : 0058
-[04Bh 0075 001h] Revision : 04
-[04Ch 0076 004h] Identifier : 00000001
-[050h 0080 004h] Mapping Count : 00000001
-[054h 0084 004h] Mapping Offset : 00000044
-
-[058h 0088 008h] Base Address : 0000000009050000
-[060h 0096 004h] Flags (decoded below) : 00000001
+[040h 0064 008h] Base Address : 0000000009050000
+[048h 0072 004h] Flags (decoded below) : 00000001
COHACC Override : 1
HTTU Override : 0
Proximity Domain Valid : 0
DeviceID Valid : 0
-[064h 0100 004h] Reserved : 00000000
-[068h 0104 008h] VATOS Address : 0000000000000000
-[070h 0112 004h] Model : 00000000
-[074h 0116 004h] Event GSIV : 0000006A
-[078h 0120 004h] PRI GSIV : 0000006B
-[07Ch 0124 004h] GERR GSIV : 0000006D
-[080h 0128 004h] Sync GSIV : 0000006C
-[084h 0132 004h] Proximity Domain : 00000000
-[088h 0136 004h] Device ID Mapping Index : 00000000
+[04Ch 0076 004h] Reserved : 00000000
+[050h 0080 008h] VATOS Address : 0000000000000000
+[058h 0088 004h] Model : 00000000
+[05Ch 0092 004h] Event GSIV : 0000006A
+[060h 0096 004h] PRI GSIV : 0000006B
+[064h 0100 004h] GERR GSIV : 0000006D
+[068h 0104 004h] Sync GSIV : 0000006C
+[06Ch 0108 004h] Proximity Domain : 00000000
+[070h 0112 004h] Device ID Mapping Index : 00000000
-[08Ch 0140 004h] Input base : 00000000
-[090h 0144 004h] ID Count : 0000FFFF
-[094h 0148 004h] Output Base : 00000000
-[098h 0152 004h] Output Reference : 00000030
-[09Ch 0156 004h] Flags (decoded below) : 00000000
+[074h 0116 004h] Input base : 00000000
+[078h 0120 004h] ID Count : 0000FFFF
+[07Ch 0124 004h] Output Base : 00000000
+[080h 0128 004h] Output Reference : 00000030
+[084h 0132 004h] Flags (decoded below) : 00000000
Single Mapping : 0
-[0A0h 0160 001h] Type : 02
-[0A1h 0161 002h] Length : 004C
-[0A3h 0163 001h] Revision : 03
-[0A4h 0164 004h] Identifier : 00000002
-[0A8h 0168 004h] Mapping Count : 00000002
-[0ACh 0172 004h] Mapping Offset : 00000024
+[088h 0136 001h] Type : 02
+[089h 0137 002h] Length : 004C
+[08Bh 0139 001h] Revision : 03
+[08Ch 0140 004h] Identifier : 00000001
+[090h 0144 004h] Mapping Count : 00000002
+[094h 0148 004h] Mapping Offset : 00000024
-[0B0h 0176 008h] Memory Properties : [IORT Memory Access Properties]
-[0B0h 0176 004h] Cache Coherency : 00000001
-[0B4h 0180 001h] Hints (decoded below) : 00
+[098h 0152 008h] Memory Properties : [IORT Memory Access Properties]
+[098h 0152 004h] Cache Coherency : 00000001
+[09Ch 0156 001h] Hints (decoded below) : 00
Transient : 0
Write Allocate : 0
Read Allocate : 0
Override : 0
-[0B5h 0181 002h] Reserved : 0000
-[0B7h 0183 001h] Memory Flags (decoded below) : 03
+[09Dh 0157 002h] Reserved : 0000
+[09Fh 0159 001h] Memory Flags (decoded below) : 03
Coherency : 1
Device Attribute : 1
Ensured Coherency of Accesses : 0
-[0B8h 0184 004h] ATS Attribute : 00000000
-[0BCh 0188 004h] PCI Segment Number : 00000000
-[0C0h 0192 001h] Memory Size Limit : 40
-[0C1h 0193 002h] PASID Capabilities : 0000
-[0C3h 0195 001h] Reserved : 00
+[0A0h 0160 004h] ATS Attribute : 00000000
+[0A4h 0164 004h] PCI Segment Number : 00000000
+[0A8h 0168 001h] Memory Size Limit : 40
+[0A9h 0169 002h] PASID Capabilities : 0000
+[0ABh 0171 001h] Reserved : 00
-[0C4h 0196 004h] Input base : 00000000
-[0C8h 0200 004h] ID Count : 000000FF
-[0CCh 0204 004h] Output Base : 00000000
-[0D0h 0208 004h] Output Reference : 00000048
-[0D4h 0212 004h] Flags (decoded below) : 00000000
+[0ACh 0172 004h] Input base : 00000000
+[0B0h 0176 004h] ID Count : 000000FF
+[0B4h 0180 004h] Output Base : 00000000
+[0B8h 0184 004h] Output Reference : 00000030
+[0BCh 0188 004h] Flags (decoded below) : 00000000
Single Mapping : 0
-[0D8h 0216 004h] Input base : 00000100
-[0DCh 0220 004h] ID Count : 0000FEFF
-[0E0h 0224 004h] Output Base : 00000100
-[0E4h 0228 004h] Output Reference : 00000030
-[0E8h 0232 004h] Flags (decoded below) : 00000000
+[0C0h 0192 004h] Input base : 00000100
+[0C4h 0196 004h] ID Count : 0000FEFF
+[0C8h 0200 004h] Output Base : 00000100
+[0CCh 0204 004h] Output Reference : 00000030
+[0D0h 0208 004h] Flags (decoded below) : 00000000
Single Mapping : 0
-Raw Table Data: Length 236 (0xEC)
+Raw Table Data: Length 212 (0xD4)
- 0000: 49 4F 52 54 EC 00 00 00 03 57 42 4F 43 48 53 20 // IORT.....WBOCHS
+ 0000: 49 4F 52 54 D4 00 00 00 03 A3 42 4F 43 48 53 20 // IORT......BOCHS
0010: 42 58 50 43 20 20 20 20 01 00 00 00 42 58 50 43 // BXPC ....BXPC
0020: 01 00 00 00 03 00 00 00 30 00 00 00 00 00 00 00 // ........0.......
- 0030: 00 18 00 01 00 00 00 00 00 00 00 00 00 00 00 00 // ................
- 0040: 01 00 00 00 00 00 00 00 04 58 00 04 01 00 00 00 // .........X......
- 0050: 01 00 00 00 44 00 00 00 00 00 05 09 00 00 00 00 // ....D...........
- 0060: 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 // ................
- 0070: 00 00 00 00 6A 00 00 00 6B 00 00 00 6D 00 00 00 // ....j...k...m...
- 0080: 6C 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 // l...............
- 0090: FF FF 00 00 00 00 00 00 30 00 00 00 00 00 00 00 // ........0.......
- 00A0: 02 4C 00 03 02 00 00 00 02 00 00 00 24 00 00 00 // .L..........$...
- 00B0: 01 00 00 00 00 00 00 03 00 00 00 00 00 00 00 00 // ................
- 00C0: 40 00 00 00 00 00 00 00 FF 00 00 00 00 00 00 00 // @...............
- 00D0: 48 00 00 00 00 00 00 00 00 01 00 00 FF FE 00 00 // H...............
- 00E0: 00 01 00 00 30 00 00 00 00 00 00 00 // ....0.......
+ 0030: 04 58 00 04 00 00 00 00 01 00 00 00 44 00 00 00 // .X..........D...
+ 0040: 00 00 05 09 00 00 00 00 01 00 00 00 00 00 00 00 // ................
+ 0050: 00 00 00 00 00 00 00 00 00 00 00 00 6A 00 00 00 // ............j...
+ 0060: 6B 00 00 00 6D 00 00 00 6C 00 00 00 00 00 00 00 // k...m...l.......
+ 0070: 00 00 00 00 00 00 00 00 FF FF 00 00 00 00 00 00 // ................
+ 0080: 30 00 00 00 00 00 00 00 02 4C 00 03 01 00 00 00 // 0........L......
+ 0090: 02 00 00 00 24 00 00 00 01 00 00 00 00 00 00 03 // ....$...........
+ 00A0: 00 00 00 00 00 00 00 00 40 00 00 00 00 00 00 00 // ........@.......
+ 00B0: FF 00 00 00 00 00 00 00 30 00 00 00 00 00 00 00 // ........0.......
+ 00C0: 00 01 00 00 FF FE 00 00 00 01 00 00 30 00 00 00 // ............0...
+ 00D0: 00 00 00 00 // ....
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
tests/qtest/bios-tables-test-allowed-diff.h | 3 ---
tests/data/acpi/aarch64/virt/APIC.its_off | Bin 184 -> 164 bytes
tests/data/acpi/aarch64/virt/IORT.its_off | Bin 236 -> 212 bytes
3 files changed, 3 deletions(-)
diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
index 3421dd5adf3..dfb8523c8bf 100644
--- a/tests/qtest/bios-tables-test-allowed-diff.h
+++ b/tests/qtest/bios-tables-test-allowed-diff.h
@@ -1,4 +1 @@
/* List of comma-separated changed AML files to ignore */
-"tests/data/acpi/aarch64/virt/APIC.its_off",
-"tests/data/acpi/aarch64/virt/FACP.its_off",
-"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
index c37d05d6e05805304f10afe73eb7cb9100fcccfa..f24ac8fbff5261a52434abcfcff96dbdc7709de4 100644
GIT binary patch
delta 18
ZcmdnNxP+0*F~HM#2?GNI%e#qOvj8xy1yKM1
delta 39
jcmZ3&xPy_)F~HM#2Ll5G%kqg_vqbnsfJ`vp;DE6Jpmzmj
diff --git a/tests/data/acpi/aarch64/virt/IORT.its_off b/tests/data/acpi/aarch64/virt/IORT.its_off
index 0fceb820d509e852ca0849baf568a8e93e426738..25f64407c4aac11bda2d1ac24cd8999a1d84c80d 100644
GIT binary patch
delta 37
tcmaFEc!iP6(?2NW3IhWJ^WuqIYOE{~3@i*2eZv_UCoa`yG?;i%8UVn-3Gn~`
delta 61
zcmcb@_=b_o(?2NW4FdxMbNECqHCYA;21XzU4qzOX2nH6$iKgL<OcQ&x89gTMlm-Aj
CGzq}~
--
2.47.1
^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH-for-10.1 v3 3/9] hw/arm/virt: Simplify create_its()
2025-04-03 20:40 ` [PATCH-for-10.1 v3 3/9] hw/arm/virt: Simplify create_its() Philippe Mathieu-Daudé
@ 2025-04-04 2:59 ` Gustavo Romero
2025-04-07 12:05 ` Eric Auger
1 sibling, 0 replies; 30+ messages in thread
From: Gustavo Romero @ 2025-04-04 2:59 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, qemu-devel
Cc: Andrew Jones, Alex Bennée, Michael S. Tsirkin, qemu-arm,
Udo Steinberg, Shannon Zhao, Igor Mammedov, Peter Maydell,
Ani Sinha, Richard Henderson
Hi Phil,
On 4/3/25 17:40, Philippe Mathieu-Daudé wrote:
> No need to strstr() check the class name when we can
> use kvm_irqchip_in_kernel().
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> ---
> hw/arm/virt.c | 12 +++---------
> 1 file changed, 3 insertions(+), 9 deletions(-)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 177e9e0eadb..326986deb16 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -711,21 +711,15 @@ static inline DeviceState *create_acpi_ged(VirtMachineState *vms)
> static void create_its(VirtMachineState *vms)
> {
> VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
> - const char *itsclass = its_class_name();
> DeviceState *dev;
>
> - if (!strcmp(itsclass, "arm-gicv3-its")) {
> - if (vmc->no_tcg_its) {
> - itsclass = NULL;
> - }
> - }
> -
> - if (!itsclass) {
> + assert(!vmc->no_its);
> + if (!kvm_irqchip_in_kernel() && vmc->no_tcg_its) {
> /* Do nothing if not supported */
> return;
> }
>
> - dev = qdev_new(itsclass);
> + dev = qdev_new(its_class_name());
>
> object_property_set_link(OBJECT(dev), "parent-gicv3", OBJECT(vms->gic),
> &error_abort);
Reviewed-by: Gustavo Romero <gustavo.romero@linaro.org>
Cheers,
Gustavo
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH-for-10.1 v3 5/9] qtest/bios-tables-test: Add test for -M virt,its=off
2025-04-03 20:40 ` [PATCH-for-10.1 v3 5/9] qtest/bios-tables-test: Add test for -M virt, its=off Philippe Mathieu-Daudé
@ 2025-04-04 3:00 ` Gustavo Romero
2025-04-07 12:55 ` Eric Auger
0 siblings, 1 reply; 30+ messages in thread
From: Gustavo Romero @ 2025-04-04 3:00 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, qemu-devel
Cc: Andrew Jones, Alex Bennée, Michael S. Tsirkin, qemu-arm,
Udo Steinberg, Shannon Zhao, Igor Mammedov, Peter Maydell,
Ani Sinha
Hi Phil,
On 4/3/25 17:40, Philippe Mathieu-Daudé wrote:
> Add the use case reported as issue #2886 [*]. The test
> passes while it shouldn't. We are going to fix that in
> the following commits.
I think this organization is not ideal. I like better your first
version, i.e., adding the correct blobs that make the new test pass,
as you did in:
[PATCH-for-10.0 1/5] qtest/bios-tables-test: Add test for -M virt, its=o
https://mail.gnu.org/archive/html/qemu-devel/2025-03/msg07081.html
The way it is here if you checkout this commit and run the test
(make check) the acpi test will _not pass_ because of the other changes
in the APIC, FADT, and IORT tables caused by the other VM options
(virtualization, gic-version, and iommu), as I pointed out in [0], even
tho the bug is present and its=off does not take effect.
Cheers,
Gustavo
[0] https://mail.gnu.org/archive/html/qemu-devel/2025-04/msg00438.html
> [*] https://gitlab.com/qemu-project/qemu/-/issues/2886
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> tests/qtest/bios-tables-test.c | 21 +++++++++++++++++++++
> 1 file changed, 21 insertions(+)
>
> diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
> index 0a333ec4353..baaf199e01c 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",
> + .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 virtualization=on,secure=off "
> + "-M gic-version=max,its=off,iommu=smmuv3", &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);
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH-for-10.1 v3 6/9] qtest/bios-tables-test: Whitelist aarch64/virt 'its_off' variant blobs
2025-04-03 20:40 ` [PATCH-for-10.1 v3 6/9] qtest/bios-tables-test: Whitelist aarch64/virt 'its_off' variant blobs Philippe Mathieu-Daudé
@ 2025-04-04 3:01 ` Gustavo Romero
2025-04-09 14:05 ` Igor Mammedov
0 siblings, 1 reply; 30+ messages in thread
From: Gustavo Romero @ 2025-04-04 3:01 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, qemu-devel
Cc: Andrew Jones, Alex Bennée, Michael S. Tsirkin, qemu-arm,
Udo Steinberg, Shannon Zhao, Igor Mammedov, Peter Maydell,
Ani Sinha
Hi Phil,
On 4/3/25 17:40, Philippe Mathieu-Daudé wrote:
> We are going to fix the test_acpi_aarch64_virt_tcg_its_off()
> test. In preparation, copy the ACPI tables which will be
> altered as 'its_off' variants, and whitelist them.
>
> Reviewed-by: Gustavo Romero <gustavo.romero@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> tests/qtest/bios-tables-test-allowed-diff.h | 3 +++
> tests/qtest/bios-tables-test.c | 1 +
> tests/data/acpi/aarch64/virt/APIC.its_off | Bin 0 -> 184 bytes
> tests/data/acpi/aarch64/virt/FACP.its_off | Bin 0 -> 276 bytes
> tests/data/acpi/aarch64/virt/IORT.its_off | Bin 0 -> 236 bytes
> 5 files changed, 4 insertions(+)
> create mode 100644 tests/data/acpi/aarch64/virt/APIC.its_off
> create mode 100644 tests/data/acpi/aarch64/virt/FACP.its_off
> create mode 100644 tests/data/acpi/aarch64/virt/IORT.its_off
>
> diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
> index dfb8523c8bf..3421dd5adf3 100644
> --- a/tests/qtest/bios-tables-test-allowed-diff.h
> +++ b/tests/qtest/bios-tables-test-allowed-diff.h
> @@ -1 +1,4 @@
> /* List of comma-separated changed AML files to ignore */
> +"tests/data/acpi/aarch64/virt/APIC.its_off",
> +"tests/data/acpi/aarch64/virt/FACP.its_off",
> +"tests/data/acpi/aarch64/virt/IORT.its_off",
I think your first approach is the correct one: you add the blobs
when adding the new test, so they would go into patch 5/9 in this series,
making the test pass without adding anything to bios-tables-test-allowed-diff.h.
Then in this patch only add the APIC.its_off table to the bios-tables-test-allowed-diff.h
since that's the table that changes when the fix is in place, as you did in:
https://mail.gnu.org/archive/html/qemu-devel/2025-03/msg07082.html
Plus, adding the blobs, which are actually related to the test in the other
patch, and ignoring them at the same time looks confusing to me. I understand
that only the blob that changes (APIC.its_off) with the fix should be temporarily
ignored and, later, updated, as in your first series.
Cheers,
Gustavo
> diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
> index baaf199e01c..55366bf4956 100644
> --- a/tests/qtest/bios-tables-test.c
> +++ b/tests/qtest/bios-tables-test.c
> @@ -2151,6 +2151,7 @@ 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",
> 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..c37d05d6e05805304f10afe73eb7cb9100fcccfa
> GIT binary patch
> literal 184
> zcmZ<^@O0k6z`($=+{xeBBUr&HBEVSz2pEB4AU24G0Uik$i-7~iVgWL^17JJ`2AFzr
> bgb+@aBn}xq0gwb2)Q)cq{30-g9B_L93G4|0
>
> literal 0
> HcmV?d00001
>
> diff --git a/tests/data/acpi/aarch64/virt/FACP.its_off b/tests/data/acpi/aarch64/virt/FACP.its_off
> new file mode 100644
> index 0000000000000000000000000000000000000000..606dac3fe4b55c31fd68b25d3a4127eeef227434
> GIT binary patch
> literal 276
> zcmZ>BbPf<<WME(uaq@Te2v%^42yj*a0-z8Bhz+8t3j|P&V`N}P6&N^PpsQ~v$aVnZ
> CVg~^L
>
> 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
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH-for-10.1 v3 7/9] hw/arm/virt-acpi: Always build IORT table (even with GIC ITS disabled)
2025-04-03 20:40 ` [PATCH-for-10.1 v3 7/9] hw/arm/virt-acpi: Always build IORT table (even with GIC ITS disabled) Philippe Mathieu-Daudé
@ 2025-04-04 3:02 ` Gustavo Romero
2025-04-07 13:30 ` Eric Auger
1 sibling, 0 replies; 30+ messages in thread
From: Gustavo Romero @ 2025-04-04 3:02 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, qemu-devel
Cc: Andrew Jones, Alex Bennée, Michael S. Tsirkin, qemu-arm,
Udo Steinberg, Shannon Zhao, Igor Mammedov, Peter Maydell,
Ani Sinha
Hi Phil,
On 4/3/25 17:40, Philippe Mathieu-Daudé wrote:
> Citing Gustavo [*]:
>
> Gating IORT table generation entirely based on the presence
> of ITS looks wrong because IORT table has data beyond GIC ITS,
> like for SMMUv3 etc..
>
> [*] https://lore.kernel.org/qemu-devel/bae6e29a-7290-47d2-8caf-14702ee091f0@linaro.org/
>
> Reported-by: Gustavo Romero <gustavo.romero@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Maybe it's worth to cite the "IO Remapping Table Platform Design Document",
in particular page 10, which reads:
"the IORT provides an ACPI description for IO topology, SMMUs, and GIC ITSs"
https://developer.arm.com/documentation/den0049/latest/
Now that the target is 10.1, let me thing a bit more about it.
Cheers,
Gustavo
> ---
> hw/arm/virt-acpi-build.c | 38 +++++++++++++++++++++-----------------
> 1 file changed, 21 insertions(+), 17 deletions(-)
>
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 1c389ef5cdb..e7e27951cb9 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -287,6 +287,7 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> uint32_t id = 0;
> GArray *smmu_idmaps = g_array_new(false, true, sizeof(AcpiIortIdMapping));
> GArray *its_idmaps = g_array_new(false, true, sizeof(AcpiIortIdMapping));
> + bool has_its = its_enabled(vms);
>
> AcpiTable table = { .sig = "IORT", .rev = 3, .oem_id = vms->oem_id,
> .oem_table_id = vms->oem_table_id };
> @@ -325,9 +326,12 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>
> nb_nodes = 3; /* RC, ITS, SMMUv3 */
> rc_mapping_count = smmu_idmaps->len + its_idmaps->len;
> - } else {
> + } else if (has_its) {
> nb_nodes = 2; /* RC, ITS */
> rc_mapping_count = 1;
> + } else {
> + nb_nodes = 1; /* RC */
> + rc_mapping_count = 1;
> }
> /* Number of IORT Nodes */
> build_append_int_noprefix(table_data, nb_nodes, 4);
> @@ -336,17 +340,19 @@ 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 (has_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;
> @@ -429,7 +435,7 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> build_iort_id_mapping(table_data, range->input_base,
> range->id_count, IORT_NODE_OFFSET);
> }
> - } else {
> + } else if (has_its) {
> /* output IORT node is the ITS group node (the first node) */
> build_iort_id_mapping(table_data, 0, 0x10000, IORT_NODE_OFFSET);
> }
> @@ -979,10 +985,8 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
> vms->oem_table_id);
> }
>
> - if (its_enabled(vms)) {
> - 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) {
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH-for-10.1 v3 9/9] qtest/bios-tables-test: Update aarch64/virt 'its_off' variant blobs
2025-04-03 20:40 ` [PATCH-for-10.1 v3 9/9] qtest/bios-tables-test: Update aarch64/virt 'its_off' variant blobs Philippe Mathieu-Daudé
@ 2025-04-04 3:04 ` Gustavo Romero
0 siblings, 0 replies; 30+ messages in thread
From: Gustavo Romero @ 2025-04-04 3:04 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, qemu-devel
Cc: Andrew Jones, Alex Bennée, Michael S. Tsirkin, qemu-arm,
Udo Steinberg, Shannon Zhao, Igor Mammedov, Peter Maydell,
Ani Sinha
Hi Phil,
On 4/3/25 17:40, Philippe Mathieu-Daudé wrote:
> Commit the blobs generated by tests/data/acpi/rebuild-expected-aml.sh.
In accordance with my comments in 5/9 and 6/9 about the blobs organization,
after the fix, I think only APIC blob should be updated and removed from
the "ignore list".
Cheers,
Gustavo
> Changes in the tables:
>
> @@ -1,32 +1,32 @@
> /*
> * Intel ACPI Component Architecture
> * AML/ASL+ Disassembler version 20240927 (64-bit version)
> * Copyright (c) 2000 - 2023 Intel Corporation
> *
> * Disassembly of tests/data/acpi/aarch64/virt/APIC.its_off
> *
> * ACPI Data Table [APIC]
> *
> * Format: [HexOffset DecimalOffset ByteLength] FieldName : FieldValue (in hex)
> */
>
> [000h 0000 004h] Signature : "APIC" [Multiple APIC Description Table (MADT)]
> -[004h 0004 004h] Table Length : 000000B8
> +[004h 0004 004h] Table Length : 000000A4
> [008h 0008 001h] Revision : 04
> -[009h 0009 001h] Checksum : A7
> +[009h 0009 001h] Checksum : EE
> [00Ah 0010 006h] Oem ID : "BOCHS "
> [010h 0016 008h] Oem Table ID : "BXPC "
> [018h 0024 004h] Oem Revision : 00000001
> [01Ch 0028 004h] Asl Compiler ID : "BXPC"
> [020h 0032 004h] Asl Compiler Revision : 00000001
>
> [024h 0036 004h] Local Apic Address : 00000000
> [028h 0040 004h] Flags (decoded below) : 00000000
> PC-AT Compatibility : 0
>
> [02Ch 0044 001h] Subtable Type : 0C [Generic Interrupt Distributor]
> [02Dh 0045 001h] Length : 18
> [02Eh 0046 002h] Reserved : 0000
> [030h 0048 004h] Local GIC Hardware ID : 00000000
> [034h 0052 008h] Base Address : 0000000008000000
> [03Ch 0060 004h] Interrupt Base : 00000000
> @@ -49,37 +49,29 @@
> [06Ch 0108 008h] Virtual GIC Base Address : 0000000000000000
> [074h 0116 008h] Hypervisor GIC Base Address : 0000000000000000
> [07Ch 0124 004h] Virtual GIC Interrupt : 00000019
> [080h 0128 008h] Redistributor Base Address : 0000000000000000
> [088h 0136 008h] ARM MPIDR : 0000000000000000
> [090h 0144 001h] Efficiency Class : 00
> [091h 0145 001h] Reserved : 00
> [092h 0146 002h] SPE Overflow Interrupt : 0000
> [094h 0148 002h] TRBE Interrupt : 100E
>
> [094h 0148 001h] Subtable Type : 0E [Generic Interrupt Redistributor]
> [095h 0149 001h] Length : 10
> [097h 0151 002h] Reserved : 0000
> [098h 0152 008h] Base Address : 00000000080A0000
> [0A0h 0160 004h] Length : 00F60000
>
> -[0A4h 0164 001h] Subtable Type : 0F [Generic Interrupt Translator]
> -[0A5h 0165 001h] Length : 14
> -[0A7h 0167 002h] Reserved : 0000
> -[0A8h 0168 004h] Translation ID : 00000000
> -[0ACh 0172 008h] Base Address : 0000000008080000
> -[0B4h 0180 004h] Reserved : 00000000
> +Raw Table Data: Length 164 (0xA4)
>
> -Raw Table Data: Length 184 (0xB8)
> -
> - 0000: 41 50 49 43 B8 00 00 00 04 A7 42 4F 43 48 53 20 // APIC......BOCHS
> + 0000: 41 50 49 43 A4 00 00 00 04 EE 42 4F 43 48 53 20 // APIC......BOCHS
> 0010: 42 58 50 43 20 20 20 20 01 00 00 00 42 58 50 43 // BXPC ....BXPC
> 0020: 01 00 00 00 00 00 00 00 00 00 00 00 0C 18 00 00 // ................
> 0030: 00 00 00 00 00 00 00 08 00 00 00 00 00 00 00 00 // ................
> 0040: 04 00 00 00 0B 50 00 00 00 00 00 00 00 00 00 00 // .....P..........
> 0050: 01 00 00 00 00 00 00 00 17 00 00 00 00 00 00 00 // ................
> 0060: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 // ................
> 0070: 00 00 00 00 00 00 00 00 00 00 00 00 19 00 00 00 // ................
> 0080: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 // ................
> 0090: 00 00 00 00 0E 10 00 00 00 00 0A 08 00 00 00 00 // ................
> - 00A0: 00 00 F6 00 0F 14 00 00 00 00 00 00 00 00 08 08 // ................
> - 00B0: 00 00 00 00 00 00 00 00 // ........
> + 00A0: 00 00 F6 00 // ....
>
> /*
> * Intel ACPI Component Architecture
> * AML/ASL+ Disassembler version 20240927 (64-bit version)
> * Copyright (c) 2000 - 2023 Intel Corporation
> *
> * Disassembly of tests/data/acpi/aarch64/virt/FACP
> *
> * ACPI Data Table [FACP]
> *
> * Format: [HexOffset DecimalOffset ByteLength] FieldName : FieldValue (in hex)
> */
>
> [000h 0000 004h] Signature : "FACP" [Fixed ACPI Description Table (FADT)]
> [004h 0004 004h] Table Length : 00000114
> [008h 0008 001h] Revision : 06
> -[009h 0009 001h] Checksum : 12
> +[009h 0009 001h] Checksum : 14
> [00Ah 0010 006h] Oem ID : "BOCHS "
> [010h 0016 008h] Oem Table ID : "BXPC "
> [018h 0024 004h] Oem Revision : 00000001
> [01Ch 0028 004h] Asl Compiler ID : "BXPC"
> [020h 0032 004h] Asl Compiler Revision : 00000001
>
> [024h 0036 004h] FACS Address : 00000000
> [028h 0040 004h] DSDT Address : 00000000
> [02Ch 0044 001h] Model : 00
> [02Dh 0045 001h] PM Profile : 00 [Unspecified]
> [02Eh 0046 002h] SCI Interrupt : 0000
> [030h 0048 004h] SMI Command Port : 00000000
> [034h 0052 001h] ACPI Enable Value : 00
> [035h 0053 001h] ACPI Disable Value : 00
> [036h 0054 001h] S4BIOS Command : 00
> [037h 0055 001h] P-State Control : 00
> @@ -82,35 +82,35 @@
> Use Platform Timer (V4) : 0
> RTC_STS valid on S4 wake (V4) : 0
> Remote Power-on capable (V4) : 0
> Use APIC Cluster Model (V4) : 0
> Use APIC Physical Destination Mode (V4) : 0
> Hardware Reduced (V5) : 1
> Low Power S0 Idle (V5) : 0
>
> [074h 0116 00Ch] Reset Register : [Generic Address Structure]
> [074h 0116 001h] Space ID : 00 [SystemMemory]
> [075h 0117 001h] Bit Width : 00
> [076h 0118 001h] Bit Offset : 00
> [077h 0119 001h] Encoded Access Width : 00 [Undefined/Legacy]
> [078h 0120 008h] Address : 0000000000000000
>
> [080h 0128 001h] Value to cause reset : 00
> -[081h 0129 002h] ARM Flags (decoded below) : 0003
> +[081h 0129 002h] ARM Flags (decoded below) : 0001
> PSCI Compliant : 1
> - Must use HVC for PSCI : 1
> + Must use HVC for PSCI : 0
>
> [083h 0131 001h] FADT Minor Revision : 03
> [084h 0132 008h] FACS Address : 0000000000000000
> [08Ch 0140 008h] DSDT Address : 0000000000000000
> [094h 0148 00Ch] PM1A Event Block : [Generic Address Structure]
> [094h 0148 001h] Space ID : 00 [SystemMemory]
> [095h 0149 001h] Bit Width : 00
> [096h 0150 001h] Bit Offset : 00
> [097h 0151 001h] Encoded Access Width : 00 [Undefined/Legacy]
> [098h 0152 008h] Address : 0000000000000000
>
> [0A0h 0160 00Ch] PM1B Event Block : [Generic Address Structure]
> [0A0h 0160 001h] Space ID : 00 [SystemMemory]
> [0A1h 0161 001h] Bit Width : 00
> [0A2h 0162 001h] Bit Offset : 00
> [0A3h 0163 001h] Encoded Access Width : 00 [Undefined/Legacy]
> @@ -164,34 +164,34 @@
> [0F5h 0245 001h] Bit Width : 00
> [0F6h 0246 001h] Bit Offset : 00
> [0F7h 0247 001h] Encoded Access Width : 00 [Undefined/Legacy]
> [0F8h 0248 008h] Address : 0000000000000000
>
> [100h 0256 00Ch] Sleep Status Register : [Generic Address Structure]
> [100h 0256 001h] Space ID : 00 [SystemMemory]
> [101h 0257 001h] Bit Width : 00
> [102h 0258 001h] Bit Offset : 00
> [103h 0259 001h] Encoded Access Width : 00 [Undefined/Legacy]
> [104h 0260 008h] Address : 0000000000000000
>
> [10Ch 0268 008h] Hypervisor ID : 00000000554D4551
>
> Raw Table Data: Length 276 (0x114)
>
> - 0000: 46 41 43 50 14 01 00 00 06 12 42 4F 43 48 53 20 // FACP......BOCHS
> + 0000: 46 41 43 50 14 01 00 00 06 14 42 4F 43 48 53 20 // FACP......BOCHS
> 0010: 42 58 50 43 20 20 20 20 01 00 00 00 42 58 50 43 // BXPC ....BXPC
> 0020: 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 // ................
> 0030: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 // ................
> 0040: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 // ................
> 0050: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 // ................
> 0060: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 // ................
> 0070: 00 00 10 00 00 00 00 00 00 00 00 00 00 00 00 00 // ................
> - 0080: 00 03 00 03 00 00 00 00 00 00 00 00 00 00 00 00 // ................
> + 0080: 00 01 00 03 00 00 00 00 00 00 00 00 00 00 00 00 // ................
> 0090: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 // ................
> 00A0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 // ................
> 00B0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 // ................
> 00C0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 // ................
> 00D0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 // ................
> 00E0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 // ................
> 00F0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 // ................
> 0100: 00 00 00 00 00 00 00 00 00 00 00 00 51 45 4D 55 // ............QEMU
> 0110: 00 00 00 00 // ....
>
> @@ -1,122 +1,111 @@
> /*
> * Intel ACPI Component Architecture
> * AML/ASL+ Disassembler version 20240927 (64-bit version)
> * Copyright (c) 2000 - 2023 Intel Corporation
> *
> * Disassembly of tests/data/acpi/aarch64/virt/IORT.its_off
> *
> * ACPI Data Table [IORT]
> *
> * Format: [HexOffset DecimalOffset ByteLength] FieldName : FieldValue (in hex)
> */
>
> [000h 0000 004h] Signature : "IORT" [IO Remapping Table]
> -[004h 0004 004h] Table Length : 000000EC
> +[004h 0004 004h] Table Length : 000000D4
> [008h 0008 001h] Revision : 03
> -[009h 0009 001h] Checksum : 57
> +[009h 0009 001h] Checksum : A3
> [00Ah 0010 006h] Oem ID : "BOCHS "
> [010h 0016 008h] Oem Table ID : "BXPC "
> [018h 0024 004h] Oem Revision : 00000001
> [01Ch 0028 004h] Asl Compiler ID : "BXPC"
> [020h 0032 004h] Asl Compiler Revision : 00000001
>
> [024h 0036 004h] Node Count : 00000003
> [028h 0040 004h] Node Offset : 00000030
> [02Ch 0044 004h] Reserved : 00000000
>
> -[030h 0048 001h] Type : 00
> -[031h 0049 002h] Length : 0018
> -[033h 0051 001h] Revision : 01
> +[030h 0048 001h] Type : 04
> +[031h 0049 002h] Length : 0058
> +[033h 0051 001h] Revision : 04
> [034h 0052 004h] Identifier : 00000000
> -[038h 0056 004h] Mapping Count : 00000000
> -[03Ch 0060 004h] Mapping Offset : 00000000
> +[038h 0056 004h] Mapping Count : 00000001
> +[03Ch 0060 004h] Mapping Offset : 00000044
>
> -[040h 0064 004h] ItsCount : 00000001
> -[044h 0068 004h] Identifiers : 00000000
> -
> -[048h 0072 001h] Type : 04
> -[049h 0073 002h] Length : 0058
> -[04Bh 0075 001h] Revision : 04
> -[04Ch 0076 004h] Identifier : 00000001
> -[050h 0080 004h] Mapping Count : 00000001
> -[054h 0084 004h] Mapping Offset : 00000044
> -
> -[058h 0088 008h] Base Address : 0000000009050000
> -[060h 0096 004h] Flags (decoded below) : 00000001
> +[040h 0064 008h] Base Address : 0000000009050000
> +[048h 0072 004h] Flags (decoded below) : 00000001
> COHACC Override : 1
> HTTU Override : 0
> Proximity Domain Valid : 0
> DeviceID Valid : 0
> -[064h 0100 004h] Reserved : 00000000
> -[068h 0104 008h] VATOS Address : 0000000000000000
> -[070h 0112 004h] Model : 00000000
> -[074h 0116 004h] Event GSIV : 0000006A
> -[078h 0120 004h] PRI GSIV : 0000006B
> -[07Ch 0124 004h] GERR GSIV : 0000006D
> -[080h 0128 004h] Sync GSIV : 0000006C
> -[084h 0132 004h] Proximity Domain : 00000000
> -[088h 0136 004h] Device ID Mapping Index : 00000000
> +[04Ch 0076 004h] Reserved : 00000000
> +[050h 0080 008h] VATOS Address : 0000000000000000
> +[058h 0088 004h] Model : 00000000
> +[05Ch 0092 004h] Event GSIV : 0000006A
> +[060h 0096 004h] PRI GSIV : 0000006B
> +[064h 0100 004h] GERR GSIV : 0000006D
> +[068h 0104 004h] Sync GSIV : 0000006C
> +[06Ch 0108 004h] Proximity Domain : 00000000
> +[070h 0112 004h] Device ID Mapping Index : 00000000
>
> -[08Ch 0140 004h] Input base : 00000000
> -[090h 0144 004h] ID Count : 0000FFFF
> -[094h 0148 004h] Output Base : 00000000
> -[098h 0152 004h] Output Reference : 00000030
> -[09Ch 0156 004h] Flags (decoded below) : 00000000
> +[074h 0116 004h] Input base : 00000000
> +[078h 0120 004h] ID Count : 0000FFFF
> +[07Ch 0124 004h] Output Base : 00000000
> +[080h 0128 004h] Output Reference : 00000030
> +[084h 0132 004h] Flags (decoded below) : 00000000
> Single Mapping : 0
>
> -[0A0h 0160 001h] Type : 02
> -[0A1h 0161 002h] Length : 004C
> -[0A3h 0163 001h] Revision : 03
> -[0A4h 0164 004h] Identifier : 00000002
> -[0A8h 0168 004h] Mapping Count : 00000002
> -[0ACh 0172 004h] Mapping Offset : 00000024
> +[088h 0136 001h] Type : 02
> +[089h 0137 002h] Length : 004C
> +[08Bh 0139 001h] Revision : 03
> +[08Ch 0140 004h] Identifier : 00000001
> +[090h 0144 004h] Mapping Count : 00000002
> +[094h 0148 004h] Mapping Offset : 00000024
>
> -[0B0h 0176 008h] Memory Properties : [IORT Memory Access Properties]
> -[0B0h 0176 004h] Cache Coherency : 00000001
> -[0B4h 0180 001h] Hints (decoded below) : 00
> +[098h 0152 008h] Memory Properties : [IORT Memory Access Properties]
> +[098h 0152 004h] Cache Coherency : 00000001
> +[09Ch 0156 001h] Hints (decoded below) : 00
> Transient : 0
> Write Allocate : 0
> Read Allocate : 0
> Override : 0
> -[0B5h 0181 002h] Reserved : 0000
> -[0B7h 0183 001h] Memory Flags (decoded below) : 03
> +[09Dh 0157 002h] Reserved : 0000
> +[09Fh 0159 001h] Memory Flags (decoded below) : 03
> Coherency : 1
> Device Attribute : 1
> Ensured Coherency of Accesses : 0
> -[0B8h 0184 004h] ATS Attribute : 00000000
> -[0BCh 0188 004h] PCI Segment Number : 00000000
> -[0C0h 0192 001h] Memory Size Limit : 40
> -[0C1h 0193 002h] PASID Capabilities : 0000
> -[0C3h 0195 001h] Reserved : 00
> +[0A0h 0160 004h] ATS Attribute : 00000000
> +[0A4h 0164 004h] PCI Segment Number : 00000000
> +[0A8h 0168 001h] Memory Size Limit : 40
> +[0A9h 0169 002h] PASID Capabilities : 0000
> +[0ABh 0171 001h] Reserved : 00
>
> -[0C4h 0196 004h] Input base : 00000000
> -[0C8h 0200 004h] ID Count : 000000FF
> -[0CCh 0204 004h] Output Base : 00000000
> -[0D0h 0208 004h] Output Reference : 00000048
> -[0D4h 0212 004h] Flags (decoded below) : 00000000
> +[0ACh 0172 004h] Input base : 00000000
> +[0B0h 0176 004h] ID Count : 000000FF
> +[0B4h 0180 004h] Output Base : 00000000
> +[0B8h 0184 004h] Output Reference : 00000030
> +[0BCh 0188 004h] Flags (decoded below) : 00000000
> Single Mapping : 0
>
> -[0D8h 0216 004h] Input base : 00000100
> -[0DCh 0220 004h] ID Count : 0000FEFF
> -[0E0h 0224 004h] Output Base : 00000100
> -[0E4h 0228 004h] Output Reference : 00000030
> -[0E8h 0232 004h] Flags (decoded below) : 00000000
> +[0C0h 0192 004h] Input base : 00000100
> +[0C4h 0196 004h] ID Count : 0000FEFF
> +[0C8h 0200 004h] Output Base : 00000100
> +[0CCh 0204 004h] Output Reference : 00000030
> +[0D0h 0208 004h] Flags (decoded below) : 00000000
> Single Mapping : 0
>
> -Raw Table Data: Length 236 (0xEC)
> +Raw Table Data: Length 212 (0xD4)
>
> - 0000: 49 4F 52 54 EC 00 00 00 03 57 42 4F 43 48 53 20 // IORT.....WBOCHS
> + 0000: 49 4F 52 54 D4 00 00 00 03 A3 42 4F 43 48 53 20 // IORT......BOCHS
> 0010: 42 58 50 43 20 20 20 20 01 00 00 00 42 58 50 43 // BXPC ....BXPC
> 0020: 01 00 00 00 03 00 00 00 30 00 00 00 00 00 00 00 // ........0.......
> - 0030: 00 18 00 01 00 00 00 00 00 00 00 00 00 00 00 00 // ................
> - 0040: 01 00 00 00 00 00 00 00 04 58 00 04 01 00 00 00 // .........X......
> - 0050: 01 00 00 00 44 00 00 00 00 00 05 09 00 00 00 00 // ....D...........
> - 0060: 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 // ................
> - 0070: 00 00 00 00 6A 00 00 00 6B 00 00 00 6D 00 00 00 // ....j...k...m...
> - 0080: 6C 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 // l...............
> - 0090: FF FF 00 00 00 00 00 00 30 00 00 00 00 00 00 00 // ........0.......
> - 00A0: 02 4C 00 03 02 00 00 00 02 00 00 00 24 00 00 00 // .L..........$...
> - 00B0: 01 00 00 00 00 00 00 03 00 00 00 00 00 00 00 00 // ................
> - 00C0: 40 00 00 00 00 00 00 00 FF 00 00 00 00 00 00 00 // @...............
> - 00D0: 48 00 00 00 00 00 00 00 00 01 00 00 FF FE 00 00 // H...............
> - 00E0: 00 01 00 00 30 00 00 00 00 00 00 00 // ....0.......
> + 0030: 04 58 00 04 00 00 00 00 01 00 00 00 44 00 00 00 // .X..........D...
> + 0040: 00 00 05 09 00 00 00 00 01 00 00 00 00 00 00 00 // ................
> + 0050: 00 00 00 00 00 00 00 00 00 00 00 00 6A 00 00 00 // ............j...
> + 0060: 6B 00 00 00 6D 00 00 00 6C 00 00 00 00 00 00 00 // k...m...l.......
> + 0070: 00 00 00 00 00 00 00 00 FF FF 00 00 00 00 00 00 // ................
> + 0080: 30 00 00 00 00 00 00 00 02 4C 00 03 01 00 00 00 // 0........L......
> + 0090: 02 00 00 00 24 00 00 00 01 00 00 00 00 00 00 03 // ....$...........
> + 00A0: 00 00 00 00 00 00 00 00 40 00 00 00 00 00 00 00 // ........@.......
> + 00B0: FF 00 00 00 00 00 00 00 30 00 00 00 00 00 00 00 // ........0.......
> + 00C0: 00 01 00 00 FF FE 00 00 00 01 00 00 30 00 00 00 // ............0...
> + 00D0: 00 00 00 00 // ....
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> tests/qtest/bios-tables-test-allowed-diff.h | 3 ---
> tests/data/acpi/aarch64/virt/APIC.its_off | Bin 184 -> 164 bytes
> tests/data/acpi/aarch64/virt/IORT.its_off | Bin 236 -> 212 bytes
> 3 files changed, 3 deletions(-)
>
> diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
> index 3421dd5adf3..dfb8523c8bf 100644
> --- a/tests/qtest/bios-tables-test-allowed-diff.h
> +++ b/tests/qtest/bios-tables-test-allowed-diff.h
> @@ -1,4 +1 @@
> /* List of comma-separated changed AML files to ignore */
> -"tests/data/acpi/aarch64/virt/APIC.its_off",
> -"tests/data/acpi/aarch64/virt/FACP.its_off",
> -"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
> index c37d05d6e05805304f10afe73eb7cb9100fcccfa..f24ac8fbff5261a52434abcfcff96dbdc7709de4 100644
> GIT binary patch
> delta 18
> ZcmdnNxP+0*F~HM#2?GNI%e#qOvj8xy1yKM1
>
> delta 39
> jcmZ3&xPy_)F~HM#2Ll5G%kqg_vqbnsfJ`vp;DE6Jpmzmj
>
> diff --git a/tests/data/acpi/aarch64/virt/IORT.its_off b/tests/data/acpi/aarch64/virt/IORT.its_off
> index 0fceb820d509e852ca0849baf568a8e93e426738..25f64407c4aac11bda2d1ac24cd8999a1d84c80d 100644
> GIT binary patch
> delta 37
> tcmaFEc!iP6(?2NW3IhWJ^WuqIYOE{~3@i*2eZv_UCoa`yG?;i%8UVn-3Gn~`
>
> delta 61
> zcmcb@_=b_o(?2NW4FdxMbNECqHCYA;21XzU4qzOX2nH6$iKgL<OcQ&x89gTMlm-Aj
> CGzq}~
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH-for-10.1 v3 1/9] hw/arm/virt: Remove pointless VirtMachineState::tcg_its field
2025-04-03 20:40 ` [PATCH-for-10.1 v3 1/9] hw/arm/virt: Remove pointless VirtMachineState::tcg_its field Philippe Mathieu-Daudé
@ 2025-04-07 11:57 ` Eric Auger
0 siblings, 0 replies; 30+ messages in thread
From: Eric Auger @ 2025-04-07 11:57 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, qemu-devel
Cc: Andrew Jones, Alex Bennée, Michael S. Tsirkin, qemu-arm,
Udo Steinberg, Shannon Zhao, Gustavo Romero, Igor Mammedov,
Peter Maydell, Ani Sinha
Hi Philippe,
On 4/3/25 10:40 PM, Philippe Mathieu-Daudé wrote:
> VirtMachineState::tcg_its has the negated logic value of
> VirtMachineClass::no_tcg_its. Directly use the latter,
> removing the former.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Eric
> ---
> include/hw/arm/virt.h | 1 -
> hw/arm/virt.c | 13 +++++--------
> 2 files changed, 5 insertions(+), 9 deletions(-)
>
> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> index c8e94e6aedc..17c160429ea 100644
> --- a/include/hw/arm/virt.h
> +++ b/include/hw/arm/virt.h
> @@ -150,7 +150,6 @@ struct VirtMachineState {
> bool highmem_mmio;
> bool highmem_redists;
> bool its;
> - bool tcg_its;
> bool virt;
> bool ras;
> bool mte;
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index a96452f17a4..177e9e0eadb 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -710,11 +710,12 @@ static inline DeviceState *create_acpi_ged(VirtMachineState *vms)
>
> static void create_its(VirtMachineState *vms)
> {
> + VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
> const char *itsclass = its_class_name();
> DeviceState *dev;
>
> if (!strcmp(itsclass, "arm-gicv3-its")) {
> - if (!vms->tcg_its) {
> + if (vmc->no_tcg_its) {
> itsclass = NULL;
> }
> }
> @@ -831,7 +832,9 @@ static void create_gic(VirtMachineState *vms, MemoryRegion *mem)
> redist_region_count);
>
> if (!kvm_irqchip_in_kernel()) {
> - if (vms->tcg_its) {
> + VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
> +
> + if (!vmc->no_tcg_its) {
> object_property_set_link(OBJECT(vms->gic), "sysmem",
> OBJECT(mem), &error_fatal);
> qdev_prop_set_bit(vms->gic, "has-lpi", true);
> @@ -3357,12 +3360,6 @@ static void virt_instance_init(Object *obj)
> } else {
> /* Default allows ITS instantiation */
> vms->its = true;
> -
> - if (vmc->no_tcg_its) {
> - vms->tcg_its = false;
> - } else {
> - vms->tcg_its = true;
> - }
> }
>
> /* Default disallows iommu instantiation */
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH-for-10.1 v3 2/9] hw/intc/gicv3_its: Do not check its_class_name() for NULL
2025-04-03 20:40 ` [PATCH-for-10.1 v3 2/9] hw/intc/gicv3_its: Do not check its_class_name() for NULL Philippe Mathieu-Daudé
@ 2025-04-07 11:57 ` Eric Auger
0 siblings, 0 replies; 30+ messages in thread
From: Eric Auger @ 2025-04-07 11:57 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, qemu-devel
Cc: Andrew Jones, Alex Bennée, Michael S. Tsirkin, qemu-arm,
Udo Steinberg, Shannon Zhao, Gustavo Romero, Igor Mammedov,
Peter Maydell, Ani Sinha, Richard Henderson
On 4/3/25 10:40 PM, Philippe Mathieu-Daudé wrote:
> Since commit cc5e719e2c8 ("kvm: require KVM_CAP_SIGNAL_MSI"),
> its_class_name() single implementation doesn't return NULL
> anymore. Update the prototype docstring, and remove the
> pointless checks.
>
> Reported-by: Gustavo Romero <gustavo.romero@linaro.org>
> 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>
Eric
> ---
> include/hw/intc/arm_gicv3_its_common.h | 2 +-
> hw/arm/virt-acpi-build.c | 4 ++--
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/include/hw/intc/arm_gicv3_its_common.h b/include/hw/intc/arm_gicv3_its_common.h
> index 7dc712b38d2..3c7b543b018 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);
>
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 3ac8f8e1786..9b7fc99f170 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -741,7 +741,7 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> memmap[VIRT_HIGH_GIC_REDIST2].size);
> }
>
> - if (its_class_name() && !vmc->no_its) {
> + if (!vmc->no_its) {
> /*
> * ACPI spec, Revision 6.0 Errata A
> * (original 6.0 definition has invalid Length)
> @@ -973,7 +973,7 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
> vms->oem_table_id);
> }
>
> - if (its_class_name() && !vmc->no_its) {
> + if (!vmc->no_its) {
> acpi_add_table(table_offsets, tables_blob);
> build_iort(tables_blob, tables->linker, vms);
> }
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH-for-10.1 v3 3/9] hw/arm/virt: Simplify create_its()
2025-04-03 20:40 ` [PATCH-for-10.1 v3 3/9] hw/arm/virt: Simplify create_its() Philippe Mathieu-Daudé
2025-04-04 2:59 ` Gustavo Romero
@ 2025-04-07 12:05 ` Eric Auger
1 sibling, 0 replies; 30+ messages in thread
From: Eric Auger @ 2025-04-07 12:05 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, qemu-devel
Cc: Andrew Jones, Alex Bennée, Michael S. Tsirkin, qemu-arm,
Udo Steinberg, Shannon Zhao, Gustavo Romero, Igor Mammedov,
Peter Maydell, Ani Sinha, Richard Henderson
On 4/3/25 10:40 PM, Philippe Mathieu-Daudé wrote:
> No need to strstr() check the class name when we can
> use kvm_irqchip_in_kernel().
>
> 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>
Eric
> ---
> hw/arm/virt.c | 12 +++---------
> 1 file changed, 3 insertions(+), 9 deletions(-)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 177e9e0eadb..326986deb16 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -711,21 +711,15 @@ static inline DeviceState *create_acpi_ged(VirtMachineState *vms)
> static void create_its(VirtMachineState *vms)
> {
> VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
> - const char *itsclass = its_class_name();
> DeviceState *dev;
>
> - if (!strcmp(itsclass, "arm-gicv3-its")) {
> - if (vmc->no_tcg_its) {
> - itsclass = NULL;
> - }
> - }
> -
> - if (!itsclass) {
> + assert(!vmc->no_its);
> + if (!kvm_irqchip_in_kernel() && vmc->no_tcg_its) {
> /* Do nothing if not supported */
> return;
> }
>
> - dev = qdev_new(itsclass);
> + dev = qdev_new(its_class_name());
>
> object_property_set_link(OBJECT(dev), "parent-gicv3", OBJECT(vms->gic),
> &error_abort);
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH-for-10.1 v3 4/9] hw/arm/virt-acpi: Factor its_enabled() helper out
2025-04-03 20:40 ` [PATCH-for-10.1 v3 4/9] hw/arm/virt-acpi: Factor its_enabled() helper out Philippe Mathieu-Daudé
@ 2025-04-07 12:08 ` Eric Auger
0 siblings, 0 replies; 30+ messages in thread
From: Eric Auger @ 2025-04-07 12:08 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, qemu-devel
Cc: Andrew Jones, Alex Bennée, Michael S. Tsirkin, qemu-arm,
Udo Steinberg, Shannon Zhao, Gustavo Romero, Igor Mammedov,
Peter Maydell, Ani Sinha, Richard Henderson
On 4/3/25 10:40 PM, Philippe Mathieu-Daudé wrote:
> GIC ITS is checked for the MADT and IORT tables.
> Factor the checks out to the its_enabled() helper.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Reviewed-by: Gustavo Romero <gustavo.romero@linaro.org>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Eric
> ---
> hw/arm/virt-acpi-build.c | 12 +++++++++---
> 1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 9b7fc99f170..1c389ef5cdb 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -208,6 +208,13 @@ static void acpi_dsdt_add_tpm(Aml *scope, VirtMachineState *vms)
> #define ROOT_COMPLEX_ENTRY_SIZE 36
> #define IORT_NODE_OFFSET 48
>
> +static bool its_enabled(VirtMachineState *vms)
> +{
> + VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
> +
> + return !vmc->no_its;
> +}
> +
> /*
> * Append an ID mapping entry as described by "Table 4 ID mapping format" in
> * "IO Remapping Table System Software on ARM Platforms", Chapter 3.
> @@ -670,7 +677,6 @@ static void
> build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> {
> int i;
> - VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
> const MemMapEntry *memmap = vms->memmap;
> AcpiTable table = { .sig = "APIC", .rev = 4, .oem_id = vms->oem_id,
> .oem_table_id = vms->oem_table_id };
> @@ -741,7 +747,7 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> memmap[VIRT_HIGH_GIC_REDIST2].size);
> }
>
> - if (!vmc->no_its) {
> + if (its_enabled(vms)) {
> /*
> * ACPI spec, Revision 6.0 Errata A
> * (original 6.0 definition has invalid Length)
> @@ -973,7 +979,7 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
> vms->oem_table_id);
> }
>
> - if (!vmc->no_its) {
> + if (its_enabled(vms)) {
> acpi_add_table(table_offsets, tables_blob);
> build_iort(tables_blob, tables->linker, vms);
> }
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH-for-10.1 v3 5/9] qtest/bios-tables-test: Add test for -M virt,its=off
2025-04-04 3:00 ` [PATCH-for-10.1 v3 5/9] qtest/bios-tables-test: Add test for -M virt,its=off Gustavo Romero
@ 2025-04-07 12:55 ` Eric Auger
0 siblings, 0 replies; 30+ messages in thread
From: Eric Auger @ 2025-04-07 12:55 UTC (permalink / raw)
To: Gustavo Romero, Philippe Mathieu-Daudé, qemu-devel
Cc: Andrew Jones, Alex Bennée, Michael S. Tsirkin, qemu-arm,
Udo Steinberg, Shannon Zhao, Igor Mammedov, Peter Maydell,
Ani Sinha
On 4/4/25 5:00 AM, Gustavo Romero wrote:
> Hi Phil,
>
> On 4/3/25 17:40, Philippe Mathieu-Daudé wrote:
>> Add the use case reported as issue #2886 [*]. The test
>> passes while it shouldn't. We are going to fix that in
>> the following commits.
>
> I think this organization is not ideal. I like better your first
> version, i.e., adding the correct blobs that make the new test pass,
> as you did in:
>
> [PATCH-for-10.0 1/5] qtest/bios-tables-test: Add test for -M virt, its=o
> https://mail.gnu.org/archive/html/qemu-devel/2025-03/msg07081.html
>
> The way it is here if you checkout this commit and run the test
> (make check) the acpi test will _not pass_ because of the other changes
> in the APIC, FADT, and IORT tables caused by the other VM options
> (virtualization, gic-version, and iommu), as I pointed out in [0], even
> tho the bug is present and its=off does not take effect.
I do agree with Gustavo. This looks introduced in a non std way.
Found
https://lore.kernel.org/all/20241107123446.902801-6-Jonathan.Cameron@huawei.com/
as an example:
step1) create dummy entries for the variant table data + mark those as
ignored in tests/qtest/bios-tables-test-allowed-diff.h
<https://lore.kernel.org/all/20241107123446.902801-4-Jonathan.Cameron@huawei.com/#Z31tests:qtest:bios-tables-test-allowed-diff.h>
step2) create the new test
step3) Generate & store the new tables and remove lines in
bios-tables-test-allowed-diff.h
<https://lore.kernel.org/all/20241107123446.902801-6-Jonathan.Cameron@huawei.com/#Z31tests:qtest:bios-tables-test-allowed-diff.h>
Thanks
Eric
>
>
> Cheers,
> Gustavo
>
> [0] https://mail.gnu.org/archive/html/qemu-devel/2025-04/msg00438.html
>
>
>> [*] https://gitlab.com/qemu-project/qemu/-/issues/2886
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>> tests/qtest/bios-tables-test.c | 21 +++++++++++++++++++++
>> 1 file changed, 21 insertions(+)
>>
>> diff --git a/tests/qtest/bios-tables-test.c
>> b/tests/qtest/bios-tables-test.c
>> index 0a333ec4353..baaf199e01c 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",
>> + .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 virtualization=on,secure=off "
>> + "-M gic-version=max,its=off,iommu=smmuv3", &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);
>
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH-for-10.1 v3 7/9] hw/arm/virt-acpi: Always build IORT table (even with GIC ITS disabled)
2025-04-03 20:40 ` [PATCH-for-10.1 v3 7/9] hw/arm/virt-acpi: Always build IORT table (even with GIC ITS disabled) Philippe Mathieu-Daudé
2025-04-04 3:02 ` Gustavo Romero
@ 2025-04-07 13:30 ` Eric Auger
1 sibling, 0 replies; 30+ messages in thread
From: Eric Auger @ 2025-04-07 13:30 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, qemu-devel
Cc: Andrew Jones, Alex Bennée, Michael S. Tsirkin, qemu-arm,
Udo Steinberg, Shannon Zhao, Gustavo Romero, Igor Mammedov,
Peter Maydell, Ani Sinha
Hi Philippe,
On 4/3/25 10:40 PM, Philippe Mathieu-Daudé wrote:
> Citing Gustavo [*]:
>
> Gating IORT table generation entirely based on the presence
> of ITS looks wrong because IORT table has data beyond GIC ITS,
> like for SMMUv3 etc..
> [*] https://lore.kernel.org/qemu-devel/bae6e29a-7290-47d2-8caf-14702ee091f0@linaro.org/
>
> Reported-by: Gustavo Romero <gustavo.romero@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> hw/arm/virt-acpi-build.c | 38 +++++++++++++++++++++-----------------
> 1 file changed, 21 insertions(+), 17 deletions(-)
>
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 1c389ef5cdb..e7e27951cb9 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -287,6 +287,7 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> uint32_t id = 0;
> GArray *smmu_idmaps = g_array_new(false, true, sizeof(AcpiIortIdMapping));
> GArray *its_idmaps = g_array_new(false, true, sizeof(AcpiIortIdMapping));
> + bool has_its = its_enabled(vms);
>
> AcpiTable table = { .sig = "IORT", .rev = 3, .oem_id = vms->oem_id,
> .oem_table_id = vms->oem_table_id };
> @@ -325,9 +326,12 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>
> nb_nodes = 3; /* RC, ITS, SMMUv3 */
> rc_mapping_count = smmu_idmaps->len + its_idmaps->len;
> - } else {
> + } else if (has_its) {
> nb_nodes = 2; /* RC, ITS */
> rc_mapping_count = 1;
> + } else {
> + nb_nodes = 1; /* RC */
> + rc_mapping_count = 1;
> }
> /* Number of IORT Nodes */
> build_append_int_noprefix(table_data, nb_nodes, 4);
> @@ -336,17 +340,19 @@ 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 (has_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);
> + }
Do we allow configs where we have SMMU and no ITS?
The IORT spec says "
ID mappings that are defined in an SMMU IORT node can only have an ITS
Group node as an output reference,
or no IDs in the case of a system that does not have ITS units. All
other object types are explicitly forbidden.
"
So in case we do not have any ITS we also need to take care of ID
mappings of SMMUs
Eric
>
> if (vms->iommu == VIRT_IOMMU_SMMUV3) {
> int irq = vms->irqmap[VIRT_SMMU] + ARM_SPI_BASE;
> @@ -429,7 +435,7 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> build_iort_id_mapping(table_data, range->input_base,
> range->id_count, IORT_NODE_OFFSET);
> }
> - } else {
> + } else if (has_its) {
> /* output IORT node is the ITS group node (the first node) */
> build_iort_id_mapping(table_data, 0, 0x10000, IORT_NODE_OFFSET);
> }
> @@ -979,10 +985,8 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
> vms->oem_table_id);
> }
>
> - if (its_enabled(vms)) {
> - 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) {
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH-for-10.1 v3 6/9] qtest/bios-tables-test: Whitelist aarch64/virt 'its_off' variant blobs
2025-04-04 3:01 ` Gustavo Romero
@ 2025-04-09 14:05 ` Igor Mammedov
2025-04-09 15:49 ` Gustavo Romero
0 siblings, 1 reply; 30+ messages in thread
From: Igor Mammedov @ 2025-04-09 14:05 UTC (permalink / raw)
To: Gustavo Romero
Cc: Philippe Mathieu-Daudé, qemu-devel, Andrew Jones,
Alex Bennée, Michael S. Tsirkin, qemu-arm, Udo Steinberg,
Shannon Zhao, Peter Maydell, Ani Sinha
On Fri, 4 Apr 2025 00:01:22 -0300
Gustavo Romero <gustavo.romero@linaro.org> wrote:
> Hi Phil,
>
> On 4/3/25 17:40, Philippe Mathieu-Daudé wrote:
> > We are going to fix the test_acpi_aarch64_virt_tcg_its_off()
> > test. In preparation, copy the ACPI tables which will be
> > altered as 'its_off' variants, and whitelist them.
> >
> > Reviewed-by: Gustavo Romero <gustavo.romero@linaro.org>
> > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > ---
> > tests/qtest/bios-tables-test-allowed-diff.h | 3 +++
> > tests/qtest/bios-tables-test.c | 1 +
> > tests/data/acpi/aarch64/virt/APIC.its_off | Bin 0 -> 184 bytes
> > tests/data/acpi/aarch64/virt/FACP.its_off | Bin 0 -> 276 bytes
> > tests/data/acpi/aarch64/virt/IORT.its_off | Bin 0 -> 236 bytes
> > 5 files changed, 4 insertions(+)
> > create mode 100644 tests/data/acpi/aarch64/virt/APIC.its_off
> > create mode 100644 tests/data/acpi/aarch64/virt/FACP.its_off
> > create mode 100644 tests/data/acpi/aarch64/virt/IORT.its_off
> >
> > diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
> > index dfb8523c8bf..3421dd5adf3 100644
> > --- a/tests/qtest/bios-tables-test-allowed-diff.h
> > +++ b/tests/qtest/bios-tables-test-allowed-diff.h
> > @@ -1 +1,4 @@
> > /* List of comma-separated changed AML files to ignore */
> > +"tests/data/acpi/aarch64/virt/APIC.its_off",
> > +"tests/data/acpi/aarch64/virt/FACP.its_off",
> > +"tests/data/acpi/aarch64/virt/IORT.its_off",
>
> I think your first approach is the correct one: you add the blobs
> when adding the new test, so they would go into patch 5/9 in this series,
> making the test pass without adding anything to bios-tables-test-allowed-diff.h.
> Then in this patch only add the APIC.its_off table to the bios-tables-test-allowed-diff.h
> since that's the table that changes when the fix is in place, as you did in:
if APIC.its_off is the only one that's changing, but FACP/IORT blobs are the same
as suffix-less blobs, one can omit copying FACP/IORT as test harness will fallback
to suffix-less blob if the one with suffix isn't found.
if blobs are different from defaults then create empty blobs and whitelist them in the same patch
then do your changes and then update blobs & wipeout withe list.
Phil,
the process is described in doc comment at the top of tests/qtest/bios-tables-test.c
> https://mail.gnu.org/archive/html/qemu-devel/2025-03/msg07082.html
>
> Plus, adding the blobs, which are actually related to the test in the other
> patch, and ignoring them at the same time looks confusing to me. I understand
> that only the blob that changes (APIC.its_off) with the fix should be temporarily
> ignored and, later, updated, as in your first series.
>
>
> Cheers,
> Gustavo
>
> > diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
> > index baaf199e01c..55366bf4956 100644
> > --- a/tests/qtest/bios-tables-test.c
> > +++ b/tests/qtest/bios-tables-test.c
> > @@ -2151,6 +2151,7 @@ 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",
> > 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..c37d05d6e05805304f10afe73eb7cb9100fcccfa
> > GIT binary patch
> > literal 184
> > zcmZ<^@O0k6z`($=+{xeBBUr&HBEVSz2pEB4AU24G0Uik$i-7~iVgWL^17JJ`2AFzr
> > bgb+@aBn}xq0gwb2)Q)cq{30-g9B_L93G4|0
> >
> > literal 0
> > HcmV?d00001
> >
> > diff --git a/tests/data/acpi/aarch64/virt/FACP.its_off b/tests/data/acpi/aarch64/virt/FACP.its_off
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..606dac3fe4b55c31fd68b25d3a4127eeef227434
> > GIT binary patch
> > literal 276
> > zcmZ>BbPf<<WME(uaq@Te2v%^42yj*a0-z8Bhz+8t3j|P&V`N}P6&N^PpsQ~v$aVnZ
> > CVg~^L
> >
> > 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
> >
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH-for-10.1 v3 6/9] qtest/bios-tables-test: Whitelist aarch64/virt 'its_off' variant blobs
2025-04-09 14:05 ` Igor Mammedov
@ 2025-04-09 15:49 ` Gustavo Romero
2025-04-10 6:50 ` Igor Mammedov
0 siblings, 1 reply; 30+ messages in thread
From: Gustavo Romero @ 2025-04-09 15:49 UTC (permalink / raw)
To: Igor Mammedov
Cc: Philippe Mathieu-Daudé, qemu-devel, Andrew Jones,
Alex Bennée, Michael S. Tsirkin, qemu-arm, Udo Steinberg,
Shannon Zhao, Peter Maydell, Ani Sinha
Hi Igor,
On 4/9/25 11:05, Igor Mammedov wrote:
> On Fri, 4 Apr 2025 00:01:22 -0300
> Gustavo Romero <gustavo.romero@linaro.org> wrote:
>
>> Hi Phil,
>>
>> On 4/3/25 17:40, Philippe Mathieu-Daudé wrote:
>>> We are going to fix the test_acpi_aarch64_virt_tcg_its_off()
>>> test. In preparation, copy the ACPI tables which will be
>>> altered as 'its_off' variants, and whitelist them.
>>>
>>> Reviewed-by: Gustavo Romero <gustavo.romero@linaro.org>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>> tests/qtest/bios-tables-test-allowed-diff.h | 3 +++
>>> tests/qtest/bios-tables-test.c | 1 +
>>> tests/data/acpi/aarch64/virt/APIC.its_off | Bin 0 -> 184 bytes
>>> tests/data/acpi/aarch64/virt/FACP.its_off | Bin 0 -> 276 bytes
>>> tests/data/acpi/aarch64/virt/IORT.its_off | Bin 0 -> 236 bytes
>>> 5 files changed, 4 insertions(+)
>>> create mode 100644 tests/data/acpi/aarch64/virt/APIC.its_off
>>> create mode 100644 tests/data/acpi/aarch64/virt/FACP.its_off
>>> create mode 100644 tests/data/acpi/aarch64/virt/IORT.its_off
>>>
>>> diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
>>> index dfb8523c8bf..3421dd5adf3 100644
>>> --- a/tests/qtest/bios-tables-test-allowed-diff.h
>>> +++ b/tests/qtest/bios-tables-test-allowed-diff.h
>>> @@ -1 +1,4 @@
>>> /* List of comma-separated changed AML files to ignore */
>>> +"tests/data/acpi/aarch64/virt/APIC.its_off",
>>> +"tests/data/acpi/aarch64/virt/FACP.its_off",
>>> +"tests/data/acpi/aarch64/virt/IORT.its_off",
>>
>> I think your first approach is the correct one: you add the blobs
>> when adding the new test, so they would go into patch 5/9 in this series,
>> making the test pass without adding anything to bios-tables-test-allowed-diff.h.
>> Then in this patch only add the APIC.its_off table to the bios-tables-test-allowed-diff.h
>> since that's the table that changes when the fix is in place, as you did in:
>
> if APIC.its_off is the only one that's changing, but FACP/IORT blobs are the same
> as suffix-less blobs, one can omit copying FACP/IORT as test harness will fallback
> to suffix-less blob if the one with suffix isn't found.
OK. Just clarifying and for the records, this is not the case for this series
> if blobs are different from defaults then create empty blobs and whitelist them in the same patch
> then do your changes and then update blobs & wipeout withe list.
Thanks for confirming it. That's what I suggested to Phil in my first review and what
I understood from the prescription in bios-tables-test.c.
However, on second thoughts, for this particular series, isn't it better to have the following commit sequence instead:
1) Add the new test and the new blobs that make the test pass, i.e. APIC.suffix, FACP.suffix, and IORT.suffix (they are different than the default suffix-less blobs)
2) Whitelist only the APIC.suffix since that's the table that will change with the fix
3) Add the fix (which changes the APIC table so a new APIC.suffix blob is needed and also stops generating the IORT table, so no more IORT.suffix blob is necessary)
4) Finally, update only the APIC.suffix blob and remove the IORT.suffix blob and wipe out the whitelist
This way:
A) It's clear that only ACPI blob changed with the fix, because there is no addition of a FACP.suffix blob in 4) (it remains the same)
B) It's clear that the IORT table is removed with the fix and is not relevant anymore for the test
What do you think?
Cheers,
Gustavo
> Phil,
> the process is described in doc comment at the top of tests/qtest/bios-tables-test.c
>
>> https://mail.gnu.org/archive/html/qemu-devel/2025-03/msg07082.html
>>
>> Plus, adding the blobs, which are actually related to the test in the other
>> patch, and ignoring them at the same time looks confusing to me. I understand
>> that only the blob that changes (APIC.its_off) with the fix should be temporarily
>> ignored and, later, updated, as in your first series.
>>
>>
>> Cheers,
>> Gustavo
>>
>>> diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
>>> index baaf199e01c..55366bf4956 100644
>>> --- a/tests/qtest/bios-tables-test.c
>>> +++ b/tests/qtest/bios-tables-test.c
>>> @@ -2151,6 +2151,7 @@ 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",
>>> 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..c37d05d6e05805304f10afe73eb7cb9100fcccfa
>>> GIT binary patch
>>> literal 184
>>> zcmZ<^@O0k6z`($=+{xeBBUr&HBEVSz2pEB4AU24G0Uik$i-7~iVgWL^17JJ`2AFzr
>>> bgb+@aBn}xq0gwb2)Q)cq{30-g9B_L93G4|0
>>>
>>> literal 0
>>> HcmV?d00001
>>>
>>> diff --git a/tests/data/acpi/aarch64/virt/FACP.its_off b/tests/data/acpi/aarch64/virt/FACP.its_off
>>> new file mode 100644
>>> index 0000000000000000000000000000000000000000..606dac3fe4b55c31fd68b25d3a4127eeef227434
>>> GIT binary patch
>>> literal 276
>>> zcmZ>BbPf<<WME(uaq@Te2v%^42yj*a0-z8Bhz+8t3j|P&V`N}P6&N^PpsQ~v$aVnZ
>>> CVg~^L
>>>
>>> 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
>>>
>>
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH-for-10.1 v3 6/9] qtest/bios-tables-test: Whitelist aarch64/virt 'its_off' variant blobs
2025-04-09 15:49 ` Gustavo Romero
@ 2025-04-10 6:50 ` Igor Mammedov
2025-04-10 16:22 ` Gustavo Romero
0 siblings, 1 reply; 30+ messages in thread
From: Igor Mammedov @ 2025-04-10 6:50 UTC (permalink / raw)
To: Gustavo Romero
Cc: Philippe Mathieu-Daudé, qemu-devel, Andrew Jones,
Alex Bennée, Michael S. Tsirkin, qemu-arm, Udo Steinberg,
Shannon Zhao, Peter Maydell, Ani Sinha
On Wed, 9 Apr 2025 12:49:36 -0300
Gustavo Romero <gustavo.romero@linaro.org> wrote:
> Hi Igor,
>
> On 4/9/25 11:05, Igor Mammedov wrote:
> > On Fri, 4 Apr 2025 00:01:22 -0300
> > Gustavo Romero <gustavo.romero@linaro.org> wrote:
> >
> >> Hi Phil,
> >>
> >> On 4/3/25 17:40, Philippe Mathieu-Daudé wrote:
> >>> We are going to fix the test_acpi_aarch64_virt_tcg_its_off()
> >>> test. In preparation, copy the ACPI tables which will be
> >>> altered as 'its_off' variants, and whitelist them.
> >>>
> >>> Reviewed-by: Gustavo Romero <gustavo.romero@linaro.org>
> >>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> >>> ---
> >>> tests/qtest/bios-tables-test-allowed-diff.h | 3 +++
> >>> tests/qtest/bios-tables-test.c | 1 +
> >>> tests/data/acpi/aarch64/virt/APIC.its_off | Bin 0 -> 184 bytes
> >>> tests/data/acpi/aarch64/virt/FACP.its_off | Bin 0 -> 276 bytes
> >>> tests/data/acpi/aarch64/virt/IORT.its_off | Bin 0 -> 236 bytes
> >>> 5 files changed, 4 insertions(+)
> >>> create mode 100644 tests/data/acpi/aarch64/virt/APIC.its_off
> >>> create mode 100644 tests/data/acpi/aarch64/virt/FACP.its_off
> >>> create mode 100644 tests/data/acpi/aarch64/virt/IORT.its_off
> >>>
> >>> diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
> >>> index dfb8523c8bf..3421dd5adf3 100644
> >>> --- a/tests/qtest/bios-tables-test-allowed-diff.h
> >>> +++ b/tests/qtest/bios-tables-test-allowed-diff.h
> >>> @@ -1 +1,4 @@
> >>> /* List of comma-separated changed AML files to ignore */
> >>> +"tests/data/acpi/aarch64/virt/APIC.its_off",
> >>> +"tests/data/acpi/aarch64/virt/FACP.its_off",
> >>> +"tests/data/acpi/aarch64/virt/IORT.its_off",
> >>
> >> I think your first approach is the correct one: you add the blobs
> >> when adding the new test, so they would go into patch 5/9 in this series,
> >> making the test pass without adding anything to bios-tables-test-allowed-diff.h.
> >> Then in this patch only add the APIC.its_off table to the bios-tables-test-allowed-diff.h
> >> since that's the table that changes when the fix is in place, as you did in:
> >
> > if APIC.its_off is the only one that's changing, but FACP/IORT blobs are the same
> > as suffix-less blobs, one can omit copying FACP/IORT as test harness will fallback
> > to suffix-less blob if the one with suffix isn't found.
>
> OK. Just clarifying and for the records, this is not the case for this series
>
>
> > if blobs are different from defaults then create empty blobs and whitelist them in the same patch
> > then do your changes and then update blobs & wipeout withe list.
>
> Thanks for confirming it. That's what I suggested to Phil in my first review and what
> I understood from the prescription in bios-tables-test.c.
>
> However, on second thoughts, for this particular series, isn't it better to have the following commit sequence instead:
>
> 1) Add the new test and the new blobs that make the test pass, i.e. APIC.suffix, FACP.suffix, and IORT.suffix (they are different than the default suffix-less blobs)
blobs should be a separate commit (that way it's easier for maintainer to rebase them,
if they clash during merge with some other change.
> 2) Whitelist only the APIC.suffix since that's the table that will change with the fix
> 3) Add the fix (which changes the APIC table so a new APIC.suffix blob is needed and also stops generating the IORT table, so no more IORT.suffix blob is necessary)
> 4) Finally, update only the APIC.suffix blob and remove the IORT.suffix blob and wipe out the whitelist
>
> This way:
>
> A) It's clear that only ACPI blob changed with the fix, because there is no addition of a FACP.suffix blob in 4) (it remains the same)
> B) It's clear that the IORT table is removed with the fix and is not relevant anymore for the test
I'd just mention it in commit log so that later no one would wonder why we are adding and then removing tables
As for the rest of suggestions, it looks fine to me.
>
> What do you think?
>
>
> Cheers,
> Gustavo
>
> > Phil,
> > the process is described in doc comment at the top of tests/qtest/bios-tables-test.c
> >
> >> https://mail.gnu.org/archive/html/qemu-devel/2025-03/msg07082.html
> >>
> >> Plus, adding the blobs, which are actually related to the test in the other
> >> patch, and ignoring them at the same time looks confusing to me. I understand
> >> that only the blob that changes (APIC.its_off) with the fix should be temporarily
> >> ignored and, later, updated, as in your first series.
> >>
> >>
> >> Cheers,
> >> Gustavo
> >>
> >>> diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
> >>> index baaf199e01c..55366bf4956 100644
> >>> --- a/tests/qtest/bios-tables-test.c
> >>> +++ b/tests/qtest/bios-tables-test.c
> >>> @@ -2151,6 +2151,7 @@ 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",
> >>> 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..c37d05d6e05805304f10afe73eb7cb9100fcccfa
> >>> GIT binary patch
> >>> literal 184
> >>> zcmZ<^@O0k6z`($=+{xeBBUr&HBEVSz2pEB4AU24G0Uik$i-7~iVgWL^17JJ`2AFzr
> >>> bgb+@aBn}xq0gwb2)Q)cq{30-g9B_L93G4|0
> >>>
> >>> literal 0
> >>> HcmV?d00001
> >>>
> >>> diff --git a/tests/data/acpi/aarch64/virt/FACP.its_off b/tests/data/acpi/aarch64/virt/FACP.its_off
> >>> new file mode 100644
> >>> index 0000000000000000000000000000000000000000..606dac3fe4b55c31fd68b25d3a4127eeef227434
> >>> GIT binary patch
> >>> literal 276
> >>> zcmZ>BbPf<<WME(uaq@Te2v%^42yj*a0-z8Bhz+8t3j|P&V`N}P6&N^PpsQ~v$aVnZ
> >>> CVg~^L
> >>>
> >>> 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
> >>>
> >>
> >
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH-for-10.1 v3 6/9] qtest/bios-tables-test: Whitelist aarch64/virt 'its_off' variant blobs
2025-04-10 6:50 ` Igor Mammedov
@ 2025-04-10 16:22 ` Gustavo Romero
2025-04-17 21:06 ` Gustavo Romero
2025-05-06 15:36 ` Igor Mammedov
0 siblings, 2 replies; 30+ messages in thread
From: Gustavo Romero @ 2025-04-10 16:22 UTC (permalink / raw)
To: Igor Mammedov
Cc: Philippe Mathieu-Daudé, qemu-devel, Andrew Jones,
Alex Bennée, Michael S. Tsirkin, qemu-arm, Udo Steinberg,
Shannon Zhao, Peter Maydell, Ani Sinha
Hi Igor,
On 4/10/25 03:50, Igor Mammedov wrote:
> On Wed, 9 Apr 2025 12:49:36 -0300
> Gustavo Romero <gustavo.romero@linaro.org> wrote:
>
>> Hi Igor,
>>
>> On 4/9/25 11:05, Igor Mammedov wrote:
>>> On Fri, 4 Apr 2025 00:01:22 -0300
>>> Gustavo Romero <gustavo.romero@linaro.org> wrote:
>>>
>>>> Hi Phil,
>>>>
>>>> On 4/3/25 17:40, Philippe Mathieu-Daudé wrote:
>>>>> We are going to fix the test_acpi_aarch64_virt_tcg_its_off()
>>>>> test. In preparation, copy the ACPI tables which will be
>>>>> altered as 'its_off' variants, and whitelist them.
>>>>>
>>>>> Reviewed-by: Gustavo Romero <gustavo.romero@linaro.org>
>>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>>> ---
>>>>> tests/qtest/bios-tables-test-allowed-diff.h | 3 +++
>>>>> tests/qtest/bios-tables-test.c | 1 +
>>>>> tests/data/acpi/aarch64/virt/APIC.its_off | Bin 0 -> 184 bytes
>>>>> tests/data/acpi/aarch64/virt/FACP.its_off | Bin 0 -> 276 bytes
>>>>> tests/data/acpi/aarch64/virt/IORT.its_off | Bin 0 -> 236 bytes
>>>>> 5 files changed, 4 insertions(+)
>>>>> create mode 100644 tests/data/acpi/aarch64/virt/APIC.its_off
>>>>> create mode 100644 tests/data/acpi/aarch64/virt/FACP.its_off
>>>>> create mode 100644 tests/data/acpi/aarch64/virt/IORT.its_off
>>>>>
>>>>> diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
>>>>> index dfb8523c8bf..3421dd5adf3 100644
>>>>> --- a/tests/qtest/bios-tables-test-allowed-diff.h
>>>>> +++ b/tests/qtest/bios-tables-test-allowed-diff.h
>>>>> @@ -1 +1,4 @@
>>>>> /* List of comma-separated changed AML files to ignore */
>>>>> +"tests/data/acpi/aarch64/virt/APIC.its_off",
>>>>> +"tests/data/acpi/aarch64/virt/FACP.its_off",
>>>>> +"tests/data/acpi/aarch64/virt/IORT.its_off",
>>>>
>>>> I think your first approach is the correct one: you add the blobs
>>>> when adding the new test, so they would go into patch 5/9 in this series,
>>>> making the test pass without adding anything to bios-tables-test-allowed-diff.h.
>>>> Then in this patch only add the APIC.its_off table to the bios-tables-test-allowed-diff.h
>>>> since that's the table that changes when the fix is in place, as you did in:
>>>
>>> if APIC.its_off is the only one that's changing, but FACP/IORT blobs are the same
>>> as suffix-less blobs, one can omit copying FACP/IORT as test harness will fallback
>>> to suffix-less blob if the one with suffix isn't found.
>>
>> OK. Just clarifying and for the records, this is not the case for this series
>>
>>
>>> if blobs are different from defaults then create empty blobs and whitelist them in the same patch
>>> then do your changes and then update blobs & wipeout withe list.
>>
>> Thanks for confirming it. That's what I suggested to Phil in my first review and what
>> I understood from the prescription in bios-tables-test.c.
>>
>> However, on second thoughts, for this particular series, isn't it better to have the following commit sequence instead:
>>
>> 1) Add the new test and the new blobs that make the test pass, i.e. APIC.suffix, FACP.suffix, and IORT.suffix (they are different than the default suffix-less blobs)
>
> blobs should be a separate commit (that way it's easier for maintainer to rebase them,
> if they clash during merge with some other change.
I see. What is a bit confusing here is that the series consists in
one blob addition act (for the new test) and one blob update/removal act (after the fix).
>> 2) Whitelist only the APIC.suffix since that's the table that will change with the fix
>> 3) Add the fix (which changes the APIC table so a new APIC.suffix blob is needed and also stops generating the IORT table, so no more IORT.suffix blob is necessary)
>> 4) Finally, update only the APIC.suffix blob and remove the IORT.suffix blob and wipe out the whitelist
>>
>> This way:
>>
>> A) It's clear that only ACPI blob changed with the fix, because there is no addition of a FACP.suffix blob in 4) (it remains the same)
>> B) It's clear that the IORT table is removed with the fix and is not relevant anymore for the test
>
> I'd just mention it in commit log so that later no one would wonder why we are adding and then removing tables
>
> As for the rest of suggestions, it looks fine to me.
Well, 2) won't make sense anymore since APIC.suffix would be already in the
whitelist in the previous patch that added the empty blobs. Since there won't
be a commit that adds _only_ the APIC.suffix to the whitelist, in preparation
for the fix, this info is "lost" in the series, even tho it's possible to
mention in the commit message.
Hence, what I think is not ideal from a maintainer's/reviewer's perspective,
is that in one commit all the blobs are updated/removed at once, which is
confusing because the fix did not touch the FACP table (for instance) and
this table is updated with APIC and with the removal of IORT, altogether,
in the last commit.
So, for this series, which adds new blobs and _also_ updates and removes some
of them, how about the following organization:
- 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
- Patch 3 : Add the APIC.suffix blob to the whitelist (the table that changes due to the fix)
- Patch 4 - n : Fix(es)
- Patch (n+1) : Update the APIC.suffix blob, remove IORT.suffix blob, and remove the APIC.suffix blob from the whitelist
* Add the APIC diff to the commit log
* Mention in the commit log that IORT.suffix is removed because IORT table is no long generated after the fix
This way: a) no commit fails the test and b) blobs are added/updated/removed in separate commits
What do you think?
Cheers,
Gustavo
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH-for-10.1 v3 0/9] hw/arm: GIC ITS=off ACPI tables fixes
2025-04-03 20:40 [PATCH-for-10.1 v3 0/9] hw/arm: GIC ITS=off ACPI tables fixes Philippe Mathieu-Daudé
` (8 preceding siblings ...)
2025-04-03 20:40 ` [PATCH-for-10.1 v3 9/9] qtest/bios-tables-test: Update aarch64/virt 'its_off' variant blobs Philippe Mathieu-Daudé
@ 2025-04-15 8:19 ` Philippe Mathieu-Daudé
2025-04-17 21:08 ` Gustavo Romero
9 siblings, 1 reply; 30+ messages in thread
From: Philippe Mathieu-Daudé @ 2025-04-15 8:19 UTC (permalink / raw)
To: Gustavo Romero, Alex Bennée
Cc: qemu-devel, Andrew Jones, Michael S. Tsirkin, qemu-arm,
Udo Steinberg, Shannon Zhao, Igor Mammedov, Peter Maydell,
Ani Sinha
Hi Gustavo,
Could you take over this series? You seem better suited
with ACPI and testing with Aarch64 virt machine :)
Thanks,
Phil.
On 3/4/25 22:40, Philippe Mathieu-Daudé wrote:
>
> Fix ACPI tables for '-M its=off' CLI option.
>
> Regards,
>
> Phil.
>
> Philippe Mathieu-Daudé (9):
> hw/arm/virt: Remove pointless VirtMachineState::tcg_its field
> hw/intc/gicv3_its: Do not check its_class_name() for NULL
> hw/arm/virt: Simplify create_its()
> hw/arm/virt-acpi: Factor its_enabled() helper out
> qtest/bios-tables-test: Add test for -M virt,its=off
> qtest/bios-tables-test: Whitelist aarch64/virt 'its_off' variant blobs
> hw/arm/virt-acpi: Always build IORT table (even with GIC ITS disabled)
> hw/arm/virt-acpi: Do not advertise disabled GIC ITS
> qtest/bios-tables-test: Update aarch64/virt 'its_off' variant blobs
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH-for-10.1 v3 6/9] qtest/bios-tables-test: Whitelist aarch64/virt 'its_off' variant blobs
2025-04-10 16:22 ` Gustavo Romero
@ 2025-04-17 21:06 ` Gustavo Romero
2025-04-17 21:21 ` Michael S. Tsirkin
2025-05-06 15:36 ` Igor Mammedov
1 sibling, 1 reply; 30+ messages in thread
From: Gustavo Romero @ 2025-04-17 21:06 UTC (permalink / raw)
To: Igor Mammedov, Michael S . Tsirkin
Cc: Philippe Mathieu-Daudé, qemu-devel, Andrew Jones,
Alex Bennée, qemu-arm, Udo Steinberg, Shannon Zhao,
Peter Maydell, Ani Sinha
Hi Igor and Michael,
On 4/10/25 13:22, Gustavo Romero wrote:
> Hi Igor,
>
> On 4/10/25 03:50, Igor Mammedov wrote:
>> On Wed, 9 Apr 2025 12:49:36 -0300
>> Gustavo Romero <gustavo.romero@linaro.org> wrote:
>>
>>> Hi Igor,
>>>
>>> On 4/9/25 11:05, Igor Mammedov wrote:
>>>> On Fri, 4 Apr 2025 00:01:22 -0300
>>>> Gustavo Romero <gustavo.romero@linaro.org> wrote:
>>>>> Hi Phil,
>>>>>
>>>>> On 4/3/25 17:40, Philippe Mathieu-Daudé wrote:
>>>>>> We are going to fix the test_acpi_aarch64_virt_tcg_its_off()
>>>>>> test. In preparation, copy the ACPI tables which will be
>>>>>> altered as 'its_off' variants, and whitelist them.
>>>>>>
>>>>>> Reviewed-by: Gustavo Romero <gustavo.romero@linaro.org>
>>>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>>>> ---
>>>>>> tests/qtest/bios-tables-test-allowed-diff.h | 3 +++
>>>>>> tests/qtest/bios-tables-test.c | 1 +
>>>>>> tests/data/acpi/aarch64/virt/APIC.its_off | Bin 0 -> 184 bytes
>>>>>> tests/data/acpi/aarch64/virt/FACP.its_off | Bin 0 -> 276 bytes
>>>>>> tests/data/acpi/aarch64/virt/IORT.its_off | Bin 0 -> 236 bytes
>>>>>> 5 files changed, 4 insertions(+)
>>>>>> create mode 100644 tests/data/acpi/aarch64/virt/APIC.its_off
>>>>>> create mode 100644 tests/data/acpi/aarch64/virt/FACP.its_off
>>>>>> create mode 100644 tests/data/acpi/aarch64/virt/IORT.its_off
>>>>>>
>>>>>> diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
>>>>>> index dfb8523c8bf..3421dd5adf3 100644
>>>>>> --- a/tests/qtest/bios-tables-test-allowed-diff.h
>>>>>> +++ b/tests/qtest/bios-tables-test-allowed-diff.h
>>>>>> @@ -1 +1,4 @@
>>>>>> /* List of comma-separated changed AML files to ignore */
>>>>>> +"tests/data/acpi/aarch64/virt/APIC.its_off",
>>>>>> +"tests/data/acpi/aarch64/virt/FACP.its_off",
>>>>>> +"tests/data/acpi/aarch64/virt/IORT.its_off",
>>>>>
>>>>> I think your first approach is the correct one: you add the blobs
>>>>> when adding the new test, so they would go into patch 5/9 in this series,
>>>>> making the test pass without adding anything to bios-tables-test-allowed-diff.h.
>>>>> Then in this patch only add the APIC.its_off table to the bios-tables-test-allowed-diff.h
>>>>> since that's the table that changes when the fix is in place, as you did in:
>>>>
>>>> if APIC.its_off is the only one that's changing, but FACP/IORT blobs are the same
>>>> as suffix-less blobs, one can omit copying FACP/IORT as test harness will fallback
>>>> to suffix-less blob if the one with suffix isn't found.
>>>
>>> OK. Just clarifying and for the records, this is not the case for this series
>>>
>>>
>>>> if blobs are different from defaults then create empty blobs and whitelist them in the same patch
>>>> then do your changes and then update blobs & wipeout withe list.
>>>
>>> Thanks for confirming it. That's what I suggested to Phil in my first review and what
>>> I understood from the prescription in bios-tables-test.c.
>>>
>>> However, on second thoughts, for this particular series, isn't it better to have the following commit sequence instead:
>>>
>>> 1) Add the new test and the new blobs that make the test pass, i.e. APIC.suffix, FACP.suffix, and IORT.suffix (they are different than the default suffix-less blobs)
>>
>> blobs should be a separate commit (that way it's easier for maintainer to rebase them,
>> if they clash during merge with some other change.
>
> I see. What is a bit confusing here is that the series consists in
> one blob addition act (for the new test) and one blob update/removal act (after the fix).
>
>
>>> 2) Whitelist only the APIC.suffix since that's the table that will change with the fix
>>> 3) Add the fix (which changes the APIC table so a new APIC.suffix blob is needed and also stops generating the IORT table, so no more IORT.suffix blob is necessary)
>>> 4) Finally, update only the APIC.suffix blob and remove the IORT.suffix blob and wipe out the whitelist
>>>
>>> This way:
>>>
>>> A) It's clear that only ACPI blob changed with the fix, because there is no addition of a FACP.suffix blob in 4) (it remains the same)
>>> B) It's clear that the IORT table is removed with the fix and is not relevant anymore for the test
>>
>> I'd just mention it in commit log so that later no one would wonder why we are adding and then removing tables
>>
>> As for the rest of suggestions, it looks fine to me.
>
> Well, 2) won't make sense anymore since APIC.suffix would be already in the
> whitelist in the previous patch that added the empty blobs. Since there won't
> be a commit that adds _only_ the APIC.suffix to the whitelist, in preparation
> for the fix, this info is "lost" in the series, even tho it's possible to
> mention in the commit message.
>
> Hence, what I think is not ideal from a maintainer's/reviewer's perspective,
> is that in one commit all the blobs are updated/removed at once, which is
> confusing because the fix did not touch the FACP table (for instance) and
> this table is updated with APIC and with the removal of IORT, altogether,
> in the last commit.
>
> So, for this series, which adds new blobs and _also_ updates and removes some
> of them, how about the following organization:
>
> - 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
>
> - Patch 3 : Add the APIC.suffix blob to the whitelist (the table that changes due to the fix)
> - Patch 4 - n : Fix(es)
> - Patch (n+1) : Update the APIC.suffix blob, remove IORT.suffix blob, and remove the APIC.suffix blob from the whitelist
> * Add the APIC diff to the commit log
> * Mention in the commit log that IORT.suffix is removed because IORT table is no long generated after the fix
>
> This way: a) no commit fails the test and b) blobs are added/updated/removed in separate commits
>
> What do you think?
I’d really appreciate it if you could confirm whether this organization makes sense.
Cheers,
Gustavo
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH-for-10.1 v3 0/9] hw/arm: GIC ITS=off ACPI tables fixes
2025-04-15 8:19 ` [PATCH-for-10.1 v3 0/9] hw/arm: GIC ITS=off ACPI tables fixes Philippe Mathieu-Daudé
@ 2025-04-17 21:08 ` Gustavo Romero
0 siblings, 0 replies; 30+ messages in thread
From: Gustavo Romero @ 2025-04-17 21:08 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, Alex Bennée
Cc: qemu-devel, Andrew Jones, Michael S. Tsirkin, qemu-arm,
Udo Steinberg, Shannon Zhao, Igor Mammedov, Peter Maydell,
Ani Sinha
Hi Phil,
On 4/15/25 05:19, Philippe Mathieu-Daudé wrote:
> Hi Gustavo,
>
> Could you take over this series? You seem better suited
> with ACPI and testing with Aarch64 virt machine :)
>
> Thanks,
Sure. I just need to clarify a couple of things regarding
the IORT spec. Meanwhile I'm (hopefully) waiting Igor or
Michael confirm that the patch organization I've suggested
makes sense for them.
Cheers,
Gustavo
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH-for-10.1 v3 6/9] qtest/bios-tables-test: Whitelist aarch64/virt 'its_off' variant blobs
2025-04-17 21:06 ` Gustavo Romero
@ 2025-04-17 21:21 ` Michael S. Tsirkin
0 siblings, 0 replies; 30+ messages in thread
From: Michael S. Tsirkin @ 2025-04-17 21:21 UTC (permalink / raw)
To: Gustavo Romero
Cc: Igor Mammedov, Philippe Mathieu-Daudé, qemu-devel,
Andrew Jones, Alex Bennée, qemu-arm, Udo Steinberg,
Shannon Zhao, Peter Maydell, Ani Sinha
On Thu, Apr 17, 2025 at 06:06:14PM -0300, Gustavo Romero wrote:
> Hi Igor and Michael,
>
> On 4/10/25 13:22, Gustavo Romero wrote:
> > Hi Igor,
> >
> > On 4/10/25 03:50, Igor Mammedov wrote:
> > > On Wed, 9 Apr 2025 12:49:36 -0300
> > > Gustavo Romero <gustavo.romero@linaro.org> wrote:
> > >
> > > > Hi Igor,
> > > >
> > > > On 4/9/25 11:05, Igor Mammedov wrote:
> > > > > On Fri, 4 Apr 2025 00:01:22 -0300
> > > > > Gustavo Romero <gustavo.romero@linaro.org> wrote:
> > > > > > Hi Phil,
> > > > > >
> > > > > > On 4/3/25 17:40, Philippe Mathieu-Daudé wrote:
> > > > > > > We are going to fix the test_acpi_aarch64_virt_tcg_its_off()
> > > > > > > test. In preparation, copy the ACPI tables which will be
> > > > > > > altered as 'its_off' variants, and whitelist them.
> > > > > > >
> > > > > > > Reviewed-by: Gustavo Romero <gustavo.romero@linaro.org>
> > > > > > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > > > > > > ---
> > > > > > > tests/qtest/bios-tables-test-allowed-diff.h | 3 +++
> > > > > > > tests/qtest/bios-tables-test.c | 1 +
> > > > > > > tests/data/acpi/aarch64/virt/APIC.its_off | Bin 0 -> 184 bytes
> > > > > > > tests/data/acpi/aarch64/virt/FACP.its_off | Bin 0 -> 276 bytes
> > > > > > > tests/data/acpi/aarch64/virt/IORT.its_off | Bin 0 -> 236 bytes
> > > > > > > 5 files changed, 4 insertions(+)
> > > > > > > create mode 100644 tests/data/acpi/aarch64/virt/APIC.its_off
> > > > > > > create mode 100644 tests/data/acpi/aarch64/virt/FACP.its_off
> > > > > > > create mode 100644 tests/data/acpi/aarch64/virt/IORT.its_off
> > > > > > >
> > > > > > > diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
> > > > > > > index dfb8523c8bf..3421dd5adf3 100644
> > > > > > > --- a/tests/qtest/bios-tables-test-allowed-diff.h
> > > > > > > +++ b/tests/qtest/bios-tables-test-allowed-diff.h
> > > > > > > @@ -1 +1,4 @@
> > > > > > > /* List of comma-separated changed AML files to ignore */
> > > > > > > +"tests/data/acpi/aarch64/virt/APIC.its_off",
> > > > > > > +"tests/data/acpi/aarch64/virt/FACP.its_off",
> > > > > > > +"tests/data/acpi/aarch64/virt/IORT.its_off",
> > > > > >
> > > > > > I think your first approach is the correct one: you add the blobs
> > > > > > when adding the new test, so they would go into patch 5/9 in this series,
> > > > > > making the test pass without adding anything to bios-tables-test-allowed-diff.h.
> > > > > > Then in this patch only add the APIC.its_off table to the bios-tables-test-allowed-diff.h
> > > > > > since that's the table that changes when the fix is in place, as you did in:
> > > > >
> > > > > if APIC.its_off is the only one that's changing, but FACP/IORT blobs are the same
> > > > > as suffix-less blobs, one can omit copying FACP/IORT as test harness will fallback
> > > > > to suffix-less blob if the one with suffix isn't found.
> > > >
> > > > OK. Just clarifying and for the records, this is not the case for this series
> > > >
> > > >
> > > > > if blobs are different from defaults then create empty blobs and whitelist them in the same patch
> > > > > then do your changes and then update blobs & wipeout withe list.
> > > >
> > > > Thanks for confirming it. That's what I suggested to Phil in my first review and what
> > > > I understood from the prescription in bios-tables-test.c.
> > > >
> > > > However, on second thoughts, for this particular series, isn't it better to have the following commit sequence instead:
> > > >
> > > > 1) Add the new test and the new blobs that make the test pass, i.e. APIC.suffix, FACP.suffix, and IORT.suffix (they are different than the default suffix-less blobs)
> > >
> > > blobs should be a separate commit (that way it's easier for maintainer to rebase them,
> > > if they clash during merge with some other change.
> >
> > I see. What is a bit confusing here is that the series consists in
> > one blob addition act (for the new test) and one blob update/removal act (after the fix).
> >
> >
> > > > 2) Whitelist only the APIC.suffix since that's the table that will change with the fix
> > > > 3) Add the fix (which changes the APIC table so a new APIC.suffix blob is needed and also stops generating the IORT table, so no more IORT.suffix blob is necessary)
> > > > 4) Finally, update only the APIC.suffix blob and remove the IORT.suffix blob and wipe out the whitelist
> > > >
> > > > This way:
> > > >
> > > > A) It's clear that only ACPI blob changed with the fix, because there is no addition of a FACP.suffix blob in 4) (it remains the same)
> > > > B) It's clear that the IORT table is removed with the fix and is not relevant anymore for the test
> > >
> > > I'd just mention it in commit log so that later no one would wonder why we are adding and then removing tables
> > >
> > > As for the rest of suggestions, it looks fine to me.
> >
> > Well, 2) won't make sense anymore since APIC.suffix would be already in the
> > whitelist in the previous patch that added the empty blobs. Since there won't
> > be a commit that adds _only_ the APIC.suffix to the whitelist, in preparation
> > for the fix, this info is "lost" in the series, even tho it's possible to
> > mention in the commit message.
> >
> > Hence, what I think is not ideal from a maintainer's/reviewer's perspective,
> > is that in one commit all the blobs are updated/removed at once, which is
> > confusing because the fix did not touch the FACP table (for instance) and
> > this table is updated with APIC and with the removal of IORT, altogether,
> > in the last commit.
> >
> > So, for this series, which adds new blobs and _also_ updates and removes some
> > of them, how about the following organization:
> >
> > - 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
> >
> > - Patch 3 : Add the APIC.suffix blob to the whitelist (the table that changes due to the fix)
> > - Patch 4 - n : Fix(es)
> > - Patch (n+1) : Update the APIC.suffix blob, remove IORT.suffix blob, and remove the APIC.suffix blob from the whitelist
> > * Add the APIC diff to the commit log
> > * Mention in the commit log that IORT.suffix is removed because IORT table is no long generated after the fix
> >
> > This way: a) no commit fails the test and b) blobs are added/updated/removed in separate commits
> >
> > What do you think?
>
> I’d really appreciate it if you could confirm whether this organization makes sense.
>
>
> Cheers,
> Gustavo
looks ok. Igor?
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH-for-10.1 v3 6/9] qtest/bios-tables-test: Whitelist aarch64/virt 'its_off' variant blobs
2025-04-10 16:22 ` Gustavo Romero
2025-04-17 21:06 ` Gustavo Romero
@ 2025-05-06 15:36 ` Igor Mammedov
1 sibling, 0 replies; 30+ messages in thread
From: Igor Mammedov @ 2025-05-06 15:36 UTC (permalink / raw)
To: Gustavo Romero
Cc: Philippe Mathieu-Daudé, qemu-devel, Andrew Jones,
Alex Bennée, Michael S. Tsirkin, qemu-arm, Udo Steinberg,
Shannon Zhao, Peter Maydell, Ani Sinha
On Thu, 10 Apr 2025 13:22:06 -0300
Gustavo Romero <gustavo.romero@linaro.org> wrote:
> Hi Igor,
>
> On 4/10/25 03:50, Igor Mammedov wrote:
> > On Wed, 9 Apr 2025 12:49:36 -0300
> > Gustavo Romero <gustavo.romero@linaro.org> wrote:
> >
> >> Hi Igor,
> >>
> >> On 4/9/25 11:05, Igor Mammedov wrote:
> >>> On Fri, 4 Apr 2025 00:01:22 -0300
> >>> Gustavo Romero <gustavo.romero@linaro.org> wrote:
> >>>
> >>>> Hi Phil,
> >>>>
> >>>> On 4/3/25 17:40, Philippe Mathieu-Daudé wrote:
> >>>>> We are going to fix the test_acpi_aarch64_virt_tcg_its_off()
> >>>>> test. In preparation, copy the ACPI tables which will be
> >>>>> altered as 'its_off' variants, and whitelist them.
> >>>>>
> >>>>> Reviewed-by: Gustavo Romero <gustavo.romero@linaro.org>
> >>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> >>>>> ---
> >>>>> tests/qtest/bios-tables-test-allowed-diff.h | 3 +++
> >>>>> tests/qtest/bios-tables-test.c | 1 +
> >>>>> tests/data/acpi/aarch64/virt/APIC.its_off | Bin 0 -> 184 bytes
> >>>>> tests/data/acpi/aarch64/virt/FACP.its_off | Bin 0 -> 276 bytes
> >>>>> tests/data/acpi/aarch64/virt/IORT.its_off | Bin 0 -> 236 bytes
> >>>>> 5 files changed, 4 insertions(+)
> >>>>> create mode 100644 tests/data/acpi/aarch64/virt/APIC.its_off
> >>>>> create mode 100644 tests/data/acpi/aarch64/virt/FACP.its_off
> >>>>> create mode 100644 tests/data/acpi/aarch64/virt/IORT.its_off
> >>>>>
> >>>>> diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
> >>>>> index dfb8523c8bf..3421dd5adf3 100644
> >>>>> --- a/tests/qtest/bios-tables-test-allowed-diff.h
> >>>>> +++ b/tests/qtest/bios-tables-test-allowed-diff.h
> >>>>> @@ -1 +1,4 @@
> >>>>> /* List of comma-separated changed AML files to ignore */
> >>>>> +"tests/data/acpi/aarch64/virt/APIC.its_off",
> >>>>> +"tests/data/acpi/aarch64/virt/FACP.its_off",
> >>>>> +"tests/data/acpi/aarch64/virt/IORT.its_off",
> >>>>
> >>>> I think your first approach is the correct one: you add the blobs
> >>>> when adding the new test, so they would go into patch 5/9 in this series,
> >>>> making the test pass without adding anything to bios-tables-test-allowed-diff.h.
> >>>> Then in this patch only add the APIC.its_off table to the bios-tables-test-allowed-diff.h
> >>>> since that's the table that changes when the fix is in place, as you did in:
> >>>
> >>> if APIC.its_off is the only one that's changing, but FACP/IORT blobs are the same
> >>> as suffix-less blobs, one can omit copying FACP/IORT as test harness will fallback
> >>> to suffix-less blob if the one with suffix isn't found.
> >>
> >> OK. Just clarifying and for the records, this is not the case for this series
> >>
> >>
> >>> if blobs are different from defaults then create empty blobs and whitelist them in the same patch
> >>> then do your changes and then update blobs & wipeout withe list.
> >>
> >> Thanks for confirming it. That's what I suggested to Phil in my first review and what
> >> I understood from the prescription in bios-tables-test.c.
> >>
> >> However, on second thoughts, for this particular series, isn't it better to have the following commit sequence instead:
> >>
> >> 1) Add the new test and the new blobs that make the test pass, i.e. APIC.suffix, FACP.suffix, and IORT.suffix (they are different than the default suffix-less blobs)
> >
> > blobs should be a separate commit (that way it's easier for maintainer to rebase them,
> > if they clash during merge with some other change.
>
> I see. What is a bit confusing here is that the series consists in
> one blob addition act (for the new test) and one blob update/removal act (after the fix).
>
>
> >> 2) Whitelist only the APIC.suffix since that's the table that will change with the fix
> >> 3) Add the fix (which changes the APIC table so a new APIC.suffix blob is needed and also stops generating the IORT table, so no more IORT.suffix blob is necessary)
> >> 4) Finally, update only the APIC.suffix blob and remove the IORT.suffix blob and wipe out the whitelist
> >>
> >> This way:
> >>
> >> A) It's clear that only ACPI blob changed with the fix, because there is no addition of a FACP.suffix blob in 4) (it remains the same)
> >> B) It's clear that the IORT table is removed with the fix and is not relevant anymore for the test
> >
> > I'd just mention it in commit log so that later no one would wonder why we are adding and then removing tables
> >
> > As for the rest of suggestions, it looks fine to me.
>
> Well, 2) won't make sense anymore since APIC.suffix would be already in the
> whitelist in the previous patch that added the empty blobs. Since there won't
> be a commit that adds _only_ the APIC.suffix to the whitelist, in preparation
> for the fix, this info is "lost" in the series, even tho it's possible to
> mention in the commit message.
>
> Hence, what I think is not ideal from a maintainer's/reviewer's perspective,
> is that in one commit all the blobs are updated/removed at once, which is
> confusing because the fix did not touch the FACP table (for instance) and
> this table is updated with APIC and with the removal of IORT, altogether,
> in the last commit.
>
> So, for this series, which adds new blobs and _also_ updates and removes some
> of them, how about the following organization:
>
> - 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
>
> - 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 bynanry so it can be folded into 4 or be a separate patch (either way works for me)
> - Patch (n+1) : Update the APIC.suffix blob, remove IORT.suffix blob, and remove the APIC.suffix blob from the whitelist
> * Add the APIC diff to the commit log
> * Mention in the commit log that IORT.suffix is removed because IORT table is no long generated after the fix
>
> This way: a) no commit fails the test and b) blobs are added/updated/removed in separate commits
>
> What do you think?
LGTM
>
> Cheers,
> Gustavo
>
^ permalink raw reply [flat|nested] 30+ messages in thread
end of thread, other threads:[~2025-05-06 15:37 UTC | newest]
Thread overview: 30+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-04-03 20:40 [PATCH-for-10.1 v3 0/9] hw/arm: GIC ITS=off ACPI tables fixes Philippe Mathieu-Daudé
2025-04-03 20:40 ` [PATCH-for-10.1 v3 1/9] hw/arm/virt: Remove pointless VirtMachineState::tcg_its field Philippe Mathieu-Daudé
2025-04-07 11:57 ` Eric Auger
2025-04-03 20:40 ` [PATCH-for-10.1 v3 2/9] hw/intc/gicv3_its: Do not check its_class_name() for NULL Philippe Mathieu-Daudé
2025-04-07 11:57 ` Eric Auger
2025-04-03 20:40 ` [PATCH-for-10.1 v3 3/9] hw/arm/virt: Simplify create_its() Philippe Mathieu-Daudé
2025-04-04 2:59 ` Gustavo Romero
2025-04-07 12:05 ` Eric Auger
2025-04-03 20:40 ` [PATCH-for-10.1 v3 4/9] hw/arm/virt-acpi: Factor its_enabled() helper out Philippe Mathieu-Daudé
2025-04-07 12:08 ` Eric Auger
2025-04-03 20:40 ` [PATCH-for-10.1 v3 5/9] qtest/bios-tables-test: Add test for -M virt, its=off Philippe Mathieu-Daudé
2025-04-04 3:00 ` [PATCH-for-10.1 v3 5/9] qtest/bios-tables-test: Add test for -M virt,its=off Gustavo Romero
2025-04-07 12:55 ` Eric Auger
2025-04-03 20:40 ` [PATCH-for-10.1 v3 6/9] qtest/bios-tables-test: Whitelist aarch64/virt 'its_off' variant blobs Philippe Mathieu-Daudé
2025-04-04 3:01 ` Gustavo Romero
2025-04-09 14:05 ` Igor Mammedov
2025-04-09 15:49 ` Gustavo Romero
2025-04-10 6:50 ` Igor Mammedov
2025-04-10 16:22 ` Gustavo Romero
2025-04-17 21:06 ` Gustavo Romero
2025-04-17 21:21 ` Michael S. Tsirkin
2025-05-06 15:36 ` Igor Mammedov
2025-04-03 20:40 ` [PATCH-for-10.1 v3 7/9] hw/arm/virt-acpi: Always build IORT table (even with GIC ITS disabled) Philippe Mathieu-Daudé
2025-04-04 3:02 ` Gustavo Romero
2025-04-07 13:30 ` Eric Auger
2025-04-03 20:40 ` [PATCH-for-10.1 v3 8/9] hw/arm/virt-acpi: Do not advertise disabled GIC ITS Philippe Mathieu-Daudé
2025-04-03 20:40 ` [PATCH-for-10.1 v3 9/9] qtest/bios-tables-test: Update aarch64/virt 'its_off' variant blobs Philippe Mathieu-Daudé
2025-04-04 3:04 ` Gustavo Romero
2025-04-15 8:19 ` [PATCH-for-10.1 v3 0/9] hw/arm: GIC ITS=off ACPI tables fixes Philippe Mathieu-Daudé
2025-04-17 21:08 ` 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).