Hi Oliver, On 11/8/22 9:13 AM, Oliver Upton wrote:
On Tue, Nov 08, 2022 at 08:44:52AM +0800, Gavin Shan wrote:Frankly, I don't expect the capability to be disabled. Similar to KVM_CAP_DIRTY_LOG_RING or KVM_CAP_DIRTY_LOG_RING_ACQ_REL, it would a one-shot capability and only enablement is allowed. The disablement was suggested by Oliver without providing a clarify, even I dropped it for several times. I would like to see if there is particular reason why Oliver want to disable the capability. kvm->dirty_ring_with_bitmap = cap->args[0]; If Oliver agrees that the capability needn't to be disabled. The whole chunk of code can be squeezed into kvm_vm_ioctl_enable_dirty_log_ring_with_bitmap() to make kvm_vm_ioctl_enable_cap_generic() a bit clean, as I said above.Sorry, I don't believe there is much use in disabling the cap, and really that hunk just came from lazily matching the neigbhoring caps when sketching out some suggestions. Oops!
Ok. It doesn't really matter too much except the comments seems conflicting. Thanks for confirming it's unnecessary to disable the capability.
Sean and Oliver, could you help to confirm if the changes look good to you? :) static int kvm_vm_ioctl_enable_dirty_log_ring_with_bitmap(struct kvm *kvm)This function name is ridiculously long...
Yeah, It seems I tempted to make the function name as comments :)
{ int i, r = 0; if (!IS_ENABLED(CONFIG_HAVE_KVM_DIRTY_RING_WITH_BITMAP) || !kvm->dirty_ring_size) return -EINVAL; mutex_lock(&kvm->slots_lock); /* We only allow it to set once */ if (kvm->dirty_ring_with_bitmap) { r = -EINVAL; goto out_unlock; }I don't believe this check is strictly necessary. Something similar to this makes sense with caps that take a numeric value (like KVM_CAP_DIRTY_LOG_RING), but this one is a one-way boolean.
Yep, it's not required strictly since it can represent two states.
/* * Avoid a race between memslot creation and enabling the ring + * bitmap capability to guarantee that no memslots have been * created without a bitmap.You'll want to pick up Sean's suggestion on the comment which, again, I drafted this in haste :-)
Ok, no worries :)
*/ for (i = 0; i < KVM_ADDRESS_SPACE_NUM; i++) { if (!kvm_memslots_empty(__kvm_memslots(kvm, i))) { r = -EINVAL; goto out_unlock; } }I'd much prefer you take Sean's suggestion and just create a helper to test that all memslots are empty. You avoid the insanely long function name and avoid the use of a goto statement. That is to say, leave the rest of the implementation inline in kvm_vm_ioctl_enable_cap_generic() static bool kvm_are_all_memslots_empty(struct kvm *kvm) { int i; lockdep_assert_held(&kvm->slots_lock); for (i = 0; i < KVM_ADDRESS_SPACE_NUM; i++) { if (!kvm_memslots_empty(__kvm_memslots(kvm, i))) return false; } return true; } static int kvm_vm_ioctl_enable_cap_generic(struct kvm *kvm, struct kvm_enable_cap *cap) { switch (cap->cap) { [...] 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) 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; } }
Ok. Lets change the chunk as Sean suggested in v9, which should be posted soon. Thanks, Gavin _______________________________________________ kvmarm mailing list kvmarm@xxxxxxxxxxxxxxxxxxxxx https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
