Re: [PATCH v14 Kernel 4/7] vfio iommu: Implementation of ioctl for dirty pages tracking.

[Kirti Wankhede]

On 3/19/2020 9:15 AM, Alex Williamson wrote:
> On Thu, 19 Mar 2020 01:11:11 +0530
> Kirti Wankhede <address@hidden> wrote:
>
> > VFIO_IOMMU_DIRTY_PAGES ioctl performs three operations:
> > - Start dirty pages tracking while migration is active
> > - Stop dirty pages tracking.
> > - Get dirty pages bitmap. Its user space application's responsibility to
> >    copy content of dirty pages from source to destination during migration.
> >
> > To prevent DoS attack, memory for bitmap is allocated per vfio_dma
> > structure. Bitmap size is calculated considering smallest supported page
> > size. Bitmap is allocated for all vfio_dmas when dirty logging is enabled
> >
> > Bitmap is populated for already pinned pages when bitmap is allocated for
> > a vfio_dma with the smallest supported page size. Update bitmap from
> > pinning functions when tracking is enabled. When user application queries
> > bitmap, check if requested page size is same as page size used to
> > populated bitmap. If it is equal, copy bitmap, but if not equal, return
> > error.
> >
> > Signed-off-by: Kirti Wankhede <address@hidden>
> > Reviewed-by: Neo Jia <address@hidden>
> > ---
> >   drivers/vfio/vfio_iommu_type1.c | 205 +++++++++++++++++++++++++++++++++++++++-
> >   1 file changed, 203 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> > index 70aeab921d0f..d6417fb02174 100644
> > --- a/drivers/vfio/vfio_iommu_type1.c
> > +++ b/drivers/vfio/vfio_iommu_type1.c
> > @@ -71,6 +71,7 @@ struct vfio_iommu {
> >         unsigned int            dma_avail;
> >         bool                    v2;
> >         bool                    nesting;
> > +       bool                    dirty_page_tracking;
> >   };
> >   
> >   struct vfio_domain {
> > @@ -91,6 +92,7 @@ struct vfio_dma {
> >         bool                    lock_cap;       /* capable(CAP_IPC_LOCK) */
> >         struct task_struct      *task;
> >         struct rb_root          pfn_list;       /* Ex-user pinned pfn list */
> > +       unsigned long           *bitmap;
>
> We've made the bitmap a width invariant u64 else, should be here as
> well.

Changing to u64 causes compile time warnings as below. Keeping 'unsigned 
long *'

drivers/vfio/vfio_iommu_type1.c: In function ‘vfio_dma_bitmap_alloc_all’:
drivers/vfio/vfio_iommu_type1.c:232:8: warning: passing argument 1 of 
‘bitmap_set’ from incompatible pointer type [enabled by default]
        (vpfn->iova - dma->iova) / pgsize, 1);
        ^
In file included from ./include/linux/cpumask.h:12:0,
                 from ./arch/x86/include/asm/cpumask.h:5,
                 from ./arch/x86/include/asm/msr.h:11,
                 from ./arch/x86/include/asm/processor.h:22,
                 from ./arch/x86/include/asm/cpufeature.h:5,
                 from ./arch/x86/include/asm/thread_info.h:53,
                 from ./include/linux/thread_info.h:38,
                 from ./arch/x86/include/asm/preempt.h:7,
                 from ./include/linux/preempt.h:78,
                 from ./include/linux/spinlock.h:51,
                 from ./include/linux/seqlock.h:36,
                 from ./include/linux/time.h:6,
                 from ./include/linux/compat.h:10,
                 from drivers/vfio/vfio_iommu_type1.c:24:
./include/linux/bitmap.h:405:29: note: expected ‘long unsigned int *’ 
but argument is of type ‘u64 *’
 static __always_inline void bitmap_set(unsigned long *map, unsigned 
int start,

Thanks,
Kirti