[Patch 0/5] PPC64-HWBKPT: Hardware Breakpoint interfaces - ver XXII

[Patch 0/5] PPC64-HWBKPT: Hardware Breakpoint interfaces - ver XXII

[K.Prasad]

Hi All,
	Please find a new set of patches that have the following changes.

Changelog - ver XXII
--------------------
(Version XXI: linuxppc-dev ref:20100525091314.GA29003@in.ibm.com)
- Extraneous breakpoint exceptions are now properly handled; causative
  instruction will be single-stepped and debug register values restored.
- Restoration of breakpoints during signal handling through thread_change_pc()
  had defects which are now fixed.
- Breakpoints are flushed through flush_ptrace_hw_breakpoint() call in both
  flush_thread() and prepare_to_copy() functions. 'ptrace_bps[]' and
  'last_hit_ubp' members are now promptly cleaned-up.
- Single-step exception is now conditionally emulated upon hitting
  alignment_exception.
- Rebased to commit 31f46717997a83bdf6db0dd04810c0a329eb3148 of linux-2.6 tree.

Kindly let me know your comments for the same.

Thanks,
K.Prasad

Changelog - ver XXI
--------------------
(Version XX: linuxppc-dev ref:20100524103136.GA8131@in.ibm.com)
- Decision to emulate an instruction is now based on whether the causative
  instruction is in user_mode() or not.
- Breakpoints don't have to be cleared during sigreturn. A 'double-hit' on
  hw_breakpoint_handler() is harmless for non-ptrace instructions.
- Minor changes to aid code brevity.

Changelog - ver XX
--------------------
(Version XIX: linuxppc-dev ref: 20100524040137.GA20873@in.ibm.com)
- Non task-bound breakpoints will only be emulated. Breakpoint will be
  unregistered with a warning if emulation fails.

Changelog - ver XIX
--------------------
(Version XVIII: linuxppc-dev ref: 20100512033055.GA6384@in.ibm.com)
- Increased coverage of breakpoints during concurrent alignment_exception
  and signal handling (which previously had 'blind-spots').
- Support for kernel-thread breakpoints and kernel-space breakpoints inside the
  context of a user-space process.
- Patches re-based to commit f4b87dee923342505e1ddba8d34ce9de33e75050, thereby
  necessitating minor changes to arch_validate_hwbkpt_settings().

Changelog - ver XVIII
--------------------
(Version XVII: linuxppc-dev ref: 20100414034340.GA6571@in.ibm.com)
- Slight code restructuring for brevity and coding-style corrections.
- emulate_single_step() now notifies DIE_SSTEP to registered handlers;
  causes single_step_dabr_instruction() to be invoked after alignment_exception.
- hw-breakpoint restoration variables are cleaned-up before unregistration
  through arch_unregister_hw_breakpoint().
- SIGTRAP is no longer generated for non-ptrace user-space breakpoints.

Changelog - ver XVII
--------------------
(Version XVI: linuxppc-dev ref: 20100330095809.GA14403@in.ibm.com)
- CONFIG_HAVE_HW_BREAKPOINT is now used to define the scope of the new code
  (in lieu of CONFIG_PPC_BOOK3S_64).
- CONFIG_HAVE_HW_BREAKPOINT is now dependant upon CONFIG_PERF_EVENTS and
  CONFIG_PPC_BOOK3S_64 (to overcome build failures under certain configs).
- Included a target in arch/powerpc/lib/Makefile to build sstep.o when
  HAVE_HW_BREAKPOINT.
- Added a dummy definition for hw_breakpoint_disable() when !HAVE_HW_BREAKPOINT.
- Tested builds under defconfigs for ppc64, cell and g5 (found no patch induced
  failures).

Changelog - ver XVI
--------------------
(Version XV: linuxppc-dev ref: 20100323140639.GA21836@in.ibm.com)
- Used a new config option CONFIG_PPC_BOOK3S_64 (in lieu of
  CONFIG_PPC64/CPU_FTR_HAS_DABR) to limit the scope of the new code.
