On Thu, 6 Aug 2026 09:07:35 +0900 Masami Hiramatsu (Google) <[email protected]> wrote:
> On Tue, 4 Aug 2026 01:07:23 +0200 > Peter Zijlstra <[email protected]> wrote: > > > On Tue, Aug 04, 2026 at 07:55:17AM +0900, Masami Hiramatsu wrote: > > > On Mon, 3 Aug 2026 08:57:44 +0200 > > > Peter Zijlstra <[email protected]> wrote: > > > > > > > On Sun, Aug 02, 2026 at 05:18:37PM +0900, Masami Hiramatsu (Google) > > > > wrote: > > > > > From: Jinchao Wang <[email protected]> > > > > > > > > > > Hardware breakpoint installation and removal run with IRQs disabled, > > > > > but > > > > > an NMI can still enter the same code through KGDB. The interrupted > > > > > operation and the NMI can consequently claim the same slot or > > > > > overwrite > > > > > each other's DR7 state. > > > > > > > > Is KGDB really the only way to trip this? Mostly I think we let KGDB > > > > have the pieces if it does something 'funny'. > > > > > > Hmm, I think even if so, this detection is better to be handled in > > > hw_breakpoint layer. And I plan to use this from kprobe events, > > > which is also a kind of NMI. > > > > > > > > > > > > diff --git a/arch/x86/kernel/hw_breakpoint.c > > > > > b/arch/x86/kernel/hw_breakpoint.c > > > > > index f846c15f21ca..9ef24b55737f 100644 > > > > > --- a/arch/x86/kernel/hw_breakpoint.c > > > > > +++ b/arch/x86/kernel/hw_breakpoint.c > > > > > @@ -40,6 +40,9 @@ > > > > > DEFINE_PER_CPU(unsigned long, cpu_dr7); > > > > > EXPORT_PER_CPU_SYMBOL(cpu_dr7); > > > > > > > > > > +/* Sequence number of the per-CPU DR7 state. */ > > > > > +DEFINE_PER_CPU(unsigned int, cpu_dr7_seq); This is actual sequence number. > > > > > > > > > diff --git a/arch/x86/kernel/nmi.c b/arch/x86/kernel/nmi.c > > > > > index 3c9f60d6ca5a..f55a0cbd5927 100644 > > > > > --- a/arch/x86/kernel/nmi.c > > > > > +++ b/arch/x86/kernel/nmi.c > > > > > @@ -532,10 +532,13 @@ enum nmi_states { > > > > > static DEFINE_PER_CPU(enum nmi_states, nmi_state); > > > > > static DEFINE_PER_CPU(unsigned long, nmi_cr2); > > > > > static DEFINE_PER_CPU(unsigned long, nmi_dr7); > > > > > +static DEFINE_PER_CPU(unsigned int, nmi_dr7_seq); and this is a backup. > > > > > > > > This is weird, why have two distinct sequence numbers for dr7? > > > > > > nmi_dr7_seq is for sequence number of operation, which is for > > > detecting dr7 overwrite in NMI. > > > nmi_dr7 is for saving the DR7. > > > > I mean cpu_dr7_seq and nmi_dr7_seq. > > Oops, Indeed. Let me fix it. Thanks! BTW, do we really need nmi_dr7 as a per-cpu variable? Since this patch changes local_db_save()/restore() to handle both DR7 and its sequence number, we need to save/restore both. I thought it is used for handling nested NMI but in exc_nmi, we have this gate check before using the nmi_dr7. if (this_cpu_read(nmi_state) != NMI_NOT_RUNNING) { this_cpu_write(nmi_state, NMI_LATCHED); return; } Thus the nested NMI is handled serial, not stacked. So we can use a local variable instead of per-cpu variable. Thank you, > > > > -- > Masami Hiramatsu (Google) <[email protected]> -- Masami Hiramatsu (Google) <[email protected]>
