| Message ID | 1589871253-10650-1-git-send-email-kwankhede@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
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>
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
diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c index 0a420594483a..963ae4348b3c 100644 --- a/drivers/vfio/vfio_iommu_type1.c +++ b/drivers/vfio/vfio_iommu_type1.c @@ -1018,23 +1018,25 @@ static int verify_bitmap_size(uint64_t npages, uint64_t bitmap_size) } static int vfio_dma_do_unmap(struct vfio_iommu *iommu, - struct vfio_iommu_type1_dma_unmap *unmap) + struct vfio_iommu_type1_dma_unmap *unmap, + struct vfio_bitmap *bitmap) { - uint64_t mask; struct vfio_dma *dma, *dma_last = NULL; - size_t unmapped = 0; + size_t unmapped = 0, pgsize; int ret = 0, retries = 0; + unsigned long pgshift; mutex_lock(&iommu->lock); - mask = ((uint64_t)1 << __ffs(iommu->pgsize_bitmap)) - 1; + pgshift = __ffs(iommu->pgsize_bitmap); + pgsize = (size_t)1 << pgshift; - if (unmap->iova & mask) { + if (unmap->iova & (pgsize - 1)) { ret = -EINVAL; goto unlock; } - if (!unmap->size || unmap->size & mask) { + if (!unmap->size || unmap->size & (pgsize - 1)) { ret = -EINVAL; goto unlock; } @@ -1045,9 +1047,15 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu, goto unlock; } - WARN_ON(mask & PAGE_MASK); -again: + /* When dirty tracking is enabled, allow only min supported pgsize */ + if ((unmap->flags & VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP) && + (!iommu->dirty_page_tracking || (bitmap->pgsize != pgsize))) { + ret = -EINVAL; + goto unlock; + } + WARN_ON((pgsize - 1) & PAGE_MASK); +again: /* * vfio-iommu-type1 (v1) - User mappings were coalesced together to * avoid tracking individual mappings. This means that the granularity @@ -1085,6 +1093,7 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu, ret = -EINVAL; goto unlock; } + dma = vfio_find_dma(iommu, unmap->iova + unmap->size - 1, 0); if (dma && dma->iova + dma->size != unmap->iova + unmap->size) { ret = -EINVAL; @@ -1128,6 +1137,14 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu, mutex_lock(&iommu->lock); goto again; } + + if (unmap->flags & VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP) { + ret = update_user_bitmap(bitmap->data, dma, + unmap->iova, pgsize); + if (ret) + break; + } + unmapped += dma->size; vfio_remove_dma(iommu, dma); } @@ -2466,17 +2483,40 @@ static long vfio_iommu_type1_ioctl(void *iommu_data, } else if (cmd == VFIO_IOMMU_UNMAP_DMA) { struct vfio_iommu_type1_dma_unmap unmap; - long ret; + struct vfio_bitmap bitmap = { 0 }; + int ret; minsz = offsetofend(struct vfio_iommu_type1_dma_unmap, size); if (copy_from_user(&unmap, (void __user *)arg, minsz)) return -EFAULT; - if (unmap.argsz < minsz || unmap.flags) + if (unmap.argsz < minsz || + unmap.flags & ~VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP) return -EINVAL; - ret = vfio_dma_do_unmap(iommu, &unmap); + if (unmap.flags & VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP) { + unsigned long pgshift; + + if (unmap.argsz < (minsz + sizeof(bitmap))) + return -EINVAL; + + if (copy_from_user(&bitmap, + (void __user *)(arg + minsz), + sizeof(bitmap))) + return -EFAULT; + + if (!access_ok((void __user *)bitmap.data, bitmap.size)) + return -EINVAL; + + pgshift = __ffs(bitmap.pgsize); + ret = verify_bitmap_size(unmap.size >> pgshift, + bitmap.size); + if (ret) + return ret; + } + + ret = vfio_dma_do_unmap(iommu, &unmap, &bitmap); if (ret) return ret; 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 + * before unmapping IO virtual addresses. When this flag is set, user must + * provide data[] as structure vfio_bitmap. User must allocate memory to get + * bitmap, zero the bitmap memory and must set size of allocated memory in + * vfio_bitmap.size field. A bit in bitmap represents one page of user provided + * page size in 'pgsize', consecutively starting from iova offset. Bit set + * indicates page at that offset from iova is dirty. Bitmap of pages in the + * range of unmapped size is returned in 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)