From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51081) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Yuh11-0000Np-VX for qemu-devel@nongnu.org; Tue, 19 May 2015 08:53:03 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Yuh0x-0000P6-Sy for qemu-devel@nongnu.org; Tue, 19 May 2015 08:52:59 -0400 Received: from mail-wi0-f182.google.com ([209.85.212.182]:33215) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Yuh0x-0000Ot-Jm for qemu-devel@nongnu.org; Tue, 19 May 2015 08:52:55 -0400 Received: by wicmx19 with SMTP id mx19so115695812wic.0 for ; Tue, 19 May 2015 05:52:55 -0700 (PDT) Message-ID: <555B321F.2070102@linaro.org> Date: Tue, 19 May 2015 14:52:47 +0200 From: Eric Auger MIME-Version: 1.0 References: <1431624430-3996-1-git-send-email-ashoks@broadcom.com> <555A08FB.1030807@linaro.org> In-Reply-To: <555A08FB.1030807@linaro.org> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [RFC PATCH] hw/arm/virt: Added preliminary GICv3 support for kvm mode List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Ashok Kumar , qemu-devel@nongnu.org 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 >> --- >> 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; >> } >> >> >