Re: [PATCH v6 3/8] KVM: Add support for using dirty ring in conjunction with bitmap

[Oliver Upton <oliver.upton@linux.dev>]

On Wed, Oct 19, 2022 at 06:20:32AM +0800, Gavin Shan wrote:
> Hi Peter,
> 
> On 10/19/22 12:07 AM, Peter Xu wrote:
> > On Tue, Oct 11, 2022 at 02:14:42PM +0800, Gavin Shan wrote:

[...]

> > IMHO it'll be great to start with something like below to describe the
> > userspace's responsibility to proactively detect the WITH_BITMAP cap:
> > 
> >    Before using the dirty rings, the userspace needs to detect the cap of
> >    KVM_CAP_DIRTY_LOG_RING_WITH_BITMAP to see whether the ring structures
> >    need to be backed by per-slot bitmaps.
> > 
> >    When KVM_CAP_DIRTY_LOG_RING_WITH_BITMAP returns 1, it means the arch can
> >    dirty guest pages without vcpu/ring context, so that some of the dirty
> >    information will still be maintained in the bitmap structure.
> > 
> >    Note that the bitmap here is only a backup of the ring structure, and it
> >    doesn't need to be collected until the final switch-over of migration
> >    process.  Normally the bitmap should only contain a very small amount of
> >    dirty pages only, which needs to be transferred during VM downtime.
> > 
> >    To collect dirty bits in the backup bitmap, the userspace can use the
> >    same KVM_GET_DIRTY_LOG ioctl.  Since it's always the last phase of
> >    migration that needs the fetching of dirty bitmap, KVM_CLEAR_DIRTY_LOG
> >    ioctl should not be needed in this case and its behavior undefined.
> > 
> > That's how I understand this new cap, but let me know if you think any of
> > above is inproper.
> > 
> 
> Yes, It looks much better to describe how KVM_CAP_DIRTY_LOG_RING_WITH_BITMAP
> is used. However, the missed part is the capability is still need to be enabled
> prior to KVM_CAP_DIRTY_LOG_RING_ACQ_REL on ARM64. It means the capability needs
> to be acknowledged (confirmed) by user space. Otherwise, KVM_CAP_DIRTY_LOG_RING_ACQ_REL
> can't be enabled successfully. It seems Oliver, you and I aren't on same page for
> this part. Please refer to below reply for more discussion. After the discussion
> is finalized, I can amend the description accordingly here.

I'll follow up on the details of the CAP below, but wanted to explicitly
note some stuff for documentation:

Collecting the dirty bitmap should be the very last thing that the VMM
does before transmitting state to the target VMM. You'll want to make
sure that the dirty state is final and avoid missing dirty pages from
another ioctl ordered after bitmap collection.

[...]

> > > +	case KVM_CAP_DIRTY_LOG_RING_WITH_BITMAP:
> > > +		kvm->dirty_ring_with_bitmap = true;
> > 
> > IIUC what Oliver wanted to suggest is we can avoid enabling of this cap,
> > then we don't need dirty_ring_with_bitmap field but instead we can check
> > against CONFIG_HAVE_KVM_DIRTY_RING_WITH_BITMAP when needed.
> > 
> > I think that'll make sense, because without the bitmap the ring won't work
> > with arm64, so not valid to not enable it at all.  But good to double check
> > with Oliver too.
> > 
> > The rest looks good to me, thanks,
> > 
> 
> It was suggested by Oliver to expose KVM_CAP_DIRTY_LOG_RING_WITH_BITMAP. The
> user space also needs to enable the capability prior to KVM_CAP_DIRTY_LOG_RING_ACQ_REL
> on ARM64. I may be missing something since Oliver and you had lots of discussion
> on this particular new capability.
> 
> I'm fine to drop the bits to enable KVM_CAP_DIRTY_LOG_RING_WITH_BITMAP. It means
> the capability is exposed to user space on ARM64 and user space need __not__ to
> enable it prior to KVM_CAP_DIRTY_LOG_RING_ACQ_REL. I would like Oliver helps to
> confirm before I'm able to post v7.

IMO you really want the explicit buy-in from userspace, as failure to
collect the dirty bitmap will result in a dead VM on the other side of
migration. Fundamentally we're changing the ABI of
KVM_CAP_DIRTY_LOG_RING[_ACQ_REL].

--
Thanks,
Oliver
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm