Re: [PATCH v8 3/7] KVM: Support dirty ring in conjunction with bitmap

Gavin Shan <gshan@xxxxxxxxxx> · Tue, 8 Nov 2022 11:30:42 +0800

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