Re: [PATCH Kernel v22 6/8] vfio iommu: Update UNMAP_DMA ioctl to get dirty bitmap before unmap

[Cornelia Huck <cohuck@redhat.com>]

On Tue, 19 May 2020 12:24:13 +0530
Kirti Wankhede <kwankhede@nvidia.com> wrote:

> DMA mapped pages, including those pinned by mdev vendor drivers, might
> get unpinned and unmapped while migration is active and device is still
> running. For example, in pre-copy phase while guest driver could access
> those pages, host device or vendor driver can dirty these mapped pages.
> Such pages should be marked dirty so as to maintain memory consistency
> for a user making use of dirty page tracking.
> 
> To get bitmap during unmap, user should allocate memory for bitmap, set
> it all zeros, set size of allocated memory, set page size to be
> considered for bitmap and set flag VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP.
> 
> Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com>
> Reviewed-by: Neo Jia <cjia@nvidia.com>
> ---
>  drivers/vfio/vfio_iommu_type1.c | 62 +++++++++++++++++++++++++++++++++--------
>  include/uapi/linux/vfio.h       | 10 +++++++
>  2 files changed, 61 insertions(+), 11 deletions(-)

(...)

> @@ -1085,6 +1093,7 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
>  			ret = -EINVAL;
>  			goto unlock;
>  		}
> +

Nit: unrelated whitespace change.

>  		dma = vfio_find_dma(iommu, unmap->iova + unmap->size - 1, 0);
>  		if (dma && dma->iova + dma->size != unmap->iova + unmap->size) {
>  			ret = -EINVAL;

(...)

> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> index 4850c1fef1f8..a1dd2150971e 100644
> --- a/include/uapi/linux/vfio.h
> +++ b/include/uapi/linux/vfio.h
> @@ -1048,12 +1048,22 @@ struct vfio_bitmap {
>   * field.  No guarantee is made to the user that arbitrary unmaps of iova
>   * or size different from those used in the original mapping call will
>   * succeed.
> + * VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP should be set to get dirty bitmap

s/dirty bitmap/the dirty bitmap/

> + * before unmapping IO virtual addresses. When this flag is set, user must

s/user/the user/

> + * provide data[] as structure vfio_bitmap. User must allocate memory to get

"provide a struct vfio_bitmap in data[]" ?


> + * bitmap, zero the bitmap memory and must set size of allocated memory in
> + * vfio_bitmap.size field.

"The user must provide zero-allocated memory via vfio_bitmap.data and
its size in the vfio_bitmap.size field." ?


> A bit in bitmap represents one page of user provided

s/bitmap/the bitmap/

> + * page size in 'pgsize', consecutively starting from iova offset. Bit set

s/Bit set/A set bit/

> + * indicates page at that offset from iova is dirty. Bitmap of pages in the

s/indicates page/indicates that the page/

> + * range of unmapped size is returned in vfio_bitmap.data

"A bitmap of the pages in the range of the unmapped size is returned in
the user-provided vfio_bitmap.data." ?

>   */
>  struct vfio_iommu_type1_dma_unmap {
>  	__u32	argsz;
>  	__u32	flags;
> +#define VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP (1 << 0)
>  	__u64	iova;				/* IO virtual address */
>  	__u64	size;				/* Size of mapping (bytes) */
> +	__u8    data[];
>  };
>  
>  #define VFIO_IOMMU_UNMAP_DMA _IO(VFIO_TYPE, VFIO_BASE + 14)

With the nits addressed,
Reviewed-by: Cornelia Huck <cohuck@redhat.com>