The following series replaces the ptrace helpers used for single step
enable/disable for uprobes on powerpc, with uprobe specific code.
We reuse the kprobe code to enable single stepping by making it generic
and save/restore the MSR (and DBCR for BookE) across the single step.
This series applies on top of the patches posted by Oleg at :
https://lkml.org/lkml/2012/10/28/92
Patches have been verified on Power6 and PPC440 (BookE).
---
Suzuki K. Poulose (2):
powerpc: Move the single step enable code to a generic path
uprobes/powerpc: Make use of generic routines to enable single step
arch/powerpc/include/asm/probes.h | 29 +++++++++++++++++++++++++++++
arch/powerpc/include/asm/uprobes.h | 4 ++++
arch/powerpc/kernel/kprobes.c | 21 +--------------------
arch/powerpc/kernel/uprobes.c | 11 +++++++++--
4 files changed, 43 insertions(+), 22 deletions(-)
--
Suzuki
From: Suzuki K. Poulose <[email protected]>
This patch moves the single step enable code used by kprobe to a generic
routine so that, it can be re-used by other code, in this case,
uprobes.
Signed-off-by: Suzuki K. Poulose <[email protected]>
Cc: [email protected]
---
arch/powerpc/include/asm/probes.h | 29 +++++++++++++++++++++++++++++
arch/powerpc/kernel/kprobes.c | 21 +--------------------
2 files changed, 30 insertions(+), 20 deletions(-)
diff --git a/arch/powerpc/include/asm/probes.h b/arch/powerpc/include/asm/probes.h
index 5f1e15b..836e9b9 100644
--- a/arch/powerpc/include/asm/probes.h
+++ b/arch/powerpc/include/asm/probes.h
@@ -38,5 +38,34 @@ typedef u32 ppc_opcode_t;
#define is_trap(instr) (IS_TW(instr) || IS_TWI(instr))
#endif /* CONFIG_PPC64 */
+#ifdef CONFIG_PPC_ADV_DEBUG_REGS
+#define MSR_SINGLESTEP (MSR_DE)
+#else
+#define MSR_SINGLESTEP (MSR_SE)
+#endif
+
+/* Enable single stepping for the current task */
+static inline void enable_single_step(struct pt_regs *regs)
+{
+
+ /*
+ * We turn off async exceptions to ensure that the single step will
+ * be for the instruction we have the kprobe on, if we dont its
+ * possible we'd get the single step reported for an exception handler
+ * like Decrementer or External Interrupt
+ */
+ regs->msr &= ~MSR_EE;
+ regs->msr |= MSR_SINGLESTEP;
+#ifdef CONFIG_PPC_ADV_DEBUG_REGS
+ regs->msr &= ~MSR_CE;
+ mtspr(SPRN_DBCR0, mfspr(SPRN_DBCR0) | DBCR0_IC | DBCR0_IDM);
+#ifdef CONFIG_PPC_47x
+ isync();
+#endif
+#endif
+
+}
+
+
#endif /* __KERNEL__ */
#endif /* _ASM_POWERPC_PROBES_H */
diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
index e88c643..92f1be7 100644
--- a/arch/powerpc/kernel/kprobes.c
+++ b/arch/powerpc/kernel/kprobes.c
@@ -36,12 +36,6 @@
#include <asm/sstep.h>
#include <asm/uaccess.h>
-#ifdef CONFIG_PPC_ADV_DEBUG_REGS
-#define MSR_SINGLESTEP (MSR_DE)
-#else
-#define MSR_SINGLESTEP (MSR_SE)
-#endif
-
DEFINE_PER_CPU(struct kprobe *, current_kprobe) = NULL;
DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk);
@@ -104,20 +98,7 @@ void __kprobes arch_remove_kprobe(struct kprobe *p)
static void __kprobes prepare_singlestep(struct kprobe *p, struct pt_regs *regs)
{
- /* We turn off async exceptions to ensure that the single step will
- * be for the instruction we have the kprobe on, if we dont its
- * possible we'd get the single step reported for an exception handler
- * like Decrementer or External Interrupt */
- regs->msr &= ~MSR_EE;
- regs->msr |= MSR_SINGLESTEP;
-#ifdef CONFIG_PPC_ADV_DEBUG_REGS
- regs->msr &= ~MSR_CE;
- mtspr(SPRN_DBCR0, mfspr(SPRN_DBCR0) | DBCR0_IC | DBCR0_IDM);
-#ifdef CONFIG_PPC_47x
- isync();
-#endif
-#endif
-
+ enable_single_step(regs);
/*
* On powerpc we should single step on the original
* instruction even if the probed insn is a trap
From: Suzuki K. Poulose <[email protected]>
Replace the ptrace helpers with the powerpc generic routines to
enable/disable single step. We save/restore the MSR (and DCBR for BookE)
across for the operation.
Signed-off-by: Suzuki K. Poulose <[email protected]>
---
arch/powerpc/include/asm/uprobes.h | 4 ++++
arch/powerpc/kernel/uprobes.c | 11 +++++++++--
2 files changed, 13 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/include/asm/uprobes.h b/arch/powerpc/include/asm/uprobes.h
index b532060..884be93 100644
--- a/arch/powerpc/include/asm/uprobes.h
+++ b/arch/powerpc/include/asm/uprobes.h
@@ -43,6 +43,10 @@ struct arch_uprobe {
struct arch_uprobe_task {
unsigned long saved_trap_nr;
+ unsigned long saved_msr;
+#ifdef CONFIG_PPC_ADV_DEBUG_REGS
+ unsigned long saved_dbcr;
+#endif
};
extern int arch_uprobe_analyze_insn(struct arch_uprobe *aup, struct mm_struct *mm, unsigned long addr);
diff --git a/arch/powerpc/kernel/uprobes.c b/arch/powerpc/kernel/uprobes.c
index bc77834..c407c07 100644
--- a/arch/powerpc/kernel/uprobes.c
+++ b/arch/powerpc/kernel/uprobes.c
@@ -62,10 +62,14 @@ int arch_uprobe_pre_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
struct arch_uprobe_task *autask = ¤t->utask->autask;
autask->saved_trap_nr = current->thread.trap_nr;
+ autask->saved_msr = regs->msr;
+#ifdef CONFIG_PPC_ADV_DEBUG_REGS
+ autask->saved_dbcr = mfspr(SPRN_DBCR0);
+#endif
current->thread.trap_nr = UPROBE_TRAP_NR;
regs->nip = current->utask->xol_vaddr;
- user_enable_single_step(current);
+ enable_single_step(regs);
return 0;
}
@@ -121,8 +125,11 @@ int arch_uprobe_post_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
* to be executed.
*/
regs->nip = utask->vaddr + MAX_UINSN_BYTES;
+ regs->msr = utask->autask.saved_msr;
+#ifdef CONFIG_PPC_ADV_DEBUG_REGS
+ mtspr(SPRN_DBCR0, utask->autask.saved_dbcr);
+#endif
- user_disable_single_step(current);
return 0;
}
On 11/26/2012 12:05 PM, Suzuki K. Poulose wrote:
> diff --git a/arch/powerpc/include/asm/probes.h b/arch/powerpc/include/asm/probes.h
> index 5f1e15b..836e9b9 100644
> --- a/arch/powerpc/include/asm/probes.h
> +++ b/arch/powerpc/include/asm/probes.h
> @@ -38,5 +38,34 @@ typedef u32 ppc_opcode_t;
> #define is_trap(instr) (IS_TW(instr) || IS_TWI(instr))
> #endif /* CONFIG_PPC64 */
>
> +#ifdef CONFIG_PPC_ADV_DEBUG_REGS
> +#define MSR_SINGLESTEP (MSR_DE)
> +#else
> +#define MSR_SINGLESTEP (MSR_SE)
> +#endif
> +
> +/* Enable single stepping for the current task */
> +static inline void enable_single_step(struct pt_regs *regs)
> +{
> +
> + /*
> + * We turn off async exceptions to ensure that the single step will
> + * be for the instruction we have the kprobe on, if we dont its
it is
> + * possible we'd get the single step reported for an exception handler
> + * like Decrementer or External Interrupt
> + */
Hmmm. The TRM for E400 says
|5.11.1 e500 Exception Priorities
|The following is a prioritized listing of e500 exceptions:
| 4. Critical input
| 5. Debug interrupt
| 6. External input
| 22. Decrementer
The list has been cut down a little. That means the debug interrupt
comes before external interrupt and before the decrement fires.
And if single step is what wakes you up then DBSR[ICMP] is set. Am I
missing something or is this FSL only not not book-e
> #endif /* __KERNEL__ */
> #endif /* _ASM_POWERPC_PROBES_H */
Sebastian
On 11/26, Suzuki K. Poulose wrote:
>
> @@ -121,8 +125,11 @@ int arch_uprobe_post_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
> * to be executed.
> */
> regs->nip = utask->vaddr + MAX_UINSN_BYTES;
> + regs->msr = utask->autask.saved_msr;
> +#ifdef CONFIG_PPC_ADV_DEBUG_REGS
> + mtspr(SPRN_DBCR0, utask->autask.saved_dbcr);
> +#endif
>
> - user_disable_single_step(current);
Don't we need the same change in arch_uprobe_abort_xol() ?
Oleg.
On 11/26/2012 10:31 PM, Oleg Nesterov wrote:
> On 11/26, Suzuki K. Poulose wrote:
>>
>> @@ -121,8 +125,11 @@ int arch_uprobe_post_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
>> * to be executed.
>> */
>> regs->nip = utask->vaddr + MAX_UINSN_BYTES;
>> + regs->msr = utask->autask.saved_msr;
>> +#ifdef CONFIG_PPC_ADV_DEBUG_REGS
>> + mtspr(SPRN_DBCR0, utask->autask.saved_dbcr);
>> +#endif
>>
>> - user_disable_single_step(current);
>
> Don't we need the same change in arch_uprobe_abort_xol() ?
Yes, we do. Thanks for catching that. I will fix it.
Thanks for the review.
Suzuki
On 11/26/2012 11:40 PM, Sebastian Andrzej Siewior wrote:
> On 11/26/2012 12:05 PM, Suzuki K. Poulose wrote:
>> diff --git a/arch/powerpc/include/asm/probes.h
>> b/arch/powerpc/include/asm/probes.h
>> index 5f1e15b..836e9b9 100644
>> --- a/arch/powerpc/include/asm/probes.h
>> +++ b/arch/powerpc/include/asm/probes.h
>> @@ -38,5 +38,34 @@ typedef u32 ppc_opcode_t;
>> #define is_trap(instr) (IS_TW(instr) || IS_TWI(instr))
>> #endif /* CONFIG_PPC64 */
>>
>> +#ifdef CONFIG_PPC_ADV_DEBUG_REGS
>> +#define MSR_SINGLESTEP (MSR_DE)
>> +#else
>> +#define MSR_SINGLESTEP (MSR_SE)
>> +#endif
>> +
>> +/* Enable single stepping for the current task */
>> +static inline void enable_single_step(struct pt_regs *regs)
>> +{
>> +
>> + /*
>> + * We turn off async exceptions to ensure that the single step will
>> + * be for the instruction we have the kprobe on, if we dont its
> it is
>
>> + * possible we'd get the single step reported for an exception
>> handler
>> + * like Decrementer or External Interrupt
>> + */
>
> Hmmm. The TRM for E400 says
>
> |5.11.1 e500 Exception Priorities
> |The following is a prioritized listing of e500 exceptions:
> | 4. Critical input
> | 5. Debug interrupt
> | 6. External input
> | 22. Decrementer
>
> The list has been cut down a little. That means the debug interrupt
> comes before external interrupt and before the decrement fires.
>
> And if single step is what wakes you up then DBSR[ICMP] is set. Am I
> missing something or is this FSL only not not book-e
>
You are right. The same priority applies for Book3S as well. The above
code/comment was initially written for kprobe. So I didn't bother to
double check the same, when I moved it to the common place.
I will send a v2 with the required changes.
Thanks for the question !
Cheers
Suzuki