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

[Alex Williamson]

On Tue, 19 May 2020 17:35:07 +0200
Cornelia Huck <address@hidden> wrote:

> On Fri, 15 May 2020 23:05:24 +0530
> Kirti Wankhede <address@hidden> wrote:
> 
> > On 5/15/2020 4:29 PM, Cornelia Huck wrote:  
> > > On Fri, 15 May 2020 02:07:43 +0530
> > > Kirti Wankhede <address@hidden> 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 <address@hidden>
> > >> Reviewed-by: Neo Jia <address@hidden>
> > >> ---
> > >>   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.

> > >> User must provide data[] as the structure
> > >> + * vfio_iommu_type1_dirty_bitmap_get through which user provides IOVA 
> > >> range and
> > >> + * pgsize.    
> > > 
> > > "The user must specify the IOVA range and the pgsize through the
> > > vfio_iommu_type1_dirty_bitmap_get structure in the data[] portion."
> > > 
> > > ?
> > >     
> > >> This interface supports to get bitmap of smallest supported pgsize
> > >> + * only and can be modified in future to get bitmap of specified 
> > >> pgsize.    
> > > 
> > > That's a current restriction? How can the user find out whether it has
> > > been lifted (or, more generally, find out which pgsize values are
> > > supported)?    
> > 
> > Migration capability is added to IOMMU info chain. That gives supported 
> > pgsize bitmap by IOMMU driver.  
> 
> Add that info?
> 
> "The supported pgsize values for this interface are reported via the
> migration capability in the IOMMU info chain."
> 
> >   
> > >     
> > >> + * User must allocate memory for bitmap, zero the bitmap memory  and 
> > >> set size
> > >> + * of allocated memory in bitmap.size field.    
> > > 
> > > "The user must provide a zeroed memory area for the bitmap memory and
> > > specify its size in bitmap.size."
> > > 
> > > ?
> > >     
> > >> One bit is used to represent one
> > >> + * page consecutively starting from iova offset. User should provide 
> > >> page size
> > >> + * in bitmap.pgsize field.    
> > > 
> > > s/User/The user/
> > > 
> > > Is that the 'pgsize' the comment above talks about?
> > >     
> > 
> > By specifying pgsize here user can ask for bitmap of specific pgsize.  
> 
> "The user should provide the page size for the bitmap in the
> bitmap.pgsize field." ?
> 
> >   
> > >> Bit set in bitmap indicates page at that offset from
> > >> + * iova is dirty.    
> > > 
> > > "A bit set in the bitmap indicates that the page at that offset from
> > > iova is dirty." ?
> > >     
> > >> 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