Re: [RFC 2/2] hw/arm/virt: kvm: allow gicv3 by default if host does not support v2

[Andrew Jones]

On Wed, Feb 26, 2020 at 06:05:00PM +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. In that case we end up with this error:
> 
> qemu-system-aarch64: PMU: KVM_SET_DEVICE_ATTR: Invalid argument
> qemu-system-aarch64: failed to set irq for PMU
> and qemu aborts.
> 
> This patch keeps the default v2 selection in all cases except
> in the KVM accelerated mode when the host does not support v2.
> This case did not work anyway so we do not break any compatibility.
> Now we get v3 selected in such a case.

> Also if the end-user explicitly
> sets v2 whereas this latter is not supported, we also are
> informed that v2 is not selected by thos host instead of getting the
> above PMU related message.

I would change the above to:

Also, if the end-user explicitly sets v2 and this is not supported by
the host, then the user gets a more informative error message than
the PMU invalid argument message above.

> 
> Signed-off-by: Eric Auger <address@hidden>
> Reported-by: Dr. David Alan Gilbert <address@hidden>
> ---
>  hw/arm/virt.c         | 30 ++++++++++++++++++++++++++++++
>  include/hw/arm/virt.h |  1 +
>  2 files changed, 31 insertions(+)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index a196bbf0d5..b37b0c40c1 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -1554,6 +1554,33 @@ static void machvirt_init(MachineState *machine)
>                  }
>              }
>          }
> +    } else if (kvm_enabled()) {
> +        int probe_bitmap = kvm_arm_vgic_probe();
> +
> +        if (!probe_bitmap) {
> +            error_report(
> +                "Unable to determine GIC version supported by host");
> +            exit(1);
> +        }
> +        if (!vms->gic_version_user_selected) {
> +            /*
> +             * by default v2 is supposed to be chosen: check it is
> +             * supported by the host. Otherwise take v3.
> +             */
> +            if (probe_bitmap & KVM_ARM_VGIC_V2) {
> +                vms->gic_version = 2;
> +            } else if (probe_bitmap & KVM_ARM_VGIC_V3) {
> +                vms->gic_version = 3;
> +            }
> +        } else { /* user explicitly set the version to 2 or 3 */
> +            if (vms->gic_version == 2 && !(probe_bitmap & KVM_ARM_VGIC_V2)) {
> +                error_report("GICv2 is not supported by the host");
> +                exit(1);
> +            } else if (vms->gic_version == 3 && !(probe_bitmap & 
> KVM_ARM_VGIC_V3)) {
> +                error_report("GICv3 is not supported by the host");
> +                exit(1);
> +            }
> +        }
>      }
>  
>      if (!cpu_type_valid(machine->cpu_type)) {
> @@ -1840,6 +1867,7 @@ static void virt_set_gic_version(Object *obj, const 
> char *value, Error **errp)
>  {
>      VirtMachineState *vms = VIRT_MACHINE(obj);
>  
> +    vms->gic_version_user_selected = true;
>      if (!strcmp(value, "3")) {
>          vms->gic_version = 3;
>      } else if (!strcmp(value, "2")) {
> @@ -1851,6 +1879,7 @@ static void virt_set_gic_version(Object *obj, const 
> char *value, Error **errp)
>      } else {
>          error_setg(errp, "Invalid gic-version value");
>          error_append_hint(errp, "Valid values are 3, 2, host, max.\n");
> +        vms->gic_version_user_selected = false;
>      }
>  }
>  
> @@ -2103,6 +2132,7 @@ static void virt_instance_init(Object *obj)
>                                      NULL);
>      /* Default GIC type is v2 */
>      vms->gic_version = 2;
> +    vms->gic_version_user_selected = false;
>      object_property_add_str(obj, "gic-version", virt_get_gic_version,
>                          virt_set_gic_version, NULL);
>      object_property_set_description(obj, "gic-version",
> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> index 71508bf40c..e363cde452 100644
> --- a/include/hw/arm/virt.h
> +++ b/include/hw/arm/virt.h
> @@ -124,6 +124,7 @@ typedef struct {
>      bool its;
>      bool virt;
>      int32_t gic_version;
> +    bool gic_version_user_selected;
>      VirtIOMMUType iommu;
>      struct arm_boot_info bootinfo;
>      MemMapEntry *memmap;
> -- 
> 2.20.1
> 
>

I just noticed that virt_get_gic_version() returns "2" if the user didn't
explicitly request "3". I guess that hasn't been a problem for "max"
and "host" because nobody is looking, at least not until after gic_version
has been fully initialized. It is wrong though (for example, with TCG
"max" should be "3", not "2"). Ideally virt_get_gic_version() would always
return what will actually be used by the guest, but maybe pre-init it
could return "max", "host", and "nosel" instead. We could then use the
property value "nosel" in machvirt_init() to determine whether or not the
user provided input, rather than adding gic_version_user_selected.

Thanks,
drew