| Message ID | 20250207030900.1808-1-yan.y.zhao@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Small changes related to prefetch and spurious faults | expand |
On Fri, Feb 07, 2025, Yan Zhao wrote: > Make sure pfn is not changed for a spurious fault by warning in the TDP > MMU. For shadow path, only treat a prefetch fault as spurious when pfn is > not changed, since the rmap removal and add are required when pfn is > changed. I like sanity checks, but I don't like special casing "prefetch" faults like this. KVM should _never_ change the PFN of a shadow-present SPTE. The TDP MMU already BUG()s on this, and mmu_spte_update() WARNs on the transition.
On Fri, Feb 07, 2025 at 07:07:06AM -0800, Sean Christopherson wrote: > On Fri, Feb 07, 2025, Yan Zhao wrote: > > Make sure pfn is not changed for a spurious fault by warning in the TDP > > MMU. For shadow path, only treat a prefetch fault as spurious when pfn is > > not changed, since the rmap removal and add are required when pfn is > > changed. > > I like sanity checks, but I don't like special casing "prefetch" faults like this. > KVM should _never_ change the PFN of a shadow-present SPTE. The TDP MMU already > BUG()s on this, and mmu_spte_update() WARNs on the transition. However, both TDP MMU and mmu_set_spte() return RET_PF_SPURIOUS directly before the BUG() in TDP MMU or mmu_spte_update() could be hit.
On Sat, Feb 08, 2025, Yan Zhao wrote: > On Fri, Feb 07, 2025 at 07:07:06AM -0800, Sean Christopherson wrote: > > On Fri, Feb 07, 2025, Yan Zhao wrote: > > > Make sure pfn is not changed for a spurious fault by warning in the TDP > > > MMU. For shadow path, only treat a prefetch fault as spurious when pfn is > > > not changed, since the rmap removal and add are required when pfn is > > > changed. > > > > I like sanity checks, but I don't like special casing "prefetch" faults like this. > > KVM should _never_ change the PFN of a shadow-present SPTE. The TDP MMU already > > BUG()s on this, and mmu_spte_update() WARNs on the transition. > However, both TDP MMU and mmu_set_spte() return RET_PF_SPURIOUS directly before > the BUG() in TDP MMU or mmu_spte_update() could be hit. Ah, that's very different than treating a prefetch fault as !spurious though. I would be a-ok with this: if (is_shadow_present_pte(iter->old_spte) && (fault->prefetch || is_access_allowed(fault, iter->old_spte)) && is_last_spte(iter->old_spte, iter->level)) { WARN_ON_ONCE(fault->pfn != spte_to_pfn(iter->old_spte)); return RET_PF_SPURIOUS; }
On Mon, Feb 10, 2025 at 02:23:38PM -0800, Sean Christopherson wrote: > On Sat, Feb 08, 2025, Yan Zhao wrote: > > On Fri, Feb 07, 2025 at 07:07:06AM -0800, Sean Christopherson wrote: > > > On Fri, Feb 07, 2025, Yan Zhao wrote: > > > > Make sure pfn is not changed for a spurious fault by warning in the TDP > > > > MMU. For shadow path, only treat a prefetch fault as spurious when pfn is > > > > not changed, since the rmap removal and add are required when pfn is > > > > changed. > > > > > > I like sanity checks, but I don't like special casing "prefetch" faults like this. > > > KVM should _never_ change the PFN of a shadow-present SPTE. The TDP MMU already > > > BUG()s on this, and mmu_spte_update() WARNs on the transition. > > However, both TDP MMU and mmu_set_spte() return RET_PF_SPURIOUS directly before > > the BUG() in TDP MMU or mmu_spte_update() could be hit. > > Ah, that's very different than treating a prefetch fault as !spurious though. I > would be a-ok with this: > > if (is_shadow_present_pte(iter->old_spte) && > (fault->prefetch || is_access_allowed(fault, iter->old_spte)) && > is_last_spte(iter->old_spte, iter->level)) { > WARN_ON_ONCE(fault->pfn != spte_to_pfn(iter->old_spte)); > return RET_PF_SPURIOUS; > } Thanks! Will also update the shadow MMU part as below. if (is_shadow_present_pte(*sptep)) { if (prefetch && is_last_spte(*sptep, level)) { WARN_ON_ONCE(pfn != spte_to_pfn(*sptep)); return RET_PF_SPURIOUS; } }
diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c index 3d74e680006f..47fd3712afe6 100644 --- a/arch/x86/kvm/mmu/mmu.c +++ b/arch/x86/kvm/mmu/mmu.c @@ -2846,7 +2846,8 @@ static int mmu_set_spte(struct kvm_vcpu *vcpu, struct kvm_memory_slot *slot, } if (is_shadow_present_pte(*sptep)) { - if (prefetch && is_last_spte(*sptep, level)) + if (prefetch && is_last_spte(*sptep, level) && + pfn == spte_to_pfn(*sptep)) return RET_PF_SPURIOUS; /* diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c index 5f9e7374220e..8b37e4f542f3 100644 --- a/arch/x86/kvm/mmu/tdp_mmu.c +++ b/arch/x86/kvm/mmu/tdp_mmu.c @@ -1139,7 +1139,8 @@ static int tdp_mmu_map_handle_target_level(struct kvm_vcpu *vcpu, if (is_shadow_present_pte(iter->old_spte) && is_access_allowed(fault, iter->old_spte) && - is_last_spte(iter->old_spte, iter->level)) + is_last_spte(iter->old_spte, iter->level) && + !WARN_ON_ONCE(fault->pfn != spte_to_pfn(iter->old_spte))) return RET_PF_SPURIOUS; if (unlikely(!fault->slot))
Make sure pfn is not changed for a spurious fault by warning in the TDP MMU. For shadow path, only treat a prefetch fault as spurious when pfn is not changed, since the rmap removal and add are required when pfn is changed. Cc: Sean Christopherson <seanjc@google.com> Signed-off-by: Yan Zhao <yan.y.zhao@intel.com> --- arch/x86/kvm/mmu/mmu.c | 3 ++- arch/x86/kvm/mmu/tdp_mmu.c | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-)