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

[Peter Maydell <peter.maydell@linaro.org>]

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