Re: [Qemu-devel] [PATCH v10 5/5] hw/arm/virt: Add gic-version option to virt machine

[Eric Auger <eric.auger@linaro.org>]

Hi Pavel,
On 08/18/2015 03:33 PM, Pavel Fedin wrote:
> Add gic_version to VirtMachineState, set it to value of the option
> and pass it around where necessary. Instantiate devices and fdt
> nodes according to the choice.
> 
> max_cpus for virt machine increased to 126 (calculated from redistributor
> space available in the memory map).
 GICv2 compatibility check happens
> inside arm_gic_common_realize().
> 
> ITS regions are added to the memory map too, however currently they
> are not used, just reserved.
> 
> Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
> ---
>  hw/arm/virt.c         | 111 +++++++++++++++++++++++++++++++++++++++++---------
>  include/hw/arm/fdt.h  |   2 +-
>  include/hw/arm/virt.h |   5 ++-
>  3 files changed, 96 insertions(+), 22 deletions(-)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index d5a8417..e090640 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -50,6 +50,7 @@
>  #include "hw/arm/fdt.h"
>  #include "hw/intc/arm_gic_common.h"
>  #include "kvm_arm.h"
> +#include "qapi/visitor.h"
>  
>  /* Number of external interrupt lines to configure the GIC with */
>  #define NUM_IRQS 256
> @@ -79,6 +80,7 @@ typedef struct {
>  typedef struct {
>      MachineState parent;
>      bool secure;
> +    int32_t gic_version;
>  } VirtMachineState;
>  
>  #define TYPE_VIRT_MACHINE   "virt"
> @@ -109,6 +111,9 @@ static const MemMapEntry a15memmap[] = {
>      [VIRT_GIC_DIST] =           { 0x08000000, 0x00010000 },
>      [VIRT_GIC_CPU] =            { 0x08010000, 0x00010000 },
>      [VIRT_GIC_V2M] =            { 0x08020000, 0x00001000 },
> +    [VIRT_ITS_CONTROL] =        { 0x08020000, 0x00010000 },
> +    [VIRT_ITS_TRANSLATION] =    { 0x08030000, 0x00010000 },
> +    [VIRT_GIC_REDIST] =         { 0x08040000, 0x00FC0000 },
Although you put it in the commit msg, a comment saying it corresponds
to 2*64kB*126 CPUs may be worth I think

     { 0x09000000, 0x00001000 },
>      [VIRT_RTC] =                { 0x09010000, 0x00001000 },
>      [VIRT_FW_CFG] =             { 0x09020000, 0x0000000a },
> @@ -258,10 +263,13 @@ static void fdt_add_timer_nodes(const VirtBoardInfo *vbi)
>       * they are edge-triggered.
>       */
>      ARMCPU *armcpu;
> +    uint32_t max;
>      uint32_t irqflags = GIC_FDT_IRQ_FLAGS_EDGE_LO_HI;
>  
> +    /* Argument is 32 bit but 8 bits are reserved for flags */
it seems to exist a different semantic for his 3d cell depending on
GICv2/GICv3:
- in Documentation/devicetree/bindings/arm/gic.txt it is mentionned
bits[15:8] PPI interrupt cpu mask (so 8 bits only)
- in gic-v3.txt it is said the 3d cell is the flags, encoded as follows:
        bits[3:0] trigger type and level flags.
                1 = edge triggered
                4 = level triggered

> +    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");
>  
> @@ -285,6 +293,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);
> @@ -321,25 +341,36 @@ static void fdt_add_v2m_gic_node(VirtBoardInfo *vbi)
>      qemu_fdt_setprop_cell(vbi->fdt, "/intc/v2m", "phandle", vbi->v2m_phandle);
>  }
>  
> -static void fdt_add_gic_node(VirtBoardInfo *vbi)
> +static void fdt_add_gic_node(VirtBoardInfo *vbi, int type)
>  {
>      vbi->gic_phandle = qemu_fdt_alloc_phandle(vbi->fdt);
>      qemu_fdt_setprop_cell(vbi->fdt, "/", "interrupt-parent", vbi->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",
> -                                     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", "#address-cells", 0x2);
>      qemu_fdt_setprop_cell(vbi->fdt, "/intc", "#size-cells", 0x2);
>      qemu_fdt_setprop(vbi->fdt, "/intc", "ranges", NULL, 0);
> +    if (type == 3) {
> +        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,
> +                                     2, vbi->memmap[VIRT_GIC_REDIST].base,
> +                                     2, vbi->memmap[VIRT_GIC_REDIST].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", vbi->gic_phandle);
>  }
>  
> @@ -362,18 +393,18 @@ static void create_v2m(VirtBoardInfo *vbi, qemu_irq *pic)
>      fdt_add_v2m_gic_node(vbi);
>  }
>  
> -static void create_gic(VirtBoardInfo *vbi, qemu_irq *pic)
> +static void create_gic(VirtBoardInfo *vbi, qemu_irq *pic, int type)
>  {
> -    /* We create a standalone GIC v2 */
> +    /* We create a standalone GIC */
>      DeviceState *gicdev;
>      SysBusDevice *gicbusdev;
>      const char *gictype;
>      int i;
>  
> -    gictype = gic_class_name();
> +    gictype = (type == 3) ? gicv3_class_name() : gic_class_name();
>  
>      gicdev = qdev_create(NULL, gictype);
> -    qdev_prop_set_uint32(gicdev, "revision", 2);
> +    qdev_prop_set_uint32(gicdev, "revision", type);
>      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).
> @@ -382,7 +413,11 @@ static void create_gic(VirtBoardInfo *vbi, qemu_irq *pic)
>      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);
> +    if (type == 3) {
> +        sysbus_mmio_map(gicbusdev, 1, vbi->memmap[VIRT_GIC_REDIST].base);
> +    } else {
> +        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
> @@ -417,9 +452,11 @@ static void create_gic(VirtBoardInfo *vbi, qemu_irq *pic)
>          pic[i] = qdev_get_gpio_in(gicdev, i);
>      }
>  
> -    fdt_add_gic_node(vbi);
> +    fdt_add_gic_node(vbi, type);
>  
> -    create_v2m(vbi, pic);
> +    if (type == 2) {
> +        create_v2m(vbi, pic);
> +    }
>  }
>  
>  static void create_uart(const VirtBoardInfo *vbi, qemu_irq *pic)
> @@ -723,7 +760,10 @@ static void create_pcie(const VirtBoardInfo *vbi, qemu_irq *pic)
>      qemu_fdt_setprop_cells(vbi->fdt, nodename, "bus-range", 0,
>                             nr_pcie_buses - 1);
>  
> -    qemu_fdt_setprop_cells(vbi->fdt, nodename, "msi-parent", vbi->v2m_phandle);
> +    if (vbi->v2m_phandle) {
> +        qemu_fdt_setprop_cells(vbi->fdt, nodename, "msi-parent",
> +                               vbi->v2m_phandle);
> +    }
>  
>      qemu_fdt_setprop_sized_cells(vbi->fdt, nodename, "reg",
>                                   2, base_ecam, 2, size_ecam);
> @@ -883,7 +923,7 @@ static void machvirt_init(MachineState *machine)
>  
>      create_flash(vbi);
>  
> -    create_gic(vbi, pic);
> +    create_gic(vbi, pic, vms->gic_version);
>  
>      create_uart(vbi, pic);
>  
> @@ -941,6 +981,26 @@ 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)
> +{
> +    VirtMachineState *vms = VIRT_MACHINE(obj);
> +
> +    visit_type_int32(v, &vms->gic_version, name, errp);
> +}
> +
> +static void virt_set_gic_version(Object *obj, Visitor *v, void *opaque,
> +                                 const char *name, Error **errp)
> +{
> +    VirtMachineState *vms = VIRT_MACHINE(obj);
> +
> +    visit_type_int32(v, &vms->gic_version, name, errp);
> +    if (vms->gic_version != 2 && vms->gic_version != 3) {
> +        error_report ("Only GICv2 and GICv3 supported currently\n");
checkpatch reports a warning: WARNING: space prohibited between function
name and open parenthesis '('. Also in v11 I think.

Best Regards

Eric

> +        exit(1);
> +    }
> +}
> +
>  static void virt_instance_init(Object *obj)
>  {
>      VirtMachineState *vms = VIRT_MACHINE(obj);
> @@ -953,6 +1013,14 @@ static void virt_instance_init(Object *obj)
>                                      "Set on/off to enable/disable the ARM "
>                                      "Security Extensions (TrustZone)",
>                                      NULL);
> +
> +    /* Default GIC type is v2 */
> +    vms->gic_version = 2;
> +    object_property_add(obj, "gic-version", "int", virt_get_gic_version,
> +                        virt_set_gic_version, NULL, NULL, NULL);
> +    object_property_set_description(obj, "gic-version",
> +                                    "Set GIC version. "
> +                                    "Valid values are 2 and 3", NULL);
>  }
>  
>  static void virt_class_init(ObjectClass *oc, void *data)
> @@ -962,7 +1030,10 @@ static void virt_class_init(ObjectClass *oc, void *data)
>      mc->name = TYPE_VIRT_MACHINE;
>      mc->desc = "ARM Virtual Machine",
>      mc->init = machvirt_init;
> -    mc->max_cpus = 8;
> +    /* Our maximum number of CPUs depends on how many redistributors
> +     * we can fit into memory map
> +     */
> +    mc->max_cpus = a15memmap[VIRT_GIC_REDIST].size / 0x20000;
>      mc->has_dynamic_sysbus = true;
>      mc->block_default_type = IF_VIRTIO;
>      mc->no_cdrom = 1;
> diff --git a/include/hw/arm/fdt.h b/include/hw/arm/fdt.h
> index c3d5015..dd794dd 100644
> --- a/include/hw/arm/fdt.h
> +++ b/include/hw/arm/fdt.h
> @@ -29,6 +29,6 @@
>  #define GIC_FDT_IRQ_FLAGS_LEVEL_LO 8
>  
>  #define GIC_FDT_IRQ_PPI_CPU_START 8
> -#define GIC_FDT_IRQ_PPI_CPU_WIDTH 8
> +#define GIC_FDT_IRQ_PPI_CPU_WIDTH 24
>  
>  #endif
> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> index d22fd8e..6c5a9c9 100644
> --- a/include/hw/arm/virt.h
> +++ b/include/hw/arm/virt.h
> @@ -46,6 +46,10 @@ enum {
>      VIRT_CPUPERIPHS,
>      VIRT_GIC_DIST,
>      VIRT_GIC_CPU,
> +    VIRT_GIC_V2M,
> +    VIRT_ITS_CONTROL,
> +    VIRT_ITS_TRANSLATION,
> +    VIRT_GIC_REDIST,
>      VIRT_UART,
>      VIRT_MMIO,
>      VIRT_RTC,
> @@ -54,7 +58,6 @@ enum {
>      VIRT_PCIE_MMIO,
>      VIRT_PCIE_PIO,
>      VIRT_PCIE_ECAM,
> -    VIRT_GIC_V2M,
>      VIRT_PLATFORM_BUS,
>  };
>  
>