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.
--
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>
