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 :-)
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).
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.
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.
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.
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.
