[PATCH v2 0/4] uprobes/powerpc: Replace ptrace helpers for single stepping

[PATCH v2 0/4] uprobes/powerpc: Replace ptrace helpers for single stepping

[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).

Changes since V1: 

 * Don't disable external interrupts. (Sebastian)
 * Introduced routines for saving/restoring the context for sstep.
 * Restore the context in arch_uprobe_abort_xol() (Oleg)


---

Suzuki K. Poulose (4):
      kprobes/powerpc: Do not disable External interrupts during single step
      powerpc: Move the single step enable code to a generic path
      uprobes/powerpc: Introduce routines for save/restore context
      uprobes/powerpc: Make use of generic routines to enable single step


 arch/powerpc/include/asm/probes.h  |   25 +++++++++++++++++++++++++
 arch/powerpc/include/asm/uprobes.h |    4 ++++
 arch/powerpc/kernel/kprobes.c      |   21 +--------------------
 arch/powerpc/kernel/uprobes.c      |   32 +++++++++++++++++++++++++-------
 4 files changed, 55 insertions(+), 27 deletions(-)

-- 
Suzuki

[PATCH v2 1/4] kprobes/powerpc: Do not disable External interrupts during single step

[Suzuki K. Poulose]

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

External/Decrement exceptions have lower priority than the Debug Exception.
So, we don't have to disable the External interrupts before a single step.
However, on BookE, Critical Input Exception(CE) has higher priority than a
Debug Exception. Hence we mask them.

Signed-off-by: 	Suzuki K. Poulose <suzuki@in.ibm.com>
Cc:		Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc:		Ananth N Mavinakaynahalli <ananth@in.ibm.com>
Cc:		Kumar Gala <galak@kernel.crashing.org>
Cc:		linuxppc-dev@ozlabs.org
---
 arch/powerpc/kernel/kprobes.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
index e88c643..4901b34 100644
--- a/arch/powerpc/kernel/kprobes.c
+++ b/arch/powerpc/kernel/kprobes.c
@@ -104,13 +104,13 @@ 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
+	/* 
+	 * We turn off Critical Input Exception(CE) to ensure that the single
+	 * step will be for the instruction we have the probe on; if we don't,
+	 * it is possible we'd get the single step reported for CE.
+	 */
 	regs->msr &= ~MSR_CE;
 	mtspr(SPRN_DBCR0, mfspr(SPRN_DBCR0) | DBCR0_IC | DBCR0_IDM);
 #ifdef CONFIG_PPC_47x

[PATCH v2 2/4] 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 header so that, it can be re-used by other code, in this case,
uprobes. No functional changes.

Signed-off-by: Suzuki K. Poulose <suzuki@in.ibm.com>
Cc:	Ananth N Mavinakaynahalli <ananth@in.ibm.com>
Cc:	Kumar Gala <galak@kernel.crashing.org>
Cc:	linuxppc-dev@ozlabs.org
---
 arch/powerpc/include/asm/probes.h |   25 +++++++++++++++++++++++++
 arch/powerpc/kernel/kprobes.c     |   21 +--------------------
 2 files changed, 26 insertions(+), 20 deletions(-)

diff --git a/arch/powerpc/include/asm/probes.h b/arch/powerpc/include/asm/probes.h
index 5f1e15b..f94a44f 100644
--- a/arch/powerpc/include/asm/probes.h
+++ b/arch/powerpc/include/asm/probes.h
@@ -38,5 +38,30 @@ 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)
+{
+	regs->msr |= MSR_SINGLESTEP;
+#ifdef CONFIG_PPC_ADV_DEBUG_REGS
+	/* 
+	 * We turn off Critical Input Exception(CE) to ensure that the single
+	 * step will be for the instruction we have the probe on; if we don't,
+	 * it is possible we'd get the single step reported for CE.
+	 */
+	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 4901b34..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)
 {
-	regs->msr |= MSR_SINGLESTEP;
-#ifdef CONFIG_PPC_ADV_DEBUG_REGS
-	/* 
-	 * We turn off Critical Input Exception(CE) to ensure that the single
-	 * step will be for the instruction we have the probe on; if we don't,
-	 * it is possible we'd get the single step reported for CE.
-	 */
-	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

[PATCH v2 3/4] uprobes/powerpc: Introduce routines for save/restore context

[Suzuki K. Poulose]

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

Introduce routines for saving and restoring the context
befre/after the single step. No functional changes involved.

These will be extended later to save/restore more info about
the process once we replace the ptrace helpers.

Signed-off-by: Suzuki K. Poulose <suzuki@in.ibm.com>
---
 arch/powerpc/kernel/uprobes.c |   16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/kernel/uprobes.c b/arch/powerpc/kernel/uprobes.c
index bc77834..1a62353 100644
--- a/arch/powerpc/kernel/uprobes.c
+++ b/arch/powerpc/kernel/uprobes.c
@@ -52,6 +52,16 @@ int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe,
 	return 0;
 }
 
