Re: [PATCH v5 3/7] KVM: x86: hyper-v: Move the remote TLB flush logic out of vmx

[Vitaly Kuznetsov]

Paolo Bonzini <pbonzini@xxxxxxxxxx> writes:

> On 10/06/21 13:20, Vitaly Kuznetsov wrote:
>
>>> +static inline void hv_track_root_tdp(struct kvm_vcpu *vcpu, hpa_t root_tdp)
>>> +{
>>> +	struct kvm_arch *kvm_arch = &vcpu->kvm->arch;
>>> +
>>> +	if (kvm_x86_ops.tlb_remote_flush == hv_remote_flush_tlb) {
>>> +		spin_lock(&kvm_arch->hv_root_tdp_lock);
>>> +		vcpu->arch.hv_root_tdp = root_tdp;
>>> +		if (root_tdp != kvm_arch->hv_root_tdp)
>>> +			kvm_arch->hv_root_tdp = INVALID_PAGE;
>>> +		spin_unlock(&kvm_arch->hv_root_tdp_lock);
>>> +	}
>>> +}
>>> +#else
>>> +static inline void hv_track_root_tdp(struct kvm_vcpu *vcpu, hpa_t root_tdp)
>>> +{
>>> +}
>>> +#endif
>>> +#endif
>> 
>> Super-nitpick: I'd suggest adding /* __ARCH_X86_KVM_KVM_ONHYPERV_H__ */
>> to the second '#endif' and /* IS_ENABLED(CONFIG_HYPERV) */ to '#else'
>> and the first one: files/functions tend to grow and it becomes hard to
>> see where the particular '#endif/#else' belongs.
>
> Done, thanks.  I've also changed the #if to just "#ifdef CONFIG_HYPERV", 
> since IS_ENABLED is only needed in C statements.

kvm/queue fails to compile and I blame this change:

In file included from arch/x86/kvm/svm/svm_onhyperv.c:16:
arch/x86/kvm/svm/svm_onhyperv.h: In function ‘svm_hv_hardware_setup’:
arch/x86/kvm/svm/svm_onhyperv.h:56:34: error: ‘hv_remote_flush_tlb’ undeclared (first use in this function); did you mean ‘svm_flush_tlb’?
   56 |   svm_x86_ops.tlb_remote_flush = hv_remote_flush_tlb;
      |                                  ^~~~~~~~~~~~~~~~~~~
      |                                  svm_flush_tlb
arch/x86/kvm/svm/svm_onhyperv.h:56:34: note: each undeclared identifier is reported only once for each function it appears in
arch/x86/kvm/svm/svm_onhyperv.h:58:5: error: ‘hv_remote_flush_tlb_with_range’ undeclared (first use in this function)
   58 |     hv_remote_flush_tlb_with_range;
      |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
make[2]: *** [scripts/Makefile.build:272: arch/x86/kvm/svm/svm_onhyperv.o] Error 1
make[2]: *** Waiting for unfinished jobs....
In file included from arch/x86/kvm/svm/svm.c:47:
arch/x86/kvm/svm/svm_onhyperv.h: In function ‘svm_hv_hardware_setup’:
arch/x86/kvm/svm/svm_onhyperv.h:56:34: error: ‘hv_remote_flush_tlb’ undeclared (first use in this function); did you mean ‘svm_flush_tlb’?
   56 |   svm_x86_ops.tlb_remote_flush = hv_remote_flush_tlb;
      |                                  ^~~~~~~~~~~~~~~~~~~
      |                                  svm_flush_tlb
arch/x86/kvm/svm/svm_onhyperv.h:56:34: note: each undeclared identifier is reported only once for each function it appears in
arch/x86/kvm/svm/svm_onhyperv.h:58:5: error: ‘hv_remote_flush_tlb_with_range’ undeclared (first use in this function)
   58 |     hv_remote_flush_tlb_with_range;
      |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
make[2]: *** [scripts/Makefile.build:272: arch/x86/kvm/svm/svm.o] Error 1
arch/x86/kvm/vmx/vmx.c: In function ‘hardware_setup’:
arch/x86/kvm/vmx/vmx.c:7752:34: error: ‘hv_remote_flush_tlb’ undeclared (first use in this function)
 7752 |   vmx_x86_ops.tlb_remote_flush = hv_remote_flush_tlb;
      |                                  ^~~~~~~~~~~~~~~~~~~
arch/x86/kvm/vmx/vmx.c:7752:34: note: each undeclared identifier is reported only once for each function it appears in
arch/x86/kvm/vmx/vmx.c:7754:5: error: ‘hv_remote_flush_tlb_with_range’ undeclared (first use in this function)
 7754 |     hv_remote_flush_tlb_with_range;
      |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~


(Note: CONFIG_HYPERV can be 'm'.)

The following:

index 96da53edfe83..1c67abf2eba9 100644
--- a/arch/x86/kvm/kvm_onhyperv.h
+++ b/arch/x86/kvm/kvm_onhyperv.h
@@ -6,7 +6,7 @@
 #ifndef __ARCH_X86_KVM_KVM_ONHYPERV_H__
 #define __ARCH_X86_KVM_KVM_ONHYPERV_H__
 
-#ifdef CONFIG_HYPERV
+#if IS_ENABLED(CONFIG_HYPERV)
 int hv_remote_flush_tlb_with_range(struct kvm *kvm,
                struct kvm_tlb_range *range);
 int hv_remote_flush_tlb(struct kvm *kvm);

saves the day for me.

-- 
Vitaly