Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) client-ip=2620:137:e000::3:2;
From:   Vitaly Kuznetsov <vkuznets@redhat.com>
To:     Sean Christopherson <seanjc@google.com>,
        Paolo Bonzini <pbonzini@redhat.com>
Cc:     kvm@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] KVM: x86/mmu: Declare flush_remote_tlbs{_range}() hooks
 iff HYPERV!=n
In-Reply-To: <20231018192325.1893896-1-seanjc@google.com>
References: <20231018192325.1893896-1-seanjc@google.com>
Date:   Thu, 19 Oct 2023 10:41:59 +0200
Message-ID: <87wmvj57hk.fsf@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

Sean Christopherson <seanjc@google.com> writes:

> 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.
>
> Cc: Vitaly Kuznetsov <vkuznets@redhat.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/include/asm/kvm-x86-ops.h |  2 ++
>  arch/x86/include/asm/kvm_host.h    | 12 ++++++++++++
>  arch/x86/kvm/mmu/mmu.c             | 12 ++++--------
>  3 files changed, 18 insertions(+), 8 deletions(-)
>
> diff --git a/arch/x86/include/asm/kvm-x86-ops.h b/arch/x86/include/asm/kvm-x86-ops.h
> index 26b628d84594..f482216bbdb8 100644
> --- a/arch/x86/include/asm/kvm-x86-ops.h
> +++ b/arch/x86/include/asm/kvm-x86-ops.h
> @@ -55,8 +55,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 7c228ae05df0..f0d1ac871465 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1614,9 +1614,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.
> @@ -1825,6 +1827,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_TLBS
>  static inline int kvm_arch_flush_remote_tlbs(struct kvm *kvm)
>  {
> @@ -1836,6 +1839,15 @@ static inline int kvm_arch_flush_remote_tlbs(struct kvm *kvm)
>  }
>  
>  #define __KVM_HAVE_ARCH_FLUSH_REMOTE_TLBS_RANGE
> +static inline int kvm_arch_flush_remote_tlbs_range(struct kvm *kvm, gfn_t gfn,
> +						   u64 nr_pages)
> +{
> +	if (!kvm_x86_ops.flush_remote_tlbs_range)
> +		return -EOPNOTSUPP;
> +
> +	return static_call(kvm_x86_flush_remote_tlbs_range)(kvm, gfn, nr_pages);
> +}
> +#endif /* CONFIG_HYPERV */
>  
>  #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 5d3dc7119e57..0702f5234d69 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -271,15 +271,11 @@ 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;
> -}
> -
> -int kvm_arch_flush_remote_tlbs_range(struct kvm *kvm, gfn_t gfn, u64 nr_pages)
> -{
> -	if (!kvm_x86_ops.flush_remote_tlbs_range)
> -		return -EOPNOTSUPP;
> -
> -	return static_call(kvm_x86_flush_remote_tlbs_range)(kvm, gfn, nr_pages);
> +#else
> +	return false;
> +#endif
>  }
>  
>  static gfn_t kvm_mmu_page_get_gfn(struct kvm_mmu_page *sp, int index);
>
> base-commit: 437bba5ad2bba00c2056c896753a32edf80860cc

Makes sense,

Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>

I can take it to my CONFIG_KVM_HYPERV series but it doesn't seem to
intersect with it so I guess there's no need for that.

-- 
Vitaly