Re: [Qemu-devel] [PATCH 1/3] hw/arm/virt: Create the GIC ourselves rather than (ab)using a15mpcore_priv

[Peter Crosthwaite]

On Fri, Apr 25, 2014 at 3:54 AM, Peter Maydell <address@hidden> 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 <address@hidden>
> ---
>  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
>
>