PPC64-HWBKPT: Disable interrupts for data breakpoint exceptions

Data address breakpoint exceptions are currently handled along with page-faults which require interrupts to remain in enabled state. Since exception handling for data breakpoints aren't pre-empt safe, we handle them separately.

Signed-off-by K.Prasad <[email protected]>

Implement perf-events based hw-breakpoint interfaces for PPC64 processors. These interfaces help arbitrate requests from various users and schedules them as appropriate.

===================================================================

arch/powerpc/kernel/hw_breakpoint.c new file +3 -0

 
+	switch (bp->attr.bp_type) {
+	case HW_BREAKPOINT_R:
+		info->type = DABR_DATA_READ;

arch/powerpc/kernel/hw_breakpoint.c new file +7 -0

+	/*
+	 * Do not disable MSR_SE if the process was already in
+	 * single-stepping mode. We cannot reliable detect single-step mode
+	 * for kernel-space breakpoints, so this cannot work along with other
+	 * debuggers (like KGDB, xmon) which may be single-stepping kernel code.
+	 */
+	if (!(user_bp && test_thread_flag(TIF_SINGLESTEP)))

Quoted 3 lines · K.Prasad

Implement perf-events based hw-breakpoint interfaces for PPC64 processors. These interfaces help arbitrate requests from various users and schedules them as appropriate.

[snip]

Quoted 4 lines

arch/powerpc/include/asm/hw_breakpoint.h new file +1 -0

@@ -0,0 +1,45 @@
+#ifndef _PPC_BOOK3S_64_HW_BREAKPOINT_H

arch/powerpc/include/asm/hw_breakpoint.h line 5

Quoted 4 lines

arch/powerpc/include/asm/hw_breakpoint.h new file +4 -0

+#define _PPC_BOOK3S_64_HW_BREAKPOINT_H
+
+#ifdef	__KERNEL__
+#define	__ARCH_HW_BREAKPOINT_H

This symbol doesn't seem to be referenced anywhere. Is it really necessary to define it? I know x86 and sh do, but maybe it's a leftover there.

Quoted 13 lines

===================================================================

arch/powerpc/kernel/hw_breakpoint.c new file +9 -0

 
+	switch (bp->attr.bp_type) {
+	case HW_BREAKPOINT_R:
+		info->type = DABR_DATA_READ;
+		break;
+	case HW_BREAKPOINT_W:
+		info->type = DABR_DATA_WRITE;
+		break;
+	case HW_BREAKPOINT_R | HW_BREAKPOINT_W:
+		info->type = (DABR_DATA_READ | DABR_DATA_WRITE);

arch/powerpc/kernel/hw_breakpoint.c line 118

Quoted 4 lines

arch/powerpc/kernel/hw_breakpoint.c new file +4 -0

+		break;
+	default:
+		return ret;
+	}

You have code like this in several places -- maybe this should be encapsulated in a helper function. Also, I think it could be written more compactly.

Quoted 7 lines

arch/powerpc/kernel/hw_breakpoint.c new file +7 -0

+	/*
+	 * Return early after invoking user-callback function without restoring
+	 * DABR if the breakpoint is from ptrace which always operates in
+	 * one-shot mode
+	 */
+	if (is_ptrace_bp == true) {
+		perf_bp_event(bp, regs);

arch/powerpc/kernel/hw_breakpoint.c line 195

Quoted 3 lines

arch/powerpc/kernel/hw_breakpoint.c new file +3 -0

+		rc = NOTIFY_DONE;
+		goto out;
+	}

As far as I can see, returning NOTIFY_DONE won't tell do_dabr() that you have handled the exception, and it will go on to do the debugger_dabr_match() call and generate a signal to the current process. Is that what you want? If so a comment would be useful.

Also, the " == true" part is completely redundant. Normal kernel style would be if (is_ptrace_bp) { ..., and similarly the (is_ptrace_bp == false) above should be !is_ptrace_bp.

Quoted 7 lines

arch/powerpc/kernel/hw_breakpoint.c new file +7 -0

+
+	/*
+	 * Do not emulate user-space instructions from kernel-space,
+	 * instead single-step them.
+	 */
+	if (is_kernel == false) {
+		current->thread.last_hit_ubp = bp;

arch/powerpc/kernel/hw_breakpoint.c line 205

Quoted 3 lines

arch/powerpc/kernel/hw_breakpoint.c new file +3 -0

+		regs->msr |= MSR_SE;
+		goto out;
+	}

I'm a bit worried about what could happen from here on. We go back out to userspace and try to execute the load or store. Besides executing successfully and taking the trace interrupt, there are several other possibilities:

- we could get an alignment interrupt
- we could get a page fault (page not present or protection violation)
- we could get a MMU hash table miss (unlikely since the low-level code calls hash_page before do_dabr, but possible since the HPTE could in principle have been evicted in the meantime).
- we could even get an illegal or privileged instruction trap -- if some other thread had changed the instruction in the meantime.

The alignment interrupt case is mostly OK, except that it will call emulate_single_step after emulating the load or store, which doesn't do quite what we want -- unlike single_step_exception(), it doesn't call notify_die(), so we won't get back into single_step_dabr_instruction(), but instead it just sends a SIGTRAP. That needs to be fixed, but note that single_step_exception() only applies to "classic"/server processors at present because it hits the MSR SE and BE bits directly rather than using clear_single_step().

The MMU hash table miss case looks to be OK -- we'll just return to userspace and re-execute the instruction, with MSR[SE] still set.

The page fault case should be OK if it just results in inserting a PTE (possibly after some context switches) or in the process being terminated. If, however, it results in a signal we could end up with a stale value in current->thread.last_hit_ubp if the signal handler doesn't return to the instruction (and there is no requirement for it to do so). If the process later gets single-stepped for any reason it could get misinterpreted, and we could end up accessing freed memory if the perf_event associated with bp has been closed and freed in the meantime.

Similarly, if the instruction has been changed underneath us, we would end up with current->thread.last_hit_ubp being stale. We do need to handle this case if only to ensure that we don't create a vulnerability that could be used to attack the kernel.

arch/powerpc/kernel/hw_breakpoint.c line 208

Quoted 3 lines

arch/powerpc/kernel/hw_breakpoint.c new file +3 -0

+
+	stepped = emulate_step(regs, regs->nip);
+	/* emulate_step() could not execute it, single-step them */

Note that at present, emulate_step() will never emulate a load or store, so this will always return 0 unless someone has changed the instruction underneath us.

Quoted 4 lines

arch/powerpc/kernel/hw_breakpoint.c new file +4 -0

+	if (stepped == 0) {
+		regs->msr |= MSR_SE;
+		per_cpu(last_hit_bp, cpu) = bp;
+		goto out;

arch/powerpc/kernel/hw_breakpoint.c

Quoted 1 line

arch/powerpc/kernel/hw_breakpoint.c new file +1 -0

+	}

This is subject to most of the same possibilities as the user address case. We need to make sure that if we ever get the situation where the instruction is never going to be executed then we clear last_hit_bp for this cpu. By the way, __get_cpu_var(x) is more efficient than per_cpu(x, cpu) and is equivalent if cpu == smp_processor_id().

Quoted 12 lines

arch/powerpc/kernel/hw_breakpoint.c new file +12 -0

+	/*
+	 * Do not disable MSR_SE if the process was already in
+	 * single-stepping mode. We cannot reliable detect single-step mode
+	 * for kernel-space breakpoints, so this cannot work along with other
+	 * debuggers (like KGDB, xmon) which may be single-stepping kernel code.
+	 */
+	if (!(user_bp && test_thread_flag(TIF_SINGLESTEP)))
+		regs->msr &= ~MSR_SE;
+
+	/* Deliver signal to user-space */
+	if (user_bp) {
+		info.si_signo = SIGTRAP;

arch/powerpc/kernel/hw_breakpoint.c line 283

Quoted 5 lines

arch/powerpc/kernel/hw_breakpoint.c new file +5 -0

+		info.si_errno = 0;
+		info.si_code = TRAP_HWBKPT;
+		info.si_addr = (void __user *)bp_info->address;
+		force_sig_info(SIGTRAP, &info, current);
+	}

Why are we delivering a signal to userspace here? This seems unnecessary to me.

Ensuring that we get control back reliably after executing the instruction, or when we get to the point where the instruction will never be executed, seems to be difficult to get right in all corner cases. It may be better to just emulate the load or store unconditionally. We don't currently have code to do that, but fix_alignment() is pretty close to what we would need (it would need to handle lbz/stb and it would need to use 2, 4 or 8-byte loads/stores rather than 1-byte loads/stores in the aligned case).

Quoted 2 lines · Paul Mackerras

On Wed, Apr 14, 2010 at 09:18:27AM +0530, K.Prasad wrote:

It has been pointed out to me before (Roland's mail Ref:linuxppc-dev
message-id: [email protected]) that there will be too many corner cases that will be difficult to foresee, however your above list appears to be exhaustive. While the alternatives to this being a fallback to one-shot breakpoints (thereby leading to confusing hw-breakpoint interface semantics), this is an attempt to generate continuous and 'trigger-after-execute' (for non-ptrace requests) breakpoint exceptions. I believe that, with the addressal of concerns cited above, the resultant patchset would be one that achieves the stated design goals with no loss to existing functionality.

I intend to send out another version of this patchset with fixes as described in my replies above (unless I hear objections to it :-)).

It is true that the breakpoint exceptions will go amiss following the alignment exception, and be restored when the thread single-steps due to other requests causing undesirable effects. (Borrowing from some of the discussions I had with BenH, earlier) There can be two ways of changing the implementation to counter it:

- To sense that the impending exception (alignment, page-fault, single-step) is a successor to a hw-breakpoint exception (and that restoration of debug register values is necessary), somewhere early in exceptions-64s.S and jump to a common handler, say do_single_step_dabr() which does a majority of single_step_dabr_instruction().
- To modify emulate_single_step() to also do a notify_die(DIE_SSTEP,...) in addition to its existing code. This would invoke single_step_dabr_instruction() where the breakpoints can be restored.

I must admit that it is not clear to me when you say "doesn't return to the instruction" and "instruction has been changed underneath". Are you

referring to the fact that the thread which generated breakpoints hits could have moved out of the CPU due to a scheduler induced context switch (which is an apparent cause for current->thread.last_hit_ubp to turn stale) or is there a different source for such a change that I don't realise?

Given that 'last_hit_ubp' is safely ensconced inside 'thread_struct', the ill-effects of a possible pre-emption during a page-fault will be suitably handled i.e. the pending single-step exception will be generated on the processor to which 'current' is migrated to, and the breakpoint will be set on the new processor.

However, the possibility that current->thread.last_hit_ubp points to a perf_event structure that is unregistered and freed does exist, and I did not foresee the risk. An arch-specific function that hooks onto release_bp_slot() would be required to perform the cleanup. I will submit modify the patch to that effect. Thanks for pointing it out.

In conjunction with what you've stated below, do you suggest that emulate_step() be replaced with fix_alignment() which appears to be more powerful at emulation (or carve out a helper function for fix_alignment() that does only emulation and which can be invoked here)?

Quoted 6 lines · K.Prasad

It is true that the breakpoint exceptions will go amiss following the alignment exception, and be restored when the thread single-steps due to other requests causing undesirable effects. (Borrowing from some of the discussions I had with BenH, earlier) There can be two ways of changing the implementation to counter it:

Quoted 9 lines

- To sense that the impending exception (alignment, page-fault, single-step) is a successor to a hw-breakpoint exception (and that restoration of debug register values is necessary), somewhere early in exceptions-64s.S and jump to a common handler, say do_single_step_dabr() which does a majority of single_step_dabr_instruction().
- To modify emulate_single_step() to also do a notify_die(DIE_SSTEP,...) in addition to its existing code. This would invoke single_step_dabr_instruction() where the breakpoints can be restored.

I thought you would change the explicit regs->msr modification in single_step_exception() to clear_single_step(), then just make emulate_single_step() call single_step_exception().

Quoted 2 lines

I must admit that it is not clear to me when you say "doesn't return to the instruction" and "instruction has been changed underneath". Are you

Suppose the address at which the data breakpoint has been unmapped, and the process has a handler for the SIGSEGV signal. When we try to single-step the load or store, we will get a DSI (0x300) interrupt, call into do_page_fault, and end up sending the process a SIGSEGV. That will invoke the signal handler, which can then do anything it likes. It can do a blocking system call, it can longjmp() back into its main event, or it can return from the signal handler. Only in the last case will it retry the load or store, and then only if the signal handler hasn't modified the NIP value in the signal frame. That's what I mean by "doesn't return to the instruction".

The instruction could be changed underneath us if the program is multi-threaded and another thread writes another instruction to the instruction word where the load or store is. Or it could use mmap() to map some other page at the address of the load or store. Either way we could end up with a different instruction there.

Quoted 11 lines

referring to the fact that the thread which generated breakpoints hits could have moved out of the CPU due to a scheduler induced context switch (which is an apparent cause for current->thread.last_hit_ubp to turn stale) or is there a different source for such a change that I don't realise?

Given that 'last_hit_ubp' is safely ensconced inside 'thread_struct', the ill-effects of a possible pre-emption during a page-fault will be suitably handled i.e. the pending single-step exception will be generated on the processor to which 'current' is migrated to, and the breakpoint will be set on the new processor.

If we do get a context switch, e.g. as a result of a page fault, and then switch back to the task, it looks to me like we will end up with MSR_SE and DABR both set. I don't suppose that will actually cause any real problem besides double-counting the hit.

Quoted 5 lines

However, the possibility that current->thread.last_hit_ubp points to a perf_event structure that is unregistered and freed does exist, and I did not foresee the risk. An arch-specific function that hooks onto release_bp_slot() would be required to perform the cleanup. I will submit modify the patch to that effect. Thanks for pointing it out.

Yes, I think we need that.

Quoted 4 lines

In conjunction with what you've stated below, do you suggest that emulate_step() be replaced with fix_alignment() which appears to be more powerful at emulation (or carve out a helper function for fix_alignment() that does only emulation and which can be invoked here)?

Something like that eventually, but not for a first pass.

Paul.

Okay. I will re-use single_step_exception() after modifications; it appearsto have no in-kernel users for it.

Quoted 11 lines

Suppose the address at which the data breakpoint has been unmapped, and the process has a handler for the SIGSEGV signal. When we try to single-step the load or store, we will get a DSI (0x300) interrupt, call into do_page_fault, and end up sending the process a SIGSEGV. That will invoke the signal handler, which can then do anything it likes. It can do a blocking system call, it can longjmp() back into its main event, or it can return from the signal handler. Only in the last case will it retry the load or store, and then only if the signal handler hasn't modified the NIP value in the signal frame. That's what I mean by "doesn't return to the instruction".

At the outset, this seemed to be a scary thing to happen; but turns out to be harmful only to the extent of generating a false hw-breakpoint exception in certain cases. A case-by-case basis analysis reveals thus:

Consider an instruction stream i1, i2, i3, ... iN, where i1 has finished execution and i2 is about to be executed but has generated a DSI interrupt with the above-mentioned conditions i.e. DSISR indicates a DABR match + Page-Table entry not found. Now according to do_hash_page in exception-64s.S (as pasted below), do_page_fault() and do_dabr() are invoked one after the other.

_STATIC(do_hash_page)

	std	r3,_DAR(r1)
	std	r4,_DSISR(r1)

	andis.	r0,r4,0xa410		/* weird error? */
	bne-	handle_page_fault	/* if not, try to insert a HPTE */
	andis.  r0,r4,DSISR_DABRMATCH@h
	bne-    handle_dabr_fault

Thus, when control returns to user-space to instruction 'i2', the hw_breakpoint_handler() has completed execution, and a SIGSEGV is pending to be delivered and single-stepping enabled MSR_SE is set. Upon return to user-space, the handler for SIGSEGV is executed and it may perform one of the following (as you stated previously):
(a) Make a blocking syscall, eventually yielding the CPU to a new thread
(b) Jump to a different instruction in user-space, say iN, and not complete the execution of instruction i2 at all.
(c) Return to instruction i2 and complete the execution.

In case of (a), the context-switches should not affect the ability to single-step the instruction when the thread is eventually scheduled to run. The thread, when scheduled onto the CPU will complete signal handling, return to execute instruction i2, cause single-step exception, restore breakpoints and run smoothly thereafter.

In case of (b), the new instruction iN is single-stepped, the breakpoint values are restored and the hw-breakpoint exception callback is invoked after iN is executed. The user of this breakpoint i.e. the caller of register_user_hw_breakpoint() who had placed a breakpoint on addressed accessed by instruction i2 will be confused to find that an unrelated instruction (which may not be a load/store) has caused the breakpoint.

If so desired, we may adopt the 'trigger-before-execute' semantics for user-space breakpoints wherein the hw-breakpoint callback (through perf_bp_event()) is invoked in hw_breakpoint_handler() itself. This would indicate to the user that the impending instruction causes a DABR 'hit' but it may or may not be executed due to the role of signal-handler or due to self-modifying code (as mentioned below).

Kindly let me know what you think about it.

(c) is the normal execution path we desire. The instruction i2 will be safely single-stepped and breakpoints are restored.

Quoted 6 lines

The instruction could be changed underneath us if the program is multi-threaded and another thread writes another instruction to the instruction word where the load or store is. Or it could use mmap() to map some other page at the address of the load or store. Either way we could end up with a different instruction there.

If the instruction that originally caused the DABR exception is changed, the new instruction in its place would still single-step to restore breakpoint values. However the user of breakpoint interface will be confused to find that the callback is invoked for an irrelevant instruction.

It could be circumvented, to an extent, through the use of trigger-before-execute semantics (as described before).

Quoted 5 lines

If we do get a context switch, e.g. as a result of a page fault, and then switch back to the task, it looks to me like we will end up with MSR_SE and DABR both set. I don't suppose that will actually cause any real problem besides double-counting the hit.

Page fault exception will be handled before hw_breakpoint_handler(), hence MSR_SE would not have been set if a context-switch happened in pange-fault handling itself. I don't see a case where both MSR_SE and DABR will be set together.

Thanks for the comments. Let me know if the analysis above is incorrect or if I've failed to recognise any important issue that you pointed out. I will send out a patch with changes to emulate_single_step() in the next version of the patchset, if I don't hear any further comments.

Quoted 2 lines · K.Prasad

Okay. I will re-use single_step_exception() after modifications; it appearsto have no in-kernel users for it.

It's called from exceptions-64s.S, head_32.S and head_8xx.S in arch/powerpc/kernel.

Quoted 23 lines

Quoted 11 lines

Suppose the address at which the data breakpoint has been unmapped, and the process has a handler for the SIGSEGV signal. When we try to single-step the load or store, we will get a DSI (0x300) interrupt, call into do_page_fault, and end up sending the process a SIGSEGV. That will invoke the signal handler, which can then do anything it likes. It can do a blocking system call, it can longjmp() back into its main event, or it can return from the signal handler. Only in the last case will it retry the load or store, and then only if the signal handler hasn't modified the NIP value in the signal frame. That's what I mean by "doesn't return to the instruction".

At the outset, this seemed to be a scary thing to happen; but turns out to be harmful only to the extent of generating a false hw-breakpoint exception in certain cases. A case-by-case basis analysis reveals thus:

Consider an instruction stream i1, i2, i3, ... iN, where i1 has finished execution and i2 is about to be executed but has generated a DSI interrupt with the above-mentioned conditions i.e. DSISR indicates a DABR match + Page-Table entry not found. Now according to do_hash_page in exception-64s.S (as pasted below), do_page_fault() and do_dabr() are invoked one after the other.

Quoted 8 lines

_STATIC(do_hash_page)

	std	r3,_DAR(r1)
	std	r4,_DSISR(r1)

	andis.	r0,r4,0xa410		/* weird error? */
	bne-	handle_page_fault	/* if not, try to insert a HPTE */
	andis.  r0,r4,DSISR_DABRMATCH@h
	bne-    handle_dabr_fault

Note that bne is not a procedure call; we'll actually get two DSIs in this scenario. But I don't think that matters. Also note that the branch to handle_page_fault here is not for the HPTE-not-found case; it's for the unusual errors. So we'll handle the HPTE insertion after handling the DABR match.

Quoted 10 lines

Thus, when control returns to user-space to instruction 'i2', the hw_breakpoint_handler() has completed execution, and a SIGSEGV is pending to be delivered and single-stepping enabled MSR_SE is set. Upon return to user-space, the handler for SIGSEGV is executed and it may perform one of the following (as you stated previously):
(a) Make a blocking syscall, eventually yielding the CPU to a new thread
(b) Jump to a different instruction in user-space, say iN, and not complete the execution of instruction i2 at all.
(c) Return to instruction i2 and complete the execution.

Quoted 5 lines

In case of (a), the context-switches should not affect the ability to single-step the instruction when the thread is eventually scheduled to run. The thread, when scheduled onto the CPU will complete signal handling, return to execute instruction i2, cause single-step exception, restore breakpoints and run smoothly thereafter.

Right. However, the thread is running the signal handler without the DABR being set, which is unfortunate.

Quoted 6 lines

In case of (b), the new instruction iN is single-stepped, the breakpoint values are restored and the hw-breakpoint exception callback is invoked after iN is executed. The user of this breakpoint i.e. the caller of register_user_hw_breakpoint() who had placed a breakpoint on addressed accessed by instruction i2 will be confused to find that an unrelated instruction (which may not be a load/store) has caused the breakpoint.

That's the case if the signal handler modifies the NIP value in the register set saved on the stack and returns. If the signal handler instead simply jumps to instruction iN (e.g. with longjmp or siglongjmp), we'll never get the single-step callback.

Quoted 25 lines

If so desired, we may adopt the 'trigger-before-execute' semantics for user-space breakpoints wherein the hw-breakpoint callback (through perf_bp_event()) is invoked in hw_breakpoint_handler() itself. This would indicate to the user that the impending instruction causes a DABR 'hit' but it may or may not be executed due to the role of signal-handler or due to self-modifying code (as mentioned below).

Kindly let me know what you think about it.

(c) is the normal execution path we desire. The instruction i2 will be safely single-stepped and breakpoints are restored.

Quoted 6 lines

The instruction could be changed underneath us if the program is multi-threaded and another thread writes another instruction to the instruction word where the load or store is. Or it could use mmap() to map some other page at the address of the load or store. Either way we could end up with a different instruction there.

If the instruction that originally caused the DABR exception is changed, the new instruction in its place would still single-step to restore breakpoint values. However the user of breakpoint interface will be confused to find that the callback is invoked for an irrelevant instruction.

Quoted 2 lines

It could be circumvented, to an extent, through the use of trigger-before-execute semantics (as described before).

I don't think we want to do trigger-before-execute. Ideally what we want to ensure at all times is that either DABR is set (enabled) or MSR.SE is set, but not both. To ensure that we'd have to modify the signal delivery code and possibly other places.

Quoted 10 lines

Quoted 5 lines

If we do get a context switch, e.g. as a result of a page fault, and then switch back to the task, it looks to me like we will end up with MSR_SE and DABR both set. I don't suppose that will actually cause any real problem besides double-counting the hit.

Page fault exception will be handled before hw_breakpoint_handler(), hence MSR_SE would not have been set if a context-switch happened in pange-fault handling itself. I don't see a case where both MSR_SE and DABR will be set together.

Imagine this scenario: we get the DABR match, set MSR_SE and return to the task. In the meantime another higher-priority task has become runnable and our need_resched flag is set, so we call schedule() on the way back out to usermode. The other task runs and then blocks and our task gets scheduled again. As part of the context switch, arch_install_hw_breakpoint() will get called and will set DABR. So we'll return to usermode with both DABR and MSE_SE set.

Quoted 4 lines

Thanks for the comments. Let me know if the analysis above is incorrect or if I've failed to recognise any important issue that you pointed out. I will send out a patch with changes to emulate_single_step() in the next version of the patchset, if I don't hear any further comments.

We haven't discussed at all the case where the breakpoint is a per-cpu breakpoint or where it's a per-task breakpoint but the DABR match occurs within the kernel -- which can happen, even for a user address, in __get_user, __put_user, __copy_tofrom_user, etc. If the access there is to a bad address, we'll invoke the exception case in bad_page_fault(), which looks to be another place where we need to recognize that single-stepping won't succeed and reinstall the DABR setting. Do we count that as an event or not? - I'm not sure.

Quoted 3 lines

Right. However, the thread is running the signal handler without the DABR being set, which is unfortunate.

In order to keep the breakpoint active during signal handling, a PowerPC specific signal handling code, say do_signal_pending() in arch/powerpc/kernel/signal.c, must be tapped to check for/restore the breakpoint for the process (if it exists).

I'm afraid if this is more complexity than we want to handle in the first patchset. I agree that this will create a 'blind-spot' of code which cannot not be monitored using breakpoints and may limit debugging efforts (specially for memory corruption); however suspecting that signal handlers (especially those that don't return to original instruction) would be rare, I feel that this could be a 'feature' that can be brought later-in. What do you think?

