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 <mohamed@unpredictable.fr>
---
hw/arm/virt-acpi-build.c | 27 ++++++--
hw/arm/virt.c | 137 +++++++++++++++++++++++++++++++++++++--
include/hw/arm/virt.h | 8 ++-
3 files changed, 157 insertions(+), 15 deletions(-)
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 03b4342574..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 (vms->its) {
+ 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 (vms->its) {
+ 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 (vms->its) {
+ 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 (vms->its) {
+ 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 (vms->its) {
+ 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.
@@ -946,7 +957,7 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
memmap[VIRT_HIGH_GIC_REDIST2].size);
}
- if (vms->its) {
+ if (virt_is_its_enabled(vms)) {
/*
* ACPI spec, Revision 6.0 Errata A
* (original 6.0 definition has invalid Length)
@@ -960,7 +971,9 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
build_append_int_noprefix(table_data, memmap[VIRT_GIC_ITS].base, 8);
build_append_int_noprefix(table_data, 0, 4); /* Reserved */
}
- } else {
+ }
+
+ 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 4badc1a734..3f62247f94 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -737,7 +737,7 @@ static void create_its(VirtMachineState *vms)
{
DeviceState *dev;
- assert(vms->its);
+ assert(virt_is_its_enabled(vms));
if (!kvm_irqchip_in_kernel() && !vms->tcg_its) {
/*
* Do nothing if ITS is neither supported by the host nor emulated by
@@ -957,9 +957,9 @@ static void create_gic(VirtMachineState *vms, MemoryRegion *mem)
fdt_add_gic_node(vms);
- if (vms->gic_version != VIRT_GIC_VERSION_2 && vms->its) {
+ if (virt_is_its_enabled(vms)) {
create_its(vms);
- } else if (vms->gic_version == VIRT_GIC_VERSION_2) {
+ } else if (virt_is_gicv2m_enabled(vms)) {
create_v2m(vms);
}
}
@@ -2132,6 +2132,36 @@ static void finalize_gic_version(VirtMachineState *vms)
gics_supported, max_cpus);
}
+static void finalize_msi_controller(VirtMachineState *vms)
+{
+ if (vms->msi_controller == VIRT_MSI_LEGACY_OPT_ITS_OFF) {
+ if (vms->gic_version == 2) {
+ vms->msi_controller = VIRT_MSI_CTRL_GICV2M;
+ }
+ else {
+ vms->msi_controller = VIRT_MSI_CTRL_NONE;
+ }
+ }
+ if (vms->msi_controller == VIRT_MSI_CTRL_AUTO) {
+ if (vms->gic_version == VIRT_GIC_VERSION_2) {
+ vms->msi_controller = VIRT_MSI_CTRL_GICV2M;
+ }
+ else {
+ vms->msi_controller = VIRT_MSI_CTRL_ITS;
+ }
+ }
+
+ if (vms->msi_controller == VIRT_MSI_CTRL_ITS) {
+ if (vms->gic_version == VIRT_GIC_VERSION_2) {
+ /* Older Qemu releases accepted this, but better to error. */
+ error_report("GICv2 + ITS is an invalid configuration.");
+ exit(1);
+ }
+ }
+
+ assert(vms->msi_controller != VIRT_MSI_CTRL_AUTO);
+}
+
/*
* virt_post_cpus_gic_realized() must be called after the CPUs and
* the GIC have both been realized.
@@ -2251,6 +2281,7 @@ static void machvirt_init(MachineState *machine)
* KVM is not available yet
*/
finalize_gic_version(vms);
+ finalize_msi_controller(vms);
if (vms->secure) {
/*
@@ -2700,18 +2731,101 @@ static void virt_set_highmem_mmio_size(Object *obj, Visitor *v,
extended_memmap[VIRT_HIGH_PCIE_MMIO].size = size;
}
+bool virt_is_its_enabled(VirtMachineState *vms)
+{
+ switch (vms->msi_controller) {
+ case VIRT_MSI_CTRL_NONE:
+ return false;
+ case VIRT_MSI_CTRL_ITS:
+ return true;
+ case VIRT_MSI_CTRL_GICV2M:
+ return false;
+ case VIRT_MSI_CTRL_AUTO:
+ /*
+ * Earlier Qemu considered its=on as the default when using the GICv2.
+ * It is safe to do this because this is called is prior to
+ * finalize_msi_controller.
+ */
+ return true;
+ default:
+ g_assert_not_reached();
+ }
+}
+
+bool virt_is_gicv2m_enabled(VirtMachineState *vms)
+{
+ switch (vms->msi_controller) {
+ case VIRT_MSI_CTRL_NONE:
+ return false;
+ case VIRT_MSI_CTRL_ITS:
+ case VIRT_MSI_CTRL_GICV2M:
+ case VIRT_MSI_CTRL_AUTO:
+ return !virt_is_its_enabled(vms);
+ default:
+ g_assert_not_reached();
+ }
+}
+
+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:
+ case VIRT_MSI_LEGACY_OPT_ITS_OFF:
+ 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;
+ default:
+ g_assert_not_reached();
+ }
+ return g_strdup(val);
+}
+
+static void virt_set_msi(Object *obj, const char *value, Error **errp)
+{
+ ERRP_GUARD();
+ VirtMachineState *vms = VIRT_MACHINE(obj);
+
+ 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 bool virt_get_its(Object *obj, Error **errp)
{
VirtMachineState *vms = VIRT_MACHINE(obj);
- return vms->its;
+ return virt_is_its_enabled(vms);
}
static void virt_set_its(Object *obj, bool value, Error **errp)
{
VirtMachineState *vms = VIRT_MACHINE(obj);
- vms->its = value;
+ if (value) {
+ vms->msi_controller = VIRT_MSI_CTRL_ITS;
+ } else {
+ vms->msi_controller = VIRT_MSI_LEGACY_OPT_ITS_OFF;
+ }
}
static bool virt_get_dtb_randomness(Object *obj, Error **errp)
@@ -3038,6 +3152,9 @@ 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:
+ case VIRT_MSI_LEGACY_OPT_ITS_OFF:
+ g_assert_not_reached();
}
resv_prop_str = g_strdup_printf("0x%"PRIx64":0x%"PRIx64":%u",
db_start, db_end,
@@ -3438,6 +3555,12 @@ static void virt_machine_class_init(ObjectClass *oc, const void *data)
"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);
@@ -3493,8 +3616,8 @@ static void virt_instance_init(Object *obj)
vms->highmem_mmio = true;
vms->highmem_redists = true;
- /* Default allows ITS instantiation */
- vms->its = true;
+ /* Default allows ITS instantiation if available */
+ vms->msi_controller = VIRT_MSI_CTRL_AUTO;
/* Allow ITS emulation if the machine version supports it */
vms->tcg_its = !vmc->no_tcg_its;
diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
index 5907d41dbb..6ca849e3fd 100644
--- a/include/hw/arm/virt.h
+++ b/include/hw/arm/virt.h
@@ -101,6 +101,10 @@ typedef enum VirtIOMMUType {
typedef enum VirtMSIControllerType {
VIRT_MSI_CTRL_NONE,
+ /* This value is overriden at runtime.*/
+ VIRT_MSI_CTRL_AUTO,
+ /* Legacy option: its=off provides a GICv2m when using GICv2 */
+ VIRT_MSI_LEGACY_OPT_ITS_OFF,
VIRT_MSI_CTRL_GICV2M,
VIRT_MSI_CTRL_ITS,
} VirtMSIControllerType;
@@ -146,7 +150,6 @@ struct VirtMachineState {
bool highmem_ecam;
bool highmem_mmio;
bool highmem_redists;
- bool its;
bool tcg_its;
bool virt;
bool ras;
@@ -216,4 +219,7 @@ static inline int virt_gicv3_redist_region_count(VirtMachineState *vms)
vms->highmem_redists) ? 2 : 1;
}
+bool virt_is_its_enabled(VirtMachineState *vms);
+bool virt_is_gicv2m_enabled(VirtMachineState *vms);
+
#endif /* QEMU_ARM_VIRT_H */
--
2.50.1 (Apple Git-155)On Wed, 21 Jan 2026 at 13:41, Mohamed Mediouni <mohamed@unpredictable.fr> wrote:
>
> 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 <mohamed@unpredictable.fr>
> ---
> hw/arm/virt-acpi-build.c | 27 ++++++--
> hw/arm/virt.c | 137 +++++++++++++++++++++++++++++++++++++--
> include/hw/arm/virt.h | 8 ++-
> 3 files changed, 157 insertions(+), 15 deletions(-)
>
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 03b4342574..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;
> +}
We don't need to keep identical bug-for-bug ACPI tables like that.
If we were incorrectly reporting an ITS in a GICv2-only ACPI table,
that was a bug and we can fix it. (This might need adjusting of the
golden reference ACPI data in some of the bios-tables-tests if we
were testing that, so it ought to go in its own patch.)
> +bool virt_is_its_enabled(VirtMachineState *vms)
> +{
> + switch (vms->msi_controller) {
> + case VIRT_MSI_CTRL_NONE:
> + return false;
> + case VIRT_MSI_CTRL_ITS:
> + return true;
> + case VIRT_MSI_CTRL_GICV2M:
> + return false;
> + case VIRT_MSI_CTRL_AUTO:
> + /*
> + * Earlier Qemu considered its=on as the default when using the GICv2.
> + * It is safe to do this because this is called is prior to
> + * finalize_msi_controller.
> + */
> + return true;
> + default:
> + g_assert_not_reached();
> + }
> +}
> +
> +bool virt_is_gicv2m_enabled(VirtMachineState *vms)
> +{
> + switch (vms->msi_controller) {
> + case VIRT_MSI_CTRL_NONE:
> + return false;
> + case VIRT_MSI_CTRL_ITS:
> + case VIRT_MSI_CTRL_GICV2M:
> + case VIRT_MSI_CTRL_AUTO:
> + return !virt_is_its_enabled(vms);
> + default:
> + g_assert_not_reached();
> + }
> +}
Why do we need these? The idea of finalize_msi_controller()
is that it fixes vms->msi_controller so that you can just
directly say vms->msi_controller == VIRT_MSI_CTRL_ITS or
vms->msi_controller == VIRT_MSI_CTRL_GICV2M rather than
having to have more complicated logic.
thanks
-- PMM
> On 27. Jan 2026, at 11:54, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Wed, 21 Jan 2026 at 13:41, Mohamed Mediouni <mohamed@unpredictable.fr> wrote:
>>
>> 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 <mohamed@unpredictable.fr>
>> ---
>> hw/arm/virt-acpi-build.c | 27 ++++++--
>> hw/arm/virt.c | 137 +++++++++++++++++++++++++++++++++++++--
>> include/hw/arm/virt.h | 8 ++-
>> 3 files changed, 157 insertions(+), 15 deletions(-)
>>
>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>> index 03b4342574..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;
>> +}
>
> We don't need to keep identical bug-for-bug ACPI tables like that.
> If we were incorrectly reporting an ITS in a GICv2-only ACPI table,
> that was a bug and we can fix it. (This might need adjusting of the
> golden reference ACPI data in some of the bios-tables-tests if we
> were testing that, so it ought to go in its own patch.)
>
Hello,
I’m a bit concerned about breaking hibernation in this case…
My intent was keeping this behavior for now and then add a machine model version dependent toggle in a follow-up patch.
>> +bool virt_is_its_enabled(VirtMachineState *vms)
>> +{
>> + switch (vms->msi_controller) {
>> + case VIRT_MSI_CTRL_NONE:
>> + return false;
>> + case VIRT_MSI_CTRL_ITS:
>> + return true;
>> + case VIRT_MSI_CTRL_GICV2M:
>> + return false;
>> + case VIRT_MSI_CTRL_AUTO:
>> + /*
>> + * Earlier Qemu considered its=on as the default when using the GICv2.
>> + * It is safe to do this because this is called is prior to
>> + * finalize_msi_controller.
>> + */
>> + return true;
>> + default:
>> + g_assert_not_reached();
>> + }
>> +}
>> +
>> +bool virt_is_gicv2m_enabled(VirtMachineState *vms)
>> +{
>> + switch (vms->msi_controller) {
>> + case VIRT_MSI_CTRL_NONE:
>> + return false;
>> + case VIRT_MSI_CTRL_ITS:
>> + case VIRT_MSI_CTRL_GICV2M:
>> + case VIRT_MSI_CTRL_AUTO:
>> + return !virt_is_its_enabled(vms);
>> + default:
>> + g_assert_not_reached();
>> + }
>> +}
>
> Why do we need these? The idea of finalize_msi_controller()
> is that it fixes vms->msi_controller so that you can just
> directly say vms->msi_controller == VIRT_MSI_CTRL_ITS or
> vms->msi_controller == VIRT_MSI_CTRL_GICV2M rather than
> having to have more complicated logic.
>
It’s because of object_class_property_add_str(oc, "msi", virt_get_msi, virt_set_msi) (and the same applies for the its property), is it safe to assume that virt_get_msi/virt_get_its will never be called from that prior to finalize?
> thanks
> -- PMM
>
On Tue, 27 Jan 2026 at 12:46, Mohamed Mediouni <mohamed@unpredictable.fr> wrote:
> > On 27. Jan 2026, at 11:54, Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> > On Wed, 21 Jan 2026 at 13:41, Mohamed Mediouni <mohamed@unpredictable.fr> wrote:
> >> +/*
> >> + * 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;
> >> +}
> >
> > We don't need to keep identical bug-for-bug ACPI tables like that.
> > If we were incorrectly reporting an ITS in a GICv2-only ACPI table,
> > that was a bug and we can fix it. (This might need adjusting of the
> > golden reference ACPI data in some of the bios-tables-tests if we
> > were testing that, so it ought to go in its own patch.)
> >
> Hello,
>
> I’m a bit concerned about breaking hibernation in this case…
>
> My intent was keeping this behavior for now and then add a machine model version dependent toggle in a follow-up patch.
I'm not an ACPI table expert but my understanding is that it's
OK to change the ACPI table contents without having to make
those changes machine-version dependent. See e.g. commit d6afe18b7242,
which changed the ACPI tables for the its=off case and did not
make those changes machine-version specific.
> >> +bool virt_is_its_enabled(VirtMachineState *vms)
> > Why do we need these? The idea of finalize_msi_controller()
> > is that it fixes vms->msi_controller so that you can just
> > directly say vms->msi_controller == VIRT_MSI_CTRL_ITS or
> > vms->msi_controller == VIRT_MSI_CTRL_GICV2M rather than
> > having to have more complicated logic.
> >
> It’s because of object_class_property_add_str(oc, "msi", virt_get_msi, virt_set_msi) (and the same applies for the its property), is it safe to assume that virt_get_msi/virt_get_its will never be called from that prior to finalize?
The get and set property functions need to handle the
msi_controller field not having been finalized, but
they are only getting and setting it.
thanks
-- PMM
> On 27. Jan 2026, at 13:54, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Tue, 27 Jan 2026 at 12:46, Mohamed Mediouni <mohamed@unpredictable.fr> wrote:
>>> On 27. Jan 2026, at 11:54, Peter Maydell <peter.maydell@linaro.org> wrote:
>>>
>>> On Wed, 21 Jan 2026 at 13:41, Mohamed Mediouni <mohamed@unpredictable.fr> wrote:
>>>> +/*
>>>> + * 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;
>>>> +}
>>>
>>> We don't need to keep identical bug-for-bug ACPI tables like that.
>>> If we were incorrectly reporting an ITS in a GICv2-only ACPI table,
>>> that was a bug and we can fix it. (This might need adjusting of the
>>> golden reference ACPI data in some of the bios-tables-tests if we
>>> were testing that, so it ought to go in its own patch.)
>>>
>> Hello,
>>
>> I’m a bit concerned about breaking hibernation in this case…
>>
>> My intent was keeping this behavior for now and then add a machine model version dependent toggle in a follow-up patch.
>
> I'm not an ACPI table expert but my understanding is that it's
> OK to change the ACPI table contents without having to make
> those changes machine-version dependent. See e.g. commit d6afe18b7242,
> which changed the ACPI tables for the its=off case and did not
> make those changes machine-version specific.
>
Hi,
That commit didn’t affect the default/regular config so it wouldn't have been too problematic in practice.
Have had countless issues with how brittle hibernation is* - and more or less subtle ACPI table changes
tend to break it.
* and Qemu doesn’t provide an FACS (nevermind one with an HW signature) to force a clean reboot
- which Windows ignores anyway
>>>> +bool virt_is_its_enabled(VirtMachineState *vms)
>
>>> Why do we need these? The idea of finalize_msi_controller()
>>> is that it fixes vms->msi_controller so that you can just
>>> directly say vms->msi_controller == VIRT_MSI_CTRL_ITS or
>>> vms->msi_controller == VIRT_MSI_CTRL_GICV2M rather than
>>> having to have more complicated logic.
>>>
>> It’s because of object_class_property_add_str(oc, "msi", virt_get_msi, virt_set_msi) (and the same applies for the its property), is it safe to assume that virt_get_msi/virt_get_its will never be called from that prior to finalize?
>
> The get and set property functions need to handle the
> msi_controller field not having been finalized, but
> they are only getting and setting it.
>
> thanks
> -- PMM
>
On Tue, 27 Jan 2026 at 13:40, Mohamed Mediouni <mohamed@unpredictable.fr> wrote:
>
>
>
> > On 27. Jan 2026, at 13:54, Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> > On Tue, 27 Jan 2026 at 12:46, Mohamed Mediouni <mohamed@unpredictable.fr> wrote:
> >>> On 27. Jan 2026, at 11:54, Peter Maydell <peter.maydell@linaro.org> wrote:
> >>>
> >>> On Wed, 21 Jan 2026 at 13:41, Mohamed Mediouni <mohamed@unpredictable.fr> wrote:
> >>>> +/*
> >>>> + * 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;
> >>>> +}
> >>>
> >>> We don't need to keep identical bug-for-bug ACPI tables like that.
> >>> If we were incorrectly reporting an ITS in a GICv2-only ACPI table,
> >>> that was a bug and we can fix it. (This might need adjusting of the
> >>> golden reference ACPI data in some of the bios-tables-tests if we
> >>> were testing that, so it ought to go in its own patch.)
> >>>
> >> Hello,
> >>
> >> I’m a bit concerned about breaking hibernation in this case…
> >>
> >> My intent was keeping this behavior for now and then add a machine model version dependent toggle in a follow-up patch.
> >
> > I'm not an ACPI table expert but my understanding is that it's
> > OK to change the ACPI table contents without having to make
> > those changes machine-version dependent. See e.g. commit d6afe18b7242,
> > which changed the ACPI tables for the its=off case and did not
> > make those changes machine-version specific.
> That commit didn’t affect the default/regular config so it wouldn't have been too problematic in practice.
>
> Have had countless issues with how brittle hibernation is* - and more or less subtle ACPI table changes
> tend to break it.
I don't want to add back-compat handling for this to the virt
board unless one of our ACPI table experts says that yes we
do need to keep the ACPI table contents identical for older
versioned machine types.
thanks
-- PMM
On Tue, Jan 27, 2026 at 02:10:08PM +0000, Peter Maydell wrote:
> On Tue, 27 Jan 2026 at 13:40, Mohamed Mediouni <mohamed@unpredictable.fr> wrote:
> >
> >
> >
> > > On 27. Jan 2026, at 13:54, Peter Maydell <peter.maydell@linaro.org> wrote:
> > >
> > > On Tue, 27 Jan 2026 at 12:46, Mohamed Mediouni <mohamed@unpredictable.fr> wrote:
> > >>> On 27. Jan 2026, at 11:54, Peter Maydell <peter.maydell@linaro.org> wrote:
> > >>>
> > >>> On Wed, 21 Jan 2026 at 13:41, Mohamed Mediouni <mohamed@unpredictable.fr> wrote:
> > >>>> +/*
> > >>>> + * 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;
> > >>>> +}
> > >>>
> > >>> We don't need to keep identical bug-for-bug ACPI tables like that.
> > >>> If we were incorrectly reporting an ITS in a GICv2-only ACPI table,
> > >>> that was a bug and we can fix it. (This might need adjusting of the
> > >>> golden reference ACPI data in some of the bios-tables-tests if we
> > >>> were testing that, so it ought to go in its own patch.)
> > >>>
> > >> Hello,
> > >>
> > >> I’m a bit concerned about breaking hibernation in this case…
> > >>
> > >> My intent was keeping this behavior for now and then add a machine model version dependent toggle in a follow-up patch.
> > >
> > > I'm not an ACPI table expert but my understanding is that it's
> > > OK to change the ACPI table contents without having to make
> > > those changes machine-version dependent. See e.g. commit d6afe18b7242,
> > > which changed the ACPI tables for the its=off case and did not
> > > make those changes machine-version specific.
>
> > That commit didn’t affect the default/regular config so it wouldn't have been too problematic in practice.
> >
> > Have had countless issues with how brittle hibernation is* - and more or less subtle ACPI table changes
> > tend to break it.
>
> I don't want to add back-compat handling for this to the virt
> board unless one of our ACPI table experts says that yes we
> do need to keep the ACPI table contents identical for older
> versioned machine types.
>
> thanks
> -- PMM
Generally what we have handles VM migration, not hybernation. The issue
with hybernation is guests might cache some data to speed up init. If
you want to go overboard you would have to never change anything at all
in the firmware. Whether they do it in any specific instance is hard to
predict 100% but our approach generally is to try with a couple of
popular guests, and not to worry too much ahead of time otherwise. For
several reasons: one is most changes are harmless, another one there's
no special race here, if there's an issue you can trigger it
predictably, yet another one is hybernation is a rarely used feature.
For the specific change, I'd guess just test and if fine - fix it without a compat.
--
MST
> On 27. Jan 2026, at 16:34, Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Tue, Jan 27, 2026 at 02:10:08PM +0000, Peter Maydell wrote:
>> On Tue, 27 Jan 2026 at 13:40, Mohamed Mediouni <mohamed@unpredictable.fr> wrote:
>>>
>>>
>>>
>>>> On 27. Jan 2026, at 13:54, Peter Maydell <peter.maydell@linaro.org> wrote:
>>>>
>>>> On Tue, 27 Jan 2026 at 12:46, Mohamed Mediouni <mohamed@unpredictable.fr> wrote:
>>>>>> On 27. Jan 2026, at 11:54, Peter Maydell <peter.maydell@linaro.org> wrote:
>>>>>>
>>>>>> On Wed, 21 Jan 2026 at 13:41, Mohamed Mediouni <mohamed@unpredictable.fr> wrote:
>>>>>>> +/*
>>>>>>> + * 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;
>>>>>>> +}
>>>>>>
>>>>>> We don't need to keep identical bug-for-bug ACPI tables like that.
>>>>>> If we were incorrectly reporting an ITS in a GICv2-only ACPI table,
>>>>>> that was a bug and we can fix it. (This might need adjusting of the
>>>>>> golden reference ACPI data in some of the bios-tables-tests if we
>>>>>> were testing that, so it ought to go in its own patch.)
>>>>>>
>>>>> Hello,
>>>>>
>>>>> I’m a bit concerned about breaking hibernation in this case…
>>>>>
>>>>> My intent was keeping this behavior for now and then add a machine model version dependent toggle in a follow-up patch.
>>>>
>>>> I'm not an ACPI table expert but my understanding is that it's
>>>> OK to change the ACPI table contents without having to make
>>>> those changes machine-version dependent. See e.g. commit d6afe18b7242,
>>>> which changed the ACPI tables for the its=off case and did not
>>>> make those changes machine-version specific.
>>
>>> That commit didn’t affect the default/regular config so it wouldn't have been too problematic in practice.
>>>
>>> Have had countless issues with how brittle hibernation is* - and more or less subtle ACPI table changes
>>> tend to break it.
>>
>> I don't want to add back-compat handling for this to the virt
>> board unless one of our ACPI table experts says that yes we
>> do need to keep the ACPI table contents identical for older
>> versioned machine types.
>>
>> thanks
>> -- PMM
>
>
> Generally what we have handles VM migration, not hybernation. The issue
> with hybernation is guests might cache some data to speed up init. If
> you want to go overboard you would have to never change anything at all
> in the firmware. Whether they do it in any specific instance is hard to
> predict 100% but our approach generally is to try with a couple of
> popular guests, and not to worry too much ahead of time otherwise. For
> several reasons: one is most changes are harmless, another one there's
> no special race here, if there's an issue you can trigger it
> predictably, yet another one is hybernation is a rarely used feature.
>
> For the specific change, I'd guess just test and if fine - fix it without a compat.
>
Hi,
It turns out that even the Ubuntu stock kernel of all things has hibernation disabled on arm64.
Had to install the linux-aws variant to test it and that testing didn’t go too well without even making
any Qemu changes between suspend and resume.
So considering it as unimportant…
(And just sent a new rev with putting the new behaviour, had to tackle a bug on the way)
Thank you,
-Mohamed
> --
> MST
>
>
© 2016 - 2026 Red Hat, Inc.