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

[PATCH v3 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-v3

History:

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 -> v2:
- 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: Check the chosen gic version is supported by the
    host
  hw/arm/virt: kvm: allow gicv3 by default if v2 cannot work

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

-- 
2.20.1

[PATCH v3 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 856808599d..c093f0ab85 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -2144,7 +2144,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 v3 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>

---

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

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index c093f0ab85..b449a445de 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -298,7 +298,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);
@@ -439,7 +439,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",
@@ -518,7 +518,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);
@@ -1469,7 +1469,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;
@@ -1560,15 +1560,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();
@@ -1627,7 +1627,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 +=
@@ -1855,7 +1855,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);
 }
@@ -1865,13 +1865,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");
@@ -2139,7 +2139,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",
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;
 }
-- 
2.20.1

[PATCH v3 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
---
 hw/arm/virt.c         | 54 ++++++++++++++++++++++++++-----------------
 include/hw/arm/virt.h |  1 +
 2 files changed, 34 insertions(+), 21 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index b449a445de..338d56999f 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1534,6 +1534,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);
@@ -1560,25 +1591,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);
@@ -2138,8 +2151,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",
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 {
-- 
2.20.1

[PATCH v3 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>
---
 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 338d56999f..eb8c57c85e 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1553,11 +1553,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)
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

[PATCH v3 5/6] hw/arm/virt: kvm: Check the chosen gic version is supported by the host

[Eric Auger]

Restructure the finalize_gic_version with switch cases and, in
KVM mode, 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.

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:
- explictly list V2 and V3 in the switch/case
- fix indent
---
 hw/arm/virt.c | 77 +++++++++++++++++++++++++++++++++++----------------
 1 file changed, 53 insertions(+), 24 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index eb8c57c85e..aeb6c45e51 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1542,33 +1542,62 @@ 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 = 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;
-                }
-            }
+        if (!probe_bitmap) {
+            error_report("Unable to determine GIC version supported by host");
+            exit(1);
         }
