| Message ID | 1445361732-16257-10-git-send-email-shlomopongratz@gmail.com |
|---|---|
| State | New |
| Headers | show |
Hello! > -----Original Message----- > From: Shlomo Pongratz [mailto:shlomopongratz@gmail.com] > Sent: Tuesday, October 20, 2015 8:22 PM > To: qemu-devel@nongnu.org > Cc: p.fedin@samsung.com; peter.maydell@linaro.org; eric.auger@linaro.org; > shannon.zhao@linaro.org; imammedo@redhat.com; ashoks@broadcom.com; Shlomo Pongratz > Subject: [PATCH RFC V5 9/9] hw/arm: Add virt-v3 machine that uses GIC-500 > > From: Shlomo Pongratz <shlomo.pongratz@huawei.com> > > 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 <shlomo.pongratz@huawei.com> > --- > 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
See inline. On Wednesday, October 21, 2015, Pavel Fedin <p.fedin@samsung.com> wrote: > Hello! > > > -----Original Message----- > > From: Shlomo Pongratz [mailto:shlomopongratz@gmail.com <javascript:;>] > > Sent: Tuesday, October 20, 2015 8:22 PM > > To: qemu-devel@nongnu.org <javascript:;> > > Cc: p.fedin@samsung.com <javascript:;>; peter.maydell@linaro.org > <javascript:;>; eric.auger@linaro.org <javascript:;>; > > shannon.zhao@linaro.org <javascript:;>; imammedo@redhat.com > <javascript:;>; ashoks@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.pongratz@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.pongratz@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 > >
Hello! > I see, just how do I pass the gic version from the command line? Easy. -machine virt,gic-version=3 > GICV3 is accessed by system instructions that exists only in ARCH64. Wrong. In 32-bit mode the CPU sees them as CP15 registers. See "8.5 AArch32 System register descriptions". I would say, system registers are nothing new, these are just renamed CP15 registers, with "CPn" prefix removed. Kind regards, Pavel Fedin Expert Engineer Samsung Electronics Research center Russia
See inline On Wednesday, October 21, 2015, Pavel Fedin <p.fedin@samsung.com> wrote: > Hello! > > > I see, just how do I pass the gic version from the command line? > > Easy. -machine virt,gic-version=3 > > > GICV3 is accessed by system instructions that exists only in ARCH64. > > Wrong. In 32-bit mode the CPU sees them as CP15 registers. See "8.5 > AArch32 System register descriptions". > I would say, system registers are nothing new, these are just renamed > CP15 registers, with "CPn" prefix removed. > I assume I can add the system registers to target-arm/cpu.c but I wonder if someone really needs to simulate more than 8 AArch32 CPU(s) > Kind regards, > Pavel Fedin > Expert Engineer > Samsung Electronics Research center Russia > > >
On 21 October 2015 at 12:33, Shlomo Pongratz <shlomopongratz@gmail.com> wrote: > I assume I can add the system registers to target-arm/cpu.c but I wonder if > someone really needs to simulate more than 8 AArch32 CPU(s) The system register implementation belongs in the gic code, not target-arm/. We already have support for devices that say "I have some system registers, please add them to this CPU". The mechanism is the same for system registers for both 32-bit and 64-bit, incidentally. thanks -- PMM
On Wednesday, October 21, 2015, Peter Maydell <peter.maydell@linaro.org> wrote: > On 21 October 2015 at 12:33, Shlomo Pongratz <shlomopongratz@gmail.com > <javascript:;>> wrote: > > I assume I can add the system registers to target-arm/cpu.c but I wonder > if > > someone really needs to simulate more than 8 AArch32 CPU(s) > > The system register implementation belongs in the gic code, not > target-arm/. We already have support for devices that say > "I have some system registers, please add them to this CPU". > > I don't understand. The system registers are defined in ARM Architecture reference Manual. It is true that the real implementation is in arm_gicv3_interrupts.c But the crn, crm, op0, and op1 of the instructions are in CPU domain. > The mechanism is the same for system registers for both 32-bit > and 64-bit, incidentally. > > I agree. > thanks > -- PMM >
On 21 October 2015 at 14:01, Shlomo Pongratz <shlomopongratz@gmail.com> wrote: > On Wednesday, October 21, 2015, Peter Maydell <peter.maydell@linaro.org> > wrote: >> The system register implementation belongs in the gic code, not >> target-arm/. We already have support for devices that say >> "I have some system registers, please add them to this CPU". >> > > I don't understand. > The system registers are defined in ARM Architecture reference Manual. > It is true that the real implementation is in arm_gicv3_interrupts.c > But the crn, crm, op0, and op1 of the instructions are in CPU domain. Well, this comes down to "do you want to design the GICv3 emulation to preserve the split the hardware has between the cpu interface and the GIC proper". In hardware there's actually a defined protocol between the two, so you can have CPUs from one implementor that talk to a GIC from another implementor. For QEMU that seems like overkill, as we will only ever have one GICv3 implementation and one CPU implementation. So we should just have the GICv3 provide the CPU system register implementations. But the code for those belongs in hw/intc/: that should call the function for "add these system registers" which we have already: define_arm_cp_regs_with_opaque(). (We use this in hw/arm/pxa2xx_pic, for instance.) thanks -- PMM
Hello! >> The system register implementation belongs in the gic code, not >> target-arm/. We already have support for devices that say >> "I have some system registers, please add them to this CPU". > I don't understand. > The system registers are defined in ARM Architecture reference Manual. > It is true that the real implementation is in arm_gicv3_interrupts.c > But the crn, crm, op0, and op1 of the instructions are in CPU domain. Not really. If you take a closer look, you'll see that crn, crm, op1, op2 are the same for both ARM64 and ARM32. The only difference is that ARM64 uses op0 = 3, and ARM32 uses cp = 15. Both of these specifiers can coexist in the descriptor table. I think Peter wants to tell that you should not insert your register table and registration function into target-arm/cpu64.c. This code should go to hw/intc/arm_gicv3_cpu_interface.c, add .cp = 15, and - voila - it magically works with both ARM32 and ARM64. Kind regards, Pavel Fedin Expert Engineer Samsung Electronics Research center Russia
O.K. On Wednesday, October 21, 2015, Pavel Fedin <p.fedin@samsung.com> wrote: > Hello! > > >> The system register implementation belongs in the gic code, not > >> target-arm/. We already have support for devices that say > >> "I have some system registers, please add them to this CPU". > > > I don't understand. > > The system registers are defined in ARM Architecture reference Manual. > > It is true that the real implementation is in arm_gicv3_interrupts.c > > But the crn, crm, op0, and op1 of the instructions are in CPU domain. > > Not really. If you take a closer look, you'll see that crn, crm, op1, op2 > are the same for both ARM64 and ARM32. The only difference is that ARM64 > uses op0 = 3, and ARM32 uses cp = 15. Both of these specifiers can coexist > in the descriptor table. > I think Peter wants to tell that you should not insert your register > table and registration function into target-arm/cpu64.c. This code should > go to hw/intc/arm_gicv3_cpu_interface.c, add .cp = 15, and - voila - it > magically works with both ARM32 and ARM64. > > Kind regards, > Pavel Fedin > Expert Engineer > Samsung Electronics Research center Russia > > >
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 */ [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); } 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"; +#else + error_report("KVM GICv3 acceleration is not supported on this " + "platform\n"); +#endif } exit(1);