Re: [PATCH V6 mlx5-next 08/15] vfio: Define device migration protocol v2

[Cornelia Huck <cohuck@redhat.com>]

On Wed, Feb 02 2022, Alex Williamson <alex.williamson@redhat.com> wrote:

> On Tue, 1 Feb 2022 20:24:59 -0400
> Jason Gunthorpe <jgg@nvidia.com> wrote:
>
>> On Tue, Feb 01, 2022 at 02:49:16PM -0700, Alex Williamson wrote:
>> > On Tue, 1 Feb 2022 14:36:20 -0400
>> > Jason Gunthorpe <jgg@nvidia.com> wrote:

>> > > I don't want to touch capabilities, but we can try to use feature for
>> > > set state. Please confirm this is what you want.  
>> > 
>> > It's a team sport, but to me it seems like it fits well both in my
>> > mental model of interacting with a device feature, without
>> > significantly altering the uAPI you're defining anyway.  
>> 
>> Well, my advice is that ioctls are fine, and a bit easier all around.
>> eg strace and syzkaller are a bit easier if everything neatly maps
>> into one struct per ioctl - their generator tools are optimized for
>> this common case.
>> 
>> Simple multiplexors are next-best-fine, but there should be a clear
>> idea when to use the multiplexer, or not.
>> 
>> Things like the cap chains enter a whole world of adventure for
>> strace/syzkaller :)
>
> vfio's argsz/flags is not only a standard framework, but it's one that
> promotes extensions.  We were able to add capability chains with
> backwards compatibility because of this design.  IMO, that's avoided
> ioctl sprawl; we've been able to maintain a fairly small set of core
> ioctls rather than add add a new ioctl every time we want to describe
> some new property of a device or region or IOMMU.  I think that
> improves the usability of the uAPI.  I certainly wouldn't want to
> program to a uAPI with a million ioctls.  A counter argument is that
> we're making the interface more complex, but at the same time we're
> adding shared infrastructure for dealing with that complexity.
>
> Of course we do continue to add new ioctls as necessary, including this
> FEATURE ioctl, and I recognize that with such a generic multiplexer we
> run the risk of over using it, ie. everything looks like a nail.  You
> initially did not see the fit for setting device state as interacting
> with a device feature, but it doesn't seem like you had a strong
> objection to my explanation of it in that context.
>
> So I think if the FEATURE ioctl has an ongoing place in our uAPI (using
> it to expose migration flags would seem to be a point in that
> direction) and it doesn't require too many contortions to think of the
> operation we're trying to perform on the device as interacting with a
> device FEATURE, and there are no functional or performance implications
> of it, I would think we should use it.  To do otherwise would suggest
> that we should consider the FEATURE ioctl a failed experiment and not
> continue to expand its use.
>
> I'd be interested to hear more input on this from the community.

My personal take would be: a new ioctl is more suitable for things that
may be implemented by different backends, but in a non-generic way, and
for mandatory functionality; the FEATURE ioctl is more suitable for
things that either are very specific to a certain backend (i.e. don't
reserve an ioctl for something that will only ever be used on one
platform), or for things that have a lot of commonality for the backends
that implement them (i.e. you are using a familiar scheme to interact
with them.)

From staring at the code and the discussion here for a bit (I have not
yet made my way through all of this except in a superficial way), I'd
lean more towards using FEATURE here.