[PATCH 0/2] uprobes/powerpc: Replace ptrace single step helpers

[PATCH 0/2] uprobes/powerpc: Replace ptrace single step helpers

[Suzuki K. Poulose]

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

[PATCH 1/2] powerpc: Move the single step enable code to a generic path

[Suzuki K. Poulose]

From: Suzuki K. Poulose <suzuki@in.ibm.com>

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 <suzuki@in.ibm.com>
Cc:	linuxppc-dev@ozlabs.org
---
 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

Re: [PATCH 1/2] powerpc: Move the single step enable code to a generic path

[Sebastian Andrzej Siewior]

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

Re: [PATCH 1/2] powerpc: Move the single step enable code to a generic path

[Suzuki K. Poulose]

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

[PATCH 2/2] uprobes/powerpc: Make use of generic routines to enable single step

[Suzuki K. Poulose]

From: Suzuki K. Poulose <suzuki@in.ibm.com>

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 <suzuki@in.ibm.com>
---
 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 = &current->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;
 }
 

Re: [PATCH 2/2] uprobes/powerpc: Make use of generic routines to enable single step

[Oleg Nesterov]

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.

Re: [PATCH 2/2] uprobes/powerpc: Make use of generic routines to enable single step

[Suzuki K. Poulose]

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