| Message ID | 20210512140813.112884-8-bruno.larsen@eldorado.org.br (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/ppc: add support to disable-tcg | expand |
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, ®); > + 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~
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, ®); >> + 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 figured this was too complicated again, but didn't have a better idea > > 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. > ok, yeah, I can definitely try this. > > r~
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, ®); >> + 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?
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, ®); >>> + 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~
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, ®); >>>> + 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.
diff --git a/target/ppc/gdbstub.c b/target/ppc/gdbstub.c index 17e41fc113..65759e0c1c 100644 --- a/target/ppc/gdbstub.c +++ b/target/ppc/gdbstub.c @@ -23,6 +23,7 @@ #include "exec/helper-proto.h" #include "internal.h" #include "helper_regs.h" +#include "kvm_ppc.h" static int ppc_gdb_register_len_apple(int n) { 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, ®); + if (ret < 0) + { + fprintf(stderr, "Unable to set FPSCR to KVM: %s", strerror(errno)); + } +} diff --git a/target/ppc/kvm_ppc.h b/target/ppc/kvm_ppc.h index 989f61ace0..ff365e57a6 100644 --- a/target/ppc/kvm_ppc.h +++ b/target/ppc/kvm_ppc.h @@ -86,6 +86,12 @@ void kvmppc_set_reg_tb_offset(PowerPCCPU *cpu, int64_t tb_offset); int kvm_handle_nmi(PowerPCCPU *cpu, struct kvm_run *run); +#ifndef CONFIG_TCG +# define store_fpscr kvmppc_store_fpscr +#endif +void kvmppc_store_fpscr(CPUPPCState *env, uint64_t arg, uint32_t mask); + + #else static inline uint32_t kvmppc_get_tbfreq(void)
some common code needs to store information in fpscr, but this function relies on TCG cde to work. This patch adds a kvm way to do it, and a transparent way to call it when TCG is not compiled Signed-off-by: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br> --- target/ppc/gdbstub.c | 1 + target/ppc/kvm.c | 14 ++++++++++++++ target/ppc/kvm_ppc.h | 6 ++++++ 3 files changed, 21 insertions(+)