[PATCH/RFC] Change the program check exception from EXC_XFER_STD to EXC_XFER_EE.

[PATCH/RFC] Change the program check exception from EXC_XFER_STD to EXC_XFER_EE.

[Scott Wood]

This patch causes interrupts to be enabled in program_check_exception()
if they were enabled in the faulting context.  This is required because
do_mathemu() calls get_user(), which can sleep, and thus triggers an
assertion if called with interrupts disabled.

Signed-off-by: Scott Wood <scottwood@freescale.com>
---
Is there any reason why ProgramCheck needs to be EXC_XFER_STD?  Has
nobody ever used math emulation with CONFIG_DEBUG_SPINLOCK_SLEEP before?

 arch/powerpc/kernel/head_32.S |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/powerpc/kernel/head_32.S b/arch/powerpc/kernel/head_32.S
index 99507a8..4d58552 100644
--- a/arch/powerpc/kernel/head_32.S
+++ b/arch/powerpc/kernel/head_32.S
@@ -432,7 +432,7 @@ Alignment:
 	EXC_XFER_EE(0x600, alignment_exception)
 
 /* Program check exception */
-	EXCEPTION(0x700, ProgramCheck, program_check_exception, EXC_XFER_STD)
+	EXCEPTION(0x700, ProgramCheck, program_check_exception, EXC_XFER_EE)
 
 /* Floating-point unavailable */
 	. = 0x800
-- 
1.4.2.3

Re: [PATCH/RFC] Change the program check exception from EXC_XFER_STD to EXC_XFER_EE.

[Kumar Gala]

On Dec 6, 2006, at 3:50 PM, Scott Wood wrote:

> This patch causes interrupts to be enabled in  
> program_check_exception()
> if they were enabled in the faulting context.  This is required  
> because
> do_mathemu() calls get_user(), which can sleep, and thus triggers an
> assertion if called with interrupts disabled.
>
> Signed-off-by: Scott Wood <scottwood@freescale.com>
> ---
> Is there any reason why ProgramCheck needs to be EXC_XFER_STD?  Has
> nobody ever used math emulation with CONFIG_DEBUG_SPINLOCK_SLEEP  
> before?

This has come up before and I think our desired solution is to use  
use local_irq_enable() in traps.c.  For some reason we didn't realize  
that we needed to fix the do_mathemu() path.

Take a look at the following threads:
http://lkml.org/lkml/2006/2/22/243
http://ozlabs.org/pipermail/linuxppc-dev/2006-August/025016.html
http://git.kernel.org/git/?p=linux/kernel/git/paulus/ 
powerpc.git;a=commitdiff;h=cd8a5673e9abb3fde0a1c25ee63a60fe1908c6f5

- kumar

Re: [PATCH/RFC] Change the program check exception from EXC_XFER_STD to EXC_XFER_EE.

[Paul Mackerras]

Scott Wood writes:

> Is there any reason why ProgramCheck needs to be EXC_XFER_STD?

Yes.  It used to be EXC_XFER_EE, but I changed it to EXC_XFER_STD
because I didn't want the possibility of preemption on the way from a
kernel breakpoint into a kernel debugger (xmon/kgdb/etc.).

I think the solution to your problem is to rearrange
program_check_exception a little, to make sure we do the
local_irq_enable before calling do_mathemu (but still after the
debugger_bpt call).

Paul.

Re: [PATCH/RFC] Change the program check exception from EXC_XFER_STD to EXC_XFER_EE.

[Kumar Gala]

On Dec 6, 2006, at 4:09 PM, Paul Mackerras wrote:

> Scott Wood writes:
>
>> Is there any reason why ProgramCheck needs to be EXC_XFER_STD?
>
> Yes.  It used to be EXC_XFER_EE, but I changed it to EXC_XFER_STD
> because I didn't want the possibility of preemption on the way from a
> kernel breakpoint into a kernel debugger (xmon/kgdb/etc.).
>
> I think the solution to your problem is to rearrange
> program_check_exception a little, to make sure we do the
> local_irq_enable before calling do_mathemu (but still after the
> debugger_bpt call).

We don't need to rearrange anything, just put the local_irq_enable()  
call in do_mathemu().  If we are a debugger than REASON_TRAP will  
have to be set and we dont bother with do_mathemu() than.

Do you think we should also move the local_irq_enable() that exists  
in the handler into emulate_instruction().

- k

Re: [PATCH/RFC] Change the program check exception from EXC_XFER_STD to EXC_XFER_EE.

[Paul Mackerras]

Kumar Gala writes:

> We don't need to rearrange anything, just put the local_irq_enable()  
> call in do_mathemu().  If we are a debugger than REASON_TRAP will  
> have to be set and we dont bother with do_mathemu() than.
> 
> Do you think we should also move the local_irq_enable() that exists  
> in the handler into emulate_instruction().

I'd rather have the local_irq_enable() calls in
program_check_exception() so they're all in one place, preferably with
a comment explaining why they're there.  Having an unpaired
local_irq_enable in do_mathemu() would seem a bit weird to me.

Paul.

Re: [PATCH/RFC] Change the program check exception from EXC_XFER_STD to EXC_XFER_EE.

[Kumar Gala]

On Fri, 8 Dec 2006, Paul Mackerras wrote:

