[PATCH v2 0/3] Fix mtfsf, mtfsfi and mtfsb1 bug

[PATCH v2 0/3] Fix mtfsf, mtfsfi and mtfsb1 bug

[Lucas Mateus Castro (alqotel)]

The instructions mtfsf, mtfsfi and mtfsb1, when called, fail to set the FI
bit (bit 46 in the FPSCR) and can set to 1 the reserved bit 52 of the
FPSCR, as reported in https://gitlab.com/qemu-project/qemu/-/issues/266
(although the bug report is only for mtfsf, the bug applies to mtfsfi and
mtfsb1 as well).

These instructions also fail to throw an exception when the exception
and enabling bits are set, this can be tested by adding
'prctl(PR_SET_FPEXC, PR_FP_EXC_PRECISE);' before the __builtin_mtfsf
call in the test case of the bug report.

These patches aim to fix these issues.

Changes from v1:
- added a test for mtfsf (patch 3)
- moved "Resolves" to second patch
- removed gen_reset_fpstatus() from mtfsf,mtfsfi and mtfsb1 instructions

Lucas Mateus Castro (alqotel) (3):
  target/ppc: Fixed call to deferred exception
  target/ppc: ppc_store_fpscr doesn't update bit 52
  test/tcg/ppc64le: test mtfsf

 target/ppc/cpu.c                   |  2 +-
 target/ppc/cpu.h                   |  3 ++
 target/ppc/fpu_helper.c            | 41 ++++++++++++++++++++++
 target/ppc/helper.h                |  1 +
 target/ppc/translate/fp-impl.c.inc |  9 ++---
 tests/tcg/ppc64/Makefile.target    |  1 +
 tests/tcg/ppc64le/Makefile.target  |  1 +
 tests/tcg/ppc64le/mtfsf.c          | 56 ++++++++++++++++++++++++++++++
 8 files changed, 107 insertions(+), 7 deletions(-)
 create mode 100644 tests/tcg/ppc64le/mtfsf.c

-- 
2.31.1

[PATCH v2 1/3] target/ppc: Fixed call to deferred exception

[Lucas Mateus Castro (alqotel)]

mtfsf, mtfsfi and mtfsb1 instructions call helper_float_check_status
after updating the value of FPSCR, but helper_float_check_status
checks fp_status and fp_status isn't updated based on FPSCR and
since the value of fp_status is reset earlier in the instruction,
it's always 0.

Because of this helper_float_check_status would change the FI bit to 0
as this bit checks if the last operation was inexact and
float_flag_inexact is always 0.

These instructions also don't throw exceptions correctly since
helper_float_check_status throw exceptions based on fp_status.

This commit created a new helper, helper_fpscr_check_status that checks
FPSCR value instead of fp_status and checks for a larger variety of
exceptions than do_float_check_status.

Since fp_status isn't used, gen_reset_fpstatus() was removed.

The hardware used to compare QEMU's behavior to was a Power9.

Signed-off-by: Lucas Mateus Castro (alqotel) <lucas.araujo@eldorado.org.br>
---
 target/ppc/fpu_helper.c            | 41 ++++++++++++++++++++++++++++++
 target/ppc/helper.h                |  1 +
 target/ppc/translate/fp-impl.c.inc |  9 +++----
 3 files changed, 45 insertions(+), 6 deletions(-)

diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c
index c4896cecc8..f086cb503f 100644
--- a/target/ppc/fpu_helper.c
+++ b/target/ppc/fpu_helper.c
@@ -414,6 +414,47 @@ void helper_store_fpscr(CPUPPCState *env, uint64_t val, uint32_t nibbles)
     ppc_store_fpscr(env, val);
 }
 
