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

[Kirti Wankhede <kwankhede@nvidia.com>]

On 5/20/2020 3:57 PM, Cornelia Huck wrote:
> 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,

Done.

> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> 

Thanks.

Kirti