[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v15 Kernel 4/7] vfio iommu: Implementation of ioctl for dirty

From: Kirti Wankhede
Subject: Re: [PATCH v15 Kernel 4/7] vfio iommu: Implementation of ioctl for dirty pages tracking.
Date: Sat, 21 Mar 2020 00:12:04 +0530
User-agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:68.0) Gecko/20100101 Thunderbird/68.6.0

On 3/20/2020 11:31 PM, Alex Williamson wrote:
On Fri, 20 Mar 2020 23:19:14 +0530
Kirti Wankhede <address@hidden> wrote:

On 3/20/2020 4:27 AM, Alex Williamson wrote:
On Fri, 20 Mar 2020 01:46:41 +0530
Kirti Wankhede <address@hidden> wrote:


+static int vfio_iova_dirty_bitmap(struct vfio_iommu *iommu, dma_addr_t iova,
+                                 size_t size, uint64_t pgsize,
+                                 u64 __user *bitmap)
+       struct vfio_dma *dma;
+       unsigned long pgshift = __ffs(pgsize);
+       unsigned int npages, bitmap_size;
+       dma = vfio_find_dma(iommu, iova, 1);
+       if (!dma)
+               return -EINVAL;
+       if (dma->iova != iova || dma->size != size)
+               return -EINVAL;
+       npages = dma->size >> pgshift;
+       bitmap_size = DIRTY_BITMAP_BYTES(npages);
+       /* mark all pages dirty if all pages are pinned and mapped. */
+       if (dma->iommu_mapped)
+               bitmap_set(dma->bitmap, 0, npages);
+       if (copy_to_user((void __user *)bitmap, dma->bitmap, bitmap_size))
+               return -EFAULT;

We still need to reset the bitmap here, clearing and re-adding the
pages that are still pinned.


I thought you agreed on my reply to it

  > Why re-populate when there will be no change since
  > vfio_iova_dirty_bitmap() is called holding iommu->lock? If there is any
  > pin request while vfio_iova_dirty_bitmap() is still working, it will
  > wait till iommu->lock is released. Bitmap will be populated when page is
  > pinned.

As coded, dirty bits are only ever set in the bitmap, never cleared.
If a page is unpinned between iterations of the user recording the
dirty bitmap, it should be marked dirty in the iteration immediately
after the unpinning and not marked dirty in the following iteration.
That doesn't happen here.  We're reporting cumulative dirty pages since
logging was enabled, we need to be reporting dirty pages since the user
last retrieved the dirty bitmap.  The bitmap should be cleared and
currently pinned pages re-added after copying to the user.  Thanks,

Does that mean, we have to track every iteration? do we really need that tracking?

Generally the flow is:
- vendor driver pin x pages
- Enter pre-copy-phase where vCPUs are running - user starts dirty pages tracking, then user asks dirty bitmap, x pages reported dirty by VFIO_IOMMU_DIRTY_PAGES ioctl with _GET flag - In pre-copy phase, vendor driver pins y more pages, now bitmap consists of x+y bits set - In pre-copy phase, vendor driver unpins z pages, but bitmap is not updated, so again bitmap consists of x+y bits set.
- Enter in stop-and-copy phase, vCPUs are stopped, mdev devices are stopped
- user asks dirty bitmap - Since here vCPU and mdev devices are stopped, pages should not get dirty by guest driver or the physical device. Hence, x+y dirty pages would be reported.

I don't think we need to track every iteration of bitmap reporting.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]