[Qemu-devel] [PATCH V2] RISC-V: fix single stepping over ret and other branching instructions

Fabien Chouteau posted 1 patch 5 years, 1 month ago
Test docker-mingw@fedora passed
Test docker-clang@ubuntu passed
Test asan passed
Test checkpatch passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20190325114554.7388-1-chouteau@adacore.com
Maintainers: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>, Palmer Dabbelt <palmer@sifive.com>, Alistair Francis <Alistair.Francis@wdc.com>, Sagar Karandikar <sagark@eecs.berkeley.edu>
.../riscv/insn_trans/trans_privileged.inc.c   |  8 ++---
target/riscv/insn_trans/trans_rvi.inc.c       |  6 ++--
target/riscv/translate.c                      | 30 +++++++++++++++----
3 files changed, 32 insertions(+), 12 deletions(-)
[Qemu-devel] [PATCH V2] RISC-V: fix single stepping over ret and other branching instructions
Posted by Fabien Chouteau 5 years, 1 month ago
This patch introduces wrappers around the tcg_gen_exit_tb() and
tcg_gen_lookup_and_goto_ptr() functions that handle single stepping,
i.e. call gen_exception_debug() when single stepping is enabled.

Theses functions are then used instead of the originals, bringing single
stepping handling in places where it was previously ignored such as jalr
and system branch instructions (ecall, mret, sret, etc.).

Signed-off-by: Fabien Chouteau <chouteau@adacore.com>
---
 .../riscv/insn_trans/trans_privileged.inc.c   |  8 ++---
 target/riscv/insn_trans/trans_rvi.inc.c       |  6 ++--
 target/riscv/translate.c                      | 30 +++++++++++++++----
 3 files changed, 32 insertions(+), 12 deletions(-)

diff --git a/target/riscv/insn_trans/trans_privileged.inc.c b/target/riscv/insn_trans/trans_privileged.inc.c
index acb605923e..664d6ba3f2 100644
--- a/target/riscv/insn_trans/trans_privileged.inc.c
+++ b/target/riscv/insn_trans/trans_privileged.inc.c
@@ -22,7 +22,7 @@ static bool trans_ecall(DisasContext *ctx, arg_ecall *a)
 {
     /* always generates U-level ECALL, fixed in do_interrupt handler */
     generate_exception(ctx, RISCV_EXCP_U_ECALL);
-    tcg_gen_exit_tb(NULL, 0); /* no chaining */
+    exit_tb(ctx); /* no chaining */
     ctx->base.is_jmp = DISAS_NORETURN;
     return true;
 }
@@ -30,7 +30,7 @@ static bool trans_ecall(DisasContext *ctx, arg_ecall *a)
 static bool trans_ebreak(DisasContext *ctx, arg_ebreak *a)
 {
     generate_exception(ctx, RISCV_EXCP_BREAKPOINT);
-    tcg_gen_exit_tb(NULL, 0); /* no chaining */
+    exit_tb(ctx); /* no chaining */
     ctx->base.is_jmp = DISAS_NORETURN;
     return true;
 }
@@ -47,7 +47,7 @@ static bool trans_sret(DisasContext *ctx, arg_sret *a)
 
     if (has_ext(ctx, RVS)) {
         gen_helper_sret(cpu_pc, cpu_env, cpu_pc);
-        tcg_gen_exit_tb(NULL, 0); /* no chaining */
+        exit_tb(ctx); /* no chaining */
         ctx->base.is_jmp = DISAS_NORETURN;
     } else {
         return false;
@@ -68,7 +68,7 @@ static bool trans_mret(DisasContext *ctx, arg_mret *a)
 #ifndef CONFIG_USER_ONLY
     tcg_gen_movi_tl(cpu_pc, ctx->base.pc_next);
     gen_helper_mret(cpu_pc, cpu_env, cpu_pc);
-    tcg_gen_exit_tb(NULL, 0); /* no chaining */
+    exit_tb(ctx); /* no chaining */
     ctx->base.is_jmp = DISAS_NORETURN;
     return true;
 #else
diff --git a/target/riscv/insn_trans/trans_rvi.inc.c b/target/riscv/insn_trans/trans_rvi.inc.c
index d420a4d8b2..37b0b9dd19 100644
--- a/target/riscv/insn_trans/trans_rvi.inc.c
+++ b/target/riscv/insn_trans/trans_rvi.inc.c
@@ -60,7 +60,7 @@ static bool trans_jalr(DisasContext *ctx, arg_jalr *a)
     if (a->rd != 0) {
         tcg_gen_movi_tl(cpu_gpr[a->rd], ctx->pc_succ_insn);
     }
-    tcg_gen_lookup_and_goto_ptr();
+    lookup_and_goto_ptr(ctx);
 
     if (misaligned) {
         gen_set_label(misaligned);
@@ -483,7 +483,7 @@ static bool trans_fence_i(DisasContext *ctx, arg_fence_i *a)
      * however we need to end the translation block
      */
     tcg_gen_movi_tl(cpu_pc, ctx->pc_succ_insn);
-    tcg_gen_exit_tb(NULL, 0);
+    exit_tb(ctx);
     ctx->base.is_jmp = DISAS_NORETURN;
     return true;
 }
@@ -504,7 +504,7 @@ static bool trans_fence_i(DisasContext *ctx, arg_fence_i *a)
     gen_io_end(); \
     gen_set_gpr(a->rd, dest); \
     tcg_gen_movi_tl(cpu_pc, ctx->pc_succ_insn); \
-    tcg_gen_exit_tb(NULL, 0); \
+    exit_tb(ctx); \
     ctx->base.is_jmp = DISAS_NORETURN; \
     tcg_temp_free(source1); \
     tcg_temp_free(csr_store); \
diff --git a/target/riscv/translate.c b/target/riscv/translate.c
index 049fa65c66..7417c532f2 100644
--- a/target/riscv/translate.c
+++ b/target/riscv/translate.c
@@ -109,6 +109,26 @@ static void gen_exception_debug(void)
     tcg_temp_free_i32(helper_tmp);
 }
 
