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

[PATCH v3 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 v2:
- changed patch order to add the mtfsf test after the FI bit and
  deferred exception fix(patch 1) as these are the errors tested here
- moved code to check FP_VE only once in helper_fpscr_check_status
- tests/tcg/ppc64le/mtfsf.c tests if the signal code is correct
- FPSCR bits 0-28 can't be set anymore as they're reserved bits
- changed (11ull << 11) in FPSCR_MTFS_MASK to PPC_BIT(52) to make it clearer

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
  test/tcg/ppc64le: test mtfsf
  target/ppc: ppc_store_fpscr doesn't update bits 0 to 28 and 52

 target/ppc/cpu.c                   |  2 +-
 target/ppc/cpu.h                   |  4 ++
 target/ppc/fpu_helper.c            | 48 +++++++++++++++++++++++
 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          | 61 ++++++++++++++++++++++++++++++
 8 files changed, 120 insertions(+), 7 deletions(-)
 create mode 100644 tests/tcg/ppc64le/mtfsf.c

-- 
2.31.1

[PATCH v3 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            | 48 ++++++++++++++++++++++++++++++
 target/ppc/helper.h                |  1 +
 target/ppc/translate/fp-impl.c.inc |  9 ++----
 3 files changed, 52 insertions(+), 6 deletions(-)

diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c
index c4896cecc8..bb72715827 100644
--- a/target/ppc/fpu_helper.c
+++ b/target/ppc/fpu_helper.c
@@ -414,6 +414,54 @@ 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_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_VE) {
+        if (fpscr & FP_VXSOFT) {
+            error = POWERPC_EXCP_FP_VXSOFT;
+        } else if (fpscr & FP_VXSNAN) {
+            error = POWERPC_EXCP_FP_VXSNAN;
+        } else if (fpscr & FP_VXISI) {
+            error = POWERPC_EXCP_FP_VXISI;
+        } else if (fpscr & FP_VXIDI) {
+            error = POWERPC_EXCP_FP_VXIDI;
+        } else if (fpscr & FP_VXZDZ) {
+            error = POWERPC_EXCP_FP_VXZDZ;
+        } else if (fpscr & FP_VXIMZ) {
+            error = POWERPC_EXCP_FP_VXIMZ;
+        } else if (fpscr & FP_VXVC) {
+            error = POWERPC_EXCP_FP_VXVC;
+        } else if (fpscr & FP_VXSQRT) {
+            error = POWERPC_EXCP_FP_VXSQRT;
+        } else if (fpscr & FP_VXCVI) {
+            error = POWERPC_EXCP_FP_VXCVI;
+        } else {
+            return;
+        }
+    } else {
+        return;
+    }
+    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 v3 2/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         | 61 +++++++++++++++++++++++++++++++
 3 files changed, 63 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..b3d31f3637
--- /dev/null
+++ b/tests/tcg/ppc64le/mtfsf.c
@@ -0,0 +1,61 @@
+#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)
+{
+    if (si->si_code == FPE_FLTINV) {
+        exit(0);
+    }
+    exit(1);
+}
+
+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.
+     * But if a different exception is chosen si_code check should
+     * change accordingly.
+     */
+    fpscr.ll = FP_VE | FP_VXSOFT;
+    __builtin_mtfsf(0b11111111, fpscr.d);
+
+    return 1;
+}
-- 
2.31.1

[PATCH v3 3/3] target/ppc: ppc_store_fpscr doesn't update bits 0 to 28 and 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).

The bits 0 to 27 were also added to the mask, as they are marked as
reserved in the PowerISA and bit 28 is a reserved extension of the DRN
field (bits 29:31) but can't be set using mtfsfi, while the other DRN
bits may be set using mtfsfi instruction, so bit 28 was also added to
the mask.

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 | 4 ++++
 2 files changed, 5 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..441d3dce19 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -759,6 +759,10 @@ 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 (~(MAKE_64BIT_MASK(36, 28) | PPC_BIT(28) |        \
+                           FP_FEX | FP_VX | PPC_BIT(52)))
+
 /*****************************************************************************/
 /* Vector status and control register */
 #define VSCR_NJ         16 /* Vector non-java */
