* [PATCH v2 1/2] target/riscv: Reduce duplicated code in trans_rvh.c.inc
@ 2021-02-22 18:49 Alex Richardson
2021-02-22 18:49 ` [PATCH v2 2/2] target/riscv: Call check_access() before tcg_temp_new() Alex Richardson
2021-02-24 0:24 ` [PATCH v2 1/2] target/riscv: Reduce duplicated code in trans_rvh.c.inc Richard Henderson
0 siblings, 2 replies; 4+ messages in thread
From: Alex Richardson @ 2021-02-22 18:49 UTC (permalink / raw)
Cc: open list:RISC-V TCG CPUs, Sagar Karandikar, Bastian Koppelmann,
open list:All patches CC here, Alistair Francis, Alex Richardson,
Palmer Dabbelt
I rencently merged CHERI QEMU up to 5.2, and we have to modify all
functions that perform memory accesses. Factoring these almost-indentical
functions into two shared helpers makes our changes a lot smaller and
should also make this code easier to maintain.
---
target/riscv/insn_trans/trans_rvh.c.inc | 199 ++++--------------------
1 file changed, 29 insertions(+), 170 deletions(-)
diff --git a/target/riscv/insn_trans/trans_rvh.c.inc b/target/riscv/insn_trans/trans_rvh.c.inc
index ce7ed5affb..c66268a9b0 100644
--- a/target/riscv/insn_trans/trans_rvh.c.inc
+++ b/target/riscv/insn_trans/trans_rvh.c.inc
@@ -28,7 +28,7 @@ static void check_access(DisasContext *ctx) {
}
#endif
-static bool trans_hlv_b(DisasContext *ctx, arg_hlv_b *a)
+static bool gen_hlv(DisasContext *ctx, int rs1, int rd, MemOp memop)
{
REQUIRE_EXT(ctx, RVH);
#ifndef CONFIG_USER_ONLY
@@ -37,10 +37,10 @@ static bool trans_hlv_b(DisasContext *ctx, arg_hlv_b *a)
check_access(ctx);
- gen_get_gpr(t0, a->rs1);
+ gen_get_gpr(t0, rs1);
- tcg_gen_qemu_ld_tl(t1, t0, ctx->mem_idx | TB_FLAGS_PRIV_HYP_ACCESS_MASK, MO_SB);
- gen_set_gpr(a->rd, t1);
+ tcg_gen_qemu_ld_tl(t1, t0, ctx->mem_idx | TB_FLAGS_PRIV_HYP_ACCESS_MASK, memop);
+ gen_set_gpr(rd, t1);
tcg_temp_free(t0);
tcg_temp_free(t1);
@@ -50,224 +50,83 @@ static bool trans_hlv_b(DisasContext *ctx, arg_hlv_b *a)
#endif
}
-static bool trans_hlv_h(DisasContext *ctx, arg_hlv_h *a)
+
+static bool gen_hsv(DisasContext *ctx, int rs1, int rs2, MemOp memop)
{
REQUIRE_EXT(ctx, RVH);
#ifndef CONFIG_USER_ONLY
TCGv t0 = tcg_temp_new();
- TCGv t1 = tcg_temp_new();
+ TCGv dat = tcg_temp_new();
check_access(ctx);
- gen_get_gpr(t0, a->rs1);
+ gen_get_gpr(t0, rs1);
+ gen_get_gpr(dat, rs2);
- tcg_gen_qemu_ld_tl(t1, t0, ctx->mem_idx | TB_FLAGS_PRIV_HYP_ACCESS_MASK, MO_TESW);
- gen_set_gpr(a->rd, t1);
+ tcg_gen_qemu_st_tl(dat, t0, ctx->mem_idx | TB_FLAGS_PRIV_HYP_ACCESS_MASK, memop);
tcg_temp_free(t0);
- tcg_temp_free(t1);
+ tcg_temp_free(dat);
return true;
#else
return false;
#endif
}
-static bool trans_hlv_w(DisasContext *ctx, arg_hlv_w *a)
+static bool trans_hlv_b(DisasContext *ctx, arg_hlv_b *a)
{
- REQUIRE_EXT(ctx, RVH);
-#ifndef CONFIG_USER_ONLY
- TCGv t0 = tcg_temp_new();
- TCGv t1 = tcg_temp_new();
-
- check_access(ctx);
-
- gen_get_gpr(t0, a->rs1);
+ return gen_hlv(ctx, a->rs1, a->rd, MO_SB);
+}
- tcg_gen_qemu_ld_tl(t1, t0, ctx->mem_idx | TB_FLAGS_PRIV_HYP_ACCESS_MASK, MO_TESL);
- gen_set_gpr(a->rd, t1);
+static bool trans_hlv_h(DisasContext *ctx, arg_hlv_h *a)
+{
+ return gen_hlv(ctx, a->rs1, a->rd, MO_TESW);
+}
- tcg_temp_free(t0);
- tcg_temp_free(t1);
- return true;
-#else
- return false;
-#endif
+static bool trans_hlv_w(DisasContext *ctx, arg_hlv_w *a)
+{
+ return gen_hlv(ctx, a->rs1, a->rd, MO_TESL);
}
static bool trans_hlv_bu(DisasContext *ctx, arg_hlv_bu *a)
{
- REQUIRE_EXT(ctx, RVH);
-#ifndef CONFIG_USER_ONLY
- TCGv t0 = tcg_temp_new();
- TCGv t1 = tcg_temp_new();
-
- check_access(ctx);
-
- gen_get_gpr(t0, a->rs1);
-
- tcg_gen_qemu_ld_tl(t1, t0, ctx->mem_idx | TB_FLAGS_PRIV_HYP_ACCESS_MASK, MO_UB);
- gen_set_gpr(a->rd, t1);
-
- tcg_temp_free(t0);
- tcg_temp_free(t1);
- return true;
-#else
- return false;
-#endif
+ return gen_hlv(ctx, a->rs1, a->rd, MO_UB);
}
static bool trans_hlv_hu(DisasContext *ctx, arg_hlv_hu *a)
{
- REQUIRE_EXT(ctx, RVH);
-#ifndef CONFIG_USER_ONLY
- TCGv t0 = tcg_temp_new();
- TCGv t1 = tcg_temp_new();
-
- check_access(ctx);
-
- gen_get_gpr(t0, a->rs1);
- tcg_gen_qemu_ld_tl(t1, t0, ctx->mem_idx | TB_FLAGS_PRIV_HYP_ACCESS_MASK, MO_TEUW);
- gen_set_gpr(a->rd, t1);
-
- tcg_temp_free(t0);
- tcg_temp_free(t1);
- return true;
-#else
- return false;
-#endif
+ return gen_hlv(ctx, a->rs1, a->rd, MO_TEUW);
}
static bool trans_hsv_b(DisasContext *ctx, arg_hsv_b *a)
{
- REQUIRE_EXT(ctx, RVH);
-#ifndef CONFIG_USER_ONLY
- TCGv t0 = tcg_temp_new();
- TCGv dat = tcg_temp_new();
-
- check_access(ctx);
-
- gen_get_gpr(t0, a->rs1);
- gen_get_gpr(dat, a->rs2);
-
- tcg_gen_qemu_st_tl(dat, t0, ctx->mem_idx | TB_FLAGS_PRIV_HYP_ACCESS_MASK, MO_SB);
-
- tcg_temp_free(t0);
- tcg_temp_free(dat);
- return true;
-#else
- return false;
-#endif
+ return gen_hlv(ctx, a->rs1, a->rs2, MO_SB);
}
static bool trans_hsv_h(DisasContext *ctx, arg_hsv_h *a)
{
- REQUIRE_EXT(ctx, RVH);
-#ifndef CONFIG_USER_ONLY
- TCGv t0 = tcg_temp_new();
- TCGv dat = tcg_temp_new();
-
- check_access(ctx);
-
- gen_get_gpr(t0, a->rs1);
- gen_get_gpr(dat, a->rs2);
-
- tcg_gen_qemu_st_tl(dat, t0, ctx->mem_idx | TB_FLAGS_PRIV_HYP_ACCESS_MASK, MO_TESW);
-
- tcg_temp_free(t0);
- tcg_temp_free(dat);
- return true;
-#else
- return false;
-#endif
+ return gen_hlv(ctx, a->rs1, a->rs2, MO_TESW);
}
static bool trans_hsv_w(DisasContext *ctx, arg_hsv_w *a)
{
- REQUIRE_EXT(ctx, RVH);
-#ifndef CONFIG_USER_ONLY
- TCGv t0 = tcg_temp_new();
- TCGv dat = tcg_temp_new();
-
- check_access(ctx);
-
- gen_get_gpr(t0, a->rs1);
- gen_get_gpr(dat, a->rs2);
-
- tcg_gen_qemu_st_tl(dat, t0, ctx->mem_idx | TB_FLAGS_PRIV_HYP_ACCESS_MASK, MO_TESL);
-
- tcg_temp_free(t0);
- tcg_temp_free(dat);
- return true;
-#else
- return false;
-#endif
+ return gen_hlv(ctx, a->rs1, a->rs2, MO_TESL);
}
#ifdef TARGET_RISCV64
static bool trans_hlv_wu(DisasContext *ctx, arg_hlv_wu *a)
{
- REQUIRE_EXT(ctx, RVH);
-#ifndef CONFIG_USER_ONLY
- TCGv t0 = tcg_temp_new();
- TCGv t1 = tcg_temp_new();
-
- check_access(ctx);
-
- gen_get_gpr(t0, a->rs1);
-
- tcg_gen_qemu_ld_tl(t1, t0, ctx->mem_idx | TB_FLAGS_PRIV_HYP_ACCESS_MASK, MO_TEUL);
- gen_set_gpr(a->rd, t1);
-
- tcg_temp_free(t0);
- tcg_temp_free(t1);
- return true;
-#else
- return false;
-#endif
+ return gen_hlv(ctx, a->rs1, a->rd, MO_TEUL);
}
static bool trans_hlv_d(DisasContext *ctx, arg_hlv_d *a)
{
- REQUIRE_EXT(ctx, RVH);
-#ifndef CONFIG_USER_ONLY
- TCGv t0 = tcg_temp_new();
- TCGv t1 = tcg_temp_new();
-
- check_access(ctx);
-
- gen_get_gpr(t0, a->rs1);
-
- tcg_gen_qemu_ld_tl(t1, t0, ctx->mem_idx | TB_FLAGS_PRIV_HYP_ACCESS_MASK, MO_TEQ);
- gen_set_gpr(a->rd, t1);
-
- tcg_temp_free(t0);
- tcg_temp_free(t1);
- return true;
-#else
- return false;
-#endif
+ return gen_hlv(ctx, a->rs1, a->rd, MO_TEQ);
}
static bool trans_hsv_d(DisasContext *ctx, arg_hsv_d *a)
{
- REQUIRE_EXT(ctx, RVH);
-#ifndef CONFIG_USER_ONLY
- TCGv t0 = tcg_temp_new();
- TCGv dat = tcg_temp_new();
-
- check_access(ctx);
-
- gen_get_gpr(t0, a->rs1);
- gen_get_gpr(dat, a->rs2);
-
- tcg_gen_qemu_st_tl(dat, t0, ctx->mem_idx | TB_FLAGS_PRIV_HYP_ACCESS_MASK, MO_TEQ);
-
- tcg_temp_free(t0);
- tcg_temp_free(dat);
- return true;
-#else
- return false;
-#endif
+ return gen_hlv(ctx, a->rs1, a->rs2, MO_TEQ);
}
#endif
--
2.30.0
^ permalink raw reply related [flat|nested] 4+ messages in thread* [PATCH v2 2/2] target/riscv: Call check_access() before tcg_temp_new()
2021-02-22 18:49 [PATCH v2 1/2] target/riscv: Reduce duplicated code in trans_rvh.c.inc Alex Richardson
@ 2021-02-22 18:49 ` Alex Richardson
2021-02-24 0:31 ` Richard Henderson
2021-02-24 0:24 ` [PATCH v2 1/2] target/riscv: Reduce duplicated code in trans_rvh.c.inc Richard Henderson
1 sibling, 1 reply; 4+ messages in thread
From: Alex Richardson @ 2021-02-22 18:49 UTC (permalink / raw)
Cc: open list:RISC-V TCG CPUs, Sagar Karandikar, Bastian Koppelmann,
open list:All patches CC here, Alistair Francis, Alex Richardson,
Palmer Dabbelt
---
target/riscv/insn_trans/trans_rvh.c.inc | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/target/riscv/insn_trans/trans_rvh.c.inc b/target/riscv/insn_trans/trans_rvh.c.inc
index c66268a9b0..c6bbc54d68 100644
--- a/target/riscv/insn_trans/trans_rvh.c.inc
+++ b/target/riscv/insn_trans/trans_rvh.c.inc
@@ -32,11 +32,11 @@ static bool gen_hlv(DisasContext *ctx, int rs1, int rd, MemOp memop)
{
REQUIRE_EXT(ctx, RVH);
#ifndef CONFIG_USER_ONLY
+ check_access(ctx);
+
TCGv t0 = tcg_temp_new();
TCGv t1 = tcg_temp_new();
- check_access(ctx);
-
gen_get_gpr(t0, rs1);
tcg_gen_qemu_ld_tl(t1, t0, ctx->mem_idx | TB_FLAGS_PRIV_HYP_ACCESS_MASK, memop);
@@ -55,11 +55,11 @@ static bool gen_hsv(DisasContext *ctx, int rs1, int rs2, MemOp memop)
{
REQUIRE_EXT(ctx, RVH);
#ifndef CONFIG_USER_ONLY
+ check_access(ctx);
+
TCGv t0 = tcg_temp_new();
TCGv dat = tcg_temp_new();
- check_access(ctx);
-
gen_get_gpr(t0, rs1);
gen_get_gpr(dat, rs2);
@@ -134,11 +134,11 @@ static bool trans_hlvx_hu(DisasContext *ctx, arg_hlvx_hu *a)
{
REQUIRE_EXT(ctx, RVH);
#ifndef CONFIG_USER_ONLY
+ check_access(ctx);
+
TCGv t0 = tcg_temp_new();
TCGv t1 = tcg_temp_new();
- check_access(ctx);
-
gen_get_gpr(t0, a->rs1);
gen_helper_hyp_hlvx_hu(t1, cpu_env, t0);
@@ -156,11 +156,11 @@ static bool trans_hlvx_wu(DisasContext *ctx, arg_hlvx_wu *a)
{
REQUIRE_EXT(ctx, RVH);
#ifndef CONFIG_USER_ONLY
+ check_access(ctx);
+
TCGv t0 = tcg_temp_new();
TCGv t1 = tcg_temp_new();
- check_access(ctx);
-
gen_get_gpr(t0, a->rs1);
gen_helper_hyp_hlvx_wu(t1, cpu_env, t0);
--
2.30.0
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH v2 2/2] target/riscv: Call check_access() before tcg_temp_new()
2021-02-22 18:49 ` [PATCH v2 2/2] target/riscv: Call check_access() before tcg_temp_new() Alex Richardson
@ 2021-02-24 0:31 ` Richard Henderson
0 siblings, 0 replies; 4+ messages in thread
From: Richard Henderson @ 2021-02-24 0:31 UTC (permalink / raw)
To: Alex Richardson
Cc: open list:RISC-V TCG CPUs, Sagar Karandikar, Bastian Koppelmann,
open list:All patches CC here, Alistair Francis, Palmer Dabbelt
On 2/22/21 10:49 AM, Alex Richardson wrote:
> ---
> target/riscv/insn_trans/trans_rvh.c.inc | 16 ++++++++--------
> 1 file changed, 8 insertions(+), 8 deletions(-)
Any particular reason?
At first I thought this was a macro with a hidden return, but it isn't. I
would understand a cleanup like
if (check_access(ctx)) {
TCGv t0 = tcg_temp_new();
TCGv t1 = tcg_temp_new();
...
where we do not generate the rest of the code if we raised an exception.
Alternately, if the nesting seems ugly,
if (!check_access(ctx)) {
return true;
}
...
return true;
r~
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2 1/2] target/riscv: Reduce duplicated code in trans_rvh.c.inc
2021-02-22 18:49 [PATCH v2 1/2] target/riscv: Reduce duplicated code in trans_rvh.c.inc Alex Richardson
2021-02-22 18:49 ` [PATCH v2 2/2] target/riscv: Call check_access() before tcg_temp_new() Alex Richardson
@ 2021-02-24 0:24 ` Richard Henderson
1 sibling, 0 replies; 4+ messages in thread
From: Richard Henderson @ 2021-02-24 0:24 UTC (permalink / raw)
To: Alex Richardson
Cc: open list:RISC-V TCG CPUs, Sagar Karandikar, Bastian Koppelmann,
open list:All patches CC here, Alistair Francis, Palmer Dabbelt
On 2/22/21 10:49 AM, Alex Richardson wrote:
> I rencently merged CHERI QEMU up to 5.2, and we have to modify all
> functions that perform memory accesses. Factoring these almost-indentical
> functions into two shared helpers makes our changes a lot smaller and
> should also make this code easier to maintain.
> ---
> target/riscv/insn_trans/trans_rvh.c.inc | 199 ++++--------------------
> 1 file changed, 29 insertions(+), 170 deletions(-)
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
r~
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-02-24 0:32 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-02-22 18:49 [PATCH v2 1/2] target/riscv: Reduce duplicated code in trans_rvh.c.inc Alex Richardson
2021-02-22 18:49 ` [PATCH v2 2/2] target/riscv: Call check_access() before tcg_temp_new() Alex Richardson
2021-02-24 0:31 ` Richard Henderson
2021-02-24 0:24 ` [PATCH v2 1/2] target/riscv: Reduce duplicated code in trans_rvh.c.inc Richard Henderson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).