Hi Peter/Oliver,
On 10/11/22 11:56 AM, Gavin Shan wrote:
On 10/11/22 9:12 AM, Oliver Upton wrote:
On Mon, Oct 10, 2022 at 08:20:29PM -0400, Peter Xu wrote:
On Mon, Oct 10, 2022 at 11:58:22PM +0000, Oliver Upton wrote:
I think this further drives the point home -- there's zero need for the
bitmap with dirty ring on x86, so why even support it? The proposal of
ALLOW_BITMAP && DIRTY_RING should be arm64-specific. Any other arch that
needs to dirty memory outside of a vCPU context can opt-in to the
behavior.
Yeah that sounds working too, but it'll be slightly hackish as then the
user app will need some "#ifdef ARM64" blocks for e.g. sync dirty bitmap.
With the new cap the user app can implement the whole ring with generic
code.
Isn't the current route of exposing ALLOW_BITMAP on other arches for no
reason headed in exactly that direction? Userspace would need to know if
it _really_ needs the dirty bitmap in addition to the dirty ring, which
could take the form of architecture ifdeffery.
OTOH, if the cap is only exposed when it is absolutely necessary, an
arch-generic live migration implementation could enable the cap whenever
it is advertized and scan the bitmap accordingly.
The VMM must know something about the architecture it is running on, as
it calls KVM_DEV_ARM_ITS_SAVE_TABLES after all...
It looks good to me by using CONFIG_HAVE_KVM_DIRTY_RING_USE_BITMAP to
opt-in KVM_CAP_DIRTY_LOG_RING_WITH_BITMAP. The most important point is
to ensure 'kvm->dirty_ring_with_bitmap == true' when dirty ring capability
is enabled. In this way, we can fail early when KVM_CAP_DIRTY_LOG_RING_WITH_BITMAP
isn't enabled on attempt to enable dirty ring capability.
If both of you agree, I will integrate the suggested code changes in
next respin, with necessary tweak.
- In kvm_vm_ioctl_enable_cap_generic(), 'kvm->dirty_ring_with_bitmap' is
updated to 'true' unconditionally.
static int kvm_vm_ioctl_enable_cap_generic(struct kvm *kvm,
struct kvm_enable_cap *cap)
{
:
case KVM_CAP_DIRTY_LOG_RING_WITH_BITMAP:
kvm->dirty_ring_with_bitmap = true;
return 0;
}
- In mark_page_dirty_in_slot(), we need comprehensive checks like below.
void mark_page_dirty_in_slot(struct kvm *kvm,
const struct kvm_memory_slot *memslot,
gfn_t gfn)
{
#ifdef CONFIG_HAVE_KVM_DIRTY_RING
if (WARN_ON_ONCE(vcpu && vcpu->kvm != kvm))
return;
#ifndef CONFIG_HAVE_KVM_DIRTY_RING_WITH_BITMAP
if (WARN_ON_ONCE(!vcpu))
return;
#endif
#endif
}
- Use kvm_dirty_ring_exclusive(), which was suggested by Peter before.
The function is used in various spots to allow the dirty bitmap is
created and accessed.
bool kvm_dirty_ring_exclusive(struct kvm *kvm)
{
return kvm->dirty_ring_size && !kvm->dirty_ring_with_bitmap;
}
I've included Oliver's suggested changes into v6, which was just posted:
https://lore.kernel.org/kvmarm/3123a04f-a674-782b-9e9b-0baf3db49ebc@xxxxxxxxxx/
Please find your time to review v6 directly, thanks!
Also more flexible to expose it as generic cap? E.g., one day x86 can
enable this too for whatever reason (even though I don't think so..).
I had imagined something like this patch where the arch opts-in to some
generic construct if it *requires* the use of both the ring and bitmap
(very rough sketch).
Thanks,
Gavin
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
