qemu-devel
[Top][All Lists]
Advanced

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

Re: [PATCH 07/11] target/ppc: added KVM fallback to fpscr manipulation


From: Richard Henderson
Subject: Re: [PATCH 07/11] target/ppc: added KVM fallback to fpscr manipulation
Date: Thu, 13 May 2021 17:45:43 -0500
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.8.1

On 5/13/21 11:36 AM, Bruno Piazera Larsen wrote:

On 12/05/2021 15:20, Richard Henderson wrote:
On 5/12/21 9:08 AM, Bruno Larsen (billionai) wrote:
diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
index 104a308abb..a8a720eb48 100644
--- a/target/ppc/kvm.c
+++ b/target/ppc/kvm.c
@@ -2947,3 +2947,17 @@ bool kvm_arch_cpu_check_are_resettable(void)
  {
      return true;
  }
+
+void kvmppc_store_fpscr(CPUPPCState *env, uint64_t arg, uint32_t mask)
+{
+    CPUState *cs = env_cpu(env);
+    struct kvm_one_reg reg;
+    int ret;
+    reg.id = KVM_REG_PPC_FPSCR;
+    reg.addr = (uintptr_t) &env->fpscr;
+    ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
+    if (ret < 0)
+    {
+        fprintf(stderr, "Unable to set FPSCR to KVM: %s", strerror(errno));
+    }
+}

This is all unnecessary.  All you need to do is store to env->fpscr and the value will be synced back with kvm_put_fp.

I'll note that some of the trouble you may be having with extracting helper_store_fpscr to a ppc_store_fpscr function is due to an existing bug in the tcg code:

Storing to fpscr should *never* raise an exception -- see MTFSF, MTFSB0, MTFSB1.  Thus the mucking about with cs->exception_index and env->error_code is incorrect.

In addition, the masking is being done weirdly and could use a complete overhaul.

This could all be rewritten as

-- %< -- cpu.h

 /* Invalid operation exception summary */
- #define fpscr_ix ((env->fpscr) & ((1 << FPSCR_VXSNAN) ...
+ #define FPSCR_IX  ((1 << FPSCR_VXSNAN) | ...)

-- %< -- cpu.c

// move fpscr_set_rounding_mode here

void ppc_store_fpscr(CPUPPCState *env, target_ulong val)
{
    /* Recompute exception summary state. */
    val &= ~(FP_VX | FP_FEX);
    if (val & FPSCR_IX) {
        val |= FP_VX;
    }
    if ((val >> FPSCR_XX) & (val >> FPSCR_XE) & 0x1f) {
        val |= FP_FEX;
    }
    env->fpscr = val;
    if (tcg_enabled()) {
        fpscr_set_rounding_mode(env);
    }
}

-- %< -- fpu_helper.c

void helper_store_fpscr(CPUPPCState *env, target_ulong val,
                        uint32_t nibbles)
{
    target_ulong mask = 0;

    /* TODO: Push this expansion back to translation time. */
    for (int i = 0; i < sizeof(target_ulong) * 2; ++i) {
        if (nibbles & (1 << i)) {
            mask |= (target_ulong)0xf << (4 * i);
        }
    }

    val = (val & mask) | (env->fpscr & ~mask);
    ppc_store_fpscr(env, val);
}
That expansion can't be moved to translation time, because gdbstub would also need that code in a variety of functions, so better to keep it in that central location,

void helper_fpscr_clrbit(CPUPPCState *env, uint32_t bit)
{
    uint32_t mask = 1u << bit;
    if (env->fpscr & mask) {
        ppc_store_fpscr(env, env->fpscr & ~mask);
    }
}

void helper_fpscr_setbit(CPUPPCState *env, uint32_t bit)
{
    uint32_t mask = 1u << bit;
    if (!(env->fpscr & mask)) {
        ppc_store_fpscr(env, env->fpscr | mask);
    }
}

There are a couple of other uses of fpscr_set_rounding_mode, where the softfloat value is changed temporarily (do_fri, VSX_ROUND). These should simply save the previous softfloat value (using get_float_rounding_mode) around the operation instead of re-computing from fpscr.

Which leaves us with exactly one use of fpscr_set_rounding_mode, which can then be moved to cpu.c next to ppc_store_fpscr.


r~

I was implementing this solution, but ran into a problem: We needed store_fpscr for gdbstub.c, that's the original reason we made that new function to begin with. This solution, although it improves the handling of fpscr, doesn't fix the original problem.

Why not?  Did you miss the cpu.c cut at the very top?

What I think we can do is put the logic that is in helper_store_fpscr into store_fpscr, move it to cpu.c, and have the helper call the non-helper function. That way we conserve helper_* as TCG-specific and have the overhaul.

That is exactly what I have written above.


r~



reply via email to

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