Re: [Patch 1/1] PPC64-HWBKPT: Implement hw-breakpoints for PPC64

["K.Prasad" <prasad@linux.vnet.ibm.com>]

On Sun, Dec 13, 2009 at 04:56:48PM -0800, Roland McGrath wrote:
> I can't see anything you've done to keep this use of MSR_SE in the
> user-mode register state from interfering with user_enable_single_step().
> It looks to me like you'd swallow the normal step indications.
> 

Yes, it does unset MSR_SE bit in single_step_dabr_instruction()
irrespective of whether it was previously enabled through
user_enable_single_step(). This could be mitigated with the use of a
separate flag which can be used to conditionally unset MSR_SE, however
given further concerns about pre-emption (as expressed by you below),
I'm afraid of substantial revamp of the user-space semantics.

> Likewise I'm not very clear on the interaction with kprobes, kgdb,
> or whatnot for kernel-mode cases.  But I'll leave those concerns to
> others, since I know more about the user-mode situations.
>

Kprobes has been tested to work simultaneously with hw-breakpoints. KGDB
has not been ported yet to use the hw-breakpoint interfaces (KGDB had
issues in it, that prevented it from being tested during our
development...though its maintainer has begun showing interest
recently).

Xmon was (and I believe is still) in a state where data breakpoints did
not work. It needs to be ported too, to benefit from the hw-breakpoint
interfaces.
 
> Back to the user-mode case, is it really reasonable to disable
> preemption in hw_breakpoint_handler and leave it so across returning
> to user mode?  (Is that even possible?  I thought user mode was
> always preemptible.)  That is done very casually with little comment
> in hw_breakpoint_handler and single_step_dabr_instruction, but it
> seems like an extremely deep and magical thing that merits more
> explanation.  I guess the need for it has to do with the per_cpu
> variable you're using, but the whole situation is not very clear on
> first reading.  Even for kernel mode, what does this mean when the
> stepped instruction does a page fault?
> 

I must admit that the issue of pre-emption should have been given more
thought. Suppose there's a stream of user-space instructions "i1, i2, i3,
.....i<n>" and if 'i2' instruction can cause a hw-breakpoint exception,
then there exists a small window between i1 and i2 where pre-emption is
disabled (while a schedule operation could have taken place otherwise).

Disabling pre-emption is necessary to ensure that hw-breakpoints are
enabled immediately after the causative instruction has finished
execution (the control flow may go astray if pre-emption occurs between
i1 and i2). The root cause of this behaviour is a combination of
'trigger-before-execute' behaviour (for data-exceptions) and a desire
for 'continuous' exceptions (as opposed to one-shot behaviour seen in
ptrace). The per-cpu variable 'last_hit_bp' just helps identify a
single-step exception resulting from a hbp_handler vs other sources.

Resorting to one-shot behaviour (which is the easiest workaround
available) will break the desired uniformity in behaviour for
hw-breakpoint interfaces - say every register_user_<> interface must be
accompanied by a unregister_<> interface, etc. Post perf-events'
integration, ensuring a one-shot behaviour might also have its own
bunch of undesirable consequences (such as circular locks), that must be
overcome.

Unless I see a way to re-instate the breakpoints (surviving a
pre-emption), I will send out a new patch that resorts to a one-shot
behaviour for user-space (kernel-space is fine though).

Thank you for the insightful comments!

K.Prasad