-- 
2.31.1

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

[BALATON Zoltan]

On Wed, 24 Nov 2021, Lucas Mateus Castro (alqotel) wrote:
> 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            | 48 ++++++++++++++++++++++++++++++
> target/ppc/helper.h                |  1 +
> target/ppc/translate/fp-impl.c.inc |  9 ++----
> 3 files changed, 52 insertions(+), 6 deletions(-)
>
> diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c
> index c4896cecc8..bb72715827 100644
> --- a/target/ppc/fpu_helper.c
> +++ b/target/ppc/fpu_helper.c
> @@ -414,6 +414,54 @@ 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_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)) {

I wonder if these tests could be simplified by combining the masks if you 
want to test for both bits set so e.g. fpscr & (FP_ZX | FP_ZE) should be 
the same, shouldn't it?

Regards,
BALATON Zoltan

> +        error = POWERPC_EXCP_FP_ZX;
> +    } else if (fpscr & FP_VE) {
> +        if (fpscr & FP_VXSOFT) {
> +            error = POWERPC_EXCP_FP_VXSOFT;
> +        } else if (fpscr & FP_VXSNAN) {
> +            error = POWERPC_EXCP_FP_VXSNAN;
> +        } else if (fpscr & FP_VXISI) {
> +            error = POWERPC_EXCP_FP_VXISI;
> +        } else if (fpscr & FP_VXIDI) {
> +            error = POWERPC_EXCP_FP_VXIDI;
> +        } else if (fpscr & FP_VXZDZ) {
> +            error = POWERPC_EXCP_FP_VXZDZ;
> +        } else if (fpscr & FP_VXIMZ) {
> +            error = POWERPC_EXCP_FP_VXIMZ;
> +        } else if (fpscr & FP_VXVC) {
> +            error = POWERPC_EXCP_FP_VXVC;
> +        } else if (fpscr & FP_VXSQRT) {
> +            error = POWERPC_EXCP_FP_VXSQRT;
> +        } else if (fpscr & FP_VXCVI) {
> +            error = POWERPC_EXCP_FP_VXCVI;
> +        } else {
> +            return;
> +        }
> +    } else {
> +        return;
> +    }
> +    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)
>

Re: [PATCH v3 3/3] target/ppc: ppc_store_fpscr doesn't update bits 0 to 28 and 52

[BALATON Zoltan]

On Wed, 24 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).
>
> The bits 0 to 27 were also added to the mask, as they are marked as
> reserved in the PowerISA and bit 28 is a reserved extension of the DRN
> field (bits 29:31) but can't be set using mtfsfi, while the other DRN
> bits may be set using mtfsfi instruction, so bit 28 was also added to
> the mask.
>
> 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

doesn't explicitly mention (no s needed)

Regards,
BALATON Zoltan

> 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 | 4 ++++
> 2 files changed, 5 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..441d3dce19 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -759,6 +759,10 @@ 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 (~(MAKE_64BIT_MASK(36, 28) | PPC_BIT(28) |        \
> +                           FP_FEX | FP_VX | PPC_BIT(52)))
> +
> /*****************************************************************************/
> /* Vector status and control register */
> #define VSCR_NJ         16 /* Vector non-java */
>

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

[David Gibson]

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

