Hi Sean,
On 11/9/22 12:25 AM, Sean Christopherson wrote:
On Tue, Nov 08, 2022, Gavin Shan wrote:
diff --git a/virt/kvm/Kconfig b/virt/kvm/Kconfig
index 800f9470e36b..228be1145cf3 100644
--- a/virt/kvm/Kconfig
+++ b/virt/kvm/Kconfig
@@ -33,6 +33,14 @@ config HAVE_KVM_DIRTY_RING_ACQ_REL
bool
select HAVE_KVM_DIRTY_RING
+# Only architectures that need to dirty memory outside of a vCPU
+# context should select this, advertising to userspace the
+# requirement to use a dirty bitmap in addition to the vCPU dirty
+# ring.
The Kconfig does more than advertise a feature to userspace.
# Allow enabling both the dirty bitmap and dirty ring. Only architectures that
# need to dirty memory outside of a vCPU context should select this.
Agreed. The comments will be adjusted accordingly.
+config HAVE_KVM_DIRTY_RING_WITH_BITMAP
I think we should replace "HAVE" with "NEED". Any architecture that supports the
dirty ring can easily support ring+bitmap, but based on the discussion from v5[*],
the comment above, and the fact that the bitmap will _never_ be used in the
proposed implementation because x86 will always have a vCPU, this Kconfig should
only be selected if the bitmap is needed to support migration.
[*] https://lore.kernel.org/all/Y0SxnoT5u7+1TCT+@xxxxxxxxxx
Both look good to me. Lets change it to CONFIG_NEED_KVM_DIRTY_RING_WITH_BITMAP
then.
+ bool
+ depends on HAVE_KVM_DIRTY_RING
+
config HAVE_KVM_EVENTFD
bool
select EVENTFD
diff --git a/virt/kvm/dirty_ring.c b/virt/kvm/dirty_ring.c
index fecbb7d75ad2..f95cbcdd74ff 100644
--- a/virt/kvm/dirty_ring.c
+++ b/virt/kvm/dirty_ring.c
@@ -21,6 +21,18 @@ u32 kvm_dirty_ring_get_rsvd_entries(void)
return KVM_DIRTY_RING_RSVD_ENTRIES + kvm_cpu_dirty_log_size();
}
+bool kvm_use_dirty_bitmap(struct kvm *kvm)
+{
+ lockdep_assert_held(&kvm->slots_lock);
+
+ return !kvm->dirty_ring_size || kvm->dirty_ring_with_bitmap;
+}
+
+bool __weak kvm_arch_allow_write_without_running_vcpu(struct kvm *kvm)
Rather than __weak, what about wrapping this with an #ifdef to effectively force
architectures to implement the override if they need ring+bitmap? Given that the
bitmap will never be used if there's a running vCPU, selecting the Kconfig without
overriding this utility can't possibly be correct.
#ifndef CONFIG_NEED_KVM_DIRTY_RING_WITH_BITMAP
bool kvm_arch_allow_write_without_running_vcpu(struct kvm *kvm)
{
return false;
}
#endif
It's a good idea, which will be included to next revision :)
@@ -4588,6 +4608,29 @@ static int kvm_vm_ioctl_enable_cap_generic(struct kvm *kvm,
return -EINVAL;
return kvm_vm_ioctl_enable_dirty_log_ring(kvm, cap->args[0]);
+ case KVM_CAP_DIRTY_LOG_RING_WITH_BITMAP: {
+ int r = -EINVAL;
+
+ if (!IS_ENABLED(CONFIG_HAVE_KVM_DIRTY_RING_WITH_BITMAP) ||
+ !kvm->dirty_ring_size)
I have no objection to disallowing userspace from disabling the combo, but I
think it's worth requiring cap->args[0] to be '0' just in case we change our minds
in the future.
I assume you're suggesting to have non-zero value in cap->args[0] to enable the
capability.
if (!IS_ENABLED(CONFIG_HAVE_KVM_DIRTY_RING_WITH_BITMAP) ||
!kvm->dirty_ring_size || !cap->args[0])
return r;
+ return r;
+
+ mutex_lock(&kvm->slots_lock);
+
+ /*
+ * For simplicity, allow enabling ring+bitmap if and only if
+ * there are no memslots, e.g. to ensure all memslots allocate
+ * a bitmap after the capability is enabled.
+ */
+ if (kvm_are_all_memslots_empty(kvm)) {
+ kvm->dirty_ring_with_bitmap = true;
+ r = 0;
+ }
+
+ mutex_unlock(&kvm->slots_lock);
+
+ return r;
+ }
default:
return kvm_vm_ioctl_enable_cap(kvm, cap);
}
Thanks,
Gavin
