Re: [PATCH 4/4] KVM: x86/mmu: Optimize not-present/MMIO SPTE check in get_mmio_spte()

[Vitaly Kuznetsov <vkuznets@redhat.com>]

Sean Christopherson <seanjc@google.com> writes:

> Check only the terminal leaf for a "!PRESENT || MMIO" SPTE when looking
> for reserved bits on valid, non-MMIO SPTEs.  The get_walk() helpers
> terminate their walks if a not-present or MMIO SPTE is encountered, i.e.
> the non-terminal SPTEs have already been verified to be regular SPTEs.
> This eliminates an extra check-and-branch in a relatively hot loop.
>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/kvm/mmu/mmu.c | 20 +++++++++++++-------
>  1 file changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index 4798a4472066..769855f5f0a1 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -3511,7 +3511,7 @@ static int get_walk(struct kvm_vcpu *vcpu, u64 addr, u64 *sptes, int *root_level
>  	return leaf;
>  }
>  
> -/* return true if reserved bit is detected on spte. */
> +/* return true if reserved bit(s) are detected on a valid, non-MMIO SPTE. */
>  static bool get_mmio_spte(struct kvm_vcpu *vcpu, u64 addr, u64 *sptep)
>  {
>  	u64 sptes[PT64_ROOT_MAX_LEVEL + 1];
> @@ -3534,11 +3534,20 @@ static bool get_mmio_spte(struct kvm_vcpu *vcpu, u64 addr, u64 *sptep)
>  		return reserved;
>  	}
>  
> +	*sptep = sptes[leaf];
> +
> +	/*
> +	 * Skip reserved bits checks on the terminal leaf if it's not a valid
> +	 * SPTE.  Note, this also (intentionally) skips MMIO SPTEs, which, by
> +	 * design, always have reserved bits set.  The purpose of the checks is
> +	 * to detect reserved bits on non-MMIO SPTEs. i.e. buggy SPTEs.
> +	 */
> +	if (!is_shadow_present_pte(sptes[leaf]))
> +		leaf++;
> +
>  	rsvd_check = &vcpu->arch.mmu->shadow_zero_check;
>  
> -	for (level = root; level >= leaf; level--) {
> -		if (!is_shadow_present_pte(sptes[level]))
> -			break;
> +	for (level = root; level >= leaf; level--)
>  		/*
>  		 * Use a bitwise-OR instead of a logical-OR to aggregate the
>  		 * reserved bit and EPT's invalid memtype/XWR checks to avoid
> @@ -3546,7 +3555,6 @@ static bool get_mmio_spte(struct kvm_vcpu *vcpu, u64 addr, u64 *sptep)
>  		 */
>  		reserved |= __is_bad_mt_xwr(rsvd_check, sptes[level]) |
>  			    __is_rsvd_bits_set(rsvd_check, sptes[level], level);
> -	}
>  
>  	if (reserved) {
>  		pr_err("%s: detect reserved bits on spte, addr 0x%llx, dump hierarchy:\n",
> @@ -3556,8 +3564,6 @@ static bool get_mmio_spte(struct kvm_vcpu *vcpu, u64 addr, u64 *sptep)
>  			       sptes[level], level);
>  	}
>  
> -	*sptep = sptes[leaf];
> -
>  	return reserved;
>  }

FWIW (as I got a bit lost in tdp-mmu code when checking that
is_shadow_present_pte() is always performed)

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

-- 
Vitaly