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

[Cornelia Huck <cohuck@redhat.com>]

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." ?

> 
> 
> >> 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." ?