Re: [PATCH v2 13/15] uapi: hyperv: Add mshv driver headers hvhdk.h, hvhdk_mini.h, hvgdk.h, hvgdk_mini.h

[Greg KH]

On Tue, Oct 10, 2023 at 03:49:48PM -0700, Nuno Das Neves wrote:
> On 8/25/2023 11:24 AM, Nuno Das Neves wrote:
> > On 8/19/2023 3:26 AM, Greg KH wrote:
> > > 
> > > My "strong" opinion is the one kernel development rule that we have,
> > > "you can not break userspace".  So, if you change these
> > > values/structures/whatever in the future, and userspace tools break,
> > > that's not ok and the changes have to be reverted.
> > > 
> > > If you can control both sides of the API here (with open tools that you
> > > can guarantee everyone will always update to), then yes, you can change
> > > the api in the future.
> > > 
> > 
> > This is true for us - we contribute and maintain support for this driver
> > in Cloud Hypervisor[1], an open source VMM.
> > 
> 
> Hi Greg,
> 
> Bringing up this discussion again so we can clear up any confusion on the
> uapi headers in this patch set.
> 
> The headers consist of the ioctls in mshv.h, and the hypervisor ABIs in
> the *hdk.h files. The ioctls depend on the hypervisor ABIs.
> 
> We will add (to the next version), an ioctl like KVM_GET_API_VERSION [1].
> This will return a version number for the ioctl interface that increments
> any time there is a breaking change. Userspace would be required to check
> this before calling any other ioctl, and it can exit gracefully if there
> is a mismatch.
> 
> That's how KVM evolved its userspace ABI. We want to use the same approach.

That's one way to do interfaces, but there are better ways as you know.
What do you expect to change that is going to require such heavy-handed
treatment of structures and ioctls?

And how are you going to handle backwards compatiblity?

Versioning things is almost always a pain, and should only be done as a
last resort as a sign of an api that is not well understood at creation
time.  Surely you have studied how other hypervisor interfaces work and
know what you need to do for yours by now, right?  This isn't the first
hypervisor api in Linux like KVM was when it was introduced a few
decades ago.  Why not learn from past mistakes and design patterns
instead of just blindly imitating old apis that perhaps should not be
imitated at all?

> I also wanted to reiterate that we are the only maintainers and users of
> the userspace code for this driver via Cloud Hypervisor [2].

For today, maybe, but you can never guarantee that in the future as you
well know.

> We generate rust bindings from these headers using bindgen [3].

What does rust have to do with anything here?  You can use m4 to parse
the headers for all we care :)

> Taking this into account, is the above a viable path for this patch set?

I have no idea, sorry, I don't see any patches here to review as the
original set is long-gone from my queue.

Just submit your fixed up patch series based on the previous review
comments and it will be reviewed again, just like all kernel patches
are.  Why is this set somehow more special than anything else?

Perhaps you all should take the time to do some kernel patch reviews of
other stuff sent to the mailing lists to get an idea of how this whole
process works, and to get better integrated into the kernel development
community, before dumping a huge patchset on us with lots of process
questions like this?  Why are you asking the community to do a lot of
work and hand-holding when you aren't helping others out as well?

good luck!

greg k-h