On Fri, 20 Dec 2019 00:12:30 +0530
Kirti Wankhede <kwankhede@xxxxxxxxxx> wrote:
> On 12/19/2019 3:09 AM, Alex Williamson wrote:
> > On Tue, 17 Dec 2019 14:54:14 +0530
> > Kirti Wankhede <kwankhede@xxxxxxxxxx> wrote:
> >
> >> On 12/17/2019 10:45 AM, Yan Zhao wrote:
> >>> On Tue, Dec 17, 2019 at 04:21:39AM +0800, Kirti Wankhede wrote:
> >>>> + } else if (range.flags &
> >>>> + VFIO_IOMMU_DIRTY_PAGES_FLAG_GET_BITMAP) {
> >>>> + uint64_t iommu_pgmask;
> >>>> + unsigned long pgshift = __ffs(range.pgsize);
> >>>> + unsigned long *bitmap;
> >>>> + long bsize;
> >>>> +
> >>>> + iommu_pgmask =
> >>>> + ((uint64_t)1 << __ffs(vfio_pgsize_bitmap(iommu))) - 1;
> >>>> +
> >>>> + if (((range.pgsize - 1) & iommu_pgmask) !=
> >>>> + (range.pgsize - 1))
> >>>> + return -EINVAL;
> >>>> +
> >>>> + if (range.iova & iommu_pgmask)
> >>>> + return -EINVAL;
> >>>> + if (!range.size || range.size > SIZE_MAX)
> >>>> + return -EINVAL;
> >>>> + if (range.iova + range.size < range.iova)
> >>>> + return -EINVAL;
> >>>> +
> >>>> + bsize = verify_bitmap_size(range.size >> pgshift,
> >>>> + range.bitmap_size);
> >>>> + if (bsize)
> >>>> + return ret;
> >>>> +
> >>>> + bitmap = kmalloc(bsize, GFP_KERNEL);
> >>>> + if (!bitmap)
> >>>> + return -ENOMEM;
> >>>> +
> >>>> + ret = copy_from_user(bitmap,
> >>>> + (void __user *)range.bitmap, bsize) ? -EFAULT : 0;
> >>>> + if (ret)
> >>>> + goto bitmap_exit;
> >>>> +
> >>>> + iommu->dirty_page_tracking = false;
> >>> why iommu->dirty_page_tracking is false here?
> >>> suppose this ioctl can be called several times.
> >>>
> >>
> >> This ioctl can be called several times, but once this ioctl is called
> >> that means vCPUs are stopped and VFIO devices are stopped (i.e. in
> >> stop-and-copy phase) and dirty pages bitmap are being queried by user.
> >
> > Do not assume how userspace works or its intent. If dirty tracking is
> > on, it should remain on until the user turns it off. We cannot assume
> > userspace uses a one-shot approach. Thanks,
> >
>
> Dirty tracking should be on until user turns it off or user reads
> bitmap, right? This ioctl is used to read bitmap.
No, dirty bitmap tracking is on until the user turns it off, period.
Retrieving the bitmap is probably only looking at a portion of the
container address space at a time, anything else would place
impractical requirements on the user allocated bitmap. We also need to
support a usage model where the user is making successive calls, where
each should report pages dirtied since the previous call. If the user
is required to re-enable tracking, there's an irreconcilable gap
between the call to retrieve the dirty bitmap and their opportunity to
re-enable dirty tracking. It's fundamentally broken to automatically
disable tracking on read. Thanks,
Alex
