Am 14. Januar 2026 23:12:40 UTC schrieb Mohamed Mediouni
<[email protected]>:
>
>
>> 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
Yeah, seems like a case for language lawyers...
>
>>> 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 */
>
