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: Bruno Piazera Larsen
Subject: Re: [PATCH 07/11] target/ppc: added KVM fallback to fpscr manipulation
Date: Fri, 14 May 2021 08:12:57 -0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.8.1


On 13/05/2021 19:45, Richard Henderson wrote:
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?
So the plan was to have gdbstub call ppc_store_fpscr? I assumed it wasn't since there is one less parameter in the new function. Now that I took another look, gdbstub has the mask as 0xffffffff, so it's easier than I thought.

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.

Not exactly, because the expansion of nibbles into mask is still in helper_store_fpscr, and that's what I meant.

--
Bruno Piazera Larsen
Instituto de Pesquisas ELDORADO
Departamento Computação Embarcada
Analista de Software Trainee
Aviso Legal - Disclaimer
reply via email to
[Prev in Thread] Current Thread [Next in Thread]