From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44410) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZAEzD-000531-1b for qemu-devel@nongnu.org; Wed, 01 Jul 2015 06:11:25 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZAEz7-0004MM-JR for qemu-devel@nongnu.org; Wed, 01 Jul 2015 06:11:22 -0400 Received: from mail-lb0-f171.google.com ([209.85.217.171]:36571) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZAEz7-0004MG-8j for qemu-devel@nongnu.org; Wed, 01 Jul 2015 06:11:17 -0400 Received: by lbbpo10 with SMTP id po10so9740845lbb.3 for ; Wed, 01 Jul 2015 03:11:15 -0700 (PDT) Date: Wed, 1 Jul 2015 12:11:17 +0200 From: Christoffer Dall Message-ID: <20150701101117.GA16763@cbox> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: Re: [Qemu-devel] [PATCH RFC 1/4] Add virt-v3 machine that uses GIC-500 List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Pavel Fedin Cc: Shlomo Pongratz , qemu-devel@nongnu.org, Ashok Kumar , Eric Auger On Fri, May 22, 2015 at 01:58:41PM +0300, Pavel Fedin wrote: > This patch introduces kernel_irqchip_type member in Machine class. Currently it it used only by virt machine for its internal purposes, however in future it is to be passed to KVM in kvm_irqchip_create(). The variable is defined as int in order to be architecture agnostic, for potential future users. Can you use a decent editor/mail agent and break your lines at 72 chars for commit messages please? This commit message is very code-specific and doesn't explain the overall change/purpose; for example I don't understand from reading this commit if the patch expects a new machine virt-v3 or using machine properties as discussed before. I think it's the former and I think you should re-spin these patches using a property, since Peter already clearly expressed how this should be done and it's no use reviewing something which we already know is not the right approach: https://lists.gnu.org/archive/html/qemu-devel/2015-05/msg02204.html Thanks, -Christoffer > > Signed-off-by: Pavel Fedin > > --- > hw/arm/virt.c | 148 +++++++++++++++++++++++++++++++++++++++++++--------- > include/hw/boards.h | 1 + > 2 files changed, 123 insertions(+), 26 deletions(-) > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c > index a1186c5..15724b2 100644 > --- a/hw/arm/virt.c > +++ b/hw/arm/virt.c > @@ -66,6 +66,10 @@ enum { > VIRT_CPUPERIPHS, > VIRT_GIC_DIST, > VIRT_GIC_CPU, > + VIRT_GIC_DIST_SPI = VIRT_GIC_CPU, > + VIRT_ITS_CONTROL, > + VIRT_ITS_TRANSLATION, > + VIRT_LPI, > VIRT_UART, > VIRT_MMIO, > VIRT_RTC, > @@ -107,6 +111,8 @@ typedef struct { > #define VIRT_MACHINE_CLASS(klass) \ > OBJECT_CLASS_CHECK(VirtMachineClass, klass, TYPE_VIRT_MACHINE) > > +#define TYPE_VIRTV3_MACHINE "virt-v3" > + > /* Addresses and sizes of our components. > * 0..128MB is space for a flash device so we can run bootrom code such as UEFI. > * 128MB..256MB is used for miscellaneous device I/O. > @@ -121,25 +127,29 @@ typedef struct { > */ > static const MemMapEntry a15memmap[] = { > /* Space up to 0x8000000 is reserved for a boot ROM */ > - [VIRT_FLASH] = { 0, 0x08000000 }, > - [VIRT_CPUPERIPHS] = { 0x08000000, 0x00020000 }, > + [VIRT_FLASH] = { 0, 0x08000000 }, > + [VIRT_CPUPERIPHS] = { 0x08000000, 0x00020000 }, > /* GIC distributor and CPU interfaces sit inside the CPU peripheral space */ > - [VIRT_GIC_DIST] = { 0x08000000, 0x00010000 }, > - [VIRT_GIC_CPU] = { 0x08010000, 0x00010000 }, > - [VIRT_UART] = { 0x09000000, 0x00001000 }, > - [VIRT_RTC] = { 0x09010000, 0x00001000 }, > - [VIRT_FW_CFG] = { 0x09020000, 0x0000000a }, > - [VIRT_MMIO] = { 0x0a000000, 0x00000200 }, > + [VIRT_GIC_DIST] = { 0x08000000, 0x00010000 }, > + [VIRT_GIC_CPU] = { 0x08010000, 0x00010000 }, > + /* On v3 VIRT_GIC_DIST_SPI takes place of VIRT_GIC_CPU */ > + [VIRT_ITS_CONTROL] = { 0x08020000, 0x00010000 }, > + [VIRT_ITS_TRANSLATION] = { 0x08030000, 0x00010000 }, > + [VIRT_LPI] = { 0x08040000, 0x00800000 }, > + [VIRT_UART] = { 0x09000000, 0x00001000 }, > + [VIRT_RTC] = { 0x09010000, 0x00001000 }, > + [VIRT_FW_CFG] = { 0x09020000, 0x0000000a }, > + [VIRT_MMIO] = { 0x0a000000, 0x00000200 }, > /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size */ > /* > * PCIE verbose map: > * > - * MMIO window { 0x10000000, 0x2eff0000 }, > - * PIO window { 0x3eff0000, 0x00010000 }, > - * ECAM { 0x3f000000, 0x01000000 }, > + * MMIO window { 0x10000000, 0x2eff0000 }, > + * PIO window { 0x3eff0000, 0x00010000 }, > + * ECAM { 0x3f000000, 0x01000000 }, > */ > - [VIRT_PCIE] = { 0x10000000, 0x30000000 }, > - [VIRT_MEM] = { 0x40000000, 30ULL * 1024 * 1024 * 1024 }, > + [VIRT_PCIE] = { 0x10000000, 0x30000000 }, > + [VIRT_MEM] = { 0x40000000, 30ULL * 1024 * 1024 * 1024 }, > }; > > static const int a15irqmap[] = { > @@ -273,9 +283,11 @@ static void fdt_add_timer_nodes(const VirtBoardInfo *vbi) > */ > ARMCPU *armcpu; > uint32_t irqflags = GIC_FDT_IRQ_FLAGS_EDGE_LO_HI; > + /* Argument is 32 bit but 8 bits are reserved for flags */ > + uint32_t 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"); > > @@ -299,6 +311,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); > @@ -326,7 +350,7 @@ static void fdt_add_cpu_nodes(const VirtBoardInfo *vbi) > } > } > > -static uint32_t fdt_add_gic_node(const VirtBoardInfo *vbi) > +static uint32_t fdt_add_gic_node(const VirtBoardInfo *vbi, int type) > { > uint32_t gic_phandle; > > @@ -334,35 +358,65 @@ 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"); > 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", > + if (type == KVM_DEV_TYPE_ARM_VGIC_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, > +#if 0 /* Currently no need for SPI & ITS */ > + 2, vbi->memmap[VIRT_GIC_DIST_SPI].base, > + 2, vbi->memmap[VIRT_GIC_DIST_SPI].size, > + 2, vbi->memmap[VIRT_ITS_CONTROL].base, > + 2, vbi->memmap[VIRT_ITS_CONTROL].size, > + 2, vbi->memmap[VIRT_ITS_TRANSLATION].base, > + 2, vbi->memmap[VIRT_ITS_TRANSLATION].size, > +#endif > + 2, vbi->memmap[VIRT_LPI].base, > + 2, vbi->memmap[VIRT_LPI].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", gic_phandle); > > return gic_phandle; > } > > -static uint32_t create_gic(const VirtBoardInfo *vbi, qemu_irq *pic) > +static uint32_t create_gic(const VirtBoardInfo *vbi, qemu_irq *pic, int type) > { > /* We create a standalone GIC v2 */ > DeviceState *gicdev; > SysBusDevice *gicbusdev; > - const char *gictype = "arm_gic"; > + const char *gictype; > int i; > > - if (kvm_irqchip_in_kernel()) { > + if (type == KVM_DEV_TYPE_ARM_VGIC_V3) { > + gictype = "arm_gicv3"; > + } else if (kvm_irqchip_in_kernel()) { > gictype = "kvm-arm-gic"; > + } else { > + gictype = "arm_gic"; > } > > 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; > + } > + > + qdev_prop_set_uint32(gicdev, "revision", > + type == KVM_DEV_TYPE_ARM_VGIC_V3 ? 3 : 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). > @@ -372,6 +426,11 @@ static uint32_t create_gic(const VirtBoardInfo *vbi, qemu_irq *pic) > 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 == KVM_DEV_TYPE_ARM_VGIC_V3) { > + sysbus_mmio_map(gicbusdev, 2, vbi->memmap[VIRT_ITS_CONTROL].base); > + sysbus_mmio_map(gicbusdev, 3, vbi->memmap[VIRT_ITS_TRANSLATION].base); > + sysbus_mmio_map(gicbusdev, 4, vbi->memmap[VIRT_LPI].base); > + } > > /* Wire the outputs from each CPU's generic timer to the > * appropriate GIC PPI inputs, and the GIC's IRQ output to > @@ -398,7 +457,7 @@ static uint32_t create_gic(const VirtBoardInfo *vbi, qemu_irq *pic) > pic[i] = qdev_get_gpio_in(gicdev, i); > } > > - return fdt_add_gic_node(vbi); > + return fdt_add_gic_node(vbi, type); > } > > static void create_uart(const VirtBoardInfo *vbi, qemu_irq *pic) > @@ -817,7 +876,7 @@ static void machvirt_init(MachineState *machine) > > create_flash(vbi); > > - gic_phandle = create_gic(vbi, pic); > + gic_phandle = create_gic(vbi, pic, machine->kernel_irqchip_type); > > create_uart(vbi, pic); > > @@ -859,7 +918,7 @@ static void virt_set_secure(Object *obj, bool value, Error **errp) > vms->secure = value; > } > > -static void virt_instance_init(Object *obj) > +static void virt_instance_init_common(Object *obj) > { > VirtMachineState *vms = VIRT_MACHINE(obj); > > @@ -873,6 +932,14 @@ static void virt_instance_init(Object *obj) > NULL); > } > > +static void virt_instance_init(Object *obj) > +{ > + MachineState *ms = MACHINE(obj); > + > + ms->kernel_irqchip_type = KVM_DEV_TYPE_ARM_VGIC_V2; > + virt_instance_init_common(obj); > +} > + > static void virt_class_init(ObjectClass *oc, void *data) > { > MachineClass *mc = MACHINE_CLASS(oc); > @@ -892,9 +959,38 @@ static const TypeInfo machvirt_info = { > .class_init = virt_class_init, > }; > > +static void virtv3_instance_init(Object *obj) > +{ > + MachineState *ms = MACHINE(obj); > + > + ms->kernel_irqchip_type = KVM_DEV_TYPE_ARM_VGIC_V3; > + virt_instance_init_common(obj); > +} > + > +static void virtv3_class_init(ObjectClass *oc, void *data) > +{ > + MachineClass *mc = MACHINE_CLASS(oc); > + > + mc->name = TYPE_VIRTV3_MACHINE; > + mc->desc = "ARM Virtual Machine with GICv3", > + mc->init = machvirt_init; > + /* With gic3 full implementation (with bitops) rase the lmit to 128 */ > + mc->max_cpus = 64; > +} > + > +static const TypeInfo machvirtv3_info = { > + .name = TYPE_VIRTV3_MACHINE, > + .parent = TYPE_VIRT_MACHINE, > + .instance_size = sizeof(VirtMachineState), > + .instance_init = virtv3_instance_init, > + .class_size = sizeof(VirtMachineClass), > + .class_init = virtv3_class_init, > +}; > + > static void machvirt_machine_init(void) > { > type_register_static(&machvirt_info); > + type_register_static(&machvirtv3_info); > } > > machine_init(machvirt_machine_init); > diff --git a/include/hw/boards.h b/include/hw/boards.h > index 1f11881..3eb32f2 100644 > --- a/include/hw/boards.h > +++ b/include/hw/boards.h > @@ -138,6 +138,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; > -- > 1.9.5.msysgit.0 > >