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

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

[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 now end up with the following error:

"qemu-system-aarch64: Initialization of device kvm-arm-gic failed:
error creating in-kernel VGIC: No such device
Perhaps the host CPU does not support GICv2?"

since "1904f9b5f1  hw/intc/arm_gic_kvm: Don't assume kernel can
provide a GICv2" which already allowed to output an explicit error
message.

This patch keeps the default v2 selection in all cases except
in the KVM accelerated mode when v2 cannot work:
- either because the host does not support v2 in-kernel emulation or
- because more than 8 vcpus were requested.

Those cases did not work anyway so we do not break any compatibility.
Now we get v3 selected in such a case.

Best Regards

Eric

This series can be found at:
https://github.com/eauger/qemu/tree/v4.2.0-gic-version-v4

History:

v3 -> v4:
- do not probe the host GIC version if kernel-irqchip=off
- In KVM mode / userspace irqchip we immediatly output an error
  in case the end-user explicitly selected v3. Also we warn the
  end-user about the weird usage of host in that case (only
  userspace GICv2 is supported).
- Removed R-b on last 2 patches

v2 -> v3:
- replaced defines by VirtGICType enum type
- fixed some style issue
- collected Richard and Dres's R-b
  except on "hw/arm/virt: Introduce VirtGICType enum type" just
  to make sure this matches their expectation.

RFC -> v1:
- 1904f9b5f1  hw/intc/arm_gic_kvm: Don't assume kernel can
provide a GICv2" now has landed upstream
- Fix gic-version description
- Introduce finalize_gic_version and use switch/cases
- take into account smp value


Eric Auger (6):
  hw/arm/virt: Document 'max' value in gic-version property description
  hw/arm/virt: Introduce VirtGICType enum type
  hw/arm/virt: Introduce finalize_gic_version()
  target/arm/kvm: Let kvm_arm_vgic_probe() return a bitmap
  hw/arm/virt: kvm: Restructure finalize_gic_version()
  hw/arm/virt: kvm: allow gicv3 by default if v2 cannot work

 include/hw/arm/virt.h |  12 +++-
 target/arm/kvm_arm.h  |   3 +
 hw/arm/virt.c         | 146 +++++++++++++++++++++++++++++++++---------
 target/arm/kvm.c      |  14 ++--
 4 files changed, 135 insertions(+), 40 deletions(-)

-- 
2.20.1

[PATCH v4 1/6] hw/arm/virt: Document 'max' value in gic-version property description

[Eric Auger]

Mention 'max' value in the gic-version property description.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Andrew Jones <drjones@redhat.com>
---
 hw/arm/virt.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 32d865a488..7c38c6c7f9 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -2146,7 +2146,8 @@ static void virt_instance_init(Object *obj)
                         virt_set_gic_version, NULL);
     object_property_set_description(obj, "gic-version",
                                     "Set GIC version. "
-                                    "Valid values are 2, 3 and host", NULL);
+                                    "Valid values are 2, 3, host and max",
+                                    NULL);
 
     vms->highmem_ecam = !vmc->no_highmem_ecam;
 
-- 
2.20.1

[PATCH v4 2/6] hw/arm/virt: Introduce VirtGICType enum type

[Eric Auger]

We plan to introduce yet another value for the gic version (nosel).
As we already use exotic values such as 0 and -1, let's introduce
a dedicated enum type and let vms->gic_version take this
type.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Andrew Jones <drjones@redhat.com>

---

v3 -> v4:
- added R-bs

v2 -> v3:
- replaced defines by enum VirtGICType
- use that new type for vms->gic_version
---
 include/hw/arm/virt.h | 11 +++++++++--
 hw/arm/virt.c         | 30 +++++++++++++++---------------
 2 files changed, 24 insertions(+), 17 deletions(-)

diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
index 02f500cb8e..c0827cacdf 100644
--- a/include/hw/arm/virt.h
+++ b/include/hw/arm/virt.h
@@ -95,6 +95,13 @@ typedef enum VirtIOMMUType {
     VIRT_IOMMU_VIRTIO,
 } VirtIOMMUType;
 
+typedef enum VirtGICType {
+    VIRT_GIC_VERSION_MAX,
+    VIRT_GIC_VERSION_HOST,
+    VIRT_GIC_VERSION_2,
+    VIRT_GIC_VERSION_3,
+} VirtGICType;
+
 typedef struct MemMapEntry {
     hwaddr base;
     hwaddr size;
@@ -123,7 +130,7 @@ typedef struct {
     bool highmem_ecam;
     bool its;
     bool virt;
-    int32_t gic_version;
+    VirtGICType gic_version;
     VirtIOMMUType iommu;
     uint16_t virtio_iommu_bdf;
     struct arm_boot_info bootinfo;
@@ -162,7 +169,7 @@ static inline int virt_gicv3_redist_region_count(VirtMachineState *vms)
     uint32_t redist0_capacity =
                 vms->memmap[VIRT_GIC_REDIST].size / GICV3_REDIST_SIZE;
 
-    assert(vms->gic_version == 3);
+    assert(vms->gic_version == VIRT_GIC_VERSION_3);
 
     return vms->smp_cpus > redist0_capacity ? 2 : 1;
 }
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 7c38c6c7f9..4ed1f0cb2e 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -299,7 +299,7 @@ static void fdt_add_timer_nodes(const VirtMachineState *vms)
         irqflags = GIC_FDT_IRQ_FLAGS_EDGE_LO_HI;
     }
 