Quoted 8 lines

Imagine this scenario: we get the DABR match, set MSR_SE and return to the task. In the meantime another higher-priority task has become runnable and our need_resched flag is set, so we call schedule() on the way back out to usermode. The other task runs and then blocks and our task gets scheduled again. As part of the context switch, arch_install_hw_breakpoint() will get called and will set DABR. So we'll return to usermode with both DABR and MSE_SE set.

I didn't foresee such a possibility. I think this can be handled by putting a check in arch_install_hw_breakpoint() as shown below:

int arch_install_hw_breakpoint(struct perf_event *bp)

{
	struct arch_hw_breakpoint *info = counter_arch_bp(bp);
	struct perf_event **slot = &__get_cpu_var(bp_per_reg);

	*slot = bp;
	if (!current->thread.last_hit_ubp)
		set_dabr(info->address | info->type | DABR_TRANSLATION);
	return 0;
}

On Thu, May 20, 2010 at 09:36:03AM +0530, K.Prasad wrote:

Quoted 8 lines

Quoted 3 lines

Right. However, the thread is running the signal handler without the DABR being set, which is unfortunate.

In order to keep the breakpoint active during signal handling, a PowerPC specific signal handling code, say do_signal_pending() in arch/powerpc/kernel/signal.c, must be tapped to check for/restore the breakpoint for the process (if it exists).

