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

[Eric Auger]

Hi Pavel,
On 05/21/2015 08:47 AM, Pavel Fedin wrote:
>  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.
here is my current understanding:

It is said in Linux Documentation/virtual/kvm/api.txt that the GICv3
must be instantiated through KVM_CREATE_DEVICE and not with
KVM_CREATE_IRQCHIP. If KVM_CREATE_DEVICE(GICV3,test mode=true) fails
this means either we have a recent kernel supporting KVM_CREATE_DEVICE
but we do not have GICV3 HW or we have an old kernel that does not
support KVM_CREATE_DEVICE for GICV3. In both cases we cannot instantiate
GICv3. QEMU KVM GICv3 device would also fail instantiating GICV3 through
KVM_CREATE_DEVICE (non test mode) and prevent the VM from starting.

So to me it is sensible to instantiate GICV2 through legacy
KVM_CREATE_IRQCHIP API if both KVM_CREATE_DEVICE(test mode=true) failed.

To me kvm_arch_irqchip_create code only is meant to handle the case  new
KVM_CREATE_DEVICE is NOT supported. In that case only the GICv2 is
instantiable through the legacy API.

In all other cases, the instantiation should be handled in the very QEMU
vGIC device through KVM_CREATE_DEVICE. There you can use test mode to
know whether the device supports V2 compat mode since
KVM_CREATE_DEVICE(GICV2,test mode=true) should succeed. So to me we have
ways to discriminate all combinations without kernel addition but I
would rather put that code in the QEMU GIC device instead of
kvm_arch_irqchip_create.

Let me know what you think. Ashok, any opinion?

Best Regards

Eric

 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
> 
>