[Qemu-devel] [PATCH v8 0/5] vGICv3 support

[Qemu-devel] [PATCH v8 0/5] vGICv3 support

[Pavel Fedin]

This series introduces support for GICv3 by KVM. Software emulation is
currently not supported.

This patchset applies on top of:
http://lists.nongnu.org/archive/html/qemu-devel/2015-08/msg00518.html

v7 => v8
- Removed all unused SW emulation code
- Removed unnecessary attributes from common class
- Set "non-migratable" flag for GICv3 device
- Removed unnecessary conditions from kvm_arm_gicv3_realize()
- Fixed GIC type setting in vexpress model, was done in wrong place
- Cleaned up virt machine memory map

v6 => v7
- Wrap own GIC type definitions on top of KVM ones. Fixed build on
  non-ARM-Linux hosts

v5 => v6
- Fixed various checkpatch.pl style warnings
- Removed TODO in gicv3_init_irqs_and_mmio(), relevant memory API patch
  included
- gicv3_init_irqs_and_mmio() now takes 3 arguments instead of 4. It is more
  convenient to pass MMIO descriptors as array

v4 => v5
- Do not reintroduce several constants shared with GICv2, reuse them instead.
- Added gicv3_init_irqs_and_mmio() in base class, to be used by both software
  emulation and KVM code. Avoids code duplication.
- Do not add NULL msi-parent phandle to PCI device in the FDT
- Removed a couple of stale things from virt.c

v3 => v4
- Fixed stupid build breakage in patch 0002
- Rebased on top of current master, patch 0003 adjusted according to
  kvm_irqchip_create() changes
- Added assertion against uninitialized kernel_irqchip_type
- Removed kernel_irqchip_type initialization from models which do not
  use KVM vGIC

v2 => v3
- Removed some unrelated and unnecessary changes from virt machine,
  occasionally slipped in; some of them caused qemu to crash on ARM32.
- Fixed build for ARM32; vGICv3 code requires definitions which are
  present only in ARM64 kernel

v1 => v2
- Base class included, taken from the series by Shlomo Pongratz:
  http://lists.nongnu.org/archive/html/qemu-devel/2015-06/msg01512.html
  The code is refactored as little as possible in order to simplify
  further addition of software emulation:
  - Minor fixes in code style and comments, according to old reviews
  - Removed REV_V3 definition because it's currently not used, and it does
    not add any meaning to number 3.
  - Removed reserved regions for MBI and ITS (except for 'virt' machine
    memory map). These should go to separate classes when implemented.
- Improved commit messages
- vGIC patches restructured
- Use 'gicversion' option instead of virt-v3 machine

Pavel Fedin (4):
  Extract some reusable vGIC code
  Introduce irqchip type specification for KVM
  Initial implementation of vGICv3
  Add gicversion option to virt machine

Shlomo Pongratz (1):
  Implement GIC-500 base class

 hw/arm/vexpress.c                  |   3 +
 hw/arm/virt.c                      | 135 +++++++++++++++++++++++++++++-----
 hw/intc/Makefile.objs              |   2 +
 hw/intc/arm_gic_kvm.c              |  84 ++++++++++-----------
 hw/intc/arm_gicv3_common.c         | 147 +++++++++++++++++++++++++++++++++++++
 hw/intc/arm_gicv3_kvm.c            | 143 ++++++++++++++++++++++++++++++++++++
 hw/intc/vgic_common.h              |  43 +++++++++++
 include/hw/arm/fdt.h               |   2 +-
 include/hw/arm/virt.h              |   5 +-
 include/hw/boards.h                |   1 +
 include/hw/intc/arm_gicv3_common.h |  70 ++++++++++++++++++
 include/sysemu/kvm.h               |   3 +-
 kvm-all.c                          |   2 +-
 stubs/kvm.c                        |   2 +-
 target-arm/kvm-consts.h            |   6 ++
 target-arm/kvm.c                   |  13 +++-
 16 files changed, 590 insertions(+), 71 deletions(-)
 create mode 100644 hw/intc/arm_gicv3_common.c
 create mode 100644 hw/intc/arm_gicv3_kvm.c
 create mode 100644 hw/intc/vgic_common.h
 create mode 100644 include/hw/intc/arm_gicv3_common.h

-- 
1.9.5.msysgit.0

[Qemu-devel] [PATCH v8 1/5] Implement GIC-500 base class

[Pavel Fedin]

From: Shlomo Pongratz <shlomo.pongratz@huawei.com>

This class is to be used by both software and KVM implementations of GICv3

Currently it is mostly a placeholder, but in future it is supposed to hold
qemu's representation of GICv3 state, which is necessary for migration.

Signed-off-by: Shlomo Pongratz <shlomo.pongratz@huawei.com>
Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
---
 hw/intc/Makefile.objs              |   1 +
 hw/intc/arm_gicv3_common.c         | 147 +++++++++++++++++++++++++++++++++++++
 include/hw/intc/arm_gicv3_common.h |  70 ++++++++++++++++++
 3 files changed, 218 insertions(+)
 create mode 100644 hw/intc/arm_gicv3_common.c
 create mode 100644 include/hw/intc/arm_gicv3_common.h

diff --git a/hw/intc/Makefile.objs b/hw/intc/Makefile.objs
index 092d8a8..1317e5a 100644
--- a/hw/intc/Makefile.objs
+++ b/hw/intc/Makefile.objs
@@ -12,6 +12,7 @@ common-obj-$(CONFIG_IOAPIC) += ioapic_common.o
 common-obj-$(CONFIG_ARM_GIC) += arm_gic_common.o
 common-obj-$(CONFIG_ARM_GIC) += arm_gic.o
 common-obj-$(CONFIG_ARM_GIC) += arm_gicv2m.o
+common-obj-$(CONFIG_ARM_GIC) += arm_gicv3_common.o
 common-obj-$(CONFIG_OPENPIC) += openpic.o
 
 obj-$(CONFIG_APIC) += apic.o apic_common.o
diff --git a/hw/intc/arm_gicv3_common.c b/hw/intc/arm_gicv3_common.c
new file mode 100644
index 0000000..4e19e5c
--- /dev/null
+++ b/hw/intc/arm_gicv3_common.c
@@ -0,0 +1,147 @@
+/*
+ * ARM GICv3 support - common bits of emulated and KVM kernel model
+ *
+ * Copyright (c) 2012 Linaro Limited
+ * Copyright (c) 2015 Huawei.
+ * Written by Peter Maydell
+ * Extended to 64 cores by Shlomo Pongratz
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include "hw/intc/arm_gicv3_common.h"
+
+static void gicv3_pre_save(void *opaque)
+{
+    GICv3State *s = (GICv3State *)opaque;
+    ARMGICv3CommonClass *c = ARM_GICV3_COMMON_GET_CLASS(s);
+
+    if (c->pre_save) {
+        c->pre_save(s);
+    }
+}
+
+static int gicv3_post_load(void *opaque, int version_id)
+{
+    GICv3State *s = (GICv3State *)opaque;
+    ARMGICv3CommonClass *c = ARM_GICV3_COMMON_GET_CLASS(s);
+
+    if (c->post_load) {
+        c->post_load(s);
+    }
+    return 0;
+}
+
+static const VMStateDescription vmstate_gicv3 = {
+    .name = "arm_gicv3",
+    .unmigratable = 1,
+    .pre_save = gicv3_pre_save,
+    .post_load = gicv3_post_load,
+};
+
+void gicv3_init_irqs_and_mmio(GICv3State *s, qemu_irq_handler handler,
+                              const MemoryRegionOps *ops)
+{
+    SysBusDevice *sbd = SYS_BUS_DEVICE(s);
+    int i;
+
+    /* For the GIC, also expose incoming GPIO lines for PPIs for each CPU.
+     * GPIO array layout is thus:
+     *  [0..N-1] spi
+     *  [N..N+31] PPIs for CPU 0
+     *  [N+32..N+63] PPIs for CPU 1
+     *   ...
+     */
+    i = s->num_irq - GIC_INTERNAL + GIC_INTERNAL * s->num_cpu;
+    qdev_init_gpio_in(DEVICE(s), handler, i);
+
+    for (i = 0; i < s->num_cpu; i++) {
+        sysbus_init_irq(sbd, &s->parent_irq[i]);
+    }
+    for (i = 0; i < s->num_cpu; i++) {
+        sysbus_init_irq(sbd, &s->parent_fiq[i]);
+    }
+
+    memory_region_init_io(&s->iomem_dist, OBJECT(s), ops, s,
+                          "gicv3_dist", 0x10000);
+    memory_region_init_io(&s->iomem_redist, OBJECT(s), ops ? &ops[1] : NULL, s,
+                          "gicv3_redist", 0x800000);
+
+    sysbus_init_mmio(sbd, &s->iomem_dist);
+    sysbus_init_mmio(sbd, &s->iomem_redist);
+}
+
+static void arm_gicv3_common_realize(DeviceState *dev, Error **errp)
+{
+    GICv3State *s = ARM_GICV3_COMMON(dev);
+
+    if (s->num_cpu > GICV3_NCPU) {
+        error_setg(errp, "requested %u CPUs exceeds GIC maximum %d",
+                   s->num_cpu, GICV3_NCPU);
+        return;
+    }
+    if (s->num_irq > GICV3_MAXIRQ) {
+        error_setg(errp,
+                   "requested %u interrupt lines exceeds GIC maximum %d",
+                   s->num_irq, GICV3_MAXIRQ);
+        return;
+    }
+    /* ITLinesNumber is represented as (N / 32) - 1 (see
+     * gic_dist_readb) so this is an implementation imposed
+     * restriction, not an architectural one:
+     */
+    if (s->num_irq < 32 || (s->num_irq % 32)) {
+        error_setg(errp,
+                   "%d interrupt lines unsupported: not divisible by 32",
+                   s->num_irq);
+        return;
+    }
+}
+
+static void arm_gicv3_common_reset(DeviceState *dev)
+{
+    /* TODO */
+}
+
+static Property arm_gicv3_common_properties[] = {
+    DEFINE_PROP_UINT32("num-cpu", GICv3State, num_cpu, 1),
+    DEFINE_PROP_UINT32("num-irq", GICv3State, num_irq, 32),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void arm_gicv3_common_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->reset = arm_gicv3_common_reset;
+    dc->realize = arm_gicv3_common_realize;
+    dc->props = arm_gicv3_common_properties;
+    dc->vmsd = &vmstate_gicv3;
+}
+
+static const TypeInfo arm_gicv3_common_type = {
+    .name = TYPE_ARM_GICV3_COMMON,
+    .parent = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(GICv3State),
+    .class_size = sizeof(ARMGICv3CommonClass),
+    .class_init = arm_gicv3_common_class_init,
+    .abstract = true,
+};
+
+static void register_types(void)
+{
+    type_register_static(&arm_gicv3_common_type);
+}
+
+type_init(register_types)
diff --git a/include/hw/intc/arm_gicv3_common.h b/include/hw/intc/arm_gicv3_common.h
new file mode 100644
index 0000000..c47cba5
--- /dev/null
+++ b/include/hw/intc/arm_gicv3_common.h
@@ -0,0 +1,70 @@
+/*
+ * ARM GIC support
+ *
+ * Copyright (c) 2012 Linaro Limited
+ * Copyright (c) 2015 Huawei.
+ * Written by Peter Maydell
+ * Extended to 64 cores by Shlomo Pongratz
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef HW_ARM_GICV3_COMMON_H
+#define HW_ARM_GICV3_COMMON_H
+
+#include "hw/sysbus.h"
+#include "hw/intc/arm_gic_common.h"
+
+/* Maximum number of possible interrupts, determined by the GIC architecture */
+#define GICV3_MAXIRQ 1020
+#define GICV3_NCPU 64
+
+typedef struct GICv3State {
+    /*< private >*/
+    SysBusDevice parent_obj;
+    /*< public >*/
+
+    qemu_irq parent_irq[GICV3_NCPU];
+    qemu_irq parent_fiq[GICV3_NCPU];
+
+    MemoryRegion iomem_dist; /* Distributor */
+    MemoryRegion iomem_redist; /* Redistributor */
+
+    uint32_t num_cpu;
+    uint32_t num_irq;
+
+    int dev_fd; /* kvm device fd if backed by kvm vgic support */
+} GICv3State;
+
+#define TYPE_ARM_GICV3_COMMON "arm_gicv3_common"
+#define ARM_GICV3_COMMON(obj) \
+     OBJECT_CHECK(GICv3State, (obj), TYPE_ARM_GICV3_COMMON)
+#define ARM_GICV3_COMMON_CLASS(klass) \
+     OBJECT_CLASS_CHECK(ARMGICv3CommonClass, (klass), TYPE_ARM_GICV3_COMMON)
+#define ARM_GICV3_COMMON_GET_CLASS(obj) \
+     OBJECT_GET_CLASS(ARMGICv3CommonClass, (obj), TYPE_ARM_GICV3_COMMON)
+
+typedef struct ARMGICv3CommonClass {
+    /*< private >*/
+    SysBusDeviceClass parent_class;
+    /*< public >*/
+
+    void (*pre_save)(GICv3State *s);
+    void (*post_load)(GICv3State *s);
+} ARMGICv3CommonClass;
+
+void gicv3_init_irqs_and_mmio(GICv3State *s, qemu_irq_handler handler,
+                              const MemoryRegionOps *ops);
+
+#endif
-- 
1.9.5.msysgit.0

