Re: [PATCH v17 02/13] perf/x86, KVM: Prevent host debug register leak into guest OS on NMI

[Masami Hiramatsu (Google) <mhiramat@kernel.org>]

On Wed, 23 Sep 2026 08:32:08 -0700
Sean Christopherson <seanjc@google.com> 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/aqgGRKOE138ePqmX@google.com
> 
> > > +		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) <mhiramat@kernel.org>