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

From: Cornelia Huck <cohuck@redhat.com>
To: Yishai Hadas <yishaih@nvidia.com>,
	alex.williamson@redhat.com, bhelgaas@google.com, jgg@nvidia.com,
	saeedm@nvidia.com
Cc: 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, yishaih@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 16:21:11 +0100	[thread overview]
Message-ID: <87fso870k8.fsf@redhat.com> (raw)
In-Reply-To: <20220224142024.147653-11-yishaih@nvidia.com>

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
> --- a/include/uapi/linux/vfio.h
> +++ 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.

So, if we use my suggestion from the last round, what about making the
new addition

"VFIO_MIGRATION_P2P means that RUNNING_P2P is supported in addition to
the STOP_COPY states. It depends on VFIO_MIGRATION_STOP_COPY."

Maybe we could also use the additional clarification

"at least one of the flags below must be set, and flags may depend on or
conflict with each other."

That implies that VFIO_MIGRATION_STOP_COPY is mandatory with the current
set of defined flags. I would not really object to adding "This flag is
currently mandatory", but I do not like singling it out in the general
description of how the flags work.

Sorry if that sounds nitpicky, but I think we really need to make it
clear that we have some nice possible flexibility with how the flags
work.