> Kumar Gala writes:
>
>> We don't need to rearrange anything, just put the local_irq_enable()
>> call in do_mathemu().  If we are a debugger than REASON_TRAP will
>> have to be set and we dont bother with do_mathemu() than.
>>
>> Do you think we should also move the local_irq_enable() that exists
>> in the handler into emulate_instruction().
>
> I'd rather have the local_irq_enable() calls in
> program_check_exception() so they're all in one place, preferably with
> a comment explaining why they're there.  Having an unpaired
> local_irq_enable in do_mathemu() would seem a bit weird to me.

Ok, how about something like this, I'm a bit concerned about what effect 
moving the do_mathemu call will have on things like the 405.

I'll put this in my tree with some other cleanup patches if it looks ok.

- k

diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index fde820e..63073f9 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -792,9 +792,12 @@ #ifdef CONFIG_MATH_EMULATION
  	 * hardware people - not sure if it can happen on any illegal
  	 * instruction or only on FP instructions, whether there is a
  	 * pattern to occurences etc. -dgibson 31/Mar/2003 */
-	if (!(reason & REASON_TRAP) && do_mathemu(regs) == 0) {
-		emulate_single_step(regs);
-		return;
+	if (!(reason & REASON_TRAP)) {
+		local_irq_enable();
+		if (do_mathemu(regs) == 0) {
+			emulate_single_step(regs);
+			return;
+		}
  	}
  #endif /* CONFIG_MATH_EMULATION */

Re: [PATCH/RFC] Change the program check exception from EXC_XFER_STD to EXC_XFER_EE.

[Kumar Gala]

On Dec 8, 2006, at 2:07 AM, Kumar Gala wrote:

>
>
> On Fri, 8 Dec 2006, Paul Mackerras wrote:
>
>> Kumar Gala writes:
>>
>>> We don't need to rearrange anything, just put the local_irq_enable()
>>> call in do_mathemu().  If we are a debugger than REASON_TRAP will
>>> have to be set and we dont bother with do_mathemu() than.
>>>
>>> Do you think we should also move the local_irq_enable() that exists
>>> in the handler into emulate_instruction().
>>
>> I'd rather have the local_irq_enable() calls in
>> program_check_exception() so they're all in one place, preferably  
>> with
>> a comment explaining why they're there.  Having an unpaired
>> local_irq_enable in do_mathemu() would seem a bit weird to me.
>
> Ok, how about something like this, I'm a bit concerned about what  
> effect
> moving the do_mathemu call will have on things like the 405.
>
> I'll put this in my tree with some other cleanup patches if it  
> looks ok.
>
> - k

We never closed on this.

- k

> diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
> index fde820e..63073f9 100644
> --- a/arch/powerpc/kernel/traps.c
> +++ b/arch/powerpc/kernel/traps.c
> @@ -792,9 +792,12 @@ #ifdef CONFIG_MATH_EMULATION
>   	 * hardware people - not sure if it can happen on any illegal
>   	 * instruction or only on FP instructions, whether there is a
>   	 * pattern to occurences etc. -dgibson 31/Mar/2003 */
> -	if (!(reason & REASON_TRAP) && do_mathemu(regs) == 0) {
> -		emulate_single_step(regs);
> -		return;
> +	if (!(reason & REASON_TRAP)) {
> +		local_irq_enable();
> +		if (do_mathemu(regs) == 0) {
> +			emulate_single_step(regs);
> +			return;
> +		}
>   	}
>   #endif /* CONFIG_MATH_EMULATION */
>
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-dev

Re: [PATCH/RFC] Change the program check exception from EXC_XFER_STD to EXC_XFER_EE.

[Kumar Gala]

On Dec 8, 2006, at 2:07 AM, Kumar Gala wrote:

> On Fri, 8 Dec 2006, Paul Mackerras wrote:
>
>> Kumar Gala writes:
>>
>>> We don't need to rearrange anything, just put the local_irq_enable()
>>> call in do_mathemu().  If we are a debugger than REASON_TRAP will
>>> have to be set and we dont bother with do_mathemu() than.
>>>
>>> Do you think we should also move the local_irq_enable() that exists
>>> in the handler into emulate_instruction().
>>
>> I'd rather have the local_irq_enable() calls in
>> program_check_exception() so they're all in one place, preferably  
>> with
>> a comment explaining why they're there.  Having an unpaired
>> local_irq_enable in do_mathemu() would seem a bit weird to me.
>
> Ok, how about something like this, I'm a bit concerned about what  
> effect
> moving the do_mathemu call will have on things like the 405.
>
> I'll put this in my tree with some other cleanup patches if it  
> looks ok.
>
> - k

We never closed on this.

- k

> diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
> index fde820e..63073f9 100644
> --- a/arch/powerpc/kernel/traps.c
> +++ b/arch/powerpc/kernel/traps.c
> @@ -792,9 +792,12 @@ #ifdef CONFIG_MATH_EMULATION
>   	 * hardware people - not sure if it can happen on any illegal
>   	 * instruction or only on FP instructions, whether there is a
>   	 * pattern to occurences etc. -dgibson 31/Mar/2003 */
> -	if (!(reason & REASON_TRAP) && do_mathemu(regs) == 0) {
> -		emulate_single_step(regs);
> -		return;
> +	if (!(reason & REASON_TRAP)) {
> +		local_irq_enable();
> +		if (do_mathemu(regs) == 0) {
> +			emulate_single_step(regs);
> +			return;
> +		}
>   	}
>   #endif /* CONFIG_MATH_EMULATION */
>
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-dev