Re: [Qemu-devel] [RFC PATCH] hw/arm/virt: Added preliminary GICv3 support for kvm mode

[Pavel Fedin]

 Hello! Sorry, missed this message, catching up...

> After a closer look, kvm_arch_irqchip_create is called here with test
> mode = true. With that option, at kernel level, we only check the kvm
> device registered its ops and do not go further (see
> kvm_ioctl_create_device in kvm_main.c): typically the gic kvm_device is
> not created at that stage.

 Ah, sorry, yes. I missed KVM_CREATE_DEVICE_TEST flag.

> So if my understanding is correct, that piece
> of code only should & does check either GICv2 or GICV3 was seen in the
> host dt and accordingly registered. I think Ashok
> kvm_arch_irqchip_create code is good here.

 IMHO not really because from the beginning we want only a particular GIC type 
instead of
just something. And there can be a situation where we want e.g. GICv3 in our 
virtual
machine, but only GICv2 is supported by the KVM (because of hardware). Or vice 
versa, we
could ask for GICv2, but KVM could do only GICv3 (not all GICv3's have backwards
compatibility option).
 Ashok's code would become confused in this case. It will probe for anything 
and return
OK, while the kernel actually cannot do what we really want.
 And one more glitch. In case if we want GICv3, we should not return just 
false, because
in this case qemu falls back to KVM_CREATE_IRQCHIP ioctl, which does not have 
test mode,
and will actually instantiate GICv2. I believe this can cause problems.
 To be more clear, here is a snip from my code. I can post the whole thing as 
RFC patches,
but they are not quite ready yet.
--- cut ---
int kvm_arch_irqchip_create(KVMState *s, int type)
{
    int ret;

    /* If we can create the VGIC using the newer device control API, we
     * let the device do this when it initializes itself, otherwise we
     * fall back to the old API */

    ret = kvm_create_device(s, type, true);
    if (ret == 0) {
        return 1;
    }

    /* Fallback will create VGIC v2 */
    if (type != KVM_DEV_TYPE_ARM_VGIC_V2) {
        return ret;
    }
    return 0;
}
--- cut ---
 'type' here comes from MachineState (see my last virt-v3 patch):
--- cut ---
static int kvm_irqchip_create(MachineState *machine, KVMState *s)
{
    int ret;

    if (!machine_kernel_irqchip_allowed(machine) ||
        (!kvm_check_extension(s, KVM_CAP_IRQCHIP) &&
         (kvm_vm_enable_cap(s, KVM_CAP_S390_IRQCHIP, 0) < 0))) {
        return 0;
    }

    /* First probe and see if there's a arch-specific hook to create the
     * in-kernel irqchip for us */
    ret = kvm_arch_irqchip_create(s, machine->kernel_irqchip_type);
    if (ret < 0) {
        return ret;
    } else if (ret == 0) {
        ret = kvm_vm_ioctl(s, KVM_CREATE_IRQCHIP);
        if (ret < 0) {
            fprintf(stderr, "Create kernel irqchip failed\n");
            return ret;
        }
    }
--- cut ---

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia