Re: [Qemu-devel] [RFC PATCH] hw/arm/virt: Added preliminary GICv3 support for kvm mode

[Eric Auger]

Hi Ashok,
On 05/14/2015 07:27 PM, Ashok Kumar wrote:
> Added -M virt,gicversion=2,3 property to configure GICv2 or GICv3.
> GICv3 save/restore is not supported as vgic-v3-emul.c is yet to support
> them.
> 
> Signed-off-by: Ashok Kumar <address@hidden>
> ---
> Tested KVM/GICv3 in ARM fastmodel.
> Tested TCG/GICv2.
> Not tested KVM/GICv2.

I reproduced your test case on fast model too and it boots fine. I
encountered a small compilation issue related to arm_gic_init_memory
proto (needed a const char *name).

As a general comment you should run checkpatch.pl since you have qemu
style issues. Also the patch would deserve to be split into several
patch files and a cover letter would be needed I think with enriched
description. At least you could separate arm_gic_kvm additions from virt
ones, all the more so it could grow in the future...
> 
>  hw/arm/virt.c                    | 56 ++++++++++++++++++++++++++++++---
>  hw/intc/arm_gic_kvm.c            | 68 
> ++++++++++++++++++++++++++--------------
>  hw/intc/gic_internal.h           |  7 +++++
>  include/hw/intc/arm_gic_common.h |  5 ++-
>  target-arm/kvm.c                 |  5 +++
>  5 files changed, 111 insertions(+), 30 deletions(-)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 565f573..bb22d61 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -43,6 +43,7 @@
>  #include "qemu/bitops.h"
>  #include "qemu/error-report.h"
>  #include "hw/pci-host/gpex.h"
> +#include "qapi/visitor.h"
>  
>  #define NUM_VIRTIO_TRANSPORTS 32
>  
> @@ -87,6 +88,7 @@ typedef struct VirtBoardInfo {
>      void *fdt;
>      int fdt_size;
>      uint32_t clock_phandle;
> +    int gic_version;
I wonder whether it wouldn't be better located in VirtMachineState as
uint32_t and add a version uint32_t arg to fdt_add_gic_node & create_gic.

Or why not using a string property that you then convert into an enum
value. This would pave the way for GICv2m addition. This would also
remove the need for adding qapi/visitor.h I guess

>  } VirtBoardInfo;
>  
>  typedef struct {
> @@ -330,16 +332,22 @@ static uint32_t fdt_add_gic_node(const VirtBoardInfo 
> *vbi)
>      qemu_fdt_setprop_cell(vbi->fdt, "/", "interrupt-parent", 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");
> +    if (vbi->gic_version == 3)
> +        qemu_fdt_setprop_string(vbi->fdt, "/intc", "compatible",
> +                                "arm,gic-v3");
> +    else
> +        /* '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);
> +                                     2, vbi->gic_version == 3 ?
> +                                     vbi->smp_cpus * 0x20000 :
when reaching 128 (is it the max?) cores may overwrite VIRT_UART base?
> +                                     vbi->memmap[VIRT_GIC_CPU].size);
>      qemu_fdt_setprop_cell(vbi->fdt, "/intc", "phandle", gic_phandle);
>  
>      return gic_phandle;
> @@ -356,9 +364,13 @@ static uint32_t create_gic(const VirtBoardInfo *vbi, 
> qemu_irq *pic)
>      if (kvm_irqchip_in_kernel()) {
>          gictype = "kvm-arm-gic";
>      }
> +    else if (vbi->gic_version == 3) {
> +        fprintf (stderr, "GICv3 is not yet supported in tcg mode\n");
> +        exit (1);
> +    }
>  
>      gicdev = qdev_create(NULL, gictype);
> -    qdev_prop_set_uint32(gicdev, "revision", 2);
> +    qdev_prop_set_uint32(gicdev, "revision", vbi->gic_version);
>      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).
> @@ -853,6 +865,35 @@ 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)
> +{
> +    int64_t value = machines[0].gic_version;
> +
> +    visit_type_int(v, &value, name, errp);
> +
> +    return;
> +}
> +
> +static void virt_set_gic_version(Object *obj, Visitor *v, void *opaque,
> +                                 const char *name, Error **errp)
> +{
> +    int64_t value;
> +    int i;
> +
> +    visit_type_int(v, &value, name, errp);
> +
> +    if (value > 3 || value < 2) {
> +        error_report ("Only GICv2 and GICv3 supported currently\n");
> +        exit(1);
> +    }
> +
> +    for (i = 0; i < ARRAY_SIZE(machines); i++)
> +        machines[i].gic_version = value;
may be simplified if str prop and enum stored in VirtMachineState?
> +
> +    return;
> +}
> +
>  static void virt_instance_init(Object *obj)
>  {
>      VirtMachineState *vms = VIRT_MACHINE(obj);
> @@ -865,6 +906,11 @@ static void virt_instance_init(Object *obj)
>                                      "Set on/off to enable/disable the ARM "
>                                      "Security Extensions (TrustZone)",
>                                      NULL);
> +    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);
default value could be documented
>  }
>  
>  static void virt_class_init(ObjectClass *oc, void *data)
> diff --git a/hw/intc/arm_gic_kvm.c b/hw/intc/arm_gic_kvm.c
> index e1952ad..0027dca 100644
> --- a/hw/intc/arm_gic_kvm.c
> +++ b/hw/intc/arm_gic_kvm.c
> @@ -89,7 +89,8 @@ static void kvm_arm_gic_set_irq(void *opaque, int irq, int 
> level)
>  
>  static bool kvm_arm_gic_can_save_restore(GICState *s)
>  {
> -    return s->dev_fd >= 0;
> +    /* GICv3 doesn't support save restore yet */
> +    return s->dev_fd >= 0 && s->revision != REV_GICV3;
>  }
>  
>  static bool kvm_gic_supports_attr(GICState *s, int group, int attrnum)
> @@ -529,6 +530,28 @@ static void kvm_arm_gic_reset(DeviceState *dev)
>      kvm_arm_gic_put(s);
>  }
>  
> +static void arm_gic_init_memory (GICState *s, SysBusDevice *sbd,
kvm_arm_gic_init_memory?
> +                                        uint32_t gicver, uint32_t dist_t,
> +                                        uint32_t distsz, uint32_t mem_t,
> +                                        uint32_t memsz, MemoryRegion *mem,
> +                                        char *name)
> +{
> +    /* Distributor */
> +    memory_region_init_reservation(&s->iomem, OBJECT(s),
> +                                   "kvm-gic_dist", distsz);
> +    sysbus_init_mmio(sbd, &s->iomem);
> +
> +    kvm_arm_register_device(&s->iomem, (gicver << KVM_ARM_DEVICE_ID_SHIFT) | 
> dist_t,
> +                            KVM_DEV_ARM_VGIC_GRP_ADDR, dist_t, s->dev_fd);
> +    
> +    /* GICv2 - GICV cpu register interface region 
> +     * GICv3 - Redistributor register interface region */
> +    memory_region_init_reservation(mem, OBJECT(s),
> +                                   name, memsz);
> +    sysbus_init_mmio(sbd, mem);
> +    kvm_arm_register_device(mem, (gicver << KVM_ARM_DEVICE_ID_SHIFT) | mem_t,
> +                            KVM_DEV_ARM_VGIC_GRP_ADDR, mem_t, s->dev_fd);
> +}
>  static void kvm_arm_gic_realize(DeviceState *dev, Error **errp)
>  {
>      int i;
> @@ -563,7 +586,10 @@ static void kvm_arm_gic_realize(DeviceState *dev, Error 
> **errp)
>  
>      /* Try to create the device via the device control API */
>      s->dev_fd = -1;
> -    ret = kvm_create_device(kvm_state, KVM_DEV_TYPE_ARM_VGIC_V2, false);
> +    if (s->revision == REV_GICV3)
> +        ret = kvm_create_device(kvm_state, KVM_DEV_TYPE_ARM_VGIC_V3, false);
> +    else
> +        ret = kvm_create_device(kvm_state, KVM_DEV_TYPE_ARM_VGIC_V2, false);
>      if (ret >= 0) {
>          s->dev_fd = ret;
>      } else if (ret != -ENODEV && ret != -ENOTSUP) {
> @@ -583,29 +609,23 @@ static void kvm_arm_gic_realize(DeviceState *dev, Error 
> **errp)
>                            KVM_DEV_ARM_VGIC_CTRL_INIT, 0, 0, 1);
>      }
>  
> -    /* Distributor */
> -    memory_region_init_reservation(&s->iomem, OBJECT(s),
> -                                   "kvm-gic_dist", 0x1000);
> -    sysbus_init_mmio(sbd, &s->iomem);
> -    kvm_arm_register_device(&s->iomem,
> -                            (KVM_ARM_DEVICE_VGIC_V2 << 
> KVM_ARM_DEVICE_ID_SHIFT)
> -                            | KVM_VGIC_V2_ADDR_TYPE_DIST,
> -                            KVM_DEV_ARM_VGIC_GRP_ADDR,
> +    if (s->revision == REV_GICV3)
> +        arm_gic_init_memory(s, sbd, KVM_ARM_DEVICE_VGIC_V3,
> +                            KVM_VGIC_V3_ADDR_TYPE_DIST,
> +                            KVM_VGIC_V3_DIST_SIZE,
> +                            KVM_VGIC_V3_ADDR_TYPE_REDIST,
> +                            KVM_VGIC_V3_REDIST_SIZE,
> +                            &s->rdistiomem[0], "kvm-gic_rdist");
> +    else
> +        /* CPU interface for current core. Unlike arm_gic, we don't
> +         * provide the "interface for core #N" memory regions, because
> +         * cores with a VGIC don't have those.
> +         */
> +        arm_gic_init_memory(s, sbd, KVM_ARM_DEVICE_VGIC_V2,
>                              KVM_VGIC_V2_ADDR_TYPE_DIST,
> -                            s->dev_fd);
> -    /* CPU interface for current core. Unlike arm_gic, we don't
> -     * provide the "interface for core #N" memory regions, because
> -     * cores with a VGIC don't have those.
> -     */
> -    memory_region_init_reservation(&s->cpuiomem[0], OBJECT(s),
> -                                   "kvm-gic_cpu", 0x1000);
> -    sysbus_init_mmio(sbd, &s->cpuiomem[0]);
> -    kvm_arm_register_device(&s->cpuiomem[0],
> -                            (KVM_ARM_DEVICE_VGIC_V2 << 
> KVM_ARM_DEVICE_ID_SHIFT)
> -                            | KVM_VGIC_V2_ADDR_TYPE_CPU,
> -                            KVM_DEV_ARM_VGIC_GRP_ADDR,
> -                            KVM_VGIC_V2_ADDR_TYPE_CPU,
> -                            s->dev_fd);
> +                            0x1000,
KVM_VGIC_V2_DIST_SIZE?
 KVM_VGIC_V2_ADDR_TYPE_CPU,
> +                            0x1000,
KVM_VGIC_V2_DIST_SIZE?
 &s->cpuiomem[0],
> +                            "kvm-gic_cpu");
>  }
>  
>  static void kvm_arm_gic_class_init(ObjectClass *klass, void *data)
> diff --git a/hw/intc/gic_internal.h b/hw/intc/gic_internal.h
> index e87ef36..9f9246b 100644
> --- a/hw/intc/gic_internal.h
> +++ b/hw/intc/gic_internal.h
> @@ -53,8 +53,15 @@
>  
>  /* The special cases for the revision property: */
>  #define REV_11MPCORE 0
> +#define REV_GICV2     2
> +#define REV_GICV3     3
Wouldn't it make sense to include that in hw/intc/arm_gic.h to reuse
that in virt.c?