+static void uprobe_save_context_sstep(struct arch_uprobe_task *autask)
+{
+	autask->saved_trap_nr = current->thread.trap_nr;
+}
+
+static void uprobe_restore_context_sstep(struct arch_uprobe_task *autask)
+{
+	current->thread.trap_nr = autask->saved_trap_nr;
+}
+
 /*
  * arch_uprobe_pre_xol - prepare to execute out of line.
  * @auprobe: the probepoint information.
@@ -61,7 +71,7 @@ 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;
+	uprobe_save_context_sstep(autask);
 	current->thread.trap_nr = UPROBE_TRAP_NR;
 	regs->nip = current->utask->xol_vaddr;
 
@@ -111,7 +121,7 @@ int arch_uprobe_post_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
 
 	WARN_ON_ONCE(current->thread.trap_nr != UPROBE_TRAP_NR);
 
-	current->thread.trap_nr = utask->autask.saved_trap_nr;
+	uprobe_restore_context_sstep(&utask->autask);
 
 	/*
 	 * On powerpc, except for loads and stores, most instructions
@@ -164,7 +174,7 @@ void arch_uprobe_abort_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
 {
 	struct uprobe_task *utask = current->utask;
 
-	current->thread.trap_nr = utask->autask.saved_trap_nr;
+	uprobe_restore_context_sstep(&utask->autask);
 	instruction_pointer_set(regs, utask->vaddr);
 
 	user_disable_single_step(current);

[PATCH v2 4/4] 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. We don't have to disable the single step,
as restoring the MSR/DBCR would restore the previous state.

Signed-off-by: Suzuki K. Poulose <suzuki@in.ibm.com>
---
 arch/powerpc/include/asm/uprobes.h |    4 ++++
 arch/powerpc/kernel/uprobes.c      |   26 +++++++++++++++++---------
 2 files changed, 21 insertions(+), 9 deletions(-)

diff --git a/arch/powerpc/include/asm/uprobes.h b/arch/powerpc/include/asm/uprobes.h
index b532060..10a521c 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_dbcr0;
+#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 1a62353..6af55c4 100644
--- a/arch/powerpc/kernel/uprobes.c
+++ b/arch/powerpc/kernel/uprobes.c
@@ -52,14 +52,25 @@ int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe,
 	return 0;
 }
 
-static void uprobe_save_context_sstep(struct arch_uprobe_task *autask)
+static void uprobe_save_context_sstep(struct arch_uprobe_task *autask,
+					struct pt_regs *regs)
 {
 	autask->saved_trap_nr = current->thread.trap_nr;
+	autask->saved_msr = regs->msr;
+#ifdef CONFIG_PPC_ADV_DEBUG_REGS
+	autask->saved_dbcr0 = mfspr(SPRN_DBCR0);
+#endif
 }
 
-static void uprobe_restore_context_sstep(struct arch_uprobe_task *autask)
+static void uprobe_restore_context_sstep(struct arch_uprobe_task *autask,
+						struct pt_regs *regs)
 {
 	current->thread.trap_nr = autask->saved_trap_nr;
+
+	regs->msr = autask->saved_msr;
+#ifdef CONFIG_PPC_ADV_DEBUG_REGS
+	mtspr(SPRN_DBCR0, autask->saved_dbcr0);
+#endif
 }
 
 /*
@@ -71,11 +82,11 @@ int arch_uprobe_pre_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
 {
 	struct arch_uprobe_task *autask = &current->utask->autask;
 
-	uprobe_save_context_sstep(autask);
+	uprobe_save_context_sstep(autask, regs);
 	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,7 +132,7 @@ int arch_uprobe_post_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
 
 	WARN_ON_ONCE(current->thread.trap_nr != UPROBE_TRAP_NR);
 
-	uprobe_restore_context_sstep(&utask->autask);
+	uprobe_restore_context_sstep(&utask->autask, regs);
 
 	/*
 	 * On powerpc, except for loads and stores, most instructions
@@ -132,7 +143,6 @@ int arch_uprobe_post_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
 	 */
 	regs->nip = utask->vaddr + MAX_UINSN_BYTES;
 
-	user_disable_single_step(current);
 	return 0;
 }
 
@@ -174,10 +184,8 @@ void arch_uprobe_abort_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
 {
 	struct uprobe_task *utask = current->utask;
 
-	uprobe_restore_context_sstep(&utask->autask);
+	uprobe_restore_context_sstep(&utask->autask, regs);
 	instruction_pointer_set(regs, utask->vaddr);
-
-	user_disable_single_step(current);
 }
 
 /*

Re: [PATCH v2 3/4] uprobes/powerpc: Introduce routines for save/restore context

[Ananth N Mavinakayanahalli]

On Mon, Dec 03, 2012 at 08:39:35PM +0530, Suzuki K. Poulose wrote:
> From: Suzuki K. Poulose <suzuki@in.ibm.com>
> 
> Introduce routines for saving and restoring the context
> befre/after the single step. No functional changes involved.
> 
> These will be extended later to save/restore more info about
> the process once we replace the ptrace helpers.
> 
> Signed-off-by: Suzuki K. Poulose <suzuki@in.ibm.com>
> ---
>  arch/powerpc/kernel/uprobes.c |   16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/uprobes.c b/arch/powerpc/kernel/uprobes.c
> index bc77834..1a62353 100644
> --- a/arch/powerpc/kernel/uprobes.c
> +++ b/arch/powerpc/kernel/uprobes.c
> @@ -52,6 +52,16 @@ int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe,
>  	return 0;
>  }
> 
> +static void uprobe_save_context_sstep(struct arch_uprobe_task *autask)
> +{
> +	autask->saved_trap_nr = current->thread.trap_nr;
> +}
> +
> +static void uprobe_restore_context_sstep(struct arch_uprobe_task *autask)
> +{
> +	current->thread.trap_nr = autask->saved_trap_nr;
> +}

Can't the two above be inline?

Ananth

Re: [PATCH v2 3/4] uprobes/powerpc: Introduce routines for save/restore context

[Suzuki K. Poulose]

On 12/03/2012 08:45 PM, Ananth N Mavinakayanahalli wrote:
> On Mon, Dec 03, 2012 at 08:39:35PM +0530, Suzuki K. Poulose wrote:
>> From: Suzuki K. Poulose <suzuki@in.ibm.com>
>>
>> Introduce routines for saving and restoring the context
>> befre/after the single step. No functional changes involved.
>>
>> These will be extended later to save/restore more info about
>> the process once we replace the ptrace helpers.
>>
>> Signed-off-by: Suzuki K. Poulose <suzuki@in.ibm.com>
>> ---
>>   arch/powerpc/kernel/uprobes.c |   16 +++++++++++++---
>>   1 file changed, 13 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/powerpc/kernel/uprobes.c b/arch/powerpc/kernel/uprobes.c
>> index bc77834..1a62353 100644
>> --- a/arch/powerpc/kernel/uprobes.c
>> +++ b/arch/powerpc/kernel/uprobes.c
>> @@ -52,6 +52,16 @@ int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe,
>>   	return 0;
>>   }
>>
>> +static void uprobe_save_context_sstep(struct arch_uprobe_task *autask)
>> +{
>> +	autask->saved_trap_nr = current->thread.trap_nr;
>> +}
>> +
>> +static void uprobe_restore_context_sstep(struct arch_uprobe_task *autask)
>> +{
>> +	current->thread.trap_nr = autask->saved_trap_nr;
>> +}
>
> Can't the two above be inline?
I had this discussion with Srikar and he was of the opinion that, we
should leave it as just static and let the compiler do the optimization.


Thanks
Suzuki

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

[Ananth N Mavinakayanahalli]

On Mon, Dec 03, 2012 at 08:38:37PM +0530, Suzuki K. Poulose wrote:
> From: Suzuki K. Poulose <suzuki@in.ibm.com>
> 
> This patch moves the single step enable code used by kprobe to a generic
> routine header so that, it can be re-used by other code, in this case,
> uprobes. No functional changes.
> 
> Signed-off-by: Suzuki K. Poulose <suzuki@in.ibm.com>
> Cc:	Ananth N Mavinakaynahalli <ananth@in.ibm.com>
> Cc:	Kumar Gala <galak@kernel.crashing.org>
> Cc:	linuxppc-dev@ozlabs.org

Acked-by: Ananth N Mavinakayanahalli <ananth@in.ibm.com>

Re: [PATCH v2 3/4] uprobes/powerpc: Introduce routines for save/restore context

[Ananth N Mavinakayanahalli]

On Mon, Dec 03, 2012 at 08:39:35PM +0530, Suzuki K. Poulose wrote:
> From: Suzuki K. Poulose <suzuki@in.ibm.com>
> 
> Introduce routines for saving and restoring the context
> befre/after the single step. No functional changes involved.
> 
> These will be extended later to save/restore more info about
> the process once we replace the ptrace helpers.
> 
> Signed-off-by: Suzuki K. Poulose <suzuki@in.ibm.com>

Acked-by: Ananth N Mavinakayanahalli <ananth@in.ibm.com>

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

[Ananth N Mavinakayanahalli]

On Mon, Dec 03, 2012 at 08:40:32PM +0530, Suzuki K. Poulose wrote:
> 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. We don't have to disable the single step,
> as restoring the MSR/DBCR would restore the previous state.
> 
> Signed-off-by: Suzuki K. Poulose <suzuki@in.ibm.com>

Acked-by: Ananth N Mavinakayanahalli <ananth@in.ibm.com>

Re: [PATCH v2 1/4] kprobes/powerpc: Do not disable External interrupts during single step

[Suzuki K. Poulose]

On 12/03/2012 08:37 PM, Suzuki K. Poulose wrote:
> From: Suzuki K. Poulose <suzuki@in.ibm.com>
>
> External/Decrement exceptions have lower priority than the Debug Exception.
> So, we don't have to disable the External interrupts before a single step.
> However, on BookE, Critical Input Exception(CE) has higher priority than a
> Debug Exception. Hence we mask them.
>
> Signed-off-by: 	Suzuki K. Poulose <suzuki@in.ibm.com>
> Cc:		Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Cc:		Ananth N Mavinakaynahalli <ananth@in.ibm.com>
> Cc:		Kumar Gala <galak@kernel.crashing.org>
> Cc:		linuxppc-dev@ozlabs.org
> ---
>   arch/powerpc/kernel/kprobes.c |   10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
> index e88c643..4901b34 100644
> --- a/arch/powerpc/kernel/kprobes.c
> +++ b/arch/powerpc/kernel/kprobes.c
> @@ -104,13 +104,13 @@ 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
> +	/*
> +	 * We turn off Critical Input Exception(CE) to ensure that the single
> +	 * step will be for the instruction we have the probe on; if we don't,
> +	 * it is possible we'd get the single step reported for CE.
> +	 */
>   	regs->msr &= ~MSR_CE;
>   	mtspr(SPRN_DBCR0, mfspr(SPRN_DBCR0) | DBCR0_IC | DBCR0_IDM);
>   #ifdef CONFIG_PPC_47x
>

