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@xxxxxxxxx> > --- > 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 >
