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. > +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 > + 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 > @@ -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. > + 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); > } > -- > 2.23.0 >
