Re: [PATCH v4 6/6] hw/arm/virt: kvm: allow gicv3 by default if v2 cannot work

[Andrew Jones]

On Wed, Mar 11, 2020 at 12:16:26PM +0100, Eric Auger wrote:
> At the moment if the end-user does not specify the gic-version along
> with KVM acceleration, v2 is set by default. However most of the
> systems now have GICv3 and sometimes they do not support GICv2
> compatibility.
> 
> This patch keeps the default v2 selection in all cases except
> in the KVM accelerated mode when either
> - the host does not support GICv2 in-kernel emulation or
> - number of VCPUS exceeds 8.
> 
> Those cases did not work anyway so we do not break any compatibility.
> Now we get v3 selected in such a case.
> 
> Signed-off-by: Eric Auger <address@hidden>
> Reported-by: Dr. David Alan Gilbert <address@hidden>
> 
> ---
> 
> v3 -> v4:
> - Deal with the case where v3 is not supported by the host and
>   the number of vcpus exceeds 8
> 
> v2 -> v3:
> - add ()
> ---
>  hw/arm/virt.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index f5ff2d9006..6becf9aaae 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -1543,6 +1543,8 @@ static void virt_set_memmap(VirtMachineState *vms)
>   */
>  static void finalize_gic_version(VirtMachineState *vms)
>  {
> +    unsigned int max_cpus = MACHINE(vms)->smp.max_cpus;
> +
>      if (kvm_enabled()) {
>          int probe_bitmap;
>  
> @@ -1583,7 +1585,20 @@ static void finalize_gic_version(VirtMachineState *vms)
>              }
>              return;
>          case VIRT_GIC_VERSION_NOSEL:
> -            vms->gic_version = VIRT_GIC_VERSION_2;
> +            if ((probe_bitmap & KVM_ARM_VGIC_V2) && max_cpus <= GIC_NCPU) {
> +                vms->gic_version = VIRT_GIC_VERSION_2;
> +            } else if (probe_bitmap & KVM_ARM_VGIC_V3) {
> +                /*
> +                 * in case the host does not support v2 in-kernel emulation 
> or
> +                 * the end-user requested more than 8 VCPUs we now default
> +                 * to v3. In any case defaulting to v2 would be broken.
> +                 */
> +                vms->gic_version = VIRT_GIC_VERSION_3;
> +            } else {

(probe & V3) is only == !(probe & V2) since we don't have more versions.
If we did, then the assumption cpus > GIC_NCPU here wouldn't be correct.
I'd just make this an 'else if (cpus > GIC_NCPU)' to be explicit.

> +                error_report("host only supports in-kernel GICv2 emulation "
> +                             "but more than 8 vcpus are requested");
> +                exit(1);
> +            }
>              break;
>          case VIRT_GIC_VERSION_2:
>          case VIRT_GIC_VERSION_3:
> -- 
> 2.20.1
> 

Otherwise

Reviewed-by: Andrew Jones <address@hidden>