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

[Bruno Piazera Larsen <bruno.larsen@eldorado.org.br>]

[-- Attachment #1: Type: text/plain, Size: 4485 bytes --]


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 
<https://www.eldorado.org.br/?utm_campaign=assinatura_de_e-mail&utm_medium=email&utm_source=RD+Station>
Departamento Computação Embarcada
Analista de Software Trainee
Aviso Legal - Disclaimer <https://www.eldorado.org.br/disclaimer.html>

[-- Attachment #2: Type: text/html, Size: 6846 bytes --]