| Message ID | c7f3e909955e510a49728b24dfcae2f5e50566df.1439207299.git.p.fedin@samsung.com |
|---|---|
| State | New |
| Headers | show |
On 10 August 2015 at 13:06, Pavel Fedin <p.fedin@samsung.com> wrote: > From: Shlomo Pongratz <shlomo.pongratz@huawei.com> > > This class is to be used by both software and KVM implementations of GICv3 > > Currently it is mostly a placeholder, but in future it is supposed to hold > qemu's representation of GICv3 state, which is necessary for migration. > > Signed-off-by: Shlomo Pongratz <shlomo.pongratz@huawei.com> > Signed-off-by: Pavel Fedin <p.fedin@samsung.com> > + memory_region_init_io(&s->iomem_dist, OBJECT(s), ops, s, > + "gicv3_dist", 0x10000); > + memory_region_init_io(&s->iomem_redist, OBJECT(s), ops ? &ops[1] : NULL, s, > + "gicv3_redist", 0x800000); Why is the redistributor region so huge? The spec says a redistributor has four 64KB frames in its memory map: * RD_base * SGI_base * VLPI_base * Reserved which is a lot smaller than this memory region... > + > +/* Maximum number of possible interrupts, determined by the GIC architecture */ > +#define GICV3_MAXIRQ 1020 The comment could use updating, because part of the point of GICv3 is to not have that limit... > +#define GICV3_NCPU 64 What is imposing this NCPU limit? > + > +typedef struct GICv3State { > + /*< private >*/ > + SysBusDevice parent_obj; > + /*< public >*/ > + > + qemu_irq parent_irq[GICV3_NCPU]; > + qemu_irq parent_fiq[GICV3_NCPU]; We should be allocating memory for the right number of irqs and fiqs based on the number of actual CPUs, because we don't really want a compile-time arbitrary limit on NCPUs. thanks -- PMM
> > +#define GICV3_NCPU 64 > > What is imposing this NCPU limit? Currently qemu does not support Aff2 field. Can we have more than 64 CPUs only with Aff0 and Aff1? Well, if you really-really insist, i can just raise it to 128 and stop it finally. It's actually Shlomo's heritage, and i believe his SW emulation will decrease this down to 64 again. It can be changed at any moment, i don't see any serious obstacles for it. You ask this question for the 3rd time IIRC, and i answer the same thing for the 3rd time. Does this little #define really worth it? Shlomo, your word? > > + qemu_irq parent_irq[GICV3_NCPU]; > > + qemu_irq parent_fiq[GICV3_NCPU]; > > We should be allocating memory for the right number of irqs > and fiqs based on the number of actual CPUs, because we don't > really want a compile-time arbitrary limit on NCPUs. These arrays are just placeholders, so we reserve the maximum possible space for them. When we initialize the actual objects, of course we use only 'num_cpu' slots in each of the array. sizeof(qemu_irq) is rather small (IIRC it's a pointer accompanied by number). Does it worth g_malloc()ing them dynamically? Kind regards, Pavel Fedin Expert Engineer Samsung Electronics Research center Russia
On 11 August 2015 at 08:53, Pavel Fedin <p.fedin@samsung.com> wrote: >> > +#define GICV3_NCPU 64 >> >> What is imposing this NCPU limit? > > Currently qemu does not support Aff2 field. Can we have more > than 64 CPUs only with Aff0 and Aff1? The GIC code itself doesn't care, so it shouldn't be imposing its own limit, even if other parts of the code do for now. > Well, if you really-really insist, i can just raise it to 128 ...and where does 128 come from? > and stop it finally. It's actually Shlomo's heritage, and i believe > his SW emulation will decrease this down to 64 again. No it won't, because "don't impose an arbitrary 64 bit limit" was one of my review comments on the emulation code; that will need to be fixed before the emulation code can be accepted. > It can be changed at any moment, i don't see any serious obstacles > for it. You ask this question for the 3rd time IIRC, and i answer > the same thing for the 3rd time. Yes, if you don't make changes that I think are needed then I'll continue to point them out in future review rounds... Part of the aim of the GICv3 code is to not have an arbitrary limit on the number of vcpus we can have. In the GICv2 code we have a #define for the maximum number of CPUs, because the GICv2 architecture itself sets that maximum. For GICv3 I don't want to bake an arbitrary limit into the code if it's not a limit imposed by the GICv3 architecture. >> > + qemu_irq parent_irq[GICV3_NCPU]; >> > + qemu_irq parent_fiq[GICV3_NCPU]; >> >> We should be allocating memory for the right number of irqs >> and fiqs based on the number of actual CPUs, because we don't >> really want a compile-time arbitrary limit on NCPUs. > > These arrays are just placeholders, so we reserve the maximum > possible space for them. When we initialize the actual objects, > of course we use only 'num_cpu' slots in each of the array. > sizeof(qemu_irq) is rather small (IIRC it's a pointer accompanied > by number). Does it worth g_malloc()ing them dynamically? It's not the memory usage so much as that if NCPU isn't a compile time constant you can't have them be plain arrays. -- PMM
Hello! > No it won't, because "don't impose an arbitrary 64 bit limit" > was one of my review comments on the emulation code; that > will need to be fixed before the emulation code can be accepted. Sorry for may be being ignorant, i really had no time to read GICv3 arch manual from beginning to end. Do you want to tell me that GICv3 architecture puts no limit at all on CPU number, so i could have 128, 1024, 134435242, etc ? Kind regards, Pavel Fedin Expert Engineer Samsung Electronics Research center Russia
On 11 August 2015 at 10:35, Pavel Fedin <p.fedin@samsung.com> wrote: > Hello! > >> No it won't, because "don't impose an arbitrary 64 bit limit" >> was one of my review comments on the emulation code; that >> will need to be fixed before the emulation code can be accepted. > > Sorry for may be being ignorant, i really had no time to read GICv3 > arch manual from beginning to end. Do you want to tell me that GICv3 > architecture puts no limit at all on CPU number, so i could have 128, > 1024, 134435242, etc ? Not as far as I know, beyond the limitations on the Affinity fields (Aff0 recommended to be 0..15, and the total number allowed via all 4 affinity fields). In any case, if you want to impose a compile-time limit in the QEMU code then you need to point out the part of the GIC spec that imposes that limit. thanks -- PMM
Hello! > In any case, if you want > to impose a compile-time limit in the QEMU code then you need > to point out the part of the GIC spec that imposes that limit. Ok, i agreed and gave up. Will do in v9. :) By the way, how to migrate such a thing? Is migration of variable-length state structures supported? Kind regards, Pavel Fedin Expert Engineer Samsung Electronics Research center Russia
On 11 August 2015 at 11:39, Pavel Fedin <p.fedin@samsung.com> wrote: > By the way, how to migrate such a thing? Is migration of > variable-length state structures supported? Yes; this is what the _VARRAY_ vmstate macros are for. -- PMM
diff --git a/hw/intc/Makefile.objs b/hw/intc/Makefile.objs index 092d8a8..1317e5a 100644 --- a/hw/intc/Makefile.objs +++ b/hw/intc/Makefile.objs @@ -12,6 +12,7 @@ common-obj-$(CONFIG_IOAPIC) += ioapic_common.o common-obj-$(CONFIG_ARM_GIC) += arm_gic_common.o common-obj-$(CONFIG_ARM_GIC) += arm_gic.o common-obj-$(CONFIG_ARM_GIC) += arm_gicv2m.o +common-obj-$(CONFIG_ARM_GIC) += arm_gicv3_common.o common-obj-$(CONFIG_OPENPIC) += openpic.o obj-$(CONFIG_APIC) += apic.o apic_common.o diff --git a/hw/intc/arm_gicv3_common.c b/hw/intc/arm_gicv3_common.c new file mode 100644 index 0000000..4e19e5c --- /dev/null +++ b/hw/intc/arm_gicv3_common.c @@ -0,0 +1,147 @@ +/* + * ARM GICv3 support - common bits of emulated and KVM kernel model + * + * Copyright (c) 2012 Linaro Limited + * Copyright (c) 2015 Huawei. + * Written by Peter Maydell + * Extended to 64 cores by Shlomo Pongratz + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License along + * with this program; if not, see <http://www.gnu.org/licenses/>. + */ + +#include "hw/intc/arm_gicv3_common.h" + +static void gicv3_pre_save(void *opaque) +{ + GICv3State *s = (GICv3State *)opaque; + ARMGICv3CommonClass *c = ARM_GICV3_COMMON_GET_CLASS(s); + + if (c->pre_save) { + c->pre_save(s); + } +} + +static int gicv3_post_load(void *opaque, int version_id) +{ + GICv3State *s = (GICv3State *)opaque; + ARMGICv3CommonClass *c = ARM_GICV3_COMMON_GET_CLASS(s); + + if (c->post_load) { + c->post_load(s); + } + return 0; +} + +static const VMStateDescription vmstate_gicv3 = { + .name = "arm_gicv3", + .unmigratable = 1, + .pre_save = gicv3_pre_save, + .post_load = gicv3_post_load, +}; + +void gicv3_init_irqs_and_mmio(GICv3State *s, qemu_irq_handler handler, + const MemoryRegionOps *ops) +{ + SysBusDevice *sbd = SYS_BUS_DEVICE(s); + int i; + + /* For the GIC, also expose incoming GPIO lines for PPIs for each CPU. + * GPIO array layout is thus: + * [0..N-1] spi + * [N..N+31] PPIs for CPU 0 + * [N+32..N+63] PPIs for CPU 1 + * ... + */ + i = s->num_irq - GIC_INTERNAL + GIC_INTERNAL * s->num_cpu; + qdev_init_gpio_in(DEVICE(s), handler, i); + + for (i = 0; i < s->num_cpu; i++) { + sysbus_init_irq(sbd, &s->parent_irq[i]); + } + for (i = 0; i < s->num_cpu; i++) { + sysbus_init_irq(sbd, &s->parent_fiq[i]); + } + + memory_region_init_io(&s->iomem_dist, OBJECT(s), ops, s, + "gicv3_dist", 0x10000); + memory_region_init_io(&s->iomem_redist, OBJECT(s), ops ? &ops[1] : NULL, s, + "gicv3_redist", 0x800000); + + sysbus_init_mmio(sbd, &s->iomem_dist); + sysbus_init_mmio(sbd, &s->iomem_redist); +} + +static void arm_gicv3_common_realize(DeviceState *dev, Error **errp) +{ + GICv3State *s = ARM_GICV3_COMMON(dev); + + if (s->num_cpu > GICV3_NCPU) { + error_setg(errp, "requested %u CPUs exceeds GIC maximum %d", + s->num_cpu, GICV3_NCPU); + return; + } + if (s->num_irq > GICV3_MAXIRQ) { + error_setg(errp, + "requested %u interrupt lines exceeds GIC maximum %d", + s->num_irq, GICV3_MAXIRQ); + return; + } + /* ITLinesNumber is represented as (N / 32) - 1 (see + * gic_dist_readb) so this is an implementation imposed + * restriction, not an architectural one: + */ + if (s->num_irq < 32 || (s->num_irq % 32)) { + error_setg(errp, + "%d interrupt lines unsupported: not divisible by 32", + s->num_irq); + return; + } +} + +static void arm_gicv3_common_reset(DeviceState *dev) +{ + /* TODO */ +} + +static Property arm_gicv3_common_properties[] = { + DEFINE_PROP_UINT32("num-cpu", GICv3State, num_cpu, 1), + DEFINE_PROP_UINT32("num-irq", GICv3State, num_irq, 32), + DEFINE_PROP_END_OF_LIST(), +}; + +static void arm_gicv3_common_class_init(ObjectClass *klass, void *data) +{ + DeviceClass *dc = DEVICE_CLASS(klass); + + dc->reset = arm_gicv3_common_reset; + dc->realize = arm_gicv3_common_realize; + dc->props = arm_gicv3_common_properties; + dc->vmsd = &vmstate_gicv3; +} + +static const TypeInfo arm_gicv3_common_type = { + .name = TYPE_ARM_GICV3_COMMON, + .parent = TYPE_SYS_BUS_DEVICE, + .instance_size = sizeof(GICv3State), + .class_size = sizeof(ARMGICv3CommonClass), + .class_init = arm_gicv3_common_class_init, + .abstract = true, +}; + +static void register_types(void) +{ + type_register_static(&arm_gicv3_common_type); +} + +type_init(register_types) diff --git a/include/hw/intc/arm_gicv3_common.h b/include/hw/intc/arm_gicv3_common.h new file mode 100644 index 0000000..c47cba5 --- /dev/null +++ b/include/hw/intc/arm_gicv3_common.h @@ -0,0 +1,70 @@ +/* + * ARM GIC support + * + * Copyright (c) 2012 Linaro Limited + * Copyright (c) 2015 Huawei. + * Written by Peter Maydell + * Extended to 64 cores by Shlomo Pongratz + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License along + * with this program; if not, see <http://www.gnu.org/licenses/>. + */ + +#ifndef HW_ARM_GICV3_COMMON_H +#define HW_ARM_GICV3_COMMON_H + +#include "hw/sysbus.h" +#include "hw/intc/arm_gic_common.h" + +/* Maximum number of possible interrupts, determined by the GIC architecture */ +#define GICV3_MAXIRQ 1020 +#define GICV3_NCPU 64 + +typedef struct GICv3State { + /*< private >*/ + SysBusDevice parent_obj; + /*< public >*/ + + qemu_irq parent_irq[GICV3_NCPU]; + qemu_irq parent_fiq[GICV3_NCPU]; + + MemoryRegion iomem_dist; /* Distributor */ + MemoryRegion iomem_redist; /* Redistributor */ + + uint32_t num_cpu; + uint32_t num_irq; + + int dev_fd; /* kvm device fd if backed by kvm vgic support */ +} GICv3State; + +#define TYPE_ARM_GICV3_COMMON "arm_gicv3_common" +#define ARM_GICV3_COMMON(obj) \ + OBJECT_CHECK(GICv3State, (obj), TYPE_ARM_GICV3_COMMON) +#define ARM_GICV3_COMMON_CLASS(klass) \ + OBJECT_CLASS_CHECK(ARMGICv3CommonClass, (klass), TYPE_ARM_GICV3_COMMON) +#define ARM_GICV3_COMMON_GET_CLASS(obj) \ + OBJECT_GET_CLASS(ARMGICv3CommonClass, (obj), TYPE_ARM_GICV3_COMMON) + +typedef struct ARMGICv3CommonClass { + /*< private >*/ + SysBusDeviceClass parent_class; + /*< public >*/ + + void (*pre_save)(GICv3State *s); + void (*post_load)(GICv3State *s); +} ARMGICv3CommonClass; + +void gicv3_init_irqs_and_mmio(GICv3State *s, qemu_irq_handler handler, + const MemoryRegionOps *ops); + +#endif