On Thu, Nov 25, 2021 at 01:49:46AM +0100, BALATON Zoltan wrote:
> On Wed, 24 Nov 2021, Lucas Mateus Castro (alqotel) wrote:
> > 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            | 48 ++++++++++++++++++++++++++++++
> > target/ppc/helper.h                |  1 +
> > target/ppc/translate/fp-impl.c.inc |  9 ++----
> > 3 files changed, 52 insertions(+), 6 deletions(-)
> > 
> > diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c
> > index c4896cecc8..bb72715827 100644
> > --- a/target/ppc/fpu_helper.c
> > +++ b/target/ppc/fpu_helper.c
> > @@ -414,6 +414,54 @@ 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_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)) {
> 
> I wonder if these tests could be simplified by combining the masks if you
> want to test for both bits set so e.g. fpscr & (FP_ZX | FP_ZE) should be the
> same, shouldn't it?

No, it's not.  In fact your version is equivalent as a boolean to
	((fpscr & FP_ZX) || (fpscr & FP_ZE))

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Richard Henderson]

On 11/24/21 6:25 PM, Lucas Mateus Castro (alqotel) wrote:
> 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            | 48 ++++++++++++++++++++++++++++++
>   target/ppc/helper.h                |  1 +
>   target/ppc/translate/fp-impl.c.inc |  9 ++----
>   3 files changed, 52 insertions(+), 6 deletions(-)

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


r~

Re: [PATCH v3 2/3] test/tcg/ppc64le: test mtfsf

[Richard Henderson]

On 11/24/21 6:25 PM, Lucas Mateus Castro (alqotel) wrote:
> 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         | 61 +++++++++++++++++++++++++++++++
>   3 files changed, 63 insertions(+)
>   create mode 100644 tests/tcg/ppc64le/mtfsf.c

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

r~

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

[BALATON Zoltan]

On Thu, 25 Nov 2021, David Gibson wrote:
> On Thu, Nov 25, 2021 at 01:49:46AM +0100, BALATON Zoltan wrote:
>> On Wed, 24 Nov 2021, Lucas Mateus Castro (alqotel) wrote:
>>> 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            | 48 ++++++++++++++++++++++++++++++
>>> target/ppc/helper.h                |  1 +
>>> target/ppc/translate/fp-impl.c.inc |  9 ++----
>>> 3 files changed, 52 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c
>>> index c4896cecc8..bb72715827 100644
>>> --- a/target/ppc/fpu_helper.c
>>> +++ b/target/ppc/fpu_helper.c
>>> @@ -414,6 +414,54 @@ 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_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)) {
>>
>> I wonder if these tests could be simplified by combining the masks if you
>> want to test for both bits set so e.g. fpscr & (FP_ZX | FP_ZE) should be the
>> same, shouldn't it?
>
> No, it's not.  In fact your version is equivalent as a boolean to
> 	((fpscr & FP_ZX) || (fpscr & FP_ZE))

Indeed, it was too late when I wrote. I was really thinking
(fprscr & (FP_ZX | FP_ZE)) == (FP_ZX | FP_ZE) but that's not simpler so 
that answers my question.

Regards,
BALATON Zoltan

Re: [PATCH v3 2/3] test/tcg/ppc64le: test mtfsf

[Cédric Le Goater]

Hello Lucas,

On 11/24/21 18:25, Lucas Mateus Castro (alqotel) wrote:
> 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>

Could you please rebase on mainline and resend ?

Thanks,

C.
  
> ---
>   tests/tcg/ppc64/Makefile.target   |  1 +
>   tests/tcg/ppc64le/Makefile.target |  1 +
>   tests/tcg/ppc64le/mtfsf.c         | 61 +++++++++++++++++++++++++++++++
>   3 files changed, 63 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..b3d31f3637
> --- /dev/null
> +++ b/tests/tcg/ppc64le/mtfsf.c
> @@ -0,0 +1,61 @@
> +#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)
> +{
> +    if (si->si_code == FPE_FLTINV) {
> +        exit(0);
> +    }
> +    exit(1);
> +}
> +
> +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.
> +     * But if a different exception is chosen si_code check should
> +     * change accordingly.
> +     */
> +    fpscr.ll = FP_VE | FP_VXSOFT;
> +    __builtin_mtfsf(0b11111111, fpscr.d);
> +
> +    return 1;
> +}
>