[PATCH v4 12/17] hw/arm: virt: cleanly fail on attempt to use the platform vGIC together with ITS

Mohamed Mediouni posted 17 patches 3 months, 1 week ago
Maintainers: Cameron Esfahani <dirty@apple.com>, Roman Bolshakov <rbolshakov@ddn.com>, Phil Dennis-Jordan <phil@philjordan.eu>, Mads Ynddal <mads@ynddal.dk>, Sunil Muthuswamy <sunilmut@microsoft.com>, Mohamed Mediouni <mohamed@unpredictable.fr>, Peter Maydell <peter.maydell@linaro.org>, "Michael S. Tsirkin" <mst@redhat.com>, Igor Mammedov <imammedo@redhat.com>, Ani Sinha <anisinha@redhat.com>, Shannon Zhao <shannon.zhaosl@gmail.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 v4 12/17] hw/arm: virt: cleanly fail on attempt to use the platform vGIC together with ITS
Posted by Mohamed Mediouni 3 months, 1 week ago
Windows Hypervisor Platform's vGIC doesn't support ITS.
Deal with this by reporting to the user and not creating the ITS device.

Signed-off-by: Mohamed Mediouni <mohamed@unpredictable.fr>
---
 hw/arm/virt.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 98a1c74c42..0039f6a12b 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -741,6 +741,16 @@ static void create_its(VirtMachineState *vms)
         return;
     }
 
+    if (whpx_enabled() && vms->tcg_its) {
+        /*
+         * Signal to the user when ITS is neither supported by the host
+         * nor emulated by the machine.
+         */
+        info_report("ITS not supported on WHPX.");
+        info_report("To support MSIs, use its=off to enable GICv3 + GICv2m.");
+        return;
+    }
+
     dev = qdev_new(its_class_name());
 
     object_property_set_link(OBJECT(dev), "parent-gicv3", OBJECT(vms->gic),
-- 
2.39.5 (Apple Git-154)
Re: [PATCH v4 12/17] hw/arm: virt: cleanly fail on attempt to use the platform vGIC together with ITS
Posted by Philippe Mathieu-Daudé 3 months, 1 week ago
On 4/8/25 16:23, Mohamed Mediouni wrote:
> Windows Hypervisor Platform's vGIC doesn't support ITS.
> Deal with this by reporting to the user and not creating the ITS device.
> 
> Signed-off-by: Mohamed Mediouni <mohamed@unpredictable.fr>
> ---
>   hw/arm/virt.c | 10 ++++++++++
>   1 file changed, 10 insertions(+)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 98a1c74c42..0039f6a12b 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -741,6 +741,16 @@ static void create_its(VirtMachineState *vms)
>           return;
>       }
>   
> +    if (whpx_enabled() && vms->tcg_its) {
> +        /*
> +         * Signal to the user when ITS is neither supported by the host
> +         * nor emulated by the machine.
> +         */
> +        info_report("ITS not supported on WHPX.");
> +        info_report("To support MSIs, use its=off to enable GICv3 + GICv2m.");

So if the users deliberately asks for its=on, we ignore the request and
keep going. Shouldn't we just exit so the user adapts its command line?

> +        return;
> +    }
> +
>       dev = qdev_new(its_class_name());
>   
>       object_property_set_link(OBJECT(dev), "parent-gicv3", OBJECT(vms->gic),
Re: [PATCH v4 12/17] hw/arm: virt: cleanly fail on attempt to use the platform vGIC together with ITS
Posted by Mohamed Mediouni 3 months, 1 week ago

> On 5. Aug 2025, at 00:59, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> 
> On 4/8/25 16:23, Mohamed Mediouni wrote:
>> Windows Hypervisor Platform's vGIC doesn't support ITS.
>> Deal with this by reporting to the user and not creating the ITS device.
>> Signed-off-by: Mohamed Mediouni <mohamed@unpredictable.fr>
>> ---
>>  hw/arm/virt.c | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index 98a1c74c42..0039f6a12b 100644
>> --- a/hw/arm/virt.c
>> +++ b/hw/arm/virt.c
>> @@ -741,6 +741,16 @@ static void create_its(VirtMachineState *vms)
>>          return;
>>      }
>>  +    if (whpx_enabled() && vms->tcg_its) {
>> +        /*
>> +         * Signal to the user when ITS is neither supported by the host
>> +         * nor emulated by the machine.
>> +         */
>> +        info_report("ITS not supported on WHPX.");
>> +        info_report("To support MSIs, use its=off to enable GICv3 + GICv2m.");
> 
> So if the users deliberately asks for its=on, we ignore the request and
> keep going. Shouldn't we just exit so the user adapts its command line?
Maybe that’s the best way to go… what makes me a bit hesitant on that one though is that the default is its=on.
>> +        return;
>> +    }
>> +
>>      dev = qdev_new(its_class_name());
>>        object_property_set_link(OBJECT(dev), "parent-gicv3", OBJECT(vms->gic),
> 
> 
Re: [PATCH v4 12/17] hw/arm: virt: cleanly fail on attempt to use the platform vGIC together with ITS
Posted by Pierrick Bouvier 3 months, 1 week ago
On 8/4/25 7:23 AM, Mohamed Mediouni wrote:
> Windows Hypervisor Platform's vGIC doesn't support ITS.
> Deal with this by reporting to the user and not creating the ITS device.
> 
> Signed-off-by: Mohamed Mediouni <mohamed@unpredictable.fr>
> ---
>   hw/arm/virt.c | 10 ++++++++++
>   1 file changed, 10 insertions(+)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 98a1c74c42..0039f6a12b 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -741,6 +741,16 @@ static void create_its(VirtMachineState *vms)
>           return;
>       }
>   
> +    if (whpx_enabled() && vms->tcg_its) {
> +        /*
> +         * Signal to the user when ITS is neither supported by the host
> +         * nor emulated by the machine.
> +         */
> +        info_report("ITS not supported on WHPX.");
> +        info_report("To support MSIs, use its=off to enable GICv3 + GICv2m.");
> +        return;
> +    }
> +
>       dev = qdev_new(its_class_name());
>   
>       object_property_set_link(OBJECT(dev), "parent-gicv3", OBJECT(vms->gic),

Is it equivalent to simply using its=off, or is there a difference?
The info_report seems to imply it's not the same.
Re: [PATCH v4 12/17] hw/arm: virt: cleanly fail on attempt to use the platform vGIC together with ITS
Posted by Mohamed Mediouni 3 months, 1 week ago

> On 4. Aug 2025, at 21:50, Pierrick Bouvier <pierrick.bouvier@linaro.org> wrote:
> 
> On 8/4/25 7:23 AM, Mohamed Mediouni wrote:
>> Windows Hypervisor Platform's vGIC doesn't support ITS.
>> Deal with this by reporting to the user and not creating the ITS device.
>> Signed-off-by: Mohamed Mediouni <mohamed@unpredictable.fr>
>> ---
>>  hw/arm/virt.c | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index 98a1c74c42..0039f6a12b 100644
>> --- a/hw/arm/virt.c
>> +++ b/hw/arm/virt.c
>> @@ -741,6 +741,16 @@ static void create_its(VirtMachineState *vms)
>>          return;
>>      }
>>  +    if (whpx_enabled() && vms->tcg_its) {
>> +        /*
>> +         * Signal to the user when ITS is neither supported by the host
>> +         * nor emulated by the machine.
>> +         */
>> +        info_report("ITS not supported on WHPX.");
>> +        info_report("To support MSIs, use its=off to enable GICv3 + GICv2m.");
>> +        return;
>> +    }
>> +
>>      dev = qdev_new(its_class_name());
>>        object_property_set_link(OBJECT(dev), "parent-gicv3", OBJECT(vms->gic),
> 
> Is it equivalent to simply using its=off, or is there a difference?
> The info_report seems to imply it's not the same.

Not equivalent.

Regular system: GICv3 + ITS
This configuration (for the newest machine version): GICv3 with no MSIs
And its=off explicitly: GICv3 + GICv2m

It became not equivalent since the intro of GICv3 + GICv2m in patch 2 of this series.

Thank you,



Re: [PATCH v4 12/17] hw/arm: virt: cleanly fail on attempt to use the platform vGIC together with ITS
Posted by Pierrick Bouvier 3 months, 1 week ago
On 8/4/25 12:56 PM, Mohamed Mediouni wrote:
> 
> 
>> On 4. Aug 2025, at 21:50, Pierrick Bouvier 
>> <pierrick.bouvier@linaro.org> wrote:
>>
>> On 8/4/25 7:23 AM, Mohamed Mediouni wrote:
>>> Windows Hypervisor Platform's vGIC doesn't support ITS.
>>> Deal with this by reporting to the user and not creating the ITS device.
>>> Signed-off-by: Mohamed Mediouni <mohamed@unpredictable.fr>
>>> ---
>>>  hw/arm/virt.c | 10 ++++++++++
>>>  1 file changed, 10 insertions(+)
>>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>>> index 98a1c74c42..0039f6a12b 100644
>>> --- a/hw/arm/virt.c
>>> +++ b/hw/arm/virt.c
>>> @@ -741,6 +741,16 @@ static void create_its(VirtMachineState *vms)
>>>          return;
>>>      }
>>>  +    if (whpx_enabled() && vms->tcg_its) {
>>> +        /*
>>> +         * Signal to the user when ITS is neither supported by the host
>>> +         * nor emulated by the machine.
>>> +         */
>>> +        info_report("ITS not supported on WHPX.");
>>> +        info_report("To support MSIs, use its=off to enable GICv3 + 
>>> GICv2m.");
>>> +        return;
>>> +    }
>>> +
>>>      dev = qdev_new(its_class_name());
>>>        object_property_set_link(OBJECT(dev), "parent-gicv3", 
>>> OBJECT(vms->gic),
>>
>> Is it equivalent to simply using its=off, or is there a difference?
>> The info_report seems to imply it's not the same.
> 
> Not equivalent.
> 
> Regular system: GICv3 + ITS
> This configuration (for the newest machine version): GICv3 with no MSIs
> And its=off explicitly: GICv3 + GICv2m
> 
> It became not equivalent since the intro of GICv3 + GICv2m in patch 2 of 
> this series.
>
> Thank you,
> 

I see. It could be worth adding this information to commit message.
With that,
Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

Slightly off topic for this commit, is there any downside to always have 
GICv3 + GICv2m setup enabled? Do some systems don't support GICv2m?

Thanks,
Pierrick

Re: [PATCH v4 12/17] hw/arm: virt: cleanly fail on attempt to use the platform vGIC together with ITS
Posted by Mohamed Mediouni 3 months, 1 week ago

> On 4. Aug 2025, at 22:40, Pierrick Bouvier <pierrick.bouvier@linaro.org> wrote:
> 
> On 8/4/25 12:56 PM, Mohamed Mediouni wrote:
>>> On 4. Aug 2025, at 21:50, Pierrick Bouvier <pierrick.bouvier@linaro.org> wrote:
>>> 
>>> Is it equivalent to simply using its=off, or is there a difference?
>>> The info_report seems to imply it's not the same.
>> Not equivalent.
>> Regular system: GICv3 + ITS
>> This configuration (for the newest machine version): GICv3 with no MSIs
>> And its=off explicitly: GICv3 + GICv2m
>> It became not equivalent since the intro of GICv3 + GICv2m in patch 2 of this series.
>> 
>> Thank you,
> 
> I see. It could be worth adding this information to commit message.
> With that,
> Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> 
> Slightly off topic for this commit, is there any downside to always have GICv3 + GICv2m setup enabled? Do some systems don't support GICv2m?
> 
GICv2m is modelled as a device external from the GIC, and so it can be emulated everywhere.

It can be done but then it supposes that the same Qemu command line will expose a different device model (instead of just MSI support missing) when ran on a different system. Not sure that’s the right thing to do…

That’s because the current setup already has ITS on by default when a GICv3 is chosen.

Note that GICv3 + GICv2m* hardware exists in the wild (Graviton1 notably) but VMs have a GICv3 w/ ITS configuration there.

* not quite, but gets shoehorned in with Linux quirks

Thank you,
-m
Re: [PATCH v4 12/17] hw/arm: virt: cleanly fail on attempt to use the platform vGIC together with ITS
Posted by Pierrick Bouvier 3 months, 1 week ago
On 8/4/25 1:59 PM, Mohamed Mediouni wrote:
> 
> 
>> On 4. Aug 2025, at 22:40, Pierrick Bouvier <pierrick.bouvier@linaro.org> wrote:
>>
>> On 8/4/25 12:56 PM, Mohamed Mediouni wrote:
>>>> On 4. Aug 2025, at 21:50, Pierrick Bouvier <pierrick.bouvier@linaro.org> wrote:
>>>>
>>>> Is it equivalent to simply using its=off, or is there a difference?
>>>> The info_report seems to imply it's not the same.
>>> Not equivalent.
>>> Regular system: GICv3 + ITS
>>> This configuration (for the newest machine version): GICv3 with no MSIs
>>> And its=off explicitly: GICv3 + GICv2m
>>> It became not equivalent since the intro of GICv3 + GICv2m in patch 2 of this series.
>>>
>>> Thank you,
>>
>> I see. It could be worth adding this information to commit message.
>> With that,
>> Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>>
>> Slightly off topic for this commit, is there any downside to always have GICv3 + GICv2m setup enabled? Do some systems don't support GICv2m?
>>
> GICv2m is modelled as a device external from the GIC, and so it can be emulated everywhere.
> 
> It can be done but then it supposes that the same Qemu command line will expose a different device model (instead of just MSI support missing) when ran on a different system. Not sure that’s the right thing to do…
> 

Indeed, it might be surprising, but on the other side, we already do 
something specific to whpx, so it might be better to choose the default 
offering the maximum support.

 From user point of view, the question is how hard it would be for them 
to guess what is missing in case they need MSI support, and didn't see 
the warning message about using its=off in this case.

Peter, any opinion on this?