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

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

[Eric Auger]

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.

Best Regards

Eric

This series can be found at:
https://github.com/eauger/qemu/tree/gic_version_rfc_v1


Eric Auger (2):
  target/arm/kvm: Let kvm_arm_vgic_probe() return a bitmap
  hw/arm/virt: kvm: allow gicv3 by default if host does not support v2

 hw/arm/virt.c         | 41 +++++++++++++++++++++++++++++++++++++++--
 include/hw/arm/virt.h |  1 +
 target/arm/kvm.c      | 14 ++++++++------
 target/arm/kvm_arm.h  |  3 +++
 4 files changed, 51 insertions(+), 8 deletions(-)

-- 
2.20.1

[RFC 1/2] target/arm/kvm: Let kvm_arm_vgic_probe() return a bitmap

[Eric Auger]

Let convert kvm_arm_vgic_probe() so that it returns a
bitmap of supported VGIC versions. This allows to expose
the case where the host GICv3 also supports GICv2 emulation.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 hw/arm/virt.c        | 11 +++++++++--
 target/arm/kvm.c     | 14 ++++++++------
 target/arm/kvm_arm.h |  3 +++
 3 files changed, 20 insertions(+), 8 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index e591a126e7..a196bbf0d5 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1540,11 +1540,18 @@ static void machvirt_init(MachineState *machine)
                 vms->gic_version = 3;
             }
         } else {
-            vms->gic_version = kvm_arm_vgic_probe();
-            if (!vms->gic_version) {
+            int probe_bitmap = kvm_arm_vgic_probe();
+
+            if (!probe_bitmap) {
                 error_report(
                     "Unable to determine GIC version supported by host");
                 exit(1);
+            } else {
+                if (probe_bitmap & KVM_ARM_VGIC_V3) {
+                    vms->gic_version = 3;
+                } else {
+                    vms->gic_version = 2;
+                }
             }
         }
     }
diff --git a/target/arm/kvm.c b/target/arm/kvm.c
index 85860e6f95..390077c518 100644
--- a/target/arm/kvm.c
+++ b/target/arm/kvm.c
@@ -874,15 +874,17 @@ int kvm_arch_irqchip_create(KVMState *s)
 
 int kvm_arm_vgic_probe(void)
 {
+    int val = 0;
+
     if (kvm_create_device(kvm_state,
                           KVM_DEV_TYPE_ARM_VGIC_V3, true) == 0) {
-        return 3;
-    } else if (kvm_create_device(kvm_state,
-                                 KVM_DEV_TYPE_ARM_VGIC_V2, true) == 0) {
-        return 2;
-    } else {
-        return 0;
+        val |= KVM_ARM_VGIC_V3;
     }
+    if (kvm_create_device(kvm_state,
+                          KVM_DEV_TYPE_ARM_VGIC_V2, true) == 0) {
+        val |= KVM_ARM_VGIC_V2;
+    }
+    return val;
 }
 
 int kvm_arm_set_irq(int cpu, int irqtype, int irq, int level)
diff --git a/target/arm/kvm_arm.h b/target/arm/kvm_arm.h
index ae9e075d75..48bf5e16d5 100644
--- a/target/arm/kvm_arm.h
+++ b/target/arm/kvm_arm.h
@@ -15,6 +15,9 @@
 #include "exec/memory.h"
 #include "qemu/error-report.h"
 
+#define KVM_ARM_VGIC_V2   (1 << 0)
+#define KVM_ARM_VGIC_V3   (1 << 1)
+
 /**
  * kvm_arm_vcpu_init:
  * @cs: CPUState
-- 
2.20.1

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

[Eric Auger]

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.

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

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 <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"). 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

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

[Auger Eric]

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
> 
> 

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.
> 
> 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) {

How about 'if ((probe_bitmap & KVM_ARM_VGIC_V2) && max_cpus <= GIC_NCPU)',
because even on gicv3 hosts that support gicv2 guests command lines that
didn't specify a gic version and did specify more than 8 vcpus were also
broken. We can now automagically allow those to work too.

Thanks,
drew