-    } else if (vms->gic_version == VIRT_GIC_VERSION_NOSEL) {
+
+        switch (vms->gic_version) {
+        case VIRT_GIC_VERSION_NOSEL:
+            vms->gic_version = VIRT_GIC_VERSION_2;
+            break;
+        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_2:
+        case VIRT_GIC_VERSION_3:
+            break;
+        }
+
+        if (!kvm_irqchip_in_kernel()) {
+            return;
+        }
+
+        /* 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 v3 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>
Reviewed-by: Andrew Jones <drjones@redhat.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

---

v2 -> v3:
- add ()
---
 hw/arm/virt.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index aeb6c45e51..2d1dee50f8 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1542,6 +1542,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 = kvm_arm_vgic_probe();
 
@@ -1552,7 +1554,17 @@ static void finalize_gic_version(VirtMachineState *vms)
 
         switch (vms->gic_version) {
         case VIRT_GIC_VERSION_NOSEL:
-            vms->gic_version = VIRT_GIC_VERSION_2;
+            if (((probe_bitmap & KVM_ARM_VGIC_V2) && max_cpus <= GIC_NCPU) ||
+                !kvm_irqchip_in_kernel()) {
+                vms->gic_version = VIRT_GIC_VERSION_2;
+            } else {
+                /*
+                 * 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;
+            }
             break;
         case VIRT_GIC_VERSION_HOST:
         case VIRT_GIC_VERSION_MAX:
-- 
2.20.1

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

[Philippe Mathieu-Daudé]

On 3/2/20 11:55 AM, Eric Auger wrote:
> 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>
> 
> ---
> 
> v2 -> v3:
> - replaced defines by enum VirtGICType
> - use that new type for vms->gic_version
> ---
>   hw/arm/virt.c         | 30 +++++++++++++++---------------
>   include/hw/arm/virt.h | 11 +++++++++--

Please have a look at scripts/git.orderfile, it helps making review 
easier/quicker.

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

>   2 files changed, 24 insertions(+), 17 deletions(-)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index c093f0ab85..b449a445de 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -298,7 +298,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);
> @@ -439,7 +439,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",
> @@ -518,7 +518,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);
> @@ -1469,7 +1469,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;
> @@ -1560,15 +1560,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();
> @@ -1627,7 +1627,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 +=
> @@ -1855,7 +1855,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);
>   }
> @@ -1865,13 +1865,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");
> @@ -2139,7 +2139,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",
> 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;
>   }
> 

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

[Andrew Jones]

On Mon, Mar 02, 2020 at 11:55:12AM +0100, Eric Auger wrote:
> 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>
> 
> ---
> 
> v2 -> v3:
> - replaced defines by enum VirtGICType
> - use that new type for vms->gic_version
> ---
>  hw/arm/virt.c         | 30 +++++++++++++++---------------
>  include/hw/arm/virt.h | 11 +++++++++--
>  2 files changed, 24 insertions(+), 17 deletions(-)
>

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

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

[Auger Eric]

Hi Philippe,

On 3/2/20 12:18 PM, Philippe Mathieu-Daudé wrote:
> On 3/2/20 11:55 AM, Eric Auger wrote:
>> 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>
>>
>> ---
>>
>> v2 -> v3:
>> - replaced defines by enum VirtGICType
>> - use that new type for vms->gic_version
>> ---
>>   hw/arm/virt.c         | 30 +++++++++++++++---------------
>>   include/hw/arm/virt.h | 11 +++++++++--
> 
> Please have a look at scripts/git.orderfile, it helps making review
> easier/quicker.
OK I added it to my .git/config. Thanks for pointer
> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Thanks!

Eric
> 
>>   2 files changed, 24 insertions(+), 17 deletions(-)
>>
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index c093f0ab85..b449a445de 100644
>> --- a/hw/arm/virt.c
>> +++ b/hw/arm/virt.c
>> @@ -298,7 +298,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);
>> @@ -439,7 +439,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",
>> @@ -518,7 +518,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);
>> @@ -1469,7 +1469,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;
>> @@ -1560,15 +1560,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();
>> @@ -1627,7 +1627,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 +=
>> @@ -1855,7 +1855,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);
>>   }
>> @@ -1865,13 +1865,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");
>> @@ -2139,7 +2139,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",
>> 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;
>>   }
>>
> 
> 

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

[Richard Henderson]

On 3/2/20 2:55 AM, Eric Auger wrote:
> 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>
> 
> ---
> 
> v2 -> v3:
> - replaced defines by enum VirtGICType
> - use that new type for vms->gic_version
> ---
>  hw/arm/virt.c         | 30 +++++++++++++++---------------
>  include/hw/arm/virt.h | 11 +++++++++--
>  2 files changed, 24 insertions(+), 17 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH v3 5/6] hw/arm/virt: kvm: Check the chosen gic version is supported by the host

[Peter Maydell]

On Mon, 2 Mar 2020 at 10:55, Eric Auger <eric.auger@redhat.com> wrote:
>
> Restructure the finalize_gic_version with switch cases and, in
> KVM mode, 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.
>
> 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:
> - explictly list V2 and V3 in the switch/case
> - fix indent
> ---
>  hw/arm/virt.c | 77 +++++++++++++++++++++++++++++++++++----------------
>  1 file changed, 53 insertions(+), 24 deletions(-)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index eb8c57c85e..aeb6c45e51 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -1542,33 +1542,62 @@ 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 = kvm_arm_vgic_probe();

Previously we would only do kvm_arm_vgic_probe() if the
user asked for 'host' or 'max'. Now we do it always,
which means that if the user is on a really old kernel
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.
I don't think we should put too much weight on continuing
to theoretically support ancient kernels which we're not
actually testing against, but it does seem a bit odd to
probe even if we don't need to know the answer.

More relevant to actual plausible use cases, if
kvm_irqchip_in_kernel() == false, we shouldn't be
probing the kernel to ask what kind of GIC to use.

thanks
-- PMM

Re: [PATCH v3 5/6] hw/arm/virt: kvm: Check the chosen gic version is supported by the host

[Auger Eric]

Hi Peter,

On 3/9/20 2:28 PM, Peter Maydell wrote:
> On Mon, 2 Mar 2020 at 10:55, Eric Auger <eric.auger@redhat.com> wrote:
>>
>> Restructure the finalize_gic_version with switch cases and, in
>> KVM mode, 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.
>>
>> 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:
>> - explictly list V2 and V3 in the switch/case
>> - fix indent
>> ---
>>  hw/arm/virt.c | 77 +++++++++++++++++++++++++++++++++++----------------
>>  1 file changed, 53 insertions(+), 24 deletions(-)
>>
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index eb8c57c85e..aeb6c45e51 100644
>> --- a/hw/arm/virt.c
>> +++ b/hw/arm/virt.c
>> @@ -1542,33 +1542,62 @@ 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 = kvm_arm_vgic_probe();
> 
> Previously we would only do kvm_arm_vgic_probe() if the
> user asked for 'host' or 'max'. Now we do it always,
> which means that if the user is on a really old kernel
> 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.
> I don't think we should put too much weight on continuing
> to theoretically support ancient kernels which we're not
> actually testing against, but it does seem a bit odd to
> probe even if we don't need to know the answer.
> 
> More relevant to actual plausible use cases, if
> kvm_irqchip_in_kernel() == false, we shouldn't be
> probing the kernel to ask what kind of GIC to use.

OK I will fix that case. I understand that the former point is not critical.

Thank you for the feedback.

Eric
> 
> thanks
> -- PMM
> 

Re: [PATCH v3 5/6] hw/arm/virt: kvm: Check the chosen gic version is supported by the host

[Auger Eric]

Hi Peter,

On 3/9/20 2:28 PM, Peter Maydell wrote:
> On Mon, 2 Mar 2020 at 10:55, Eric Auger <eric.auger@redhat.com> wrote:
>>
>> Restructure the finalize_gic_version with switch cases and, in
>> KVM mode, 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.
>>
>> 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:
>> - explictly list V2 and V3 in the switch/case
>> - fix indent
>> ---
>>  hw/arm/virt.c | 77 +++++++++++++++++++++++++++++++++++----------------
>>  1 file changed, 53 insertions(+), 24 deletions(-)
>>
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index eb8c57c85e..aeb6c45e51 100644
>> --- a/hw/arm/virt.c
>> +++ b/hw/arm/virt.c
>> @@ -1542,33 +1542,62 @@ 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 = kvm_arm_vgic_probe();
> 
> Previously we would only do kvm_arm_vgic_probe() if the
> user asked for 'host' or 'max'. Now we do it always,
> which means that if the user is on a really old kernel
> 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.
> I don't think we should put too much weight on continuing
> to theoretically support ancient kernels which we're not
> actually testing against, but it does seem a bit odd to
> probe even if we don't need to know the answer.
> 
> More relevant to actual plausible use cases, if
> kvm_irqchip_in_kernel() == false, we shouldn't be
> probing the kernel to ask what kind of GIC to use.
I think the existing code also does the same:
kvm_arm_vgic_probe() gets called as soon as vms->gic_version <= 0 &&
kvm_enabled() whatever the state of kvm_irqchip_in_kernel().

So in case the host only supports GICv2, in kvm mode with userspace
irqchip we would use GICV2 in host/max mode. If host supports GICv3 we
would select GICv3 which is not supported in !kvm_irqchip_in_kernel().

So do I understand correctly that you want me to change that behavior
and always set v2 in KVM/!kvm_irqchip_in_kernel() max/host mode?

Thanks

Eric

> 
> thanks
> -- PMM
> 

Re: [PATCH v3 5/6] hw/arm/virt: kvm: Check the chosen gic version is supported by the host

[Peter Maydell]

On Tue, 10 Mar 2020 at 18:02, Auger Eric <eric.auger@redhat.com> wrote:
>
> Hi Peter,
>
> On 3/9/20 2:28 PM, Peter Maydell wrote:
> > On Mon, 2 Mar 2020 at 10:55, Eric Auger <eric.auger@redhat.com> wrote:
> >>
> >> Restructure the finalize_gic_version with switch cases and, in
> >> KVM mode, 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.
> >>
> >> 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:
> >> - explictly list V2 and V3 in the switch/case
> >> - fix indent
> >> ---
> >>  hw/arm/virt.c | 77 +++++++++++++++++++++++++++++++++++----------------
> >>  1 file changed, 53 insertions(+), 24 deletions(-)
> >>
> >> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> >> index eb8c57c85e..aeb6c45e51 100644
> >> --- a/hw/arm/virt.c
> >> +++ b/hw/arm/virt.c
> >> @@ -1542,33 +1542,62 @@ 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 = kvm_arm_vgic_probe();
> >
> > Previously we would only do kvm_arm_vgic_probe() if the
> > user asked for 'host' or 'max'. Now we do it always,
> > which means that if the user is on a really old kernel
> > 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.
> > I don't think we should put too much weight on continuing
> > to theoretically support ancient kernels which we're not
> > actually testing against, but it does seem a bit odd to
> > probe even if we don't need to know the answer.
> >
> > More relevant to actual plausible use cases, if
> > kvm_irqchip_in_kernel() == false, we shouldn't be
> > probing the kernel to ask what kind of GIC to use.
> I think the existing code also does the same:
> kvm_arm_vgic_probe() gets called as soon as vms->gic_version <= 0 &&
> kvm_enabled() whatever the state of kvm_irqchip_in_kernel().

Yes, but your change here makes it call kvm_arm_vgic_probe()
even if the gic_version was explicitly set to 2 or 3
by the user, doesn't it ? That's what I was commenting on.

> So in case the host only supports GICv2, in kvm mode with userspace
> irqchip we would use GICV2 in host/max mode. If host supports GICv3 we
> would select GICv3 which is not supported in !kvm_irqchip_in_kernel().

> So do I understand correctly that you want me to change that behavior
> and always set v2 in KVM/!kvm_irqchip_in_kernel() max/host mode?

I think:
(1) we should retain the current behaviour that if the user
asked for userspace-irqchip and specifically chose gic
version 2, then we don't probe the kernel for its capabilities,
we just create a userspace gicv2.

(2) we should also retain the current behaviour that we
default to "2" if the user requests userspace-irqchip but
doesn't specify the gic-version.

(3) the ideal-world behaviour would be that we correctly
handle the user asking for userspace-irqchip plus either "max"
(choose '2') or "3" (produce a useful error message if we
don't do so already). I'm not sure what "host" should mean
here. But this point (3) is separate from what this series is
doing, I think, and is basically fixing the bug that we didn't
think about the userspace-irqchip case when we implemented
"host" and "max" or when we added GICv3 support.

thanks
-- PMM