Re: [RFC PATCH 04/18] virt/mshv: request version ioctl

[Nuno Das Neves]

On 3/5/2021 1:18 AM, Vitaly Kuznetsov wrote:
> Nuno Das Neves <nunodasneves@xxxxxxxxxxxxxxxxxxx> writes:
> 
>> On 2/9/2021 5:11 AM, Vitaly Kuznetsov wrote:
>>> Nuno Das Neves <nunodasneves@xxxxxxxxxxxxxxxxxxx> writes:
>>>
> ...
>>>> +
>>>> +3.1 MSHV_REQUEST_VERSION
>>>> +------------------------
>>>> +:Type: /dev/mshv ioctl
>>>> +:Parameters: pointer to a u32
>>>> +:Returns: 0 on success
>>>> +
>>>> +Before issuing any other ioctls, a MSHV_REQUEST_VERSION ioctl must be called to
>>>> +establish the interface version with the kernel module.
>>>> +
>>>> +The caller should pass the MSHV_VERSION as an argument.
>>>> +
>>>> +The kernel module will check which interface versions it supports and return 0
>>>> +if one of them matches.
>>>> +
>>>> +This /dev/mshv file descriptor will remain 'locked' to that version as long as
>>>> +it is open - this ioctl can only be called once per open.
>>>> +
>>>
>>> KVM used to have KVM_GET_API_VERSION too but this turned out to be not
>>> very convenient so we use capabilities (KVM_CHECK_EXTENSION/KVM_ENABLE_CAP)
>>> instead.
>>>
>>
>> The goal of MSHV_REQUEST_VERSION is to support changes to APIs in the core set.
>> When we add new features/ioctls beyond the core we can use an extension/capability
>> approach like KVM.
>>
> 
> Driver versions is a very bad idea from distribution/stable kernel point
> of view as it presumes that the history is linear. It is not.
> 
> Imagine you have the following history upstream:
> 
> MSHV_REQUEST_VERSION = 1
> <100 commits with features/fixes>
> MSHV_REQUEST_VERSION = 2
> <another 100 commits with features/fixes>
> MSHV_REQUEST_VERSION = 2
> 
> Now I'm a linux distribution / stable kernel maintainer. My kernel is at
> MSHV_REQUEST_VERSION = 1. Now I want to backport 1 feature from between
> VER=1 and VER=2 and another feature from between VER=2 and VER=3. My
> history now looks like
> 
> MSHV_REQUEST_VERSION = 1
> <5 commits from between VER=1 and VER=2>
>    Which version should I declare here???? 
> <5 commits from between VER=2 and VER=3>
>    Which version should I declare here???? 
> 
> If I keep VER=1 then userspace will think that I don't have any extra
> features added and just won't use them. If I change VER to 2/3, it'll
> think I have *all* features from between these versions.
> 
> The only reasonable way to manage this is to attach a "capability" to
> every ABI change and expose this capability *in the same commit which
> introduces the change to the ABI*. This way userspace will now exactly
> which ioctls are available and what are their interfaces.
> 
> Also, trying to define "core set" is hard but you don't really need
> to.
> 

We've had some internal discussion on this.

There is bound to be some iteration before this ABI is stable, since even the
underlying Microsoft hypervisor interfaces aren't stable just yet.

It might make more sense to just have an IOCTL to check if the API is stable yet.
This would be analogous to checking if kVM_GET_API_VERSION returns 12.

How does this sound as a proposal?
An MSHV_CHECK_EXTENSION ioctl to query extensions to the core /dev/mshv API.

It takes a single argument, an integer named MSHV_CAP_* corresponding to
the extension to check the existence of.

The ioctl will return 0 if the extension is unsupported, or a positive integer
if supported.

We can initially include a capability called MSHV_CAP_CORE_API_STABLE.
If supported, the core APIs are stable.