- Disabled breakpoints before kexec of the machine using hw_breakpoint_disable().
- Minor optimisation in exception-64s.S to check for data breakpoint exceptions
  in DSISR finally (after check for other causes) + changes in code comments and 
  representation of DSISR_DABRMATCH constant.
- Rebased to commit ae6be51ed01d6c4aaf249a207b4434bc7785853b of linux-2.6.

Changelog - ver XV
--------------------
(Version XIV: linuxppc-dev ref: 20100308181232.GA3406@in.ibm.com)

- Additional patch to disable interrupts during data breakpoint exception
  handling.
- Moved HBP_NUM definition to cputable.h under a new CPU_FTR definition
  (CPU_FTR_HAS_DABR).
- Filtering of extraneous exceptions (due to accesses outside symbol length) is
  by-passed for ptrace requests.
- Removed flush_ptrace_hw_breakpoint() from __switch_to() due to incorrect
  coding placement.
- Changes to code comments as per community reviews for previous version.
- Minor coding-style changes in hw_breakpoint.c as per review comments.
- Re-based to commit ae6be51ed01d6c4aaf249a207b4434bc7785853b of linux-2.6

Changelog - ver XIV
--------------------
(Version XIII: linuxppc-dev ref: 20100215055605.GB3670@in.ibm.com)

- Removed the 'name' field from 'struct arch_hw_breakpoint'.
- All callback invocations through bp->overflow_handler() are replaced with
  perf_bp_event().
- Removed the check for pre-existing single-stepping mode in
  hw_breakpoint_handler() as this check is unreliable while in kernel-space.
  Side effect of this change is the non-triggering of hw-breakpoints while
  single-stepping kernel through KGDB or Xmon.
- Minor code-cleanups and addition of comments in hw_breakpoint_handler() and
  single_step_dabr_instruction().
- Re-based to commit 25cf84cf377c0aae5dbcf937ea89bc7893db5176 of linux-2.6

Changelog - ver XIII
--------------------
(Version XII: linuxppc-dev ref: 20100121084640.GA3252@in.ibm.com)

- Fixed a bug for user-space breakpoints (triggered following the failure of a
  breakpoint request).
- Re-based on commit 724e6d3fe8003c3f60bf404bf22e4e331327c596 of linux-2.6
  
Changelog - ver XII
--------------------
(Version XI: linuxppc-dev ref: 20100119091234.GA9971@in.ibm.com)

- Unset MSR_SE only if kernel was not previously in single-step mode.
- Pre-emption is now enabled before returning from the hw-breakpoint exception
  handler.
- Variables to track the source of single-step exception (breakpoint from kernel,
  user-space vs single-stepping due to other requests) are added.
- Extraneous hw-breakpoint exceptions (due to memory accesses lying outside
  monitored symbol length) is now done for both kernel and user-space
  (previously only user-space).
- single_step_dabr_instruction() now returns NOTIFY_DONE if kernel was in
  single-step mode even before the hw-breakpoint. This enables other users of
  single-step mode to be notified of the exception.
- User-space instructions are not emulated from kernel-space, they are instead
  single-stepped.
  
Changelog - ver XI
------------------
(Version X: linuxppc-dev ref: 20091211160144.GA23156@in.ibm.com)
- Conditionally unset MSR_SE in the single-step handler
- Added comments to explain the duration and need for pre-emption
disable following hw-breakpoint exception.

Changelog - ver X
------------------
- Re-write the PPC64 patches for the new implementation of hw-breakpoints that
  uses the perf-layer.
- Rebased to commit 7622fc234190a37d4e9fe3ed944a2b61a63fca03 of -tip.

Changelog - ver IX
-------------------
- Invocation of user-defined callback will be 'trigger-after-execute' (except
  for ptrace).
- Creation of a new global per-CPU breakpoint structure to help invocation of
  user-defined callback from single-step handler.
(Changes made now)
- Validation before registration will fail only if the address does not match
  the kernel symbol's (if specified) resolved address
  (through kallsyms_lookup_name()).
- 'symbolsize' value is expected to within the range contained by the symbol's
  starting address and the end of a double-word boundary (8 Bytes).
- PPC64's arch-dependant code is now aware of 'cpumask' in 'struct hw_breakpoint'
  and can accomodate requests for a subset of CPUs in the system.
