Re: [PATCH v8 3/7] KVM: Support dirty ring in conjunction with bitmap

[Marc Zyngier <maz@kernel.org>]

On Mon, 07 Nov 2022 10:45:34 +0000,
Gavin Shan <gshan@redhat.com> wrote:
> 
> Hi Marc, Peter, Oliver and Sean,
> 
> On 11/5/22 7:40 AM, Gavin Shan wrote:
> > ARM64 needs to dirty memory outside of a VCPU context when VGIC/ITS is
> > enabled. It's conflicting with that ring-based dirty page tracking always
> > requires a running VCPU context.
> > 
> > Introduce a new flavor of dirty ring that requires the use of both VCPU
> > dirty rings and a dirty bitmap. The expectation is that for non-VCPU
> > sources of dirty memory (such as the VGIC/ITS on arm64), KVM writes to
> > the dirty bitmap. Userspace should scan the dirty bitmap before migrating
> > the VM to the target.
> > 
> > Use an additional capability to advertise this behavior. The newly added
> > capability (KVM_CAP_DIRTY_LOG_RING_WITH_BITMAP) can't be enabled before
> > KVM_CAP_DIRTY_LOG_RING_ACQ_REL on ARM64. In this way, the newly added
> > capability is treated as an extension of KVM_CAP_DIRTY_LOG_RING_ACQ_REL.
> > 
> > Suggested-by: Marc Zyngier <maz@kernel.org>
> > Suggested-by: Peter Xu <peterx@redhat.com>
> > Co-developed-by: Oliver Upton <oliver.upton@linux.dev>
> > Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
> > Signed-off-by: Gavin Shan <gshan@redhat.com>
> > Acked-by: Peter Xu <peterx@redhat.com>
> > ---
> >   Documentation/virt/kvm/api.rst | 33 ++++++++++++++++++-----
> >   include/linux/kvm_dirty_ring.h |  7 +++++
> >   include/linux/kvm_host.h       |  1 +
> >   include/uapi/linux/kvm.h       |  1 +
> >   virt/kvm/Kconfig               |  8 ++++++
> >   virt/kvm/dirty_ring.c          | 10 +++++++
> >   virt/kvm/kvm_main.c            | 49 +++++++++++++++++++++++++++-------
> >   7 files changed, 93 insertions(+), 16 deletions(-)
> > 
> > diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
> > index eee9f857a986..2ec32bd41792 100644
> > --- a/Documentation/virt/kvm/api.rst
> > +++ b/Documentation/virt/kvm/api.rst
> > @@ -8003,13 +8003,6 @@ flushing is done by the KVM_GET_DIRTY_LOG ioctl).  To achieve that, one
> >   needs to kick the vcpu out of KVM_RUN using a signal.  The resulting
> >   vmexit ensures that all dirty GFNs are flushed to the dirty rings.
> >   -NOTE: the capability KVM_CAP_DIRTY_LOG_RING and the corresponding
> > -ioctl KVM_RESET_DIRTY_RINGS are mutual exclusive to the existing ioctls
> > -KVM_GET_DIRTY_LOG and KVM_CLEAR_DIRTY_LOG.  After enabling
> > -KVM_CAP_DIRTY_LOG_RING with an acceptable dirty ring size, the virtual
> > -machine will switch to ring-buffer dirty page tracking and further
> > -KVM_GET_DIRTY_LOG or KVM_CLEAR_DIRTY_LOG ioctls will fail.
> > -
> >   NOTE: KVM_CAP_DIRTY_LOG_RING_ACQ_REL is the only capability that
> >   should be exposed by weakly ordered architecture, in order to indicate
> >   the additional memory ordering requirements imposed on userspace when
> > @@ -8018,6 +8011,32 @@ Architecture with TSO-like ordering (such as x86) are allowed to
> >   expose both KVM_CAP_DIRTY_LOG_RING and KVM_CAP_DIRTY_LOG_RING_ACQ_REL
> >   to userspace.
> >   +After using the dirty rings, the userspace needs to detect the
> > capability
> > +of KVM_CAP_DIRTY_LOG_RING_WITH_BITMAP to see whether the ring structures
> > +need to be backed by per-slot bitmaps. With this capability advertised
> > +and supported, it means the architecture can dirty guest pages without
> > +vcpu/ring context, so that some of the dirty information will still be
> > +maintained in the bitmap structure. KVM_CAP_DIRTY_LOG_RING_WITH_BITMAP
> > +can't be enabled until the capability of KVM_CAP_DIRTY_LOG_RING_ACQ_REL
> > +has been enabled.
> > +
> > +Note that the bitmap here is only a backup of the ring structure, and
> > +normally should only contain a very small amount of dirty pages, which
> > +needs to be transferred during VM downtime. Collecting the dirty bitmap
> > +should be the very last thing that the VMM does before transmitting state
> > +to the target VM. VMM needs to ensure that the dirty state is final and
> > +avoid missing dirty pages from another ioctl ordered after the bitmap
> > +collection.
> > +
> > +To collect dirty bits in the backup bitmap, the userspace can use the
> > +same KVM_GET_DIRTY_LOG ioctl. KVM_CLEAR_DIRTY_LOG shouldn't be needed
> > +and its behavior is undefined since collecting the dirty bitmap always
> > +happens in the last phase of VM's migration.
> > +
> > +NOTE: One example of using the backup bitmap is saving arm64 vgic/its
> > +tables through KVM_DEV_ARM_{VGIC_GRP_CTRL, ITS_SAVE_TABLES} command on
> > +KVM device "kvm-arm-vgic-its" during VM's migration.
> > +
> 
> In order to speed up the review and reduce unnecessary respins. After
> collecting comments on PATCH[v8 3/7] from Marc and Peter, I would change
> above description as below. Could you please confirm it looks good to you?
> 
> In the 4th paragraph, the words starting from "Collecting the dirty bitmap..."
> to the end, was previously suggested by Oliver, even Marc suggested to avoid
> mentioning "migration".
> 
>   After enabling the dirty rings, the userspace needs to detect the
>   capability of KVM_CAP_DIRTY_LOG_RING_WITH_BITMAP to see whether the ring
>   structures need to be backed by per-slot bitmaps. With this capability

s/need/can/. If there was a *need*, it should happen automatically
without user intervention.

>   advertised, it means the architecture can dirty guest pages without
>   vcpu/ring context, so that some of the dirty information will still be
>   maintained in the bitmap structure. KVM_CAP_DIRTY_LOG_RING_WITH_BITMAP
>   can't be enabled if the capability of KVM_CAP_DIRTY_LOG_RING_ACQ_REL
>   hasn't been enabled, or any memslot has been existing.
> 
>   Note that the bitmap here is only a backup of the ring structure. The
>   use of the ring and bitmap combination is only beneficial if there is
>   only a very small amount of memory that is dirtied out of vcpu/ring
>   context. Otherwise, the stand-alone per-slot bitmap mechanism needs to
>   be considered.
> 
>   To collect dirty bits in the backup bitmap, userspace can use the same
>   KVM_GET_DIRTY_LOG ioctl. KVM_CLEAR_DIRTY_LOG isn't needed as long as all
>   the generation of the dirty bits is done in a single pass. Collecting
>   the dirty bitmap should be the very last thing that the VMM does before
>   transmitting state to the target VM. VMM needs to ensure that the dirty
>   state is final and avoid missing dirty pages from another ioctl ordered
>   after the bitmap collection.

I would replace "transmitting state to the target VM" with
"considering the state as complete", as I still object to casting this
API into the migration mold. People use this stuff more far more than
migration (checkpointing, for example).

> 
>   NOTE: One example of using the backup bitmap is saving arm64 vgic/its
>   tables through KVM_DEV_ARM_{VGIC_GRP_CTRL, ITS_SAVE_TABLES} command on
>   KVM device "kvm-arm-vgic-its" during VM's migration.

Same remark about migration.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm