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

[Stafford Horne]

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?

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

This may all be OK, but could you help explain the theory of operation?  Also,
have you tested this?

-Stafford