| Message ID | 1398362083-17737-2-git-send-email-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
On Fri, Apr 25, 2014 at 3:54 AM, Peter Maydell <peter.maydell@linaro.org> wrote: > Rather than having the virt machine model create an a15mpcore_priv > device regardless of the actual CPU type in order to instantiate the GIC, > move to having the machine model create the GIC directly. This > corresponds to a system which uses a standalone GIC (eg the GIC-400) > rather than the one built in to the CPU core. > > The primary motivation for this is to support the Cortex-A57, > which for a KVM configuration will use a GICv2, which is not > built into the CPU. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > hw/arm/virt.c | 82 ++++++++++++++++++++++++++++++++++++++--------------------- > 1 file changed, 53 insertions(+), 29 deletions(-) > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c > index 2bbc931..ecff256 100644 > --- a/hw/arm/virt.c > +++ b/hw/arm/virt.c > @@ -75,8 +75,6 @@ typedef struct MemMapEntry { > typedef struct VirtBoardInfo { > struct arm_boot_info bootinfo; > const char *cpu_model; > - const char *qdevname; > - const char *gic_compatible; I'm not sure I understand the motivation for removing the data driven type and dts-compat. They seem useful to me esp. considering there may be variation if some virt boards want later GIC versions and others stay behind. Cant these just drive the GIC type and compat rather than going back to hardcodedness? > const MemMapEntry *memmap; > const int *irqmap; > int smp_cpus; > @@ -117,16 +115,11 @@ static const int a15irqmap[] = { > static VirtBoardInfo machines[] = { > { > .cpu_model = "cortex-a15", > - .qdevname = "a15mpcore_priv", Which would mean this would just change over to GICs qdev name. > - .gic_compatible = "arm,cortex-a15-gic", This would stay as is. > .memmap = a15memmap, > .irqmap = a15irqmap, > }, > { > .cpu_model = "host", > - /* We use the A15 private peripheral model to get a V2 GIC */ > - .qdevname = "a15mpcore_priv", > - .gic_compatible = "arm,cortex-a15-gic", > .memmap = a15memmap, > .irqmap = a15irqmap, > }, > @@ -251,8 +244,9 @@ static void 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", > - vbi->gic_compatible); > + "arm,cortex-a15-gic"); no change here either I think. > 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", > @@ -263,6 +257,56 @@ static void fdt_add_gic_node(const VirtBoardInfo *vbi) > qemu_fdt_setprop_cell(vbi->fdt, "/intc", "phandle", gic_phandle); > } > > +static void create_gic(const VirtBoardInfo *vbi, qemu_irq *pic) > +{ > + /* We create a standalone GIC v2 */ > + DeviceState *gicdev; > + SysBusDevice *gicbusdev; > + const char *gictype = "arm_gic"; And this remains data driven. > + int i; > + > + if (kvm_irqchip_in_kernel()) { > + gictype = "kvm-arm-gic"; > + } > + > + gicdev = qdev_create(NULL, gictype); > + 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). > + */ > + qdev_prop_set_uint32(gicdev, "num-irq", NUM_IRQS + 32); > + 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); > + > + /* Wire the outputs from each CPU's generic timer to the > + * appropriate GIC PPI inputs, and the GIC's IRQ output to > + * the CPU's IRQ input. > + */ > + for (i = 0; i < smp_cpus; i++) { > + DeviceState *cpudev = DEVICE(qemu_get_cpu(i)); > + int ppibase = NUM_IRQS + i * 32; > + /* physical timer; we wire it up to the non-secure timer's ID, > + * since a real A15 always has TrustZone but QEMU doesn't. > + */ > + qdev_connect_gpio_out(cpudev, 0, > + qdev_get_gpio_in(gicdev, ppibase + 30)); > + /* virtual timer */ > + qdev_connect_gpio_out(cpudev, 1, > + qdev_get_gpio_in(gicdev, ppibase + 27)); I'd take the oppurtunity to tie the dts PPI mappings and these magic numbers together. Eg, macroify "30" "27" and then macrofiy: qemu_fdt_setprop_cells(vbi->fdt, "/timer", "interrupts", GIC_FDT_IRQ_TYPE_PPI, 13, irqflags, GIC_FDT_IRQ_TYPE_PPI, 14, irqflags, GIC_FDT_IRQ_TYPE_PPI, 11, irqflags, GIC_FDT_IRQ_TYPE_PPI, 10, irqflags); with the -16 shift. Regards, Peter > + > + sysbus_connect_irq(gicbusdev, i, qdev_get_gpio_in(cpudev, ARM_CPU_IRQ)); > + } > + > + for (i = 0; i < NUM_IRQS; i++) { > + pic[i] = qdev_get_gpio_in(gicdev, i); > + } > + > + fdt_add_gic_node(vbi); > +} > + > static void create_uart(const VirtBoardInfo *vbi, qemu_irq *pic) > { > char *nodename; > @@ -340,8 +384,6 @@ static void machvirt_init(QEMUMachineInitArgs *args) > MemoryRegion *sysmem = get_system_memory(); > int n; > MemoryRegion *ram = g_new(MemoryRegion, 1); > - DeviceState *dev; > - SysBusDevice *busdev; > const char *cpu_model = args->cpu_model; > VirtBoardInfo *vbi; > > @@ -404,25 +446,7 @@ static void machvirt_init(QEMUMachineInitArgs *args) > vmstate_register_ram_global(ram); > memory_region_add_subregion(sysmem, vbi->memmap[VIRT_MEM].base, ram); > > - dev = qdev_create(NULL, vbi->qdevname); > - qdev_prop_set_uint32(dev, "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). > - */ > - qdev_prop_set_uint32(dev, "num-irq", NUM_IRQS + 32); > - qdev_init_nofail(dev); > - busdev = SYS_BUS_DEVICE(dev); > - sysbus_mmio_map(busdev, 0, vbi->memmap[VIRT_CPUPERIPHS].base); > - fdt_add_gic_node(vbi); > - for (n = 0; n < smp_cpus; n++) { > - DeviceState *cpudev = DEVICE(qemu_get_cpu(n)); > - > - sysbus_connect_irq(busdev, n, qdev_get_gpio_in(cpudev, ARM_CPU_IRQ)); > - } > - > - for (n = 0; n < NUM_IRQS; n++) { > - pic[n] = qdev_get_gpio_in(dev, n); > - } > + create_gic(vbi, pic); > > create_uart(vbi, pic); > > -- > 1.9.2 > >
On 27 April 2014 02:09, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: > On Fri, Apr 25, 2014 at 3:54 AM, Peter Maydell <peter.maydell@linaro.org> wrote: >> Rather than having the virt machine model create an a15mpcore_priv >> device regardless of the actual CPU type in order to instantiate the GIC, >> move to having the machine model create the GIC directly. This >> corresponds to a system which uses a standalone GIC (eg the GIC-400) >> rather than the one built in to the CPU core. >> >> The primary motivation for this is to support the Cortex-A57, >> which for a KVM configuration will use a GICv2, which is not >> built into the CPU. >> >> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> >> --- >> hw/arm/virt.c | 82 ++++++++++++++++++++++++++++++++++++++--------------------- >> 1 file changed, 53 insertions(+), 29 deletions(-) >> >> diff --git a/hw/arm/virt.c b/hw/arm/virt.c >> index 2bbc931..ecff256 100644 >> --- a/hw/arm/virt.c >> +++ b/hw/arm/virt.c >> @@ -75,8 +75,6 @@ typedef struct MemMapEntry { >> typedef struct VirtBoardInfo { >> struct arm_boot_info bootinfo; >> const char *cpu_model; >> - const char *qdevname; >> - const char *gic_compatible; > > I'm not sure I understand the motivation for removing the data driven > type and dts-compat. They seem useful to me esp. considering there may > be variation if some virt boards want later GIC versions and others > stay behind. Cant these just drive the GIC type and compat rather than > going back to hardcodedness? Basically they're not what you'd want for "maybe GICv2, maybe GICv2 + v2m, maybe GICv3". I think for those you probably end up needing a simple enum and three different functions for setup. I put these into the struct when I was expecting to have a lot of CPU container objects like a15mpcore_priv, which all behave essentially the same way. The GIC objects definitely won't, so this is definitely insufficient, and we can't know what's actually going to be required until we've got a GICv3 we can wire up. So it seemed best to remove the now-pointless fields. (Also, GICv2 vs v3 vs v2+v2m is probably not a per-CPU decision; it's orthogonal. So even if we wanted it data driven it might need to go somewhere else.) >> const MemMapEntry *memmap; >> const int *irqmap; >> int smp_cpus; >> @@ -117,16 +115,11 @@ static const int a15irqmap[] = { >> static VirtBoardInfo machines[] = { >> { >> .cpu_model = "cortex-a15", >> - .qdevname = "a15mpcore_priv", > > Which would mean this would just change over to GICs qdev name. > >> - .gic_compatible = "arm,cortex-a15-gic", > > This would stay as is. > >> .memmap = a15memmap, >> .irqmap = a15irqmap, >> }, >> { >> .cpu_model = "host", >> - /* We use the A15 private peripheral model to get a V2 GIC */ >> - .qdevname = "a15mpcore_priv", >> - .gic_compatible = "arm,cortex-a15-gic", >> .memmap = a15memmap, >> .irqmap = a15irqmap, >> }, >> @@ -251,8 +244,9 @@ static void 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", >> - vbi->gic_compatible); >> + "arm,cortex-a15-gic"); > > no change here either I think. > >> 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", >> @@ -263,6 +257,56 @@ static void fdt_add_gic_node(const VirtBoardInfo *vbi) >> qemu_fdt_setprop_cell(vbi->fdt, "/intc", "phandle", gic_phandle); >> } >> >> +static void create_gic(const VirtBoardInfo *vbi, qemu_irq *pic) >> +{ >> + /* We create a standalone GIC v2 */ >> + DeviceState *gicdev; >> + SysBusDevice *gicbusdev; >> + const char *gictype = "arm_gic"; > > And this remains data driven. Except it can't, because of: >> + int i; >> + >> + if (kvm_irqchip_in_kernel()) { >> + gictype = "kvm-arm-gic"; >> + } So what you would need to be expressing in the data is "arm_gic if external irqchip, else kvm-arm-gic". One field isn't enough for that. >> + for (i = 0; i < smp_cpus; i++) { >> + DeviceState *cpudev = DEVICE(qemu_get_cpu(i)); >> + int ppibase = NUM_IRQS + i * 32; >> + /* physical timer; we wire it up to the non-secure timer's ID, >> + * since a real A15 always has TrustZone but QEMU doesn't. >> + */ >> + qdev_connect_gpio_out(cpudev, 0, >> + qdev_get_gpio_in(gicdev, ppibase + 30)); >> + /* virtual timer */ >> + qdev_connect_gpio_out(cpudev, 1, >> + qdev_get_gpio_in(gicdev, ppibase + 27)); > > I'd take the oppurtunity to tie the dts PPI mappings and these magic > numbers together. Eg, macroify "30" "27" and then macrofiy: > > qemu_fdt_setprop_cells(vbi->fdt, "/timer", "interrupts", > GIC_FDT_IRQ_TYPE_PPI, 13, irqflags, > GIC_FDT_IRQ_TYPE_PPI, 14, irqflags, > GIC_FDT_IRQ_TYPE_PPI, 11, irqflags, > GIC_FDT_IRQ_TYPE_PPI, 10, irqflags); > > with the -16 shift. Maybe. I think I'll put that in a separate patch. thanks -- PMM
diff --git a/hw/arm/virt.c b/hw/arm/virt.c index 2bbc931..ecff256 100644 --- a/hw/arm/virt.c +++ b/hw/arm/virt.c @@ -75,8 +75,6 @@ typedef struct MemMapEntry { typedef struct VirtBoardInfo { struct arm_boot_info bootinfo; const char *cpu_model; - const char *qdevname; - const char *gic_compatible; const MemMapEntry *memmap; const int *irqmap; int smp_cpus; @@ -117,16 +115,11 @@ static const int a15irqmap[] = { static VirtBoardInfo machines[] = { { .cpu_model = "cortex-a15", - .qdevname = "a15mpcore_priv", - .gic_compatible = "arm,cortex-a15-gic", .memmap = a15memmap, .irqmap = a15irqmap, }, { .cpu_model = "host", - /* We use the A15 private peripheral model to get a V2 GIC */ - .qdevname = "a15mpcore_priv", - .gic_compatible = "arm,cortex-a15-gic", .memmap = a15memmap, .irqmap = a15irqmap, }, @@ -251,8 +244,9 @@ static void 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", - vbi->gic_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", @@ -263,6 +257,56 @@ static void fdt_add_gic_node(const VirtBoardInfo *vbi) qemu_fdt_setprop_cell(vbi->fdt, "/intc", "phandle", gic_phandle); } +static void create_gic(const VirtBoardInfo *vbi, qemu_irq *pic) +{ + /* We create a standalone GIC v2 */ + DeviceState *gicdev; + SysBusDevice *gicbusdev; + const char *gictype = "arm_gic"; + int i; + + if (kvm_irqchip_in_kernel()) { + gictype = "kvm-arm-gic"; + } + + gicdev = qdev_create(NULL, gictype); + 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). + */ + qdev_prop_set_uint32(gicdev, "num-irq", NUM_IRQS + 32); + 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); + + /* Wire the outputs from each CPU's generic timer to the + * appropriate GIC PPI inputs, and the GIC's IRQ output to + * the CPU's IRQ input. + */ + for (i = 0; i < smp_cpus; i++) { + DeviceState *cpudev = DEVICE(qemu_get_cpu(i)); + int ppibase = NUM_IRQS + i * 32; + /* physical timer; we wire it up to the non-secure timer's ID, + * since a real A15 always has TrustZone but QEMU doesn't. + */ + qdev_connect_gpio_out(cpudev, 0, + qdev_get_gpio_in(gicdev, ppibase + 30)); + /* virtual timer */ + qdev_connect_gpio_out(cpudev, 1, + qdev_get_gpio_in(gicdev, ppibase + 27)); + + sysbus_connect_irq(gicbusdev, i, qdev_get_gpio_in(cpudev, ARM_CPU_IRQ)); + } + + for (i = 0; i < NUM_IRQS; i++) { + pic[i] = qdev_get_gpio_in(gicdev, i); + } + + fdt_add_gic_node(vbi); +} + static void create_uart(const VirtBoardInfo *vbi, qemu_irq *pic) { char *nodename; @@ -340,8 +384,6 @@ static void machvirt_init(QEMUMachineInitArgs *args) MemoryRegion *sysmem = get_system_memory(); int n; MemoryRegion *ram = g_new(MemoryRegion, 1); - DeviceState *dev; - SysBusDevice *busdev; const char *cpu_model = args->cpu_model; VirtBoardInfo *vbi; @@ -404,25 +446,7 @@ static void machvirt_init(QEMUMachineInitArgs *args) vmstate_register_ram_global(ram); memory_region_add_subregion(sysmem, vbi->memmap[VIRT_MEM].base, ram); - dev = qdev_create(NULL, vbi->qdevname); - qdev_prop_set_uint32(dev, "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). - */ - qdev_prop_set_uint32(dev, "num-irq", NUM_IRQS + 32); - qdev_init_nofail(dev); - busdev = SYS_BUS_DEVICE(dev); - sysbus_mmio_map(busdev, 0, vbi->memmap[VIRT_CPUPERIPHS].base); - fdt_add_gic_node(vbi); - for (n = 0; n < smp_cpus; n++) { - DeviceState *cpudev = DEVICE(qemu_get_cpu(n)); - - sysbus_connect_irq(busdev, n, qdev_get_gpio_in(cpudev, ARM_CPU_IRQ)); - } - - for (n = 0; n < NUM_IRQS; n++) { - pic[n] = qdev_get_gpio_in(dev, n); - } + create_gic(vbi, pic); create_uart(vbi, pic);
Rather than having the virt machine model create an a15mpcore_priv device regardless of the actual CPU type in order to instantiate the GIC, move to having the machine model create the GIC directly. This corresponds to a system which uses a standalone GIC (eg the GIC-400) rather than the one built in to the CPU core. The primary motivation for this is to support the Cortex-A57, which for a KVM configuration will use a GICv2, which is not built into the CPU. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- hw/arm/virt.c | 82 ++++++++++++++++++++++++++++++++++++++--------------------- 1 file changed, 53 insertions(+), 29 deletions(-)