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

From: Alex Williamson <alex.williamson@redhat.com>
To: Jason Gunthorpe <jgg@nvidia.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 09:35:42 -0700	[thread overview]
Message-ID: <20220224093542.3730bb24.alex.williamson@redhat.com> (raw)
In-Reply-To: <20220224161330.GA19295@nvidia.com>

On Thu, 24 Feb 2022 12:13:30 -0400
Jason Gunthorpe <jgg@nvidia.com> wrote:

> 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.

Yes.

> 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.

Chatting with Connie offline, I think the clarification that might help
is something alone the lines that the combination of bits must support
migration, which currently requires the STOP_COPY and RESUMING states.
The VFIO_MIGRATION_P2P flag alone does not provide these states.  The
only flag in the current specification to provide these states is
VFIO_MIGRATION_STOP_COPY.  I don't think we want to preclude that some
future flag might provide variants of STOP_COPY and RESUMING, so it's
not so much that VFIO_MIGRATION_STOP_COPY is mandatory, but it is
currently the only flag which provides the base degree of migration
support.

How or if that translates to an actual documentation update, I'm not
sure.  As it stands, we're not speculating about future support, we're
only stating these two combinations are valid.  Future combinations may
or may not include VFIO_MIGRATION_STOP_COPY.  As the existing proposed
comment indicates, other combinations are TBD.  Connie?  Thanks,

Alex