| Message ID | 1dbab1e73e54f0fca4d6d05d50986d2dd396f867.1437731107.git.p.fedin@samsung.com |
|---|---|
| State | New |
| Headers | show |
Hi. I have been reading the GIC specs over the last few days and I think I'm getting closer to understanding what we need to do with this series to get it to a committable state (without tying it up with GICv3 emulation). I'll send some mail about that tomorrow probably, when I've figured out the details. In the meantime, some review comments below (mostly minor stuff). On 24 July 2015 at 10:55, Pavel Fedin <p.fedin@samsung.com> wrote: > Get/put routines are missing, live migration is not possible. This commit message could do with being made a bit less terse. > Signed-off-by: Pavel Fedin <p.fedin@samsung.com> > --- > hw/intc/Makefile.objs | 3 + > hw/intc/arm_gicv3_kvm.c | 155 ++++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 158 insertions(+) > create mode 100644 hw/intc/arm_gicv3_kvm.c > > diff --git a/hw/intc/Makefile.objs b/hw/intc/Makefile.objs > index 1317e5a..e2525a8 100644 > --- a/hw/intc/Makefile.objs > +++ b/hw/intc/Makefile.objs > @@ -17,6 +17,9 @@ common-obj-$(CONFIG_OPENPIC) += openpic.o > > obj-$(CONFIG_APIC) += apic.o apic_common.o > obj-$(CONFIG_ARM_GIC_KVM) += arm_gic_kvm.o > +ifeq ($(ARCH), aarch64) # Only 64-bit KVM can use these > +obj-$(CONFIG_ARM_GIC_KVM) += arm_gicv3_kvm.o Does it actually fail to compile in a 32-bit KVM config? Anyway, probably better to be consistent with how target-arm/Makefile.objs enables kvm64.o: obj-$(call land,$(CONFIG_ARM_GIC_KVM),$(TARGET_AARCH64)) += kvm64.o > +static void kvm_arm_gicv3_reset(DeviceState *dev) > +{ > + GICv3State *s = ARM_GICV3_COMMON(dev); > + KVMARMGICv3Class *kgc = KVM_ARM_GICV3_GET_CLASS(s); > + > + DPRINTF("Reset\n"); > + > + kgc->parent_reset(dev); > + kvm_arm_gicv3_put(s); > +} If we don't currently do anything in reset then does the GIC just go wrong on a VM reset? > +static void kvm_arm_gicv3_realize(DeviceState *dev, Error **errp) > +{ > + GICv3State *s = KVM_ARM_GICV3(dev); > + KVMARMGICv3Class *kgc = KVM_ARM_GICV3_GET_CLASS(s); > + Error *local_err = NULL; > + int ret; > + > + DPRINTF("kvm_arm_gicv3_realize\n"); > + > + kgc->parent_realize(dev, &local_err); > + if (local_err) { > + error_propagate(errp, local_err); > + return; > + } > + > + gicv3_init_irqs_and_mmio(s, kvm_arm_gicv3_set_irq, NULL); > + > + /* Try to create the device via the device control API */ > + s->dev_fd = -1; > + ret = kvm_create_device(kvm_state, KVM_DEV_TYPE_ARM_VGIC_V3, false); > + if (ret >= 0) { > + s->dev_fd = ret; > + } else if (ret != -ENODEV && ret != -ENOTSUP) { Why aren't ENODEV and ENOTSUP fatal errors like other errnos? > + error_setg_errno(errp, -ret, "error creating in-kernel VGIC"); > + return; > + } > + > + if (kvm_gic_supports_attr(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_NR_IRQS, 0)) { Is there any kernel which supports GICv3 but does not support this attribute? I would hope not, in which case we can skip the conditional check for support. > + uint32_t numirqs = s->num_irq; > + DPRINTF("KVM_DEV_ARM_VGIC_GRP_NR_IRQS = %u\n", numirqs); > + kvm_gic_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_NR_IRQS, > + 0, 0, &numirqs, 1); > + } > + > + /* Tell the kernel to complete VGIC initialization now */ > + if (kvm_gic_supports_attr(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_CTRL, > + KVM_DEV_ARM_VGIC_CTRL_INIT)) { Ditto. > + DPRINTF("KVM_DEV_ARM_VGIC_CTRL_INIT\n"); > + kvm_gic_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_CTRL, > + KVM_DEV_ARM_VGIC_CTRL_INIT, 0, 0, 1); > + } > + > + kvm_arm_register_device(&s->iomem_dist, -1, KVM_DEV_ARM_VGIC_GRP_ADDR, > + KVM_VGIC_V3_ADDR_TYPE_DIST, s->dev_fd); > + kvm_arm_register_device(&s->iomem_lpi, -1, KVM_DEV_ARM_VGIC_GRP_ADDR, > + KVM_VGIC_V3_ADDR_TYPE_REDIST, s->dev_fd); > +} thanks -- PMM
Hello! > On 24 July 2015 at 10:55, Pavel Fedin <p.fedin@samsung.com> wrote: > > Get/put routines are missing, live migration is not possible. > > This commit message could do with being made a bit less terse. Ok. Subject was self-explanatory, so i didn't have much to add. > > +static void kvm_arm_gicv3_reset(DeviceState *dev) > > +{ > > + GICv3State *s = ARM_GICV3_COMMON(dev); > > + KVMARMGICv3Class *kgc = KVM_ARM_GICV3_GET_CLASS(s); > > + > > + DPRINTF("Reset\n"); > > + > > + kgc->parent_reset(dev); > > + kvm_arm_gicv3_put(s); > > +} > > If we don't currently do anything in reset then does the GIC just > go wrong on a VM reset? No it doesn't, reset works. > > + /* Try to create the device via the device control API */ > > + s->dev_fd = -1; > > + ret = kvm_create_device(kvm_state, KVM_DEV_TYPE_ARM_VGIC_V3, false); > > + if (ret >= 0) { > > + s->dev_fd = ret; > > + } else if (ret != -ENODEV && ret != -ENOTSUP) { > > Why aren't ENODEV and ENOTSUP fatal errors like other errnos? Stupid leftover from vGICv2 code, copypasted. I have already removed it in my repo, just decided to delay the respin until i know the fate of this: https://lists.gnu.org/archive/html/qemu-devel/2015-07/msg05778.html > > > + error_setg_errno(errp, -ret, "error creating in-kernel VGIC"); > > + return; > > + } > > + > > + if (kvm_gic_supports_attr(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_NR_IRQS, 0)) { > > Is there any kernel which supports GICv3 but does not support > this attribute? I would hope not, in which case we can skip the > conditional check for support. > > > + uint32_t numirqs = s->num_irq; > > + DPRINTF("KVM_DEV_ARM_VGIC_GRP_NR_IRQS = %u\n", numirqs); > > + kvm_gic_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_NR_IRQS, > > + 0, 0, &numirqs, 1); > > + } > > + > > + /* Tell the kernel to complete VGIC initialization now */ > > + if (kvm_gic_supports_attr(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_CTRL, > > + KVM_DEV_ARM_VGIC_CTRL_INIT)) { > > Ditto. I intentionally put some tracing to these conditions. On my system KVM_DEV_ARM_VGIC_GRP_NR_IRQS is supported and KVM_DEV_ARM_VGIC_CTRL_INIT is not. So will it always be this way? Kind regards, Pavel Fedin Expert Engineer Samsung Electronics Research center Russia
On 31 July 2015 at 10:32, Pavel Fedin <p.fedin@samsung.com> wrote: > Hello! > >> On 24 July 2015 at 10:55, Pavel Fedin <p.fedin@samsung.com> wrote: >> > +static void kvm_arm_gicv3_reset(DeviceState *dev) >> > +{ >> > + GICv3State *s = ARM_GICV3_COMMON(dev); >> > + KVMARMGICv3Class *kgc = KVM_ARM_GICV3_GET_CLASS(s); >> > + >> > + DPRINTF("Reset\n"); >> > + >> > + kgc->parent_reset(dev); >> > + kvm_arm_gicv3_put(s); >> > +} >> >> If we don't currently do anything in reset then does the GIC just >> go wrong on a VM reset? > > No it doesn't, reset works. So who resets the KVM internal GIC state and how? >> > + error_setg_errno(errp, -ret, "error creating in-kernel VGIC"); >> > + return; >> > + } >> > + >> > + if (kvm_gic_supports_attr(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_NR_IRQS, 0)) { >> >> Is there any kernel which supports GICv3 but does not support >> this attribute? I would hope not, in which case we can skip the >> conditional check for support. >> >> > + uint32_t numirqs = s->num_irq; >> > + DPRINTF("KVM_DEV_ARM_VGIC_GRP_NR_IRQS = %u\n", numirqs); >> > + kvm_gic_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_NR_IRQS, >> > + 0, 0, &numirqs, 1); >> > + } >> > + >> > + /* Tell the kernel to complete VGIC initialization now */ >> > + if (kvm_gic_supports_attr(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_CTRL, >> > + KVM_DEV_ARM_VGIC_CTRL_INIT)) { >> >> Ditto. > > I intentionally put some tracing to these conditions. On my > system KVM_DEV_ARM_VGIC_GRP_NR_IRQS is supported and > KVM_DEV_ARM_VGIC_CTRL_INIT is not. So will it always be this way? That is weird. I thought that the kernel devs had agreed that all new VGIC code should require explicit initialization (ie that lazy init-on-first-time-it's-needed was only there for legacy guests and the old GICv2). The lazy-init stuff was a big source of bugs where things weren't inited at the point where you expected, which is why it should have gone away... -- PMM
On 31 July 2015 at 11:08, Peter Maydell <peter.maydell@linaro.org> wrote: > On 31 July 2015 at 10:32, Pavel Fedin <p.fedin@samsung.com> wrote: >> I intentionally put some tracing to these conditions. On my >> system KVM_DEV_ARM_VGIC_GRP_NR_IRQS is supported and >> KVM_DEV_ARM_VGIC_CTRL_INIT is not. So will it always be this way? > > That is weird. I thought that the kernel devs had agreed that all > new VGIC code should require explicit initialization (ie that > lazy init-on-first-time-it's-needed was only there for legacy > guests and the old GICv2). The lazy-init stuff was a big source > of bugs where things weren't inited at the point where you > expected, which is why it should have gone away... I just checked with Marc, and he agreed that all kernels with GICv3 support should support VGIC_CTRL_INIT. What kernel are you running? thanks -- PMM
Hello! > >> I intentionally put some tracing to these conditions. On my > >> system KVM_DEV_ARM_VGIC_GRP_NR_IRQS is supported and > >> KVM_DEV_ARM_VGIC_CTRL_INIT is not. So will it always be this way? > > > > That is weird. I thought that the kernel devs had agreed that all > > new VGIC code should require explicit initialization (ie that > > lazy init-on-first-time-it's-needed was only there for legacy > > guests and the old GICv2). The lazy-init stuff was a big source > > of bugs where things weren't inited at the point where you > > expected, which is why it should have gone away... > > I just checked with Marc, and he agreed that all kernels > with GICv3 support should support VGIC_CTRL_INIT. What kernel > are you running? v3.18 with backported GICv3 support. It is a choice of our HW vendor and their current official kernel. Kind regards, Pavel Fedin Expert Engineer Samsung Electronics Research center Russia
On 31 July 2015 at 12:12, Pavel Fedin <p.fedin@samsung.com> wrote: > Hello! > >> >> I intentionally put some tracing to these conditions. On my >> >> system KVM_DEV_ARM_VGIC_GRP_NR_IRQS is supported and >> >> KVM_DEV_ARM_VGIC_CTRL_INIT is not. So will it always be this way? >> > >> > That is weird. I thought that the kernel devs had agreed that all >> > new VGIC code should require explicit initialization (ie that >> > lazy init-on-first-time-it's-needed was only there for legacy >> > guests and the old GICv2). The lazy-init stuff was a big source >> > of bugs where things weren't inited at the point where you >> > expected, which is why it should have gone away... >> >> I just checked with Marc, and he agreed that all kernels >> with GICv3 support should support VGIC_CTRL_INIT. What kernel >> are you running? > > v3.18 with backported GICv3 support. It is a choice of our > HW vendor and their current official kernel. Sounds like their backporting is buggy, then. They need to fix it, I don't think we need to support broken branches in QEMU. thanks -- PMM
Hello! > >> I just checked with Marc, and he agreed that all kernels > >> with GICv3 support should support VGIC_CTRL_INIT. What kernel > >> are you running? > > > > v3.18 with backported GICv3 support. It is a choice of our > > HW vendor and their current official kernel. > > Sounds like their backporting is buggy, then. They need to > fix it, I don't think we need to support broken branches > in QEMU. Ok, i will recheck it. So, final conclusion: both conditions should be removed and their code should be executed unconditionally. Correct ? Kind regards, Pavel Fedin Expert Engineer Samsung Electronics Research center Russia
On 31 July 2015 at 12:15, Pavel Fedin <p.fedin@samsung.com> wrote: > Hello! > >> >> I just checked with Marc, and he agreed that all kernels >> >> with GICv3 support should support VGIC_CTRL_INIT. What kernel >> >> are you running? >> > >> > v3.18 with backported GICv3 support. It is a choice of our >> > HW vendor and their current official kernel. >> >> Sounds like their backporting is buggy, then. They need to >> fix it, I don't think we need to support broken branches >> in QEMU. > > Ok, i will recheck it. > So, final conclusion: both conditions should be removed and their > code should be executed unconditionally. Correct ? Yes. thanks -- PMM
Hello! > > obj-$(CONFIG_APIC) += apic.o apic_common.o > > obj-$(CONFIG_ARM_GIC_KVM) += arm_gic_kvm.o > > +ifeq ($(ARCH), aarch64) # Only 64-bit KVM can use these > > +obj-$(CONFIG_ARM_GIC_KVM) += arm_gicv3_kvm.o > > Does it actually fail to compile in a 32-bit KVM config? I forgot to reply to this. Yes, it does, because KVM_VGIC_V3_ADDR_TYPE_xxx definitions are available only for 64 bits. Kind regards, Pavel Fedin Expert Engineer Samsung Electronics Research center Russia
diff --git a/hw/intc/Makefile.objs b/hw/intc/Makefile.objs index 1317e5a..e2525a8 100644 --- a/hw/intc/Makefile.objs +++ b/hw/intc/Makefile.objs @@ -17,6 +17,9 @@ common-obj-$(CONFIG_OPENPIC) += openpic.o obj-$(CONFIG_APIC) += apic.o apic_common.o obj-$(CONFIG_ARM_GIC_KVM) += arm_gic_kvm.o +ifeq ($(ARCH), aarch64) # Only 64-bit KVM can use these +obj-$(CONFIG_ARM_GIC_KVM) += arm_gicv3_kvm.o +endif obj-$(CONFIG_STELLARIS) += armv7m_nvic.o obj-$(CONFIG_EXYNOS4) += exynos4210_gic.o exynos4210_combiner.o obj-$(CONFIG_GRLIB) += grlib_irqmp.o diff --git a/hw/intc/arm_gicv3_kvm.c b/hw/intc/arm_gicv3_kvm.c new file mode 100644 index 0000000..5cb06e8 --- /dev/null +++ b/hw/intc/arm_gicv3_kvm.c @@ -0,0 +1,155 @@ +/* + * ARM Generic Interrupt Controller using KVM in-kernel support + * + * Copyright (c) 2015 Samsung Electronics Co., Ltd. + * Written by Pavel Fedin + * Based on vGICv2 code by Peter Maydell + * + * 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/sysbus.h" +#include "sysemu/kvm.h" +#include "kvm_arm.h" +#include "gicv3_internal.h" +#include "vgic_common.h" + +#ifdef DEBUG_GICV3_KVM +#define DPRINTF(fmt, ...) \ + do { fprintf(stderr, "kvm_gicv3: " fmt, ## __VA_ARGS__); } while (0) +#else +#define DPRINTF(fmt, ...) \ + do { } while (0) +#endif + +#define TYPE_KVM_ARM_GICV3 "kvm-arm-gicv3" +#define KVM_ARM_GICV3(obj) \ + OBJECT_CHECK(GICv3State, (obj), TYPE_KVM_ARM_GICV3) +#define KVM_ARM_GICV3_CLASS(klass) \ + OBJECT_CLASS_CHECK(KVMARMGICv3Class, (klass), TYPE_KVM_ARM_GICV3) +#define KVM_ARM_GICV3_GET_CLASS(obj) \ + OBJECT_GET_CLASS(KVMARMGICv3Class, (obj), TYPE_KVM_ARM_GICV3) + +typedef struct KVMARMGICv3Class { + ARMGICv3CommonClass parent_class; + DeviceRealize parent_realize; + void (*parent_reset)(DeviceState *dev); +} KVMARMGICv3Class; + +static void kvm_arm_gicv3_set_irq(void *opaque, int irq, int level) +{ + GICv3State *s = (GICv3State *)opaque; + + kvm_arm_gic_set_irq(s->num_irq, irq, level); +} + +static void kvm_arm_gicv3_put(GICv3State *s) +{ + /* TODO */ + DPRINTF("Cannot put kernel gic state, no kernel interface\n"); +} + +static void kvm_arm_gicv3_get(GICv3State *s) +{ + /* TODO */ + DPRINTF("Cannot get kernel gic state, no kernel interface\n"); +} + +static void kvm_arm_gicv3_reset(DeviceState *dev) +{ + GICv3State *s = ARM_GICV3_COMMON(dev); + KVMARMGICv3Class *kgc = KVM_ARM_GICV3_GET_CLASS(s); + + DPRINTF("Reset\n"); + + kgc->parent_reset(dev); + kvm_arm_gicv3_put(s); +} + +static void kvm_arm_gicv3_realize(DeviceState *dev, Error **errp) +{ + GICv3State *s = KVM_ARM_GICV3(dev); + KVMARMGICv3Class *kgc = KVM_ARM_GICV3_GET_CLASS(s); + Error *local_err = NULL; + int ret; + + DPRINTF("kvm_arm_gicv3_realize\n"); + + kgc->parent_realize(dev, &local_err); + if (local_err) { + error_propagate(errp, local_err); + return; + } + + gicv3_init_irqs_and_mmio(s, kvm_arm_gicv3_set_irq, NULL); + + /* Try to create the device via the device control API */ + s->dev_fd = -1; + ret = kvm_create_device(kvm_state, KVM_DEV_TYPE_ARM_VGIC_V3, false); + if (ret >= 0) { + s->dev_fd = ret; + } else if (ret != -ENODEV && ret != -ENOTSUP) { + error_setg_errno(errp, -ret, "error creating in-kernel VGIC"); + return; + } + + if (kvm_gic_supports_attr(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_NR_IRQS, 0)) { + uint32_t numirqs = s->num_irq; + DPRINTF("KVM_DEV_ARM_VGIC_GRP_NR_IRQS = %u\n", numirqs); + kvm_gic_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_NR_IRQS, + 0, 0, &numirqs, 1); + } + + /* Tell the kernel to complete VGIC initialization now */ + if (kvm_gic_supports_attr(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_CTRL, + KVM_DEV_ARM_VGIC_CTRL_INIT)) { + DPRINTF("KVM_DEV_ARM_VGIC_CTRL_INIT\n"); + kvm_gic_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_CTRL, + KVM_DEV_ARM_VGIC_CTRL_INIT, 0, 0, 1); + } + + kvm_arm_register_device(&s->iomem_dist, -1, KVM_DEV_ARM_VGIC_GRP_ADDR, + KVM_VGIC_V3_ADDR_TYPE_DIST, s->dev_fd); + kvm_arm_register_device(&s->iomem_lpi, -1, KVM_DEV_ARM_VGIC_GRP_ADDR, + KVM_VGIC_V3_ADDR_TYPE_REDIST, s->dev_fd); +} + +static void kvm_arm_gicv3_class_init(ObjectClass *klass, void *data) +{ + DeviceClass *dc = DEVICE_CLASS(klass); + ARMGICv3CommonClass *agcc = ARM_GICV3_COMMON_CLASS(klass); + KVMARMGICv3Class *kgc = KVM_ARM_GICV3_CLASS(klass); + + agcc->pre_save = kvm_arm_gicv3_get; + agcc->post_load = kvm_arm_gicv3_put; + kgc->parent_realize = dc->realize; + kgc->parent_reset = dc->reset; + dc->realize = kvm_arm_gicv3_realize; + dc->reset = kvm_arm_gicv3_reset; +} + +static const TypeInfo kvm_arm_gicv3_info = { + .name = TYPE_KVM_ARM_GICV3, + .parent = TYPE_ARM_GICV3_COMMON, + .instance_size = sizeof(GICv3State), + .class_init = kvm_arm_gicv3_class_init, + .class_size = sizeof(KVMARMGICv3Class), +}; + +static void kvm_arm_gicv3_register_types(void) +{ + type_register_static(&kvm_arm_gicv3_info); +} + +type_init(kvm_arm_gicv3_register_types)
Get/put routines are missing, live migration is not possible. Signed-off-by: Pavel Fedin <p.fedin@samsung.com> --- hw/intc/Makefile.objs | 3 + hw/intc/arm_gicv3_kvm.c | 155 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 158 insertions(+) create mode 100644 hw/intc/arm_gicv3_kvm.c