Re: [PATCH] OpenRISC: Handle r0 with care

[Jonas Bonn <jonas@southpole.se>]

[-- Attachment #1: Type: text/plain, Size: 2670 bytes --]

Hi Richard,

On Mon, 2012-03-05 at 22:07 +0100, Richard Weinberger wrote:
> Depending on the OpenRISC implementation a rough task may able
> to change r0 and corrupt other taks.
> Handle this case by setting r0 to zero on each entry point.
> Also ensure that r0 is really zero before jumping into _start.
> 
> Signed-off-by: Richard Weinberger <richard@nod.at>
> 

Given the difficulty that was expressed on IRC to understand that this
was a real problem, I think a longer explanation is in order here.  In
particular, the "hardware" people should be able to read this and get a
feeling for the implications of having a writable r0.

> diff --git a/arch/openrisc/kernel/entry.S b/arch/openrisc/kernel/entry.S
> index d5f9c35..7c9c4f6 100644
> --- a/arch/openrisc/kernel/entry.S
> +++ b/arch/openrisc/kernel/entry.S
> @@ -130,6 +130,7 @@ handler:							;\
>  #define UNHANDLED_EXCEPTION(handler,vector)			\
>  	.global	handler						;\
>  handler:							;\
> +	l.andi	r0,r0,0						;\
>  	/* r1, EPCR, ESR already saved */			;\
>  	l.sw    PT_GPR2(r1),r2					;\
>  	l.sw    PT_GPR3(r1),r3					;\
> @@ -185,8 +186,8 @@ handler:							;\
>  /* ---[ 0x100: RESET exception ]----------------------------------------- */

If you're clearing r0 in EXCEPTION_HANDLE in head.S, then you probably
don't need to clear it again here... this should be in the same
execution path, I think.

>  
>  EXCEPTION_ENTRY(_tng_kernel_start)
> +	l.andi r0,r0,0
>  	l.jal	_start
> -	 l.andi r0,r0,0
>  

No, that was already correct.  The delay slot (indented one space for
clarity) is executed before the jump instruction.

>  /* ---[ 0x200: BUS exception ]------------------------------------------- */
>  
> @@ -976,6 +977,7 @@ ENTRY(_kernel_thread_helper)
>  
>  	.align 0x400
>  ENTRY(_switch)
> +	l.andi	r0,r0,0
>  	/* We don't store SR as _switch only gets called in a context where
>  	 * the SR will be the same going in and coming out... */
>  

I'm scratching my head a bit on this one... why do we need to clear r0
here?

> diff --git a/arch/openrisc/kernel/head.S b/arch/openrisc/kernel/head.S
> index c75018d..c439324 100644
> --- a/arch/openrisc/kernel/head.S
> +++ b/arch/openrisc/kernel/head.S
> @@ -152,6 +152,7 @@
>   */
>  
>  #define EXCEPTION_HANDLE(handler)				\
> +	l.andi	r0,r0,0						;\
>  	EXCEPTION_T_STORE_GPR30					;\
>  	l.mfspr r30,r0,SPR_ESR_BASE				;\
>  	l.andi  r30,r30,SPR_SR_SM				;\

Doing the same thing to UNHANDLED_EXCEPTION in head.S seems to me like
the right to do... it's moot, as it's unhandled, but it would be nice to
have that path be 'correct', too.

Thanks,
Jonas


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]