[Qemu-devel] [PATCH v8 2/5] Extract some reusable vGIC code

[Pavel Fedin]

These functions are useful also for vGICv3 implementation. Make them accessible
from within other modules.

Actually kvm_dist_get() and kvm_dist_put() could also be made reusable, but
they would require two extra parameters (s->dev_fd and s->num_cpu) as well as
lots of typecasts of 's' to DeviceState * and back to GICState *. This makes
the code very ugly so i decided to stop at this point. I tried also an
approach with making a base class for all possible GICs, but it would contain
only three variables (dev_fd, cpu_num and irq_num), and accessing them through
the rest of the code would be again tedious (either ugly casts or qemu-style
separate object pointer). So i disliked it too.

Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
---
 hw/intc/arm_gic_kvm.c | 84 ++++++++++++++++++++++++---------------------------
 hw/intc/vgic_common.h | 43 ++++++++++++++++++++++++++
 2 files changed, 82 insertions(+), 45 deletions(-)
 create mode 100644 hw/intc/vgic_common.h

diff --git a/hw/intc/arm_gic_kvm.c b/hw/intc/arm_gic_kvm.c
index e5d0f67..6ce539b 100644
--- a/hw/intc/arm_gic_kvm.c
+++ b/hw/intc/arm_gic_kvm.c
@@ -23,6 +23,7 @@
 #include "sysemu/kvm.h"
 #include "kvm_arm.h"
 #include "gic_internal.h"
+#include "vgic_common.h"
 
 //#define DEBUG_GIC_KVM
 
@@ -52,7 +53,7 @@ typedef struct KVMARMGICClass {
     void (*parent_reset)(DeviceState *dev);
 } KVMARMGICClass;
 
