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

Cornelia Huck
2020-05-19T15:35:27Z

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