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

[Richard Henderson <richard.henderson@linaro.org>]

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~