linux-kernel - Re: [PATCH v2 01/25] KVM: x86/mmu: avoid indirect call for get

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 8 Mar 2022 16:16:31 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Paolo Bonzini <pbonzini@...hat.com>
Cc:     linux-kernel@...r.kernel.org, kvm@...r.kernel.org,
        dmatlack@...gle.com
Subject: Re: [PATCH v2 01/25] KVM: x86/mmu: avoid indirect call for get_cr3

On Mon, Feb 21, 2022, Paolo Bonzini wrote:
> Most of the time, calls to get_guest_pgd result in calling kvm_read_cr3
> (the exception is only nested TDP).  Check if that is the case if
> retpolines are enabled, thus avoiding an expensive indirect call.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@...hat.com>
> ---
>  arch/x86/kvm/mmu.h             | 10 ++++++++++
>  arch/x86/kvm/mmu/mmu.c         | 15 ++++++++-------
>  arch/x86/kvm/mmu/paging_tmpl.h |  2 +-
>  arch/x86/kvm/x86.c             |  2 +-
>  4 files changed, 20 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/x86/kvm/mmu.h b/arch/x86/kvm/mmu.h
> index 1d0c1904d69a..6ee4436e46f1 100644
> --- a/arch/x86/kvm/mmu.h
> +++ b/arch/x86/kvm/mmu.h
> @@ -116,6 +116,16 @@ static inline void kvm_mmu_load_pgd(struct kvm_vcpu *vcpu)
>  					  vcpu->arch.mmu->shadow_root_level);
>  }
>  
> +extern unsigned long kvm_get_guest_cr3(struct kvm_vcpu *vcpu);

No extern please, it's superfluous and against KVM style.  Moot point though, see
below.

> +static inline unsigned long kvm_mmu_get_guest_pgd(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu)

Wrap the params, no reason to make this line so long.

> +{
> +#ifdef CONFIG_RETPOLINE
> +	if (mmu->get_guest_pgd == kvm_get_guest_cr3)
> +		return kvm_read_cr3(vcpu);

This is unnecessarily fragile and confusing at first glance.  Compilers are smart
enough to generate a non-inline version of functions if they're used for function
pointers, while still inlining where appropriate.  In other words, just drop
kvm_get_guest_cr3() entirely, a al get_pdptr => kvm_pdptr_read().

---
 arch/x86/kvm/mmu.h     |  6 +++---
 arch/x86/kvm/mmu/mmu.c | 11 +++--------
 2 files changed, 6 insertions(+), 11 deletions(-)

diff --git a/arch/x86/kvm/mmu.h b/arch/x86/kvm/mmu.h
index 3af66b9df640..50528d39de8d 100644
--- a/arch/x86/kvm/mmu.h
+++ b/arch/x86/kvm/mmu.h
@@ -117,11 +117,11 @@ static inline void kvm_mmu_load_pgd(struct kvm_vcpu *vcpu)
 					  vcpu->arch.mmu->shadow_root_level);
 }

-extern unsigned long kvm_get_guest_cr3(struct kvm_vcpu *vcpu);
-static inline unsigned long kvm_mmu_get_guest_pgd(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu)
+static inline unsigned long kvm_mmu_get_guest_pgd(struct kvm_vcpu *vcpu,
+						  struct kvm_mmu *mmu)
 {
 #ifdef CONFIG_RETPOLINE
-	if (mmu->get_guest_pgd == kvm_get_guest_cr3)
+	if (mmu->get_guest_pgd == kvm_read_cr3)
 		return kvm_read_cr3(vcpu);
 #endif
 	return mmu->get_guest_pgd(vcpu);
diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index 995c3450c20f..cc2414397e4b 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -4234,11 +4234,6 @@ void kvm_mmu_new_pgd(struct kvm_vcpu *vcpu, gpa_t new_pgd)
 }
 EXPORT_SYMBOL_GPL(kvm_mmu_new_pgd);

-unsigned long kvm_get_guest_cr3(struct kvm_vcpu *vcpu)
-{
-	return kvm_read_cr3(vcpu);
-}
-
 static bool sync_mmio_spte(struct kvm_vcpu *vcpu, u64 *sptep, gfn_t gfn,
 			   unsigned int access)
 {
@@ -4793,7 +4788,7 @@ static void init_kvm_tdp_mmu(struct kvm_vcpu *vcpu)
 	context->invlpg = NULL;
 	context->shadow_root_level = kvm_mmu_get_tdp_level(vcpu);
 	context->direct_map = true;
-	context->get_guest_pgd = kvm_get_guest_cr3;
+	context->get_guest_pgd = kvm_read_cr3;
 	context->get_pdptr = kvm_pdptr_read;
 	context->inject_page_fault = kvm_inject_page_fault;
 	context->root_level = role_regs_to_root_level(&regs);
@@ -4968,7 +4963,7 @@ static void init_kvm_softmmu(struct kvm_vcpu *vcpu)

 	kvm_init_shadow_mmu(vcpu, &regs);

-	context->get_guest_pgd	   = kvm_get_guest_cr3;
+	context->get_guest_pgd	   = kvm_read_cr3;
 	context->get_pdptr         = kvm_pdptr_read;
 	context->inject_page_fault = kvm_inject_page_fault;
 }
@@ -5000,7 +4995,7 @@ static void init_kvm_nested_mmu(struct kvm_vcpu *vcpu)
 		return;

 	g_context->mmu_role.as_u64 = new_role.as_u64;
-	g_context->get_guest_pgd     = kvm_get_guest_cr3;
+	g_context->get_guest_pgd     = kvm_read_cr3;
 	g_context->get_pdptr         = kvm_pdptr_read;
 	g_context->inject_page_fault = kvm_inject_page_fault;
 	g_context->root_level        = new_role.base.level;

base-commit: c31df3e63672c14d8b52e34606c823e2166024b8
--