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.fe...@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.fe...@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