Ben, Kumar,

Could you please review this patch ?


Thanks
Suzuki

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

[Oleg Nesterov]

On 12/03, Suzuki K. Poulose wrote:
>
> 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. We don't have to disable the single step,
> as restoring the MSR/DBCR would restore the previous state.

Obviously I can't review this series (although it looks fine to me).

Just one note,

> @@ -121,7 +132,7 @@ int arch_uprobe_post_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
>
>  	WARN_ON_ONCE(current->thread.trap_nr != UPROBE_TRAP_NR);
>
> -	uprobe_restore_context_sstep(&utask->autask);
> +	uprobe_restore_context_sstep(&utask->autask, regs);

I am not sure ppc needs this, but note that x86 does a bit more.

Not only we need to restore the "single-step" state, we need to
send SIGTRAP if it was not set by us. The same for _skip_sstep.

But even if I am right I do not suggest to change this series,
this can be done as a separate patch.

Oleg.

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

[Suzuki K. Poulose]

On 12/15/2012 01:32 AM, Oleg Nesterov wrote:
> On 12/03, Suzuki K. Poulose wrote:
>>
>> 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. We don't have to disable the single step,
>> as restoring the MSR/DBCR would restore the previous state.
>
> Obviously I can't review this series (although it looks fine to me).
>
> Just one note,
>
>> @@ -121,7 +132,7 @@ int arch_uprobe_post_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
>>
>>   	WARN_ON_ONCE(current->thread.trap_nr != UPROBE_TRAP_NR);
>>
>> -	uprobe_restore_context_sstep(&utask->autask);
>> +	uprobe_restore_context_sstep(&utask->autask, regs);
>
> I am not sure ppc needs this, but note that x86 does a bit more.
>
> Not only we need to restore the "single-step" state, we need to
> send SIGTRAP if it was not set by us. The same for _skip_sstep.
>
Ok. I will investigate that part and do the necessary.

