On 05/18/2015 05:44 PM, Eric Auger wrote:
> 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 <ash...@broadcom.com>
>> ---
>> 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.
Hi Ashok,
so please ignore that last comment
Thanks
Eric
>
> Best Regards
>
> Eric
>> +
>> return 0;
>> }
>>
>>
>