Re: [PATCH v2 4/6] target/openrisc: Use cpu_unwind_state_data for mfspr

[Richard Henderson]

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~