Re: [PATCH] target/openrisc: fix icount handling for timer instructions

[Stafford Horne]

On Wed, Mar 31, 2021 at 10:27:21AM +0300, Pavel Dovgalyuk wrote:
> On 31.03.2021 01:05, Stafford Horne wrote:
> > Hi Pavel,
> > 
> > Thanks for the patch.
> > 
> > On Mon, Mar 29, 2021 at 10:42:41AM +0300, Pavel Dovgalyuk wrote:
> > > This patch adds icount handling to mfspr/mtspr instructions
> > > that may deal with hardware timers.
> > > 
> > > Signed-off-by: Pavel Dovgalyuk <pavel.dovgalyuk@ispras.ru>
> > > ---
> > >   target/openrisc/translate.c |   15 +++++++++++++++
> > >   1 file changed, 15 insertions(+)
> > > 
> > > diff --git a/target/openrisc/translate.c b/target/openrisc/translate.c
> > > index c6dce879f1..a9c81f8bd5 100644
> > > --- a/target/openrisc/translate.c
> > > +++ b/target/openrisc/translate.c
> > > @@ -884,6 +884,18 @@ static bool trans_l_mfspr(DisasContext *dc, 
> > > arg_l_mfspr *a)
> > >           gen_illegal_exception(dc);
> > >       } else {
> > >           TCGv spr = tcg_temp_new();
> > > +
> > > +        if (tb_cflags(dc->base.tb) & CF_USE_ICOUNT) {
> > > +            gen_io_start();
> > > +            if (dc->delayed_branch) {
> > > +                tcg_gen_mov_tl(cpu_pc, jmp_pc);
> > > +                tcg_gen_discard_tl(jmp_pc);
> > > +            } else {
> > > +                tcg_gen_movi_tl(cpu_pc, dc->base.pc_next + 4);
> > > +            }
> > > +            dc->base.is_jmp = DISAS_EXIT;
> > > +        }
> > 
> > I don't know alot about how the icount works.  But I read this document to 
> > help
> > understand this patch.
> > 
> > https://qemu.readthedocs.io/en/latest/devel/tcg-icount.html
> > 
> > Could you explain why we need to exit the tb on mfspr?  This may just be 
> > reading
> > a timer value, but I am not sure why we need it?
> 
> Because virtual clock in icount mode is correct only at the end of the
> block.
> Allowing virtual clock reads in other places will make execution
> non-deterministic, because icount is updated to the value, which it gets
> after the block ends.

OK, got it.

> > 
> > >           tcg_gen_ori_tl(spr, cpu_R(dc, a->a), a->k);
> > >           gen_helper_mfspr(cpu_R(dc, a->d), cpu_env, cpu_R(dc, a->d), 
> > > spr);
> > >           tcg_temp_free(spr);
> > > @@ -898,6 +910,9 @@ static bool trans_l_mtspr(DisasContext *dc, 
> > > arg_l_mtspr *a)
> > >       } else {
> > >           TCGv spr;
> > > +        if (tb_cflags(dc->base.tb) & CF_USE_ICOUNT) {
> > > +            gen_io_start();
> > > +        }
> > 
> > Here and above, why do we need to call gen_io_start()?  This seems to need 
> > to be
> > called before io operations.
> 
> gen_io_start allows reading icount for the instruction.
> It is needed to prevent invalid reads in the middle of the block.
> 
> > 
> > This may all be OK, but could you help explain the theory of operation?  
> > Also,
> > have you tested this?
> 
> I have record/replay tests for openrisc, but I can't submit them without
> this patch, because they will fail.

OK.

Acked-by: Stafford Horne <shorne@gmail.com>

I am not currently maintaining an openrisc queue, but I could start one.  Do you
have another way to submit this upstream?

-Stafford