Re: [PATCH v8 02/14] KVM: Declare kvm_arch_flush_remote_tlbs() globally

[Sean Christopherson]

On Thu, Aug 10, 2023, Raghavendra Rao Ananta wrote:
> On Thu, Aug 10, 2023 at 3:20 PM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
> > On Thu, Aug 10, 2023, Raghavendra Rao Ananta wrote:
> > > After doing some experiments, I think it works because of the order in
> > > which the inline-definition and the declaration are laid out. If the
> > > 'inline' part of the function comes first and then the declaration, we
> > > don't see any error. However if the positions were reversed, we would
> > > see an error. (I'm not sure what the technical reason for this is).
> > >
> > > Just to be safe, I can move the definition to arch/x86/kvm/mmu/mmu.c
> > > as a non-inline function.
> >
> > No need, asm/kvm_host.h _must_ be included before the declaration, otherwise the
> > declaration wouldn't be made because __KVM_HAVE_ARCH_FLUSH_REMOTE_TLBS wouldn't
> > be defined.  I.e. we won't run into issues where the non-static declaration comes
> > before the static inline definition.
> >
> > C99 explicitly covers this case:
> >
> >   6.2.2 Linkages of identifiers
> >
> >   ...
> >
> >   If the declaration of a file scope identifier for an object or a function contains the storage-
> >   class specifier static, the identifier has internal linkage.
> >
> >   For an identifier declared with the storage-class specifier extern in a scope in which a
> >   prior declaration of that identifier is visible if the prior declaration specifies internal or
> >   external linkage, the linkage of the identifier at the later declaration is the same as the
> >   linkage specified at the prior declaration. If no prior declaration is visible, or if the prior
> >   declaration specifies no linkage, then the identifier has external linkage.
> >
> > In short, because the "static inline" declared internal linkage first, it wins.
> Thanks for sharing this! I can keep the 'static inline' definition as
> is then. However, since a later patch (patch-05/14) defines
> kvm_arch_flush_remote_tlbs_range() in arch/x86/kvm/mmu/mmu.c, do you
> think we can move this definition to the .c file as well for
> consistency?

We "can", but I don't see any reason to do so.  Trying to make helpers consistently
inline or not is usually a fools errand.  And in this case, I'd actually rather go
the opposite direction and make the range variant an inline.

Ha!  And I can justify that with minimal effort.  The below makes the helper a
straight passthrough for CONFIG_HYPERV=n builds, at which point I think it makes
sense for it to be inline.

If it won't slow your series down even more, any objection to sliding the below
patch in somewhere before patch 5?  And then add a patch to inline the range-based
helper?

Disclaimer: compile tested only.

---
From: Sean Christopherson <seanjc@xxxxxxxxxx>
Date: Thu, 10 Aug 2023 15:58:53 -0700
Subject: [PATCH] KVM: x86/mmu: Declare flush_remote_tlbs{_range}() hooks iff
 HYPERV!=n

Declare the kvm_x86_ops hooks used to wire up paravirt TLB flushes when
running under Hyper-V if and only if CONFIG_HYPERV!=n.  Wrapping yet more
code with IS_ENABLED(CONFIG_HYPERV) eliminates a handful of conditional
branches, and makes it super obvious why the hooks *might* be valid.

Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx>
---
 arch/x86/include/asm/kvm-x86-ops.h | 2 ++
 arch/x86/include/asm/kvm_host.h    | 4 ++++
 arch/x86/kvm/mmu/mmu.c             | 6 ++++++
 3 files changed, 12 insertions(+)

diff --git a/arch/x86/include/asm/kvm-x86-ops.h b/arch/x86/include/asm/kvm-x86-ops.h
index 13bc212cd4bc..6bc1ab0627b7 100644
--- a/arch/x86/include/asm/kvm-x86-ops.h
+++ b/arch/x86/include/asm/kvm-x86-ops.h
@@ -54,8 +54,10 @@ KVM_X86_OP(set_rflags)
 KVM_X86_OP(get_if_flag)
 KVM_X86_OP(flush_tlb_all)
 KVM_X86_OP(flush_tlb_current)
+#if IS_ENABLED(CONFIG_HYPERV)
 KVM_X86_OP_OPTIONAL(flush_remote_tlbs)
 KVM_X86_OP_OPTIONAL(flush_remote_tlbs_range)
+#endif
 KVM_X86_OP(flush_tlb_gva)
 KVM_X86_OP(flush_tlb_guest)
 KVM_X86_OP(vcpu_pre_run)
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 60d430b4650f..04fc80112dfe 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1604,9 +1604,11 @@ struct kvm_x86_ops {
 
 	void (*flush_tlb_all)(struct kvm_vcpu *vcpu);
 	void (*flush_tlb_current)(struct kvm_vcpu *vcpu);
+#if IS_ENABLED(CONFIG_HYPERV)
 	int  (*flush_remote_tlbs)(struct kvm *kvm);
 	int  (*flush_remote_tlbs_range)(struct kvm *kvm, gfn_t gfn,
 					gfn_t nr_pages);
+#endif
 
 	/*
 	 * Flush any TLB entries associated with the given GVA.
@@ -1814,6 +1816,7 @@ static inline struct kvm *kvm_arch_alloc_vm(void)
 #define __KVM_HAVE_ARCH_VM_FREE
 void kvm_arch_free_vm(struct kvm *kvm);
 
+#if IS_ENABLED(CONFIG_HYPERV)
 #define __KVM_HAVE_ARCH_FLUSH_REMOTE_TLB
 static inline int kvm_arch_flush_remote_tlb(struct kvm *kvm)
 {
@@ -1823,6 +1826,7 @@ static inline int kvm_arch_flush_remote_tlb(struct kvm *kvm)
 	else
 		return -ENOTSUPP;
 }
+#endif
 
 #define kvm_arch_pmi_in_guest(vcpu) \
 	((vcpu) && (vcpu)->arch.handling_intr_from_guest)
diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index 9e4cd8b4a202..0189dfecce1f 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -271,18 +271,24 @@ static inline unsigned long kvm_mmu_get_guest_pgd(struct kvm_vcpu *vcpu,
 
 static inline bool kvm_available_flush_remote_tlbs_range(void)
 {
+#if IS_ENABLED(CONFIG_HYPERV)
 	return kvm_x86_ops.flush_remote_tlbs_range;
+#else
+	return false;
+#endif
 }
 
 void kvm_flush_remote_tlbs_range(struct kvm *kvm, gfn_t start_gfn,
 				 gfn_t nr_pages)
 {
+#if IS_ENABLED(CONFIG_HYPERV)
 	int ret = -EOPNOTSUPP;
 
 	if (kvm_x86_ops.flush_remote_tlbs_range)
 		ret = static_call(kvm_x86_flush_remote_tlbs_range)(kvm, start_gfn,
 								   nr_pages);
 	if (ret)
+#endif
 		kvm_flush_remote_tlbs(kvm);
 }
 

base-commit: bc9e68820274c025840d3056d63f938d74ca35bb
--