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

[Gavin Shan <gshan@redhat.com>]

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+@google.com
> 

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

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm