diff mbox series

[v2,05/14] target/riscv: Calculate address according to XLEN

Message ID 20211110070452.48539-6-zhiwei_liu@c-sky.com (mailing list archive)
State New, archived
Headers show
Series Support UXL filed in xstatus | expand

Commit Message

LIU Zhiwei Nov. 10, 2021, 7:04 a.m. UTC 
Signed-off-by: LIU Zhiwei <zhiwei_liu@c-sky.com>
---
 target/riscv/insn_trans/trans_rvd.c.inc | 23 ++---------------------
 target/riscv/insn_trans/trans_rvf.c.inc | 23 ++---------------------
 target/riscv/insn_trans/trans_rvi.c.inc | 18 ++----------------
 target/riscv/translate.c                | 13 +++++++++++++
 4 files changed, 19 insertions(+), 58 deletions(-)

Comments

Richard Henderson Nov. 10, 2021, 10:52 a.m. UTC | #1 
On 11/10/21 8:04 AM, LIU Zhiwei wrote:
> Signed-off-by: LIU Zhiwei <zhiwei_liu@c-sky.com>
> ---
>   target/riscv/insn_trans/trans_rvd.c.inc | 23 ++---------------------
>   target/riscv/insn_trans/trans_rvf.c.inc | 23 ++---------------------
>   target/riscv/insn_trans/trans_rvi.c.inc | 18 ++----------------
>   target/riscv/translate.c                | 13 +++++++++++++
>   4 files changed, 19 insertions(+), 58 deletions(-)
> 
> diff --git a/target/riscv/insn_trans/trans_rvd.c.inc b/target/riscv/insn_trans/trans_rvd.c.inc
> index 64fb0046f7..29066a8ef3 100644
> --- a/target/riscv/insn_trans/trans_rvd.c.inc
> +++ b/target/riscv/insn_trans/trans_rvd.c.inc
> @@ -20,19 +20,10 @@
>   
>   static bool trans_fld(DisasContext *ctx, arg_fld *a)
>   {
> -    TCGv addr;
> -
> +    TCGv addr = get_address(ctx, a->rs1, a->imm);
>       REQUIRE_FPU;
>       REQUIRE_EXT(ctx, RVD);

It would be better to leave the address calculation after the REQUIRE checks.

> +static TCGv get_address(DisasContext *ctx, int rs1, int imm)
> +{
> +    TCGv addr = temp_new(ctx);
> +    TCGv src1 = get_gpr(ctx, rs1, EXT_NONE);
> +
> +    tcg_gen_addi_tl(addr, src1, imm);
> +    addr = gen_pm_adjust_address(ctx, addr);
> +    if (get_xl(ctx) == MXL_RV32) {
> +        tcg_gen_ext32u_tl(addr, addr);
> +    }
> +    return addr;
> +}

I suspect the extend should come before the pointer mask and not after, but this is is a 
weakness in the current RVJ spec that it does not specify how the extension interacts with 
UXL.  (The reverse ordering would allow a 64-bit os to place a 32-bit application at a 
base address above 4gb, which could allow address separation without paging enabled.)

I do think we should merge gen_pm_adjust_address into this function so that we only create 
one new temporary.


r~
LIU Zhiwei Nov. 10, 2021, 1:44 p.m. UTC | #2 
On 2021/11/10 下午6:52, Richard Henderson wrote:
> On 11/10/21 8:04 AM, LIU Zhiwei wrote:
>> Signed-off-by: LIU Zhiwei <zhiwei_liu@c-sky.com>
>> ---
>>   target/riscv/insn_trans/trans_rvd.c.inc | 23 ++---------------------
>>   target/riscv/insn_trans/trans_rvf.c.inc | 23 ++---------------------
>>   target/riscv/insn_trans/trans_rvi.c.inc | 18 ++----------------
>>   target/riscv/translate.c                | 13 +++++++++++++
>>   4 files changed, 19 insertions(+), 58 deletions(-)
>>
>> diff --git a/target/riscv/insn_trans/trans_rvd.c.inc 
>> b/target/riscv/insn_trans/trans_rvd.c.inc
>> index 64fb0046f7..29066a8ef3 100644
>> --- a/target/riscv/insn_trans/trans_rvd.c.inc
>> +++ b/target/riscv/insn_trans/trans_rvd.c.inc
>> @@ -20,19 +20,10 @@
>>     static bool trans_fld(DisasContext *ctx, arg_fld *a)
>>   {
>> -    TCGv addr;
>> -
>> +    TCGv addr = get_address(ctx, a->rs1, a->imm);
>>       REQUIRE_FPU;
>>       REQUIRE_EXT(ctx, RVD);
>
> It would be better to leave the address calculation after the REQUIRE 
> checks.
OK.
>
>> +static TCGv get_address(DisasContext *ctx, int rs1, int imm)
>> +{
>> +    TCGv addr = temp_new(ctx);
>> +    TCGv src1 = get_gpr(ctx, rs1, EXT_NONE);
>> +
>> +    tcg_gen_addi_tl(addr, src1, imm);
>> +    addr = gen_pm_adjust_address(ctx, addr);
>> +    if (get_xl(ctx) == MXL_RV32) {
>> +        tcg_gen_ext32u_tl(addr, addr);
>> +    }
>> +    return addr;
>> +}
>
> I suspect the extend should come before the pointer mask and not 
> after, but this is is a weakness in the current RVJ spec that it does 
> not specify how the extension interacts with UXL.  (The reverse 
> ordering would allow a 64-bit os to place a 32-bit application at a 
> base address above 4gb, which could allow address separation without 
> paging enabled.)

Agree. Should we adjust currently, or just add a TODO comment here?

>
> I do think we should merge gen_pm_adjust_address into this function so 
> that we only create one new temporary.

I think custom instructions will be added in the future. And possibly 
there will be  some custom load/store instructions.
If we merge gen_pm_adjust_address,  we may have to split it once again 
at that time.

Thanks,
Zhiwei

>
>
> r~
Richard Henderson Nov. 10, 2021, 2:40 p.m. UTC | #3 
On 11/10/21 2:44 PM, LIU Zhiwei wrote:
>> I suspect the extend should come before the pointer mask and not after, but this is is a 
>> weakness in the current RVJ spec that it does not specify how the extension interacts 
>> with UXL.  (The reverse ordering would allow a 64-bit os to place a 32-bit application 
>> at a base address above 4gb, which could allow address separation without paging enabled.)
> 
> Agree. Should we adjust currently, or just add a TODO comment here?

Let's add a todo comment for sure.

>> I do think we should merge gen_pm_adjust_address into this function so that we only 
>> create one new temporary.
> 
> I think custom instructions will be added in the future. And possibly there will be  some 
> custom load/store instructions.
> If we merge gen_pm_adjust_address,  we may have to split it once again at that time.

I don't think so.  We're simply having one function to compute a canonical address from a 
register plus offset plus mods.

Also, patch 10 combines pm-mask with zero-extension, so we shouldn't need to do both here. 
  The checks should be combined like

     tcg_gen_addi_tl(addr, src1, imm);
     if (ctx->pm_enabled) {
         tcg_gen_and_tl(addr, addr, pm_mask);
         tcg_gen_or_tl(addr, addr, pm_base);
     } else if (get_xl(ctx) == MXL_RV32) {
         tcg_gen_ext32u_tl(addr, addr);
     }

and could possibly be extended to

     if (ctx->pm_mask_enabled) {
         tcg_gen_and_tl(addr, addr, pm_mask);
     } else if (get_xl(ctx) == MXL_RV32) {
         tcg_gen_ext32u_tl(addr, addr);
     }
     if (ctx->pm_base_enabled) {
         tcg_gen_or_tl(addr, addr, pm_base);
     }

with one more bit in TB_FLAGS, e.g.

     if (env->cur_pm_mask < (xl == MVL_RV32 ? UINT32_MAX : UINT64_MAX)) {
         flags = FIELD_DP32(flags, TB_FLAGS, PM_MASK_ENABLED, 1);
     }
     if (env->cur_pm_base != 0) {
         flags = FIELD_DP32(flags, TB_FLAGS, PM_BASE_ENABLED, 1);
     }


r~
LIU Zhiwei Nov. 11, 2021, 5:04 a.m. UTC | #4 
On 2021/11/10 下午10:40, Richard Henderson wrote:
> On 11/10/21 2:44 PM, LIU Zhiwei wrote:
>>> I suspect the extend should come before the pointer mask and not 
>>> after, but this is is a weakness in the current RVJ spec that it 
>>> does not specify how the extension interacts with UXL.  (The reverse 
>>> ordering would allow a 64-bit os to place a 32-bit application at a 
>>> base address above 4gb, which could allow address separation without 
>>> paging enabled.)
>>
>> Agree. Should we adjust currently, or just add a TODO comment here?
>
> Let's add a todo comment for sure.
>
>>> I do think we should merge gen_pm_adjust_address into this function 
>>> so that we only create one new temporary.
>>
>> I think custom instructions will be added in the future. And possibly 
>> there will be  some custom load/store instructions.
>> If we merge gen_pm_adjust_address,  we may have to split it once 
>> again at that time.
>
> I don't think so.  We're simply having one function to compute a 
> canonical address from a register plus offset plus mods.
>
> Also, patch 10 combines pm-mask with zero-extension, so we shouldn't 
> need to do both here.  The checks should be combined like
>
>     tcg_gen_addi_tl(addr, src1, imm);
>     if (ctx->pm_enabled) {
>         tcg_gen_and_tl(addr, addr, pm_mask);
>         tcg_gen_or_tl(addr, addr, pm_base);
>     } else if (get_xl(ctx) == MXL_RV32) {
>         tcg_gen_ext32u_tl(addr, addr);
>     }
>
> and could possibly be extended to
>
>     if (ctx->pm_mask_enabled) {
>         tcg_gen_and_tl(addr, addr, pm_mask);
>     } else if (get_xl(ctx) == MXL_RV32) {
>         tcg_gen_ext32u_tl(addr, addr);
>     }
>     if (ctx->pm_base_enabled) {
>         tcg_gen_or_tl(addr, addr, pm_base);
>     }
>
Can we just use

  tcg_gen_and_tl(addr, addr, pm_mask);
  tcg_gen_or_tl(addr, addr, pm_base);

Therefore we can remove all PM flags in TB_FLAGS.

Thanks,
Zhiwei

> with one more bit in TB_FLAGS, e.g.
>
>     if (env->cur_pm_mask < (xl == MVL_RV32 ? UINT32_MAX : UINT64_MAX)) {
>         flags = FIELD_DP32(flags, TB_FLAGS, PM_MASK_ENABLED, 1);
>     }
>     if (env->cur_pm_base != 0) {
>         flags = FIELD_DP32(flags, TB_FLAGS, PM_BASE_ENABLED, 1);
>     }
>
>
> r~
diff mbox series

Patch

diff --git a/target/riscv/insn_trans/trans_rvd.c.inc b/target/riscv/insn_trans/trans_rvd.c.inc
index 64fb0046f7..29066a8ef3 100644
--- a/target/riscv/insn_trans/trans_rvd.c.inc
+++ b/target/riscv/insn_trans/trans_rvd.c.inc
@@ -20,19 +20,10 @@ 
 
 static bool trans_fld(DisasContext *ctx, arg_fld *a)
 {
-    TCGv addr;
-
+    TCGv addr = get_address(ctx, a->rs1, a->imm);
     REQUIRE_FPU;
     REQUIRE_EXT(ctx, RVD);
 
-    addr = get_gpr(ctx, a->rs1, EXT_NONE);
-    if (a->imm) {
-        TCGv temp = temp_new(ctx);
-        tcg_gen_addi_tl(temp, addr, a->imm);
-        addr = temp;
-    }
-    addr = gen_pm_adjust_address(ctx, addr);
-
     tcg_gen_qemu_ld_i64(cpu_fpr[a->rd], addr, ctx->mem_idx, MO_TEQ);
 
     mark_fs_dirty(ctx);
@@ -41,21 +32,11 @@  static bool trans_fld(DisasContext *ctx, arg_fld *a)
 
 static bool trans_fsd(DisasContext *ctx, arg_fsd *a)
 {
-    TCGv addr;
-
+    TCGv addr = get_address(ctx, a->rs1, a->imm);
     REQUIRE_FPU;
     REQUIRE_EXT(ctx, RVD);
 
-    addr = get_gpr(ctx, a->rs1, EXT_NONE);
-    if (a->imm) {
-        TCGv temp = temp_new(ctx);
-        tcg_gen_addi_tl(temp, addr, a->imm);
-        addr = temp;
-    }
-    addr = gen_pm_adjust_address(ctx, addr);
-
     tcg_gen_qemu_st_i64(cpu_fpr[a->rs2], addr, ctx->mem_idx, MO_TEQ);
-
     return true;
 }
 
diff --git a/target/riscv/insn_trans/trans_rvf.c.inc b/target/riscv/insn_trans/trans_rvf.c.inc
index b5459249c4..a33897db7d 100644
--- a/target/riscv/insn_trans/trans_rvf.c.inc
+++ b/target/riscv/insn_trans/trans_rvf.c.inc
@@ -26,19 +26,10 @@ 
 static bool trans_flw(DisasContext *ctx, arg_flw *a)
 {
     TCGv_i64 dest;
-    TCGv addr;
-
+    TCGv addr = get_address(ctx, a->rs1, a->imm);
     REQUIRE_FPU;
     REQUIRE_EXT(ctx, RVF);
 
-    addr = get_gpr(ctx, a->rs1, EXT_NONE);
-    if (a->imm) {
-        TCGv temp = temp_new(ctx);
-        tcg_gen_addi_tl(temp, addr, a->imm);
-        addr = temp;
-    }
-    addr = gen_pm_adjust_address(ctx, addr);
-
     dest = cpu_fpr[a->rd];
     tcg_gen_qemu_ld_i64(dest, addr, ctx->mem_idx, MO_TEUL);
     gen_nanbox_s(dest, dest);
@@ -49,21 +40,11 @@  static bool trans_flw(DisasContext *ctx, arg_flw *a)
 
 static bool trans_fsw(DisasContext *ctx, arg_fsw *a)
 {
-    TCGv addr;
-
+    TCGv addr = get_address(ctx, a->rs1, a->imm);
     REQUIRE_FPU;
     REQUIRE_EXT(ctx, RVF);
 
-    addr = get_gpr(ctx, a->rs1, EXT_NONE);
-    if (a->imm) {
-        TCGv temp = tcg_temp_new();
-        tcg_gen_addi_tl(temp, addr, a->imm);
-        addr = temp;
-    }
-    addr = gen_pm_adjust_address(ctx, addr);
-
     tcg_gen_qemu_st_i64(cpu_fpr[a->rs2], addr, ctx->mem_idx, MO_TEUL);
-
     return true;
 }
 
diff --git a/target/riscv/insn_trans/trans_rvi.c.inc b/target/riscv/insn_trans/trans_rvi.c.inc
index e51dbc41c5..7a0b037594 100644
--- a/target/riscv/insn_trans/trans_rvi.c.inc
+++ b/target/riscv/insn_trans/trans_rvi.c.inc
@@ -137,14 +137,7 @@  static bool trans_bgeu(DisasContext *ctx, arg_bgeu *a)
 static bool gen_load(DisasContext *ctx, arg_lb *a, MemOp memop)
 {
     TCGv dest = dest_gpr(ctx, a->rd);
-    TCGv addr = get_gpr(ctx, a->rs1, EXT_NONE);
-
-    if (a->imm) {
-        TCGv temp = temp_new(ctx);
-        tcg_gen_addi_tl(temp, addr, a->imm);
-        addr = temp;
-    }
-    addr = gen_pm_adjust_address(ctx, addr);
+    TCGv addr = get_address(ctx, a->rs1, a->imm);
 
     tcg_gen_qemu_ld_tl(dest, addr, ctx->mem_idx, memop);
     gen_set_gpr(ctx, a->rd, dest);
@@ -178,16 +171,9 @@  static bool trans_lhu(DisasContext *ctx, arg_lhu *a)
 
 static bool gen_store(DisasContext *ctx, arg_sb *a, MemOp memop)
 {
-    TCGv addr = get_gpr(ctx, a->rs1, EXT_NONE);
+    TCGv addr = get_address(ctx, a->rs1, a->imm);
     TCGv data = get_gpr(ctx, a->rs2, EXT_NONE);
 
-    if (a->imm) {
-        TCGv temp = temp_new(ctx);
-        tcg_gen_addi_tl(temp, addr, a->imm);
-        addr = temp;
-    }
-    addr = gen_pm_adjust_address(ctx, addr);
-
     tcg_gen_qemu_st_tl(data, addr, ctx->mem_idx, memop);
     return true;
 }
diff --git a/target/riscv/translate.c b/target/riscv/translate.c
index a6a73ced9e..f52f6ef246 100644
--- a/target/riscv/translate.c
+++ b/target/riscv/translate.c
@@ -303,6 +303,19 @@  static TCGv gen_pm_adjust_address(DisasContext *s, TCGv src)
     }
 }
 
+static TCGv get_address(DisasContext *ctx, int rs1, int imm)
+{
+    TCGv addr = temp_new(ctx);
+    TCGv src1 = get_gpr(ctx, rs1, EXT_NONE);
+
+    tcg_gen_addi_tl(addr, src1, imm);
+    addr = gen_pm_adjust_address(ctx, addr);
+    if (get_xl(ctx) == MXL_RV32) {
+        tcg_gen_ext32u_tl(addr, addr);
+    }
+    return addr;
+}
+
 #ifndef CONFIG_USER_ONLY
 /* The states of mstatus_fs are:
  * 0 = disabled, 1 = initial, 2 = clean, 3 = dirty