Thanks
Suzuki

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

[Ananth N Mavinakayanahalli]

On Fri, Dec 14, 2012 at 09:02:41PM +0100, Oleg Nesterov wrote:
> On 12/03, Suzuki K. Poulose wrote:
> >
> > 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. We don't have to disable the single step,
> > as restoring the MSR/DBCR would restore the previous state.
> 
> Obviously I can't review this series (although it looks fine to me).
> 
> Just one note,
> 
> > @@ -121,7 +132,7 @@ int arch_uprobe_post_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
> >
> >  	WARN_ON_ONCE(current->thread.trap_nr != UPROBE_TRAP_NR);
> >
> > -	uprobe_restore_context_sstep(&utask->autask);
> > +	uprobe_restore_context_sstep(&utask->autask, regs);
> 
> I am not sure ppc needs this, but note that x86 does a bit more.
> 
> Not only we need to restore the "single-step" state, we need to
> send SIGTRAP if it was not set by us. The same for _skip_sstep.

Do you mean restoring the TF equivalent on powerpc to what it was before?

If so, powerpc has always been unique in this aspect -- the single-step
exception handler *always* resets the sstep bit in MSR. Any user needing
to continue single-stepping has to explicitly set it again.

Ananth

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

[Oleg Nesterov]

On 12/18, Ananth N Mavinakayanahalli wrote:
>
> On Fri, Dec 14, 2012 at 09:02:41PM +0100, Oleg Nesterov wrote:
> > >
> > > -	uprobe_restore_context_sstep(&utask->autask);
> > > +	uprobe_restore_context_sstep(&utask->autask, regs);
> >
> > I am not sure ppc needs this, but note that x86 does a bit more.
> >
> > Not only we need to restore the "single-step" state, we need to
> > send SIGTRAP if it was not set by us. The same for _skip_sstep.
>
> Do you mean restoring the TF equivalent on powerpc to what it was before?
>
> If so, powerpc has always been unique in this aspect -- the single-step
> exception handler *always* resets the sstep bit in MSR. Any user needing
> to continue single-stepping has to explicitly set it again.

I meant another thing.

Suppose that, say, gdb tries to single-step over the probed insn.
In this case we need to send SIGTRAP after xol/emulate. Please look at
send_sig(SIGTRAP) in arch/x86/kernel/uprobes.c:arch_uprobe_post_xol()
and arch_uprobe_skip_sstep().

Oleg.

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

[Ananth N Mavinakayanahalli]

On Tue, Dec 18, 2012 at 08:10:13PM +0100, Oleg Nesterov wrote:
> On 12/18, Ananth N Mavinakayanahalli wrote:
> >
> > On Fri, Dec 14, 2012 at 09:02:41PM +0100, Oleg Nesterov wrote:
> > > >
> > > > -	uprobe_restore_context_sstep(&utask->autask);
> > > > +	uprobe_restore_context_sstep(&utask->autask, regs);
> > >
> > > I am not sure ppc needs this, but note that x86 does a bit more.
> > >
> > > Not only we need to restore the "single-step" state, we need to
> > > send SIGTRAP if it was not set by us. The same for _skip_sstep.
> >
> > Do you mean restoring the TF equivalent on powerpc to what it was before?
> >
> > If so, powerpc has always been unique in this aspect -- the single-step
> > exception handler *always* resets the sstep bit in MSR. Any user needing
> > to continue single-stepping has to explicitly set it again.
> 
> I meant another thing.
> 
> Suppose that, say, gdb tries to single-step over the probed insn.
> In this case we need to send SIGTRAP after xol/emulate. Please look at
> send_sig(SIGTRAP) in arch/x86/kernel/uprobes.c:arch_uprobe_post_xol()
> and arch_uprobe_skip_sstep().

