Re: [PATCH v9 Kernel 2/5] vfio iommu: Add ioctl defination to get dirty pages bitmap.

[Kirti Wankhede]

On 11/13/2019 4:00 AM, Alex Williamson wrote:
> On Tue, 12 Nov 2019 22:33:37 +0530
> Kirti Wankhede <address@hidden> wrote:
>
> > All pages pinned by vendor driver through vfio_pin_pages API should be
> > considered as dirty during migration. IOMMU container maintains a list of
> > all such pinned pages. Added an ioctl defination to get bitmap of such
>
> definition
>
> > pinned pages for requested IO virtual address range.
>
> Additionally, all mapped pages are considered dirty when physically
> mapped through to an IOMMU, modulo we discussed devices opting in to
> per page pinning to indicate finer granularity with a TBD mechanism to
> figure out if any non-opt-in devices remain.

You mean, in case of device direct assignment (device pass through)?

> > Signed-off-by: Kirti Wankhede <address@hidden>
> > Reviewed-by: Neo Jia <address@hidden>
> > ---
> >   include/uapi/linux/vfio.h | 23 +++++++++++++++++++++++
> >   1 file changed, 23 insertions(+)
> >
> > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> > index 35b09427ad9f..6fd3822aa610 100644
> > --- a/include/uapi/linux/vfio.h
> > +++ b/include/uapi/linux/vfio.h
> > @@ -902,6 +902,29 @@ struct vfio_iommu_type1_dma_unmap {
> >   #define VFIO_IOMMU_ENABLE     _IO(VFIO_TYPE, VFIO_BASE + 15)
> >   #define VFIO_IOMMU_DISABLE    _IO(VFIO_TYPE, VFIO_BASE + 16)
> >   
> > +/**
> > + * VFIO_IOMMU_GET_DIRTY_BITMAP - _IOWR(VFIO_TYPE, VFIO_BASE + 17,
> > + *                                     struct vfio_iommu_type1_dirty_bitmap)
> > + *
> > + * IOCTL to get dirty pages bitmap for IOMMU container during migration.
> > + * Get dirty pages bitmap of given IO virtual addresses range using
> > + * struct vfio_iommu_type1_dirty_bitmap. Caller sets argsz, which is size of
> > + * struct vfio_iommu_type1_dirty_bitmap. User should allocate memory to get
> > + * bitmap and should set size of allocated memory in bitmap_size field.
> > + * One bit is used to represent per page consecutively starting from iova
> > + * offset. Bit set indicates page at that offset from iova is dirty.
> > + */
> > +struct vfio_iommu_type1_dirty_bitmap {
> > +       __u32        argsz;
> > +       __u32        flags;
> > +       __u64        iova;                      /* IO virtual address */
> > +       __u64        size;                      /* Size of iova range */
> > +       __u64        bitmap_size;               /* in bytes */
>
> This seems redundant.  We can calculate the size of the bitmap based on
> the iova size.

But in kernel space, we need to validate the size of memory allocated by 
user instead of assuming user is always correct, right?

> > +       void __user *bitmap;                    /* one bit per page */
>
> Should we define that as a __u64* to (a) help with the size
> calculation, and (b) assure that we can use 8-byte ops on it?
>
> However, who defines page size?  Is it necessarily the processor page
> size?  A physical IOMMU may support page sizes other than the CPU page
> size.  It might be more important to indicate the expected page size
> than the bitmap size.  Thanks,

I see in QEMU and in vfio_iommu_type1 module, page sizes considered for 
mapping are CPU page size, 4K. Do we still need to have such argument?

Thanks,
Kirti

> Alex
>
> > +};
> > +
> > +#define VFIO_IOMMU_GET_DIRTY_BITMAP             _IO(VFIO_TYPE, VFIO_BASE + 17)
> > +
> >   /* -------- Additional API for SPAPR TCE (Server POWERPC) IOMMU -------- */
> >   
> >   /*