What I would suggest is something slightly different: anything that causes the thread to change where it's executing -- signal delivery, modification of NIP with ptrace -- should cancel the single-step and reinstall the breakpoint in the DABR. In other words we just forget that we hit the breakpoint, and rely on hitting it again if we ever get back to that instruction. I think that is by far the most reliable approach.

That means that the hw-breakpoint code needs to export a function called, say, thread_change_pc(), which is called whenever we're changing a thread's userspace PC (NIP) value. If the hw-breakpoint code has put that thread into single-step mode, we cancel the single-step and if the thread is current, set DABR.

Quoted 7 lines

I'm afraid if this is more complexity than we want to handle in the first patchset. I agree that this will create a 'blind-spot' of code which cannot not be monitored using breakpoints and may limit debugging efforts (specially for memory corruption); however suspecting that signal handlers (especially those that don't return to original instruction) would be rare, I feel that this could be a 'feature' that can be brought later-in. What do you think?

I think the thread_change_pc() approach is quite a bit simpler, and I think it's better to get this right at the outset rather than have it cause bugs later on, when we've all forgotten all the curly details. :)

Quoted 14 lines

Quoted 9 lines

Imagine this scenario: we get the DABR match, set MSR_SE and return to the task. In the meantime another higher-priority task has become runnable and our need_resched flag is set, so we call schedule() on the way back out to usermode. The other task runs and then blocks and our task gets scheduled again. As part of the context switch, arch_install_hw_breakpoint() will get called and will set DABR. So we'll return to usermode with both DABR and MSE_SE set.

