Re: [PATCH Kernel v20 4/8] vfio iommu: Add ioctl definition for dirty pages tracking

[Cornelia Huck <cohuck@redhat.com>]

On Tue, 19 May 2020 09:53:56 -0600
Alex Williamson <alex.williamson@redhat.com> wrote:

> On Tue, 19 May 2020 17:35:07 +0200
> Cornelia Huck <cohuck@redhat.com> wrote:
> 
> > On Fri, 15 May 2020 23:05:24 +0530
> > Kirti Wankhede <kwankhede@nvidia.com> wrote:
> >   
> > > On 5/15/2020 4:29 PM, Cornelia Huck wrote:    
> > > > On Fri, 15 May 2020 02:07:43 +0530
> > > > Kirti Wankhede <kwankhede@nvidia.com> wrote:
> > > >       
> > > >> IOMMU container maintains a list of all pages pinned by vfio_pin_pages API.
> > > >> All pages pinned by vendor driver through this API should be considered as
> > > >> dirty during migration. When container consists of IOMMU capable device and
> > > >> all pages are pinned and mapped, then all pages are marked dirty.
> > > >> Added support to start/stop dirtied pages tracking and to get bitmap of all
> > > >> dirtied pages for requested IO virtual address range.
> > > >>
> > > >> Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com>
> > > >> Reviewed-by: Neo Jia <cjia@nvidia.com>
> > > >> ---
> > > >>   include/uapi/linux/vfio.h | 55 +++++++++++++++++++++++++++++++++++++++++++++++
> > > >>   1 file changed, 55 insertions(+)    
> > 
> > (...)
> >   
> > > >> + * When IOCTL is called with VFIO_IOMMU_DIRTY_PAGES_FLAG_GET_BITMAP flag set,
> > > >> + * IOCTL returns dirty pages bitmap for IOMMU container during migration for
> > > >> + * given IOVA range.      
> > > > 
> > > > "Calling the IOCTL with VFIO_IOMMU_DIRTY_PAGES_GET_BITMAP returns the
> > > > dirty pages bitmap for the IOMMU container for a given IOVA range." ?
> > > > 
> > > > Q: How does this interact with the two other operations? I imagine
> > > > getting an empty bitmap before _START       
> > > 
> > > No, if dirty page tracking is not started, get_bitmap IOCTL will fail 
> > > with -EINVAL.
> > >     
> > > > and a bitmap-in-progress between
> > > > _START and _STOP. > After _STOP, will subsequent calls always give the
> > > > same bitmap?
> > > >      
> > > 
> > > No, return -EINVAL.    
> > 
> > Maybe add
> > 
> > "If the IOCTL has not yet been called with
> > VFIO_IOMMU_DIRTY_PAGES_FLAG_START, or if it has been called with
> > VFIO_IOMMU_DIRTY_PAGES_FLAG_STOP, calling it with
> > VFIO_IOMMU_DIRTY_PAGES_FLAG_GET_BITMAP will return -EINVAL." ?  
> 
> Let's not specify ourselves into a corner, I think we can simply say
> that the dirty bitmap is only available while dirty logging is enabled.
> We certainly don't need to specify specific errno values that'll trip
> us up later.

"If dirty logging is not enabled, an error will be returned." ?

(...)

> > > >> Caller must set argsz including size of structure
> > > >> + * vfio_iommu_type1_dirty_bitmap_get.      
> > > > 
> > > > s/Caller/The caller/
> > > > 
> > > > Does argz also include the size of the bitmap?      
> > > 
> > > No.    
> > 
> > "The caller must set argsz to a value including the size of stuct
> > vfio_io_type1_dirty_bitmap_get, but excluding the size of the actual
> > bitmap." ?  
> 
> Yes, it wouldn't make sense for argsz to include the size of the bitmap
> itself, that's accessed independently via a user provided pointer and
> we have a separate size field for that.  Thanks,
> 
> Alex

Yes, I just wanted to make it as obvious as possible to make it easier
for folks trying to interact with this interface.