qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Pavel Dovgalyuk
Subject: Re: [PATCH] target/openrisc: fix icount handling for timer instructions
Date: Wed, 31 Mar 2021 15:48:58 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.7.1

CC'ed Paolo.


On 31.03.2021 15:33, Stafford Horne wrote:
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?

Paolo, can you queue this one?

Pavel Dovgalyuk



reply via email to

[Prev in Thread] Current Thread [Next in Thread]