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

[Auger Eric <eric.auger@redhat.com>]

Hi Drew,

On 2/26/20 7:42 PM, Andrew Jones wrote:
> 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.

Sure
> 
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> Reported-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>> ---
>>  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").

That's correct indeed.

 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.

OK I will try to address that as well.

I also notice the property description for gic-version misses "max"

Thanks

Eric
> 
> Thanks,
> drew
> 
>