+/* Wrapper around tcg_gen_exit_tb that handles single stepping */
+static void exit_tb(DisasContext *ctx)
+{
+    if (ctx->base.singlestep_enabled) {
+        gen_exception_debug();
+    } else {
+        tcg_gen_exit_tb(NULL, 0);
+    }
+}
+
+/* Wrapper around tcg_gen_lookup_and_goto_ptr that handles single stepping */
+static void lookup_and_goto_ptr(DisasContext *ctx)
+{
+    if (ctx->base.singlestep_enabled) {
+        gen_exception_debug();
+    } else {
+        tcg_gen_lookup_and_goto_ptr();
+    }
+}
+
 static void gen_exception_illegal(DisasContext *ctx)
 {
     generate_exception(ctx, RISCV_EXCP_ILLEGAL_INST);
@@ -138,14 +158,14 @@ static void gen_goto_tb(DisasContext *ctx, int n, target_ulong dest)
         /* chaining is only allowed when the jump is to the same page */
         tcg_gen_goto_tb(n);
         tcg_gen_movi_tl(cpu_pc, dest);
+
+        /* No need to check for single stepping here as use_goto_tb() will
+         * return false in case of single stepping.
+         */
         tcg_gen_exit_tb(ctx->base.tb, n);
     } else {
         tcg_gen_movi_tl(cpu_pc, dest);
-        if (ctx->base.singlestep_enabled) {
-            gen_exception_debug();
-        } else {
-            tcg_gen_lookup_and_goto_ptr();
-        }
+        lookup_and_goto_ptr(ctx);
     }
 }
 
-- 
2.17.1


Re: [Qemu-devel] [PATCH V2] RISC-V: fix single stepping over ret and other branching instructions
Posted by Richard Henderson 5 years, 1 month ago
On 3/25/19 4:45 AM, Fabien Chouteau wrote:
> This patch introduces wrappers around the tcg_gen_exit_tb() and
> tcg_gen_lookup_and_goto_ptr() functions that handle single stepping,
> i.e. call gen_exception_debug() when single stepping is enabled.
> 
> Theses functions are then used instead of the originals, bringing single
> stepping handling in places where it was previously ignored such as jalr
> and system branch instructions (ecall, mret, sret, etc.).
> 
> Signed-off-by: Fabien Chouteau <chouteau@adacore.com>
> ---
>  .../riscv/insn_trans/trans_privileged.inc.c   |  8 ++---
>  target/riscv/insn_trans/trans_rvi.inc.c       |  6 ++--
>  target/riscv/translate.c                      | 30 +++++++++++++++----
>  3 files changed, 32 insertions(+), 12 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [PATCH V2] RISC-V: fix single stepping over ret and other branching instructions
Posted by Alistair Francis 5 years, 1 month ago
On Mon, Mar 25, 2019 at 9:59 AM Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 3/25/19 4:45 AM, Fabien Chouteau wrote:
> > This patch introduces wrappers around the tcg_gen_exit_tb() and
> > tcg_gen_lookup_and_goto_ptr() functions that handle single stepping,
> > i.e. call gen_exception_debug() when single stepping is enabled.
> >
> > Theses functions are then used instead of the originals, bringing single
> > stepping handling in places where it was previously ignored such as jalr
> > and system branch instructions (ecall, mret, sret, etc.).
> >
> > Signed-off-by: Fabien Chouteau <chouteau@adacore.com>
> > ---
> >  .../riscv/insn_trans/trans_privileged.inc.c   |  8 ++---
> >  target/riscv/insn_trans/trans_rvi.inc.c       |  6 ++--
> >  target/riscv/translate.c                      | 30 +++++++++++++++----
> >  3 files changed, 32 insertions(+), 12 deletions(-)
>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

>
>
> r~
>