[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: BALATON Zoltan
Subject: Re: [PATCH 1/2] target/ppc: Fixed call to deferred exception
Date: Wed, 10 Nov 2021 21:40:59 +0100 (CET)

On Wed, 10 Nov 2021, Lucas Mateus Martins Araujo e Castro wrote:
On 10/11/2021 05:19, Mark Cave-Ayland wrote:
On 20/10/2021 13:57, Lucas Mateus Castro (alqotel) wrote:
From: "Lucas Mateus Castro (alqotel)" <lucas.castro@eldorado.org.br>

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.

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

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/266
Signed-off-by: Lucas Mateus Castro (alqotel) <lucas.castro@eldorado.org.br>
  target/ppc/fpu_helper.c            | 41 ++++++++++++++++++++++++++++++
  target/ppc/helper.h                |  1 +
  target/ppc/translate/fp-impl.c.inc |  6 ++---
  3 files changed, 45 insertions(+), 3 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 4076aa281e..baa3715e73 100644
--- a/target/ppc/helper.h
+++ b/target/ppc/helper.h
@@ -61,6 +61,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 9f7868ee28..0a9b1ecc60 100644
--- a/target/ppc/translate/fp-impl.c.inc
+++ b/target/ppc/translate/fp-impl.c.inc
@@ -782,7 +782,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 */
@@ -818,7 +818,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);

@@ -851,7 +851,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);

  /***                         Floating-point load                           ***/

FWIW the real issue here is that gen_helper_reset_fpstatus() even exists at all: see the comments around enabling hardfloat in the PPC target by Emilio and Richard at
https://lists.nongnu.org/archive/html/qemu-devel/2018-11/msg04974.html and

I have tried a few informal experiments on my MacOS images by completely removing all calls to gen_reset_fpstatus(), and whilst there were a few odd behaviours I was surprised to find that the basic OS was usable. The main issue I had was trying to come up with suitable test cases for the various instructions when my only available
hardware is a G4 Mac Mini.

So yes this patch fixes one particular use case, but the real issue is that the PPC target floating point flags need a bit of work: however once this is done it should be possible for hardfloat to be enabled via a CPU option on suitable hosts which will
bring a noticeable improvement in floating point performance.

In this case I don't think gen_helper_reset_fpstatus() is the problem, fp_status is not updated in the instruction but its value is used in helper_float_check_status(), so if the values have not been reset since the last instruction it'll contain last instruction's information and if it has (either by calling gen_helper_reset_fpstatus(), by automatically doing it every instruction or by having every instruction reset it in the end) it'll have 0. So there are 3 alternatives to solve this that I can think of:

    * Update FPSCR directly, then update fp_status based on FPSCR, for this you would either have to call a new helper to do this or update helper_store_fpscr to do this, and then expand do_float_check_status to throw more exceptions (or create a new helper to do this if expanding do_float_check_status could cause problems),

    * Just don't use fp_status, update FPSCR directly and do the deferred exception using only information from FPSCR (the one I used this patch),

    * Update only fp_status directly and call either a modified do_float_check_status or a new helper that would update FPSCR and throw the correct exception based on fp_status, this one I don't see how it would feasible in the current implementation as FPSCR has many bits without an equivalent in fp_status.

So with this I can see how to implement the 1st and 2nd option, I chose not to use the 1st one as do_float_check_status updates the FPSCR then throw the exception, which seemed unnecessary. Also looking back I should've removed gen_reset_fpstatus() as in the way it ended implemented these instructions don't interact with fp_status anywhere else, so I'll remove it in the next version.

And looking at the suggestions the current implementation could be changed to take advantage of the optimization suggested in the discussion you linked, specially the parts about checking when exception bits aren't set (but in this case it would've to be the MSR exception bits) and the part about skipping calculating a flag when marked to 1.

I haven't followed the discussion but here's another message with some links I've collected when FPU came up that may be relevant to the topic:


among those is a long thread on patchwork that has some info on the current situation. As far as I remember the oddity in handling FPU exceptions is partly because of two bits FI and FR in FPSCR that should reflect the result of the previous FPU op so has to be updated after every op which makes it hard to emulate as other CPUs usually don't do this. (We could easily improve it if we did not emulate those bits, most guest code don't use them anyway, but QEMU prefers accuracy so that way was ruled out.) Other than that the current code maybe also can be simplified and maybe optimised via some other ways which were discussed in those threads but nobody implemented any of the ideas so far. May worth reading through what was said before as there might be sume useful ideas in there.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]