-static void kvm_arm_gic_set_irq(void *opaque, int irq, int level)
+void kvm_arm_gic_set_irq(uint32_t num_irq, int irq, int level)
 {
     /* Meaning of the 'irq' parameter:
      *  [0..N-1] : external interrupts
@@ -63,10 +64,9 @@ static void kvm_arm_gic_set_irq(void *opaque, int irq, int level)
      * has separate fields in the irq number for type,
      * CPU number and interrupt number.
      */
-    GICState *s = (GICState *)opaque;
     int kvm_irq, irqtype, cpu;
 
-    if (irq < (s->num_irq - GIC_INTERNAL)) {
+    if (irq < (num_irq - GIC_INTERNAL)) {
         /* External interrupt. The kernel numbers these like the GIC
          * hardware, with external interrupt IDs starting after the
          * internal ones.
@@ -77,7 +77,7 @@ static void kvm_arm_gic_set_irq(void *opaque, int irq, int level)
     } else {
         /* Internal interrupt: decode into (cpu, interrupt id) */
         irqtype = KVM_ARM_IRQ_TYPE_PPI;
-        irq -= (s->num_irq - GIC_INTERNAL);
+        irq -= (num_irq - GIC_INTERNAL);
         cpu = irq / GIC_INTERNAL;
         irq %= GIC_INTERNAL;
     }
@@ -87,12 +87,19 @@ static void kvm_arm_gic_set_irq(void *opaque, int irq, int level)
     kvm_set_irq(kvm_state, kvm_irq, !!level);
 }
 
+static void kvm_arm_gicv2_set_irq(void *opaque, int irq, int level)
+{
+    GICState *s = (GICState *)opaque;
+
+    kvm_arm_gic_set_irq(s->num_irq, irq, level);
+}
+
 static bool kvm_arm_gic_can_save_restore(GICState *s)
 {
     return s->dev_fd >= 0;
 }
 
-static bool kvm_gic_supports_attr(GICState *s, int group, int attrnum)
+bool kvm_gic_supports_attr(int dev_fd, int group, int attrnum)
 {
     struct kvm_device_attr attr = {
         .group = group,
@@ -100,14 +107,14 @@ static bool kvm_gic_supports_attr(GICState *s, int group, int attrnum)
         .flags = 0,
     };
 
-    if (s->dev_fd == -1) {
+    if (dev_fd == -1) {
         return false;
     }
 
-    return kvm_device_ioctl(s->dev_fd, KVM_HAS_DEVICE_ATTR, &attr) == 0;
+    return kvm_device_ioctl(dev_fd, KVM_HAS_DEVICE_ATTR, &attr) == 0;
 }
 
-static void kvm_gic_access(GICState *s, int group, int offset,
+void kvm_gic_access(int dev_fd, int group, int offset,
                                    int cpu, uint32_t *val, bool write)
 {
     struct kvm_device_attr attr;
@@ -130,7 +137,7 @@ static void kvm_gic_access(GICState *s, int group, int offset,
         type = KVM_GET_DEVICE_ATTR;
     }
 
-    err = kvm_device_ioctl(s->dev_fd, type, &attr);
+    err = kvm_device_ioctl(dev_fd, type, &attr);
     if (err < 0) {
         fprintf(stderr, "KVM_{SET/GET}_DEVICE_ATTR failed: %s\n",
                 strerror(-err));
@@ -138,20 +145,6 @@ static void kvm_gic_access(GICState *s, int group, int offset,
     }
 }
 
-static void kvm_gicd_access(GICState *s, int offset, int cpu,
-                            uint32_t *val, bool write)
-{
-    kvm_gic_access(s, KVM_DEV_ARM_VGIC_GRP_DIST_REGS,
-                   offset, cpu, val, write);
-}
-
-static void kvm_gicc_access(GICState *s, int offset, int cpu,
-                            uint32_t *val, bool write)
-{
-    kvm_gic_access(s, KVM_DEV_ARM_VGIC_GRP_CPU_REGS,
-                   offset, cpu, val, write);
-}
-
 #define for_each_irq_reg(_ctr, _max_irq, _field_width) \
     for (_ctr = 0; _ctr < ((_max_irq) / (32 / (_field_width))); _ctr++)
 
@@ -291,7 +284,7 @@ static void kvm_dist_get(GICState *s, uint32_t offset, int width,
         irq = i * regsz;
         cpu = 0;
         while ((cpu < s->num_cpu && irq < GIC_INTERNAL) || cpu == 0) {
-            kvm_gicd_access(s, offset, cpu, &reg, false);
+            kvm_gicd_access(s->dev_fd, offset, cpu, &reg, false);
             for (j = 0; j < regsz; j++) {
                 field = extract32(reg, j * width, width);
                 translate_fn(s, irq + j, cpu, &field, false);
@@ -324,7 +317,7 @@ static void kvm_dist_put(GICState *s, uint32_t offset, int width,
                 translate_fn(s, irq + j, cpu, &field, true);
                 reg = deposit32(reg, j * width, width, field);
             }
-            kvm_gicd_access(s, offset, cpu, &reg, true);
+            kvm_gicd_access(s->dev_fd, offset, cpu, &reg, true);
 
             cpu++;
         }
@@ -355,10 +348,10 @@ static void kvm_arm_gic_put(GICState *s)
 
     /* s->ctlr -> GICD_CTLR */
     reg = s->ctlr;
-    kvm_gicd_access(s, 0x0, 0, &reg, true);
+    kvm_gicd_access(s->dev_fd, 0x0, 0, &reg, true);
 
     /* Sanity checking on GICD_TYPER and s->num_irq, s->num_cpu */
-    kvm_gicd_access(s, 0x4, 0, &reg, false);
+    kvm_gicd_access(s->dev_fd, 0x4, 0, &reg, false);
     num_irq = ((reg & 0x1f) + 1) * 32;
     num_cpu = ((reg & 0xe0) >> 5) + 1;
 
@@ -416,24 +409,24 @@ static void kvm_arm_gic_put(GICState *s)
     for (cpu = 0; cpu < s->num_cpu; cpu++) {
         /* s->cpu_ctlr[cpu] -> GICC_CTLR */
         reg = s->cpu_ctlr[cpu];
-        kvm_gicc_access(s, 0x00, cpu, &reg, true);
+        kvm_gicc_access(s->dev_fd, 0x00, cpu, &reg, true);
 
         /* s->priority_mask[cpu] -> GICC_PMR */
         reg = (s->priority_mask[cpu] & 0xff);
-        kvm_gicc_access(s, 0x04, cpu, &reg, true);
+        kvm_gicc_access(s->dev_fd, 0x04, cpu, &reg, true);
 
         /* s->bpr[cpu] -> GICC_BPR */
         reg = (s->bpr[cpu] & 0x7);
-        kvm_gicc_access(s, 0x08, cpu, &reg, true);
+        kvm_gicc_access(s->dev_fd, 0x08, cpu, &reg, true);
 
         /* s->abpr[cpu] -> GICC_ABPR */
         reg = (s->abpr[cpu] & 0x7);
-        kvm_gicc_access(s, 0x1c, cpu, &reg, true);
+        kvm_gicc_access(s->dev_fd, 0x1c, cpu, &reg, true);
 
         /* s->apr[n][cpu] -> GICC_APRn */
         for (i = 0; i < 4; i++) {
             reg = s->apr[i][cpu];
-            kvm_gicc_access(s, 0xd0 + i * 4, cpu, &reg, true);
+            kvm_gicc_access(s->dev_fd, 0xd0 + i * 4, cpu, &reg, true);
         }
     }
 }
@@ -454,11 +447,11 @@ static void kvm_arm_gic_get(GICState *s)
      */
 
     /* GICD_CTLR -> s->ctlr */
-    kvm_gicd_access(s, 0x0, 0, &reg, false);
+    kvm_gicd_access(s->dev_fd, 0x0, 0, &reg, false);
     s->ctlr = reg;
 
     /* Sanity checking on GICD_TYPER -> s->num_irq, s->num_cpu */
-    kvm_gicd_access(s, 0x4, 0, &reg, false);
+    kvm_gicd_access(s->dev_fd, 0x4, 0, &reg, false);
     s->num_irq = ((reg & 0x1f) + 1) * 32;
     s->num_cpu = ((reg & 0xe0) >> 5) + 1;
 
@@ -469,7 +462,7 @@ static void kvm_arm_gic_get(GICState *s)
     }
 
     /* GICD_IIDR -> ? */
-    kvm_gicd_access(s, 0x8, 0, &reg, false);
+    kvm_gicd_access(s->dev_fd, 0x8, 0, &reg, false);
 
     /* Clear all the IRQ settings */
     for (i = 0; i < s->num_irq; i++) {
@@ -507,24 +500,24 @@ static void kvm_arm_gic_get(GICState *s)
 
     for (cpu = 0; cpu < s->num_cpu; cpu++) {
         /* GICC_CTLR -> s->cpu_ctlr[cpu] */
-        kvm_gicc_access(s, 0x00, cpu, &reg, false);
+        kvm_gicc_access(s->dev_fd, 0x00, cpu, &reg, false);
         s->cpu_ctlr[cpu] = reg;
 
         /* GICC_PMR -> s->priority_mask[cpu] */
-        kvm_gicc_access(s, 0x04, cpu, &reg, false);
+        kvm_gicc_access(s->dev_fd, 0x04, cpu, &reg, false);
         s->priority_mask[cpu] = (reg & 0xff);
 
         /* GICC_BPR -> s->bpr[cpu] */
-        kvm_gicc_access(s, 0x08, cpu, &reg, false);
+        kvm_gicc_access(s->dev_fd, 0x08, cpu, &reg, false);
         s->bpr[cpu] = (reg & 0x7);
 
         /* GICC_ABPR -> s->abpr[cpu] */
-        kvm_gicc_access(s, 0x1c, cpu, &reg, false);
+        kvm_gicc_access(s->dev_fd, 0x1c, cpu, &reg, false);
         s->abpr[cpu] = (reg & 0x7);
 
         /* GICC_APRn -> s->apr[n][cpu] */
         for (i = 0; i < 4; i++) {
-            kvm_gicc_access(s, 0xd0 + i * 4, cpu, &reg, false);
+            kvm_gicc_access(s->dev_fd, 0xd0 + i * 4, cpu, &reg, false);
             s->apr[i][cpu] = reg;
         }
     }
@@ -559,7 +552,7 @@ static void kvm_arm_gic_realize(DeviceState *dev, Error **errp)
         return;
     }
 
-    gic_init_irqs_and_mmio(s, kvm_arm_gic_set_irq, NULL);
+    gic_init_irqs_and_mmio(s, kvm_arm_gicv2_set_irq, NULL);
 
     for (i = 0; i < s->num_irq - GIC_INTERNAL; i++) {
         qemu_irq irq = qdev_get_gpio_in(dev, i);
@@ -576,15 +569,16 @@ static void kvm_arm_gic_realize(DeviceState *dev, Error **errp)
         return;
     }
 
-    if (kvm_gic_supports_attr(s, KVM_DEV_ARM_VGIC_GRP_NR_IRQS, 0)) {
+    if (kvm_gic_supports_attr(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_NR_IRQS, 0)) {
         uint32_t numirqs = s->num_irq;
-        kvm_gic_access(s, KVM_DEV_ARM_VGIC_GRP_NR_IRQS, 0, 0, &numirqs, 1);
+        kvm_gic_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_NR_IRQS, 0, 0,
+                       &numirqs, 1);
     }
 
     /* Tell the kernel to complete VGIC initialization now */
-    if (kvm_gic_supports_attr(s, KVM_DEV_ARM_VGIC_GRP_CTRL,
+    if (kvm_gic_supports_attr(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_CTRL,
                               KVM_DEV_ARM_VGIC_CTRL_INIT)) {
-        kvm_gic_access(s, KVM_DEV_ARM_VGIC_GRP_CTRL,
+        kvm_gic_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_CTRL,
                           KVM_DEV_ARM_VGIC_CTRL_INIT, 0, 0, 1);
     }
 
diff --git a/hw/intc/vgic_common.h b/hw/intc/vgic_common.h
new file mode 100644
index 0000000..67241d3
--- /dev/null
+++ b/hw/intc/vgic_common.h
@@ -0,0 +1,43 @@
+/*
+ * ARM KVM vGIC utility functions
+ *
+ * Copyright (c) 2015 Samsung Electronics
+ * Written by Pavel Fedin
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef QEMU_ARM_VGIC_COMMON_H
+#define QEMU_ARM_VGIC_COMMON_H
+
+void kvm_arm_gic_set_irq(uint32_t num_irq, int irq, int level);
+bool kvm_gic_supports_attr(int dev_fd, int group, int attrnum);
+void kvm_gic_access(int dev_fd, int group, int offset, int cpu,
+                    uint32_t *val, bool write);
+
+static inline void kvm_gicd_access(int dev_fd, int offset, int cpu,
+                                   uint32_t *val, bool write)
+{
+    kvm_gic_access(dev_fd, KVM_DEV_ARM_VGIC_GRP_DIST_REGS,
+                   offset, cpu, val, write);
+}
+
+static inline void kvm_gicc_access(int dev_fd, int offset, int cpu,
+                                   uint32_t *val, bool write)
+{
+    kvm_gic_access(dev_fd, KVM_DEV_ARM_VGIC_GRP_CPU_REGS,
+                   offset, cpu, val, write);
+}
+
+#endif
-- 
1.9.5.msysgit.0

[Qemu-devel] [PATCH v8 3/5] Introduce irqchip type specification for KVM

[Pavel Fedin]

This patch introduces kernel_irqchip_type member in Machine class, which
is passed to kvm_arch_irqchip_create. Machine models which can use vGIC
now use it in order to supply correct GIC type for KVM capability
verification. The variable is defined as int in order to be
architecture-agnostic for potential future uses by other architectures.

Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
---
 hw/arm/vexpress.c       |  3 +++
 hw/arm/virt.c           |  3 +++
 include/hw/boards.h     |  1 +
 include/sysemu/kvm.h    |  3 ++-
 kvm-all.c               |  2 +-
 stubs/kvm.c             |  2 +-
 target-arm/kvm-consts.h |  6 ++++++
 target-arm/kvm.c        | 13 +++++++++++--
 8 files changed, 28 insertions(+), 5 deletions(-)

diff --git a/hw/arm/vexpress.c b/hw/arm/vexpress.c
index da21788..818399b 100644
--- a/hw/arm/vexpress.c
+++ b/hw/arm/vexpress.c
@@ -741,6 +741,9 @@ static void vexpress_instance_init(Object *obj)
                                     "Set on/off to enable/disable the ARM "
                                     "Security Extensions (TrustZone)",
                                     NULL);
+
+    /* vexpress uses GICv2 */
+    vms->parent.kernel_irqchip_type = QEMU_GIC_TYPE_V2;
 }
 
 static void vexpress_class_init(ObjectClass *oc, void *data)
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 943e523..4d04ec0 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -944,6 +944,9 @@ static void virt_instance_init(Object *obj)
                                     "Set on/off to enable/disable the ARM "
                                     "Security Extensions (TrustZone)",
                                     NULL);
+
+    /* Default GIC type is v2 */
+    vms->parent.kernel_irqchip_type = QEMU_GIC_TYPE_V2;
 }
 
 static void virt_class_init(ObjectClass *oc, void *data)
diff --git a/include/hw/boards.h b/include/hw/boards.h
index 2aec9cb..37eb767 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -127,6 +127,7 @@ struct MachineState {
     char *accel;
     bool kernel_irqchip_allowed;
     bool kernel_irqchip_required;
+    int kernel_irqchip_type;
     int kvm_shadow_mem;
     char *dtb;
     char *dumpdtb;
diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
index 983e99e..8f4d485 100644
--- a/include/sysemu/kvm.h
+++ b/include/sysemu/kvm.h
@@ -434,6 +434,7 @@ void kvm_init_irq_routing(KVMState *s);
 /**
  * kvm_arch_irqchip_create:
  * @KVMState: The KVMState pointer
+ * @type: irqchip type, architecture-specific
  *
  * Allow architectures to create an in-kernel irq chip themselves.
  *
@@ -441,7 +442,7 @@ void kvm_init_irq_routing(KVMState *s);
  *            0: irq chip was not created
  *          > 0: irq chip was created
  */
-int kvm_arch_irqchip_create(KVMState *s);
+int kvm_arch_irqchip_create(KVMState *s, int type);
 
 /**
  * kvm_set_one_reg - set a register value in KVM via KVM_SET_ONE_REG ioctl
diff --git a/kvm-all.c b/kvm-all.c
index 06e06f2..8df938d 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -1395,7 +1395,7 @@ static void kvm_irqchip_create(MachineState *machine, KVMState *s)
 
     /* First probe and see if there's a arch-specific hook to create the
      * in-kernel irqchip for us */
-    ret = kvm_arch_irqchip_create(s);
+    ret = kvm_arch_irqchip_create(s, machine->kernel_irqchip_type);
     if (ret == 0) {
         ret = kvm_vm_ioctl(s, KVM_CREATE_IRQCHIP);
     }
diff --git a/stubs/kvm.c b/stubs/kvm.c
index e7c60b6..a8505ff 100644
--- a/stubs/kvm.c
+++ b/stubs/kvm.c
@@ -1,7 +1,7 @@
 #include "qemu-common.h"
 #include "sysemu/kvm.h"
 
-int kvm_arch_irqchip_create(KVMState *s)
+int kvm_arch_irqchip_create(KVMState *s, int type)
 {
     return 0;
 }
diff --git a/target-arm/kvm-consts.h b/target-arm/kvm-consts.h
index 943bf89..0bc12b7 100644
--- a/target-arm/kvm-consts.h
+++ b/target-arm/kvm-consts.h
@@ -39,6 +39,12 @@ MISMATCH_CHECK(CP_REG_SIZE_U64, KVM_REG_SIZE_U64)
 MISMATCH_CHECK(CP_REG_ARM, KVM_REG_ARM)
 MISMATCH_CHECK(CP_REG_ARCH_MASK, KVM_REG_ARCH_MASK)
 
+#define QEMU_GIC_TYPE_V2 5
+#define QEMU_GIC_TYPE_V3 7
+
+MISMATCH_CHECK(QEMU_GIC_TYPE_V2, KVM_DEV_TYPE_ARM_VGIC_V2)
+MISMATCH_CHECK(QEMU_GIC_TYPE_V3, KVM_DEV_TYPE_ARM_VGIC_V3)
+
 #define QEMU_PSCI_0_1_FN_BASE 0x95c1ba5e
 #define QEMU_PSCI_0_1_FN(n) (QEMU_PSCI_0_1_FN_BASE + (n))
 #define QEMU_PSCI_0_1_FN_CPU_SUSPEND QEMU_PSCI_0_1_FN(0)
diff --git a/target-arm/kvm.c b/target-arm/kvm.c
index b278542..180f75f 100644
--- a/target-arm/kvm.c
+++ b/target-arm/kvm.c
@@ -583,19 +583,28 @@ void kvm_arch_init_irq_routing(KVMState *s)
 {
 }
 
-int kvm_arch_irqchip_create(KVMState *s)
+int kvm_arch_irqchip_create(KVMState *s, int type)
 {
     int ret;
 
+    /* Failure here means forgotten initialization of
+     * machine->kernel_irqchip_type in model code
+     */
+    assert(type != 0);
+
     /* If we can create the VGIC using the newer device control API, we
      * let the device do this when it initializes itself, otherwise we
      * fall back to the old API */
 
-    ret = kvm_create_device(s, KVM_DEV_TYPE_ARM_VGIC_V2, true);
+    ret = kvm_create_device(s, type, true);
     if (ret == 0) {
         return 1;
     }
 
+    /* Fallback will create VGIC v2 */
+    if (type != KVM_DEV_TYPE_ARM_VGIC_V2) {
+        return ret;
+    }
     return 0;
 }
 
-- 
1.9.5.msysgit.0

[Qemu-devel] [PATCH v8 4/5] Initial implementation of vGICv3

[Pavel Fedin]

Get/put routines are missing, live migration is not possible.

Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
---
 hw/intc/Makefile.objs   |   1 +
 hw/intc/arm_gicv3_kvm.c | 143 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 144 insertions(+)
 create mode 100644 hw/intc/arm_gicv3_kvm.c

diff --git a/hw/intc/Makefile.objs b/hw/intc/Makefile.objs
index 1317e5a..004b0c2 100644
--- a/hw/intc/Makefile.objs
+++ b/hw/intc/Makefile.objs
@@ -17,6 +17,7 @@ common-obj-$(CONFIG_OPENPIC) += openpic.o
 
 obj-$(CONFIG_APIC) += apic.o apic_common.o
 obj-$(CONFIG_ARM_GIC_KVM) += arm_gic_kvm.o
+obj-$(call land,$(CONFIG_ARM_GIC_KVM),$(TARGET_AARCH64)) += arm_gicv3_kvm.o
 obj-$(CONFIG_STELLARIS) += armv7m_nvic.o
 obj-$(CONFIG_EXYNOS4) += exynos4210_gic.o exynos4210_combiner.o
 obj-$(CONFIG_GRLIB) += grlib_irqmp.o
diff --git a/hw/intc/arm_gicv3_kvm.c b/hw/intc/arm_gicv3_kvm.c
new file mode 100644
index 0000000..e540eb1
--- /dev/null
+++ b/hw/intc/arm_gicv3_kvm.c
@@ -0,0 +1,143 @@
+/*
+ * ARM Generic Interrupt Controller using KVM in-kernel support
+ *
+ * Copyright (c) 2015 Samsung Electronics Co., Ltd.
+ * Written by Pavel Fedin
+ * Based on vGICv2 code by Peter Maydell
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include "hw/intc/arm_gicv3_common.h"
+#include "hw/sysbus.h"
+#include "sysemu/kvm.h"
+#include "kvm_arm.h"
+#include "vgic_common.h"
+
+#ifdef DEBUG_GICV3_KVM
+#define DPRINTF(fmt, ...) \
+    do { fprintf(stderr, "kvm_gicv3: " fmt, ## __VA_ARGS__); } while (0)
+#else
+#define DPRINTF(fmt, ...) \
+    do { } while (0)
+#endif
+
+#define TYPE_KVM_ARM_GICV3 "kvm-arm-gicv3"
+#define KVM_ARM_GICV3(obj) \
+     OBJECT_CHECK(GICv3State, (obj), TYPE_KVM_ARM_GICV3)
+#define KVM_ARM_GICV3_CLASS(klass) \
+     OBJECT_CLASS_CHECK(KVMARMGICv3Class, (klass), TYPE_KVM_ARM_GICV3)
+#define KVM_ARM_GICV3_GET_CLASS(obj) \
+     OBJECT_GET_CLASS(KVMARMGICv3Class, (obj), TYPE_KVM_ARM_GICV3)
+
+typedef struct KVMARMGICv3Class {
+    ARMGICv3CommonClass parent_class;
+    DeviceRealize parent_realize;
+    void (*parent_reset)(DeviceState *dev);
+} KVMARMGICv3Class;
+
+static void kvm_arm_gicv3_set_irq(void *opaque, int irq, int level)
+{
+    GICv3State *s = (GICv3State *)opaque;
+
+    kvm_arm_gic_set_irq(s->num_irq, irq, level);
+}
+
+static void kvm_arm_gicv3_put(GICv3State *s)
+{
+    /* TODO */
+    DPRINTF("Cannot put kernel gic state, no kernel interface\n");
+}
+
+static void kvm_arm_gicv3_get(GICv3State *s)
+{
+    /* TODO */
+    DPRINTF("Cannot get kernel gic state, no kernel interface\n");
+}
+
+static void kvm_arm_gicv3_reset(DeviceState *dev)
+{
+    GICv3State *s = ARM_GICV3_COMMON(dev);
+    KVMARMGICv3Class *kgc = KVM_ARM_GICV3_GET_CLASS(s);
+
+    DPRINTF("Reset\n");
+
+    kgc->parent_reset(dev);
+    kvm_arm_gicv3_put(s);
+}
+
+static void kvm_arm_gicv3_realize(DeviceState *dev, Error **errp)
+{
+    GICv3State *s = KVM_ARM_GICV3(dev);
+    KVMARMGICv3Class *kgc = KVM_ARM_GICV3_GET_CLASS(s);
+    Error *local_err = NULL;
+
+    DPRINTF("kvm_arm_gicv3_realize\n");
+
+    kgc->parent_realize(dev, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+
+    gicv3_init_irqs_and_mmio(s, kvm_arm_gicv3_set_irq, NULL);
+
+    /* Try to create the device via the device control API */
+    s->dev_fd = kvm_create_device(kvm_state, KVM_DEV_TYPE_ARM_VGIC_V3, false);
+    if (s->dev_fd < 0) {
+        error_setg_errno(errp, -s->dev_fd, "error creating in-kernel VGIC");
+        return;
+    }
+
+    kvm_gic_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_NR_IRQS,
+                   0, 0, &s->num_irq, 1);
+
+    /* Tell the kernel to complete VGIC initialization now */
+    kvm_gic_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_CTRL,
+                   KVM_DEV_ARM_VGIC_CTRL_INIT, 0, 0, 1);
+
+    kvm_arm_register_device(&s->iomem_dist, -1, KVM_DEV_ARM_VGIC_GRP_ADDR,
+                            KVM_VGIC_V3_ADDR_TYPE_DIST, s->dev_fd);
+    kvm_arm_register_device(&s->iomem_redist, -1, KVM_DEV_ARM_VGIC_GRP_ADDR,
+                            KVM_VGIC_V3_ADDR_TYPE_REDIST, s->dev_fd);
+}
+
+static void kvm_arm_gicv3_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    ARMGICv3CommonClass *agcc = ARM_GICV3_COMMON_CLASS(klass);
+    KVMARMGICv3Class *kgc = KVM_ARM_GICV3_CLASS(klass);
+
+    agcc->pre_save = kvm_arm_gicv3_get;
+    agcc->post_load = kvm_arm_gicv3_put;
+    kgc->parent_realize = dc->realize;
+    kgc->parent_reset = dc->reset;
+    dc->realize = kvm_arm_gicv3_realize;
+    dc->reset = kvm_arm_gicv3_reset;
+}
+
+static const TypeInfo kvm_arm_gicv3_info = {
+    .name = TYPE_KVM_ARM_GICV3,
+    .parent = TYPE_ARM_GICV3_COMMON,
+    .instance_size = sizeof(GICv3State),
+    .class_init = kvm_arm_gicv3_class_init,
+    .class_size = sizeof(KVMARMGICv3Class),
+};
+
+static void kvm_arm_gicv3_register_types(void)
+{
+    type_register_static(&kvm_arm_gicv3_info);
+}
+
+type_init(kvm_arm_gicv3_register_types)
-- 
1.9.5.msysgit.0

[Qemu-devel] [PATCH v8 5/5] Add gicversion option to virt machine

[Pavel Fedin]

Set kernel_irqchip_type according to value of the option and pass it
around where necessary. Instantiate devices and fdt nodes according
to the choice.

max_cpus for virt machine increased to 64. GICv2 compatibility check
happens inside arm_gic_common_realize().

Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
---
 hw/arm/virt.c         | 132 ++++++++++++++++++++++++++++++++++++++++++--------
 include/hw/arm/fdt.h  |   2 +-
 include/hw/arm/virt.h |   5 +-
 3 files changed, 118 insertions(+), 21 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 4d04ec0..f7d7555 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -49,6 +49,7 @@
 #include "hw/platform-bus.h"
 #include "hw/arm/fdt.h"
 #include "kvm_arm.h"
+#include "qapi/visitor.h"
 
 /* Number of external interrupt lines to configure the GIC with */
 #define NUM_IRQS 256
@@ -108,6 +109,9 @@ static const MemMapEntry a15memmap[] = {
     [VIRT_GIC_DIST] =           { 0x08000000, 0x00010000 },
     [VIRT_GIC_CPU] =            { 0x08010000, 0x00010000 },
     [VIRT_GIC_V2M] =            { 0x08020000, 0x00001000 },
+    [VIRT_ITS_CONTROL] =        { 0x08020000, 0x00010000 },
+    [VIRT_ITS_TRANSLATION] =    { 0x08030000, 0x00010000 },
+    [VIRT_GIC_REDIST] =         { 0x08040000, 0x00800000 },
     [VIRT_UART] =               { 0x09000000, 0x00001000 },
     [VIRT_RTC] =                { 0x09010000, 0x00001000 },
     [VIRT_FW_CFG] =             { 0x09020000, 0x0000000a },
@@ -257,10 +261,13 @@ static void fdt_add_timer_nodes(const VirtBoardInfo *vbi)
      * they are edge-triggered.
      */
     ARMCPU *armcpu;
+    uint32_t max;
     uint32_t irqflags = GIC_FDT_IRQ_FLAGS_EDGE_LO_HI;
 
+    /* Argument is 32 bit but 8 bits are reserved for flags */
+    max = (vbi->smp_cpus >= 24) ? 24 : vbi->smp_cpus;
     irqflags = deposit32(irqflags, GIC_FDT_IRQ_PPI_CPU_START,
-                         GIC_FDT_IRQ_PPI_CPU_WIDTH, (1 << vbi->smp_cpus) - 1);
+                         GIC_FDT_IRQ_PPI_CPU_WIDTH, (1 << max) - 1);
 
     qemu_fdt_add_subnode(vbi->fdt, "/timer");
 
@@ -284,6 +291,18 @@ static void fdt_add_cpu_nodes(const VirtBoardInfo *vbi)
 {
     int cpu;
 
+    /*
+     * From Documentation/devicetree/bindings/arm/cpus.txt
+     *  On ARM v8 64-bit systems value should be set to 2,
+     *  that corresponds to the MPIDR_EL1 register size.
+     *  If MPIDR_EL1[63:32] value is equal to 0 on all CPUs
+     *  in the system, #address-cells can be set to 1, since
+     *  MPIDR_EL1[63:32] bits are not used for CPUs
+     *  identification.
+     *
+     *  Now GIC500 doesn't support affinities 2 & 3 so currently
+     *  #address-cells can stay 1 until future GIC
+     */
     qemu_fdt_add_subnode(vbi->fdt, "/cpus");
     qemu_fdt_setprop_cell(vbi->fdt, "/cpus", "#address-cells", 0x1);
     qemu_fdt_setprop_cell(vbi->fdt, "/cpus", "#size-cells", 0x0);
@@ -320,25 +339,36 @@ static void fdt_add_v2m_gic_node(VirtBoardInfo *vbi)
     qemu_fdt_setprop_cell(vbi->fdt, "/intc/v2m", "phandle", vbi->v2m_phandle);
 }
 
-static void fdt_add_gic_node(VirtBoardInfo *vbi)
+static void fdt_add_gic_node(VirtBoardInfo *vbi, int type)
 {
     vbi->gic_phandle = qemu_fdt_alloc_phandle(vbi->fdt);
     qemu_fdt_setprop_cell(vbi->fdt, "/", "interrupt-parent", vbi->gic_phandle);
 
     qemu_fdt_add_subnode(vbi->fdt, "/intc");
-    /* 'cortex-a15-gic' means 'GIC v2' */
-    qemu_fdt_setprop_string(vbi->fdt, "/intc", "compatible",
-                            "arm,cortex-a15-gic");
     qemu_fdt_setprop_cell(vbi->fdt, "/intc", "#interrupt-cells", 3);
     qemu_fdt_setprop(vbi->fdt, "/intc", "interrupt-controller", NULL, 0);
-    qemu_fdt_setprop_sized_cells(vbi->fdt, "/intc", "reg",
-                                     2, vbi->memmap[VIRT_GIC_DIST].base,
-                                     2, vbi->memmap[VIRT_GIC_DIST].size,
-                                     2, vbi->memmap[VIRT_GIC_CPU].base,
-                                     2, vbi->memmap[VIRT_GIC_CPU].size);
     qemu_fdt_setprop_cell(vbi->fdt, "/intc", "#address-cells", 0x2);
     qemu_fdt_setprop_cell(vbi->fdt, "/intc", "#size-cells", 0x2);
     qemu_fdt_setprop(vbi->fdt, "/intc", "ranges", NULL, 0);
+    if (type == QEMU_GIC_TYPE_V3) {
+        qemu_fdt_setprop_string(vbi->fdt, "/intc", "compatible",
+                                "arm,gic-v3");
+        qemu_fdt_setprop_sized_cells(vbi->fdt, "/intc", "reg",
+                                     2, vbi->memmap[VIRT_GIC_DIST].base,
+                                     2, vbi->memmap[VIRT_GIC_DIST].size,
+                                     2, vbi->memmap[VIRT_GIC_REDIST].base,
+                                     2, vbi->memmap[VIRT_GIC_REDIST].size);
+    } else {
+        /* 'cortex-a15-gic' means 'GIC v2' */
+        qemu_fdt_setprop_string(vbi->fdt, "/intc", "compatible",
+                                "arm,cortex-a15-gic");
+        qemu_fdt_setprop_sized_cells(vbi->fdt, "/intc", "reg",
+                                      2, vbi->memmap[VIRT_GIC_DIST].base,
+                                      2, vbi->memmap[VIRT_GIC_DIST].size,
+                                      2, vbi->memmap[VIRT_GIC_CPU].base,
+                                      2, vbi->memmap[VIRT_GIC_CPU].size);
+    }
+
     qemu_fdt_setprop_cell(vbi->fdt, "/intc", "phandle", vbi->gic_phandle);
 }
 
@@ -361,7 +391,7 @@ static void create_v2m(VirtBoardInfo *vbi, qemu_irq *pic)
     fdt_add_v2m_gic_node(vbi);
 }
 
