Re: [Qemu-devel] [PATCH RFC V5 9/9] hw/arm: Add virt-v3 machine that uses GIC-500

[Pavel Fedin]

 Hello!

> -----Original Message-----
> From: Shlomo Pongratz [mailto:address@hidden
> Sent: Tuesday, October 20, 2015 8:22 PM
> To: address@hidden
> Cc: address@hidden; address@hidden; address@hidden;
> address@hidden; address@hidden; address@hidden; Shlomo Pongratz
> Subject: [PATCH RFC V5 9/9] hw/arm: Add virt-v3 machine that uses GIC-500
> 
> From: Shlomo Pongratz <address@hidden>
> 
> Add virt-v3 machine that uses GIC-500.

 We already concluded long time ago that adding virt-v3 machine is wrong 
approach. We already have gic-version property for the virt
machine, and especially for you i left a TODO placeholder in gicv3_class_name() 
function. Just return name of your class there, and
you're done.

> Registers the CPU system instructions and bind them to the GIC.
> Pass the cores' affinity to the GIC.
> 
> Signed-off-by: Shlomo Pongratz <address@hidden>
> ---
>  hw/arm/virt.c         | 87 
> ++++++++++++++++++++++++++++++++++++++++++++++-----
>  include/hw/arm/fdt.h  |  2 ++
>  include/hw/arm/virt.h |  1 +
>  target-arm/machine.c  |  7 ++++-
>  4 files changed, 88 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 5d38c47..d4fb8f3 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -71,6 +71,7 @@ typedef struct VirtBoardInfo {
>      uint32_t clock_phandle;
>      uint32_t gic_phandle;
>      uint32_t v2m_phandle;
> +    const char *class_name;
>  } VirtBoardInfo;
> 
>  typedef struct {
> @@ -86,6 +87,7 @@ typedef struct {
>  } VirtMachineState;
> 
>  #define TYPE_VIRT_MACHINE   MACHINE_TYPE_NAME("virt")
> +#define TYPE_VIRTV3_MACHINE MACHINE_TYPE_NAME("virt-v3")
>  #define VIRT_MACHINE(obj) \
>      OBJECT_CHECK(VirtMachineState, (obj), TYPE_VIRT_MACHINE)
>  #define VIRT_MACHINE_GET_CLASS(obj) \
> @@ -111,6 +113,7 @@ static const MemMapEntry a15memmap[] = {
>      [VIRT_CPUPERIPHS] =         { 0x08000000, 0x00020000 },
>      /* GIC distributor and CPU interfaces sit inside the CPU peripheral 
> space */
>      [VIRT_GIC_DIST] =           { 0x08000000, 0x00010000 },
> +    /* Note on GICv3 VIRT_GIC_DIST_SPI takes place of VIRT_GIC_CPU */

 Memory map of the "virt" machine already contains everything needed. BTW, 
what's "SPI"? "MBI", you meant? Well, we are going to
have an ITS, so this simple message translator will not be needed.

>      [VIRT_GIC_CPU] =            { 0x08010000, 0x00010000 },
>      [VIRT_GIC_V2M] =            { 0x08020000, 0x00001000 },
>      /* The space in between here is reserved for GICv3 CPU/vCPU/HYP */
> @@ -145,21 +148,26 @@ static VirtBoardInfo machines[] = {
>          .cpu_model = "cortex-a15",
>          .memmap = a15memmap,
>          .irqmap = a15irqmap,
> +        .class_name = TYPE_ARM_CPU,
> +
>      },
>      {
>          .cpu_model = "cortex-a53",
>          .memmap = a15memmap,
>          .irqmap = a15irqmap,
> +        .class_name = TYPE_AARCH64_CPU,
>      },
>      {
>          .cpu_model = "cortex-a57",
>          .memmap = a15memmap,
>          .irqmap = a15irqmap,
> +        .class_name = TYPE_AARCH64_CPU,
>      },
>      {
>          .cpu_model = "host",
>          .memmap = a15memmap,
>          .irqmap = a15irqmap,
> +        .class_name = TYPE_ARM_CPU,
>      },
>  };
> 
> @@ -228,7 +236,6 @@ static void fdt_add_psci_node(const VirtBoardInfo *vbi)
>      if (armcpu->psci_version == 2) {
>          const char comp[] = "arm,psci-0.2\0arm,psci";
>          qemu_fdt_setprop(fdt, "/psci", "compatible", comp, sizeof(comp));
> -
>          cpu_off_fn = QEMU_PSCI_0_2_FN_CPU_OFF;
>          if (arm_feature(&armcpu->env, ARM_FEATURE_AARCH64)) {
>              cpu_suspend_fn = QEMU_PSCI_0_2_FN64_CPU_SUSPEND;
> @@ -241,7 +248,6 @@ static void fdt_add_psci_node(const VirtBoardInfo *vbi)
>          }
>      } else {
>          qemu_fdt_setprop_string(fdt, "/psci", "compatible", "arm,psci");
> -
>          cpu_suspend_fn = QEMU_PSCI_0_1_FN_CPU_SUSPEND;
>          cpu_off_fn = QEMU_PSCI_0_1_FN_CPU_OFF;
>          cpu_on_fn = QEMU_PSCI_0_1_FN_CPU_ON;
> @@ -274,6 +280,12 @@ static void fdt_add_timer_nodes(const VirtBoardInfo 
> *vbi, int gictype)
>          irqflags = deposit32(irqflags, GIC_FDT_IRQ_PPI_CPU_START,
>                               GIC_FDT_IRQ_PPI_CPU_WIDTH,
>                               (1 << vbi->smp_cpus) - 1);
> +    } else if (gictype == 3) {
> +        uint32_t max;
> +        /* Argument is 32 bit but 8 bits are reserved for flags */
> +        max = (vbi->smp_cpus >= 24) ? 24 : vbi->smp_cpus;
> +        irqflags = deposit32(irqflags, GIC_FDT_IRQ_PPI_CPU_START,
> +                             GICV3_FDT_IRQ_PPI_CPU_WIDTH, (1 << max) - 1);
>      }
> 
>      qemu_fdt_add_subnode(vbi->fdt, "/timer");
> @@ -429,12 +441,32 @@ static void create_gic(VirtBoardInfo *vbi, qemu_irq 
> *pic, int type, bool
> secure)
>      gicdev = qdev_create(NULL, gictype);
>      qdev_prop_set_uint32(gicdev, "revision", type);
>      qdev_prop_set_uint32(gicdev, "num-cpu", smp_cpus);
> +
> +#ifdef TARGET_AARCH64
> +    if (type == 3) {
> +        qdev_prop_set_uint32(gicdev, "len-mp-affinity", smp_cpus);
> +        /* Connect GIC to CPU */
> +        for (i = 0; i < smp_cpus; i++) {
> +            char *propname;
> +            CPUState *cpu = qemu_get_cpu(i);
> +            ARMCPU *armcpu = ARM_CPU(cpu);
> +            propname = g_strdup_printf("mp-affinity[%d]", i);
> +            qdev_prop_set_uint64(gicdev, propname, armcpu->mp_affinity);
> +            g_free(propname);
> +        }
> +    }
> +#endif
>      /* 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);
>      if (!kvm_irqchip_in_kernel()) {
> -        qdev_prop_set_bit(gicdev, "has-security-extensions", secure);
> +        if (type == 3) {
> +            /* AARCH64 has 4 security levels */
> +            qdev_prop_set_uint8(gicdev, "security-levels", secure ? 1 : 0);
> +        } else {
> +            qdev_prop_set_bit(gicdev, "has-security-extensions", secure);
> +        }
>      }
>      qdev_init_nofail(gicdev);
>      gicbusdev = SYS_BUS_DEVICE(gicdev);
> @@ -445,6 +477,16 @@ static void create_gic(VirtBoardInfo *vbi, qemu_irq 
> *pic, int type, bool
> secure)
>          sysbus_mmio_map(gicbusdev, 1, vbi->memmap[VIRT_GIC_CPU].base);
>      }
> 
> +#ifdef TARGET_AARCH64
> +    if (type == 3) {
> +        /* Connect GIC to CPU */
> +        for (i = 0; i < smp_cpus; i++) {
> +            CPUState *cpu = qemu_get_cpu(i);
> +            aarch64_registers_with_opaque_set(OBJECT(cpu), (void *)gicdev);
> +        }
> +    }
> +#endif
> +
>      /* 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.
> @@ -466,7 +508,7 @@ static void create_gic(VirtBoardInfo *vbi, qemu_irq *pic, 
> int type, bool
> secure)
>          for (irq = 0; irq < ARRAY_SIZE(timer_irq); irq++) {
>              qdev_connect_gpio_out(cpudev, irq,
>                                    qdev_get_gpio_in(gicdev,
> -                                                   ppibase + 
> timer_irq[irq]));
> +                                  ppibase + timer_irq[irq]));
>          }
> 
>          sysbus_connect_irq(gicbusdev, i, qdev_get_gpio_in(cpudev, 
> ARM_CPU_IRQ));
> @@ -967,7 +1009,7 @@ static void machvirt_init(MachineState *machine)
>      create_fdt(vbi);
> 
>      for (n = 0; n < smp_cpus; n++) {
> -        ObjectClass *oc = cpu_class_by_name(TYPE_ARM_CPU, cpustr[0]);
> +        ObjectClass *oc = cpu_class_by_name(vbi->class_name, cpustr[0]);
>          CPUClass *cc = CPU_CLASS(oc);
>          Object *cpuobj;
>          Error *err = NULL;
> @@ -1116,7 +1158,7 @@ static void virt_set_gic_version(Object *obj, const 
> char *value, Error
> **errp)
>      }
>  }
> 
> -static void virt_instance_init(Object *obj)
> +static void virt_instance_init_common(Object *obj, int32_t version)
>  {
>      VirtMachineState *vms = VIRT_MACHINE(obj);
> 
> @@ -1140,8 +1182,7 @@ static void virt_instance_init(Object *obj)
>                                      "Set on/off to enable/disable using "
>                                      "physical address space above 32 bits",
>                                      NULL);
> -    /* Default GIC type is v2 */
> -    vms->gic_version = 2;
> +    vms->gic_version = version;
>      object_property_add_str(obj, "gic-version", virt_get_gic_version,
>                          virt_set_gic_version, NULL);
>      object_property_set_description(obj, "gic-version",
> @@ -1149,6 +1190,12 @@ static void virt_instance_init(Object *obj)
>                                      "Valid values are 2, 3 and host", NULL);
>  }
> 
> +static void virt_instance_init(Object *obj)
> +{
> +    /* Default GIC type is v2 */
> +    virt_instance_init_common(obj, 2);
> +}
> +
>  static void virt_class_init(ObjectClass *oc, void *data)
>  {
>      MachineClass *mc = MACHINE_CLASS(oc);
> @@ -1174,9 +1221,33 @@ static const TypeInfo machvirt_info = {
>      .class_init = virt_class_init,
>  };
> 
> +static void virtv3_instance_init(Object *obj)
> +{
> +    virt_instance_init_common(obj, 3);
> +}
> +
> +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;
> +}
> +
> +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);
>  }

 As i said before, this stuff is simply not needed.

