[PATCH] target/openrisc: Set EPCR to next PC on FPE exceptions

[PATCH] target/openrisc: Set EPCR to next PC on FPE exceptions

[Stafford Horne]

The architecture specification calls for the EPCR to be set to "Address
of next not executed instruction" when there is a floating point
exception (FPE).  This was not being done, so fix it by using the same
method as syscall.  Note, this may need a lot more work if we start
seeing floating point operations in delay slots which exceptions
enabled.

Without this patch FPU exceptions will loop, as the exception hanlding
will always return back to the failed floating point instruction.

This was not noticed in earlier testing because:

 1. The compiler usually generates code which clobbers the input operand
    such as:

      lf.div.s r19,r17,r19

 2. The target will store the operation output before to the register
    before handling the exception.  So an operation such as:

      float a = 100.0f;
      float b = 0.0f;
      float c = a / b;    /* lf.div.s r19,r17,r19 */

    Will first execute:

      100 / 0    -> Store inf to c (r19)
                 -> triggering divide by zero exception
                 -> handle and return

    Then it will exectute:

      100 / inf  -> Store 0 to c  (no exception)

To confirm the looping behavoid and the fix I used the following:

    float fpu_div(float a, float b) {
	float c;
	asm volatile("lf.div.s %0, %1, %2"
		      : "+r" (c)
		      : "r" (a), "r" (b));
	return c;
    }

Signed-off-by: Stafford Horne <shorne@gmail.com>
---
 target/openrisc/interrupt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/openrisc/interrupt.c b/target/openrisc/interrupt.c
index 3887812810..9b14b8a2c6 100644
--- a/target/openrisc/interrupt.c
+++ b/target/openrisc/interrupt.c
@@ -34,7 +34,7 @@ void openrisc_cpu_do_interrupt(CPUState *cs)
     int exception = cs->exception_index;
 
     env->epcr = env->pc;
-    if (exception == EXCP_SYSCALL) {
+    if (exception == EXCP_SYSCALL || exception == EXCP_FPE) {
         env->epcr += 4;
     }
     /* When we have an illegal instruction the error effective address
-- 
2.39.1

Re: [PATCH] target/openrisc: Set EPCR to next PC on FPE exceptions

[Stafford Horne]

On Sat, Jul 29, 2023 at 10:08:51PM +0100, Stafford Horne wrote:
> The architecture specification calls for the EPCR to be set to "Address
> of next not executed instruction" when there is a floating point
> exception (FPE).  This was not being done, so fix it by using the same
> method as syscall.  Note, this may need a lot more work if we start
> seeing floating point operations in delay slots which exceptions
> enabled.

It should be "with exceptions enabled."

> 
> Without this patch FPU exceptions will loop, as the exception hanlding

"handling"

> will always return back to the failed floating point instruction.
> 
> This was not noticed in earlier testing because:
> 
>  1. The compiler usually generates code which clobbers the input operand
>     such as:
> 
>       lf.div.s r19,r17,r19
> 
>  2. The target will store the operation output before to the register
>     before handling the exception.  So an operation such as:
> 
>       float a = 100.0f;
>       float b = 0.0f;
>       float c = a / b;    /* lf.div.s r19,r17,r19 */
> 
>     Will first execute:
> 
>       100 / 0    -> Store inf to c (r19)
>                  -> triggering divide by zero exception
>                  -> handle and return
> 
>     Then it will exectute:
> 
>       100 / inf  -> Store 0 to c  (no exception)
> 
> To confirm the looping behavoid and the fix I used the following:

"behavior"

>     float fpu_div(float a, float b) {
> 	float c;
> 	asm volatile("lf.div.s %0, %1, %2"
> 		      : "+r" (c)
> 		      : "r" (a), "r" (b));
> 	return c;
>     }
> 
> Signed-off-by: Stafford Horne <shorne@gmail.com>

-Stafford

> ---
>  target/openrisc/interrupt.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/openrisc/interrupt.c b/target/openrisc/interrupt.c
> index 3887812810..9b14b8a2c6 100644
> --- a/target/openrisc/interrupt.c
> +++ b/target/openrisc/interrupt.c
> @@ -34,7 +34,7 @@ void openrisc_cpu_do_interrupt(CPUState *cs)
>      int exception = cs->exception_index;
>  
>      env->epcr = env->pc;
> -    if (exception == EXCP_SYSCALL) {
> +    if (exception == EXCP_SYSCALL || exception == EXCP_FPE) {
>          env->epcr += 4;
>      }
>      /* When we have an illegal instruction the error effective address
> -- 
> 2.39.1
> 

Re: [PATCH] target/openrisc: Set EPCR to next PC on FPE exceptions

[Richard Henderson]

On 7/29/23 14:08, Stafford Horne wrote:
> The architecture specification calls for the EPCR to be set to "Address
> of next not executed instruction" when there is a floating point
> exception (FPE).  This was not being done, so fix it by using the same
> method as syscall.  Note, this may need a lot more work if we start
> seeing floating point operations in delay slots which exceptions
> enabled.
> 
> Without this patch FPU exceptions will loop, as the exception hanlding
> will always return back to the failed floating point instruction.
> 
> This was not noticed in earlier testing because:
> 
>   1. The compiler usually generates code which clobbers the input operand
>      such as:
> 
>        lf.div.s r19,r17,r19
> 
>   2. The target will store the operation output before to the register
>      before handling the exception.  So an operation such as:
> 
>        float a = 100.0f;
>        float b = 0.0f;
>        float c = a / b;    /* lf.div.s r19,r17,r19 */
> 
>      Will first execute:
> 
>        100 / 0    -> Store inf to c (r19)
>                   -> triggering divide by zero exception
>                   -> handle and return
> 
>      Then it will exectute:
> 
>        100 / inf  -> Store 0 to c  (no exception)
> 
> To confirm the looping behavoid and the fix I used the following:
> 
>      float fpu_div(float a, float b) {
> 	float c;
> 	asm volatile("lf.div.s %0, %1, %2"
> 		      : "+r" (c)
> 		      : "r" (a), "r" (b));
> 	return c;
>      }
> 
> Signed-off-by: Stafford Horne <shorne@gmail.com>
> ---
>   target/openrisc/interrupt.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/openrisc/interrupt.c b/target/openrisc/interrupt.c
> index 3887812810..9b14b8a2c6 100644
> --- a/target/openrisc/interrupt.c
> +++ b/target/openrisc/interrupt.c
> @@ -34,7 +34,7 @@ void openrisc_cpu_do_interrupt(CPUState *cs)
>       int exception = cs->exception_index;
>   
>       env->epcr = env->pc;
> -    if (exception == EXCP_SYSCALL) {
> +    if (exception == EXCP_SYSCALL || exception == EXCP_FPE) {
>           env->epcr += 4;
>       }
>       /* When we have an illegal instruction the error effective address

According to Table 6-3, when in a delay slot the EPCR should be the address of the jump, 
for both syscall and fpe.  This whole block should be moved down...

>     /* Set/clear dsx to indicate if we are in a delay slot exception.  */
>     if (env->dflag) {
>         env->dflag = 0;
>         env->sr |= SR_DSX;
>         env->epcr -= 4;
>     } else {
>         env->sr &= ~SR_DSX;
>     }

... into the else.

With that,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH] target/openrisc: Set EPCR to next PC on FPE exceptions

[Stafford Horne]

On Sun, Jul 30, 2023 at 10:43:45AM -0700, Richard Henderson wrote:
> On 7/29/23 14:08, Stafford Horne wrote:
> > The architecture specification calls for the EPCR to be set to "Address
> > of next not executed instruction" when there is a floating point
> > exception (FPE).  This was not being done, so fix it by using the same
> > method as syscall.  Note, this may need a lot more work if we start
> > seeing floating point operations in delay slots which exceptions
> > enabled.
> > 
> > Without this patch FPU exceptions will loop, as the exception hanlding
> > will always return back to the failed floating point instruction.
> > 
> > This was not noticed in earlier testing because:
> > 
> >   1. The compiler usually generates code which clobbers the input operand
> >      such as:
> > 
> >        lf.div.s r19,r17,r19
> > 
> >   2. The target will store the operation output before to the register
> >      before handling the exception.  So an operation such as:
> > 
> >        float a = 100.0f;
> >        float b = 0.0f;
> >        float c = a / b;    /* lf.div.s r19,r17,r19 */
> > 
> >      Will first execute:
> > 
> >        100 / 0    -> Store inf to c (r19)
> >                   -> triggering divide by zero exception
> >                   -> handle and return
> > 
> >      Then it will exectute:
> > 
> >        100 / inf  -> Store 0 to c  (no exception)
> > 
> > To confirm the looping behavoid and the fix I used the following:
> > 
> >      float fpu_div(float a, float b) {
> > 	float c;
> > 	asm volatile("lf.div.s %0, %1, %2"
> > 		      : "+r" (c)
> > 		      : "r" (a), "r" (b));
> > 	return c;
> >      }
> > 
> > Signed-off-by: Stafford Horne <shorne@gmail.com>
> > ---
> >   target/openrisc/interrupt.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/target/openrisc/interrupt.c b/target/openrisc/interrupt.c
> > index 3887812810..9b14b8a2c6 100644
> > --- a/target/openrisc/interrupt.c
> > +++ b/target/openrisc/interrupt.c
> > @@ -34,7 +34,7 @@ void openrisc_cpu_do_interrupt(CPUState *cs)
> >       int exception = cs->exception_index;
> >       env->epcr = env->pc;
> > -    if (exception == EXCP_SYSCALL) {
> > +    if (exception == EXCP_SYSCALL || exception == EXCP_FPE) {
> >           env->epcr += 4;
> >       }
> >       /* When we have an illegal instruction the error effective address
> 
> According to Table 6-3, when in a delay slot the EPCR should be the address
> of the jump, for both syscall and fpe.  This whole block should be moved
> down...
> 
> >     /* Set/clear dsx to indicate if we are in a delay slot exception.  */
> >     if (env->dflag) {
> >         env->dflag = 0;
> >         env->sr |= SR_DSX;
> >         env->epcr -= 4;
> >     } else {
> >         env->sr &= ~SR_DSX;
> >     }
> 
> ... into the else.

Thanks for looking at this.  This is correct.  I will fix it this way, though
this points to an issue in the architecture (in the manual as "Particular delay
slot issues").  If we have instructions causing exceptions in delay slots like
l.sys it will cause looping.

> With that,
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

Thank you.

-Stafford