| Message ID | 20221027100254.215253-5-richard.henderson@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | tcg: Fix x86 TARGET_TB_PCREL (#1269) | expand |
On 10/27/22 12:02, Richard Henderson wrote: > Since we do not plan to exit, use cpu_unwind_state_data > and extract exactly the data requested. > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > target/openrisc/sys_helper.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/target/openrisc/sys_helper.c b/target/openrisc/sys_helper.c > index a3508e421d..dde2fa1623 100644 > --- a/target/openrisc/sys_helper.c > +++ b/target/openrisc/sys_helper.c > @@ -199,6 +199,7 @@ target_ulong HELPER(mfspr)(CPUOpenRISCState *env, target_ulong rd, > target_ulong spr) > { > #ifndef CONFIG_USER_ONLY > + uint64_t data[TARGET_INSN_START_WORDS]; > MachineState *ms = MACHINE(qdev_get_machine()); > OpenRISCCPU *cpu = env_archcpu(env); > CPUState *cs = env_cpu(env); > @@ -232,14 +233,20 @@ target_ulong HELPER(mfspr)(CPUOpenRISCState *env, target_ulong rd, > return env->evbar; > > case TO_SPR(0, 16): /* NPC (equals PC) */ > - cpu_restore_state(cs, GETPC(), false); > + if (cpu_unwind_state_data(cs, GETPC(), data)) { > + return data[0]; > + } > return env->pc; > > case TO_SPR(0, 17): /* SR */ > return cpu_get_sr(env); > > case TO_SPR(0, 18): /* PPC */ > - cpu_restore_state(cs, GETPC(), false); > + if (cpu_unwind_state_data(cs, GETPC(), data)) { > + if (data[1] & 2) { > + return data[0] - 4; > + } > + } > return env->ppc; > > case TO_SPR(0, 32): /* EPCR */ I am struggling to understand if the fact that we are not setting cpu->env.dflag anymore in the mfspr helper is fine; here I am unfamiliar with the arch, also Ccing Philippe in case he wants to step in to review this bit. Thanks, CLaudio
On 10/28/22 20:16, Claudio Fontana wrote: > On 10/27/22 12:02, Richard Henderson wrote: >> Since we do not plan to exit, use cpu_unwind_state_data >> and extract exactly the data requested. >> >> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> >> --- >> target/openrisc/sys_helper.c | 11 +++++++++-- >> 1 file changed, 9 insertions(+), 2 deletions(-) >> >> diff --git a/target/openrisc/sys_helper.c b/target/openrisc/sys_helper.c >> index a3508e421d..dde2fa1623 100644 >> --- a/target/openrisc/sys_helper.c >> +++ b/target/openrisc/sys_helper.c >> @@ -199,6 +199,7 @@ target_ulong HELPER(mfspr)(CPUOpenRISCState *env, target_ulong rd, >> target_ulong spr) >> { >> #ifndef CONFIG_USER_ONLY >> + uint64_t data[TARGET_INSN_START_WORDS]; >> MachineState *ms = MACHINE(qdev_get_machine()); >> OpenRISCCPU *cpu = env_archcpu(env); >> CPUState *cs = env_cpu(env); >> @@ -232,14 +233,20 @@ target_ulong HELPER(mfspr)(CPUOpenRISCState *env, target_ulong rd, >> return env->evbar; >> >> case TO_SPR(0, 16): /* NPC (equals PC) */ >> - cpu_restore_state(cs, GETPC(), false); >> + if (cpu_unwind_state_data(cs, GETPC(), data)) { >> + return data[0]; >> + } >> return env->pc; >> >> case TO_SPR(0, 17): /* SR */ >> return cpu_get_sr(env); >> >> case TO_SPR(0, 18): /* PPC */ >> - cpu_restore_state(cs, GETPC(), false); >> + if (cpu_unwind_state_data(cs, GETPC(), data)) { >> + if (data[1] & 2) { >> + return data[0] - 4; >> + } >> + } >> return env->ppc; >> >> case TO_SPR(0, 32): /* EPCR */ > > I am struggling to understand if the fact that we are not setting cpu->env.dflag anymore in the mfspr helper is fine; That we might do so before was buggy. We manage dflag in openrisc_tr_tb_stop expecting a particular value. I should expand the patch comment on this. Consider: l.j L2 // branch l.mfspr r1, ppc // delay L1: boom L2: l.lwa r3, (r4) With "l.mfspr reg, ppc" executed in a delay slot, dflag would be set, but it would not be cleared by openrisc_tr_tb_stop on exiting the TB, because tb_stop thinks the value is already zero. The next TB begins at L2 with dflag set when it should not. If the load has a tlb miss, then the interrupt will be delivered with DSX set in the status register, and the pc decremented (openrisc implements restart after delay slot by re-executing the branch). This will cause the return from interrupt to go to L1, and boom! r~
diff --git a/target/openrisc/sys_helper.c b/target/openrisc/sys_helper.c index a3508e421d..dde2fa1623 100644 --- a/target/openrisc/sys_helper.c +++ b/target/openrisc/sys_helper.c @@ -199,6 +199,7 @@ target_ulong HELPER(mfspr)(CPUOpenRISCState *env, target_ulong rd, target_ulong spr) { #ifndef CONFIG_USER_ONLY + uint64_t data[TARGET_INSN_START_WORDS]; MachineState *ms = MACHINE(qdev_get_machine()); OpenRISCCPU *cpu = env_archcpu(env); CPUState *cs = env_cpu(env); @@ -232,14 +233,20 @@ target_ulong HELPER(mfspr)(CPUOpenRISCState *env, target_ulong rd, return env->evbar; case TO_SPR(0, 16): /* NPC (equals PC) */ - cpu_restore_state(cs, GETPC(), false); + if (cpu_unwind_state_data(cs, GETPC(), data)) { + return data[0]; + } return env->pc; case TO_SPR(0, 17): /* SR */ return cpu_get_sr(env); case TO_SPR(0, 18): /* PPC */ - cpu_restore_state(cs, GETPC(), false); + if (cpu_unwind_state_data(cs, GETPC(), data)) { + if (data[1] & 2) { + return data[0] - 4; + } + } return env->ppc; case TO_SPR(0, 32): /* EPCR */
Since we do not plan to exit, use cpu_unwind_state_data and extract exactly the data requested. Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- target/openrisc/sys_helper.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)