- Introduced arch_disable_hw_breakpoint() required for
  <enable><disable>_hw_breakpoint() APIs.

Changelog - ver VIII
-------------------
- Reverting changes to allow one-shot breakpoints only for ptrace requests.
- Minor changes in sanity checking in arch_validate_hwbkpt_settings().
- put_cpu_no_resched() is no longer available. Converted to put_cpu().

Changelog - ver VII
-------------------
- Allow the one-shot behaviour for exception handlers to be defined by the user.
  A new 'is_one_shot' flag is added to 'struct arch_hw_breakpoint'.

Changelog - ver VI
------------------
The task of identifying 'genuine' breakpoint exceptions from those caused by
'out-of-range' accesses turned out to be more tricky than originally thought.
Some changes to this effect were made in version IV of this patchset, but they
were not sufficient for user-space. Basically the breakpoint address received
through ptrace is always aligned to 8-bytes since ptrace receives an encoded
'data' (consisting of address | translation_enable | bkpt_type), and the size of
the symbol is not known. However for kernel-space addresses, the symbol-size can
be determined using kallsyms_lookup_size_offset() and this is used to check if
DAR (in the exception context) is
'bkpt_address <= DAR <= (bkpt_address + symbol_size)', failing which we conclude
it as a stray exception.

The following changes are made to enable check:
- Addition of a symbolsize field in 'struct arch_hw_breakpoint' field.
- Store the size of the 'watched' kernel symbol into 'symbolsize' field in
  arch_store_info(0 routine.
- Verify if the above described condition is true when is_one_shot is FALSE in
  hw_breakpoint_handler().

Changelog - ver V
------------------
- Breakpoint requests from ptrace (for user-space) are designed to be one-shot
in PPC64. The patch contains changes to retain this behaviour by returning early
in hw_breakpoint_handler() [without re-initialising DABR] and unregistering the
user-space request in ptrace_triggered(). It is safe to make a
unregister_user_hw_breakpoint() call from the breakpoint exception context
[through ptrace_triggered()] without giving rise to circular locking-dependancy.
This is because there can be no kernel code running on the CPU (which received
the exception) with the same spinlock held.

- Minor change in 'type' member of 'struct arch_hw_breakpoint' from u8 to 'int'.

Changelog - ver IV
------------------
- While DABR register requires double-word (8 bytes) aligned addresses, i.e.
the breakpoint is active over a range of 8 bytes, PPC64 allows byte-level
addressability. This may lead to stray exceptions which have to be ignored in
hw_breakpoint_handler(), when DAR != (Breakpoint request address). However DABR
will be populated with the requested breakpoint address aligned to the previous
double-word address. The code is now modified to store user-requested address
in 'bp->info.address' but update the DABR with a double-word aligned address.
- Please note that the Data Breakpoint facility in Xmon is broken as of 2.6.29
and the same has not been integrated into this facility as described in Ver I.

Changelog - ver III
------------------
- Patches are based on commit 08f16e060bf54bdc34f800ed8b5362cdeda75d8b of -tip
tree.
- The declarations in arch/powerpc/include/asm/hw_breakpoint.h are done only if
CONFIG_PPC64 is defined. This eliminates the need to conditionally include this
header file.
- load_debug_registers() is done in start_secondary() i.e. during CPU
initialisation.
- arch_check_va_<> routines in hw_breakpoint.c are now replaced with a much
simpler is_kernel_addr() check in arch_validate_hwbkpt_settings()
- Return code of hw_breakpoint_handler() when triggered due to Lazy debug
register switching is now changed to NOTIFY_STOP.
- The ptrace code no longer sets the TIF_DEBUG task flag as it is proposed to
be done in register_user_hw_breakpoint() routine.
- hw_breakpoint_handler() is now modified to use hbp_kernel_pos value to
  determine if the trigger was a user/kernel space address. The DAR register
  value is checked with the address stored in 'struct hw_breakpoint' to avoid
  handling of exceptions that belong to kprobe/Xmon.


Changelog - ver II
------------------
- Split the monolithic patch into six logical patches
- Changed the signature of arch_check_va_in_<user><kernel>space functions. They
  are now marked static.
- HB_NUM is now called as HBP_NUM (to preserve a consistent short-name
  convention)
- Introduced hw_breakpoint_disable() and changes to kexec code to disable
  breakpoints before a reboot.
- Minor changes in ptrace code to use macro-defined constants instead of
  numbers.
- Introduced a new constant definition INSTRUCTION_LEN in reg.h

Re: [Patch 0/5] PPC64-HWBKPT: Hardware Breakpoint interfaces - ver XXII

[Paul Mackerras]

On Fri, May 28, 2010 at 12:09:24PM +0530, K.Prasad wrote:

> 	Please find a new set of patches that have the following changes.

Thanks.  There are a couple of minor things still remaining (dangling
put_cpu in arch_unregister_hw_breakpoint, plus I don't think reusing
current->thread.ptrace_bps the way you did in patch 5/5 is a good
idea), but I think at this stage I'll put them in a tree together
with my latest emulate_step version and then push them to Ben H and/or
Ingo Molnar once I've done some testing.

Paul.

Re: [Patch 0/5] PPC64-HWBKPT: Hardware Breakpoint interfaces - ver XXII

[K.Prasad]

On Wed, Jun 02, 2010 at 09:33:16PM +1000, Paul Mackerras wrote:
> On Fri, May 28, 2010 at 12:09:24PM +0530, K.Prasad wrote:
> 
> > 	Please find a new set of patches that have the following changes.
> 
> Thanks.  There are a couple of minor things still remaining (dangling
> put_cpu in arch_unregister_hw_breakpoint, plus I don't think reusing
> current->thread.ptrace_bps the way you did in patch 5/5 is a good
> idea), but I think at this stage I'll put them in a tree together
> with my latest emulate_step version and then push them to Ben H and/or
> Ingo Molnar once I've done some testing.
> 
> Paul.

Hi Paul,
	Thanks for agreeing to put the patchset into a tree and push it
to the appropriate maintainers.

Meanwhile I tested the per-cpu breakpoints with the new emulate_step
patch (refer linuxppc-dev message-id:
20100602112903.GB30149@brick.ozlabs.ibm.com) and they continue to fail
due to emulate_step() failure, in my case, on a "lwz r0,0(r28)"
instruction.

About the latest patchset, given that we chose to ignore extraneous
interrupts for non-ptrace breakpoints, I thought that re-using
current->thread.ptrace_bps as a flag would be efficient than introducing
a new member in 'struct thread_struct' to do the same. I'm not sure if
you foresee any issues with that.

If so, I'd like to send a new patch (rather than a new version of the
complete patchset) to fix it along with the dangling put_cpu() in
arch_unregister_hw_breakpoint() (I forgot to remove parts of the code
between versions XIX and XX).

