On Wed, 9 Aug 2023, Eric Wheeler wrote:
> On Tue, 8 Aug 2023, Sean Christopherson wrote:
> > On Tue, Aug 08, 2023, Amaan Cheval wrote:
> > > Hey Sean,
> > >
> > > > If NUMA balancing is going nuclear and constantly zapping PTEs, the resulting
> > > > mmu_notifier events could theoretically stall a vCPU indefinitely. The reason I
> > > > dislike NUMA balancing is that it's all too easy to end up with subtle bugs
> > > > and/or misconfigured setups where the NUMA balancing logic zaps PTEs/SPTEs without
> > > > actuablly being able to move the page in the end, i.e. it's (IMO) too easy for
> > > > NUMA balancing to get false positives when determining whether or not to try and
> > > > migrate a page.
> > >
> > > What are some situations where it might not be able to move the page in the end?
> >
> > There's a pretty big list, see the "failure" paths of do_numa_page() and
> > migrate_misplaced_page().
> >
> > > > That said, it's definitely very unexpected that NUMA balancing would be zapping
> > > > SPTEs to the point where a vCPU can't make forward progress. It's theoretically
> > > > possible that that's what's happening, but quite unlikely, especially since it
> > > > sounds like you're seeing issues even with NUMA balancing disabled.
Brak indicated that they've seen this as early as v5.19. IIRC, Hunter
said that v5.15 is working fine, so I went through the >v5.15 and <v5.19
commit logs for KVM that appear to be related to EPT. Of course if the
problem is outside of KVM, then this is moot, but maybe these are worth
a second look.
Sean, could any of these commits cause or hint at the problem?
54275f74c KVM: x86/mmu: Don't attempt fast page fault just because EPT is in use
- this mentions !PRESENT related to faulting out of mmu_lock.
ec283cb1d KVM: x86/mmu: remove ept_ad field
- looks like a simple patch, but could there be a reason that
this is somehow invalid in corner cases? Here is the relevant
diff snippet:
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -5007,7 +5007,6 @@ void kvm_init_shadow_ept_mmu(struct kvm_vcpu *vcpu, bool execonly,
context->shadow_root_level = level;
- context->ept_ad = accessed_dirty;
+++ b/arch/x86/kvm/mmu/paging_tmpl.h
- #define PT_HAVE_ACCESSED_DIRTY(mmu) ((mmu)->ept_ad)
+ #define PT_HAVE_ACCESSED_DIRTY(mmu) (!(mmu)->cpu_role.base.ad_disabled)
ca2a7c22a KVM: x86/mmu: Derive EPT violation RWX bits from EPTE RWX bits
- "No functional change intended" but it mentions EPT
violations. Could something unintentional have happened here?
4f4aa80e3 KVM: X86: Handle implicit supervisor access with SMAP
- This is a small change, but maybe it would be worth a quick review
5b22bbe71 KVM: X86: Change the type of access u32 to u64
- This is just a datatype change in 5.17-rc3, probably not it.
-Eric
> > >
> > > Yep, we're definitely seeing the issue occur even with numa_balancing
> > > enabled, but the likelihood of it occurring has significantly dropped since
> > > we've disabled numa_balancing.
> >
> > So the fact that this occurs with NUMA balancing disabled means the problem likely
> > isn't with NUMA balancing itself. NUMA balancing probably exposes some underlying
> > issue due to it generating a near-constant stream of mmu_notifier invalidation.
> >
> > > > More likely is that there is a bug somewhere that results in the mmu_notifier
> > > > event refcount staying incorrectly eleveated, but that type of bug shouldn't follow
> > > > the VM across a live migration...
> > >
> > > Good news! We managed to live migrate a guest and that did "fix it".
>
> Does the VM make progress even if is migrated to a kernel that presents
> the bug?
>
> What was kernel version being migrated from and to?
>
> For example, was it from a >5.19 kernel to something earlier than 5.19?
>
> For example, if the hung VM remains stuck after migrating to a >5.19
> kernel but _not_ to a <5.19 kernel, then maybe bisect is an option.
>
>
> --
> Eric Wheeler
>
>
>
>
> > ...
> >
> > > A colleague also modified a host kernel with KFI (Kernel Function
> > > Instrumentation) and wrote a kernel module to intercept the vmexit handler,
> > > handle_ept_violation, and does an EPT walk for each pfn, compared against
> > > /proc/iomem.
> > >
> > > Assuming the EPT walking code is correct, we see this surprising result of a
> > > PDPTE's pfn=0:
> >
> > Not surprising. The entire EPTE is zero, i.e. has been zapped by KVM. This is
> > exactly what is expected.
> >
> > > Does this seem to indicate an mmu_notifier refcount issue to you, given that
> > > migration did fix it? Any way to verify?
> >
> > It doesn't move the needle either way, it just confirms what we already know: the
> > vCPU is repeatedly taking !PRESENT faults. The unexpected part is that KVM never
> > "fixes" the fault and never outright fails.
> >
> > > We haven't found any guests with `softlockup_panic=1` yet, and since we can't
> > > reproduce the issue on command ourselves yet, we might have to wait a bit - but
> > > I imagine that the fact that live migration "fixed" the locked up guest confirms
> > > that the other guests that didn't get "fixed" were likely softlocked from the
> > > CPU stalling?
> >
> > Yes.
> >
> > > If you have any suggestions on how modifying the host kernel (and then migrating
> > > a locked up guest to it) or eBPF programs that might help illuminate the issue
> > > further, let me know!
> > >
> > > Thanks for all your help so far!
> >
> > Since it sounds like you can test with a custom kernel, try running with this
> > patch and then enable the kvm_page_fault tracepoint when a vCPU gets stuck. The
> > below expands said tracepoint to capture information about mmu_notifiers and
> > memslots generation. With luck, it will reveal a smoking gun.
> >
> > ---
> > arch/x86/kvm/mmu/mmu.c | 10 ----------
> > arch/x86/kvm/mmu/mmu_internal.h | 2 ++
> > arch/x86/kvm/mmu/tdp_mmu.h | 10 ++++++++++
> > arch/x86/kvm/trace.h | 28 ++++++++++++++++++++++++++--
> > 4 files changed, 38 insertions(+), 12 deletions(-)
> >
> > diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> > index 9e4cd8b4a202..122bfc884293 100644
> > --- a/arch/x86/kvm/mmu/mmu.c
> > +++ b/arch/x86/kvm/mmu/mmu.c
> > @@ -2006,16 +2006,6 @@ static bool kvm_mmu_remote_flush_or_zap(struct kvm *kvm,
> > return true;
> > }
> >
> > -static bool is_obsolete_sp(struct kvm *kvm, struct kvm_mmu_page *sp)
> > -{
> > - if (sp->role.invalid)
> > - return true;
> > -
> > - /* TDP MMU pages do not use the MMU generation. */
> > - return !is_tdp_mmu_page(sp) &&
> > - unlikely(sp->mmu_valid_gen != kvm->arch.mmu_valid_gen);
> > -}
> > -
> > struct mmu_page_path {
> > struct kvm_mmu_page *parent[PT64_ROOT_MAX_LEVEL];
> > unsigned int idx[PT64_ROOT_MAX_LEVEL];
> > diff --git a/arch/x86/kvm/mmu/mmu_internal.h b/arch/x86/kvm/mmu/mmu_internal.h
> > index f1ef670058e5..cf7ba0abaa8f 100644
> > --- a/arch/x86/kvm/mmu/mmu_internal.h
> > +++ b/arch/x86/kvm/mmu/mmu_internal.h
> > @@ -6,6 +6,8 @@
> > #include <linux/kvm_host.h>
> > #include <asm/kvm_host.h>
> >
> > +#include "mmu.h"
> > +
> > #ifdef CONFIG_KVM_PROVE_MMU
> > #define KVM_MMU_WARN_ON(x) WARN_ON_ONCE(x)
> > #else
> > diff --git a/arch/x86/kvm/mmu/tdp_mmu.h b/arch/x86/kvm/mmu/tdp_mmu.h
> > index 0a63b1afabd3..a0d7c8acf78f 100644
> > --- a/arch/x86/kvm/mmu/tdp_mmu.h
> > +++ b/arch/x86/kvm/mmu/tdp_mmu.h
> > @@ -76,4 +76,14 @@ static inline bool is_tdp_mmu_page(struct kvm_mmu_page *sp) { return sp->tdp_mmu
> > static inline bool is_tdp_mmu_page(struct kvm_mmu_page *sp) { return false; }
> > #endif
> >
> > +static inline bool is_obsolete_sp(struct kvm *kvm, struct kvm_mmu_page *sp)
> > +{
> > + if (sp->role.invalid)
> > + return true;
> > +
> > + /* TDP MMU pages do not use the MMU generation. */
> > + return !is_tdp_mmu_page(sp) &&
> > + unlikely(sp->mmu_valid_gen != kvm->arch.mmu_valid_gen);
> > +}
> > +
> > #endif /* __KVM_X86_MMU_TDP_MMU_H */
> > diff --git a/arch/x86/kvm/trace.h b/arch/x86/kvm/trace.h
> > index 83843379813e..ff4a384ab03a 100644
> > --- a/arch/x86/kvm/trace.h
> > +++ b/arch/x86/kvm/trace.h
> > @@ -8,6 +8,8 @@
> > #include <asm/clocksource.h>
> > #include <asm/pvclock-abi.h>
> >
> > +#include "mmu/tdp_mmu.h"
> > +
> > #undef TRACE_SYSTEM
> > #define TRACE_SYSTEM kvm
> >
> > @@ -405,6 +407,13 @@ TRACE_EVENT(kvm_page_fault,
> > __field( unsigned long, guest_rip )
> > __field( u64, fault_address )
> > __field( u64, error_code )
> > + __field( unsigned long, mmu_invalidate_seq)
> > + __field( long, mmu_invalidate_in_progress)
> > + __field( unsigned long, mmu_invalidate_range_start)
> > + __field( unsigned long, mmu_invalidate_range_end)
> > + __field( bool, root_is_valid)
> > + __field( bool, root_has_sp)
> > + __field( bool, root_is_obsolete)
> > ),
> >
> > TP_fast_assign(
> > @@ -412,11 +421,26 @@ TRACE_EVENT(kvm_page_fault,
> > __entry->guest_rip = kvm_rip_read(vcpu);
> > __entry->fault_address = fault_address;
> > __entry->error_code = error_code;
> > + __entry->mmu_invalidate_seq = vcpu->kvm->mmu_invalidate_seq;
> > + __entry->mmu_invalidate_in_progress = vcpu->kvm->mmu_invalidate_in_progress;
> > + __entry->mmu_invalidate_range_start = vcpu->kvm->mmu_invalidate_range_start;
> > + __entry->mmu_invalidate_range_end = vcpu->kvm->mmu_invalidate_range_end;
> > + __entry->root_is_valid = VALID_PAGE(vcpu->arch.mmu->root.hpa);
> > + __entry->root_has_sp = VALID_PAGE(vcpu->arch.mmu->root.hpa) &&
> > + to_shadow_page(vcpu->arch.mmu->root.hpa);
> > + __entry->root_is_obsolete = VALID_PAGE(vcpu->arch.mmu->root.hpa) &&
> > + to_shadow_page(vcpu->arch.mmu->root.hpa) &&
> > + is_obsolete_sp(vcpu->kvm, to_shadow_page(vcpu->arch.mmu->root.hpa));
> > ),
> >
> > - TP_printk("vcpu %u rip 0x%lx address 0x%016llx error_code 0x%llx",
> > + TP_printk("vcpu %u rip 0x%lx address 0x%016llx error_code 0x%llx, seq = 0x%lx, in_prog = 0x%lx, start = 0x%lx, end = 0x%lx, root = %s",
> > __entry->vcpu_id, __entry->guest_rip,
> > - __entry->fault_address, __entry->error_code)
> > + __entry->fault_address, __entry->error_code,
> > + __entry->mmu_invalidate_seq, __entry->mmu_invalidate_in_progress,
> > + __entry->mmu_invalidate_range_start, __entry->mmu_invalidate_range_end,
> > + !__entry->root_is_valid ? "invalid" :
> > + !__entry->root_has_sp ? "no shadow page" :
> > + __entry->root_is_obsolete ? "obsolete" : "fresh")
> > );
> >
> > /*
> >
> > base-commit: 240f736891887939571854bd6d734b6c9291f22e
> > --
> >
> >
>
