Re: [PATCH Kernel v18 6/7] vfio iommu: Add migration capability to report supported features

[Alex Williamson <alex.williamson@redhat.com>]

On Thu, 7 May 2020 11:07:26 +0530
Kirti Wankhede <kwankhede@nvidia.com> wrote:

> On 5/7/2020 3:57 AM, Alex Williamson wrote:
> > On Mon, 4 May 2020 21:28:58 +0530
> > Kirti Wankhede <kwankhede@nvidia.com> wrote:
> >   
> >> Added migration capability in IOMMU info chain.
> >> User application should check IOMMU info chain for migration capability
> >> to use dirty page tracking feature provided by kernel module.
> >>
> >> Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com>
> >> ---
> >>   drivers/vfio/vfio_iommu_type1.c | 15 +++++++++++++++
> >>   include/uapi/linux/vfio.h       | 14 ++++++++++++++
> >>   2 files changed, 29 insertions(+)
> >>
> >> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> >> index 8b27faf1ec38..b38d278d7bff 100644
> >> --- a/drivers/vfio/vfio_iommu_type1.c
> >> +++ b/drivers/vfio/vfio_iommu_type1.c
> >> @@ -2378,6 +2378,17 @@ static int vfio_iommu_iova_build_caps(struct vfio_iommu *iommu,
> >>   	return ret;
> >>   }
> >>   
> >> +static int vfio_iommu_migration_build_caps(struct vfio_info_cap *caps)
> >> +{
> >> +	struct vfio_iommu_type1_info_cap_migration cap_mig;
> >> +
> >> +	cap_mig.header.id = VFIO_IOMMU_TYPE1_INFO_CAP_MIGRATION;
> >> +	cap_mig.header.version = 1;
> >> +	cap_mig.flags = VFIO_IOMMU_INFO_CAPS_MIGRATION_DIRTY_PAGE_TRACK;
> >> +
> >> +	return vfio_info_add_capability(caps, &cap_mig.header, sizeof(cap_mig));
> >> +}
> >> +
> >>   static long vfio_iommu_type1_ioctl(void *iommu_data,
> >>   				   unsigned int cmd, unsigned long arg)
> >>   {
> >> @@ -2427,6 +2438,10 @@ static long vfio_iommu_type1_ioctl(void *iommu_data,
> >>   		if (ret)
> >>   			return ret;
> >>   
> >> +		ret = vfio_iommu_migration_build_caps(&caps);
> >> +		if (ret)
> >> +			return ret;
> >> +
> >>   		if (caps.size) {
> >>   			info.flags |= VFIO_IOMMU_INFO_CAPS;
> >>   
> >> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> >> index e3cbf8b78623..df9ce8aaafab 100644
> >> --- a/include/uapi/linux/vfio.h
> >> +++ b/include/uapi/linux/vfio.h
> >> @@ -1013,6 +1013,20 @@ struct vfio_iommu_type1_info_cap_iova_range {
> >>   	struct	vfio_iova_range iova_ranges[];
> >>   };
> >>   
> >> +/*
> >> + * The migration capability allows to report supported features for migration.
> >> + *
> >> + * The structures below define version 1 of this capability.
> >> + */
> >> +#define VFIO_IOMMU_TYPE1_INFO_CAP_MIGRATION  1
> >> +
> >> +struct vfio_iommu_type1_info_cap_migration {
> >> +	struct	vfio_info_cap_header header;
> >> +	__u32	flags;
> >> +	/* supports dirty page tracking */
> >> +#define VFIO_IOMMU_INFO_CAPS_MIGRATION_DIRTY_PAGE_TRACK	(1 << 0)
> >> +};
> >> +  
> > 
> > What about exposing the maximum supported dirty bitmap size and the
> > supported page sizes?  Thanks,
> >   
> 
> How should user application use that?

How does a user application currently discover that only a PAGE_SIZE
dirty bitmap granularity is supported or that the when performing an
unmap while requesting the dirty bitmap, those unmaps need to be
chunked to no more than 2^31 * PAGE_SIZE?  I don't see anything in the
uapi that expresses these restrictions.

It seems we're currently relying on the QEMU implementation to
coincide with bitmap granularity support, with no mechanism other than
trial and error to validate or expand that support.  Likewise, I think
it's largely a coincidence that KVM also has the same slot size
restrictions, so we're unlikely to see an unmap exceeding this limit,
but our api does not restrict an unmap to only cover a single mapping.
We probably also need to think about whether we need to expose a
mapping chunk limitation separately or if it should be extrapolated
from the migration support (we might have non-migration reasons to
limit it at some point).

If we leave these as assumptions then we risk breaking userspace or
limiting the usefulness of expending this support.  If we include
within the uapi a mechanism for learning about these restrictions, then
it becomes a userspace problem to follow the uapi and take advantage of
the uapi provided.  Thanks,

Alex