> 
>  machine_init(machvirt_machine_init);
> diff --git a/include/hw/arm/fdt.h b/include/hw/arm/fdt.h
> index c3d5015..ad8f85c 100644
> --- a/include/hw/arm/fdt.h
> +++ b/include/hw/arm/fdt.h
> @@ -30,5 +30,7 @@
> 
>  #define GIC_FDT_IRQ_PPI_CPU_START 8
>  #define GIC_FDT_IRQ_PPI_CPU_WIDTH 8
> +#define GICV3_FDT_IRQ_PPI_CPU_WIDTH 24
> +
> 
>  #endif
> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> index f464586..53ff50a 100644
> --- a/include/hw/arm/virt.h
> +++ b/include/hw/arm/virt.h
> @@ -46,6 +46,7 @@ enum {
>      VIRT_CPUPERIPHS,
>      VIRT_GIC_DIST,
>      VIRT_GIC_CPU,
> +    VIRT_GIC_DIST_SPI = VIRT_GIC_CPU,
>      VIRT_GIC_V2M,
>      VIRT_GIC_ITS,
>      VIRT_GIC_REDIST,
> diff --git a/target-arm/machine.c b/target-arm/machine.c
> index 36a0d15..33f28be 100644
> --- a/target-arm/machine.c
> +++ b/target-arm/machine.c
> @@ -341,7 +341,12 @@ const char *gicv3_class_name(void)
>  #endif
>      } else {
>          /* TODO: Software emulation is not implemented yet */
> -        error_report("KVM is currently required for GICv3 emulation\n");
> +#ifdef TARGET_AARCH64
> +        return "arm_gicv3";

 Peter told me, there is a policy to use dash ("-") in class names. So 
"arm-gicv3".
 By the way, why does it depend on TARGET_AARCH64? Actually, TARGET_ARM and 
TARGET_AARCH64 are the same. You can make both emulators
running both 64- and 32-bit code. KVM code depends on TARGET_AARCH64 only 
because some definitions are missing on 32-bit kernels.
You don't have this restriction, so you don't need this #ifdef.

> +#else
> +        error_report("KVM GICv3 acceleration is not supported on this "
> +                     "platform\n");

 Why "KVM" here? Well, rubbish anyway because of the above. :)

> +#endif
>      }
> 
>      exit(1);
> --
> 1.9.1

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia