Re: [PATCH v10 Kernel 4/5] vfio iommu: Implementation of ioctl to for dirty pages tracking.

[Alex Williamson]

On Fri, 20 Dec 2019 00:12:30 +0530
Kirti Wankhede <address@hidden> wrote:

> On 12/19/2019 3:09 AM, Alex Williamson wrote:
> > On Tue, 17 Dec 2019 14:54:14 +0530
> > Kirti Wankhede <address@hidden> 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