diff mbox series

[v2,21/30] linux-user/microblaze: Fix SIGFPE si_codes

Message ID 20210822035537.283193-22-richard.henderson@linaro.org
State Superseded
Headers show
Series linux-user: Clean up siginfo_t handling | expand

Commit Message

Richard Henderson Aug. 22, 2021, 3:55 a.m. UTC
Fix a typo for ESR_EC_DIVZERO, which is integral not floating-point.
Fix the if ladder for decoding floating-point exceptions.

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

---
 linux-user/microblaze/cpu_loop.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

-- 
2.25.1

Comments

Peter Maydell Aug. 24, 2021, 4:55 p.m. UTC | #1
On Sun, 22 Aug 2021 at 04:55, Richard Henderson
<richard.henderson@linaro.org> wrote:
>

> Fix a typo for ESR_EC_DIVZERO, which is integral not floating-point.

> Fix the if ladder for decoding floating-point exceptions.

>

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

> ---

>  linux-user/microblaze/cpu_loop.c | 20 +++++++++++++++-----

>  1 file changed, 15 insertions(+), 5 deletions(-)

>

> diff --git a/linux-user/microblaze/cpu_loop.c b/linux-user/microblaze/cpu_loop.c

> index 9e07e52573..4a75c853b2 100644

> --- a/linux-user/microblaze/cpu_loop.c

> +++ b/linux-user/microblaze/cpu_loop.c

> @@ -81,15 +81,25 @@ void cpu_loop(CPUMBState *env)

>              env->iflags &= ~(IMM_FLAG | D_FLAG);

>              switch (env->esr & 31) {

>              case ESR_EC_DIVZERO:

> -                si_code = TARGET_FPE_FLTDIV;

> +                si_code = TARGET_FPE_INTDIV;

>                  break;

>              case ESR_EC_FPU:

> -                si_code = 0;

> -                if (env->fsr & FSR_IO) {

> +                /*

> +                 * Note that the kernel passes along fsr as si_code

> +                 * if there's no recognized bit set.  Possibly this

> +                 * implies that si_code is 0, but follow the structure.

> +                 */


In theory it should: the Microblaze processor reference guide
https://www.xilinx.com/support/documentation/sw_manuals/mb_ref_guide.pdf
defines only 5 bits in the FSR, all of which we look at here.
However our implementation provides two loopholes by which a
high bit might get set:
 * our implementation of MTS rfsr, rX doesn't prevent high bits
   being set by the guest
 * our implementation of gdbstub writes to fsr doesn't prevent
   high bits being set by the guest

I don't know whether the real h/w makes the reserved FSR high
bits RAZ/WI or not; the spec doesn't say either way.

> +                si_code = env->fsr;

> +                if (si_code & FSR_IO) {

>                      si_code = TARGET_FPE_FLTINV;

> -                }

> -                if (env->fsr & FSR_DZ) {

> +                } else if (si_code & FSR_OF) {

> +                    si_code = TARGET_FPE_FLTOVF;

> +                } else if (si_code & FSR_UF) {

> +                    si_code = TARGET_FPE_FLTUND;

> +                } else if (si_code & FSR_DZ) {

>                      si_code = TARGET_FPE_FLTDIV;

> +                } else if (si_code & FSR_DO) {

> +                    si_code = TARGET_FPE_FLTRES;

>                  }

>                  break;

>              default:


Side note: our implementation will never set FSR_DO; we don't
implement the denormal number handling the FPU does, where:
 * operations on input denormals return a QNaN and set FSR.DO
 * output denormals are flushed to + or - zero, setting FSR.UF


Anyway,
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>


thanks
-- PMM
diff mbox series

Patch

diff --git a/linux-user/microblaze/cpu_loop.c b/linux-user/microblaze/cpu_loop.c
index 9e07e52573..4a75c853b2 100644
--- a/linux-user/microblaze/cpu_loop.c
+++ b/linux-user/microblaze/cpu_loop.c
@@ -81,15 +81,25 @@  void cpu_loop(CPUMBState *env)
             env->iflags &= ~(IMM_FLAG | D_FLAG);
             switch (env->esr & 31) {
             case ESR_EC_DIVZERO:
-                si_code = TARGET_FPE_FLTDIV;
+                si_code = TARGET_FPE_INTDIV;
                 break;
             case ESR_EC_FPU:
-                si_code = 0;
-                if (env->fsr & FSR_IO) {
+                /*
+                 * Note that the kernel passes along fsr as si_code
+                 * if there's no recognized bit set.  Possibly this
+                 * implies that si_code is 0, but follow the structure.
+                 */
+                si_code = env->fsr;
+                if (si_code & FSR_IO) {
                     si_code = TARGET_FPE_FLTINV;
-                }
-                if (env->fsr & FSR_DZ) {
+                } else if (si_code & FSR_OF) {
+                    si_code = TARGET_FPE_FLTOVF;
+                } else if (si_code & FSR_UF) {
+                    si_code = TARGET_FPE_FLTUND;
+                } else if (si_code & FSR_DZ) {
                     si_code = TARGET_FPE_FLTDIV;
+                } else if (si_code & FSR_DO) {
+                    si_code = TARGET_FPE_FLTRES;
                 }
                 break;
             default: