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

login
register
mail settings
Submitter Suzuki Poulose
Date Dec. 3, 2012, 3:07 p.m.
Message ID <20121203150720.7727.91582.stgit@suzukikp>
Download mbox | patch
Permalink /patch/203378/
State Accepted
Delegated to: Benjamin Herrenschmidt
Headers show

Comments

Suzuki Poulose - Dec. 3, 2012, 3:07 p.m.
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(-)
Suzuki Poulose - Dec. 11, 2012, 5:48 a.m.
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
Benjamin Herrenschmidt - Jan. 4, 2013, 4:42 a.m.
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.
Sebastian Siewior - Jan. 7, 2013, 12:03 p.m.
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

Patch

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