> On 15. Jan 2026, at 00:08, Mohamed Mediouni <[email protected]> wrote: > > > >> On 14. Jan 2026, at 21:51, Bernhard Beschow <[email protected]> wrote: >> >> >> >> Am 14. Januar 2026 13:41:24 UTC schrieb Mohamed Mediouni >> <[email protected] <mailto:[email protected]>>: >>> Introduce a -M msi= argument to be able to control MSI-X support >>> independently >>> from ITS, as part of supporting GICv3 + GICv2m platforms. >>> >>> Remove vms->its as it's no longer needed after that change. >>> >>> Signed-off-by: Mohamed Mediouni <[email protected]> >>> --- >>> hw/arm/virt-acpi-build.c | 24 ++++++--- >>> hw/arm/virt.c | 108 +++++++++++++++++++++++++++++++-------- >>> include/hw/arm/virt.h | 4 +- >>> 3 files changed, 108 insertions(+), 28 deletions(-) >>> >>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c >>> index 86024a1a73..187dd4e272 100644 >>> --- a/hw/arm/virt-acpi-build.c >>> +++ b/hw/arm/virt-acpi-build.c >>> @@ -440,6 +440,17 @@ static void create_rc_its_idmaps(GArray *its_idmaps, >>> GArray *smmuv3_devs) >>> } >>> } >>> >>> +/* >>> + * In the prior Qemu ACPI table handling, GICv2 configurations >>> + * had vms->its=1... That's broken. >>> + * >>> + * Match that assumption to match the existing ACPI tables that >>> + * have been shipping for quite a while. >>> + */ >>> +static int is_gicv2_acpi_workaround_needed(VirtMachineState *vms) { >>> + return vms->gic_version == 2; >>> +} >>> + >>> /* >>> * Input Output Remapping Table (IORT) >>> * Conforms to "IO Remapping Table System Software on ARM Platforms", >>> @@ -473,7 +484,7 @@ build_iort(GArray *table_data, BIOSLinker *linker, >>> VirtMachineState *vms) >>> nb_nodes = num_smmus + 1; /* RC and SMMUv3 */ >>> rc_mapping_count = rc_smmu_idmaps_len; >>> >>> - if (virt_is_its_enabled(vms)) { >>> + if (virt_is_its_enabled(vms) || >>> is_gicv2_acpi_workaround_needed(vms)) { >>> /* >>> * Knowing the ID ranges from the RC to the SMMU, it's possible >>> to >>> * determine the ID ranges from RC that go directly to ITS. >>> @@ -484,7 +495,7 @@ build_iort(GArray *table_data, BIOSLinker *linker, >>> VirtMachineState *vms) >>> rc_mapping_count += rc_its_idmaps->len; >>> } >>> } else { >>> - if (virt_is_its_enabled(vms)) { >>> + if (virt_is_its_enabled(vms) || >>> is_gicv2_acpi_workaround_needed(vms)) { >>> nb_nodes = 2; /* RC and ITS */ >>> rc_mapping_count = 1; /* Direct map to ITS */ >>> } else { >>> @@ -499,7 +510,7 @@ 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 */ >>> >>> - if (virt_is_its_enabled(vms)) { >>> + if (virt_is_its_enabled(vms) || is_gicv2_acpi_workaround_needed(vms)) { >>> /* 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 >>> */; >>> @@ -518,7 +529,7 @@ build_iort(GArray *table_data, BIOSLinker *linker, >>> VirtMachineState *vms) >>> int smmu_mapping_count, offset_to_id_array; >>> int irq = sdev->irq; >>> >>> - if (virt_is_its_enabled(vms)) { >>> + if (virt_is_its_enabled(vms) || >>> is_gicv2_acpi_workaround_needed(vms)) { >>> smmu_mapping_count = 1; /* ITS Group node */ >>> offset_to_id_array = SMMU_V3_ENTRY_SIZE; /* Just after the >>> header */ >>> } else { >>> @@ -611,7 +622,7 @@ build_iort(GArray *table_data, BIOSLinker *linker, >>> VirtMachineState *vms) >>> } >>> } >>> >>> - if (virt_is_its_enabled(vms)) { >>> + if (virt_is_its_enabled(vms) || >>> is_gicv2_acpi_workaround_needed(vms)) { >>> /* >>> * Map bypassed (don't go through the SMMU) RIDs (input) to >>> * ITS Group node directly: RC -> ITS. >>> @@ -962,8 +973,7 @@ build_madt(GArray *table_data, BIOSLinker *linker, >>> VirtMachineState *vms) >>> } >>> } >>> >>> - if (!(vms->gic_version != VIRT_GIC_VERSION_2 && >>> virt_is_its_enabled(vms)) >>> - && !vms->no_gicv3_with_gicv2m) { >>> + if (virt_is_gicv2m_enabled(vms)) { >>> const uint16_t spi_base = vms->irqmap[VIRT_GIC_V2M] + ARM_SPI_BASE; >>> >>> /* 5.2.12.16 GIC MSI Frame Structure */ >>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c >>> index 553c7f62cc..0e84ccd82c 100644 >>> --- a/hw/arm/virt.c >>> +++ b/hw/arm/virt.c >>> @@ -966,12 +966,12 @@ static void create_gic(VirtMachineState *vms, >>> MemoryRegion *mem) >>> >>> fdt_add_gic_node(vms); >>> >>> - if (vms->gic_version != VIRT_GIC_VERSION_2 && >>> virt_is_its_enabled(vms)) { >>> + if (virt_is_its_enabled(vms)) { >>> create_its(vms); >>> - } else if (vms->gic_version != VIRT_GIC_VERSION_2 && >>> !vms->no_gicv3_with_gicv2m) { >>> - create_v2m(vms); >>> - } else if (vms->gic_version == VIRT_GIC_VERSION_2) { >>> + } else if (virt_is_gicv2m_enabled(vms)) { >>> create_v2m(vms); >>> + } else { >>> + vms->msi_controller = VIRT_MSI_CTRL_NONE; >>> } >>> } >>> >>> @@ -2710,32 +2710,93 @@ static void virt_set_highmem_mmio_size(Object *obj, >>> Visitor *v, >>> >>> bool virt_is_its_enabled(VirtMachineState *vms) >>> { >>> - if (vms->its == ON_OFF_AUTO_OFF) { >>> + switch (vms->msi_controller) { >>> + case VIRT_MSI_CTRL_NONE: >>> return false; >>> - } >>> - if (vms->its == ON_OFF_AUTO_AUTO) { >>> - if (whpx_enabled()) { >>> + case VIRT_MSI_CTRL_ITS: >>> + return true; >>> + case VIRT_MSI_CTRL_GICV2M: >>> + return false; >>> + case VIRT_MSI_CTRL_AUTO: >>> + if (whpx_enabled() && whpx_irqchip_in_kernel()) { >>> + return false; >>> + } >>> + if (vms->gic_version == VIRT_GIC_VERSION_2) { >>> return false; >>> } >>> + return true; >>> } >>> - return true; >> >> With the last return statement removed, I get a spurious warning from >> MSYS2/x86_64 GCC 15.2: "control reaches end of non-void function". >> >>> } >>> >>> -static void virt_get_its(Object *obj, Visitor *v, const char *name, >>> - void *opaque, Error **errp) >>> +bool virt_is_gicv2m_enabled(VirtMachineState *vms) >>> +{ >>> + switch (vms->msi_controller) { >>> + case VIRT_MSI_CTRL_NONE: >>> + return false; >>> + default: >>> + return !virt_is_its_enabled(vms); >>> + } >>> +} >>> + >>> +static char *virt_get_msi(Object *obj, Error **errp) >>> +{ >>> + VirtMachineState *vms = VIRT_MACHINE(obj); >>> + const char *val; >>> + >>> + switch (vms->msi_controller) { >>> + case VIRT_MSI_CTRL_NONE: >>> + val = "off"; >>> + break; >>> + case VIRT_MSI_CTRL_ITS: >>> + val = "its"; >>> + break; >>> + case VIRT_MSI_CTRL_GICV2M: >>> + val = "gicv2m"; >>> + break; >>> + case VIRT_MSI_CTRL_AUTO: >>> + val = "auto"; >>> + break; >>> + } >> >> Similar to the above I get a spurious warning: "'val' may be used >> uninitialized in this function". >> >> Apparently the compiler doesn't notice that both switch statements handle >> all enums. I wonder why it can figure that out in other cases… >> > Hello, > > I didn’t see this on Clang but I do see this on GCC. Will see what I can do… > > Apparently it’s > https://stackoverflow.com/questions/33607284/control-reaches-end-of-non-void-function-with-fully-handled-case-switch-over-a > and the best way to go seems to be to add an unreachable assert >
There seems to be a long-standing GCC bug filed for this: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87950 >> Other than that the series works for me! >> >> Best regards, >> Bernhard >> >>> + return g_strdup(val); >>> +} >>> + >>> +static void virt_set_msi(Object *obj, const char *value, Error **errp) >>> { >>> + ERRP_GUARD(); >>> VirtMachineState *vms = VIRT_MACHINE(obj); >>> - OnOffAuto its = vms->its; >>> >>> - visit_type_OnOffAuto(v, name, &its, errp); >>> + if (!strcmp(value, "auto")) { >>> + vms->msi_controller = VIRT_MSI_CTRL_AUTO; /* Will be overriden >>> later */ >>> + } else if (!strcmp(value, "its")) { >>> + vms->msi_controller = VIRT_MSI_CTRL_ITS; >>> + } else if (!strcmp(value, "gicv2m")) { >>> + vms->msi_controller = VIRT_MSI_CTRL_GICV2M; >>> + } else if (!strcmp(value, "none")) { >>> + vms->msi_controller = VIRT_MSI_CTRL_NONE; >>> + } else { >>> + error_setg(errp, "Invalid msi value"); >>> + error_append_hint(errp, "Valid values are auto, gicv2m, its, >>> off\n"); >>> + } >>> } >>> >>> -static void virt_set_its(Object *obj, Visitor *v, const char *name, >>> - void *opaque, Error **errp) >>> +static bool virt_get_its(Object *obj, Error **errp) >>> { >>> VirtMachineState *vms = VIRT_MACHINE(obj); >>> >>> - visit_type_OnOffAuto(v, name, &vms->its, errp); >>> + return virt_is_its_enabled(vms); >>> +} >>> + >>> +static void virt_set_its(Object *obj, bool value, Error **errp) >>> +{ >>> + VirtMachineState *vms = VIRT_MACHINE(obj); >>> + >>> + if (value) { >>> + vms->msi_controller = VIRT_MSI_CTRL_ITS; >>> + } else if (vms->no_gicv3_with_gicv2m) { >>> + vms->msi_controller = VIRT_MSI_CTRL_NONE; >>> + } else { >>> + vms->msi_controller = VIRT_MSI_CTRL_GICV2M; >>> + } >>> } >>> >>> static bool virt_get_dtb_randomness(Object *obj, Error **errp) >>> @@ -3062,6 +3123,8 @@ static void >>> virt_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev, >>> db_start = base_memmap[VIRT_GIC_V2M].base; >>> db_end = db_start + base_memmap[VIRT_GIC_V2M].size - 1; >>> break; >>> + case VIRT_MSI_CTRL_AUTO: >>> + g_assert_not_reached(); >>> } >>> resv_prop_str = g_strdup_printf("0x%"PRIx64":0x%"PRIx64":%u", >>> db_start, db_end, >>> @@ -3452,13 +3515,18 @@ static void virt_machine_class_init(ObjectClass >>> *oc, const void *data) >>> "guest CPU which implements the >>> ARM " >>> "Memory Tagging Extension"); >>> >>> - object_class_property_add(oc, "its", "OnOffAuto", >>> - virt_get_its, virt_set_its, >>> - NULL, NULL); >>> + object_class_property_add_bool(oc, "its", virt_get_its, >>> + virt_set_its); >>> object_class_property_set_description(oc, "its", >>> "Set on/off to enable/disable " >>> "ITS instantiation"); >>> >>> + object_class_property_add_str(oc, "msi", virt_get_msi, >>> + virt_set_msi); >>> + object_class_property_set_description(oc, "msi", >>> + "Set MSI settings. " >>> + "Valid values are >>> auto/gicv2m/its/off"); >>> + >>> object_class_property_add_bool(oc, "dtb-randomness", >>> virt_get_dtb_randomness, >>> virt_set_dtb_randomness); >>> @@ -3515,7 +3583,7 @@ static void virt_instance_init(Object *obj) >>> vms->highmem_redists = true; >>> >>> /* Default allows ITS instantiation if available */ >>> - vms->its = ON_OFF_AUTO_AUTO; >>> + vms->msi_controller = VIRT_MSI_CTRL_AUTO; >>> /* Allow ITS emulation if the machine version supports it */ >>> vms->tcg_its = !vmc->no_tcg_its; >>> vms->no_gicv3_with_gicv2m = false; >>> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h >>> index 394b70c62e..ff43bcb739 100644 >>> --- a/include/hw/arm/virt.h >>> +++ b/include/hw/arm/virt.h >>> @@ -101,6 +101,8 @@ typedef enum VirtIOMMUType { >>> >>> typedef enum VirtMSIControllerType { >>> VIRT_MSI_CTRL_NONE, >>> + /* This value is overriden at runtime.*/ >>> + VIRT_MSI_CTRL_AUTO, >>> VIRT_MSI_CTRL_GICV2M, >>> VIRT_MSI_CTRL_ITS, >>> } VirtMSIControllerType; >>> @@ -147,7 +149,6 @@ struct VirtMachineState { >>> bool highmem_ecam; >>> bool highmem_mmio; >>> bool highmem_redists; >>> - OnOffAuto its; >>> bool tcg_its; >>> bool virt; >>> bool ras; >>> @@ -217,5 +218,6 @@ static inline int >>> virt_gicv3_redist_region_count(VirtMachineState *vms) >>> } >>> >>> bool virt_is_its_enabled(VirtMachineState *vms); >>> +bool virt_is_gicv2m_enabled(VirtMachineState *vms); >>> >>> #endif /* QEMU_ARM_VIRT_H */
