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

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

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~