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

From: Vitaly Kuznetsov <vkuznets@redhat.com>
To: Paolo Bonzini <pbonzini@redhat.com>,
	Vineeth Pillai <viremana@linux.microsoft.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
	"K. Y. Srinivasan" <kys@microsoft.com>,
	x86@kernel.org, kvm@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-hyperv@vger.kernel.org,
	Lan Tianyu <Tianyu.Lan@microsoft.com>,
	Michael Kelley <mikelley@microsoft.com>,
	Sean Christopherson <seanjc@google.com>,
	Tom Lendacky <thomas.lendacky@amd.com>,
	Wanpeng Li <wanpengli@tencent.com>,
	Jim Mattson <jmattson@google.com>, Joerg Roedel <joro@8bytes.org>,
	Wei Liu <wei.liu@kernel.org>,
	Stephen Hemminger <sthemmin@microsoft.com>,
	Haiyang Zhang <haiyangz@microsoft.com>
Subject: Re: [PATCH v5 3/7] KVM: x86: hyper-v: Move the remote TLB flush logic out of vmx
Date: Fri, 11 Jun 2021 09:20:12 +0200	[thread overview]
Message-ID: <87k0n0yij7.fsf@vitty.brq.redhat.com> (raw)
In-Reply-To: <3b74b538-0b28-7a00-0b26-0f926cd8f37e@redhat.com>

Paolo Bonzini <pbonzini@redhat.com> 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