-static void create_gic(VirtBoardInfo *vbi, qemu_irq *pic)
+static void create_gic(VirtBoardInfo *vbi, qemu_irq *pic, int type)
 {
     /* We create a standalone GIC v2 */
     DeviceState *gicdev;
@@ -369,10 +399,28 @@ static void create_gic(VirtBoardInfo *vbi, qemu_irq *pic)
     const char *gictype;
     int i;
 
-    gictype = gic_class_name();
+    if (type == QEMU_GIC_TYPE_V3) {
+        /* TODO: Software emulation is not implemented yet */
+        if (!kvm_irqchip_in_kernel()) {
+            fprintf(stderr, "KVM is currently required for GICv3 emulation\n");
+            exit(1);
+        }
+        gictype = "kvm-arm-gicv3";
+    } else {
+        gictype = gic_class_name();
+    }
 
     gicdev = qdev_create(NULL, gictype);
-    qdev_prop_set_uint32(gicdev, "revision", 2);
+
+    for (i = 0; i < vbi->smp_cpus; i++) {
+        CPUState *cpu = qemu_get_cpu(i);
+        CPUARMState *env = cpu->env_ptr;
+        env->nvic = gicdev;
+    }
+
+    if (type == QEMU_GIC_TYPE_V2) {
+        qdev_prop_set_uint32(gicdev, "revision", 2);
+    }
     qdev_prop_set_uint32(gicdev, "num-cpu", smp_cpus);
     /* Note that the num-irq property counts both internal and external
      * interrupts; there are always 32 of the former (mandated by GIC spec).
@@ -381,7 +429,11 @@ static void create_gic(VirtBoardInfo *vbi, qemu_irq *pic)
     qdev_init_nofail(gicdev);
     gicbusdev = SYS_BUS_DEVICE(gicdev);
     sysbus_mmio_map(gicbusdev, 0, vbi->memmap[VIRT_GIC_DIST].base);
-    sysbus_mmio_map(gicbusdev, 1, vbi->memmap[VIRT_GIC_CPU].base);
+    if (type == QEMU_GIC_TYPE_V3) {
+        sysbus_mmio_map(gicbusdev, 1, vbi->memmap[VIRT_GIC_REDIST].base);
+    } else {
+        sysbus_mmio_map(gicbusdev, 1, vbi->memmap[VIRT_GIC_CPU].base);
+    }
 
     /* Wire the outputs from each CPU's generic timer to the
      * appropriate GIC PPI inputs, and the GIC's IRQ output to
@@ -408,9 +460,11 @@ static void create_gic(VirtBoardInfo *vbi, qemu_irq *pic)
         pic[i] = qdev_get_gpio_in(gicdev, i);
     }
 
-    fdt_add_gic_node(vbi);
+    fdt_add_gic_node(vbi, type);
 
-    create_v2m(vbi, pic);
+    if (type == QEMU_GIC_TYPE_V2) {
+        create_v2m(vbi, pic);
+    }
 }
 
 static void create_uart(const VirtBoardInfo *vbi, qemu_irq *pic)
@@ -714,7 +768,10 @@ static void create_pcie(const VirtBoardInfo *vbi, qemu_irq *pic)
     qemu_fdt_setprop_cells(vbi->fdt, nodename, "bus-range", 0,
                            nr_pcie_buses - 1);
 
-    qemu_fdt_setprop_cells(vbi->fdt, nodename, "msi-parent", vbi->v2m_phandle);
+    if (vbi->v2m_phandle) {
+        qemu_fdt_setprop_cells(vbi->fdt, nodename, "msi-parent",
+                               vbi->v2m_phandle);
+    }
 
     qemu_fdt_setprop_sized_cells(vbi->fdt, nodename, "reg",
                                  2, base_ecam, 2, size_ecam);
@@ -874,7 +931,7 @@ static void machvirt_init(MachineState *machine)
 
     create_flash(vbi);
 
-    create_gic(vbi, pic);
+    create_gic(vbi, pic, machine->kernel_irqchip_type);
 
     create_uart(vbi, pic);
 
@@ -932,6 +989,37 @@ static void virt_set_secure(Object *obj, bool value, Error **errp)
     vms->secure = value;
 }
 
+static void virt_get_gic_version(Object *obj, Visitor *v, void *opaque,
+                                 const char *name, Error **errp)
+{
+    MachineState *ms = MACHINE(obj);
+    int32_t value = (ms->kernel_irqchip_type == QEMU_GIC_TYPE_V3) ?
+                     3 : 2;
+
+    visit_type_int32(v, &value, name, errp);
+}
+
+static void virt_set_gic_version(Object *obj, Visitor *v, void *opaque,
+                                 const char *name, Error **errp)
+{
+    MachineState *ms = MACHINE(obj);
+    int32_t value;
+
+    visit_type_int32(v, &value, name, errp);
+
+    switch (value) {
+    case 3:
+        ms->kernel_irqchip_type = QEMU_GIC_TYPE_V3;
+        break;
+    case 2:
+        ms->kernel_irqchip_type = QEMU_GIC_TYPE_V2;
+        break;
+    default:
+        error_report("Only GICv2 and GICv3 supported currently\n");
+        exit(1);
+    }
+}
+
 static void virt_instance_init(Object *obj)
 {
     VirtMachineState *vms = VIRT_MACHINE(obj);
@@ -947,6 +1035,11 @@ static void virt_instance_init(Object *obj)
 
     /* Default GIC type is v2 */
     vms->parent.kernel_irqchip_type = QEMU_GIC_TYPE_V2;
+    object_property_add(obj, "gicversion", "int", virt_get_gic_version,
+                        virt_set_gic_version, NULL, NULL, NULL);
+    object_property_set_description(obj, "gicversion",
+                                    "Set GIC version. "
+                                    "Valid values are 2 and 3", NULL);
 }
 
 static void virt_class_init(ObjectClass *oc, void *data)