-    if (vms->gic_version == 2) {
+    if (vms->gic_version == VIRT_GIC_VERSION_2) {
         irqflags = deposit32(irqflags, GIC_FDT_IRQ_PPI_CPU_START,
                              GIC_FDT_IRQ_PPI_CPU_WIDTH,
                              (1 << vms->smp_cpus) - 1);
@@ -440,7 +440,7 @@ static void fdt_add_gic_node(VirtMachineState *vms)
     qemu_fdt_setprop_cell(vms->fdt, nodename, "#address-cells", 0x2);
     qemu_fdt_setprop_cell(vms->fdt, nodename, "#size-cells", 0x2);
     qemu_fdt_setprop(vms->fdt, nodename, "ranges", NULL, 0);
-    if (vms->gic_version == 3) {
+    if (vms->gic_version == VIRT_GIC_VERSION_3) {
         int nb_redist_regions = virt_gicv3_redist_region_count(vms);
 
         qemu_fdt_setprop_string(vms->fdt, nodename, "compatible",
@@ -519,7 +519,7 @@ static void fdt_add_pmu_nodes(const VirtMachineState *vms)
         }
     }
 
-    if (vms->gic_version == 2) {
+    if (vms->gic_version == VIRT_GIC_VERSION_2) {
         irqflags = deposit32(irqflags, GIC_FDT_IRQ_PPI_CPU_START,
                              GIC_FDT_IRQ_PPI_CPU_WIDTH,
                              (1 << vms->smp_cpus) - 1);
@@ -1470,7 +1470,7 @@ static uint64_t virt_cpu_mp_affinity(VirtMachineState *vms, int idx)
          * purposes are to make TCG consistent (with 64-bit KVM hosts)
          * and to improve SGI efficiency.
          */
-        if (vms->gic_version == 3) {
+        if (vms->gic_version == VIRT_GIC_VERSION_3) {
             clustersz = GICV3_TARGETLIST_BITS;
         } else {
             clustersz = GIC_TARGETLIST_BITS;
@@ -1561,15 +1561,15 @@ static void machvirt_init(MachineState *machine)
     /* We can probe only here because during property set
      * KVM is not available yet
      */
-    if (vms->gic_version <= 0) {
-        /* "host" or "max" */
+    if (vms->gic_version == VIRT_GIC_VERSION_HOST ||
+        vms->gic_version == VIRT_GIC_VERSION_MAX) {
         if (!kvm_enabled()) {
-            if (vms->gic_version == 0) {
+            if (vms->gic_version == VIRT_GIC_VERSION_HOST) {
                 error_report("gic-version=host requires KVM");
                 exit(1);
             } else {
                 /* "max": currently means 3 for TCG */
-                vms->gic_version = 3;
+                vms->gic_version = VIRT_GIC_VERSION_3;
             }
         } else {
             vms->gic_version = kvm_arm_vgic_probe();
@@ -1628,7 +1628,7 @@ static void machvirt_init(MachineState *machine)
     /* The maximum number of CPUs depends on the GIC version, or on how
      * many redistributors we can fit into the memory map.
      */
-    if (vms->gic_version == 3) {
+    if (vms->gic_version == VIRT_GIC_VERSION_3) {
         virt_max_cpus =
             vms->memmap[VIRT_GIC_REDIST].size / GICV3_REDIST_SIZE;
         virt_max_cpus +=
@@ -1856,7 +1856,7 @@ static void virt_set_its(Object *obj, bool value, Error **errp)
 static char *virt_get_gic_version(Object *obj, Error **errp)
 {
     VirtMachineState *vms = VIRT_MACHINE(obj);
-    const char *val = vms->gic_version == 3 ? "3" : "2";
+    const char *val = vms->gic_version == VIRT_GIC_VERSION_3 ? "3" : "2";
 
     return g_strdup(val);
 }
@@ -1866,13 +1866,13 @@ static void virt_set_gic_version(Object *obj, const char *value, Error **errp)
     VirtMachineState *vms = VIRT_MACHINE(obj);
 
     if (!strcmp(value, "3")) {
-        vms->gic_version = 3;
+        vms->gic_version = VIRT_GIC_VERSION_3;
     } else if (!strcmp(value, "2")) {
-        vms->gic_version = 2;
+        vms->gic_version = VIRT_GIC_VERSION_2;
     } else if (!strcmp(value, "host")) {
-        vms->gic_version = 0; /* Will probe later */
+        vms->gic_version = VIRT_GIC_VERSION_HOST; /* Will probe later */
     } else if (!strcmp(value, "max")) {
-        vms->gic_version = -1; /* Will probe later */
+        vms->gic_version = VIRT_GIC_VERSION_MAX; /* Will probe later */
     } else {
         error_setg(errp, "Invalid gic-version value");
         error_append_hint(errp, "Valid values are 3, 2, host, max.\n");
@@ -2141,7 +2141,7 @@ static void virt_instance_init(Object *obj)
                                     "physical address space above 32 bits",
                                     NULL);
     /* Default GIC type is v2 */
-    vms->gic_version = 2;
+    vms->gic_version = VIRT_GIC_VERSION_2;
     object_property_add_str(obj, "gic-version", virt_get_gic_version,
                         virt_set_gic_version, NULL);
     object_property_set_description(obj, "gic-version",
-- 
2.20.1

[PATCH v4 3/6] hw/arm/virt: Introduce finalize_gic_version()

[Eric Auger]

Let's move the code which freezes which gic-version to
be applied in a dedicated function. We also now set by
default the VIRT_GIC_VERSION_NO_SET. This eventually
turns into the legacy v2 choice in the finalize() function.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Andrew Jones <drjones@redhat.com>

---

v2 -> v3:
- add NOTSEL value at the end of the new enum type
---
 include/hw/arm/virt.h |  1 +
 hw/arm/virt.c         | 54 ++++++++++++++++++++++++++-----------------
 2 files changed, 34 insertions(+), 21 deletions(-)

diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
index c0827cacdf..893796d3b0 100644
--- a/include/hw/arm/virt.h
+++ b/include/hw/arm/virt.h
@@ -100,6 +100,7 @@ typedef enum VirtGICType {
     VIRT_GIC_VERSION_HOST,
     VIRT_GIC_VERSION_2,
     VIRT_GIC_VERSION_3,
+    VIRT_GIC_VERSION_NOSEL,
 } VirtGICType;
 
 typedef struct MemMapEntry {
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 4ed1f0cb2e..22da0a7ad7 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1535,6 +1535,37 @@ static void virt_set_memmap(VirtMachineState *vms)
     }
 }
 
+/*
+ * finalize_gic_version - Determines the final gic_version
+ * according to the gic-version property
+ *
+ * Default GIC type is v2
+ */
+static void finalize_gic_version(VirtMachineState *vms)
+{
+    if (vms->gic_version == VIRT_GIC_VERSION_HOST ||
+        vms->gic_version == VIRT_GIC_VERSION_MAX) {
+        if (!kvm_enabled()) {
+            if (vms->gic_version == VIRT_GIC_VERSION_HOST) {
+                error_report("gic-version=host requires KVM");
+                exit(1);
+            } else {
+                /* "max": currently means 3 for TCG */
+                vms->gic_version = VIRT_GIC_VERSION_3;
+            }
+        } else {
+            vms->gic_version = kvm_arm_vgic_probe();
+            if (!vms->gic_version) {
+                error_report(
+                    "Unable to determine GIC version supported by host");
+                exit(1);
+            }
+        }
+    } else if (vms->gic_version == VIRT_GIC_VERSION_NOSEL) {
+        vms->gic_version = VIRT_GIC_VERSION_2;
+    }
+}
+
 static void machvirt_init(MachineState *machine)
 {
     VirtMachineState *vms = VIRT_MACHINE(machine);
@@ -1561,25 +1592,7 @@ static void machvirt_init(MachineState *machine)
     /* We can probe only here because during property set
      * KVM is not available yet
      */
-    if (vms->gic_version == VIRT_GIC_VERSION_HOST ||
-        vms->gic_version == VIRT_GIC_VERSION_MAX) {
-        if (!kvm_enabled()) {
-            if (vms->gic_version == VIRT_GIC_VERSION_HOST) {
-                error_report("gic-version=host requires KVM");
-                exit(1);
-            } else {
-                /* "max": currently means 3 for TCG */
-                vms->gic_version = VIRT_GIC_VERSION_3;
-            }
-        } else {
-            vms->gic_version = kvm_arm_vgic_probe();
-            if (!vms->gic_version) {
-                error_report(
-                    "Unable to determine GIC version supported by host");
-                exit(1);
-            }
-        }
-    }
+     finalize_gic_version(vms);
 
     if (!cpu_type_valid(machine->cpu_type)) {
         error_report("mach-virt: CPU type %s not supported", machine->cpu_type);
@@ -2140,8 +2153,7 @@ static void virt_instance_init(Object *obj)
                                     "Set on/off to enable/disable using "
                                     "physical address space above 32 bits",
                                     NULL);
-    /* Default GIC type is v2 */
-    vms->gic_version = VIRT_GIC_VERSION_2;
+    vms->gic_version = VIRT_GIC_VERSION_NOSEL;
     object_property_add_str(obj, "gic-version", virt_get_gic_version,
                         virt_set_gic_version, NULL);
     object_property_set_description(obj, "gic-version",
-- 
2.20.1

[PATCH v4 4/6] target/arm/kvm: Let kvm_arm_vgic_probe() return a bitmap

[Eric Auger]

Convert kvm_arm_vgic_probe() so that it returns a
bitmap of supported in-kernel emulation VGIC versions instead
of the max version: at the moment values can be v2 and v3.
This allows to expose the case where the host GICv3 also
supports GICv2 emulation. This will be useful to choose the
default version in KVM accelerated mode.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
Reviewed-by: Andrew Jones <drjones@redhat.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/kvm_arm.h |  3 +++
 hw/arm/virt.c        | 11 +++++++++--
 target/arm/kvm.c     | 14 ++++++++------
 3 files changed, 20 insertions(+), 8 deletions(-)

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
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 22da0a7ad7..f798b6ec10 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1554,11 +1554,18 @@ static void finalize_gic_version(VirtMachineState *vms)
                 vms->gic_version = VIRT_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 = VIRT_GIC_VERSION_3;
+                } else {
+                    vms->gic_version = VIRT_GIC_VERSION_2;
+                }
             }
         }
     } else if (vms->gic_version == VIRT_GIC_VERSION_NOSEL) {
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)
-- 
2.20.1

[PATCH v4 5/6] hw/arm/virt: kvm: Restructure finalize_gic_version()

[Eric Auger]

Restructure the finalize_gic_version with switch cases and
clearly separate the following cases:

- KVM mode / in-kernel irqchip
- KVM mode / userspace irqchip
- TCG mode

In KVM mode / in-kernel irqchip , we explictly check whether
the chosen version is supported by the host. If the end-user
explicitly sets v2/v3 and this is not supported by the host,
then the user gets an explicit error message. Note that for
old kernels where the CREATE_DEVICE ioctl doesn't exist then
we will now fail if the user specifically asked for gicv2,
where previously we (probably) would have succeeded.

In KVM mode / userspace irqchip we immediatly output an error
in case the end-user explicitly selected v3. Also we warn the
end-user about the unexpected usage of gic-version=host in
that case as only userspace GICv2 is supported.

Signed-off-by: Eric Auger <eric.auger@redhat.com>

---

v3 -> v4:
- introduce a separate switch case for KVM mode / userspace
  irqchip
- don't probe the host GIC version in case of userspace
  GIC
- add error/warning messages in v3/host mode in userspace
  mode
- mention old kernel case with explicit v2/v3 regression

v2 -> v3:
- explictly list V2 and V3 in the switch/case
- fix indent
---
 hw/arm/virt.c | 89 +++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 68 insertions(+), 21 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index f798b6ec10..f5ff2d9006 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1543,33 +1543,80 @@ static void virt_set_memmap(VirtMachineState *vms)
  */
 static void finalize_gic_version(VirtMachineState *vms)
 {
-    if (vms->gic_version == VIRT_GIC_VERSION_HOST ||
-        vms->gic_version == VIRT_GIC_VERSION_MAX) {
-        if (!kvm_enabled()) {
-            if (vms->gic_version == VIRT_GIC_VERSION_HOST) {
-                error_report("gic-version=host requires KVM");
-                exit(1);
-            } else {
-                /* "max": currently means 3 for TCG */
-                vms->gic_version = VIRT_GIC_VERSION_3;
-            }
-        } else {
-            int probe_bitmap = kvm_arm_vgic_probe();
+    if (kvm_enabled()) {
+        int probe_bitmap;
 
-            if (!probe_bitmap) {
+        if (!kvm_irqchip_in_kernel()) {
+            switch (vms->gic_version) {
+            case VIRT_GIC_VERSION_HOST:
+                warn_report(
+                    "gic-version=host not relevant with kernel-irqchip=off "
+                     "as only userspace GICv2 is supported. Using v2 ...");
+                return;
+            case VIRT_GIC_VERSION_MAX:
+            case VIRT_GIC_VERSION_NOSEL:
+                vms->gic_version = VIRT_GIC_VERSION_2;
+                return;
+            case VIRT_GIC_VERSION_2:
+                return;
+            case VIRT_GIC_VERSION_3:
                 error_report(
-                    "Unable to determine GIC version supported by host");
+                    "gic-version=3 is not supported with kernel-irqchip=off");
                 exit(1);
-            } else {
-                if (probe_bitmap & KVM_ARM_VGIC_V3) {
-                    vms->gic_version = VIRT_GIC_VERSION_3;
-                } else {
-                    vms->gic_version = VIRT_GIC_VERSION_2;
-                }
             }
         }
-    } else if (vms->gic_version == VIRT_GIC_VERSION_NOSEL) {
+
+        probe_bitmap = kvm_arm_vgic_probe();
+        if (!probe_bitmap) {
+            error_report(
+                "Unable to determine GIC version supported by host");
+            exit(1);
+        }
+
+        switch (vms->gic_version) {
+        case VIRT_GIC_VERSION_HOST:
+        case VIRT_GIC_VERSION_MAX:
+            if (probe_bitmap & KVM_ARM_VGIC_V3) {
+                vms->gic_version = VIRT_GIC_VERSION_3;
+            } else {
+                vms->gic_version = VIRT_GIC_VERSION_2;
+            }
+            return;
+        case VIRT_GIC_VERSION_NOSEL:
+            vms->gic_version = VIRT_GIC_VERSION_2;
+            break;
+        case VIRT_GIC_VERSION_2:
+        case VIRT_GIC_VERSION_3:
+            break;
+        }
+
+        /* Check chosen version is effectively supported by the host */
+        if (vms->gic_version == VIRT_GIC_VERSION_2 &&
+            !(probe_bitmap & KVM_ARM_VGIC_V2)) {
+            error_report("host does not support in-kernel GICv2 emulation");
+            exit(1);
+        } else if (vms->gic_version == VIRT_GIC_VERSION_3 &&
+                   !(probe_bitmap & KVM_ARM_VGIC_V3)) {
+            error_report("host does not support in-kernel GICv3 emulation");
+            exit(1);
+        }
+        return;
+    }
+
+    /* TCG mode */
+    switch (vms->gic_version) {
+    case VIRT_GIC_VERSION_NOSEL:
         vms->gic_version = VIRT_GIC_VERSION_2;
+        break;
+    case VIRT_GIC_VERSION_MAX:
+        vms->gic_version = VIRT_GIC_VERSION_3;
+        break;
+    case VIRT_GIC_VERSION_HOST:
+        error_report("gic-version=host requires KVM");
+        exit(1);
+    case VIRT_GIC_VERSION_2:
+    case VIRT_GIC_VERSION_3:
+        break;
     }
 }
 
-- 
2.20.1

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

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

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 <eric.auger@redhat.com>
Reported-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

---

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

Re: [PATCH v4 3/6] hw/arm/virt: Introduce finalize_gic_version()

[Andrew Jones]

On Wed, Mar 11, 2020 at 12:16:23PM +0100, Eric Auger wrote:
> Let's move the code which freezes which gic-version to
> be applied in a dedicated function. We also now set by
> default the VIRT_GIC_VERSION_NO_SET. This eventually
> turns into the legacy v2 choice in the finalize() function.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Reviewed-by: Andrew Jones <drjones@redhat.com>
> 
> ---
> 
> v2 -> v3:
> - add NOTSEL value at the end of the new enum type
> ---
>  include/hw/arm/virt.h |  1 +
>  hw/arm/virt.c         | 54 ++++++++++++++++++++++++++-----------------
>  2 files changed, 34 insertions(+), 21 deletions(-)
> 
> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> index c0827cacdf..893796d3b0 100644
> --- a/include/hw/arm/virt.h
> +++ b/include/hw/arm/virt.h
> @@ -100,6 +100,7 @@ typedef enum VirtGICType {
>      VIRT_GIC_VERSION_HOST,
>      VIRT_GIC_VERSION_2,
>      VIRT_GIC_VERSION_3,
> +    VIRT_GIC_VERSION_NOSEL,
>  } VirtGICType;
>  
>  typedef struct MemMapEntry {
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 4ed1f0cb2e..22da0a7ad7 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -1535,6 +1535,37 @@ static void virt_set_memmap(VirtMachineState *vms)
>      }
>  }
>  
> +/*
> + * finalize_gic_version - Determines the final gic_version
> + * according to the gic-version property
> + *
> + * Default GIC type is v2
> + */
> +static void finalize_gic_version(VirtMachineState *vms)
> +{
> +    if (vms->gic_version == VIRT_GIC_VERSION_HOST ||
> +        vms->gic_version == VIRT_GIC_VERSION_MAX) {
> +        if (!kvm_enabled()) {
> +            if (vms->gic_version == VIRT_GIC_VERSION_HOST) {
> +                error_report("gic-version=host requires KVM");
> +                exit(1);
> +            } else {
> +                /* "max": currently means 3 for TCG */
> +                vms->gic_version = VIRT_GIC_VERSION_3;
> +            }
> +        } else {
> +            vms->gic_version = kvm_arm_vgic_probe();
> +            if (!vms->gic_version) {
> +                error_report(
> +                    "Unable to determine GIC version supported by host");
> +                exit(1);
> +            }
> +        }
> +    } else if (vms->gic_version == VIRT_GIC_VERSION_NOSEL) {
> +        vms->gic_version = VIRT_GIC_VERSION_2;
> +    }
> +}
> +
>  static void machvirt_init(MachineState *machine)
>  {
>      VirtMachineState *vms = VIRT_MACHINE(machine);
> @@ -1561,25 +1592,7 @@ static void machvirt_init(MachineState *machine)
>      /* We can probe only here because during property set
>       * KVM is not available yet
>       */
> -    if (vms->gic_version == VIRT_GIC_VERSION_HOST ||
> -        vms->gic_version == VIRT_GIC_VERSION_MAX) {
> -        if (!kvm_enabled()) {
> -            if (vms->gic_version == VIRT_GIC_VERSION_HOST) {
> -                error_report("gic-version=host requires KVM");
> -                exit(1);
> -            } else {
> -                /* "max": currently means 3 for TCG */
> -                vms->gic_version = VIRT_GIC_VERSION_3;
> -            }
> -        } else {
> -            vms->gic_version = kvm_arm_vgic_probe();
> -            if (!vms->gic_version) {
> -                error_report(
> -                    "Unable to determine GIC version supported by host");
> -                exit(1);
> -            }
> -        }
> -    }
> +     finalize_gic_version(vms);
       ^ extra space
>  
>      if (!cpu_type_valid(machine->cpu_type)) {
>          error_report("mach-virt: CPU type %s not supported", machine->cpu_type);
> @@ -2140,8 +2153,7 @@ static void virt_instance_init(Object *obj)
>                                      "Set on/off to enable/disable using "
>                                      "physical address space above 32 bits",
>                                      NULL);
> -    /* Default GIC type is v2 */
> -    vms->gic_version = VIRT_GIC_VERSION_2;
> +    vms->gic_version = VIRT_GIC_VERSION_NOSEL;
>      object_property_add_str(obj, "gic-version", virt_get_gic_version,
>                          virt_set_gic_version, NULL);
>      object_property_set_description(obj, "gic-version",
> -- 
> 2.20.1
> 

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 <eric.auger@redhat.com>
> Reported-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> 
> ---
> 
> 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 <drjones@redhat.com>

Re: [PATCH v4 5/6] hw/arm/virt: kvm: Restructure finalize_gic_version()

[Andrew Jones]

On Wed, Mar 11, 2020 at 12:16:25PM +0100, Eric Auger wrote:
> Restructure the finalize_gic_version with switch cases and
> clearly separate the following cases:
> 
> - KVM mode / in-kernel irqchip
> - KVM mode / userspace irqchip
> - TCG mode
> 
> In KVM mode / in-kernel irqchip , we explictly check whether
> the chosen version is supported by the host. If the end-user
> explicitly sets v2/v3 and this is not supported by the host,
> then the user gets an explicit error message. Note that for
> old kernels where the CREATE_DEVICE ioctl doesn't exist then
> we will now fail if the user specifically asked for gicv2,
> where previously we (probably) would have succeeded.
> 
> In KVM mode / userspace irqchip we immediatly output an error
> in case the end-user explicitly selected v3. Also we warn the
> end-user about the unexpected usage of gic-version=host in
> that case as only userspace GICv2 is supported.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> 
> ---
> 
> v3 -> v4:
> - introduce a separate switch case for KVM mode / userspace
>   irqchip
> - don't probe the host GIC version in case of userspace
>   GIC
> - add error/warning messages in v3/host mode in userspace
>   mode
> - mention old kernel case with explicit v2/v3 regression
> 
> v2 -> v3:
> - explictly list V2 and V3 in the switch/case
> - fix indent
> ---
>  hw/arm/virt.c | 89 +++++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 68 insertions(+), 21 deletions(-)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index f798b6ec10..f5ff2d9006 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -1543,33 +1543,80 @@ static void virt_set_memmap(VirtMachineState *vms)
>   */
>  static void finalize_gic_version(VirtMachineState *vms)
>  {
> -    if (vms->gic_version == VIRT_GIC_VERSION_HOST ||
> -        vms->gic_version == VIRT_GIC_VERSION_MAX) {
> -        if (!kvm_enabled()) {
> -            if (vms->gic_version == VIRT_GIC_VERSION_HOST) {
> -                error_report("gic-version=host requires KVM");
> -                exit(1);
> -            } else {
> -                /* "max": currently means 3 for TCG */
> -                vms->gic_version = VIRT_GIC_VERSION_3;
> -            }
> -        } else {
> -            int probe_bitmap = kvm_arm_vgic_probe();
> +    if (kvm_enabled()) {
> +        int probe_bitmap;
>  
> -            if (!probe_bitmap) {
> +        if (!kvm_irqchip_in_kernel()) {
> +            switch (vms->gic_version) {
> +            case VIRT_GIC_VERSION_HOST:
> +                warn_report(
> +                    "gic-version=host not relevant with kernel-irqchip=off "
> +                     "as only userspace GICv2 is supported. Using v2 ...");
> +                return;
> +            case VIRT_GIC_VERSION_MAX:
> +            case VIRT_GIC_VERSION_NOSEL:
> +                vms->gic_version = VIRT_GIC_VERSION_2;
> +                return;
> +            case VIRT_GIC_VERSION_2:
> +                return;
> +            case VIRT_GIC_VERSION_3:
>                  error_report(
> -                    "Unable to determine GIC version supported by host");
> +                    "gic-version=3 is not supported with kernel-irqchip=off");
>                  exit(1);
> -            } else {
> -                if (probe_bitmap & KVM_ARM_VGIC_V3) {
> -                    vms->gic_version = VIRT_GIC_VERSION_3;
> -                } else {
> -                    vms->gic_version = VIRT_GIC_VERSION_2;
> -                }
>              }
>          }
> -    } else if (vms->gic_version == VIRT_GIC_VERSION_NOSEL) {
> +
> +        probe_bitmap = kvm_arm_vgic_probe();
> +        if (!probe_bitmap) {
> +            error_report(
> +                "Unable to determine GIC version supported by host");

No need for this line break

> +            exit(1);
> +        }
> +
> +        switch (vms->gic_version) {
> +        case VIRT_GIC_VERSION_HOST:
> +        case VIRT_GIC_VERSION_MAX:
> +            if (probe_bitmap & KVM_ARM_VGIC_V3) {
> +                vms->gic_version = VIRT_GIC_VERSION_3;
> +            } else {
> +                vms->gic_version = VIRT_GIC_VERSION_2;
> +            }
> +            return;
> +        case VIRT_GIC_VERSION_NOSEL:
> +            vms->gic_version = VIRT_GIC_VERSION_2;
> +            break;
> +        case VIRT_GIC_VERSION_2:
> +        case VIRT_GIC_VERSION_3:
> +            break;
> +        }
> +
> +        /* Check chosen version is effectively supported by the host */
> +        if (vms->gic_version == VIRT_GIC_VERSION_2 &&
> +            !(probe_bitmap & KVM_ARM_VGIC_V2)) {
> +            error_report("host does not support in-kernel GICv2 emulation");
> +            exit(1);
> +        } else if (vms->gic_version == VIRT_GIC_VERSION_3 &&
> +                   !(probe_bitmap & KVM_ARM_VGIC_V3)) {
> +            error_report("host does not support in-kernel GICv3 emulation");
> +            exit(1);
> +        }
> +        return;
> +    }
> +
> +    /* TCG mode */
> +    switch (vms->gic_version) {
> +    case VIRT_GIC_VERSION_NOSEL:
>          vms->gic_version = VIRT_GIC_VERSION_2;
> +        break;
> +    case VIRT_GIC_VERSION_MAX:
> +        vms->gic_version = VIRT_GIC_VERSION_3;
> +        break;
> +    case VIRT_GIC_VERSION_HOST:
> +        error_report("gic-version=host requires KVM");
> +        exit(1);
> +    case VIRT_GIC_VERSION_2:
> +    case VIRT_GIC_VERSION_3:
> +        break;
>      }
>  }
>  
> -- 
> 2.20.1
>

Other than the nit,

Reviewed-by: Andrew Jones <drjones@redhat.com>