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

[LIU Zhiwei]

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~