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.dovgal...@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 <sho...@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
