Re: [Qemu-devel] [PATCH RFC 1/4] Add virt-v3 machine that uses GIC-500

[Christoffer Dall]

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 <address@hidden>
> 
> ---
>  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
> 
>