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

[Richard Henderson <richard.henderson@linaro.org>]

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);
}

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~