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~