Agreed. Thanks for the clarification Oleg.

Ananth

Re: [PATCH v2 1/4] kprobes/powerpc: Do not disable External interrupts during single step

[Benjamin Herrenschmidt]

On Tue, 2012-12-11 at 11:18 +0530, Suzuki K. Poulose wrote:
> On 12/03/2012 08:37 PM, Suzuki K. Poulose wrote:
> > From: Suzuki K. Poulose <suzuki@in.ibm.com>
> >
> > External/Decrement exceptions have lower priority than the Debug Exception.
> > So, we don't have to disable the External interrupts before a single step.
> > However, on BookE, Critical Input Exception(CE) has higher priority than a
> > Debug Exception. Hence we mask them.

I'm not sure about that one ...

>From memory, 4xx has that interesting issue which is that if you have
single step enabled and an interrupt (of *any kind* occurs), the
processor *will* step into the first instruction of the interrupt
handler. (In fact, some silicons have a bug where it can even be the
*second* instruction of the handler, which can be problematic when the
first one is a branch).

This is why you may notice that whole business we have in the handling
of debug/crit interrupts where we try to figure out if that happened,
and return with DE off if it did.

Now, the above mentioned workaround means we might not need to disable
EE indeed.

However, in any case, I don't see what your patch fixes or improves, nor
do I understand what you mean by "it is possible we'd get the single
step reported for CE". Please explain in more details and describe the
problematic scenario.

Cheers,
Ben.

Re: [PATCH v2 1/4] kprobes/powerpc: Do not disable External interrupts during single step

[Sebastian Andrzej Siewior]

On 01/04/2013 05:42 AM, Benjamin Herrenschmidt wrote:
> On Tue, 2012-12-11 at 11:18 +0530, Suzuki K. Poulose wrote:
>> On 12/03/2012 08:37 PM, Suzuki K. Poulose wrote:
>>> From: Suzuki K. Poulose<suzuki@in.ibm.com>
>>>
>>> External/Decrement exceptions have lower priority than the Debug Exception.
>>> So, we don't have to disable the External interrupts before a single step.
>>> However, on BookE, Critical Input Exception(CE) has higher priority than a
>>> Debug Exception. Hence we mask them.
>
> I'm not sure about that one ...
>
>> From memory, 4xx has that interesting issue which is that if you have
> single step enabled and an interrupt (of *any kind* occurs), the
> processor *will* step into the first instruction of the interrupt
> handler. (In fact, some silicons have a bug where it can even be the
> *second* instruction of the handler, which can be problematic when the
> first one is a branch).
>
> This is why you may notice that whole business we have in the handling
> of debug/crit interrupts where we try to figure out if that happened,
> and return with DE off if it did.
>
> Now, the above mentioned workaround means we might not need to disable
> EE indeed.
>
> However, in any case, I don't see what your patch fixes or improves, nor
> do I understand what you mean by "it is possible we'd get the single
> step reported for CE". Please explain in more details and describe the
> problematic scenario.

This change is probably my fault to some degree so let me explain. I've
been looking over the patch in first place and noticed that Suzuki
disables EE while enabling single stepping. After looking into the
manual I did not find a reason why this is done.

_If_ an external interrupt is pending and we enable EE and DE at the 
same time (via rfi) then we should never land in the external interrupt 
handler but always in the debug exception handler (and EE is disabled on 
all interrupts by the CPU). So why disable EE here?

_If_ the instruction in problem state triggers an DTLB exception then
we land in the TLB exception handler with DE bit set in MSR. I would say 
that this isn't uncommon (same goes probably for the syscall
opcode). After executing the first in instruction in kernel the CPU
should disable the DE (and CE) bit in the MSR and invoke the critical
exception handler. The critical debug exception handler seems to handle
this case. So disable DE, let the previous handler continue and exit to
problem state with DE enabled. From the uprobe point of view, we won't
stop over kernel code but only know once a problem state instruction is
over.

Based on this I did not see a reason why we should disable EE (or CE)
upfront. And for CE, it should be harmless if the code notices that we
debug problem state and continue the non-critical exception with
DE-disabled.

Now, if you come along with some CPU erratas on the 4xx CPUs where we
have to disable CE/EE because the CPU doesn't do what is expected then
I think that this should be explained in the comment :)

> Cheers,
> Ben.

Sebastian