[PATCH v6 03/23] hw/arm: virt: add GICv2m for the case when ITS is not available

Mohamed Mediouni posted 23 patches 4 months, 3 weeks ago
Maintainers: Cameron Esfahani <dirty@apple.com>, Roman Bolshakov <rbolshakov@ddn.com>, Phil Dennis-Jordan <phil@philjordan.eu>, Mads Ynddal <mads@ynddal.dk>, Pedro Barbuda <pbarbuda@microsoft.com>, Mohamed Mediouni <mohamed@unpredictable.fr>, Peter Maydell <peter.maydell@linaro.org>, Shannon Zhao <shannon.zhaosl@gmail.com>, "Michael S. Tsirkin" <mst@redhat.com>, Igor Mammedov <imammedo@redhat.com>, Ani Sinha <anisinha@redhat.com>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, Paolo Bonzini <pbonzini@redhat.com>, Richard Henderson <richard.henderson@linaro.org>, Eduardo Habkost <eduardo@habkost.net>, "Philippe Mathieu-Daudé" <philmd@linaro.org>, Yanan Wang <wangyanan55@huawei.com>, Zhao Liu <zhao1.liu@intel.com>, "Marc-André Lureau" <marcandre.lureau@redhat.com>, "Daniel P. Berrangé" <berrange@redhat.com>, Alexander Graf <agraf@csgraf.de>
There is a newer version of this series
[PATCH v6 03/23] hw/arm: virt: add GICv2m for the case when ITS is not available
Posted by Mohamed Mediouni 4 months, 3 weeks ago
On Hypervisor.framework for macOS and WHPX for Windows, the provided environment is a GICv3 without ITS.

As such, support a GICv3 w/ GICv2m for that scenario.