@@ -956,7 +1049,8 @@ static void virt_class_init(ObjectClass *oc, void *data)
     mc->name = TYPE_VIRT_MACHINE;
     mc->desc = "ARM Virtual Machine",
     mc->init = machvirt_init;
-    mc->max_cpus = 8;
+    /* With gic3 full implementation (with bitops) rase the lmit to 128 */
+    mc->max_cpus = 64;
     mc->has_dynamic_sysbus = true;
     mc->block_default_type = IF_VIRTIO;
     mc->no_cdrom = 1;
diff --git a/include/hw/arm/fdt.h b/include/hw/arm/fdt.h
index c3d5015..dd794dd 100644
--- a/include/hw/arm/fdt.h
+++ b/include/hw/arm/fdt.h
@@ -29,6 +29,6 @@
 #define GIC_FDT_IRQ_FLAGS_LEVEL_LO 8
 
 #define GIC_FDT_IRQ_PPI_CPU_START 8
-#define GIC_FDT_IRQ_PPI_CPU_WIDTH 8
+#define GIC_FDT_IRQ_PPI_CPU_WIDTH 24
 
 #endif
diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
index d22fd8e..6c5a9c9 100644
--- a/include/hw/arm/virt.h
+++ b/include/hw/arm/virt.h
@@ -46,6 +46,10 @@ enum {
     VIRT_CPUPERIPHS,
     VIRT_GIC_DIST,
     VIRT_GIC_CPU,
+    VIRT_GIC_V2M,
+    VIRT_ITS_CONTROL,
+    VIRT_ITS_TRANSLATION,
+    VIRT_GIC_REDIST,
     VIRT_UART,
     VIRT_MMIO,
     VIRT_RTC,
@@ -54,7 +58,6 @@ enum {
     VIRT_PCIE_MMIO,
     VIRT_PCIE_PIO,
     VIRT_PCIE_ECAM,
-    VIRT_GIC_V2M,
     VIRT_PLATFORM_BUS,
 };
 
-- 
1.9.5.msysgit.0

Re: [Qemu-devel] [PATCH v8 3/5] Introduce irqchip type specification for KVM

[Peter Maydell]

On 10 August 2015 at 13:06, Pavel Fedin <p.fedin@samsung.com> wrote:
> This patch introduces kernel_irqchip_type member in Machine class, which
> is passed to kvm_arch_irqchip_create. Machine models which can use vGIC
> now use it in order to supply correct GIC type for KVM capability
> verification. The variable is defined as int in order to be
> architecture-agnostic for potential future uses by other architectures.
>
> Signed-off-by: Pavel Fedin <p.fedin@samsung.com>

I still think this is the wrong approach -- see my remarks
in the previous round of patch review.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v8 5/5] Add gicversion option to virt machine

[Peter Maydell]

On 10 August 2015 at 13:06, Pavel Fedin <p.fedin@samsung.com> wrote:
> Set kernel_irqchip_type according to value of the option and pass it
> around where necessary. Instantiate devices and fdt nodes according
> to the choice.
>
> max_cpus for virt machine increased to 64. GICv2 compatibility check
> happens inside arm_gic_common_realize().

>  /* Number of external interrupt lines to configure the GIC with */
>  #define NUM_IRQS 256
> @@ -108,6 +109,9 @@ static const MemMapEntry a15memmap[] = {
>      [VIRT_GIC_DIST] =           { 0x08000000, 0x00010000 },
>      [VIRT_GIC_CPU] =            { 0x08010000, 0x00010000 },
>      [VIRT_GIC_V2M] =            { 0x08020000, 0x00001000 },
> +    [VIRT_ITS_CONTROL] =        { 0x08020000, 0x00010000 },
> +    [VIRT_ITS_TRANSLATION] =    { 0x08030000, 0x00010000 },

We don't implement ITS yet, so are these placeholders for that?
(That's OK, but if they don't do anything yet it's worth
commenting to that effect.)

Any particular reason for having two separate VIRT_ITS_*
entries? The spec mandates that the two 64K pages of ITS
have to be consecutive, so it would make life easier for
boards if they were just a single memory region.

> +    [VIRT_GIC_REDIST] =         { 0x08040000, 0x00800000 },

Is this layout borrowed from any particular real world board,
by the way?

>      VirtMachineState *vms = VIRT_MACHINE(obj);
> @@ -947,6 +1035,11 @@ static void virt_instance_init(Object *obj)
>
>      /* Default GIC type is v2 */
>      vms->parent.kernel_irqchip_type = QEMU_GIC_TYPE_V2;
> +    object_property_add(obj, "gicversion", "int", virt_get_gic_version,
> +                        virt_set_gic_version, NULL, NULL, NULL);
> +    object_property_set_description(obj, "gicversion",
> +                                    "Set GIC version. "
> +                                    "Valid values are 2 and 3", NULL);
>  }

"gic-version" would be more in line with other property naming
styles I think.

>
>  static void virt_class_init(ObjectClass *oc, void *data)
> @@ -956,7 +1049,8 @@ static void virt_class_init(ObjectClass *oc, void *data)
>      mc->name = TYPE_VIRT_MACHINE;
>      mc->desc = "ARM Virtual Machine",
>      mc->init = machvirt_init;
> -    mc->max_cpus = 8;
> +    /* With gic3 full implementation (with bitops) rase the lmit to 128 */
> +    mc->max_cpus = 64;

Comment disagrees with code, and in any case where does the CPU
maximum limit come from?

thanks
-- PMM

Re: [Qemu-devel] [PATCH v8 4/5] Initial implementation of vGICv3

[Peter Maydell]

On 10 August 2015 at 13:06, Pavel Fedin <p.fedin@samsung.com> wrote:
> Get/put routines are missing, live migration is not possible.

This commit message is too terse.

Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH v8 1/5] Implement GIC-500 base class

[Peter Maydell]

On 10 August 2015 at 13:06, Pavel Fedin <p.fedin@samsung.com> wrote:
> From: Shlomo Pongratz <shlomo.pongratz@huawei.com>
>
> This class is to be used by both software and KVM implementations of GICv3
>
> Currently it is mostly a placeholder, but in future it is supposed to hold
> qemu's representation of GICv3 state, which is necessary for migration.
>
> Signed-off-by: Shlomo Pongratz <shlomo.pongratz@huawei.com>
> Signed-off-by: Pavel Fedin <p.fedin@samsung.com>

> +    memory_region_init_io(&s->iomem_dist, OBJECT(s), ops, s,
> +                          "gicv3_dist", 0x10000);
> +    memory_region_init_io(&s->iomem_redist, OBJECT(s), ops ? &ops[1] : NULL, s,
> +                          "gicv3_redist", 0x800000);

Why is the redistributor region so huge? The spec says a
redistributor has four 64KB frames in its memory map:
 * RD_base
 * SGI_base
 * VLPI_base
 * Reserved

which is a lot smaller than this memory region...

> +
> +/* Maximum number of possible interrupts, determined by the GIC architecture */
> +#define GICV3_MAXIRQ 1020

The comment could use updating, because part of the point of GICv3
is to not have that limit...

> +#define GICV3_NCPU 64

What is imposing this NCPU limit?

