[PATCH v2 01/16] KVM: Take vcpu->mutex outside vcpu_load

[pbonzini@redhat.com (Paolo Bonzini)]

On 29/11/2017 18:17, David Hildenbrand wrote:
> On 29.11.2017 17:41, Christoffer Dall wrote:
>> As we're about to call vcpu_load() from architecture-specific
>> implementations of the KVM vcpu ioctls, but yet we access data
>> structures protected by the vcpu->mutex in the generic code, factor
>> this logic out from vcpu_load().
>>
>> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
>> ---
>>  arch/x86/kvm/vmx.c       |  4 +---
>>  arch/x86/kvm/x86.c       | 20 +++++++-------------
>>  include/linux/kvm_host.h |  2 +-
>>  virt/kvm/kvm_main.c      | 17 ++++++-----------
>>  4 files changed, 15 insertions(+), 28 deletions(-)
>>
>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>> index 714a067..e7c46d2 100644
>> --- a/arch/x86/kvm/vmx.c
>> +++ b/arch/x86/kvm/vmx.c
>> @@ -9559,10 +9559,8 @@ static void vmx_switch_vmcs(struct kvm_vcpu *vcpu, struct loaded_vmcs *vmcs)
>>  static void vmx_free_vcpu_nested(struct kvm_vcpu *vcpu)
>>  {
>>         struct vcpu_vmx *vmx = to_vmx(vcpu);
>> -       int r;
>>  
>> -       r = vcpu_load(vcpu);
>> -       BUG_ON(r);
>> +       vcpu_load(vcpu);
> I am most likely missing something, why don't we have to take the lock
> in these cases?

See earlier discussion, at these points there can be no concurrent
access; the file descriptor is not accessible yet, or is already gone.

Paolo

>>         vmx_switch_vmcs(vcpu, &vmx->vmcs01);
>>         free_nested(vmx);
>>         vcpu_put(vcpu);
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index 34c85aa..9b8f864 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -7747,16 +7747,12 @@ struct kvm_vcpu *kvm_arch_vcpu_create(struct kvm *kvm,
>>  
>>  int kvm_arch_vcpu_setup(struct kvm_vcpu *vcpu)
>>  {
>> -	int r;
>> -
>>  	kvm_vcpu_mtrr_init(vcpu);
>> -	r = vcpu_load(vcpu);
>> -	if (r)
>> -		return r;
>> +	vcpu_load(vcpu);
>>  	kvm_vcpu_reset(vcpu, false);
>>  	kvm_mmu_setup(vcpu);
>>  	vcpu_put(vcpu);
>> -	return r;
>> +	return 0;
>>  }
>>  
>>  void kvm_arch_vcpu_postcreate(struct kvm_vcpu *vcpu)
>> @@ -7766,13 +7762,15 @@ void kvm_arch_vcpu_postcreate(struct kvm_vcpu *vcpu)
>>  
>>  	kvm_hv_vcpu_postcreate(vcpu);
>>  
>> -	if (vcpu_load(vcpu))
>> +	if (mutex_lock_killable(&vcpu->mutex))
>>  		return;
>> +	vcpu_load(vcpu);
>>  	msr.data = 0x0;
>>  	msr.index = MSR_IA32_TSC;
>>  	msr.host_initiated = true;
>>  	kvm_write_tsc(vcpu, &msr);
>>  	vcpu_put(vcpu);
>> +	mutex_unlock(&vcpu->mutex);
>>  
>>  	if (!kvmclock_periodic_sync)
>>  		return;
>> @@ -7783,11 +7781,9 @@ void kvm_arch_vcpu_postcreate(struct kvm_vcpu *vcpu)
>>  
>>  void kvm_arch_vcpu_destroy(struct kvm_vcpu *vcpu)
>>  {
>> -	int r;
>>  	vcpu->arch.apf.msr_val = 0;
>>  
>> -	r = vcpu_load(vcpu);
>> -	BUG_ON(r);
>> +	vcpu_load(vcpu);
>>  	kvm_mmu_unload(vcpu);
>>  	vcpu_put(vcpu);
>>  
>> @@ -8155,9 +8151,7 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
>>  
>>  static void kvm_unload_vcpu_mmu(struct kvm_vcpu *vcpu)
>>  {
>> -	int r;
>> -	r = vcpu_load(vcpu);
>> -	BUG_ON(r);
>> +	vcpu_load(vcpu);
>>  	kvm_mmu_unload(vcpu);
>>  	vcpu_put(vcpu);
>>  }
>> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
>> index 2e754b7..a000dd8 100644
>> --- a/include/linux/kvm_host.h
>> +++ b/include/linux/kvm_host.h
>> @@ -533,7 +533,7 @@ static inline int kvm_vcpu_get_idx(struct kvm_vcpu *vcpu)
>>  int kvm_vcpu_init(struct kvm_vcpu *vcpu, struct kvm *kvm, unsigned id);
>>  void kvm_vcpu_uninit(struct kvm_vcpu *vcpu);
>>  
>> -int __must_check vcpu_load(struct kvm_vcpu *vcpu);
>> +void vcpu_load(struct kvm_vcpu *vcpu);
>>  void vcpu_put(struct kvm_vcpu *vcpu);
>>  
>>  #ifdef __KVM_HAVE_IOAPIC
>> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
>> index f169ecc..39961fb 100644
>> --- a/virt/kvm/kvm_main.c
>> +++ b/virt/kvm/kvm_main.c
>> @@ -146,17 +146,12 @@ bool kvm_is_reserved_pfn(kvm_pfn_t pfn)
>>  /*
>>   * Switches to specified vcpu, until a matching vcpu_put()
>>   */
>> -int vcpu_load(struct kvm_vcpu *vcpu)
>> +void vcpu_load(struct kvm_vcpu *vcpu)
>>  {
>> -	int cpu;
>> -
>> -	if (mutex_lock_killable(&vcpu->mutex))
>> -		return -EINTR;
>> -	cpu = get_cpu();
>> +	int cpu = get_cpu();
> 
> missing empty line.
> 
>>  	preempt_notifier_register(&vcpu->preempt_notifier);
>>  	kvm_arch_vcpu_load(vcpu, cpu);
>>  	put_cpu();
>> -	return 0;
>>  }
>>  EXPORT_SYMBOL_GPL(vcpu_load);
>>  
>> @@ -166,7 +161,6 @@ void vcpu_put(struct kvm_vcpu *vcpu)
>>  	kvm_arch_vcpu_put(vcpu);
>>  	preempt_notifier_unregister(&vcpu->preempt_notifier);
>>  	preempt_enable();
>> -	mutex_unlock(&vcpu->mutex);
>>  }
>>  EXPORT_SYMBOL_GPL(vcpu_put);
>>  
>> @@ -2529,9 +2523,9 @@ static long kvm_vcpu_ioctl(struct file *filp,
>>  #endif
>>  
>>  
>> -	r = vcpu_load(vcpu);
>> -	if (r)
>> -		return r;
>> +	if (mutex_lock_killable(&vcpu->mutex))
>> +		return -EINTR;
>> +	vcpu_load(vcpu);
>>  	switch (ioctl) {
>>  	case KVM_RUN: {
>>  		struct pid *oldpid;
>> @@ -2704,6 +2698,7 @@ static long kvm_vcpu_ioctl(struct file *filp,
>>  	}
>>  out:
>>  	vcpu_put(vcpu);
>> +	mutex_unlock(&vcpu->mutex);
>>  	kfree(fpu);
>>  	kfree(kvm_sregs);
>>  	return r;
>>
> 
>