Re: [PATCH V9 mlx5-next 10/15] vfio: Extend the device migration protocol with RUNNING_P2P

From: Jason Gunthorpe <jgg@nvidia.com>
To: Alex Williamson <alex.williamson@redhat.com>
Cc: Cornelia Huck <cohuck@redhat.com>,
	Yishai Hadas <yishaih@nvidia.com>,
	bhelgaas@google.com, saeedm@nvidia.com,
	linux-pci@vger.kernel.org, kvm@vger.kernel.org,
	netdev@vger.kernel.org, kuba@kernel.org, leonro@nvidia.com,
	kwankhede@nvidia.com, mgurtovoy@nvidia.com, maorg@nvidia.com,
	ashok.raj@intel.com, kevin.tian@intel.com,
	shameerali.kolothum.thodi@huawei.com
Subject: Re: [PATCH V9 mlx5-next 10/15] vfio: Extend the device migration protocol with RUNNING_P2P
Date: Thu, 24 Feb 2022 12:13:30 -0400	[thread overview]
Message-ID: <20220224161330.GA19295@nvidia.com> (raw)
In-Reply-To: <20220224083042.3f5ad059.alex.williamson@redhat.com>

On Thu, Feb 24, 2022 at 08:30:42AM -0700, Alex Williamson wrote:
> On Thu, 24 Feb 2022 16:21:11 +0100
> Cornelia Huck <cohuck@redhat.com> wrote:
> 
> > On Thu, Feb 24 2022, Yishai Hadas <yishaih@nvidia.com> wrote:
> > 
> > > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> > > index 22ed358c04c5..26a66f68371d 100644
> > > +++ b/include/uapi/linux/vfio.h
> > > @@ -1011,10 +1011,16 @@ struct vfio_device_feature {
> > >   *
> > >   * VFIO_MIGRATION_STOP_COPY means that STOP, STOP_COPY and
> > >   * RESUMING are supported.
> > > + *
> > > + * VFIO_MIGRATION_STOP_COPY | VFIO_MIGRATION_P2P means that RUNNING_P2P
> > > + * is supported in addition to the STOP_COPY states.
> > > + *
> > > + * Other combinations of flags have behavior to be defined in the future.
> > >   */
> > >  struct vfio_device_feature_migration {
> > >  	__aligned_u64 flags;
> > >  #define VFIO_MIGRATION_STOP_COPY	(1 << 0)
> > > +#define VFIO_MIGRATION_P2P		(1 << 1)
> > >  };  
> > 
> > Coming back to my argument (for the previous series) that this should
> > rather be "at least one of the flags below must be set". If we operate
> > under the general assumption that each flag indicates that a certain
> > functionality (including some states) is supported, and that flags may
> > depend on other flags, we might have a future flag that defines a
> > different behaviour, but does not depend on STOP_COPY, but rather
> > conflicts with it. We should not create the impression that STOP_COPY
> > will neccessarily be mandatory for all time.
> 
> This sounds more like an enum than a bitfield. 

It is kind of working in both ways.

The comment enumerates all the valid tests of the flags. This is not
really a mandatory/optional scheme.

If userspace wants to check support for what is described by
VFIO_MIGRATION_STOP_COPY | VFIO_MIGRATION_P2P then it must test both
bits exactly as the comment says.

In this way the universe of valid tests is limited, and it acts sort
of like an enumeration.

Using a bit test, not an equality, allows better options for future
expansion.

The key takeaway is that userspace cannot test bit combinations that
are not defined in the comment and expect anything - which is exactly
what the comment says:

> * Other combinations of flags have behavior to be defined in the future.

> > conflicts with it. We should not create the impression that STOP_COPY
> > will neccessarily be mandatory for all time.

We really *should* create that impression because a userspace that
does not test STOP_COPY in the cases required above is *broken* and
must be strongly discouraged from existing.

The purpose of this comment is to inform the userspace implementator,
not to muse about possible future expansion options for kernel
developers. We all agree this expansion path exists and is valid, we
need to keep that option open by helping userspace implement
correctly.

Jason