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

[Raghavendra Rao Ananta]

On Thu, Aug 10, 2023 at 4:04 PM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
>
> 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?
>
Since it is a little diverted from the rest of the series' goal (and
yes, the slowing down part), do you mind if we send it as a separate
series? :)
I'll keep the functions as we have on v8 for now.

Thank you.
Raghavendra
> 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
> --
>