[PATCH v2 0/3] OpenRISC updates for user space FPU

[PATCH v2 0/3] OpenRISC updates for user space FPU

[Stafford Horne]

Since v1:
 - Fixups suggested by Richard Henderson

This series adds support for the FPU related architecture changes defined in
architecture spec revision v1.4.

 - https://openrisc.io/revisions/r1.4

In summary the architecture changes are:

 - Change FPCSR SPR permissions to allow for reading and writing from user
   space.
 - Clarify that FPU underflow detection is done by detecting tininess before
   rounding.

Previous to this series FPCSR reads and writes from user-mode in QEMU would
throw an illegal argument exception.  The proper behavior should have been to
treat these operations as no-ops as the cpu implementations do.  As mentioned
series changes FPCSR read/write to follow the spec.

The series has been tested with the FPU support added in glibc test suite and
all math tests are passing.

Stafford Horne (3):
  target/openrisc: Allow fpcsr access in user mode
  target/openrisc: Set PC to cpu state on FPU exception
  target/openrisc: Setup FPU for detecting tininess before rounding

 target/openrisc/cpu.c        |  4 ++
 target/openrisc/fpu_helper.c | 13 ++++++-
 target/openrisc/sys_helper.c | 45 +++++++++++++++++-----
 target/openrisc/translate.c  | 72 ++++++++++++++++--------------------
 4 files changed, 82 insertions(+), 52 deletions(-)

-- 
2.39.1

[PATCH v2 1/3] target/openrisc: Allow fpcsr access in user mode

[Stafford Horne]

As per OpenRISC spec 1.4 FPCSR can be read and written in user mode.

Update mtspr and mfspr helpers to support this by moving the is_user
check into the helper.

Link: https://raw.githubusercontent.com/openrisc/doc/master/openrisc-arch-1.4-rev0.pdf
Signed-off-by: Stafford Horne <shorne@gmail.com>
---
Since v1:
 - Update commit message to remove text about no-existant logic change.

 target/openrisc/sys_helper.c | 45 +++++++++++++++++-----
 target/openrisc/translate.c  | 72 ++++++++++++++++--------------------
 2 files changed, 67 insertions(+), 50 deletions(-)

diff --git a/target/openrisc/sys_helper.c b/target/openrisc/sys_helper.c
index ec145960e3..8a0259c710 100644
--- a/target/openrisc/sys_helper.c
+++ b/target/openrisc/sys_helper.c
@@ -29,17 +29,37 @@
 
 #define TO_SPR(group, number) (((group) << 11) + (number))
 
+static inline bool is_user(CPUOpenRISCState *env)
+{
+#ifdef CONFIG_USER_ONLY
+    return true;
+#else
+    return (env->sr & SR_SM) == 0;
+#endif
+}
+
 void HELPER(mtspr)(CPUOpenRISCState *env, target_ulong spr, target_ulong rb)
 {
-#ifndef CONFIG_USER_ONLY
     OpenRISCCPU *cpu = env_archcpu(env);
+#ifndef CONFIG_USER_ONLY
     CPUState *cs = env_cpu(env);
     target_ulong mr;
     int idx;
 #endif
 
+    /* Handle user accessible SPRs first.  */
     switch (spr) {
+    case TO_SPR(0, 20): /* FPCSR */
+        cpu_set_fpcsr(env, rb);
+        return;
+    }
+
+    if (is_user(env)) {
+        raise_exception(cpu, EXCP_ILLEGAL);
+    }
+
 #ifndef CONFIG_USER_ONLY
+    switch (spr) {
     case TO_SPR(0, 11): /* EVBAR */
         env->evbar = rb;
         break;
@@ -187,12 +207,8 @@ void HELPER(mtspr)(CPUOpenRISCState *env, target_ulong spr, target_ulong rb)
         cpu_openrisc_timer_update(cpu);
         qemu_mutex_unlock_iothread();
         break;
-#endif
-
-    case TO_SPR(0, 20): /* FPCSR */
-        cpu_set_fpcsr(env, rb);
-        break;
     }
+#endif
 }
 
 target_ulong HELPER(mfspr)(CPUOpenRISCState *env, target_ulong rd,
@@ -204,10 +220,22 @@ target_ulong HELPER(mfspr)(CPUOpenRISCState *env, target_ulong rd,
     OpenRISCCPU *cpu = env_archcpu(env);
     CPUState *cs = env_cpu(env);
     int idx;
+#else
+    OpenRISCCPU *cpu = env_archcpu(env);
 #endif
 
+    /* Handle user accessible SPRs first.  */
     switch (spr) {
+    case TO_SPR(0, 20): /* FPCSR */
+        return env->fpcsr;
+    }
+
+    if (is_user(env)) {
+        raise_exception(cpu, EXCP_ILLEGAL);
+    }
+
 #ifndef CONFIG_USER_ONLY
+    switch (spr) {
     case TO_SPR(0, 0): /* VR */
         return env->vr;
 
@@ -324,11 +352,8 @@ target_ulong HELPER(mfspr)(CPUOpenRISCState *env, target_ulong rd,
         cpu_openrisc_count_update(cpu);
         qemu_mutex_unlock_iothread();
         return cpu_openrisc_count_get(cpu);
-#endif
-
-    case TO_SPR(0, 20): /* FPCSR */
-        return env->fpcsr;
     }
+#endif
 
     /* for rd is passed in, if rd unchanged, just keep it back.  */
     return rd;
diff --git a/target/openrisc/translate.c b/target/openrisc/translate.c
index 76e53c78d4..43ba0cc1ad 100644
--- a/target/openrisc/translate.c
+++ b/target/openrisc/translate.c
@@ -819,45 +819,12 @@ static bool trans_l_xori(DisasContext *dc, arg_rri *a)
 
 static bool trans_l_mfspr(DisasContext *dc, arg_l_mfspr *a)
 {
-    check_r0_write(dc, a->d);
-
-    if (is_user(dc)) {
-        gen_illegal_exception(dc);
-    } else {
-        TCGv spr = tcg_temp_new();
-
-        if (tb_cflags(dc->base.tb) & CF_USE_ICOUNT) {
-            gen_io_start();
-            if (dc->delayed_branch) {
-                tcg_gen_mov_tl(cpu_pc, jmp_pc);
-                tcg_gen_discard_tl(jmp_pc);
-            } else {
-                tcg_gen_movi_tl(cpu_pc, dc->base.pc_next + 4);
-            }
-            dc->base.is_jmp = DISAS_EXIT;
-        }
+    TCGv spr = tcg_temp_new();
 
-        tcg_gen_ori_tl(spr, cpu_R(dc, a->a), a->k);
-        gen_helper_mfspr(cpu_R(dc, a->d), cpu_env, cpu_R(dc, a->d), spr);
-    }
-    return true;
-}
-
-static bool trans_l_mtspr(DisasContext *dc, arg_l_mtspr *a)
-{
-    if (is_user(dc)) {
-        gen_illegal_exception(dc);
-    } else {
-        TCGv spr;
+    check_r0_write(dc, a->d);
 
-        if (tb_cflags(dc->base.tb) & CF_USE_ICOUNT) {
-            gen_io_start();
-        }
-        /* For SR, we will need to exit the TB to recognize the new
-         * exception state.  For NPC, in theory this counts as a branch
-         * (although the SPR only exists for use by an ICE).  Save all
-         * of the cpu state first, allowing it to be overwritten.
-         */
+    if (tb_cflags(dc->base.tb) & CF_USE_ICOUNT) {
+        gen_io_start();
         if (dc->delayed_branch) {
             tcg_gen_mov_tl(cpu_pc, jmp_pc);
             tcg_gen_discard_tl(jmp_pc);
@@ -865,11 +832,36 @@ static bool trans_l_mtspr(DisasContext *dc, arg_l_mtspr *a)
             tcg_gen_movi_tl(cpu_pc, dc->base.pc_next + 4);
         }
         dc->base.is_jmp = DISAS_EXIT;
+    }
+
+    tcg_gen_ori_tl(spr, cpu_R(dc, a->a), a->k);
+    gen_helper_mfspr(cpu_R(dc, a->d), cpu_env, cpu_R(dc, a->d), spr);
+    return true;
+}
+
+static bool trans_l_mtspr(DisasContext *dc, arg_l_mtspr *a)
+{
+    TCGv spr = tcg_temp_new();
 
-        spr = tcg_temp_new();
-        tcg_gen_ori_tl(spr, cpu_R(dc, a->a), a->k);
-        gen_helper_mtspr(cpu_env, spr, cpu_R(dc, a->b));
+    if (tb_cflags(dc->base.tb) & CF_USE_ICOUNT) {
+        gen_io_start();
     }
+    /*
+     * For SR, we will need to exit the TB to recognize the new
+     * exception state.  For NPC, in theory this counts as a branch
+     * (although the SPR only exists for use by an ICE).  Save all
+     * of the cpu state first, allowing it to be overwritten.
+     */
+    if (dc->delayed_branch) {
+        tcg_gen_mov_tl(cpu_pc, jmp_pc);
+        tcg_gen_discard_tl(jmp_pc);
+    } else {
+        tcg_gen_movi_tl(cpu_pc, dc->base.pc_next + 4);
+    }
+    dc->base.is_jmp = DISAS_EXIT;
+
+    tcg_gen_ori_tl(spr, cpu_R(dc, a->a), a->k);
+    gen_helper_mtspr(cpu_env, spr, cpu_R(dc, a->b));
     return true;
 }
 
-- 
2.39.1

Re: [PATCH v2 1/3] target/openrisc: Allow fpcsr access in user mode

[Richard Henderson]

On 5/10/23 16:32, Stafford Horne wrote:
>   void HELPER(mtspr)(CPUOpenRISCState *env, target_ulong spr, target_ulong rb)
>   {
> -#ifndef CONFIG_USER_ONLY
>       OpenRISCCPU *cpu = env_archcpu(env);
> +#ifndef CONFIG_USER_ONLY
>       CPUState *cs = env_cpu(env);

Pulled cpu out if ifdef here...

> @@ -204,10 +220,22 @@ target_ulong HELPER(mfspr)(CPUOpenRISCState *env, target_ulong rd,
>       OpenRISCCPU *cpu = env_archcpu(env);
>       CPUState *cs = env_cpu(env);
>       int idx;
> +#else
> +    OpenRISCCPU *cpu = env_archcpu(env);
>   #endif

But replicated it here.

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


r~

Re: [PATCH v2 1/3] target/openrisc: Allow fpcsr access in user mode

[Stafford Horne]

On Wed, May 10, 2023 at 05:13:03PM +0100, Richard Henderson wrote:
> On 5/10/23 16:32, Stafford Horne wrote:
> >   void HELPER(mtspr)(CPUOpenRISCState *env, target_ulong spr, target_ulong rb)
> >   {
> > -#ifndef CONFIG_USER_ONLY
> >       OpenRISCCPU *cpu = env_archcpu(env);
> > +#ifndef CONFIG_USER_ONLY
> >       CPUState *cs = env_cpu(env);
> 
> Pulled cpu out if ifdef here...
> 
> > @@ -204,10 +220,22 @@ target_ulong HELPER(mfspr)(CPUOpenRISCState *env, target_ulong rd,
> >       OpenRISCCPU *cpu = env_archcpu(env);
> >       CPUState *cs = env_cpu(env);
> >       int idx;
> > +#else
> > +    OpenRISCCPU *cpu = env_archcpu(env);
> >   #endif
> 
> But replicated it here.

Right, let me make it consistent in this patch.

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

Thank you,

-Stafford

[PATCH v2 2/3] target/openrisc: Set PC to cpu state on FPU exception

[Stafford Horne]

Store the PC to ensure the correct value can be read in the exception
handler.

Signed-off-by: Stafford Horne <shorne@gmail.com>
---
Since v1:
 - Use function do_fpe (similar to do_range) to raise exception.

 target/openrisc/fpu_helper.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/target/openrisc/fpu_helper.c b/target/openrisc/fpu_helper.c
index f9e34fa2cc..8b81d2f62f 100644
--- a/target/openrisc/fpu_helper.c
+++ b/target/openrisc/fpu_helper.c
@@ -20,8 +20,8 @@
 
 #include "qemu/osdep.h"
 #include "cpu.h"
+#include "exec/exec-all.h"
 #include "exec/helper-proto.h"
-#include "exception.h"
 #include "fpu/softfloat.h"
 
 static int ieee_ex_to_openrisc(int fexcp)
@@ -45,6 +45,15 @@ static int ieee_ex_to_openrisc(int fexcp)
     return ret;
 }
 
+static G_NORETURN
+void do_fpe(CPUOpenRISCState *env, uintptr_t pc)
+{
+    CPUState *cs = env_cpu(env);
+
+    cs->exception_index = EXCP_FPE;
+    cpu_loop_exit_restore(cs, pc);
+}
+
 void HELPER(update_fpcsr)(CPUOpenRISCState *env)
 {
     int tmp = get_float_exception_flags(&env->fp_status);
@@ -55,7 +64,7 @@ void HELPER(update_fpcsr)(CPUOpenRISCState *env)
         if (tmp) {
             env->fpcsr |= tmp;
             if (env->fpcsr & FPCSR_FPEE) {
-                helper_exception(env, EXCP_FPE);
+                do_fpe(env, GETPC());
             }
         }
     }
-- 
2.39.1

Re: [PATCH v2 2/3] target/openrisc: Set PC to cpu state on FPU exception

[Richard Henderson]

On 5/10/23 16:32, Stafford Horne wrote:
> Store the PC to ensure the correct value can be read in the exception
> handler.
> 
> Signed-off-by: Stafford Horne<shorne@gmail.com>
> ---
> Since v1:
>   - Use function do_fpe (similar to do_range) to raise exception.
> 
>   target/openrisc/fpu_helper.c | 13 +++++++++++--
>   1 file changed, 11 insertions(+), 2 deletions(-)

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

r~

[PATCH v2 3/3] target/openrisc: Setup FPU for detecting tininess before rounding

[Stafford Horne]

OpenRISC defines tininess to be detected before rounding.  Setup qemu to
obey this.

Signed-off-by: Stafford Horne <shorne@gmail.com>
---
Since v1:
 - Remove setting default NaN behavior.  I discussed with the FPU developers and
   they mentioned the OpenRISC hardware should be IEEE compliant when handling
   and forwarding NaN payloads, and they don't want try change this.

 target/openrisc/cpu.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/target/openrisc/cpu.c b/target/openrisc/cpu.c
index 0ce4f796fa..61d748cfdc 100644
--- a/target/openrisc/cpu.c
+++ b/target/openrisc/cpu.c
@@ -22,6 +22,7 @@
 #include "qemu/qemu-print.h"
 #include "cpu.h"
 #include "exec/exec-all.h"
+#include "fpu/softfloat-helpers.h"
 #include "tcg/tcg.h"
 
 static void openrisc_cpu_set_pc(CPUState *cs, vaddr value)
@@ -90,6 +91,9 @@ static void openrisc_cpu_reset_hold(Object *obj)
     s->exception_index = -1;
     cpu_set_fpcsr(&cpu->env, 0);
 
+    set_float_detect_tininess(float_tininess_before_rounding,
+                              &cpu->env.fp_status);
+
 #ifndef CONFIG_USER_ONLY
     cpu->env.picmr = 0x00000000;
     cpu->env.picsr = 0x00000000;
-- 
2.39.1

Re: [PATCH v2 3/3] target/openrisc: Setup FPU for detecting tininess before rounding

[Richard Henderson]

On 5/10/23 16:32, Stafford Horne wrote:
> OpenRISC defines tininess to be detected before rounding.  Setup qemu to
> obey this.
> 
> Signed-off-by: Stafford Horne <shorne@gmail.com>
> ---
> Since v1:
>   - Remove setting default NaN behavior.  I discussed with the FPU developers and
>     they mentioned the OpenRISC hardware should be IEEE compliant when handling
>     and forwarding NaN payloads, and they don't want try change this.

There is no such thing as IEEE compliant for NaN payloads.
All of that is implementation defined.
All OpenRISC needs to do is document its intentions (and then double-check that 
fpu/softfloat-specialize.c.inc does what is documented).


Anyway, back to this patch,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

:-)


r~

Re: [PATCH v2 3/3] target/openrisc: Setup FPU for detecting tininess before rounding

[Stafford Horne]

On Wed, May 10, 2023 at 05:16:20PM +0100, Richard Henderson wrote:
> On 5/10/23 16:32, Stafford Horne wrote:
> > OpenRISC defines tininess to be detected before rounding.  Setup qemu to
> > obey this.
> > 
> > Signed-off-by: Stafford Horne <shorne@gmail.com>
> > ---
> > Since v1:
> >   - Remove setting default NaN behavior.  I discussed with the FPU developers and
> >     they mentioned the OpenRISC hardware should be IEEE compliant when handling
> >     and forwarding NaN payloads, and they don't want try change this.
> 
> There is no such thing as IEEE compliant for NaN payloads.
> All of that is implementation defined.

I see, I haven't yet seen to IEEE 754 spec so I don't know how much is covered.
It was incorrect to assume forwarding semantics was covered.

> All OpenRISC needs to do is document its intentions (and then double-check
> that fpu/softfloat-specialize.c.inc does what is documented).

Understood, that makes sense, also reading that code I see how all other
architectures are able to ifdef their way to a specific behavior.  I will see
what our current implementions do and update the spec and qemu as a separate
task.

> 
> Anyway, back to this patch,
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> 
> :-)

Thank you ^_^

-Stafford