On Mon, Dec 16, 2024, bugzilla-daemon@xxxxxxxxxx wrote: > https://bugzilla.kernel.org/show_bug.cgi?id=219588 > > --- Comment #2 from leiyang@xxxxxxxxxx --- > (In reply to Sean Christopherson from comment #1) > > On Wed, Dec 11, 2024, bugzilla-daemon@xxxxxxxxxx wrote: > > > I hit a bug on the intel host, this problem occurs randomly: > > > [ 406.127925] ------------[ cut here ]------------ > > > [ 406.132572] WARNING: CPU: 52 PID: 12253 at > > arch/x86/kvm/mmu/tdp_mmu.c:1001 > > > tdp_mmu_map_handle_target_level+0x1f0/0x310 [kvm] > > > > Can you describe the host activity at the time of the WARN? E.g. is it under > > memory pressure and potentially swapping, is KSM or NUMA balancing active? I > > have a sound theory for how the scenario occurs on KVM's end, but I still > > think > > it's wrong for KVM to overwrite a writable SPTE with a read-only SPTE in this > > situation. > > I spent some time for this problem so late reply. When host dmesg print this > error messages which running install a new guest via automation. And I found > this bug's reproducer is run this install case after the mchine first time > running(Let me introduce more to avoid ambiguity： 1. Must to test it when the > machine first time running this kernel,that mean's if I hit this problem then > reboot host, it can not reproduced again even if I run the same tests. Yeah, WARN_ON_ONCE() suppresses the splat after a single occurrence. If KVM is built as a module, you can can unload and reload kvm.ko (and whatever vendor you're using) instead of rebooting. The flag that controls the "once" behavior is tied to the lifetime of kvm.ko. > 2. Based on 1, I also must test this installation guest case, it can not > reporduced on other cases.). But through compare, this installation cases > only used pxe install based on a internal KS cfg is different other cases. Mostly out of curiosity, what's "KS cfg"? > Sure, I think it's running under memory pressure and swapping. Based on > automation log, KSM is disable and I don't add NUMA in qemu command line. > > If you have a machine can clone avocado and run tp-qemu tests, you can prepare > env then run this case: > unattended_install.cdrom.extra_cdrom_ks.default_install.aio_threads > > > > > And does the VM have device memory or any other type of VM_PFNMAP or VM_IO > > memory exposed to it? E.g. an assigned device? If so, can you provide the > > register > > state from the other WARNs? If the PFNs are all in the same range, then > > maybe > > this is something funky with the VM_PFNMAP | VM_IO path. > > I can confirm it has VM_IO because it runing installation case, VM is > constantly performing I/O operations Heh, VM_IO and emulated I/O for the guest are two very different things. VM_IO would typically only be present if you're doing some form of device passthrough. Regardless, I don't know that it's worth trying to figure out exactly what's happening in the primary MMU. What's happening is perfectly legal, and the odds of there being an underlying bug *and* it not having any other symptoms is very low. And it definitely doesn't make sense to withold a fix just because I'm paranoid :-) So, assuming you can test custom kernels, can you test this and confirm it makes the WARN go away? If it does, or if you can't test it, I'll post a proper patch. If it doesn't fix the issue, then something completely unexpected is happening and we'll have to dig deeper. --- From: Sean Christopherson <seanjc@xxxxxxxxxx> Date: Mon, 16 Dec 2024 13:18:01 -0800 Subject: [PATCH] KVM: x86/mmu: Treat TDP MMU faults as spurious if access is already allowed Treat slow-path TDP MMU faults as spurious if the access is allowed given the existing SPTE to fix a benign warning (other than the WARN itself) due to replacing a writable SPTE with a read-only SPTE, and to avoid the unnecessary LOCK CMPXCHG and subsequent TLB flush. If a read fault races with a write fault, fast GUP fails for any reason when trying to "promote" the read fault to a writable mapping, and KVM resolves the write fault first, then KVM will end up trying to install a read-only SPTE (for a !map_writable fault) overtop a writable SPTE. Note, it's not entirely clear why fast GUP fails, or if that's even how KVM ends up with a !map_writable fault with a writable SPTE. If something else is going awry, e.g. due to a bug in mmu_notifiers, then treating read faults as spurious in this scenario could effectively mask the underlying problem. However, retrying the faulting access instead of overwriting an existing SPTE is functionally correct and desirable irrespective of the WARN, and fast GUP _can_ legitimately fail with a writable VMA, e.g. if the Accessed bit in primary MMU's PTE is toggled and causes a PTE value mismatch. The WARN was also recently added, specifically to track down scenarios where KVM is unnecessarily overwrites SPTEs, i.e. treating the fault as spurious doesn't regress KVM's bug-finding capabilities in any way. In short, letting the WARN linger because there's a tiny chance it's due to a bug elsewhere would be excessively paranoid. Fixes: 1a175082b190 ("KVM: x86/mmu: WARN and flush if resolving a TDP MMU fault clears MMU-writable") Reported-by: leiyang@xxxxxxxxxx Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219588 Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx> --- arch/x86/kvm/mmu/mmu.c | 12 ------------ arch/x86/kvm/mmu/spte.h | 17 +++++++++++++++++ arch/x86/kvm/mmu/tdp_mmu.c | 5 +++++ 3 files changed, 22 insertions(+), 12 deletions(-) diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c index 22e7ad235123..2401606db260 100644 --- a/arch/x86/kvm/mmu/mmu.c +++ b/arch/x86/kvm/mmu/mmu.c @@ -3364,18 +3364,6 @@ static bool fast_pf_fix_direct_spte(struct kvm_vcpu *vcpu, return true; } -static bool is_access_allowed(struct kvm_page_fault *fault, u64 spte) -{ - if (fault->exec) - return is_executable_pte(spte); - - if (fault->write) - return is_writable_pte(spte); - - /* Fault was on Read access */ - return spte & PT_PRESENT_MASK; -} - /* * Returns the last level spte pointer of the shadow page walk for the given * gpa, and sets *spte to the spte value. This spte may be non-preset. If no diff --git a/arch/x86/kvm/mmu/spte.h b/arch/x86/kvm/mmu/spte.h index f332b33bc817..6285c45fa56d 100644 --- a/arch/x86/kvm/mmu/spte.h +++ b/arch/x86/kvm/mmu/spte.h @@ -461,6 +461,23 @@ static inline bool is_mmu_writable_spte(u64 spte) return spte & shadow_mmu_writable_mask; } +/* + * Returns true if the access indiciated by @fault is allowed by the existing + * SPTE protections. Note, the caller is responsible for checking that the + * SPTE is a shadow-present, leaf SPTE (either before or after). + */ +static inline bool is_access_allowed(struct kvm_page_fault *fault, u64 spte) +{ + if (fault->exec) + return is_executable_pte(spte); + + if (fault->write) + return is_writable_pte(spte); + + /* Fault was on Read access */ + return spte & PT_PRESENT_MASK; +} + /* * If the MMU-writable flag is cleared, i.e. the SPTE is write-protected for * write-tracking, remote TLBs must be flushed, even if the SPTE was read-only, diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c index 4508d868f1cd..2f15e0e33903 100644 --- a/arch/x86/kvm/mmu/tdp_mmu.c +++ b/arch/x86/kvm/mmu/tdp_mmu.c @@ -985,6 +985,11 @@ static int tdp_mmu_map_handle_target_level(struct kvm_vcpu *vcpu, if (fault->prefetch && is_shadow_present_pte(iter->old_spte)) return RET_PF_SPURIOUS; + if (is_shadow_present_pte(iter->old_spte) && + is_access_allowed(fault, iter->old_spte) && + is_last_spte(iter->old_spte, iter->level)) + return RET_PF_SPURIOUS; + if (unlikely(!fault->slot)) new_spte = make_mmio_spte(vcpu, iter->gfn, ACC_ALL); else base-commit: 3522c419758ee8dca5a0e8753ee0070a22157bc1 --
