On Tue, 27 Jan 2026 at 18:28, Mohamed Mediouni <[email protected]> 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 <[email protected]>
> ---
> hw/arm/virt-acpi-build.c | 20 ++++---
> hw/arm/virt.c | 112 ++++++++++++++++++++++++++++++++++++---
> include/hw/arm/virt.h | 5 +-
> 3 files changed, 121 insertions(+), 16 deletions(-)
>
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 03b4342574..f87bf1bf2f 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
You'll find the SMMUv3 acceleration changes produce some minor
conflicts in this file that you'll need to sort out.
The rest below here is minor stuff.
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 4badc1a734..3d7f02ce0e 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(vms->msi_controller == VIRT_MSI_CTRL_ITS);
I think we colud now simply drop this assert, given the simplicity
of the calling logic.
> 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 (vms->msi_controller == VIRT_MSI_CTRL_ITS) {
> create_its(vms);
> - } else if (vms->gic_version == VIRT_GIC_VERSION_2) {
> + } else if (vms->msi_controller == VIRT_MSI_CTRL_GICV2M) {
> 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;
> + }
This needs commentary to explain why it is doing what it's
doing.
> + }
> + 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. */
"QEMU" is all-caps.
Also, do you mean "older QEMU releases accepted the command line,
and we now error"? Generally we shouldn't do that. If you mean
"the old legacy its= option permits this, but the new msi= one
diagnoses it as an error", the comment could be clearer.
> + error_report("GICv2 + ITS is an invalid configuration.");
> + exit(1);
> + }
> + }
> +
> + assert(vms->msi_controller != VIRT_MSI_CTRL_AUTO);
> +}
> +
> +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")) {
Everywhere else seems to use "off" for the fourth value of
this option (so auto, its, gicv2m, off), but here we have "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");
> + }
> +}
> + 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");
We use '/' for 'on/off' but list all the values like
"Valid values are 2, 3, 4, host and max" for a set of strings.
thanks
-- PMM
