[PATCH v6 18/20] KVM: introduce kvm_check_device_type()

[Andre Przywara]

Hi Christoffer,

so far I am done with all the changes requested except this
kvm_check_device_type() issue. It would be great if we could come to a
conclusion for this one soon so I can push the new version out.
(see below for more rationale)
...

On 12/01/15 12:33, Andre Przywara wrote:
> Hej Christoffer,
> 
> On 11/01/15 15:22, Christoffer Dall wrote:
>> On Fri, Jan 09, 2015 at 01:42:42PM +0000, Andre Przywara wrote:
>>> Hi Christoffer,
>>>
>>> On 09/01/15 12:33, Christoffer Dall wrote:
>>>> On Fri, Jan 09, 2015 at 11:54:36AM +0000, Andre Przywara wrote:
>>>>> While we can easily register and unregister KVM devices, there is
>>>>> currently no easy way of checking whether a device has been
>>>>> registered.
>>>>> Introduce kvm_check_device_type() for that purpose and use it in two
>>>>> existing functions. Also change the return code for an invalid
>>>>> type number from ENOSPC to EINVAL.
>>>>> This function will be later used by another patch set to check
>>>>> whether a KVM_CREATE_IRQCHIP ioctl is valid.
>>>>
>>>> I feel like this is misguided and the vgic should be able to figure this
>>>> stuff out internally.  Did you have code for this approach somewhere
>>>> that I can take a look at?
>>>
>>> I pushed my WIP patch on top of the kvm-gicv3/v6 tree.
>>> Given how that looks I reckoned the generic solution would be more
>>> preferable.
>>> Basically we internally decide in the _probe function whether we support
>>> GICv2 emulation or not, which is mostly driven by device tree
>>> properties. So at the moment I just register the GIC_V2 KVM device or
>>> not. Now with the "vgic internal" solution I misuse the GICV address
>>> base as a hint of the GICv2 emulation availability. Alternatively I have
>>> to introduce a new variable to mirror what the KVM device array already
>>> holds, which seems kind of exerted to me.
>>> Besides that I am not sure if the GICV address hint will always be a
>>> reliable indicator and what we will do if there will be another GIC
>>> model to be emulated in the future (maybe we need that for the ITS
>>> emulation already?)
>>
>> I don't think it looks that bad.
>>
>> Only your gicv3 and gicv2 code files know what they are capable of
>> emulating, how you choose to store this state internally in those files
>> is a somewhat orthogonal discussion from using the kvm device API.
> 
> Well, the point is that the emulation capability is a hardware property
> and thus the knowledge is actually in the host part of the VGIC (so in
> vgic-v3.c and vgic-v2.c). From here we "communicate" the capability to
> userland by registering the respective VGIC KVM devices only. Since the
> emulation part of the VGIC lives in different files (vgic.c and
> vgic-vx-emul.c) we would need some kind of export to them, too. I found
> that it would be cleaner to just re-use what we already have with the
> KVM devices.
> 
>> Using the KVM device api is just another way of storing and exposing the
>> information globally (you take registering the device types as an
>> indication of the state).
>>
>> Finally, I don't even think you ned the can_emulate function, I think
>> you should just return an error from init_vgic_model (which happens to
>> collide with my suggestion on making those functions a void function in
>> one of the previous patches) and you're done.
> 
> I think I checked this before and since the init_vgic_model()
> implementations are in vgic-vx-emul.c we don't know the hardware
> capability anymore and would need some kind of variable holding that
> information (which lead me to re-using the KVM device knowledge). But I
> will re-check if there is an easy fix in here.

So since we only see this problem with the legacy KVM_CREATE_IRQCHIP
ioctl, we could just introduce a can_emulate_gicv2 flag into vgic_params
which holds the capability of emulating a GICv2. Any potential future
VGIC models will be handled by KVM_CREATE_DEVICE only anyway, so we are
covered with this.
I now check that flag in kvm_vgic_create() when a GICv2 emulation is
requested. This function is directly called by the KVM_CREATE_IRQCHIP
handler, so the connection should be clear (and is commented).
That works and is not too ugly, only to me it looks a bit like a kludge
to avoid the KVM device check.

If you are roughly OK with this approach, I will send out a v7 with that
one.

Cheers,
Andre.

>>>
>>> So I prefer the more generic solution.
>>> Let me know what you think, I can as well drop 18/20 and merge the above
>>> mentioned patch.
>>>
>>>> I forget: Are we still requiring KVM_CREATE_IRQCHIP for VGICv3 or are we
>>>> just relying on users to use KVM_CREATE_DEVICE for anything in the
>>>> future?
>>>
>>> Since KVM_CREATE_IRQCHIP does not take an argument, we cannot use it for
>>> GICv3. So GICv3 mandates KVM_CREATE_DEVICE. We need userspace
>>> adjustments for GICv3 anyway, so that's not a problem.
>>
>> ok, so KVM_CREATE_IRQCHIP is a direct alias for KVM_CREATE_DEVICE(GIC_V2)
>> and is deprecated for GICv3?  If so, we should probably update the
>> documentation to indicate the KVM_CREATE_IRQCHIP creates a GICv2 and
>> should not be used for any other in-kernel GIC versions.
> 
> What about the following wording in api.txt:
> -----
> On ARM/arm64, a GICv2 is created. Any other VGIC versions require the
> usage of KVM_CREATE_DEVICE (which can and should also be used to create
> a virtual GICv2).
> -----
> 
> In fact both QEMU and kvmtool currently try KVM_CREATE_DEVICE first even
> for a VGICv2 on a GICv2 and only fall back to KVM_CREATE_IRQCHIP if that
> fails (to support older kernels).
> 
> Cheers,
> Andre.
>