Re: [PATCH v4 09/11] target/loongarch: Truncate high 32 bits of address in VA32 mode

[Richard Henderson]

On 8/7/23 18:54, Jiajie Chen wrote:
When running in VA32 mode(LA32 or VA32L[1-3] matching PLV), virtual
address is truncated to 32 bits before address mapping.
Signed-off-by: Jiajie Chen <c...@jia.je>
---
  target/loongarch/cpu.h                          |  6 +++++-
  target/loongarch/insn_trans/trans_atomic.c.inc  |  1 +
  target/loongarch/insn_trans/trans_fmemory.c.inc |  8 ++++++++
  target/loongarch/insn_trans/trans_lsx.c.inc     |  6 ++++++
  target/loongarch/insn_trans/trans_memory.c.inc  | 10 ++++++++++
  target/loongarch/translate.c                    | 10 ++++++++++
  6 files changed, 40 insertions(+), 1 deletion(-)

diff --git a/target/loongarch/cpu.h b/target/loongarch/cpu.h
index 69589f0aef..9ad5fcc494 100644
--- a/target/loongarch/cpu.h
+++ b/target/loongarch/cpu.h
@@ -457,7 +457,11 @@ static inline void cpu_get_tb_cpu_state(CPULoongArchState 
*env, vaddr *pc,
          va32 = 1;
      }
  
-    *pc = env->pc;
+    if (va32) {
+        *pc = (uint32_t)env->pc;
+    } else {
+        *pc = env->pc;
+    }

This is not wrong, but it might be better to zero-extend when assigning to env->pc.  There 
are other consumers of env->pc, and we are not updating all of them.

--- a/target/loongarch/insn_trans/trans_memory.c.inc
+++ b/target/loongarch/insn_trans/trans_memory.c.inc
@@ -13,6 +13,7 @@ static bool gen_load(DisasContext *ctx, arg_rr_i *a, MemOp 
mop)
          tcg_gen_addi_tl(temp, addr, a->imm);
          addr = temp;
      }
+    addr = va32_address(ctx, addr);

I did say that you should use a common helper and a single temp.
This is using two temps: one here and one in va32_address.

I suggest:

static TCGv make_address_x(DisasContext *ctx, TCGv base, TCGv addend)
{
    TCGv temp = NULL;

    if (addend || ctx->va32) {
        temp = tcg_temp_new();
    }
    if (addend) {
        tcg_gen_add_tl(temp, base, addend);
        base = temp;
    }
    if (ctx->va32) {
        tcg_gen_ext32u_tl(temp, base);
        base = temp;
    }
    return base;
}

static TCGv make_address_i(DisasContext *ctx, TCGv base, target_long ofs)
{
    TCGv addend = ofs ? tcg_constant_tl(ofs) : NULL;
    return make_address_x(ctx, base, addend);
}


So that gen_load uses

    addr = make_address_i(ctx, addr, a->imm);

and gen_loadx uses

    addr = make_address_x(ctx, src1, src2);

and gen_am uses

    addr = make_address_i(ctx, addr, 0);

and so on for all of the others.


r~