Thanks,
K.Prasad

Re: [Patch 0/5] PPC64-HWBKPT: Hardware Breakpoint interfaces - ver XXII

[Paul Mackerras]

On Fri, Jun 04, 2010 at 12:21:45PM +0530, K.Prasad wrote:

> Meanwhile I tested the per-cpu breakpoints with the new emulate_step
> patch (refer linuxppc-dev message-id:
> 20100602112903.GB30149@brick.ozlabs.ibm.com) and they continue to fail
> due to emulate_step() failure, in my case, on a "lwz r0,0(r28)"
> instruction.

Strange, what was in r28?  The emulator should handle that instruction.

> About the latest patchset, given that we chose to ignore extraneous
> interrupts for non-ptrace breakpoints, I thought that re-using
> current->thread.ptrace_bps as a flag would be efficient than introducing
> a new member in 'struct thread_struct' to do the same. I'm not sure if
> you foresee any issues with that.

I just wonder what provides exclusion between its use as a flag and
its use to hold a real ptrace breakpoint.  As far as I can see nothing
does.  If there is something, it's off in some other source file,
unless I'm missing something.  And in that case there should be a bit
fat comment explaining why it's safe.

> If so, I'd like to send a new patch (rather than a new version of the
> complete patchset) to fix it along with the dangling put_cpu() in
> arch_unregister_hw_breakpoint() (I forgot to remove parts of the code
> between versions XIX and XX).