> +
> +typedef struct GICv3State {
> +    /*< private >*/
> +    SysBusDevice parent_obj;
> +    /*< public >*/
> +
> +    qemu_irq parent_irq[GICV3_NCPU];
> +    qemu_irq parent_fiq[GICV3_NCPU];

We should be allocating memory for the right number of irqs
and fiqs based on the number of actual CPUs, because we don't
really want a compile-time arbitrary limit on NCPUs.


thanks
-- PMM

Re: [Qemu-devel] [PATCH v8 2/5] Extract some reusable vGIC code

[Peter Maydell]

On 10 August 2015 at 13:06, Pavel Fedin <p.fedin@samsung.com> wrote:
> These functions are useful also for vGICv3 implementation. Make them accessible
> from within other modules.
>
> Actually kvm_dist_get() and kvm_dist_put() could also be made reusable, but
> they would require two extra parameters (s->dev_fd and s->num_cpu) as well as
> lots of typecasts of 's' to DeviceState * and back to GICState *. This makes
> the code very ugly so i decided to stop at this point. I tried also an
> approach with making a base class for all possible GICs, but it would contain
> only three variables (dev_fd, cpu_num and irq_num), and accessing them through
> the rest of the code would be again tedious (either ugly casts or qemu-style
> separate object pointer). So i disliked it too.
>
> Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
> ---
>  hw/intc/arm_gic_kvm.c | 84 ++++++++++++++++++++++++---------------------------
>  hw/intc/vgic_common.h | 43 ++++++++++++++++++++++++++
>  2 files changed, 82 insertions(+), 45 deletions(-)
>  create mode 100644 hw/intc/vgic_common.h
>
> diff --git a/hw/intc/arm_gic_kvm.c b/hw/intc/arm_gic_kvm.c
> index e5d0f67..6ce539b 100644
> --- a/hw/intc/arm_gic_kvm.c
> +++ b/hw/intc/arm_gic_kvm.c
> @@ -23,6 +23,7 @@
>  #include "sysemu/kvm.h"
>  #include "kvm_arm.h"
>  #include "gic_internal.h"
> +#include "vgic_common.h"
>
>  //#define DEBUG_GIC_KVM
>
> @@ -52,7 +53,7 @@ typedef struct KVMARMGICClass {
>      void (*parent_reset)(DeviceState *dev);
>  } KVMARMGICClass;
>
> -static void kvm_arm_gic_set_irq(void *opaque, int irq, int level)
> +void kvm_arm_gic_set_irq(uint32_t num_irq, int irq, int level)

The need to add num_irq to this prototype reveals the ugliness
of it as an interface. It would be much nicer to convert the
GIC to using named GPIO arrays for its incoming interrupt
lines, so that you can handle the different cases of SPIs
and PPIs naturally rather than exposing the awkward mapping
from multiple different sets of interrupts into a single
integer space.

I don't insist on that being done for the GICv3 work, but it
does rather affect the shape of these functions you're trying
to factor out, so it's worth considering.

>  {
>      /* Meaning of the 'irq' parameter:
>       *  [0..N-1] : external interrupts
> @@ -63,10 +64,9 @@ static void kvm_arm_gic_set_irq(void *opaque, int irq, int level)
>       * has separate fields in the irq number for type,
>       * CPU number and interrupt number.
>       */
> -    GICState *s = (GICState *)opaque;
>      int kvm_irq, irqtype, cpu;
>
> -    if (irq < (s->num_irq - GIC_INTERNAL)) {
> +    if (irq < (num_irq - GIC_INTERNAL)) {
>          /* External interrupt. The kernel numbers these like the GIC
>           * hardware, with external interrupt IDs starting after the
>           * internal ones.
> @@ -77,7 +77,7 @@ static void kvm_arm_gic_set_irq(void *opaque, int irq, int level)
>      } else {
>          /* Internal interrupt: decode into (cpu, interrupt id) */
>          irqtype = KVM_ARM_IRQ_TYPE_PPI;
> -        irq -= (s->num_irq - GIC_INTERNAL);
> +        irq -= (num_irq - GIC_INTERNAL);
>          cpu = irq / GIC_INTERNAL;
>          irq %= GIC_INTERNAL;
>      }
> @@ -87,12 +87,19 @@ static void kvm_arm_gic_set_irq(void *opaque, int irq, int level)
>      kvm_set_irq(kvm_state, kvm_irq, !!level);
>  }
>
> +static void kvm_arm_gicv2_set_irq(void *opaque, int irq, int level)
> +{
> +    GICState *s = (GICState *)opaque;
> +
> +    kvm_arm_gic_set_irq(s->num_irq, irq, level);
> +}
> +
>  static bool kvm_arm_gic_can_save_restore(GICState *s)
>  {
>      return s->dev_fd >= 0;
>  }
>
> -static bool kvm_gic_supports_attr(GICState *s, int group, int attrnum)
> +bool kvm_gic_supports_attr(int dev_fd, int group, int attrnum)

If you want to make this public it would be better to call
it kvm_device_supports_attr() and put it in kvm-all.c, because
it's not really GIC specific.

>  {
>      struct kvm_device_attr attr = {
>          .group = group,
> @@ -100,14 +107,14 @@ static bool kvm_gic_supports_attr(GICState *s, int group, int attrnum)
>          .flags = 0,
>      };
>
> -    if (s->dev_fd == -1) {
> +    if (dev_fd == -1) {
>          return false;
>      }
>
> -    return kvm_device_ioctl(s->dev_fd, KVM_HAS_DEVICE_ATTR, &attr) == 0;
> +    return kvm_device_ioctl(dev_fd, KVM_HAS_DEVICE_ATTR, &attr) == 0;
>  }
>
> -static void kvm_gic_access(GICState *s, int group, int offset,
> +void kvm_gic_access(int dev_fd, int group, int offset,
>                                     int cpu, uint32_t *val, bool write)
>  {
>      struct kvm_device_attr attr;
> @@ -130,7 +137,7 @@ static void kvm_gic_access(GICState *s, int group, int offset,
>          type = KVM_GET_DEVICE_ATTR;
>      }
>
> -    err = kvm_device_ioctl(s->dev_fd, type, &attr);
> +    err = kvm_device_ioctl(dev_fd, type, &attr);
>      if (err < 0) {
>          fprintf(stderr, "KVM_{SET/GET}_DEVICE_ATTR failed: %s\n",
>                  strerror(-err));
> @@ -138,20 +145,6 @@ static void kvm_gic_access(GICState *s, int group, int offset,
>      }
>  }
>
> -static void kvm_gicd_access(GICState *s, int offset, int cpu,
> -                            uint32_t *val, bool write)
> -{
> -    kvm_gic_access(s, KVM_DEV_ARM_VGIC_GRP_DIST_REGS,
> -                   offset, cpu, val, write);
> -}
> -
> -static void kvm_gicc_access(GICState *s, int offset, int cpu,
> -                            uint32_t *val, bool write)
> -{
> -    kvm_gic_access(s, KVM_DEV_ARM_VGIC_GRP_CPU_REGS,
> -                   offset, cpu, val, write);
> -}

I don't think we gain much by making these two functions common,
and we do get a lot of churn in the existing code below.

> -
>  #define for_each_irq_reg(_ctr, _max_irq, _field_width) \
>      for (_ctr = 0; _ctr < ((_max_irq) / (32 / (_field_width))); _ctr++)
>
> @@ -291,7 +284,7 @@ static void kvm_dist_get(GICState *s, uint32_t offset, int width,
>          irq = i * regsz;
>          cpu = 0;
>          while ((cpu < s->num_cpu && irq < GIC_INTERNAL) || cpu == 0) {
> -            kvm_gicd_access(s, offset, cpu, &reg, false);
> +            kvm_gicd_access(s->dev_fd, offset, cpu, &reg, false);
>              for (j = 0; j < regsz; j++) {
>                  field = extract32(reg, j * width, width);
>                  translate_fn(s, irq + j, cpu, &field, false);
> @@ -324,7 +317,7 @@ static void kvm_dist_put(GICState *s, uint32_t offset, int width,
>                  translate_fn(s, irq + j, cpu, &field, true);
>                  reg = deposit32(reg, j * width, width, field);
>              }
> -            kvm_gicd_access(s, offset, cpu, &reg, true);
> +            kvm_gicd_access(s->dev_fd, offset, cpu, &reg, true);
>
>              cpu++;
>          }
> @@ -355,10 +348,10 @@ static void kvm_arm_gic_put(GICState *s)
>
>      /* s->ctlr -> GICD_CTLR */
>      reg = s->ctlr;
> -    kvm_gicd_access(s, 0x0, 0, &reg, true);
> +    kvm_gicd_access(s->dev_fd, 0x0, 0, &reg, true);
>
>      /* Sanity checking on GICD_TYPER and s->num_irq, s->num_cpu */
> -    kvm_gicd_access(s, 0x4, 0, &reg, false);
> +    kvm_gicd_access(s->dev_fd, 0x4, 0, &reg, false);
>      num_irq = ((reg & 0x1f) + 1) * 32;
>      num_cpu = ((reg & 0xe0) >> 5) + 1;
>
> @@ -416,24 +409,24 @@ static void kvm_arm_gic_put(GICState *s)
>      for (cpu = 0; cpu < s->num_cpu; cpu++) {
>          /* s->cpu_ctlr[cpu] -> GICC_CTLR */
>          reg = s->cpu_ctlr[cpu];
> -        kvm_gicc_access(s, 0x00, cpu, &reg, true);
> +        kvm_gicc_access(s->dev_fd, 0x00, cpu, &reg, true);
>
>          /* s->priority_mask[cpu] -> GICC_PMR */
>          reg = (s->priority_mask[cpu] & 0xff);
> -        kvm_gicc_access(s, 0x04, cpu, &reg, true);
> +        kvm_gicc_access(s->dev_fd, 0x04, cpu, &reg, true);
>
>          /* s->bpr[cpu] -> GICC_BPR */
>          reg = (s->bpr[cpu] & 0x7);
> -        kvm_gicc_access(s, 0x08, cpu, &reg, true);
> +        kvm_gicc_access(s->dev_fd, 0x08, cpu, &reg, true);
>
>          /* s->abpr[cpu] -> GICC_ABPR */
>          reg = (s->abpr[cpu] & 0x7);
> -        kvm_gicc_access(s, 0x1c, cpu, &reg, true);
> +        kvm_gicc_access(s->dev_fd, 0x1c, cpu, &reg, true);
>
>          /* s->apr[n][cpu] -> GICC_APRn */
>          for (i = 0; i < 4; i++) {
>              reg = s->apr[i][cpu];
> -            kvm_gicc_access(s, 0xd0 + i * 4, cpu, &reg, true);
> +            kvm_gicc_access(s->dev_fd, 0xd0 + i * 4, cpu, &reg, true);
>          }
>      }
>  }
> @@ -454,11 +447,11 @@ static void kvm_arm_gic_get(GICState *s)
>       */
>
>      /* GICD_CTLR -> s->ctlr */
> -    kvm_gicd_access(s, 0x0, 0, &reg, false);
> +    kvm_gicd_access(s->dev_fd, 0x0, 0, &reg, false);
>      s->ctlr = reg;
>
>      /* Sanity checking on GICD_TYPER -> s->num_irq, s->num_cpu */
> -    kvm_gicd_access(s, 0x4, 0, &reg, false);
> +    kvm_gicd_access(s->dev_fd, 0x4, 0, &reg, false);
>      s->num_irq = ((reg & 0x1f) + 1) * 32;
>      s->num_cpu = ((reg & 0xe0) >> 5) + 1;
>
> @@ -469,7 +462,7 @@ static void kvm_arm_gic_get(GICState *s)
>      }
>
>      /* GICD_IIDR -> ? */
> -    kvm_gicd_access(s, 0x8, 0, &reg, false);
> +    kvm_gicd_access(s->dev_fd, 0x8, 0, &reg, false);
>
>      /* Clear all the IRQ settings */
>      for (i = 0; i < s->num_irq; i++) {
> @@ -507,24 +500,24 @@ static void kvm_arm_gic_get(GICState *s)
>
>      for (cpu = 0; cpu < s->num_cpu; cpu++) {
>          /* GICC_CTLR -> s->cpu_ctlr[cpu] */
> -        kvm_gicc_access(s, 0x00, cpu, &reg, false);
> +        kvm_gicc_access(s->dev_fd, 0x00, cpu, &reg, false);
>          s->cpu_ctlr[cpu] = reg;
>
>          /* GICC_PMR -> s->priority_mask[cpu] */
> -        kvm_gicc_access(s, 0x04, cpu, &reg, false);
> +        kvm_gicc_access(s->dev_fd, 0x04, cpu, &reg, false);
>          s->priority_mask[cpu] = (reg & 0xff);
>
>          /* GICC_BPR -> s->bpr[cpu] */
> -        kvm_gicc_access(s, 0x08, cpu, &reg, false);
> +        kvm_gicc_access(s->dev_fd, 0x08, cpu, &reg, false);
>          s->bpr[cpu] = (reg & 0x7);
>
>          /* GICC_ABPR -> s->abpr[cpu] */
> -        kvm_gicc_access(s, 0x1c, cpu, &reg, false);
> +        kvm_gicc_access(s->dev_fd, 0x1c, cpu, &reg, false);
>          s->abpr[cpu] = (reg & 0x7);
>
>          /* GICC_APRn -> s->apr[n][cpu] */
>          for (i = 0; i < 4; i++) {
> -            kvm_gicc_access(s, 0xd0 + i * 4, cpu, &reg, false);
> +            kvm_gicc_access(s->dev_fd, 0xd0 + i * 4, cpu, &reg, false);
>              s->apr[i][cpu] = reg;
>          }
>      }
> @@ -559,7 +552,7 @@ static void kvm_arm_gic_realize(DeviceState *dev, Error **errp)
>          return;
>      }
>
> -    gic_init_irqs_and_mmio(s, kvm_arm_gic_set_irq, NULL);
> +    gic_init_irqs_and_mmio(s, kvm_arm_gicv2_set_irq, NULL);
>
>      for (i = 0; i < s->num_irq - GIC_INTERNAL; i++) {
>          qemu_irq irq = qdev_get_gpio_in(dev, i);
> @@ -576,15 +569,16 @@ static void kvm_arm_gic_realize(DeviceState *dev, Error **errp)
>          return;
>      }
>
> -    if (kvm_gic_supports_attr(s, KVM_DEV_ARM_VGIC_GRP_NR_IRQS, 0)) {
> +    if (kvm_gic_supports_attr(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_NR_IRQS, 0)) {
>          uint32_t numirqs = s->num_irq;
> -        kvm_gic_access(s, KVM_DEV_ARM_VGIC_GRP_NR_IRQS, 0, 0, &numirqs, 1);
> +        kvm_gic_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_NR_IRQS, 0, 0,
> +                       &numirqs, 1);
>      }
>
>      /* Tell the kernel to complete VGIC initialization now */
> -    if (kvm_gic_supports_attr(s, KVM_DEV_ARM_VGIC_GRP_CTRL,
> +    if (kvm_gic_supports_attr(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_CTRL,
>                                KVM_DEV_ARM_VGIC_CTRL_INIT)) {
> -        kvm_gic_access(s, KVM_DEV_ARM_VGIC_GRP_CTRL,
> +        kvm_gic_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_CTRL,
>                            KVM_DEV_ARM_VGIC_CTRL_INIT, 0, 0, 1);
>      }
>
> diff --git a/hw/intc/vgic_common.h b/hw/intc/vgic_common.h
> new file mode 100644
> index 0000000..67241d3
> --- /dev/null
> +++ b/hw/intc/vgic_common.h
> @@ -0,0 +1,43 @@
> +/*
> + * ARM KVM vGIC utility functions
> + *
> + * Copyright (c) 2015 Samsung Electronics
> + * Written by Pavel Fedin
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation, either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#ifndef QEMU_ARM_VGIC_COMMON_H
> +#define QEMU_ARM_VGIC_COMMON_H
> +
> +void kvm_arm_gic_set_irq(uint32_t num_irq, int irq, int level);
> +bool kvm_gic_supports_attr(int dev_fd, int group, int attrnum);
> +void kvm_gic_access(int dev_fd, int group, int offset, int cpu,
> +                    uint32_t *val, bool write);

Any new functions declared in header files should have documentation
comments in the usual format.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v8 2/5] Extract some reusable vGIC code

[Pavel Fedin]

 Hello!

> The need to add num_irq to this prototype reveals the ugliness
> of it as an interface.

> I don't think we gain much by making these two functions common,
> and we do get a lot of churn in the existing code below.

 You know, i was also thinking about it. And actually there is a way to resolve this: make some common structure holding num_irqs, num_cpus and dev_fd, and share it between GICv2 and GICv3 code. But wouldn't this be ugly too? So, i decided to make some compromise and leave it this way, at least for now.
 Regarding the latter two functions, i thought that they are useful for future live migration, aren't they? 

> It would be much nicer to convert the
> GIC to using named GPIO arrays for its incoming interrupt
> lines, so that you can handle the different cases of SPIs
> and PPIs naturally rather than exposing the awkward mapping
> from multiple different sets of interrupts into a single
> integer space.

 No no, not now. I believe this would require lots if changes in all machne models, i'm not ready for this...

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

Re: [Qemu-devel] [PATCH v8 1/5] Implement GIC-500 base class

[Pavel Fedin]

> > +#define GICV3_NCPU 64
> 
> What is imposing this NCPU limit?

 Currently qemu does not support Aff2 field. Can we have more than 64 CPUs only with Aff0 and Aff1?
 Well, if you really-really insist, i can just raise it to 128 and stop it finally. It's actually Shlomo's heritage, and i believe his SW emulation will decrease this down to 64 again. It can be changed at any moment, i don't see any serious obstacles for it. You ask this question for the 3rd time IIRC, and i answer the same thing for the 3rd time. Does this little #define really worth it?
 Shlomo, your word?

> > +    qemu_irq parent_irq[GICV3_NCPU];
> > +    qemu_irq parent_fiq[GICV3_NCPU];
> 
> We should be allocating memory for the right number of irqs
> and fiqs based on the number of actual CPUs, because we don't
> really want a compile-time arbitrary limit on NCPUs.

 These arrays are just placeholders, so we reserve the maximum possible space for them. When we initialize the actual objects, of course we use only 'num_cpu' slots in each of the array.
 sizeof(qemu_irq) is rather small (IIRC it's a pointer accompanied by number). Does it worth g_malloc()ing them dynamically?

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

Re: [Qemu-devel] [PATCH v8 1/5] Implement GIC-500 base class

[Peter Maydell]

On 11 August 2015 at 08:53, Pavel Fedin <p.fedin@samsung.com> wrote:
>> > +#define GICV3_NCPU 64
>>
>> What is imposing this NCPU limit?
>
>  Currently qemu does not support Aff2 field. Can we have more
> than 64 CPUs only with Aff0 and Aff1?

The GIC code itself doesn't care, so it shouldn't be imposing
its own limit, even if other parts of the code do for now.

>  Well, if you really-really insist, i can just raise it to 128

...and where does 128 come from?

> and stop it finally. It's actually Shlomo's heritage, and i believe
> his SW emulation will decrease this down to 64 again.

No it won't, because "don't impose an arbitrary 64 bit limit"
was one of my review comments on the emulation code; that
will need to be fixed before the emulation code can be accepted.

> It can be changed at any moment, i don't see any serious obstacles
> for it. You ask this question for the 3rd time IIRC, and i answer
> the same thing for the 3rd time.

Yes, if you don't make changes that I think are needed then I'll
continue to point them out in future review rounds...

Part of the aim of the GICv3 code is to not have an arbitrary
limit on the number of vcpus we can have. In the GICv2 code
we have a #define for the maximum number of CPUs, because the
GICv2 architecture itself sets that maximum. For GICv3 I don't
want to bake an arbitrary limit into the code if it's not a
limit imposed by the GICv3 architecture.

>> > +    qemu_irq parent_irq[GICV3_NCPU];
>> > +    qemu_irq parent_fiq[GICV3_NCPU];
>>
>> We should be allocating memory for the right number of irqs
>> and fiqs based on the number of actual CPUs, because we don't
>> really want a compile-time arbitrary limit on NCPUs.
>
>  These arrays are just placeholders, so we reserve the maximum
> possible space for them. When we initialize the actual objects,
> of course we use only 'num_cpu' slots in each of the array.
>  sizeof(qemu_irq) is rather small (IIRC it's a pointer accompanied
> by number). Does it worth g_malloc()ing them dynamically?

It's not the memory usage so much as that if NCPU isn't a
compile time constant you can't have them be plain arrays.

-- PMM

Re: [Qemu-devel] [PATCH v8 1/5] Implement GIC-500 base class

[Pavel Fedin]

 Hello!

> No it won't, because "don't impose an arbitrary 64 bit limit"
> was one of my review comments on the emulation code; that
> will need to be fixed before the emulation code can be accepted.

 Sorry for may be being ignorant, i really had no time to read GICv3 arch manual from beginning to end. Do you want to tell me that GICv3 architecture puts no limit at all on CPU number, so i could have 128, 1024, 134435242, etc ?

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

Re: [Qemu-devel] [PATCH v8 1/5] Implement GIC-500 base class

[Peter Maydell]

On 11 August 2015 at 10:35, Pavel Fedin <p.fedin@samsung.com> wrote:
>  Hello!
>
>> No it won't, because "don't impose an arbitrary 64 bit limit"
>> was one of my review comments on the emulation code; that
>> will need to be fixed before the emulation code can be accepted.
>
>  Sorry for may be being ignorant, i really had no time to read GICv3
> arch manual from beginning to end. Do you want to tell me that GICv3
> architecture puts no limit at all on CPU number, so i could have 128,
> 1024, 134435242, etc ?

Not as far as I know, beyond the limitations on the Affinity
fields (Aff0 recommended to be 0..15, and the total number
allowed via all 4 affinity fields). In any case, if you want
to impose a compile-time limit in the QEMU code then you need
to point out the part of the GIC spec that imposes that limit.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v8 1/5] Implement GIC-500 base class

[Pavel Fedin]

 Hello!

> In any case, if you want
> to impose a compile-time limit in the QEMU code then you need
> to point out the part of the GIC spec that imposes that limit.

 Ok, i agreed and gave up. Will do in v9. :)
 By the way, how to migrate such a thing? Is migration of variable-length state structures supported?

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

Re: [Qemu-devel] [PATCH v8 1/5] Implement GIC-500 base class

[Peter Maydell]

On 11 August 2015 at 11:39, Pavel Fedin <p.fedin@samsung.com> wrote:
>  By the way, how to migrate such a thing? Is migration of
> variable-length state structures supported?

Yes; this is what the _VARRAY_ vmstate macros are for.

-- PMM

Re: [Qemu-devel] [PATCH v8 3/5] Introduce irqchip type specification for KVM

[Pavel Fedin]

 Hello!

> I still think this is the wrong approach -- see my remarks
> in the previous round of patch review.

 Christoffer did not reply anything to your question back then. So - what to do? Probe for all possible GICs? Remove the probe at all?

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

Re: [Qemu-devel] [PATCH v8 3/5] Introduce irqchip type specification for KVM

[Christoffer Dall]

On Wed, Aug 12, 2015 at 1:44 PM, Pavel Fedin <p.fedin@samsung.com> wrote:
>  Hello!
>
>> I still think this is the wrong approach -- see my remarks
>> in the previous round of patch review.
>
>  Christoffer did not reply anything to your question back then. So - what to do? Probe for all possible GICs? Remove the probe at all?
>
I may have missed something here, what was the question?

-Christoffer

Re: [Qemu-devel] [PATCH v8 3/5] Introduce irqchip type specification for KVM

[Pavel Fedin]

 Hello!

> I still think this is the wrong approach -- see my remarks
> in the previous round of patch review.

 You know... I thought a little bit...
 So far, test = true in KVM_CREATE_DEVICE means that we just want to know whether this type is supported. No actual actions is done by the kernel. Is it correct? If yes, we can just leave this test as it is, because if it says that GICv2 is supported by KVM_CREATE_DEVICE, then:
1. We use new API. No KVM_IRQCHIP_CREATE.
2. GICv3 may be supported.

 Therefore, if we do this check, and it succeeds, then we just proceed, and later actually try to create GICv3. If it fails for some reason, we will see error message anyway. So would it be OK just not to touch kvm_arch_irqchip_create() at all?

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

Re: [Qemu-devel] [PATCH v8 3/5] Introduce irqchip type specification for KVM

[Peter Maydell]

On 12 August 2015 at 13:27, Pavel Fedin <p.fedin@samsung.com> wrote:
>  Hello!
>
>> I still think this is the wrong approach -- see my remarks
>> in the previous round of patch review.
>
>  You know... I thought a little bit...
>  So far, test = true in KVM_CREATE_DEVICE means that we just want to know whether this type is supported. No actual actions is done by the kernel. Is it correct? If yes, we can just leave this test as it is, because if it says that GICv2 is supported by KVM_CREATE_DEVICE, then:
> 1. We use new API. No KVM_IRQCHIP_CREATE.
> 2. GICv3 may be supported.
>
>  Therefore, if we do this check, and it succeeds, then we just
> proceed, and later actually try to create GICv3. If it fails for some
> reason, we will see error message anyway. So would it be OK just not
> to touch kvm_arch_irqchip_create() at all?

No, because then if the kernel only supports GICv3 the
code in kvm_arch_irqchip_create() (as it is currently written)
will erroneously fall back to using the old API.

Christoffer: the question was, why does kvm_arch_irqchip_create()
not only check the KVM_CAP_DEVICE_CTRL but also try to see
if it can KVM_CREATE_DEVICE the GICv2 in order to avoid
falling back to the old pre-KVM_CREATE_DEVICE API ? Are there
kernels which have the capability bit set but which can't
actually use KVM_CREATE_DEVICE to create the irqchip?

My preference here would be for kvm_arch_irqchip_create()
to just use 'is the KVM_CAP_DEVICE_CTRL capability set'
for its "can we use the new API" test; that will then
work whether we have a GICv2 or GICv3 in the host. (The
actual GIC device creation later on might fail, of course,
but that can be handled at that point. The only thing
we need to do as early as kvm_arch_irqchip_create is
determine whether we must use the old API.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH v8 3/5] Introduce irqchip type specification for KVM

[Christoffer Dall]

On Wed, Aug 12, 2015 at 2:59 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 12 August 2015 at 13:27, Pavel Fedin <p.fedin@samsung.com> wrote:
>>  Hello!
>>
>>> I still think this is the wrong approach -- see my remarks
>>> in the previous round of patch review.
>>
>>  You know... I thought a little bit...
>>  So far, test = true in KVM_CREATE_DEVICE means that we just want to know whether this type is supported. No actual actions is done by the kernel. Is it correct? If yes, we can just leave this test as it is, because if it says that GICv2 is supported by KVM_CREATE_DEVICE, then:
>> 1. We use new API. No KVM_IRQCHIP_CREATE.
>> 2. GICv3 may be supported.
>>
>>  Therefore, if we do this check, and it succeeds, then we just
>> proceed, and later actually try to create GICv3. If it fails for some
>> reason, we will see error message anyway. So would it be OK just not
>> to touch kvm_arch_irqchip_create() at all?
>
> No, because then if the kernel only supports GICv3 the
> code in kvm_arch_irqchip_create() (as it is currently written)
> will erroneously fall back to using the old API.
>
> Christoffer: the question was, why does kvm_arch_irqchip_create()
> not only check the KVM_CAP_DEVICE_CTRL but also try to see
> if it can KVM_CREATE_DEVICE the GICv2 in order to avoid
> falling back to the old pre-KVM_CREATE_DEVICE API ? Are there
> kernels which have the capability bit set but which can't
> actually use KVM_CREATE_DEVICE to create the irqchip?

My thinking probably was that technically the KVM_CAP_DEVICE_CTRL is
an orthogonal concept from how to create the vgic, and you could
advertise this capability without also supporting the GICv2 device
type.

However, I don't believe such kernels exist and they cannot in the
future as that would be because we would remove an actively supported
API.

>
> My preference here would be for kvm_arch_irqchip_create()
> to just use 'is the KVM_CAP_DEVICE_CTRL capability set'
> for its "can we use the new API" test; that will then
> work whether we have a GICv2 or GICv3 in the host. (The
> actual GIC device creation later on might fail, of course,
> but that can be handled at that point. The only thing
> we need to do as early as kvm_arch_irqchip_create is
> determine whether we must use the old API.)
>
I'm fine with this.

-Christoffer

Re: [Qemu-devel] [PATCH v8 3/5] Introduce irqchip type specification for KVM

[Eric Auger]

Hi,
On 08/12/2015 03:23 PM, Christoffer Dall wrote:
> On Wed, Aug 12, 2015 at 2:59 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 12 August 2015 at 13:27, Pavel Fedin <p.fedin@samsung.com> wrote:
>>>  Hello!
>>>
>>>> I still think this is the wrong approach -- see my remarks
>>>> in the previous round of patch review.
>>>
>>>  You know... I thought a little bit...
>>>  So far, test = true in KVM_CREATE_DEVICE means that we just want to know whether this type is supported. No actual actions is done by the kernel. Is it correct? 
yes
If yes, we can just leave this test as it is, because if it says that
GICv2 is supported by KVM_CREATE_DEVICE, then:
>>> 1. We use new API. No KVM_IRQCHIP_CREATE.
>>> 2. GICv3 may be supported.
>>>
>>>  Therefore, if we do this check, and it succeeds, then we just
>>> proceed, and later actually try to create GICv3. If it fails for some
>>> reason, we will see error message anyway. So would it be OK just not
>>> to touch kvm_arch_irqchip_create() at all?
>>
>> No, because then if the kernel only supports GICv3 the
>> code in kvm_arch_irqchip_create() (as it is currently written)
>> will erroneously fall back to using the old API.
>>
>> Christoffer: the question was, why does kvm_arch_irqchip_create()
>> not only check the KVM_CAP_DEVICE_CTRL but also try to see
>> if it can KVM_CREATE_DEVICE the GICv2 in order to avoid
>> falling back to the old pre-KVM_CREATE_DEVICE API ? Are there
>> kernels which have the capability bit set but which can't
>> actually use KVM_CREATE_DEVICE to create the irqchip?
> 
> My thinking probably was that technically the KVM_CAP_DEVICE_CTRL is
> an orthogonal concept from how to create the vgic, and you could
> advertise this capability without also supporting the GICv2 device
> type.
> 
> However, I don't believe such kernels exist
the capability was advertised for arm/arm64 with GICv2
7330672  KVM: arm-vgic: Support KVM_CREATE_DEVICE for VGIC (1 year, 8
months ago) <Christoffer Dall>
so effectively I don't think we have any arm kernel advertising the CAP
without GICv3.

 and they cannot in the
> future as that would be because we would remove an actively supported
> API.
> 
>>
>> My preference here would be for kvm_arch_irqchip_create()
>> to just use 'is the KVM_CAP_DEVICE_CTRL capability set'
>> for its "can we use the new API" test; that will then
>> work whether we have a GICv2 or GICv3 in the host. (The
>> actual GIC device creation later on might fail, of course,
>> but that can be handled at that point. The only thing
>> we need to do as early as kvm_arch_irqchip_create is
>> determine whether we must use the old API.)
another way was proposed in the past was consisting in calling first
ret = kvm_create_device(s, KVM_DEV_TYPE_ARM_VGIC_V3, true);
if this succeeds this means we have the new API (the only one used vy
v3) and hence we have it as well for VGIC_V2, we can return ...
if this fails we just can say VGICv3 KVM device hasn't registered, try
KVM_DEV_TYPE_ARM_VGIC_V2
if we have it return else fall back to older API

I think it worked as well?

Besides I think Peter's suggestion is simpler.

For info we also use KVM VFIO device now.

Best Regards

Eric
>>
> I'm fine with this.
> 
> -Christoffer
> 

Re: [Qemu-devel] [PATCH v8 3/5] Introduce irqchip type specification for KVM

[Christoffer Dall]

On Wed, Aug 12, 2015 at 4:14 PM, Eric Auger <eric.auger@linaro.org> wrote:
> Hi,
> On 08/12/2015 03:23 PM, Christoffer Dall wrote:
>> On Wed, Aug 12, 2015 at 2:59 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> On 12 August 2015 at 13:27, Pavel Fedin <p.fedin@samsung.com> wrote:
>>>>  Hello!
>>>>
>>>>> I still think this is the wrong approach -- see my remarks
>>>>> in the previous round of patch review.
>>>>
>>>>  You know... I thought a little bit...
>>>>  So far, test = true in KVM_CREATE_DEVICE means that we just want to know whether this type is supported. No actual actions is done by the kernel. Is it correct?
> yes
> If yes, we can just leave this test as it is, because if it says that
> GICv2 is supported by KVM_CREATE_DEVICE, then:
>>>> 1. We use new API. No KVM_IRQCHIP_CREATE.
>>>> 2. GICv3 may be supported.
>>>>
>>>>  Therefore, if we do this check, and it succeeds, then we just
>>>> proceed, and later actually try to create GICv3. If it fails for some
>>>> reason, we will see error message anyway. So would it be OK just not
>>>> to touch kvm_arch_irqchip_create() at all?
>>>
>>> No, because then if the kernel only supports GICv3 the
>>> code in kvm_arch_irqchip_create() (as it is currently written)
>>> will erroneously fall back to using the old API.
>>>
>>> Christoffer: the question was, why does kvm_arch_irqchip_create()
>>> not only check the KVM_CAP_DEVICE_CTRL but also try to see
>>> if it can KVM_CREATE_DEVICE the GICv2 in order to avoid
>>> falling back to the old pre-KVM_CREATE_DEVICE API ? Are there
>>> kernels which have the capability bit set but which can't
>>> actually use KVM_CREATE_DEVICE to create the irqchip?
>>
>> My thinking probably was that technically the KVM_CAP_DEVICE_CTRL is
>> an orthogonal concept from how to create the vgic, and you could
>> advertise this capability without also supporting the GICv2 device
>> type.
>>
>> However, I don't believe such kernels exist
> the capability was advertised for arm/arm64 with GICv2
> 7330672  KVM: arm-vgic: Support KVM_CREATE_DEVICE for VGIC (1 year, 8
> months ago) <Christoffer Dall>
> so effectively I don't think we have any arm kernel advertising the CAP
> without GICv3.
>
>  and they cannot in the
>> future as that would be because we would remove an actively supported
>> API.
>>
>>>
>>> My preference here would be for kvm_arch_irqchip_create()
>>> to just use 'is the KVM_CAP_DEVICE_CTRL capability set'
>>> for its "can we use the new API" test; that will then
>>> work whether we have a GICv2 or GICv3 in the host. (The
>>> actual GIC device creation later on might fail, of course,
>>> but that can be handled at that point. The only thing
>>> we need to do as early as kvm_arch_irqchip_create is
>>> determine whether we must use the old API.)
> another way was proposed in the past was consisting in calling first
> ret = kvm_create_device(s, KVM_DEV_TYPE_ARM_VGIC_V3, true);
> if this succeeds this means we have the new API (the only one used vy
> v3) and hence we have it as well for VGIC_V2, we can return ...
> if this fails we just can say VGICv3 KVM device hasn't registered, try
> KVM_DEV_TYPE_ARM_VGIC_V2
> if we have it return else fall back to older API
>
> I think it worked as well?
>
> Besides I think Peter's suggestion is simpler.
>
> For info we also use KVM VFIO device now.
>
Note that there's a difference between "I called the create-device
ioctl, and the creation of the device failed" vs. "I called the ioctl
and I got an error because the ioctl is not supported", only in the
latter case should you fall back to the older API.

Not sure if the end result is the same with the suggested approach,
based on the right error values etc.

-Christoffer

Re: [Qemu-devel] [PATCH v8 3/5] Introduce irqchip type specification for KVM

[Eric Auger]

On 08/12/2015 04:24 PM, Christoffer Dall wrote:
> On Wed, Aug 12, 2015 at 4:14 PM, Eric Auger <eric.auger@linaro.org> wrote:
>> Hi,
>> On 08/12/2015 03:23 PM, Christoffer Dall wrote:
>>> On Wed, Aug 12, 2015 at 2:59 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>>> On 12 August 2015 at 13:27, Pavel Fedin <p.fedin@samsung.com> wrote:
>>>>>  Hello!
>>>>>
>>>>>> I still think this is the wrong approach -- see my remarks
>>>>>> in the previous round of patch review.
>>>>>
>>>>>  You know... I thought a little bit...
>>>>>  So far, test = true in KVM_CREATE_DEVICE means that we just want to know whether this type is supported. No actual actions is done by the kernel. Is it correct?
>> yes
>> If yes, we can just leave this test as it is, because if it says that
>> GICv2 is supported by KVM_CREATE_DEVICE, then:
>>>>> 1. We use new API. No KVM_IRQCHIP_CREATE.
>>>>> 2. GICv3 may be supported.
>>>>>
>>>>>  Therefore, if we do this check, and it succeeds, then we just
>>>>> proceed, and later actually try to create GICv3. If it fails for some
>>>>> reason, we will see error message anyway. So would it be OK just not
>>>>> to touch kvm_arch_irqchip_create() at all?
>>>>
>>>> No, because then if the kernel only supports GICv3 the
>>>> code in kvm_arch_irqchip_create() (as it is currently written)
>>>> will erroneously fall back to using the old API.
>>>>
>>>> Christoffer: the question was, why does kvm_arch_irqchip_create()
>>>> not only check the KVM_CAP_DEVICE_CTRL but also try to see
>>>> if it can KVM_CREATE_DEVICE the GICv2 in order to avoid
>>>> falling back to the old pre-KVM_CREATE_DEVICE API ? Are there
>>>> kernels which have the capability bit set but which can't
>>>> actually use KVM_CREATE_DEVICE to create the irqchip?
>>>
>>> My thinking probably was that technically the KVM_CAP_DEVICE_CTRL is
>>> an orthogonal concept from how to create the vgic, and you could
>>> advertise this capability without also supporting the GICv2 device
>>> type.
>>>
>>> However, I don't believe such kernels exist
>> the capability was advertised for arm/arm64 with GICv2
>> 7330672  KVM: arm-vgic: Support KVM_CREATE_DEVICE for VGIC (1 year, 8
>> months ago) <Christoffer Dall>
>> so effectively I don't think we have any arm kernel advertising the CAP
>> without GICv3.
>>
>>  and they cannot in the
>>> future as that would be because we would remove an actively supported
>>> API.
>>>
>>>>
>>>> My preference here would be for kvm_arch_irqchip_create()
>>>> to just use 'is the KVM_CAP_DEVICE_CTRL capability set'
>>>> for its "can we use the new API" test; that will then
>>>> work whether we have a GICv2 or GICv3 in the host. (The
>>>> actual GIC device creation later on might fail, of course,
>>>> but that can be handled at that point. The only thing
>>>> we need to do as early as kvm_arch_irqchip_create is
>>>> determine whether we must use the old API.)
>> another way was proposed in the past was consisting in calling first
>> ret = kvm_create_device(s, KVM_DEV_TYPE_ARM_VGIC_V3, true);
>> if this succeeds this means we have the new API (the only one used vy
>> v3) and hence we have it as well for VGIC_V2, we can return ...
>> if this fails we just can say VGICv3 KVM device hasn't registered, try
>> KVM_DEV_TYPE_ARM_VGIC_V2
>> if we have it return else fall back to older API
>>
>> I think it worked as well?
>>
>> Besides I think Peter's suggestion is simpler.
>>
>> For info we also use KVM VFIO device now.
>>
> Note that there's a difference between "I called the create-device
> ioctl, and the creation of the device failed" vs. "I called the ioctl
> and I got an error because the ioctl is not supported", only in the
> latter case should you fall back to the older API.
yes understood; practically kvm_ioctl_create_device used in test mode
just looks for the specific device in the registered KVM device list and
that's it. whatever the case, API not supported or device not found it
reports -ENODEV.

Eric
> 
> Not sure if the end result is the same with the suggested approach,
> based on the right error values etc.
> 
> -Christoffer
> 

Re: [Qemu-devel] [PATCH v8 5/5] Add gicversion option to virt machine

[Pavel Fedin]

 Hello!

 I am now finishing v9 and (i hope) i fixed everything except this one. Sorry, too many mails, i occasionally skipped this one earlier.

> Any particular reason for having two separate VIRT_ITS_*
> entries? The spec mandates that the two 64K pages of ITS
> have to be consecutive, so it would make life easier for
> boards if they were just a single memory region.

 Yes, there is a reason. It is because of how in-kernel vITS works. It handles only control region. Translation register has to be handled in userspace and writes there need to be converted to KVM_SIGNAL_MSI ioctl. Therefore two regions are more convenient to use.

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia