On Wed, Apr 07, 2021 at 09:38:21AM +0200, Vitaly Kuznetsov wrote: > Nuno Das Neves <nunodasneves@xxxxxxxxxxxxxxxxxxx> writes: > > > 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. > > This sounds reasonable, I'd suggest you reserve MSHV_CAP_CORE_API_STABLE > right away but don't expose it yet so it's clear the API is not yet > stable. Test userspace you have may always assume it's running with the > latest kernel. > > Also, please be clear about the fact that /dev/mshv doesn't > provide a stable API yet so nobody builds an application on top of > it. > Very good discussion and suggestions. Thank you Vitaly. > One more though: it is probably a good idea to introduce selftests for > /dev/mshv (similar to KVM's selftests in > /tools/testing/selftests/kvm). Selftests don't really need a stable ABI > as they live in the same linux.git and can be updated in the same patch > series which changes /dev/mshv behavior. Selftests are very useful for > checking there are no regressions, especially in the situation when > there's no publicly available userspace for /dev/mshv. I think this can wait until we merge the first implementation in tree. There are still a lot of moving parts. Our (currently limited) internal test cases need more cleaning up before they are ready. I certainly don't want to distract Nuno from getting the foundation right. Wei.