OK.

Paul.

Re: [Patch 0/5] PPC64-HWBKPT: Hardware Breakpoint interfaces - ver XXII

[K.Prasad]

On Fri, Jun 04, 2010 at 07:06:48PM +1000, Paul Mackerras wrote:
> On Fri, Jun 04, 2010 at 12:21:45PM +0530, K.Prasad wrote:
> 
> > Meanwhile I tested the per-cpu breakpoints with the new emulate_step
> > patch (refer linuxppc-dev message-id:
> > 20100602112903.GB30149@brick.ozlabs.ibm.com) and they continue to fail
> > due to emulate_step() failure, in my case, on a "lwz r0,0(r28)"
> > instruction.
> 
> Strange, what was in r28?  The emulator should handle that instruction.
> 

It must be containing one of the offsets of "struct tracer" which is a
parameter to the function trace_selftest_startup_ksym(). Basically this
function does a selftest over hw-breakpoints by placing read-write
breakpoint on a dummy global-variable and performs read-write access
thereupon. The lwz instruction which fails here corresponds to the
instruction that does read-write. A complete disassemble of the function
upto the failing instruction (at address) is pasted at the end of this
mail.

> > About the latest patchset, given that we chose to ignore extraneous
> > interrupts for non-ptrace breakpoints, I thought that re-using
> > current->thread.ptrace_bps as a flag would be efficient than
> > introducing
> > a new member in 'struct thread_struct' to do the same. I'm not sure
> > if
> > you foresee any issues with that.
> 
> I just wonder what provides exclusion between its use as a flag and
> its use to hold a real ptrace breakpoint.  As far as I can see nothing
> does.  If there is something, it's off in some other source file,
> unless I'm missing something.  And in that case there should be a bit
> fat comment explaining why it's safe.
> 

The hw_breakpoint_handler() goes like this:

int __kprobes hw_breakpoint_handler(struct die_args *args)
{
...
....
	/*
	 * Return early after invoking user-callback function without restoring
	 * DABR if the breakpoint is from ptrace which always operates in
	 * one-shot mode. The ptrace-ed process will receive the SIGTRAP signal
	 * generated in do_dabr().
	 */
	if (is_ptrace_bp) {
		perf_bp_event(bp, regs);
		rc = NOTIFY_DONE;
		goto out;
	}

	/*
	 * Verify if dar lies within the address range occupied by the symbol
	 * being watched to filter extraneous exceptions.
	 */
	if (!((bp->attr.bp_addr <= dar) &&
	     (dar <= (bp->attr.bp_addr + bp->attr.bp_len)))) {
		/*
		 * This exception is triggered not because of a memory access
		 * on the monitored variable but in the double-word address
		 * range in which it is contained. We will consume this
		 * exception, considering it as 'noise'.
		 */
		is_extraneous_interrupt = true;
	}
....
...
}

Given that 'ptrace_bps' is used only for ptrace originated breakpoints
and that we return early i.e. before detecting extraneous interrupts
in hw_breakpoint_handler() (as shown above) they shouldn't overlap each
other. The following comment in hw_breakpoint_handler() explains the
same.
		/*
		 * To prevent invocation of perf_event_bp(), we shall overload
		 * thread.ptrace_bps[] pointer (unused for non-ptrace
		 * exceptions) to flag an extraneous interrupt which must be
		 * skipped.
		 */
Thanks,
K.Prasad

(gdb) disass trace_selftest_startup_ksym
Dump of assembler code for function trace_selftest_startup_ksym:
0xc000000000125580 <trace_selftest_startup_ksym+0>:	mflr    r0
0xc000000000125584 <trace_selftest_startup_ksym+4>:	std     r26,-48(r1)
0xc000000000125588 <trace_selftest_startup_ksym+8>:	std     r27,-40(r1)
0xc00000000012558c <trace_selftest_startup_ksym+12>:	std     r28,-32(r1)
0xc000000000125590 <trace_selftest_startup_ksym+16>:	std     r29,-24(r1)
0xc000000000125594 <trace_selftest_startup_ksym+20>:	std     r30,-16(r1)
0xc000000000125598 <trace_selftest_startup_ksym+24>:	std     r31,-8(r1)
0xc00000000012559c <trace_selftest_startup_ksym+28>:	std     r0,16(r1)
0xc0000000001255a0 <trace_selftest_startup_ksym+32>:	stdu    r1,-176(r1)
0xc0000000001255a4 <trace_selftest_startup_ksym+36>:	mr      r31,r1
0xc0000000001255a8 <trace_selftest_startup_ksym+40>:	ld      r30,-17784(r2)
0xc0000000001255ac <trace_selftest_startup_ksym+44>:	mr      r26,r4
0xc0000000001255b0 <trace_selftest_startup_ksym+48>:	mr      r27,r3
0xc0000000001255b4 <trace_selftest_startup_ksym+52>:	bl      0xc000000000125510 <tracer_init>
0xc0000000001255b8 <trace_selftest_startup_ksym+56>:	li      r4,3
0xc0000000001255bc <trace_selftest_startup_ksym+60>:	mr.     r29,r3
0xc0000000001255c0 <trace_selftest_startup_ksym+64>:	mr      r5,r29
0xc0000000001255c4 <trace_selftest_startup_ksym+68>:	beq     0xc0000000001255e0 <trace_selftest_startup_ksym+96>
0xc0000000001255c8 <trace_selftest_startup_ksym+72>:	ld      r3,-31520(r30)
0xc0000000001255cc <trace_selftest_startup_ksym+76>:	ld      r4,0(r27)
0xc0000000001255d0 <trace_selftest_startup_ksym+80>:	bl      0xc00000000009c560 <printk>
0xc0000000001255d4 <trace_selftest_startup_ksym+84>:	nop
0xc0000000001255d8 <trace_selftest_startup_ksym+88>:	b       0xc000000000125690 <trace_selftest_startup_ksym+272>
0xc0000000001255dc <trace_selftest_startup_ksym+92>:	nop
0xc0000000001255e0 <trace_selftest_startup_ksym+96>:	ld      r28,-31512(r30)
0xc0000000001255e4 <trace_selftest_startup_ksym+100>:	ld      r3,-31504(r30)
0xc0000000001255e8 <trace_selftest_startup_ksym+104>:	stw     r29,0(r28)
0xc0000000001255ec <trace_selftest_startup_ksym+108>:	mr      r5,r28
0xc0000000001255f0 <trace_selftest_startup_ksym+112>:	bl      0xc000000000140760 <process_new_ksym_entry>
0xc0000000001255f4 <trace_selftest_startup_ksym+116>:	nop
0xc0000000001255f8 <trace_selftest_startup_ksym+120>:	cmpwi   cr7,r3,0
0xc0000000001255fc <trace_selftest_startup_ksym+124>:	mr      r29,r3
0xc000000000125600 <trace_selftest_startup_ksym+128>:	blt     cr7,0xc00000000012567c <trace_selftest_startup_ksym+252>
0xc000000000125604 <trace_selftest_startup_ksym+132>:	lwz     r0,0(r28)
0xc000000000125608 <trace_selftest_startup_ksym+136>:	cmpwi   cr7,r0,0
0xc00000000012560c <trace_selftest_startup_ksym+140>:	bne     cr7,0xc000000000125618 <trace_selftest_startup_ksym+152>
0xc000000000125610 <trace_selftest_startup_ksym+144>:	li      r0,1

Re: [Patch 0/5] PPC64-HWBKPT: Hardware Breakpoint interfaces - ver XXII

[Paul Mackerras]

On Mon, Jun 07, 2010 at 12:33:51PM +0530, K.Prasad wrote:

> Given that 'ptrace_bps' is used only for ptrace originated breakpoints
> and that we return early i.e. before detecting extraneous interrupts
> in hw_breakpoint_handler() (as shown above) they shouldn't overlap each
> other. The following comment in hw_breakpoint_handler() explains the
> same.
> 		/*
> 		 * To prevent invocation of perf_event_bp(), we shall overload
> 		 * thread.ptrace_bps[] pointer (unused for non-ptrace
> 		 * exceptions) to flag an extraneous interrupt which must be
> 		 * skipped.
> 		 */

My point is that while we are using ptrace_bps[0] to mark a non-ptrace
breakpoint that we're single-stepping, some other process could be
ptracing this process and could get into ptrace_set_debugreg() and
would think that the process already has a ptrace breakpoint and call
modify_user_hw_breakpoint() when it should be calling
register_user_hw_breakpoint().  Or this process could die and so we
call flush_ptrace_hw_breakpoint() and it incorrectly thinks we have a
ptrace breakpoint.

If there is a reason why we can be quite sure that while we are using
current->thread.ptrace_bps[0] in this way, ptrace_set_debugreg() can
never get called with this task as the ptracee, and nor can
flush_ptrace_hw_breakpoint() get called on this task, then maybe it's
safe.  But it's not at all obviously safe.  So I'd very much rather we
just use an extra flag somewhere, that isn't used elsewhere for
anything else, so we can convince ourselves that it is all correct
without having to look at lots of different pieces of code.

There are 3 bytes of padding in struct arch_hw_breakpoint; couldn't we
use one of them as a "not really hit" flag?

Paul.

Re: [Patch 0/5] PPC64-HWBKPT: Hardware Breakpoint interfaces - ver XXII

[K.Prasad]

On Mon, Jun 07, 2010 at 09:25:59PM +1000, Paul Mackerras wrote:
> On Mon, Jun 07, 2010 at 12:33:51PM +0530, K.Prasad wrote:
> 
> > Given that 'ptrace_bps' is used only for ptrace originated breakpoints
> > and that we return early i.e. before detecting extraneous interrupts
> > in hw_breakpoint_handler() (as shown above) they shouldn't overlap each
> > other. The following comment in hw_breakpoint_handler() explains the
> > same.
> > 		/*
> > 		 * To prevent invocation of perf_event_bp(), we shall overload
> > 		 * thread.ptrace_bps[] pointer (unused for non-ptrace
> > 		 * exceptions) to flag an extraneous interrupt which must be
> > 		 * skipped.
> > 		 */
> 
> My point is that while we are using ptrace_bps[0] to mark a non-ptrace
> breakpoint that we're single-stepping, some other process could be
> ptracing this process and could get into ptrace_set_debugreg() and
> would think that the process already has a ptrace breakpoint and call
> modify_user_hw_breakpoint() when it should be calling
> register_user_hw_breakpoint().  Or this process could die and so we
> call flush_ptrace_hw_breakpoint() and it incorrectly thinks we have a
> ptrace breakpoint.
> 
> If there is a reason why we can be quite sure that while we are using
> current->thread.ptrace_bps[0] in this way, ptrace_set_debugreg() can
> never get called with this task as the ptracee, and nor can
> flush_ptrace_hw_breakpoint() get called on this task, then maybe it's
> safe.  But it's not at all obviously safe.  So I'd very much rather we
> just use an extra flag somewhere, that isn't used elsewhere for
> anything else, so we can convince ourselves that it is all correct
> without having to look at lots of different pieces of code.
> 
> There are 3 bytes of padding in struct arch_hw_breakpoint; couldn't we
> use one of them as a "not really hit" flag?
> 
> Paul.
> _______________________________________________

I get your reasoning now; ptrace_bps[] re-use will cause failures under
these circumstances. I've sent a new version of the patchset which adds
a new flag in 'struct arch_hw_breakpoint' (I was always thinking of
'struct thread_struct' before and was scared to introduce another new
member in it, thereby leading me to incorrectly optimise using ptrace_bps)
to flag extraneous_interrupt (Given that it's your idea I've added your
signed-off too).

Kindly let me know your comments, if any.

Thanks,
K.Prasad

Re: [Patch 0/5] PPC64-HWBKPT: Hardware Breakpoint interfaces - ver XXII

[Paul Mackerras]

On Wed, Jun 09, 2010 at 04:02:23PM +0530, K.Prasad wrote:

> (Given that it's your idea I've added your
> signed-off too).

Actually, you should never add someone else's signed-off-by unless
they specifically ask you to.  The signed-off-by lines are supposed to
show the path that the patch took to get into the tree, so in general
only the original author(s) and the maintainers who passed the patch
along should add signed-off-by lines.  It would only be a maintainer
who would add someone else's signed-off-by, and then only if someone
who needs to sign off on the patch had forgotten and then asked the
maintainer to correct their mistake.

In this case the appropriate way to give credit would be just a
sentence in the patch description; no formal tag is needed.

Paul.

Re: [Patch 0/5] PPC64-HWBKPT: Hardware Breakpoint interfaces - ver XXII

[Paul Mackerras]

On Fri, Jun 04, 2010 at 12:21:45PM +0530, K.Prasad wrote:

> Meanwhile I tested the per-cpu breakpoints with the new emulate_step
> patch (refer linuxppc-dev message-id:
> 20100602112903.GB30149@brick.ozlabs.ibm.com) and they continue to fail
> due to emulate_step() failure, in my case, on a "lwz r0,0(r28)"
> instruction.

You need to pass the instruction word to emulate_step(), not the
instruction address.  Also you need to have the full GPR set
available.  The patch below fixes these problems.  I'll fold these
changes into your patch 2/5.

Paul.
---
diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
index 3e423fb..f53029a 100644
--- a/arch/powerpc/kernel/exceptions-64s.S
+++ b/arch/powerpc/kernel/exceptions-64s.S
@@ -828,6 +828,7 @@ END_FW_FTR_SECTION_IFCLR(FW_FEATURE_ISERIES)
 
 /* We have a data breakpoint exception - handle it */
 handle_dabr_fault:
+	bl	.save_nvgprs
 	ld      r4,_DAR(r1)
 	ld      r5,_DSISR(r1)
 	addi    r3,r1,STACK_FRAME_OVERHEAD
diff --git a/arch/powerpc/kernel/hw_breakpoint.c b/arch/powerpc/kernel/hw_breakpoint.c
index ef70cf0..489049c 100644
--- a/arch/powerpc/kernel/hw_breakpoint.c
+++ b/arch/powerpc/kernel/hw_breakpoint.c
@@ -35,6 +35,7 @@
 #include <asm/hw_breakpoint.h>
 #include <asm/processor.h>
 #include <asm/sstep.h>
+#include <asm/uaccess.h>
 
 /*
  * Stores the breakpoints currently in use on each breakpoint address
@@ -203,6 +204,7 @@ int __kprobes hw_breakpoint_handler(struct die_args *args)
 	int stepped = 1;
 	struct arch_hw_breakpoint *info;
 	unsigned long dar = regs->dar;
+	unsigned int instr;
 
 	/* Disable breakpoints during exception handling */
 	set_dabr(0);
@@ -255,7 +257,11 @@ int __kprobes hw_breakpoint_handler(struct die_args *args)
 		goto out;
 	}
 
-	stepped = emulate_step(regs, regs->nip);
+	stepped = 0;
+	instr = 0;
+	if (!__get_user_inatomic(instr, (unsigned int *) regs->nip))
+		stepped = emulate_step(regs, instr);
+
 	/*
 	 * emulate_step() could not execute it. We've failed in reliably
 	 * handling the hw-breakpoint. Unregister it and throw a warning

Re: [Patch 0/5] PPC64-HWBKPT: Hardware Breakpoint interfaces - ver XXII

[K.Prasad]

On Tue, Jun 15, 2010 at 11:54:59AM +1000, Paul Mackerras wrote:
> On Fri, Jun 04, 2010 at 12:21:45PM +0530, K.Prasad wrote:
> 
> > Meanwhile I tested the per-cpu breakpoints with the new emulate_step
> > patch (refer linuxppc-dev message-id:
> > 20100602112903.GB30149@brick.ozlabs.ibm.com) and they continue to fail
> > due to emulate_step() failure, in my case, on a "lwz r0,0(r28)"
> > instruction.
> 
> You need to pass the instruction word to emulate_step(), not the
> instruction address.  Also you need to have the full GPR set
> available.  The patch below fixes these problems.  I'll fold these
> changes into your patch 2/5.
> 
> Paul.

The breakpoints that used to fail before (due to emulate_step() failure)
are now working fine upon applying this patch.

Please include this patch along with my 2/5 as indicated by you.

Thanks,
K.Prasad