Re: [PATCH v10 Kernel 4/5] vfio iommu: Implementation of ioctl to for dirty pages tracking.

[Yan Zhao]

On Tue, Dec 17, 2019 at 05:24:14PM +0800, Kirti Wankhede wrote:
> 
> 
> On 12/17/2019 10:45 AM, Yan Zhao wrote:
> > On Tue, Dec 17, 2019 at 04:21:39AM +0800, Kirti Wankhede wrote:
> >> VFIO_IOMMU_DIRTY_PAGES ioctl performs three operations:
> >> - Start unpinned pages dirty pages tracking while migration is active and
> >>    device is running, i.e. during pre-copy phase.
> >> - Stop unpinned pages dirty pages tracking. This is required to stop
> >>    unpinned dirty pages tracking if migration failed or cancelled during
> >>    pre-copy phase. Unpinned pages tracking is clear.
> >> - Get dirty pages bitmap. Stop unpinned dirty pages tracking and clear
> >>    unpinned pages information on bitmap read. This ioctl returns bitmap of
> >>    dirty pages, its user space application responsibility to copy content
> >>    of dirty pages from source to destination during migration.
> >>
> >> Signed-off-by: Kirti Wankhede <address@hidden>
> >> Reviewed-by: Neo Jia <address@hidden>
> >> ---
> >>   drivers/vfio/vfio_iommu_type1.c | 210 
> >> ++++++++++++++++++++++++++++++++++++++--
> >>   1 file changed, 203 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/drivers/vfio/vfio_iommu_type1.c 
> >> b/drivers/vfio/vfio_iommu_type1.c
> >> index 3f6b04f2334f..264449654d3f 100644
> >> --- a/drivers/vfio/vfio_iommu_type1.c
> >> +++ b/drivers/vfio/vfio_iommu_type1.c
> >> @@ -70,6 +70,7 @@ struct vfio_iommu {
> >>    unsigned int            dma_avail;
> >>    bool                    v2;
> >>    bool                    nesting;
> >> +  bool                    dirty_page_tracking;
> >>   };
> >>   
> >>   struct vfio_domain {
> >> @@ -112,6 +113,7 @@ struct vfio_pfn {
> >>    dma_addr_t              iova;           /* Device address */
> >>    unsigned long           pfn;            /* Host pfn */
> >>    atomic_t                ref_count;
> >> +  bool                    unpinned;
> >>   };
> >>   
> >>   struct vfio_regions {
> >> @@ -244,6 +246,32 @@ static void vfio_remove_from_pfn_list(struct vfio_dma 
> >> *dma,
> >>    kfree(vpfn);
> >>   }
> >>   
> >> +static void vfio_remove_unpinned_from_pfn_list(struct vfio_dma *dma, bool 
> >> warn)
> >> +{
> >> +  struct rb_node *n = rb_first(&dma->pfn_list);
> >> +
> >> +  for (; n; n = rb_next(n)) {
> >> +          struct vfio_pfn *vpfn = rb_entry(n, struct vfio_pfn, node);
> >> +
> >> +          if (warn)
> >> +                  WARN_ON_ONCE(vpfn->unpinned);
> >> +
> >> +          if (vpfn->unpinned)
> >> +                  vfio_remove_from_pfn_list(dma, vpfn);
> >> +  }
> >> +}
> >> +
> >> +static void vfio_remove_unpinned_from_dma_list(struct vfio_iommu *iommu)
> >> +{
> >> +  struct rb_node *n = rb_first(&iommu->dma_list);
> >> +
> >> +  for (; n; n = rb_next(n)) {
> >> +          struct vfio_dma *dma = rb_entry(n, struct vfio_dma, node);
> >> +
> >> +          vfio_remove_unpinned_from_pfn_list(dma, false);
> >> +  }
> >> +}
> >> +
> >>   static struct vfio_pfn *vfio_iova_get_vfio_pfn(struct vfio_dma *dma,
> >>                                           unsigned long iova)
> >>   {
> >> @@ -254,13 +282,17 @@ static struct vfio_pfn 
> >> *vfio_iova_get_vfio_pfn(struct vfio_dma *dma,
> >>    return vpfn;
> >>   }
> >>   
> >> -static int vfio_iova_put_vfio_pfn(struct vfio_dma *dma, struct vfio_pfn 
> >> *vpfn)
> >> +static int vfio_iova_put_vfio_pfn(struct vfio_dma *dma, struct vfio_pfn 
> >> *vpfn,
> >> +                            bool dirty_tracking)
> >>   {
> >>    int ret = 0;
> >>   
> >>    if (atomic_dec_and_test(&vpfn->ref_count)) {
> >>            ret = put_pfn(vpfn->pfn, dma->prot);
> > if physical page here is put, it may cause problem when pin this iova
> > next time:
> > vfio_iommu_type1_pin_pages {
> >      ...
> >      vpfn = vfio_iova_get_vfio_pfn(dma, iova);
> >      if (vpfn) {
> >          phys_pfn[i] = vpfn->pfn;
> >          continue;
> >      }
> >      ...
> > }
> > 
> 
> Good point. Fixing it as:
> 
>                  vpfn = vfio_iova_get_vfio_pfn(dma, iova);
>                  if (vpfn) {
> -                       phys_pfn[i] = vpfn->pfn;
> -                       continue;
> +                       if (vpfn->unpinned)
> +                               vfio_remove_from_pfn_list(dma, vpfn);
what about updating vpfn instead?

> +                       else {
> +                               phys_pfn[i] = vpfn->pfn;
> +                               continue;
> +                       }
>                  }
> 
> 
> 
> >> -          vfio_remove_from_pfn_list(dma, vpfn);
> >> +          if (dirty_tracking)
> >> +                  vpfn->unpinned = true;
> >> +          else
> >> +                  vfio_remove_from_pfn_list(dma, vpfn);
> > so the unpinned pages before dirty page tracking is not treated as
> > dirty?
> > 
> 
> Yes. That's we agreed on previous version:
> https://www.mail-archive.com/address@hidden/msg663157.html
> 
> >>    }
> >>    return ret;
> >>   }
> >> @@ -504,7 +536,7 @@ static int vfio_pin_page_external(struct vfio_dma 
> >> *dma, unsigned long vaddr,
> >>   }
> >>   
> >>   static int vfio_unpin_page_external(struct vfio_dma *dma, dma_addr_t 
> >> iova,
> >> -                              bool do_accounting)
> >> +                              bool do_accounting, bool dirty_tracking)
> >>   {
> >>    int unlocked;
> >>    struct vfio_pfn *vpfn = vfio_find_vpfn(dma, iova);
> >> @@ -512,7 +544,10 @@ static int vfio_unpin_page_external(struct vfio_dma 
> >> *dma, dma_addr_t iova,
> >>    if (!vpfn)
> >>            return 0;
> >>   
> >> -  unlocked = vfio_iova_put_vfio_pfn(dma, vpfn);
> >> +  if (vpfn->unpinned)
> >> +          return 0;
> >> +
> >> +  unlocked = vfio_iova_put_vfio_pfn(dma, vpfn, dirty_tracking);
> >>   
> >>    if (do_accounting)
> >>            vfio_lock_acct(dma, -unlocked, true);
> >> @@ -583,7 +618,8 @@ static int vfio_iommu_type1_pin_pages(void *iommu_data,
> >>   
> >>            ret = vfio_add_to_pfn_list(dma, iova, phys_pfn[i]);
> >>            if (ret) {
> >> -                  vfio_unpin_page_external(dma, iova, do_accounting);
> >> +                  vfio_unpin_page_external(dma, iova, do_accounting,
> >> +                                           false);
> >>                    goto pin_unwind;
> >>            }
> >>    }
> >> @@ -598,7 +634,7 @@ static int vfio_iommu_type1_pin_pages(void *iommu_data,
> >>   
> >>            iova = user_pfn[j] << PAGE_SHIFT;
> >>            dma = vfio_find_dma(iommu, iova, PAGE_SIZE);
> >> -          vfio_unpin_page_external(dma, iova, do_accounting);
> >> +          vfio_unpin_page_external(dma, iova, do_accounting, false);
> >>            phys_pfn[j] = 0;
> >>    }
> >>   pin_done:
> >> @@ -632,7 +668,8 @@ static int vfio_iommu_type1_unpin_pages(void 
> >> *iommu_data,
> >>            dma = vfio_find_dma(iommu, iova, PAGE_SIZE);
> >>            if (!dma)
> >>                    goto unpin_exit;
> >> -          vfio_unpin_page_external(dma, iova, do_accounting);
> >> +          vfio_unpin_page_external(dma, iova, do_accounting,
> >> +                                   iommu->dirty_page_tracking);
> >>    }
> >>   
> >>   unpin_exit:
> >> @@ -850,6 +887,88 @@ static unsigned long vfio_pgsize_bitmap(struct 
> >> vfio_iommu *iommu)
> >>    return bitmap;
> >>   }
> >>   
> >> +/*
> >> + * start_iova is the reference from where bitmaping started. This is 
> >> called
> >> + * from DMA_UNMAP where start_iova can be different than iova
> >> + */
> >> +
> >> +static void vfio_iova_dirty_bitmap(struct vfio_iommu *iommu, dma_addr_t 
> >> iova,
> >> +                            size_t size, uint64_t pgsize,
> >> +                            dma_addr_t start_iova, unsigned long *bitmap)
> >> +{
> >> +  struct vfio_dma *dma;
> >> +  dma_addr_t i = iova;
> >> +  unsigned long pgshift = __ffs(pgsize);
> >> +
> >> +  while ((dma = vfio_find_dma(iommu, i, pgsize))) {
> >> +          /* mark all pages dirty if all pages are pinned and mapped. */
> >> +          if (dma->iommu_mapped) {
> > This prevents pass-through devices from calling vfio_pin_pages to do
> > fine grained log dirty.
> 
> Yes, I mentioned that in yet TODO item in cover letter:
> 
> "If IOMMU capable device is present in the container, then all pages are
> marked dirty. Need to think smart way to know if IOMMU capable device's
> driver is smart to report pages to be marked dirty by pinning those 
> pages externally."
>
why not just check first if any vpfn present for IOMMU capable devices?

> 
> >> +                  dma_addr_t iova_limit;
> >> +
> >> +                  iova_limit = (dma->iova + dma->size) < (iova + size) ?
> >> +                               (dma->iova + dma->size) : (iova + size);
> >> +
> >> +                  for (; i < iova_limit; i += pgsize) {
> >> +                          unsigned int start;
> >> +
> >> +                          start = (i - start_iova) >> pgshift;
> >> +
> >> +                          __bitmap_set(bitmap, start, 1);
> >> +                  }
> >> +                  if (i >= iova + size)
> >> +                          return;
> >> +          } else {
> >> +                  struct rb_node *n = rb_first(&dma->pfn_list);
> >> +                  bool found = false;
> >> +
> >> +                  for (; n; n = rb_next(n)) {
> >> +                          struct vfio_pfn *vpfn = rb_entry(n,
> >> +                                                  struct vfio_pfn, node);
> >> +                          if (vpfn->iova >= i) {
> >> +                                  found = true;
> >> +                                  break;
> >> +                          }
> >> +                  }
> >> +
> >> +                  if (!found) {
> >> +                          i += dma->size;
> >> +                          continue;
> >> +                  }
> >> +
> >> +                  for (; n; n = rb_next(n)) {
> >> +                          unsigned int start;
> >> +                          struct vfio_pfn *vpfn = rb_entry(n,
> >> +                                                  struct vfio_pfn, node);
> >> +
> >> +                          if (vpfn->iova >= iova + size)
> >> +                                  return;
> >> +
> >> +                          start = (vpfn->iova - start_iova) >> pgshift;
> >> +
> >> +                          __bitmap_set(bitmap, start, 1);
> >> +
> >> +                          i = vpfn->iova + pgsize;
> >> +                  }
> >> +          }
> >> +          vfio_remove_unpinned_from_pfn_list(dma, false);
> >> +  }
> >> +}
> >> +
> >> +static long verify_bitmap_size(unsigned long npages, unsigned long 
> >> bitmap_size)
> >> +{
> >> +  long bsize;
> >> +
> >> +  if (!bitmap_size || bitmap_size > SIZE_MAX)
> >> +          return -EINVAL;
> >> +
> >> +  bsize = ALIGN(npages, BITS_PER_LONG) / sizeof(unsigned long);
> >> +
> >> +  if (bitmap_size < bsize)
> >> +          return -EINVAL;
> >> +
> >> +  return bsize;
> >> +}
> >> +
> >>   static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
> >>                         struct vfio_iommu_type1_dma_unmap *unmap)
> >>   {
> >> @@ -2298,6 +2417,83 @@ static long vfio_iommu_type1_ioctl(void *iommu_data,
> >>   
> >>            return copy_to_user((void __user *)arg, &unmap, minsz) ?
> >>                    -EFAULT : 0;
> >> +  } else if (cmd == VFIO_IOMMU_DIRTY_PAGES) {
> >> +          struct vfio_iommu_type1_dirty_bitmap range;
> >> +          uint32_t mask = VFIO_IOMMU_DIRTY_PAGES_FLAG_START |
> >> +                          VFIO_IOMMU_DIRTY_PAGES_FLAG_STOP |
> >> +                          VFIO_IOMMU_DIRTY_PAGES_FLAG_GET_BITMAP;
> >> +          int ret;
> >> +
> >> +          if (!iommu->v2)
> >> +                  return -EACCES;
> >> +
> >> +          minsz = offsetofend(struct vfio_iommu_type1_dirty_bitmap,
> >> +                              bitmap);
> >> +
> >> +          if (copy_from_user(&range, (void __user *)arg, minsz))
> >> +                  return -EFAULT;
> >> +
> >> +          if (range.argsz < minsz || range.flags & ~mask)
> >> +                  return -EINVAL;
> >> +
> >> +          if (range.flags & VFIO_IOMMU_DIRTY_PAGES_FLAG_START) {
> >> +                  iommu->dirty_page_tracking = true;
> >> +                  return 0;
> >> +          } else if (range.flags & VFIO_IOMMU_DIRTY_PAGES_FLAG_STOP) {
> >> +                  iommu->dirty_page_tracking = false;
> >> +
> >> +                  mutex_lock(&iommu->lock);
> >> +                  vfio_remove_unpinned_from_dma_list(iommu);
> >> +                  mutex_unlock(&iommu->lock);
> >> +                  return 0;
> >> +
> >> +          } else if (range.flags &
> >> +                           VFIO_IOMMU_DIRTY_PAGES_FLAG_GET_BITMAP) {
> >> +                  uint64_t iommu_pgmask;
> >> +                  unsigned long pgshift = __ffs(range.pgsize);
> >> +                  unsigned long *bitmap;
> >> +                  long bsize;
> >> +
> >> +                  iommu_pgmask =
> >> +                   ((uint64_t)1 << __ffs(vfio_pgsize_bitmap(iommu))) - 1;
> >> +
> >> +                  if (((range.pgsize - 1) & iommu_pgmask) !=
> >> +                      (range.pgsize - 1))
> >> +                          return -EINVAL;
> >> +
> >> +                  if (range.iova & iommu_pgmask)
> >> +                          return -EINVAL;
> >> +                  if (!range.size || range.size > SIZE_MAX)
> >> +                          return -EINVAL;
> >> +                  if (range.iova + range.size < range.iova)
> >> +                          return -EINVAL;
> >> +
> >> +                  bsize = verify_bitmap_size(range.size >> pgshift,
> >> +                                             range.bitmap_size);
> >> +                  if (bsize)
> >> +                          return ret;
> >> +
> >> +                  bitmap = kmalloc(bsize, GFP_KERNEL);
> >> +                  if (!bitmap)
> >> +                          return -ENOMEM;
> >> +
> >> +                  ret = copy_from_user(bitmap,
> >> +                       (void __user *)range.bitmap, bsize) ? -EFAULT : 0;
> >> +                  if (ret)
> >> +                          goto bitmap_exit;
> >> +
> >> +                  iommu->dirty_page_tracking = false;
> > why iommu->dirty_page_tracking is false here?
> > suppose this ioctl can be called several times.
> > 
> 
> This ioctl can be called several times, but once this ioctl is called 
> that means vCPUs are stopped and VFIO devices are stopped (i.e. in 
> stop-and-copy phase) and dirty pages bitmap are being queried by user.
> 
can't agree that VFIO_IOMMU_DIRTY_PAGES_FLAG_GET_BITMAP can only be
called in stop-and-copy phase.
As stated in last version, this will cause QEMU to get a wrong expectation
of VM downtime and this is also the reason for previously pinned pages
before log_sync cannot be treated as dirty. If this get bitmap ioctl can
be called early in save_setup phase, then it's no problem even all ram
is dirty.

Thanks
Yan
> 
> 
> > Thanks
> > Yan
> >> +                  mutex_lock(&iommu->lock);
> >> +                  vfio_iova_dirty_bitmap(iommu, range.iova, range.size,
> >> +                                       range.pgsize, range.iova, bitmap);
> >> +                  mutex_unlock(&iommu->lock);
> >> +
> >> +                  ret = copy_to_user((void __user *)range.bitmap, bitmap,
> >> +                                     range.bitmap_size) ? -EFAULT : 0;
> >> +bitmap_exit:
> >> +                  kfree(bitmap);
> >> +                  return ret;
> >> +          }
> >>    }
> >>   
> >>    return -ENOTTY;
> >> -- 
> >> 2.7.0
> >>