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