public inbox for linux-arm-kernel@lists.infradead.org
 help / color / mirror / Atom feed
From: Mark Rutland <mark.rutland@arm.com>
To: Rob Herring <robh@kernel.org>
Cc: Anshuman Khandual <anshuman.khandual@arm.com>,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, Jonathan Corbet <corbet@lwn.net>,
	Marc Zyngier <maz@kernel.org>,
	Oliver Upton <oliver.upton@linux.dev>,
	James Morse <james.morse@arm.com>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will@kernel.org>, Mark Brown <broonie@kernel.org>,
	kvmarm@lists.linux.dev
Subject: Re: [PATCH V3 7/7] arm64/hw_breakpoint: Enable FEAT_Debugv8p9
Date: Mon, 13 Apr 2026 09:48:24 +0100	[thread overview]
Message-ID: <adyt2FYz0Cx5r-To@J2N7QTR9R3> (raw)
In-Reply-To: <CAL_Jsq+=3CKrUDcdgSHFaSGgvnh9ZxTGEBTRLkwrYgzQ=frOdg@mail.gmail.com>

On Fri, Apr 10, 2026 at 02:55:55PM -0500, Rob Herring wrote:
> On Thu, Apr 9, 2026 at 5:52 AM Mark Rutland <mark.rutland@arm.com> wrote:
> > Both breakpoint and watchpoint exceptions are synchronous, meanning that
> > they can only be taken from the specific instruction that triggered
> > them.  However, updates to the watchpoint control registers *do* need a
> > context synchronization event before they're guarnateed to take effect.
> >
> > For a sequence:
> >
> >     // Initially:
> >     // - MDSCR, MDCR, DAIF.D permit debug exceptions at CurrentEL
> >     // - No watchpoints enabled
> >
> >     0x000:  LDR <val>, [<addr>]
> >     0x004:  MSR DBGWVR<n>_EL1, <addr>
> >     0x008:  MSR DBGWCR<n>_EL1, <configure_and_enable>
> >     0x010:  LDR <val>, [<addr>]
> >     0x014:  ISB
> >     0x018:  LDR <val>, [<addr>]
> >
> > ... we know:
> >
> >     (a) The LDR at 0x000 *cannot* trigger the watchpoint.
> 
> Why not?

Because:

(a) As noted above, both breakpoint and watchpoint exceptions are
    synchronous. They can only be taken from the specific *instruction*
    that triggered them, and the exception must be consistent with no
    subsequent instructions having been executed.

    In ARM DDI 0487 M.a.a, see:

    - Section D1.4 ("Exceptions"), and in particular RFQHGR, RTNVSL.
    - Section D2.8 ("Breakpoint exceptions")
    - Section D2.9 ("Watchpoint exceptions")

(b) Generally, writes to system registers cannot affect earlier
    instructions.

    In ARM DDI 0487 M.a.a, see: Section D24.1.2.2 ("Synchronization
    requirements for AArch64 System registers").

    Note in particular the statement: "Direct writes to System registers
    are not allowed to affect any instructions appearing in program
    order before the direct write."

> Can't the LDR complete after the MSR?

The memory effects of the LDR at 0x000 could complete after the MSRs at
0x004 and 0x008 are executed.

However, any watchpoints/breakpoint triggered by the LDR at 0x000 must
be taken *before* the next instruction (i.e. the MSR at 0x004) can be
architecturally executed, and any such execption must be consistent with
th PE state prior to that next instruction being executed.

Note that this doesn't forbid the PE from speculating the MSR and other
subsequent instructions; it just has to maintain program order (so later
instructions can't affect earlier instructions), and must be able to
discard anything that was speculated past taking an exception.

> Is ordering ensured between those? Surely the watchpoint triggers on
> completion of the load and that wouldn't stall the PE from doing the
> MSR(s)?

Hopefully the above covers these concerns?

> 
> >     (b) The LDR at 0x010 *might* trigger the matchpoint.
> >     (c) The LDR at 0x018 *must* trigger the watchpoint.
> >
> > For C code, we can enforce this order with barrier(), e.g.
> >
> >         val = *addr;
> >         barrier();
> >         write_sysreg(addr, DBGWVR<n>_EL1);
> >         write_sysreg(configure_and_enable, DBGWCR<n>_EL1);
> >         isb();
> >
> > ... where the compiler cannot re-order the memory access (or
> > write_sysreg(), or isb()) across the barrier(), and as isb() has a
> > memory clobber, the same is true for isb().
> >
> > Likewise, for the inverse sequence:
> >
> >     // Initially:
> >     // - MDSCR, MDCR, DAIF.D permit debug exceptions at CurrentEL
> >     // - Watchpoint configured and enabled for <addr>
> >
> >     0x100:  LDR <val>, [<addr>]
> >     0x104:  MSR DBGWCR<n>_EL1, <disable>
> >     0x108:  LDR <val>, [<addr>]
> >     0x110:  ISB
> >     0x114:  LDR <val>, [<addr>]
> >
> > ... we know:
> >
> >     (a) The LDR at 0x100 *must* trigger the watchpoint.
> >     (b) The LDR at 0x108 *might* trigger the watchpoint.
> >     (c) The LDR at 0x114 *cannot* trigger the watchpoint.
> >
> > > Any guidance on the flavor of dsb here? (And is there any guarantee
> > > that the access is visible to the watchpoint h/w after a dsb
> > > completes?)
> >
> > Hopefully the above was sufficient?
> >
> > As mentioned above, I think we have a latent issue where we can take a
> > breakpoint or watchpoint under arch_uninstall_hw_breakpoint(), where we
> > have:
> >
> >     arch_uninstall_hw_breakpoint(bp) {
> >         ...
> >         hw_breakpoint_control(bp, HW_BREAKPOINT_UNINSTALL) {
> >             ...
> >             hw_breakpoint_slot_setup(slots, max_slots, bp, HW_BREAKPOINT_UNINSTALL) {
> >                  ...
> >                  *slot = NULL;
> >                  ...
> >             }
> >             ...
> >             write_wb_reg(ctrl_reg, i, 0) {
> >                 ...
> >                 write_sysreg(0, ...);
> >                 isb();
> >                 ...
> >             }
> >         }
> >     }
> >
> > The HW breakpoint/watchpoint associated with 'bp' could be triggered
> > between setting '*slot' to NULL and the ISB. If that happens, then
> > do_breakpoint() won't find 'bp', and will return *without* disabling the
> > HW breakpoint or attempting to step.
> >
> > If that first exception was taken *before* the MSR in write_sysreg(),
> > then nothing has changed, and the breakpoint/watchpoint will then be
> > taken again ad infinitum.
> >
> > If that first exception was taken *after* the MSR in write_sysreg(), the
> > context synchronization provided by exception entry/return will prevent
> > it from being taken again.
> >
> > Building v6.19 and testing (with pseudo-NMI enabled):
> >
> > | #  grep write_wb_reg /proc/kallsyms
> > | ffff80008004b980 t write_wb_reg
> > | # ./perf-6.19 stat -a -C 1 -e 'mem:0xffff80008004b980/4:xk' true
> 
> I'll give it a try with v4, but that should be prevented with my
> changes to exclude kprobe regions.

That'll work for breakpoints specifically, but not for watchpoints, and
I think for that we either have to dynamically disable
watchpoint/breakpoint exceptions (e.g. via DAIF.D), or write the code to
be race-aware. I strongly suspect that the latter will be simpler.

I think we can solve that with some wider cleanup, e.g. having
arch_uninstall_hw_breakpoint() disable the breakpoint/watchpoint
*before* setting its slot to NULL, but we'll need to take some care
(e.g. to save/restore MDSELR).

I strongly suspect that we can defer implementing support for EMBWE
until we've done a more general cleanup, but I'll need to go do some
reading first.

Mark.


  reply	other threads:[~2026-04-13  8:48 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-12-16  4:08 [PATCH V3 0/7] arm64/hw_breakpoint: Enable FEAT_Debugv8p9 Anshuman Khandual
2024-12-16  4:08 ` [PATCH V3 1/7] arm64/sysreg: Update register fields for ID_AA64MMFR0_EL1 Anshuman Khandual
2024-12-16  4:08 ` [PATCH V3 2/7] arm64/sysreg: Add register fields for MDSELR_EL1 Anshuman Khandual
2024-12-16  4:08 ` [PATCH V3 3/7] arm64/sysreg: Add register fields for HDFGRTR2_EL2 Anshuman Khandual
2024-12-16  4:08 ` [PATCH V3 4/7] arm64/sysreg: Add register fields for HDFGWTR2_EL2 Anshuman Khandual
2024-12-16  4:08 ` [PATCH V3 5/7] arm64/cpufeature: Add field details for ID_AA64DFR1_EL1 register Anshuman Khandual
2024-12-16  4:08 ` [PATCH V3 6/7] arm64/boot: Enable EL2 requirements for FEAT_Debugv8p9 Anshuman Khandual
2024-12-16 23:42   ` Rob Herring
2024-12-17  8:48     ` Anshuman Khandual
2024-12-17 17:53       ` Rob Herring
2024-12-18  5:25         ` Anshuman Khandual
2024-12-18  9:10         ` Marc Zyngier
2024-12-18 13:15           ` Mark Brown
2024-12-16  4:08 ` [PATCH V3 7/7] arm64/hw_breakpoint: Enable FEAT_Debugv8p9 Anshuman Khandual
2024-12-16 10:58   ` Mark Rutland
2026-03-31 22:58     ` Rob Herring
2026-04-02 10:37       ` Mark Rutland
2026-04-02 17:46         ` Rob Herring
2026-04-09 10:52           ` Mark Rutland
2026-04-10 19:55             ` Rob Herring
2026-04-13  8:48               ` Mark Rutland [this message]
2026-03-31 23:02   ` Rob Herring

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=adyt2FYz0Cx5r-To@J2N7QTR9R3 \
    --to=mark.rutland@arm.com \
    --cc=anshuman.khandual@arm.com \
    --cc=broonie@kernel.org \
    --cc=catalin.marinas@arm.com \
    --cc=corbet@lwn.net \
    --cc=james.morse@arm.com \
    --cc=kvmarm@lists.linux.dev \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=maz@kernel.org \
    --cc=oliver.upton@linux.dev \
    --cc=robh@kernel.org \
    --cc=suzuki.poulose@arm.com \
    --cc=will@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox