On Thu, Dec 19, 2019 at 04:05:52AM +0800, Dr. David Alan Gilbert wrote:
* Yan Zhao (address@hidden) wrote:
On Tue, Dec 17, 2019 at 07:47:05PM +0800, Kirti Wankhede wrote:
On 12/17/2019 3:21 PM, Yan Zhao wrote:
On Tue, Dec 17, 2019 at 05:24:14PM +0800, Kirti Wankhede wrote:
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.
Device can also write to pages which are pinned, and then there is no
way to know pages dirtied by device during pre-copy phase.
If user ask dirty bitmap in per-copy phase, even then user will have to
query dirty bitmap in stop-and-copy phase where this will be superset
including all pages reported during pre-copy. Then instead of copying
all pages twice, its better to do it once during stop-and-copy phase.
I think the flow should be like this:
1. save_setup --> GET_BITMAP ioctl --> return bitmap for currently + previously
pinned pages and clean all previously pinned pages
2. save_pending --> GET_BITMAP ioctl --> return bitmap of (currently
pinned pages + previously pinned pages since last clean) and clean all
previously pinned pages
3. save_complete_precopy --> GET_BITMAP ioctl --> return bitmap of (currently
pinned pages + previously pinned pages since last clean) and clean all
previously pinned pages
Copying pinned pages multiple times is unavoidable because those pinned pages
are always treated as dirty. That's per vendor's implementation.
But if the pinned pages are not reported as dirty before stop-and-copy phase,
QEMU would think dirty pages has converged
and enter blackout phase, making downtime_limit severely incorrect.
I'm not sure it's any worse.
I *think* we do a last sync after we've decided to go to stop-and-copy;
wont that then mark all those pages as dirty again, so it'll have the
same behaviour?
No. something will be different.
currently, in kirti's implementation, if GET_BITMAP ioctl is called only
once in stop-and-copy phase, then before that phase, QEMU does not know those
pages are dirty.
If we can report those dirty pages earlier before stop-and-copy phase,
QEMU can at least copy other pages to reduce dirty pages to below threshold.
Take a example, let's assume those vfio dirty pages is 1Gb, and network speed is
also 1Gb. Expected vm downtime is 1s.
If before stop-and-copy phase, dirty pages produced by other pages is
also 1Gb. To meet the expected vm downtime, QEMU should copy pages to
let dirty pages be less than 1Gb, otherwise, it should not complete live
migration.
If vfio does not report this 1Gb dirty pages, QEMU would think there's
only 1Gb and stop the vm. It would then find out there's actually 2Gb and vm
downtime is 2s.
Though the expected vm downtime is always not exactly the same as the
true vm downtime, it should be caused by rapid dirty page rate, which is
not predictable.
Right?