On Wed, 23 Sep 2026 08:32:08 -0700
Sean Christopherson <[email protected]> wrote:
> On Wed, Sep 23, 2026, Peter Zijlstra wrote:
> > On Tue, Sep 22, 2026 at 01:25:07PM +0900, Masami Hiramatsu (Google) wrote:
> > > diff --git a/arch/x86/kernel/hw_breakpoint.c
> > > b/arch/x86/kernel/hw_breakpoint.c
> > > index f846c15f21ca..0473a5c95856 100644
> > > --- a/arch/x86/kernel/hw_breakpoint.c
> > > +++ b/arch/x86/kernel/hw_breakpoint.c
> > > @@ -102,6 +102,9 @@ int arch_install_hw_breakpoint(struct perf_event *bp)
> > >
> > > lockdep_assert_irqs_disabled();
> > >
> > > + if (perf_guest_in_guest())
> >
> > That naming is hilariously bad :-)
Agreed.
>
> Indeed. It's also misleading and confusing, because it's really checking for
> "in KVM's core run loop", whereas the goal of perf_guest_state() returns a
> non-zero value if and only if the IRQ/NMI really did occur while the guest was
> active (I say "the goal" because it's imperfect due to architectural
> limitations,
> but the goal is purely to detect guest PMIs).
Yeah, I see.
>
> Ugh, and routing this through perf was my suggestion[*]:
>
> : If we decide this is how to fix arch_install_hw_breakpoint() clobbering
> DRs from
> : NMI context, I would rather have more generic flag to tell perf that KVM
> is about
> : to enter the guest, e.g. so that we don't have to separately solve the
> same problem
> : for other perf events:
>
> After seeing the code, that feels like a pretty stupid suggestion. Though in
> my
> defense, I was thinking of a per-CPU flag as opposed to a new callback.
> Anyways,
> I don't think we should key off IN_GUEST_MODE and EXITING_GUEST_MODE because
> they
> are very much an arch-specific, KVM-internal concept.
OK.
>
> What I was trying to say by "more generic flag" is that I would prefer not to
> have
> a super specific cpu_dr_in_guest. I'm not opposed to have a dedicated flag
> (though
> if we can avoid one, that would be lovely). The biggest problem I see with
> adding
> a generic flag is how to make it precise enough to be useful, without end up
> with a
> confusing name. E.g. "guest_state_loaded" is terrible because KVM keeps some
> guest
> state loaded even when the task is scheduled out.
Something like "cpu_in_guest_transition"?
>
> And to Peter's point below, is arch_install_hw_breakpoint() even the right
> place
> to handle this? It seems like KGDB itself should be handling this, at which
> point
> maybe we just do something like this? Then we can provide nop stubs when KGDB
> support is disabled.
Hmm, so instead of kgdb specific flag, add a per-cpu flag for entering/exiting
guest, and use it for kgdb and other users like wprobe? (it may work similar to
in_nmi() check.)
>
> diff --git arch/x86/kvm/x86.c arch/x86/kvm/x86.c
> index 1705e7be46ec..42fa4dc44cbc 100644
> --- arch/x86/kvm/x86.c
> +++ arch/x86/kvm/x86.c
> @@ -8273,6 +8273,8 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
>
> kvm_load_xfeatures(vcpu, true);
>
> + kgdb_arch_enter_guest();
> +
> if (unlikely(vcpu->arch.switch_db_regs &&
> !(vcpu->arch.switch_db_regs &
> KVM_DEBUGREG_AUTO_SWITCH))) {
> set_debugreg(DR7_FIXED_1, 7);
> @@ -8365,6 +8367,8 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
> if (hw_breakpoint_active())
> hw_breakpoint_restore();
>
> + kgdb_arch_exit_guest();
> +
> vcpu->arch.last_vmentry_cpu = vcpu->cpu;
> vcpu->arch.last_guest_tsc = kvm_read_l1_tsc(vcpu, rdtsc());
>
> [*] https://lore.kernel.org/all/[email protected]
>
> > > + return -EBUSY;
> > > +
> > > for (i = 0; i < HBP_NUM; i++) {
> > > struct perf_event **slot = this_cpu_ptr(&bp_per_reg[i]);
> > >
> >
> > Note how the other -EBUSY return is a WARN. Why is silently not doing
> > anything not a WARN in this case?
>
> Probably because the WARN would trigger anytime KGDB's NMI craziness happens
> to
> hit a vCPU, i.e. isn't a kernel bug.
Yeah, this may confuse the caller. OK, let me change it to check
the state flag in caller side.
Thank you,
--
Masami Hiramatsu (Google) <[email protected]>
