On 2025/12/30 9:03, Mohamed Mediouni 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>
> Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> ---
> hw/arm/virt-acpi-build.c | 3 +-
> hw/arm/virt.c | 110 ++++++++++++++++++++++++++++++++-------
> include/hw/arm/virt.h | 4 +-
> 3 files changed, 94 insertions(+), 23 deletions(-)
>
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 86024a1a73..f3adb95cfe 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -962,8 +962,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 dcdb740586..1858190dd1 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,95 @@ 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;
> + default:
> + return false;
This "default" is better to be removed. Modern compilers can emit
warnings when a switch statement is not exhaustive. The default label is
not just redundant, but prevents compilers to emit such a warning.
> }
> - return true;
> }
>
> -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);
> - OnOffAuto its = vms->its;
> + const char *val;
>
> - visit_type_OnOffAuto(v, name, &its, errp);
> + 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;
> + default:
It is better to have "case VIRT_MSI_CTRL_AUTO" instead of "default" for
consistency with virt_is_its_enabled() and also to enable the compiler
warning I mentioned earlier.
> + val = "auto";
> + break;
> + }
> + return g_strdup(val);
> }
>
> -static void virt_set_its(Object *obj, Visitor *v, const char *name,
> - void *opaque, Error **errp)
> +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);
>
> - 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 +3125,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 +3517,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 +3585,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 */