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

[Bruno Piazera Larsen]

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.

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.

Toughts?

--
Bruno Piazera Larsen
Instituto de Pesquisas ELDORADO
Departamento Computação Embarcada
Analista de Software Trainee
Aviso Legal - Disclaimer