Re: [Qemu-devel] [PATCH v7 5/6] Initial implementation of vGICv3

[Peter Maydell]

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 <address@hidden> 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 <address@hidden>
> ---
>  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