[PATCH] kvm: x86: mmu: Drop the need_remote_flush() function

[PATCH] kvm: x86: mmu: Drop the need_remote_flush() function

[Junaid Shahid]

This is only used by kvm_mmu_pte_write(), which no longer actually
creates the new SPTE and instead just clears the old SPTE. So we
just need to check if the old SPTE was shadow-present instead of
calling need_remote_flush(). Hence we can drop this function. It was
incomplete anyway as it didn't take access-tracking into account.

This patch should not result in any functional change.

Signed-off-by: Junaid Shahid <junaids@google.com>
---
 arch/x86/kvm/mmu/mmu.c | 15 +--------------
 1 file changed, 1 insertion(+), 14 deletions(-)

diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index f0d7193db455..39959841beee 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -5333,19 +5333,6 @@ void kvm_mmu_free_obsolete_roots(struct kvm_vcpu *vcpu)
 	__kvm_mmu_free_obsolete_roots(vcpu->kvm, &vcpu->arch.guest_mmu);
 }
 
-static bool need_remote_flush(u64 old, u64 new)
-{
-	if (!is_shadow_present_pte(old))
-		return false;
-	if (!is_shadow_present_pte(new))
-		return true;
-	if ((old ^ new) & SPTE_BASE_ADDR_MASK)
-		return true;
-	old ^= shadow_nx_mask;
-	new ^= shadow_nx_mask;
-	return (old & ~new & SPTE_PERM_MASK) != 0;
-}
-
 static u64 mmu_pte_write_fetch_gpte(struct kvm_vcpu *vcpu, gpa_t *gpa,
 				    int *bytes)
 {
@@ -5491,7 +5478,7 @@ static void kvm_mmu_pte_write(struct kvm_vcpu *vcpu, gpa_t gpa,
 			mmu_page_zap_pte(vcpu->kvm, sp, spte, NULL);
 			if (gentry && sp->role.level != PG_LEVEL_4K)
 				++vcpu->kvm->stat.mmu_pde_zapped;
-			if (need_remote_flush(entry, *spte))
+			if (is_shadow_present_pte(entry))
 				flush = true;
 			++spte;
 		}

base-commit: a4850b5590d01bf3fb19fda3fc5d433f7382a974
-- 
2.37.1.359.gd136c6c3e2-goog

Re: [PATCH] kvm: x86: mmu: Drop the need_remote_flush() function

[David Matlack]

On Fri, Jul 22, 2022 at 07:43:16PM -0700, Junaid Shahid wrote:
> This is only used by kvm_mmu_pte_write(), which no longer actually
> creates the new SPTE and instead just clears the old SPTE. So we
> just need to check if the old SPTE was shadow-present instead of
> calling need_remote_flush(). Hence we can drop this function. It was
> incomplete anyway as it didn't take access-tracking into account.
> 
> This patch should not result in any functional change.

Even if we don't assume anything about the new SPTE, this commit flushes
TLBs in a superset of the current cases. So this change should
definitely be safe.

And then if we assume new SPTE is 0 (which it should be), I agree this
should not result in any functional change.

Reviewed-by: David Matlack <dmatlack@google.com>

> 
> Signed-off-by: Junaid Shahid <junaids@google.com>
> ---
>  arch/x86/kvm/mmu/mmu.c | 15 +--------------
>  1 file changed, 1 insertion(+), 14 deletions(-)
> 
> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index f0d7193db455..39959841beee 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -5333,19 +5333,6 @@ void kvm_mmu_free_obsolete_roots(struct kvm_vcpu *vcpu)
>  	__kvm_mmu_free_obsolete_roots(vcpu->kvm, &vcpu->arch.guest_mmu);
>  }
>  
> -static bool need_remote_flush(u64 old, u64 new)
> -{
> -	if (!is_shadow_present_pte(old))
> -		return false;
> -	if (!is_shadow_present_pte(new))
> -		return true;
> -	if ((old ^ new) & SPTE_BASE_ADDR_MASK)
> -		return true;
> -	old ^= shadow_nx_mask;
> -	new ^= shadow_nx_mask;
> -	return (old & ~new & SPTE_PERM_MASK) != 0;
> -}
> -
>  static u64 mmu_pte_write_fetch_gpte(struct kvm_vcpu *vcpu, gpa_t *gpa,
>  				    int *bytes)
>  {
> @@ -5491,7 +5478,7 @@ static void kvm_mmu_pte_write(struct kvm_vcpu *vcpu, gpa_t gpa,
>  			mmu_page_zap_pte(vcpu->kvm, sp, spte, NULL);
>  			if (gentry && sp->role.level != PG_LEVEL_4K)
>  				++vcpu->kvm->stat.mmu_pde_zapped;
> -			if (need_remote_flush(entry, *spte))
> +			if (is_shadow_present_pte(entry))
>  				flush = true;
>  			++spte;
>  		}
> 
> base-commit: a4850b5590d01bf3fb19fda3fc5d433f7382a974
> -- 
> 2.37.1.359.gd136c6c3e2-goog
> 

Re: [PATCH] kvm: x86: mmu: Drop the need_remote_flush() function

[Sean Christopherson]

On Mon, Jul 25, 2022, David Matlack wrote:
> On Fri, Jul 22, 2022 at 07:43:16PM -0700, Junaid Shahid wrote:
> > This is only used by kvm_mmu_pte_write(), which no longer actually
> > creates the new SPTE and instead just clears the old SPTE. So we
> > just need to check if the old SPTE was shadow-present instead of
> > calling need_remote_flush(). Hence we can drop this function. It was
> > incomplete anyway as it didn't take access-tracking into account.
> > 
> > This patch should not result in any functional change.
> 
> Even if we don't assume anything about the new SPTE, this commit flushes
> TLBs in a superset of the current cases. So this change should
> definitely be safe.
> 
> And then if we assume new SPTE is 0 (which it should be), I agree this
> should not result in any functional change.

Nit for posterity, zapped SPTEs don't necessarily have to be '0', e.g. KVM is
more than likely going to use 0x80000000_00000000 as the "zero" value for TDP MMU
SPTEs so that the SUPPRESS_VE is set for "zero"-initialized SPTEs (TDX requires
EPT Violation #VEs be enabled).

> Reviewed-by: David Matlack <dmatlack@google.com>

Reviewed-by: Sean Christopherson <seanjc@google.com>