See inline. On Wednesday, October 21, 2015, Pavel Fedin <p.fe...@samsung.com> wrote:
> Hello! > > > -----Original Message----- > > From: Shlomo Pongratz [mailto:shlomopongr...@gmail.com <javascript:;>] > > Sent: Tuesday, October 20, 2015 8:22 PM > > To: qemu-devel@nongnu.org <javascript:;> > > Cc: p.fe...@samsung.com <javascript:;>; peter.mayd...@linaro.org > <javascript:;>; eric.au...@linaro.org <javascript:;>; > > shannon.z...@linaro.org <javascript:;>; imamm...@redhat.com > <javascript:;>; ash...@broadcom.com <javascript:;>; Shlomo Pongratz > > Subject: [PATCH RFC V5 9/9] hw/arm: Add virt-v3 machine that uses GIC-500 > > > > From: Shlomo Pongratz <shlomo.pongr...@huawei.com <javascript:;>> > > > > 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. > > I see, just how do I pass the gic version from the command line? > > Registers the CPU system instructions and bind them to the GIC. > > Pass the cores' affinity to the GIC. > > > > Signed-off-by: Shlomo Pongratz <shlomo.pongr...@huawei.com > <javascript:;>> > > --- > > 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. > Typo. > > > [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. > O.K. I'll change to dash. GICV3 is accessed by system instructions that exists only in ARCH64. > > +#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 > >
