diff mbox

[v7,6/6] Add gicversion option to virt machine

Message ID c06f53aae954c94c8766de541da78bdfe87ea292.1437731107.git.p.fedin@samsung.com
State New
Headers show

Commit Message

Pavel Fedin July 24, 2015, 9:55 a.m. UTC 
Set kernel_irqchip_type according 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 64. GICv2 compatibility check
happens inside arm_gic_common_realize().

Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
---
 hw/arm/virt.c         | 137 ++++++++++++++++++++++++++++++++++++++++++--------
 include/hw/arm/fdt.h  |   2 +-
 include/hw/arm/virt.h |   6 ++-
 3 files changed, 121 insertions(+), 24 deletions(-)

Comments

Peter Maydell July 31, 2015, 3:44 p.m. UTC | #1 
On 24 July 2015 at 10:55, Pavel Fedin <p.fedin@samsung.com> wrote:
> Set kernel_irqchip_type according 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 64. GICv2 compatibility check
> happens inside arm_gic_common_realize().
>
>      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;
> +    }

We definitely need to come up with a something cleaner
than this (which is ugly for two reasons -- firstly
for borrowing the nvic pointer and secondly because
we shouldn't just be putting random pointers between
the two like this, ideally; and we definitely don't want
the board code to have to do it at this level.)

-- PMM
Pavel Fedin Aug. 3, 2015, 7:19 a.m. UTC | #2 
Hello!

> >      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;
> > +    }
> 
> We definitely need to come up with a something cleaner
> than this (which is ugly for two reasons

 This could be done:
a) as property
b) as global variable because 'gicdev' is a single of its kind.

 But, actually, this is currently only for TCG, which needs it in order to forward system register accesses to GICv3 code. Would it be OK if i just omit this assignment ?

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia
Peter Maydell Aug. 3, 2015, 7:57 a.m. UTC | #3 
On 3 August 2015 at 08:19, Pavel Fedin <p.fedin@samsung.com> wrote:
>  Hello!
>
>> >      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;
>> > +    }
>>
>> We definitely need to come up with a something cleaner
>> than this (which is ugly for two reasons
>
>  This could be done:
> a) as property
> b) as global variable because 'gicdev' is a single of its kind.
>
>  But, actually, this is currently only for TCG, which needs it in
> order to forward system register accesses to GICv3 code. Would it
> be OK if i just omit this assignment ?

Yes, I think so.

I'm surprised we tell the CPU about the GIC pointer for the
system register stuff -- I was expecting that we'd give the
GIC a CPU pointer. (We could in theory implement some
equivalent of the architected protocol between the redistributors
and the CPU interfaces, but I think that would be overkill.)

-- PMM
Shlomo Pongratz Aug. 3, 2015, 11:41 a.m. UTC | #4 
On Monday, August 3, 2015, Peter Maydell <peter.maydell@linaro.org> wrote:

> On 3 August 2015 at 08:19, Pavel Fedin <p.fedin@samsung.com <javascript:;>>
> wrote:
> >  Hello!
> >
> >> >      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;
> >> > +    }
> >>
> >> We definitely need to come up with a something cleaner
> >> than this (which is ugly for two reasons
> >
> >  This could be done:
> > a) as property
> > b) as global variable because 'gicdev' is a single of its kind.
> >
> >  But, actually, this is currently only for TCG, which needs it in
> > order to forward system register accesses to GICv3 code. Would it
> > be OK if i just omit this assignment ?
>
> Yes, I think so.
>
> I'm surprised we tell the CPU about the GIC pointer for the
> system register stuff -- I was expecting that we'd give the
> GIC a CPU pointer. (We could in theory implement some
> equivalent of the architected protocol between the redistributors
> and the CPU interfaces, but I think that would be overkill.)
>
> -- PMM
>

