[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH RFC 1/4] Add virt-v3 machine that uses GIC-500
|
From: |
Eric Auger |
|
Subject: |
Re: [Qemu-devel] [PATCH RFC 1/4] Add virt-v3 machine that uses GIC-500 |
|
Date: |
Mon, 25 May 2015 16:07:17 +0200 |
|
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 |
Hi Pavel,
On 05/22/2015 12:58 PM, Pavel Fedin wrote:
> This patch introduces kernel_irqchip_type member in Machine class. Currently
> it it used only by virt machine for its internal purposes, however in future
> it is to be passed to KVM in kvm_irqchip_create(). The variable is defined as
> int in order to be architecture agnostic, for potential future users.
Well your patch does much more than that. To me you put additional info
in the commit message such as rationale behind introducing a new
machine, what does it feature compared to legacy virt, ...
>
> Signed-off-by: Pavel Fedin <address@hidden>
>
> ---
> hw/arm/virt.c | 148
> +++++++++++++++++++++++++++++++++++++++++++---------
> include/hw/boards.h | 1 +
> 2 files changed, 123 insertions(+), 26 deletions(-)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index a1186c5..15724b2 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -66,6 +66,10 @@ enum {
> VIRT_CPUPERIPHS,
> VIRT_GIC_DIST,
> VIRT_GIC_CPU,
> + VIRT_GIC_DIST_SPI = VIRT_GIC_CPU,
I have the same comment as in Shlomo's patch about the naming of those
regions. Maybe you use another doc than GICv3 archi spec?
If confirmed it matches GICv2M compat region, why not VIRT_GIC_DIST_MBI?
> + VIRT_ITS_CONTROL,
> + VIRT_ITS_TRANSLATION,
> + VIRT_LPI,
VIRT_GIC_REDIST?
> VIRT_UART,
> VIRT_MMIO,
> VIRT_RTC,
> @@ -107,6 +111,8 @@ typedef struct {
> #define VIRT_MACHINE_CLASS(klass) \
> OBJECT_CLASS_CHECK(VirtMachineClass, klass, TYPE_VIRT_MACHINE)
>
> +#define TYPE_VIRTV3_MACHINE "virt-v3"
> +
> /* Addresses and sizes of our components.
> * 0..128MB is space for a flash device so we can run bootrom code such as
> UEFI.
> * 128MB..256MB is used for miscellaneous device I/O.
> @@ -121,25 +127,29 @@ typedef struct {
> */
> static const MemMapEntry a15memmap[] = {
> /* Space up to 0x8000000 is reserved for a boot ROM */
> - [VIRT_FLASH] = { 0, 0x08000000 },
> - [VIRT_CPUPERIPHS] = { 0x08000000, 0x00020000 },
> + [VIRT_FLASH] = { 0, 0x08000000 },
> + [VIRT_CPUPERIPHS] = { 0x08000000, 0x00020000 },
> /* GIC distributor and CPU interfaces sit inside the CPU peripheral
> space */
> - [VIRT_GIC_DIST] = { 0x08000000, 0x00010000 },
> - [VIRT_GIC_CPU] = { 0x08010000, 0x00010000 },
> - [VIRT_UART] = { 0x09000000, 0x00001000 },
> - [VIRT_RTC] = { 0x09010000, 0x00001000 },
> - [VIRT_FW_CFG] = { 0x09020000, 0x0000000a },
> - [VIRT_MMIO] = { 0x0a000000, 0x00000200 },
I was advised by Peter, in the past, to handle indent changes in a
different patch to ease the review.
> + [VIRT_GIC_DIST] = { 0x08000000, 0x00010000 },
> + [VIRT_GIC_CPU] = { 0x08010000, 0x00010000 },
> + /* On v3 VIRT_GIC_DIST_SPI takes place of VIRT_GIC_CPU */
With GICv3 VIRT_GIC_CPU region is not used since system registers are
used instead and the memory map hole is reused for GIC DIST MBI region?
> + [VIRT_ITS_CONTROL] = { 0x08020000, 0x00010000 },
> + [VIRT_ITS_TRANSLATION] = { 0x08030000, 0x00010000 },
> + [VIRT_LPI] = { 0x08040000, 0x00800000 },
> + [VIRT_UART] = { 0x09000000, 0x00001000 },
> + [VIRT_RTC] = { 0x09010000, 0x00001000 },
> + [VIRT_FW_CFG] = { 0x09020000, 0x0000000a },
> + [VIRT_MMIO] = { 0x0a000000, 0x00000200 },
> /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size
> */
> /*
> * PCIE verbose map:
> *
> - * MMIO window { 0x10000000, 0x2eff0000 },
> - * PIO window { 0x3eff0000, 0x00010000 },
> - * ECAM { 0x3f000000, 0x01000000 },
> + * MMIO window { 0x10000000, 0x2eff0000 },
> + * PIO window { 0x3eff0000, 0x00010000 },
> + * ECAM { 0x3f000000, 0x01000000 },
> */
> - [VIRT_PCIE] = { 0x10000000, 0x30000000 },
> - [VIRT_MEM] = { 0x40000000, 30ULL * 1024 * 1024 * 1024 },
> + [VIRT_PCIE] = { 0x10000000, 0x30000000 },
> + [VIRT_MEM] = { 0x40000000, 30ULL * 1024 * 1024 * 1024 },
> };
>
> static const int a15irqmap[] = {
> @@ -273,9 +283,11 @@ static void fdt_add_timer_nodes(const VirtBoardInfo *vbi)
> */
> ARMCPU *armcpu;
> uint32_t irqflags = GIC_FDT_IRQ_FLAGS_EDGE_LO_HI;
> + /* Argument is 32 bit but 8 bits are reserved for flags */
argument of what?
> + uint32_t 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");
>
> @@ -299,6 +311,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);
> @@ -326,7 +350,7 @@ static void fdt_add_cpu_nodes(const VirtBoardInfo *vbi)
> }
> }
>
> -static uint32_t fdt_add_gic_node(const VirtBoardInfo *vbi)
> +static uint32_t fdt_add_gic_node(const VirtBoardInfo *vbi, int type)
> {
> uint32_t gic_phandle;
>
> @@ -334,35 +358,65 @@ static uint32_t fdt_add_gic_node(const VirtBoardInfo
> *vbi)
> qemu_fdt_setprop_cell(vbi->fdt, "/", "interrupt-parent", 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);
Documentation/devicetree/bindings/arm/gic-v3.txt says
"The main GIC node must contain the appropriate #address-cells,
#size-cells and ranges properties for the reg property of all ITS
nodes." or to be done when adding the ITS nodes later on ...
> qemu_fdt_setprop(vbi->fdt, "/intc", "interrupt-controller", NULL, 0);
> - qemu_fdt_setprop_sized_cells(vbi->fdt, "/intc", "reg",
> + 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,
> +#if 0 /* Currently no need for SPI & ITS */
s/SPI/MBI, dead code to be removed when moving to PATCH
> + 2, vbi->memmap[VIRT_GIC_DIST_SPI].base,
> + 2, vbi->memmap[VIRT_GIC_DIST_SPI].size,
> + 2, vbi->memmap[VIRT_ITS_CONTROL].base,
> + 2, vbi->memmap[VIRT_ITS_CONTROL].size,
> + 2,
> vbi->memmap[VIRT_ITS_TRANSLATION].base,
> + 2,
> vbi->memmap[VIRT_ITS_TRANSLATION].size,
indeed to be moved in an ITS sub-node
> +#endif
> + 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", gic_phandle);
>
> return gic_phandle;
> }
>
> -static uint32_t create_gic(const VirtBoardInfo *vbi, qemu_irq *pic)
> +static uint32_t create_gic(const 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()) {
> + if (type == KVM_DEV_TYPE_ARM_VGIC_V3) {
> + gictype = "arm_gicv3";
> + } else if (kvm_irqchip_in_kernel()) {
> gictype = "kvm-arm-gic";
> + } else {
> + gictype = "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).
> @@ -372,6 +426,11 @@ static uint32_t create_gic(const VirtBoardInfo *vbi,
> qemu_irq *pic)
> 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, 2, vbi->memmap[VIRT_ITS_CONTROL].base);
> + sysbus_mmio_map(gicbusdev, 3,
> vbi->memmap[VIRT_ITS_TRANSLATION].base);
> + sysbus_mmio_map(gicbusdev, 4, vbi->memmap[VIRT_LPI].base);
> + }
>
> /* Wire the outputs from each CPU's generic timer to the
> * appropriate GIC PPI inputs, and the GIC's IRQ output to
> @@ -398,7 +457,7 @@ static uint32_t create_gic(const VirtBoardInfo *vbi,
> qemu_irq *pic)
> pic[i] = qdev_get_gpio_in(gicdev, i);
> }
>
> - return fdt_add_gic_node(vbi);
> + return fdt_add_gic_node(vbi, type);
> }
>
> static void create_uart(const VirtBoardInfo *vbi, qemu_irq *pic)
> @@ -817,7 +876,7 @@ static void machvirt_init(MachineState *machine)
>
> create_flash(vbi);
>
> - gic_phandle = create_gic(vbi, pic);
> + gic_phandle = create_gic(vbi, pic, machine->kernel_irqchip_type);
>
> create_uart(vbi, pic);
>
> @@ -859,7 +918,7 @@ static void virt_set_secure(Object *obj, bool value,
> Error **errp)
> vms->secure = value;
> }
>
> -static void virt_instance_init(Object *obj)
> +static void virt_instance_init_common(Object *obj)
> {
> VirtMachineState *vms = VIRT_MACHINE(obj);
>
> @@ -873,6 +932,14 @@ static void virt_instance_init(Object *obj)
> NULL);
> }
>
> +static void virt_instance_init(Object *obj)
> +{
> + MachineState *ms = MACHINE(obj);
> +
> + ms->kernel_irqchip_type = KVM_DEV_TYPE_ARM_VGIC_V2;
> + virt_instance_init_common(obj);
> +}
> +
> static void virt_class_init(ObjectClass *oc, void *data)
> {
> MachineClass *mc = MACHINE_CLASS(oc);
> @@ -892,9 +959,38 @@ static const TypeInfo machvirt_info = {
> .class_init = virt_class_init,
> };
>
> +static void virtv3_instance_init(Object *obj)
> +{
> + MachineState *ms = MACHINE(obj);
> +
> + ms->kernel_irqchip_type = KVM_DEV_TYPE_ARM_VGIC_V3;
> + virt_instance_init_common(obj);
> +}
> +
> +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;
> + /* With gic3 full implementation (with bitops) rase the lmit to 128 */
raise, limit
> + mc->max_cpus = 64;
> +}
> +
> +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);
> }
So about choice between using a new machine name/reusing legacy with
prop and interrupt controller type in machine base class I let Peter
give his guidance...
Best Regards
Eric
>
> machine_init(machvirt_machine_init);
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 1f11881..3eb32f2 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -138,6 +138,7 @@ struct MachineState {
> char *accel;
> bool kernel_irqchip_allowed;
> bool kernel_irqchip_required;
> + int kernel_irqchip_type;
> int kvm_shadow_mem;
> char *dtb;
> char *dumpdtb;
>