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

From: Wei Liu <wei.liu@kernel.org>
To: Vitaly Kuznetsov <vkuznets@redhat.com>
Cc: Nuno Das Neves <nunodasneves@linux.microsoft.com>,
	linux-hyperv@vger.kernel.org,
	virtualization@lists.linux-foundation.org,
	linux-kernel@vger.kernel.org, mikelley@microsoft.com,
	viremana@linux.microsoft.com, sunilmut@microsoft.com,
	wei.liu@kernel.org, ligrassi@microsoft.com, kys@microsoft.com
Subject: Re: [RFC PATCH 04/18] virt/mshv: request version ioctl
Date: Wed, 7 Apr 2021 13:43:02 +0000	[thread overview]
Message-ID: <20210407134302.ng6n4el2km7sujfp@liuwe-devbox-debian-v2> (raw)
In-Reply-To: <87eefmczo2.fsf@vitty.brq.redhat.com>

On Wed, Apr 07, 2021 at 09:38:21AM +0200, Vitaly Kuznetsov wrote:
> Nuno Das Neves <nunodasneves@linux.microsoft.com> writes:
> 
> > On 3/5/2021 1:18 AM, Vitaly Kuznetsov wrote:
> >> Nuno Das Neves <nunodasneves@linux.microsoft.com> writes:
> >> 
> >>> On 2/9/2021 5:11 AM, Vitaly Kuznetsov wrote:
> >>>> Nuno Das Neves <nunodasneves@linux.microsoft.com> 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.