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!
> 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...
> {
> 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.
>
> /*
> * 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 :-)
> */
> 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;
}
Hmm?
--
Thanks,
Oliver
