[PULL 0/1] OpenRISC FPU Fix for 8.1

[PULL 0/1] OpenRISC FPU Fix for 8.1

[Stafford Horne]

The following changes since commit ccdd31267678db9d80578b5f80bbe94141609ef4:

  Merge tag 'pull-qapi-2023-07-26-v2' of https://repo.or.cz/qemu/armbru into staging (2023-07-26 07:16:19 -0700)

are available in the Git repository at:

  https://github.com/stffrdhrn/qemu.git tags/or1k-pull-request-20230809

for you to fetch changes up to 765fdc1e8355d4bae563b3b185c5f9d079384164:

  target/openrisc: Set EPCR to next PC on FPE exceptions (2023-07-31 22:01:03 +0100)

----------------------------------------------------------------
OpenRISC FPU Fix for 8.1

A patch to pass the correct exception address when handling floating
point exceptions.

----------------------------------------------------------------
Stafford Horne (1):
      target/openrisc: Set EPCR to next PC on FPE exceptions

 target/openrisc/interrupt.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

[PULL 1/1] 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
pattern as syscall.  Also, we move this logic down to be done for
instructions not in the delay slot as called for by the architecture
manual.

Without this patch FPU exceptions will loop, as the exception 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 execute:

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

To confirm the looping behavior 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;
    }

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Stafford Horne <shorne@gmail.com>
---
 target/openrisc/interrupt.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/target/openrisc/interrupt.c b/target/openrisc/interrupt.c
index 3887812810..d4fdb8ce8e 100644
--- a/target/openrisc/interrupt.c
+++ b/target/openrisc/interrupt.c
@@ -34,9 +34,7 @@ void openrisc_cpu_do_interrupt(CPUState *cs)
     int exception = cs->exception_index;
 
     env->epcr = env->pc;
-    if (exception == EXCP_SYSCALL) {
-        env->epcr += 4;
-    }
+
     /* When we have an illegal instruction the error effective address
        shall be set to the illegal instruction address.  */
     if (exception == EXCP_ILLEGAL) {
@@ -63,6 +61,9 @@ void openrisc_cpu_do_interrupt(CPUState *cs)
         env->epcr -= 4;
     } else {
         env->sr &= ~SR_DSX;
+        if (exception == EXCP_SYSCALL || exception == EXCP_FPE) {
+            env->epcr += 4;
+        }
     }
 
     if (exception > 0 && exception < EXCP_NR) {
-- 
2.39.1

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

[Michael Tokarev]

09.08.2023 23:34, 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
> pattern as syscall.  Also, we move this logic down to be done for
> instructions not in the delay slot as called for by the architecture
> manual.
> 
> Without this patch FPU exceptions will loop, as the exception 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 execute:
> 
>        100 / inf  -> Store 0 to c  (no exception)
> 
> To confirm the looping behavior 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;
>      }

Is it a -stable material?  It applies cleanly to 8.0 and 7.2.
Or maybe it is not needed on older versions, not being noticed before?

/mjt

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

[Stafford Horne]

On Thu, Aug 10, 2023 at 09:35:18AM +0300, Michael Tokarev wrote:
> 09.08.2023 23:34, 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
> > pattern as syscall.  Also, we move this logic down to be done for
> > instructions not in the delay slot as called for by the architecture
> > manual.
> > 
> > Without this patch FPU exceptions will loop, as the exception 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 execute:
> > 
> >        100 / inf  -> Store 0 to c  (no exception)
> > 
> > To confirm the looping behavior 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;
> >      }
> 
> Is it a -stable material?  It applies cleanly to 8.0 and 7.2.
> Or maybe it is not needed on older versions, not being noticed before?

I would say no, it will work on 8.0 an 7.2 but this code path is not very useful
withouth the other 8.1 Floating Point Exception handling updates.

-Stafford

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

[Michael Tokarev]

10.08.2023 22:50, Stafford Horne wrote:
> On Thu, Aug 10, 2023 at 09:35:18AM +0300, Michael Tokarev wrote:
..
>> Is it a -stable material?  It applies cleanly to 8.0 and 7.2.
>> Or maybe it is not needed on older versions, not being noticed before?
> 
> I would say no, it will work on 8.0 an 7.2 but this code path is not very useful
> withouth the other 8.1 Floating Point Exception handling updates.

Thank you for letting me know. This makes good sense, and shares my expectations
too.  This particular situation is rather interesting, that's why I asked.

/mjt

Re: [PULL 0/1] OpenRISC FPU Fix for 8.1

[Richard Henderson]

On 8/9/23 13:34, Stafford Horne wrote:
> The following changes since commit ccdd31267678db9d80578b5f80bbe94141609ef4:
> 
>    Merge tag 'pull-qapi-2023-07-26-v2' ofhttps://repo.or.cz/qemu/armbru  into staging (2023-07-26 07:16:19 -0700)
> 
> are available in the Git repository at:
> 
>    https://github.com/stffrdhrn/qemu.git  tags/or1k-pull-request-20230809
> 
> for you to fetch changes up to 765fdc1e8355d4bae563b3b185c5f9d079384164:
> 
>    target/openrisc: Set EPCR to next PC on FPE exceptions (2023-07-31 22:01:03 +0100)
> 
> ----------------------------------------------------------------
> OpenRISC FPU Fix for 8.1
> 
> A patch to pass the correct exception address when handling floating
> point exceptions.

Applied, thanks.  Please update https://wiki.qemu.org/ChangeLog/8.1 as appropriate.


r~