Re: [PATCH 3/3] arm64/hw_breakpoint: Enable FEAT_Debugv8p9

[Mark Rutland <mark.rutland@arm.com>]

On Tue, Oct 29, 2024 at 01:06:38PM +0530, Anshuman Khandual wrote:
> On 10/28/24 18:17, Mark Rutland wrote:
> > On Wed, Oct 23, 2024 at 01:01:52PM +0530, Anshuman Khandual wrote:
> >> On 10/22/24 21:04, Mark Rutland wrote:

> >>> I assume this is intended to protect the bank in sequences like:
> >>>
> >>> 	MSR	MDSELR, <...>
> >>> 	ISB
> >>> 	MRS	<..._, BANKED_REGISTER
> >>
> >> Correct, it is protecting the above sequence.
> >>
> >>> ... but is theat suffucient for mutual exclusion against
> >>> exception handlers, or does that come from somewhere else?
> >>
> >> Looking at all existing use cases for breakpoint/watchpoints, it should
> >> be sufficient to protect against mutual exclusion. But thinking, do you
> >> have a particular exception handler scenario in mind where this might
> >> still be problematic ? Will keep looking into it.
> > 
> > Where does the mutual exclusion come from for the existing sequences?
> 
> Bank selection followed by indexed read/write, inherently requires mutual
> exclusion (ensuring that both these steps executed together) in order to
> prevent read/write into wrong registers. That being said, HW breakpoints
> get used in multiple different places such as perf, ptrace, debug monitor
> based single stepping etc calling platform functions which operate on the
> HW breakpoint registers here.

Yes; that's *why* I'm asking. 

> preempt_disable()/enable() sequence in the very last leaf level helpers
> such as [read|write]_wb_reg(), will ensure required mutual exclusion.

I do not believe that this assertion is correct.

I specifically gave the example of mutual exclusion against exception
handlers, and preempt_disable() ... preempt_disable() does not prevent
exceptions being taken, so disabling preemption *cannot* be sufficient
to provide mutual exclusion against exception handlers.

What prevents a race with an exception handler? e.g. 

* Does the structure of the code prevent that somehow?

* What context(s) does this code execute in?
  - Are debug exceptions always masked?
  - Do we disable breakpoints/watchpoints around (some) manipulation of
    the relevant registers?

> > We should be able to descrive should be able to describe that in the
> > commit message or in a comment somewhere (or better, with some
> > assertions that get tested).
> 
> Planning to add a comment - something like this both for read and write
> helpers.
>        /*
>         * Bank selection in MDSELR_EL1, followed by indexed read from
>         * [break|watch]point registers cannot be interrupted, as that
>         * might cause misread from wrong targets. Hence this requires
>         * mutual exclusion via preventing any preemption.
>         */

As above, I do not believe this is correct. At minimum, disabling
preemption is not the full story here.

> But regarding adding assertions, could you give some more details and
> it will be great to have some relevant examples as well.

I've given some suggestions above. Please go and read the code and
figure this out.

> > For example, what prevents watchpoint_handler() from firing in the
> > middle of arch_install_hw_breakpoint() or
> > arch_uninstall_hw_breakpoint()?
> 
> If perf is the only user, watchpoint_handler() will not get triggered
> without watchpoints being installed via arch_install_hw_breakpoint().
> Similarly once they get uninstalled via arch_uninstall_hw_breakpoint()
> there will not be active watchpoints to trigger the handler. Although
> there are other users (ptrace, debug monitor etc) besides perf which
> could also be active simultaneously and race with each other ? TBH, I
> am not sure.

Please go and read the code and figure this out.

> > Is the existing code correct?
> 
> I have not tested the concurrency aspects of the HW breakpoints enough
> to be able to answer that question. But if there is a particular concern
> here, happy to look into that.
> 
> But wondering how does this new bank indexed read/write mechanism (after
> taking care of the mutual exclusion in the leaf level helpers such as
> [read| write]_wb_reg()) still makes the existing concurrency situation
> worse off than earlier ?

Please go and read the code and figure this out.

Mark.