+void helper_fpscr_check_status(CPUPPCState *env)
+{
+    CPUState *cs = env_cpu(env);
+    target_ulong fpscr = env->fpscr;
+    int error = 0;
+
+    if ((fpscr & FP_VXSOFT) && (fpscr_ve != 0)) {
+        error = POWERPC_EXCP_FP_VXSOFT;
+    } else if ((fpscr & FP_OX) && (fpscr & FP_OE)) {
+        error = POWERPC_EXCP_FP_OX;
+    } else if ((fpscr & FP_UX) && (fpscr & FP_UE)) {
+        error = POWERPC_EXCP_FP_UX;
+    } else if ((fpscr & FP_XX) && (fpscr & FP_XE)) {
+        error = POWERPC_EXCP_FP_XX;
+    } else if ((fpscr & FP_ZX) && (fpscr & FP_ZE)) {
+        error = POWERPC_EXCP_FP_ZX;
+    } else if ((fpscr & FP_VXSNAN) && (fpscr_ve != 0)) {
+        error = POWERPC_EXCP_FP_VXSNAN;
+    } else if ((fpscr & FP_VXISI) && (fpscr_ve != 0)) {
+        error = POWERPC_EXCP_FP_VXISI;
+    } else if ((fpscr & FP_VXIDI) && (fpscr_ve != 0)) {
+        error = POWERPC_EXCP_FP_VXIDI;
+    } else if ((fpscr & FP_VXZDZ) && (fpscr_ve != 0)) {
+        error = POWERPC_EXCP_FP_VXZDZ;
+    } else if ((fpscr & FP_VXIMZ) && (fpscr_ve != 0)) {
+        error = POWERPC_EXCP_FP_VXIMZ;
+    } else if ((fpscr & FP_VXVC) && (fpscr_ve != 0)) {
+        error = POWERPC_EXCP_FP_VXVC;
+    }
+
+    if (error) {
+        cs->exception_index = POWERPC_EXCP_PROGRAM;
+        env->error_code = error | POWERPC_EXCP_FP;
+        /* Deferred floating-point exception after target FPSCR update */
+        if (fp_exceptions_enabled(env)) {
+            raise_exception_err_ra(env, cs->exception_index,
+                                   env->error_code, GETPC());
+        }
+    }
+}
+
 static void do_float_check_status(CPUPPCState *env, uintptr_t raddr)
 {
     CPUState *cs = env_cpu(env);
diff --git a/target/ppc/helper.h b/target/ppc/helper.h
index 627811cefc..632a81c676 100644
--- a/target/ppc/helper.h
+++ b/target/ppc/helper.h
@@ -63,6 +63,7 @@ DEF_HELPER_FLAGS_1(cntlzw32, TCG_CALL_NO_RWG_SE, i32, i32)
 DEF_HELPER_FLAGS_2(brinc, TCG_CALL_NO_RWG_SE, tl, tl, tl)
 
 DEF_HELPER_1(float_check_status, void, env)
+DEF_HELPER_1(fpscr_check_status, void, env)
 DEF_HELPER_1(reset_fpstatus, void, env)
 DEF_HELPER_2(compute_fprf_float64, void, env, i64)
 DEF_HELPER_3(store_fpscr, void, env, i64, i32)
diff --git a/target/ppc/translate/fp-impl.c.inc b/target/ppc/translate/fp-impl.c.inc
index d1dbb1b96b..ca195fd9d2 100644
--- a/target/ppc/translate/fp-impl.c.inc
+++ b/target/ppc/translate/fp-impl.c.inc
@@ -769,7 +769,6 @@ static void gen_mtfsb1(DisasContext *ctx)
         return;
     }
     crb = 31 - crbD(ctx->opcode);
-    gen_reset_fpstatus();
     /* XXX: we pretend we can only do IEEE floating-point computations */
     if (likely(crb != FPSCR_FEX && crb != FPSCR_VX && crb != FPSCR_NI)) {
         TCGv_i32 t0;
@@ -782,7 +781,7 @@ static void gen_mtfsb1(DisasContext *ctx)
         tcg_gen_shri_i32(cpu_crf[1], cpu_crf[1], FPSCR_OX);
     }
     /* We can raise a deferred exception */
-    gen_helper_float_check_status(cpu_env);
+    gen_helper_fpscr_check_status(cpu_env);
 }
 
 /* mtfsf */
@@ -803,7 +802,6 @@ static void gen_mtfsf(DisasContext *ctx)
         gen_inval_exception(ctx, POWERPC_EXCP_INVAL_INVAL);
         return;
     }
-    gen_reset_fpstatus();
     if (l) {
         t0 = tcg_const_i32((ctx->insns_flags2 & PPC2_ISA205) ? 0xffff : 0xff);
     } else {
@@ -818,7 +816,7 @@ static void gen_mtfsf(DisasContext *ctx)
         tcg_gen_shri_i32(cpu_crf[1], cpu_crf[1], FPSCR_OX);
     }
     /* We can raise a deferred exception */
-    gen_helper_float_check_status(cpu_env);
+    gen_helper_fpscr_check_status(cpu_env);
     tcg_temp_free_i64(t1);
 }
 
@@ -840,7 +838,6 @@ static void gen_mtfsfi(DisasContext *ctx)
         return;
     }
     sh = (8 * w) + 7 - bf;
-    gen_reset_fpstatus();
     t0 = tcg_const_i64(((uint64_t)FPIMM(ctx->opcode)) << (4 * sh));
     t1 = tcg_const_i32(1 << sh);
     gen_helper_store_fpscr(cpu_env, t0, t1);
@@ -851,7 +848,7 @@ static void gen_mtfsfi(DisasContext *ctx)
         tcg_gen_shri_i32(cpu_crf[1], cpu_crf[1], FPSCR_OX);
     }
     /* We can raise a deferred exception */
-    gen_helper_float_check_status(cpu_env);
+    gen_helper_fpscr_check_status(cpu_env);
 }
 
 static void gen_qemu_ld32fs(DisasContext *ctx, TCGv_i64 dest, TCGv addr)
-- 
2.31.1

[PATCH v2 2/3] target/ppc: ppc_store_fpscr doesn't update bit 52

[Lucas Mateus Castro (alqotel)]

This commit fixes the difference reported in the bug in the reserved
bit 52, it does this by adding this bit to the mask of bits to not be
directly altered in the ppc_store_fpscr function (the hardware used to
compare to QEMU was a Power9).

Although this is a difference reported in the bug, since it's a reserved
bit it may be a "don't care" case, as put in the bug report. Looking at
the ISA it doesn't explicitly mentions this bit can't be set, like it
does for FEX and VX, so I'm unsure if this is necessary.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/266
Signed-off-by: Lucas Mateus Castro (alqotel) <lucas.araujo@eldorado.org.br>
---
 target/ppc/cpu.c | 2 +-
 target/ppc/cpu.h | 3 +++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/target/ppc/cpu.c b/target/ppc/cpu.c
index f933d9f2bd..d7b42bae52 100644
--- a/target/ppc/cpu.c
+++ b/target/ppc/cpu.c
@@ -112,7 +112,7 @@ static inline void fpscr_set_rounding_mode(CPUPPCState *env)
 
 void ppc_store_fpscr(CPUPPCState *env, target_ulong val)
 {
-    val &= ~(FP_VX | FP_FEX);
+    val &= FPSCR_MTFS_MASK;
     if (val & FPSCR_IX) {
         val |= FP_VX;
     }
diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index e946da5f3a..53463092ab 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -759,6 +759,9 @@ enum {
                           FP_VXZDZ  | FP_VXIMZ  | FP_VXVC   | FP_VXSOFT | \
                           FP_VXSQRT | FP_VXCVI)
 
+/* FPSCR bits that can be set by mtfsf, mtfsfi and mtfsb1 */
+#define FPSCR_MTFS_MASK (~((1ull << 11) | FP_VX | FP_FEX))
+
 /*****************************************************************************/
 /* Vector status and control register */
 #define VSCR_NJ         16 /* Vector non-java */
-- 
2.31.1

[PATCH v2 3/3] test/tcg/ppc64le: test mtfsf

[Lucas Mateus Castro (alqotel)]

Added tests for the mtfsf to check if FI bit of FPSCR is being set
and if exception calls are being made correctly.

Signed-off-by: Lucas Mateus Castro (alqotel) <lucas.araujo@eldorado.org.br>
---
 tests/tcg/ppc64/Makefile.target   |  1 +
 tests/tcg/ppc64le/Makefile.target |  1 +
 tests/tcg/ppc64le/mtfsf.c         | 56 +++++++++++++++++++++++++++++++
 3 files changed, 58 insertions(+)
 create mode 100644 tests/tcg/ppc64le/mtfsf.c

diff --git a/tests/tcg/ppc64/Makefile.target b/tests/tcg/ppc64/Makefile.target
index 6ab7934fdf..8f4c7ac4ed 100644
--- a/tests/tcg/ppc64/Makefile.target
+++ b/tests/tcg/ppc64/Makefile.target
@@ -11,6 +11,7 @@ endif
 bcdsub: CFLAGS += -mpower8-vector
 
 PPC64_TESTS += byte_reverse
+PPC64_TESTS += mtfsf
 ifneq ($(DOCKER_IMAGE)$(CROSS_CC_HAS_POWER10),)
 run-byte_reverse: QEMU_OPTS+=-cpu POWER10
 run-plugin-byte_reverse-with-%: QEMU_OPTS+=-cpu POWER10
diff --git a/tests/tcg/ppc64le/Makefile.target b/tests/tcg/ppc64le/Makefile.target
index 5e65b1590d..b8cd9bf73a 100644
--- a/tests/tcg/ppc64le/Makefile.target
+++ b/tests/tcg/ppc64le/Makefile.target
@@ -10,6 +10,7 @@ endif
 bcdsub: CFLAGS += -mpower8-vector
 
 PPC64LE_TESTS += byte_reverse
+PPC64LE_TESTS += mtfsf
 ifneq ($(DOCKER_IMAGE)$(CROSS_CC_HAS_POWER10),)
 run-byte_reverse: QEMU_OPTS+=-cpu POWER10
 run-plugin-byte_reverse-with-%: QEMU_OPTS+=-cpu POWER10
diff --git a/tests/tcg/ppc64le/mtfsf.c b/tests/tcg/ppc64le/mtfsf.c
new file mode 100644
index 0000000000..9b3290d94c
--- /dev/null
+++ b/tests/tcg/ppc64le/mtfsf.c
@@ -0,0 +1,56 @@
+#include <stdlib.h>
+#include <assert.h>
+#include <signal.h>
+#include <sys/prctl.h>
+
+#define FPSCR_VE     7  /* Floating-point invalid operation exception enable */
+#define FPSCR_VXSOFT 10 /* Floating-point invalid operation exception (soft) */
+#define FPSCR_FI     17 /* Floating-point fraction inexact                   */
+
+#define FP_VE           (1ull << FPSCR_VE)
+#define FP_VXSOFT       (1ull << FPSCR_VXSOFT)
+#define FP_FI           (1ull << FPSCR_FI)
+
+void sigfpe_handler(int sig, siginfo_t *si, void *ucontext)
+{
+    exit(0);
+}
+
+int main(void)
+{
+    union {
+        double d;
+        long long ll;
+    } fpscr;
+
+    struct sigaction sa = {
+        .sa_sigaction = sigfpe_handler,
+        .sa_flags = SA_SIGINFO
+    };
+
+    /*
+     * Enable the MSR bits F0 and F1 to enable exceptions.
+     * This shouldn't be needed in linux-user as these bits are enabled by
+     * default, but this allows to execute either in a VM or a real machine
+     * to compare the behaviors.
+     */
+    prctl(PR_SET_FPEXC, PR_FP_EXC_PRECISE);
+
+    /* First test if the FI bit is being set correctly */
+    fpscr.ll = FP_FI;
+    __builtin_mtfsf(0b11111111, fpscr.d);
+    fpscr.d = __builtin_mffs();
+    assert((fpscr.ll & FP_FI) != 0);
+
+    /* Then test if the deferred exception is being called correctly */
+    sigaction(SIGFPE, &sa, NULL);
+
+    /*
+     * Although the VXSOFT exception has been chosen, based on test in a Power9
+     * any combination of exception bit + its enabling bit should work.
+     */
+    fpscr.ll = FP_VE | FP_VXSOFT;
+    __builtin_mtfsf(0b11111111, fpscr.d);
+
+    return 1;
+}
-- 
2.31.1

Re: [PATCH v2 2/3] target/ppc: ppc_store_fpscr doesn't update bit 52

[BALATON Zoltan]

On Thu, 18 Nov 2021, Lucas Mateus Castro (alqotel) wrote:
> This commit fixes the difference reported in the bug in the reserved
> bit 52, it does this by adding this bit to the mask of bits to not be
> directly altered in the ppc_store_fpscr function (the hardware used to
> compare to QEMU was a Power9).
>
> Although this is a difference reported in the bug, since it's a reserved
> bit it may be a "don't care" case, as put in the bug report. Looking at
> the ISA it doesn't explicitly mentions this bit can't be set, like it
> does for FEX and VX, so I'm unsure if this is necessary.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/266
> Signed-off-by: Lucas Mateus Castro (alqotel) <lucas.araujo@eldorado.org.br>
> ---
> target/ppc/cpu.c | 2 +-
> target/ppc/cpu.h | 3 +++
> 2 files changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/target/ppc/cpu.c b/target/ppc/cpu.c
> index f933d9f2bd..d7b42bae52 100644
> --- a/target/ppc/cpu.c
> +++ b/target/ppc/cpu.c
> @@ -112,7 +112,7 @@ static inline void fpscr_set_rounding_mode(CPUPPCState *env)
>
> void ppc_store_fpscr(CPUPPCState *env, target_ulong val)
> {
> -    val &= ~(FP_VX | FP_FEX);
> +    val &= FPSCR_MTFS_MASK;
>     if (val & FPSCR_IX) {
>         val |= FP_VX;
>     }
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index e946da5f3a..53463092ab 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -759,6 +759,9 @@ enum {
>                           FP_VXZDZ  | FP_VXIMZ  | FP_VXVC   | FP_VXSOFT | \
>                           FP_VXSQRT | FP_VXCVI)
>
> +/* FPSCR bits that can be set by mtfsf, mtfsfi and mtfsb1 */
> +#define FPSCR_MTFS_MASK (~((1ull << 11) | FP_VX | FP_FEX))

Instead of (1ull << 11) maybe PPC_BIT(52) is a bit clearer (or not 
depending on personal preference, so I don't mind either way.

Regards,
BALATON Zoltan

> +
> /*****************************************************************************/
> /* Vector status and control register */
> #define VSCR_NJ         16 /* Vector non-java */
>

Re: [PATCH v2 1/3] target/ppc: Fixed call to deferred exception

[Richard Henderson]

On 11/18/21 2:25 PM, Lucas Mateus Castro (alqotel) wrote:
> +    if ((fpscr & FP_VXSOFT) && (fpscr_ve != 0)) {
> +        error = POWERPC_EXCP_FP_VXSOFT;
> +    } else if ((fpscr & FP_OX) && (fpscr & FP_OE)) {
> +        error = POWERPC_EXCP_FP_OX;
> +    } else if ((fpscr & FP_UX) && (fpscr & FP_UE)) {
> +        error = POWERPC_EXCP_FP_UX;
> +    } else if ((fpscr & FP_XX) && (fpscr & FP_XE)) {
> +        error = POWERPC_EXCP_FP_XX;
> +    } else if ((fpscr & FP_ZX) && (fpscr & FP_ZE)) {
> +        error = POWERPC_EXCP_FP_ZX;
> +    } else if ((fpscr & FP_VXSNAN) && (fpscr_ve != 0)) {
> +        error = POWERPC_EXCP_FP_VXSNAN;
> +    } else if ((fpscr & FP_VXISI) && (fpscr_ve != 0)) {
> +        error = POWERPC_EXCP_FP_VXISI;
> +    } else if ((fpscr & FP_VXIDI) && (fpscr_ve != 0)) {
> +        error = POWERPC_EXCP_FP_VXIDI;
> +    } else if ((fpscr & FP_VXZDZ) && (fpscr_ve != 0)) {
> +        error = POWERPC_EXCP_FP_VXZDZ;
> +    } else if ((fpscr & FP_VXIMZ) && (fpscr_ve != 0)) {
> +        error = POWERPC_EXCP_FP_VXIMZ;
> +    } else if ((fpscr & FP_VXVC) && (fpscr_ve != 0)) {
> +        error = POWERPC_EXCP_FP_VXVC;
> +    }

Is there a defined order for these in the manual?  I couldn't find it quickly if so.  If 
there is no defined order, I think you should test VE only once.

Drop the use of fpscr_ve and use fpscr & FP_VE instead. (I think these hidden uses of *env 
are evil and should be banished, but that's a bit of a job.)

You could say

     } else {
         return;
     }

> +
> +    if (error) {

and then remove this test.

The rest of it looks good.


r~

Re: [PATCH v2 2/3] target/ppc: ppc_store_fpscr doesn't update bit 52

[Richard Henderson]

On 11/18/21 2:25 PM, Lucas Mateus Castro (alqotel) wrote:
> +/* FPSCR bits that can be set by mtfsf, mtfsfi and mtfsb1 */
> +#define FPSCR_MTFS_MASK (~((1ull << 11) | FP_VX | FP_FEX))

If you're going to make the reserved bit 52 read-as-zero-writes-ignored, you should do the 
same for reserved bits 0-31.  Otherwise drop this and let the bits be read-write with no 
effect.


r~

Re: [PATCH v2 1/3] target/ppc: Fixed call to deferred exception

[Cédric Le Goater]

On 11/19/21 10:18, Richard Henderson wrote:
> On 11/18/21 2:25 PM, Lucas Mateus Castro (alqotel) wrote:
>> +    if ((fpscr & FP_VXSOFT) && (fpscr_ve != 0)) {
>> +        error = POWERPC_EXCP_FP_VXSOFT;
>> +    } else if ((fpscr & FP_OX) && (fpscr & FP_OE)) {
>> +        error = POWERPC_EXCP_FP_OX;
>> +    } else if ((fpscr & FP_UX) && (fpscr & FP_UE)) {
>> +        error = POWERPC_EXCP_FP_UX;
>> +    } else if ((fpscr & FP_XX) && (fpscr & FP_XE)) {
>> +        error = POWERPC_EXCP_FP_XX;
>> +    } else if ((fpscr & FP_ZX) && (fpscr & FP_ZE)) {
>> +        error = POWERPC_EXCP_FP_ZX;
>> +    } else if ((fpscr & FP_VXSNAN) && (fpscr_ve != 0)) {
>> +        error = POWERPC_EXCP_FP_VXSNAN;
>> +    } else if ((fpscr & FP_VXISI) && (fpscr_ve != 0)) {
>> +        error = POWERPC_EXCP_FP_VXISI;
>> +    } else if ((fpscr & FP_VXIDI) && (fpscr_ve != 0)) {
>> +        error = POWERPC_EXCP_FP_VXIDI;
>> +    } else if ((fpscr & FP_VXZDZ) && (fpscr_ve != 0)) {
>> +        error = POWERPC_EXCP_FP_VXZDZ;
>> +    } else if ((fpscr & FP_VXIMZ) && (fpscr_ve != 0)) {
>> +        error = POWERPC_EXCP_FP_VXIMZ;
>> +    } else if ((fpscr & FP_VXVC) && (fpscr_ve != 0)) {
>> +        error = POWERPC_EXCP_FP_VXVC;
>> +    }
> 
> Is there a defined order for these in the manual?  I couldn't find it quickly if so.  If there is no defined order, I think you should test VE only once.
> 
> Drop the use of fpscr_ve and use fpscr & FP_VE instead. (I think these hidden uses of *env are evil and should be banished, but that's a bit of a job.)

you mean all the msr_* macros ? I agree. It's huge and I wonder how we could automate parts of it.

Thanks,

C.
  
> 
> You could say
> 
>      } else {
>          return;
>      }
> 
>> +
>> +    if (error) {
> 
> and then remove this test.
> 
> The rest of it looks good.
> 
> 
> r~

Re: [PATCH v2 1/3] target/ppc: Fixed call to deferred exception

[Richard Henderson]

On 11/19/21 10:24 AM, Cédric Le Goater wrote:
>>> +    } else if ((fpscr & FP_VXVC) && (fpscr_ve != 0)) {
>>> +        error = POWERPC_EXCP_FP_VXVC;
>>> +    }
>>
>> Is there a defined order for these in the manual?  I couldn't find it quickly if so.  If 
>> there is no defined order, I think you should test VE only once.
>>
>> Drop the use of fpscr_ve and use fpscr & FP_VE instead. (I think these hidden uses of 
>> *env are evil and should be banished, but that's a bit of a job.)
> 
> you mean all the msr_* macros ? I agree. It's huge and I wonder how we could automate 
> parts of it.

Well, those too.  But fpscr_ve is the one that caught my eye here.


r~

Re: [PATCH v2 3/3] test/tcg/ppc64le: test mtfsf

[Richard Henderson]

On 11/18/21 2:25 PM, Lucas Mateus Castro (alqotel) wrote:
> +void sigfpe_handler(int sig, siginfo_t *si, void *ucontext)
> +{
> +    exit(0);
> +}

It would be good to verify si->si_code = FPE_FLTINV,

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


r~

Re: [PATCH v2 1/3] target/ppc: Fixed call to deferred exception

[Lucas Mateus Martins Araujo e Castro]

[-- Attachment #1: Type: text/plain, Size: 2359 bytes --]


On 19/11/2021 06:18, Richard Henderson wrote:
> On 11/18/21 2:25 PM, Lucas Mateus Castro (alqotel) wrote:
>> +    if ((fpscr & FP_VXSOFT) && (fpscr_ve != 0)) {
>> +        error = POWERPC_EXCP_FP_VXSOFT;
>> +    } else if ((fpscr & FP_OX) && (fpscr & FP_OE)) {
>> +        error = POWERPC_EXCP_FP_OX;
>> +    } else if ((fpscr & FP_UX) && (fpscr & FP_UE)) {
>> +        error = POWERPC_EXCP_FP_UX;
>> +    } else if ((fpscr & FP_XX) && (fpscr & FP_XE)) {
>> +        error = POWERPC_EXCP_FP_XX;
>> +    } else if ((fpscr & FP_ZX) && (fpscr & FP_ZE)) {
>> +        error = POWERPC_EXCP_FP_ZX;
>> +    } else if ((fpscr & FP_VXSNAN) && (fpscr_ve != 0)) {
>> +        error = POWERPC_EXCP_FP_VXSNAN;
>> +    } else if ((fpscr & FP_VXISI) && (fpscr_ve != 0)) {
>> +        error = POWERPC_EXCP_FP_VXISI;
>> +    } else if ((fpscr & FP_VXIDI) && (fpscr_ve != 0)) {
>> +        error = POWERPC_EXCP_FP_VXIDI;
>> +    } else if ((fpscr & FP_VXZDZ) && (fpscr_ve != 0)) {
>> +        error = POWERPC_EXCP_FP_VXZDZ;
>> +    } else if ((fpscr & FP_VXIMZ) && (fpscr_ve != 0)) {
>> +        error = POWERPC_EXCP_FP_VXIMZ;
>> +    } else if ((fpscr & FP_VXVC) && (fpscr_ve != 0)) {
>> +        error = POWERPC_EXCP_FP_VXVC;
>> +    }
>
> Is there a defined order for these in the manual?  I couldn't find it 
> quickly if so.  If
> there is no defined order, I think you should test VE only once.
I also couldn't find a defined order, so I chose to prioritize VXSOFT 
then go by ascending order of the bit number. In the v3 I'll move VXSOFT 
with the others invalid operation bits to check VE once then.
>
> Drop the use of fpscr_ve and use fpscr & FP_VE instead. (I think these 
> hidden uses of *env
> are evil and should be banished, but that's a bit of a job.)
>
> You could say
>
>     } else {
>         return;
>     }
>
>> +
>> +    if (error) {
>
> and then remove this test.
Ok, I'll do these in v3.
>
> The rest of it looks good.
>
>
> r~
-- 
Lucas Mateus M. Araujo e Castro
Instituto de Pesquisas ELDORADO 
<https://www.eldorado.org.br/?utm_campaign=assinatura_de_e-mail&utm_medium=email&utm_source=RD+Station>
Departamento Computação Embarcada
Estagiario
Aviso Legal - Disclaimer <https://www.eldorado.org.br/disclaimer.html>

[-- Attachment #2: Type: text/html, Size: 4094 bytes --]