The problem is that each function added as a system's instruction helper to
target-arm/cpu64.c has the signature of void set(CPUARMState *env,
ARMCPRegInfo *ri, uint64_t value) or uint64_t get(CPUARMState *env,
ARMCPRegInfo *ri)
I just mimicked the way armv7m_nvic_XXXX API works.
So in a sense the CPU must be familiar with the GIC (as an opaque object of
course).

Best regards,

S.P.
Peter Maydell Aug. 3, 2015, 12:01 p.m. UTC | #5 
On 3 August 2015 at 12:41, Shlomo Pongratz <shlomopongratz@gmail.com> wrote:
>
>
> On Monday, August 3, 2015, Peter Maydell <peter.maydell@linaro.org> wrote:
>> I'm surprised we tell the CPU about the GIC pointer for the
>> system register stuff -- I was expecting that we'd give the
>> GIC a CPU pointer. (We could in theory implement some
>> equivalent of the architected protocol between the redistributors
>> and the CPU interfaces, but I think that would be overkill.)

> The problem is that each function added as a system's instruction helper to
> target-arm/cpu64.c has the signature of void set(CPUARMState *env,
> ARMCPRegInfo *ri, uint64_t value) or uint64_t get(CPUARMState *env,
> ARMCPRegInfo *ri)
> I just mimicked the way armv7m_nvic_XXXX API works.

The v7M NVIC is a terrible example to copy -- it is ancient
code mostly written before QEMU acquired various useful
abstraction layers, and has received very little maintenance
since then.

> So in a sense the CPU must be familiar with the GIC (as an opaque object of
> course).

If the GIC just registers a set of system register information with
the CPU then the CPU doesn't need to know that it's the GIC
providing those system register functions. (You can use
define_arm_cp_regs_with_opaque() to define registers such
that your callbacks can fish an opaque data pointer back out
of ri->opaque, so you can get back to the GIC data structures.)

-- PMM
diff mbox

Patch

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 30d9ab8..6eb525d 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -48,6 +48,7 @@ 
 #include "hw/arm/sysbus-fdt.h"
 #include "hw/platform-bus.h"
 #include "hw/arm/fdt.h"
+#include "qapi/visitor.h"
 
 /* Number of external interrupt lines to configure the GIC with */
 #define NUM_IRQS 256
@@ -106,7 +107,12 @@  static const MemMapEntry a15memmap[] = {
     /* GIC distributor and CPU interfaces sit inside the CPU peripheral space */
     [VIRT_GIC_DIST] =           { 0x08000000, 0x00010000 },
     [VIRT_GIC_CPU] =            { 0x08010000, 0x00010000 },
-    [VIRT_GIC_V2M] =            { 0x08020000, 0x00001000 },
+    [VIRT_GIC_V2M] =            { 0x08020000, 0x00010000 },
+    /* On v3 VIRT_GIC_DIST_MBI and VIRT_ITS_CONTROL take place of
+     * VIRT_GIC_CPU and VIRT_GIC_V2M respectively
+     */
+    [VIRT_ITS_TRANSLATION] =    { 0x08030000, 0x00010000 },
+    [VIRT_LPI] =                { 0x08040000, 0x00800000 },
     [VIRT_UART] =               { 0x09000000, 0x00001000 },
     [VIRT_RTC] =                { 0x09010000, 0x00001000 },
     [VIRT_FW_CFG] =             { 0x09020000, 0x0000000a },
@@ -256,10 +262,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 */
+    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");
 
@@ -283,6 +292,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);
@@ -319,25 +340,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 == 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,
+                                     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", vbi->gic_phandle);
 }
 
@@ -360,20 +392,35 @@  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 */
     DeviceState *gicdev;
     SysBusDevice *gicbusdev;
-    const char *gictype = "arm_gic";
+    const char *gictype;
     int i;
 
-    if (kvm_irqchip_in_kernel()) {
-        gictype = "kvm-arm-gic";
+    if (type == KVM_DEV_TYPE_ARM_VGIC_V3) {
+        /* TODO: Software emulation is not implemented yet */
+        if (!kvm_irqchip_in_kernel()) {
+            fprintf(stderr, "KVM is currently required for GICv3 emulation\n");
+            exit(1);
+        }
+        gictype = "kvm-arm-gicv3";
+    } else {
+        gictype = kvm_irqchip_in_kernel() ? "kvm-arm-gic" : "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).
@@ -382,7 +429,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 == KVM_DEV_TYPE_ARM_VGIC_V3) {
+        sysbus_mmio_map(gicbusdev, 1, vbi->memmap[VIRT_LPI].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
@@ -409,9 +460,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 == KVM_DEV_TYPE_ARM_VGIC_V2) {
+        create_v2m(vbi, pic);
+    }
 }
 
 static void create_uart(const VirtBoardInfo *vbi, qemu_irq *pic)
@@ -715,7 +768,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);
@@ -875,7 +931,7 @@  static void machvirt_init(MachineState *machine)
 
     create_flash(vbi);
 
-    create_gic(vbi, pic);
+    create_gic(vbi, pic, machine->kernel_irqchip_type);
 
     create_uart(vbi, pic);
 
@@ -933,6 +989,37 @@  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)
+{
+    MachineState *ms = MACHINE(obj);
+    int32_t value = (ms->kernel_irqchip_type == KVM_DEV_TYPE_ARM_VGIC_V3) ?
+                     3 : 2;
+
+    visit_type_int32(v, &value, name, errp);
+}
+
+static void virt_set_gic_version(Object *obj, Visitor *v, void *opaque,
+                                 const char *name, Error **errp)
+{
+    MachineState *ms = MACHINE(obj);
+    int32_t value;
+
+    visit_type_int32(v, &value, name, errp);
+
+    switch (value) {
+    case 3:
+        ms->kernel_irqchip_type = QEMU_GIC_TYPE_V3;
+        break;
+    case 2:
+        ms->kernel_irqchip_type = QEMU_GIC_TYPE_V2;
+        break;
+    default:
+        error_report("Only GICv2 and GICv3 supported currently\n");
+        exit(1);
+    }
+}
+
 static void virt_instance_init(Object *obj)
 {
     VirtMachineState *vms = VIRT_MACHINE(obj);
@@ -948,6 +1035,11 @@  static void virt_instance_init(Object *obj)
 
     /* Default GIC type is v2 */
     vms->parent.kernel_irqchip_type = QEMU_GIC_TYPE_V2;
+    object_property_add(obj, "gicversion", "int", virt_get_gic_version,
+                        virt_set_gic_version, NULL, NULL, NULL);
+    object_property_set_description(obj, "gicversion",
+                                    "Set GIC version. "
+                                    "Valid values are 2 and 3", NULL);
 }
 
 static void virt_class_init(ObjectClass *oc, void *data)
@@ -957,7 +1049,8 @@  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;
+    /* With gic3 full implementation (with bitops) rase the lmit to 128 */
+    mc->max_cpus = 64;
     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..852efb9 100644
--- a/include/hw/arm/virt.h
+++ b/include/hw/arm/virt.h
@@ -46,6 +46,11 @@  enum {
     VIRT_CPUPERIPHS,
     VIRT_GIC_DIST,
     VIRT_GIC_CPU,
+    VIRT_GIC_V2M,
+    VIRT_GIC_DIST_MBI = VIRT_GIC_CPU,
+    VIRT_ITS_CONTROL = VIRT_GIC_V2M,
+    VIRT_ITS_TRANSLATION,
+    VIRT_LPI,
     VIRT_UART,
     VIRT_MMIO,
     VIRT_RTC,
@@ -54,7 +59,6 @@  enum {
     VIRT_PCIE_MMIO,
     VIRT_PCIE_PIO,
     VIRT_PCIE_ECAM,
-    VIRT_GIC_V2M,
     VIRT_PLATFORM_BUS,
 };