Re: [PATCH 2/3] KVM: x86/mmu: Fix pf_fixed count in tdp_mmu_map_handle_target_level()

[Sean Christopherson <seanjc@google.com>]

On Wed, May 05, 2021, Kai Huang wrote:
> Currently pf_fixed is increased even when page fault requires emulation,
> or fault is spurious.  Fix by only increasing it when return value is
> RET_PF_FIXED.
> 
> Signed-off-by: Kai Huang <kai.huang@intel.com>
> ---
>  arch/x86/kvm/mmu/tdp_mmu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
> index 1cad4c9f7c34..debe8c3ec844 100644
> --- a/arch/x86/kvm/mmu/tdp_mmu.c
> +++ b/arch/x86/kvm/mmu/tdp_mmu.c
> @@ -942,7 +942,7 @@ static int tdp_mmu_map_handle_target_level(struct kvm_vcpu *vcpu, int write,
>  				       rcu_dereference(iter->sptep));
>  	}
>  
> -	if (!prefault)
> +	if (!prefault && ret == RET_PF_FIXED)
>  		vcpu->stat.pf_fixed++;

Both this patch and the existing code are wrong to check prefault, and they both
deviate from the legacy MMU (both TDP and shadow) for RET_PF_EMULATE.

For prefault==true, KVM should indeed bump pf_fixed since "prefault" really means
"async page fault completed".  In that case, the original page fault from the
guest was morphed into an async page fault and stat.pf_fixed was _not_ incremented
because KVM hasn't actually fixed the fault.  The "prefault" flag is there
purely so that KVM doesn't injected a #PF into the guest in the case where the
guest unmapped the gpa while the async page fault was being handled.

For RET_PF_EMULATE, I could go either way.  On one hand, KVM is installing a
translation that accelerates future emulated MMIO, so it's kinda sorta fixing
the page fault.  On the other handle, future emulated MMIO still page faults, it
just gets handled without going through the full page fault handler.

If we do decide to omit RET_PF_EMULATE, it should be a separate patch and should
be done for all flavors of MMU.  For this patch, the correct code is:

	if (ret != RET_PF_SPURIOUS)
		vcpu->stat.pf_fixed++;

which works because "ret" cannot be RET_PF_RETRY.

Looking through the other code, KVM also fails to bump stat.pf_fixed in the fast
page fault case.  So, if we decide to fix/adjust RET_PF_EMULATE, I think it would
make sense to handle stat.pf_fixed in a common location.

The legacy MMU also prefetches on RET_PF_EMULATE, which isn't technically wrong,
but it's pretty much guaranteed to be a waste of time since prefetching only
installs SPTEs if there is a backing memslot and PFN.

Kai, if it's ok with you, I'll fold the above "ret != RET_PF_SPURIOUS" change
into a separate mini-series to address the other issues I pointed out.  I was
hoping I could paste patches for them inline and let you carry them, but moving
stat.pf_fixed handling to a common location requires additional code shuffling
because of async page faults :-/

Thanks!

>  	return ret;
> -- 
> 2.31.1
>