On Tue, Sep 22, 2026 at 01:25:19PM +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.
>
> Claim and release per-CPU slots with cmpxchg. Update cpu_dr7 with
> single-instruction per-CPU operations, and preserve hardware-first
> disable and hardware-last enable ordering. Add a per-CPU sequence number
> so interrupted DR7 writers and restore paths detect an NMI update and
> retry from the latest shadow state.
Bah, KGDB.. Aren't there far more problems with that thing?
> diff --git a/arch/x86/include/asm/debugreg.h b/arch/x86/include/asm/debugreg.h
> index 854d82b88ff4..515d2d313d0c 100644
> --- a/arch/x86/include/asm/debugreg.h
> +++ b/arch/x86/include/asm/debugreg.h
> @@ -18,6 +18,7 @@
> #define DR7_FIXED_1 0x00000400
>
> DECLARE_PER_CPU(unsigned long, cpu_dr7);
> +DECLARE_PER_CPU(unsigned int, cpu_dr7_seq);
Would it make sense to:
typedef struct {
unsigned long dr7;
unsigned int seq;
} dr7_save_t;
> #ifndef CONFIG_PARAVIRT_XXL
> /*
> @@ -125,40 +126,69 @@ static __always_inline bool hw_breakpoint_active(void)
>
> extern void hw_breakpoint_restore(void);
>
> -static __always_inline unsigned long local_db_save(void)
> +static __always_inline void local_db_save(unsigned long *dr7,
> + unsigned int *dr7_seq)
static __always_inline dr7_save_t local_db_save(void)
> {
> }
>
> -static __always_inline void local_db_restore(unsigned long dr7)
> +static __always_inline void local_db_restore(unsigned long dr7,
> + unsigned int dr7_seq)
static __always_inline void local_db_restore(dr7_save_t dr7)
> {
> }
>
> #ifdef CONFIG_CPU_SUP_AMD
> diff --git a/arch/x86/kernel/hw_breakpoint.c b/arch/x86/kernel/hw_breakpoint.c
> index 0473a5c95856..901323ae7d6a 100644
> --- a/arch/x86/kernel/hw_breakpoint.c
> +++ b/arch/x86/kernel/hw_breakpoint.c
> @@ -106,32 +108,25 @@ int arch_install_hw_breakpoint(struct perf_event *bp)
> + do {
> + seq = this_cpu_inc_return(cpu_dr7_seq);
> + this_cpu_write(cpu_debugreg[i], info->address);
> + barrier();
> + set_debugreg(info->address, i);
> + if (info->mask)
> + amd_set_dr_addr_mask(info->mask, i);
> + this_cpu_or(cpu_dr7, encode_dr7(i, info->len, info->type));
> + barrier();
> + set_debugreg(this_cpu_read(cpu_dr7) | DR7_FIXED_1, 7);
> + barrier();
> + } while (seq != this_cpu_read(cpu_dr7_seq));
>
> return 0;
> }
> @@ -149,36 +144,34 @@ void arch_uninstall_hw_breakpoint(struct perf_event *bp)
> + do {
> + seq = this_cpu_inc_return(cpu_dr7_seq);
> + dr7 = this_cpu_read(cpu_dr7);
You're inconsistent with the leading barrier().
> + dr7 &= ~__encode_dr7(i, info->len, info->type);
> + set_debugreg(dr7 | DR7_FIXED_1, 7);
> + if (info->mask)
> + amd_set_dr_addr_mask(0, i);
> + barrier();
> + this_cpu_and(cpu_dr7,
> + ~__encode_dr7(i, info->len, info->type));
> + barrier();
> + } while (seq != this_cpu_read(cpu_dr7_seq));
> +
> + WARN_ONCE(this_cpu_cmpxchg(bp_per_reg[i], bp, NULL) != bp,
> + "Can't release breakpoint slot");
> }
These loops should be far more similar. Note how the top one does:
this_cpu_or(cpu_dr7, encode_dr7(...));
set_debugreg(this_cpu_read(cpu_dr7) | ..., 7);
while the bottom one does:
dr7 &= ~encode_dr7(...)
set_debugreg(dr7 | ...);
this_cpu_and(cpu_dr7, ~encode_dr7(...));
Why can't they both have the same shape and only one encode_dr7()
instance?
> @@ -486,12 +480,18 @@ void flush_ptrace_hw_breakpoint(struct task_struct *tsk)
>
> void hw_breakpoint_restore(void)
> {
> + unsigned int seq;
> +
> + do {
> + seq = this_cpu_inc_return(cpu_dr7_seq);
no barrier().
> + set_debugreg(this_cpu_read(cpu_debugreg[0]), 0);
> + set_debugreg(this_cpu_read(cpu_debugreg[1]), 1);
> + set_debugreg(this_cpu_read(cpu_debugreg[2]), 2);
> + set_debugreg(this_cpu_read(cpu_debugreg[3]), 3);
> + set_debugreg(DR6_RESERVED, 6);
> + set_debugreg(this_cpu_read(cpu_dr7) | DR7_FIXED_1, 7);
> + barrier();
> + } while (seq != this_cpu_read(cpu_dr7_seq));
> }
> EXPORT_SYMBOL_FOR_KVM(hw_breakpoint_restore);
I really can't say I'm a fan of this. Is KGDB really a thing?