Best Regards

Eric
>  #define REV_NVIC 0xffffffff
>  
> +/* Not defined in kernel. Hence defining it here
> + * until it is done in kernel */
> +#define KVM_ARM_DEVICE_VGIC_V3               1
> +
> +#define SZ_64K 0x10000
>  void gic_set_pending_private(GICState *s, int cpu, int irq);
>  uint32_t gic_acknowledge_irq(GICState *s, int cpu);
>  void gic_complete_irq(GICState *s, int cpu, int irq);
> diff --git a/include/hw/intc/arm_gic_common.h 
> b/include/hw/intc/arm_gic_common.h
> index f6887ed..d9be6ad 100644
> --- a/include/hw/intc/arm_gic_common.h
> +++ b/include/hw/intc/arm_gic_common.h
> @@ -101,7 +101,10 @@ typedef struct GICState {
>       * both this GIC and which CPU interface we should be accessing.
>       */
>      struct GICState *backref[GIC_NCPU];
> -    MemoryRegion cpuiomem[GIC_NCPU + 1]; /* CPU interfaces */
> +    union {
> +        MemoryRegion cpuiomem[GIC_NCPU + 1]; /* CPU interfaces */
> +        MemoryRegion rdistiomem[GIC_NCPU + 1]; /* CPU interfaces */
> +    };
>      uint32_t num_irq;
>      uint32_t revision;
>      int dev_fd; /* kvm device fd if backed by kvm vgic support */
> diff --git a/target-arm/kvm.c b/target-arm/kvm.c
> index fdd9ba3..ce94f70 100644
> --- a/target-arm/kvm.c
> +++ b/target-arm/kvm.c
> @@ -590,6 +590,11 @@ int kvm_arch_irqchip_create(KVMState *s)
>          return 1;
>      }
>  
> +    ret = kvm_create_device(s, KVM_DEV_TYPE_ARM_VGIC_V3, true);
> +    if (ret == 0) {
> +        return 1;
> +    }
The 2d creation overwrites the 1st one at kernel level  (kvm_vgic_create
in vgic.c) so as Pavel said, we need to reconsider that part or call path.

Best Regards

Eric
> +
>      return 0;
>  }
>  
>