hw/arm/virt: Fix gic-version=max when CONFIG_ARM_GIC_TCG is unset

[PATCH v2] hw/arm/virt: Fix gic-version=max when CONFIG_ARM_GIC_TCG is unset

Posted by Eric Auger 3 years, 11 months ago

In TCG mode, if gic-version=max we always select GICv3 even if
CONFIG_ARM_GIC_TCG is unset. We shall rather select GICv2.
This also brings the benefit of fixing qos tests errors for tests
using gic-version=max with CONFIG_ARM_GIC_TCG unset.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
Reviewed-by: Andrew Jones <drjones@redhat.com>
Fixes: a8a5546798c3 ("hw/intc/arm_gicv3: Introduce CONFIG_ARM_GIC_TCG Kconfig selector")

---

v1 -> v2:
- added Drew's R-b
- dropped PATCH 2/2 for the time being (requires more discussions)
---
 hw/arm/virt.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 46bf7ceddf..f29779c951 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1852,7 +1852,11 @@ static void finalize_gic_version(VirtMachineState *vms)
         vms->gic_version = VIRT_GIC_VERSION_2;
         break;
     case VIRT_GIC_VERSION_MAX:
+#ifdef CONFIG_ARM_GIC_TCG
         vms->gic_version = VIRT_GIC_VERSION_3;
+#else
+        vms->gic_version = VIRT_GIC_VERSION_2;
+#endif
         break;
     case VIRT_GIC_VERSION_HOST:
         error_report("gic-version=host requires KVM");
-- 
2.26.3

Re: [PATCH v2] hw/arm/virt: Fix gic-version=max when CONFIG_ARM_GIC_TCG is unset

Posted by Peter Maydell 3 years, 11 months ago

On Fri, 4 Mar 2022 at 17:46, Eric Auger <eric.auger@redhat.com> wrote:
>
> In TCG mode, if gic-version=max we always select GICv3 even if
> CONFIG_ARM_GIC_TCG is unset. We shall rather select GICv2.
> This also brings the benefit of fixing qos tests errors for tests
> using gic-version=max with CONFIG_ARM_GIC_TCG unset.
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> Reviewed-by: Andrew Jones <drjones@redhat.com>
> Fixes: a8a5546798c3 ("hw/intc/arm_gicv3: Introduce CONFIG_ARM_GIC_TCG Kconfig selector")
>
> ---
>
> v1 -> v2:
> - added Drew's R-b
> - dropped PATCH 2/2 for the time being (requires more discussions)
> ---
>  hw/arm/virt.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 46bf7ceddf..f29779c951 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -1852,7 +1852,11 @@ static void finalize_gic_version(VirtMachineState *vms)
>          vms->gic_version = VIRT_GIC_VERSION_2;
>          break;
>      case VIRT_GIC_VERSION_MAX:
> +#ifdef CONFIG_ARM_GIC_TCG
>          vms->gic_version = VIRT_GIC_VERSION_3;
> +#else
> +        vms->gic_version = VIRT_GIC_VERSION_2;
> +#endif
>          break;
>      case VIRT_GIC_VERSION_HOST:
>          error_report("gic-version=host requires KVM");

This doesn't look right, because the correct condition for
"should 'max' be 2 or 3" is "do we have a GICv2 or a GICv3
compiled in", which is nothing to do with "is the GIC a TCG
GIC or the KVM GIC". Indeed at this point in the code we
know we're dealing with TCG, so the right response to
"we don't have a TCG GIC" is "give up entirely".

I think fundamentally CONFIG_ARM_GIC_TCG is either misnamed
or selecting the wrong files to compile.

thanks
-- PMM

Re: [PATCH v2] hw/arm/virt: Fix gic-version=max when CONFIG_ARM_GIC_TCG is unset

Posted by Eric Auger 3 years, 11 months ago


On 3/4/22 7:36 PM, Peter Maydell wrote:
> On Fri, 4 Mar 2022 at 17:46, Eric Auger <eric.auger@redhat.com> wrote:
>> In TCG mode, if gic-version=max we always select GICv3 even if
>> CONFIG_ARM_GIC_TCG is unset. We shall rather select GICv2.
>> This also brings the benefit of fixing qos tests errors for tests
>> using gic-version=max with CONFIG_ARM_GIC_TCG unset.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> Reviewed-by: Andrew Jones <drjones@redhat.com>
>> Fixes: a8a5546798c3 ("hw/intc/arm_gicv3: Introduce CONFIG_ARM_GIC_TCG Kconfig selector")
>>
>> ---
>>
>> v1 -> v2:
>> - added Drew's R-b
>> - dropped PATCH 2/2 for the time being (requires more discussions)
>> ---
>>  hw/arm/virt.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index 46bf7ceddf..f29779c951 100644
>> --- a/hw/arm/virt.c
>> +++ b/hw/arm/virt.c
>> @@ -1852,7 +1852,11 @@ static void finalize_gic_version(VirtMachineState *vms)
>>          vms->gic_version = VIRT_GIC_VERSION_2;
>>          break;
>>      case VIRT_GIC_VERSION_MAX:
>> +#ifdef CONFIG_ARM_GIC_TCG
>>          vms->gic_version = VIRT_GIC_VERSION_3;
>> +#else
>> +        vms->gic_version = VIRT_GIC_VERSION_2;
>> +#endif
>>          break;
>>      case VIRT_GIC_VERSION_HOST:
>>          error_report("gic-version=host requires KVM");
> This doesn't look right, because the correct condition for
> "should 'max' be 2 or 3" is "do we have a GICv2 or a GICv3
> compiled in", which is nothing to do with "is the GIC a TCG
> GIC or the KVM GIC". Indeed at this point in the code we
> know we're dealing with TCG, so the right response to
> "we don't have a TCG GIC" is "give up entirely".
>
> I think fundamentally CONFIG_ARM_GIC_TCG is either misnamed
> or selecting the wrong files to compile.
Yes CONFIG_ARM_GIC_TCG is misnamed and as far as I understand selects
GICV3 TCG specific files. So I will respin and change the name of the
config.

Thanks

Eric
>
> thanks
> -- PMM
>