> 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 > 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 */