I didn't foresee such a possibility. I think this can be handled by putting a check in arch_install_hw_breakpoint() as shown below:

int arch_install_hw_breakpoint(struct perf_event *bp)

Quoted 9 lines

{
	struct arch_hw_breakpoint *info = counter_arch_bp(bp);
	struct perf_event **slot = &__get_cpu_var(bp_per_reg);

	*slot = bp;
	if (!current->thread.last_hit_ubp)
		set_dabr(info->address | info->type | DABR_TRANSLATION);
	return 0;
}

Yes, basically, but don't we need to handle per-cpu breakpoints as well? That is, we only want the extra check if this breakpoint is a per-task breakpoint. Or am I not seeing enough context here?

Also, I have just posted extensions to emulate_single_step() to handle loads and stores. I think this should be enough to handle DABR interrupts that occur in kernel mode, so we should never need to actually single-step in that case -- if emulate_step fails we should just print a warning, uninstall the breakpoint, and continue. That way we don't have to worry about all the interrupts and other eventualities that could occur while single-stepping in the kernel.

For DABR interrupts that occur in user mode, I think the approach of single-stepping together with calls to thread_change_pc() in the signal delivery and ptrace code should handle all the cases, at least for per-task breakpoints. Per-cpu breakpoints that get hit in user mode will be a bit trickier, but I assume we can restrict per-cpu breakpoints to kernel addresses for now. If you want help adding the thread_change_pc() calls to signal delivery and ptrace, let me know.