Signed-off-by: Mohamed Mediouni <mohamed@unpredictable.fr>

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
---
 hw/arm/virt-acpi-build.c | 4 +++-
 hw/arm/virt.c            | 8 ++++++++
 include/hw/arm/virt.h    | 2 ++
 3 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 96830f7c4e..7a049b8328 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -959,7 +959,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 (!vms->its && !vms->no_gicv3_with_gicv2m) {
         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 02209fadcf..01274ec804 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -956,6 +956,8 @@ static void create_gic(VirtMachineState *vms, MemoryRegion *mem)
 
     if (vms->gic_version != VIRT_GIC_VERSION_2 && vms->its) {
         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) {
         create_v2m(vms);
     }
@@ -2433,6 +2435,8 @@ static void machvirt_init(MachineState *machine)
     vms->ns_el2_virt_timer_irq = ns_el2_virt_timer_present() &&
         !vmc->no_ns_el2_virt_timer_irq;
 
+    vms->no_gicv3_with_gicv2m = vmc->no_gicv3_with_gicv2m;
+
     fdt_add_timer_nodes(vms);
     fdt_add_cpu_nodes(vms);
 
@@ -3467,6 +3471,7 @@ static void virt_instance_init(Object *obj)
     vms->its = true;
     /* Allow ITS emulation if the machine version supports it */
     vms->tcg_its = !vmc->no_tcg_its;
+    vms->no_gicv3_with_gicv2m = false;
 
     /* Default disallows iommu instantiation */
     vms->iommu = VIRT_IOMMU_NONE;
@@ -3519,8 +3524,11 @@ DEFINE_VIRT_MACHINE_AS_LATEST(10, 2)
 
 static void virt_machine_10_1_options(MachineClass *mc)
 {
+    VirtMachineClass *vmc = VIRT_MACHINE_CLASS(OBJECT_CLASS(mc));
+
     virt_machine_10_2_options(mc);
     compat_props_add(mc->compat_props, hw_compat_10_1, hw_compat_10_1_len);
+    vmc->no_gicv3_with_gicv2m = true;
 }
 DEFINE_VIRT_MACHINE(10, 1)
 
diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
index ea2cff05b0..3c030f4b5d 100644
--- a/include/hw/arm/virt.h
+++ b/include/hw/arm/virt.h
@@ -131,6 +131,7 @@ struct VirtMachineClass {
     bool no_cpu_topology;
     bool no_tcg_lpa2;
     bool no_ns_el2_virt_timer_irq;
+    bool no_gicv3_with_gicv2m;
     bool no_nested_smmu;
 };
 
@@ -178,6 +179,7 @@ struct VirtMachineState {
     char *oem_id;
     char *oem_table_id;
     bool ns_el2_virt_timer_irq;
+    bool no_gicv3_with_gicv2m;
     CXLState cxl_devices_state;
     bool legacy_smmuv3_present;
 };
-- 
2.50.1 (Apple Git-155)
Re: [PATCH v6 03/23] hw/arm: virt: add GICv2m for the case when ITS is not available
Posted by Peter Maydell 4 months, 2 weeks ago
On Sat, 20 Sept 2025 at 15:02, Mohamed Mediouni
<mohamed@unpredictable.fr> wrote:
>
> On Hypervisor.framework for macOS and WHPX for Windows, the provided environment is a GICv3 without ITS.
>
> As such, support a GICv3 w/ GICv2m for that scenario.
>
> Signed-off-by: Mohamed Mediouni <mohamed@unpredictable.fr>
>
> Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> ---
>  hw/arm/virt-acpi-build.c | 4 +++-
>  hw/arm/virt.c            | 8 ++++++++
>  include/hw/arm/virt.h    | 2 ++
>  3 files changed, 13 insertions(+), 1 deletion(-)

Looking at this I find myself wondering whether we need the
old-version back compat handling. The cases I think we have
at the moment are:

 (1) TCG, virt-6.1 and earlier: no_tcg_its is set
   -- you can have a gicv2 (always with a gicv2m)
   -- if you specify gic-version=3 you get a GICv3 without ITS
 (2) TCG, virt-6.2 and later:
   -- gic-version=2 still has gicv2m
   -- gic-version=3 by default gives you an ITS; if you also
      say its=off you get GICv3 with no ITS
   -- there is no case where we provide a GICv3 and are
      unable to provide an ITS for it
 (3) KVM (any version):
   -- gic-version=2 has a gicv2m
   -- gic-version=3 gives you an ITS by default; its=off
      will remove it
   -- there is no case where we provide a GICv3 and are
      unable to provide an ITS for it
 (4) HVF:
   -- only gic-version=2 works, you get a gicv2m

and I think what we want is:
 (a) if you explicitly disable the ITS (with its=off or via
     no_tcg_its) you get no ITS (and no gicv2m)
 (b) if you explicitly enable the ITS you should get an
     actual ITS or an error message
 (c) the default should be its=auto which gives
     you "ITS if we can, gicv2m if we can't".
     This is repurposing the its= property as "message signaled
     interrupt support", which is a little bit of a hack
     but I think OK if we're clear about it in the docs.
     (We could rename the property to "msi=(off,its,gicv2m,auto)"
     with back-compat support for "its=" but I don't know if
     that's worth the effort.)

And then that doesn't need any back-compat handling for pre-10.2
machine types or a "no_gicv3_with_gicv2m" flag, because for
10.1 and earlier there is no case that currently works and
which falls into category (c) and which doesn't give you an ITS.
(because we don't yet have hvf gicv3 implemented: that's a new
feature that never worked in 10.1.)

What do you think?

thanks
-- PMM
Re: [PATCH v6 03/23] hw/arm: virt: add GICv2m for the case when ITS is not available
Posted by Mohamed Mediouni 4 months, 1 week ago

> On 25. Sep 2025, at 18:24, Peter Maydell <peter.maydell@linaro.org> wrote:
> 
> On Sat, 20 Sept 2025 at 15:02, Mohamed Mediouni
> <mohamed@unpredictable.fr> wrote:
>> 
>> On Hypervisor.framework for macOS and WHPX for Windows, the provided environment is a GICv3 without ITS.
>> 
>> As such, support a GICv3 w/ GICv2m for that scenario.
>> 
>> Signed-off-by: Mohamed Mediouni <mohamed@unpredictable.fr>
>> 
>> Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>> ---
>> hw/arm/virt-acpi-build.c | 4 +++-
>> hw/arm/virt.c            | 8 ++++++++
>> include/hw/arm/virt.h    | 2 ++
>> 3 files changed, 13 insertions(+), 1 deletion(-)
> 
> Looking at this I find myself wondering whether we need the
> old-version back compat handling. The cases I think we have
> at the moment are:
> 
> (1) TCG, virt-6.1 and earlier: no_tcg_its is set
>   -- you can have a gicv2 (always with a gicv2m)
>   -- if you specify gic-version=3 you get a GICv3 without ITS
> (2) TCG, virt-6.2 and later:
>   -- gic-version=2 still has gicv2m
>   -- gic-version=3 by default gives you an ITS; if you also
>      say its=off you get GICv3 with no ITS
>   -- there is no case where we provide a GICv3 and are
>      unable to provide an ITS for it
> (3) KVM (any version):
>   -- gic-version=2 has a gicv2m
>   -- gic-version=3 gives you an ITS by default; its=off
>      will remove it
>   -- there is no case where we provide a GICv3 and are
>      unable to provide an ITS for it
> (4) HVF:
>   -- only gic-version=2 works, you get a gicv2m
> 
> and I think what we want is:
> (a) if you explicitly disable the ITS (with its=off or via
>     no_tcg_its) you get no ITS (and no gicv2m)
> (b) if you explicitly enable the ITS you should get an
>     actual ITS or an error message
> (c) the default should be its=auto which gives
>     you "ITS if we can, gicv2m if we can't".
>     This is repurposing the its= property as "message signaled
>     interrupt support", which is a little bit of a hack
>     but I think OK if we're clear about it in the docs.
>     (We could rename the property to "msi=(off,its,gicv2m,auto)"
>     with back-compat support for "its=" but I don't know if
>     that's worth the effort.)
> 
> And then that doesn't need any back-compat handling for pre-10.2
> machine types or a "no_gicv3_with_gicv2m" flag, because for
> 10.1 and earlier there is no case that currently works and
> which falls into category (c) and which doesn't give you an ITS.
> (because we don't yet have hvf gicv3 implemented: that's a new
> feature that never worked in 10.1.)
> 
> What do you think?

Would it be wanted to provide MSI-X support in all scenarios even with its=off?
And there’s the consequence of that making GICv3 + GICv2m only testable with auto and not with TCG or kvm, which doesn’t sound ideal.

Thanks,
> thanks
> -- PMM
Re: [PATCH v6 03/23] hw/arm: virt: add GICv2m for the case when ITS is not available
Posted by Peter Maydell 3 months, 2 weeks ago
On Thu, 2 Oct 2025 at 05:30, Mohamed Mediouni <mohamed@unpredictable.fr> wrote:
>
>
>
> > On 25. Sep 2025, at 18:24, Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> > On Sat, 20 Sept 2025 at 15:02, Mohamed Mediouni
> > <mohamed@unpredictable.fr> wrote:
> >>
> >> On Hypervisor.framework for macOS and WHPX for Windows, the provided environment is a GICv3 without ITS.
> >>
> >> As such, support a GICv3 w/ GICv2m for that scenario.
> >>
> >> Signed-off-by: Mohamed Mediouni <mohamed@unpredictable.fr>
> >>
> >> Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> >> ---
> >> hw/arm/virt-acpi-build.c | 4 +++-
> >> hw/arm/virt.c            | 8 ++++++++
> >> include/hw/arm/virt.h    | 2 ++
> >> 3 files changed, 13 insertions(+), 1 deletion(-)
> >
> > Looking at this I find myself wondering whether we need the
> > old-version back compat handling. The cases I think we have
> > at the moment are:
> >
> > (1) TCG, virt-6.1 and earlier: no_tcg_its is set
> >   -- you can have a gicv2 (always with a gicv2m)
> >   -- if you specify gic-version=3 you get a GICv3 without ITS
> > (2) TCG, virt-6.2 and later:
> >   -- gic-version=2 still has gicv2m
> >   -- gic-version=3 by default gives you an ITS; if you also
> >      say its=off you get GICv3 with no ITS
> >   -- there is no case where we provide a GICv3 and are
> >      unable to provide an ITS for it
> > (3) KVM (any version):
> >   -- gic-version=2 has a gicv2m
> >   -- gic-version=3 gives you an ITS by default; its=off
> >      will remove it
> >   -- there is no case where we provide a GICv3 and are
> >      unable to provide an ITS for it
> > (4) HVF:
> >   -- only gic-version=2 works, you get a gicv2m
> >
> > and I think what we want is:
> > (a) if you explicitly disable the ITS (with its=off or via
> >     no_tcg_its) you get no ITS (and no gicv2m)
> > (b) if you explicitly enable the ITS you should get an
> >     actual ITS or an error message
> > (c) the default should be its=auto which gives
> >     you "ITS if we can, gicv2m if we can't".
> >     This is repurposing the its= property as "message signaled
> >     interrupt support", which is a little bit of a hack
> >     but I think OK if we're clear about it in the docs.
> >     (We could rename the property to "msi=(off,its,gicv2m,auto)"
> >     with back-compat support for "its=" but I don't know if
> >     that's worth the effort.)
> >
> > And then that doesn't need any back-compat handling for pre-10.2
> > machine types or a "no_gicv3_with_gicv2m" flag, because for
> > 10.1 and earlier there is no case that currently works and
> > which falls into category (c) and which doesn't give you an ITS.
> > (because we don't yet have hvf gicv3 implemented: that's a new
> > feature that never worked in 10.1.)
> >
> > What do you think?
>
> Would it be wanted to provide MSI-X support in all scenarios
> even with its=off?

We should prefer to provide MSI-X support. If the user
explicitly asks for a config that doesn't give MSI-X
support, that's their choice to make.

> And there’s the consequence of that making GICv3 + GICv2m only
> testable with auto and not with TCG or kvm, which doesn’t sound ideal.

I guess that would be an argument for the "give the property
the right name so we can say "msi=(off,its,gicv2m,auto)". Then
you could say
 -accel tcg -machine gic-version=3,msi=gicv2m

to test that setup.

thanks
-- PMM
Re: [PATCH v6 03/23] hw/arm: virt: add GICv2m for the case when ITS is not available
Posted by Mohamed Mediouni 3 months, 2 weeks ago

> On 27. Oct 2025, at 17:03, Peter Maydell <peter.maydell@linaro.org> wrote:
> 
> On Thu, 2 Oct 2025 at 05:30, Mohamed Mediouni <mohamed@unpredictable.fr> wrote:
>> 
>> 
>> 
>>> On 25. Sep 2025, at 18:24, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> 
>>> On Sat, 20 Sept 2025 at 15:02, Mohamed Mediouni
>>> <mohamed@unpredictable.fr> wrote:
>>>> 
>>>> On Hypervisor.framework for macOS and WHPX for Windows, the provided environment is a GICv3 without ITS.
>>>> 
>>>> As such, support a GICv3 w/ GICv2m for that scenario.
>>>> 
>>>> Signed-off-by: Mohamed Mediouni <mohamed@unpredictable.fr>
>>>> 
>>>> Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>>>> ---
>>>> hw/arm/virt-acpi-build.c | 4 +++-
>>>> hw/arm/virt.c            | 8 ++++++++
>>>> include/hw/arm/virt.h    | 2 ++
>>>> 3 files changed, 13 insertions(+), 1 deletion(-)
>>> 
>>> Looking at this I find myself wondering whether we need the
>>> old-version back compat handling. The cases I think we have
>>> at the moment are:
>>> 
>>> (1) TCG, virt-6.1 and earlier: no_tcg_its is set
>>>  -- you can have a gicv2 (always with a gicv2m)
>>>  -- if you specify gic-version=3 you get a GICv3 without ITS
>>> (2) TCG, virt-6.2 and later:
>>>  -- gic-version=2 still has gicv2m
>>>  -- gic-version=3 by default gives you an ITS; if you also
>>>     say its=off you get GICv3 with no ITS
>>>  -- there is no case where we provide a GICv3 and are
>>>     unable to provide an ITS for it
>>> (3) KVM (any version):
>>>  -- gic-version=2 has a gicv2m
>>>  -- gic-version=3 gives you an ITS by default; its=off
>>>     will remove it
>>>  -- there is no case where we provide a GICv3 and are
>>>     unable to provide an ITS for it
>>> (4) HVF:
>>>  -- only gic-version=2 works, you get a gicv2m
>>> 
>>> and I think what we want is:
>>> (a) if you explicitly disable the ITS (with its=off or via
>>>    no_tcg_its) you get no ITS (and no gicv2m)
>>> (b) if you explicitly enable the ITS you should get an
>>>    actual ITS or an error message
>>> (c) the default should be its=auto which gives
>>>    you "ITS if we can, gicv2m if we can't".
>>>    This is repurposing the its= property as "message signaled
>>>    interrupt support", which is a little bit of a hack
>>>    but I think OK if we're clear about it in the docs.
>>>    (We could rename the property to "msi=(off,its,gicv2m,auto)"
>>>    with back-compat support for "its=" but I don't know if
>>>    that's worth the effort.)
>>> 
>>> And then that doesn't need any back-compat handling for pre-10.2
>>> machine types or a "no_gicv3_with_gicv2m" flag, because for
>>> 10.1 and earlier there is no case that currently works and
>>> which falls into category (c) and which doesn't give you an ITS.
>>> (because we don't yet have hvf gicv3 implemented: that's a new
>>> feature that never worked in 10.1.)
>>> 
>>> What do you think?
>> 
>> Would it be wanted to provide MSI-X support in all scenarios
>> even with its=off?
> 
> We should prefer to provide MSI-X support. If the user
> explicitly asks for a config that doesn't give MSI-X
> support, that's their choice to make.
> 
>> And there’s the consequence of that making GICv3 + GICv2m only
>> testable with auto and not with TCG or kvm, which doesn’t sound ideal.
> 
> I guess that would be an argument for the "give the property
> the right name so we can say "msi=(off,its,gicv2m,auto)". Then
> you could say
> -accel tcg -machine gic-version=3,msi=gicv2m
> 
> to test that setup.

Is there guidance around renaming properties?

Would it be proper to do:
- if its=auto, consider the new msi property
- otherwise, use the its property

Thank you,
-Mohamed
> thanks
> -- PMM
> 
Re: [PATCH v6 03/23] hw/arm: virt: add GICv2m for the case when ITS is not available
Posted by Peter Maydell 3 months, 2 weeks ago
On Mon, 27 Oct 2025 at 16:53, Mohamed Mediouni <mohamed@unpredictable.fr> wrote:
> > On 27. Oct 2025, at 17:03, Peter Maydell <peter.maydell@linaro.org> wrote:
> > I guess that would be an argument for the "give the property
> > the right name so we can say "msi=(off,its,gicv2m,auto)". Then
> > you could say
> > -accel tcg -machine gic-version=3,msi=gicv2m
> >
> > to test that setup.
>
> Is there guidance around renaming properties?

I'm not sure if there is. So the below is just my
initial suggestion.

> Would it be proper to do:
> - if its=auto, consider the new msi property
> - otherwise, use the its property

I think we should write the code in a way that looks ahead
to marking the its property as deprecated and eventually
removing it. So the handling for the new "msi" property
should be done in a way that doesn't need changes if/when we
drop the "its" property.

We don't currently attempt to detect oddball user
commandlines like "-M virt,its=on,its=off", so I don't
think we need to go to any particular effort to diagnose
the equivalently odd "-M virt,its=on,msi=off" etc.

We can implement the two options as essentially
independent, where "its=on" is equivalent to "msi=auto"
and "its=off" is equivalent to "msi=off" (i.e. what
we currently have as a bool turns into an enum, and the
set/get functions for "its" and "msi" both operate on
the same underlying struct field.)

thanks
-- PMM