| Message ID | 20181020071451.27808-2-kbastian@mail.uni-paderborn.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/riscv: Convert to decodetree | expand |
On Sat, 20 Oct 2018 00:14:23 PDT (-0700), kbastian@mail.uni-paderborn.de wrote: > CPURISCVState is rarely used, so there is no need to pass it to every > translate function. This paves the way for decodetree which only passes > DisasContext to translate functions. > > Signed-off-by: Bastian Koppelmann <kbastian@mail.uni-paderborn.de> > --- > target/riscv/translate.c | 15 ++++++++------- > 1 file changed, 8 insertions(+), 7 deletions(-) > > diff --git a/target/riscv/translate.c b/target/riscv/translate.c > index 18d7b6d147..e81b9f097e 100644 > --- a/target/riscv/translate.c > +++ b/target/riscv/translate.c > @@ -52,6 +52,7 @@ typedef struct DisasContext { > to any system register, which includes CSR_FRM, so we do not have > to reset this known value. */ > int frm; > + CPURISCVState *env; > } DisasContext; > > /* convert riscv funct3 to qemu memop for load/store */ > @@ -1789,19 +1790,19 @@ static void decode_RV32_64G(CPURISCVState *env, DisasContext *ctx) > } > } > > -static void decode_opc(CPURISCVState *env, DisasContext *ctx) > +static void decode_opc(DisasContext *ctx) > { > /* check for compressed insn */ > if (extract32(ctx->opcode, 0, 2) != 3) { > - if (!riscv_has_ext(env, RVC)) { > + if (!riscv_has_ext(ctx->env, RVC)) { > gen_exception_illegal(ctx); > } else { > ctx->pc_succ_insn = ctx->base.pc_next + 2; > - decode_RV32_64C(env, ctx); > + decode_RV32_64C(ctx->env, ctx); > } > } else { > ctx->pc_succ_insn = ctx->base.pc_next + 4; > - decode_RV32_64G(env, ctx); > + decode_RV32_64G(ctx->env, ctx); > } > } > > @@ -1846,10 +1847,10 @@ static bool riscv_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cpu, > static void riscv_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu) > { > DisasContext *ctx = container_of(dcbase, DisasContext, base); > - CPURISCVState *env = cpu->env_ptr; > + ctx->env = cpu->env_ptr; > > - ctx->opcode = cpu_ldl_code(env, ctx->base.pc_next); > - decode_opc(env, ctx); > + ctx->opcode = cpu_ldl_code(ctx->env, ctx->base.pc_next); > + decode_opc(ctx); > ctx->base.pc_next = ctx->pc_succ_insn; > > if (ctx->base.is_jmp == DISAS_NEXT) { I think this is OK, but I'm not sure. All the other ports do this in a different way: rather than including the CPU state structure in DisasContext they explicitly call out the state that can change instruction decoding by duplicating it within DisasContext. Michael's patch queue handles this in the canonical way, but I think it's a bit cleaner to avoid duplicating the state. My worry is that, since changing the CPU state changes the legal instructions, we will paint ourselves into a corner when it comes to faithfully implementing a writeable MISA register.
On 25 October 2018 at 17:38, Palmer Dabbelt <palmer@sifive.com> wrote: > On Sat, 20 Oct 2018 00:14:23 PDT (-0700), kbastian@mail.uni-paderborn.de > wrote: >> >> CPURISCVState is rarely used, so there is no need to pass it to every >> translate function. This paves the way for decodetree which only passes >> DisasContext to translate functions. >> --- a/target/riscv/translate.c >> +++ b/target/riscv/translate.c >> @@ -52,6 +52,7 @@ typedef struct DisasContext { >> to any system register, which includes CSR_FRM, so we do not have >> to reset this known value. */ >> int frm; >> + CPURISCVState *env; >> } DisasContext; > I think this is OK, but I'm not sure. All the other ports do this in a > different way: rather than including the CPU state structure in DisasContext > they explicitly call out the state that can change instruction decoding by > duplicating it within DisasContext. Yes. The reason for doing this is that it makes it easier to avoid a particular class of bug. Any target CPU state that we "bake into" the generated TCG code needs to be one of: * constant for the life of the CPU (eg ID register values, or feature bits) * encoded into the TB flags (which are filled in by the target's cpu_get_tb_cpu_state()) If you generate code that depends on some other target CPU state by accident, then this will cause hard to track down bugs when we reuse the generated TB in an executed context where that target CPU state has changed from what it was when the TB was generated. By not allowing the code generation parts of translate.c to get hold of the CPUState pointer, we can make this kind of bug much easier to spot at compile time, because any new state that we want to use in codegen has to be copied into the DisasContext. It's then fairly easy to check that we only copy into the DisasContext state that is constant-for-the-CPU or which we've got from the TB flags. That said, at the moment the riscv frontend code does not use this pattern -- it passes the CPURISCVState pointer directly into (some of) the leaf functions, like gen_jal(). So putting the CPURISCVState into DisasContext is no worse than passing it around all these translate.c functions as a function parameter. I would prefer it if the riscv code was cleaned up to do the explicit passing of only the required bits of state in DisasContext (as for instance target/arm/ does). But I don't have a strong opinion on the ordering of doing that versus doing this conversion to decodetree. thanks -- PMM
On Thu, 25 Oct 2018 09:54:56 PDT (-0700), Peter Maydell wrote: > On 25 October 2018 at 17:38, Palmer Dabbelt <palmer@sifive.com> wrote: >> On Sat, 20 Oct 2018 00:14:23 PDT (-0700), kbastian@mail.uni-paderborn.de >> wrote: >>> >>> CPURISCVState is rarely used, so there is no need to pass it to every >>> translate function. This paves the way for decodetree which only passes >>> DisasContext to translate functions. > >>> --- a/target/riscv/translate.c >>> +++ b/target/riscv/translate.c >>> @@ -52,6 +52,7 @@ typedef struct DisasContext { >>> to any system register, which includes CSR_FRM, so we do not have >>> to reset this known value. */ >>> int frm; >>> + CPURISCVState *env; >>> } DisasContext; > > >> I think this is OK, but I'm not sure. All the other ports do this in a >> different way: rather than including the CPU state structure in DisasContext >> they explicitly call out the state that can change instruction decoding by >> duplicating it within DisasContext. > > Yes. The reason for doing this is that it makes it easier to avoid > a particular class of bug. Any target CPU state that we "bake into" > the generated TCG code needs to be one of: > * constant for the life of the CPU (eg ID register values, or > feature bits) > * encoded into the TB flags (which are filled in by > the target's cpu_get_tb_cpu_state()) > If you generate code that depends on some other target CPU state > by accident, then this will cause hard to track down bugs when > we reuse the generated TB in an executed context where that > target CPU state has changed from what it was when the TB was > generated. > > By not allowing the code generation parts of translate.c to get > hold of the CPUState pointer, we can make this kind of bug much > easier to spot at compile time, because any new state that we > want to use in codegen has to be copied into the DisasContext. > It's then fairly easy to check that we only copy into the > DisasContext state that is constant-for-the-CPU or which we've > got from the TB flags. > > That said, at the moment the riscv frontend code does not > use this pattern -- it passes the CPURISCVState pointer > directly into (some of) the leaf functions, like gen_jal(). > So putting the CPURISCVState into DisasContext is no worse > than passing it around all these translate.c functions as > a function parameter. > > I would prefer it if the riscv code was cleaned up to > do the explicit passing of only the required bits of state > in DisasContext (as for instance target/arm/ does). But I > don't have a strong opinion on the ordering of doing that > versus doing this conversion to decodetree. OK, that makes sense. Since our port is already broken WRT this behavior and the decode tree stuff will be nice to have, I'm happy taking this change now and fixing this later -- that way we won't have to couple two major changes into a single patch set. Reviewed-by: Palmer Dabbelt <palmer@sifive.com> Thanks!
diff --git a/target/riscv/translate.c b/target/riscv/translate.c index 18d7b6d147..e81b9f097e 100644 --- a/target/riscv/translate.c +++ b/target/riscv/translate.c @@ -52,6 +52,7 @@ typedef struct DisasContext { to any system register, which includes CSR_FRM, so we do not have to reset this known value. */ int frm; + CPURISCVState *env; } DisasContext; /* convert riscv funct3 to qemu memop for load/store */ @@ -1789,19 +1790,19 @@ static void decode_RV32_64G(CPURISCVState *env, DisasContext *ctx) } } -static void decode_opc(CPURISCVState *env, DisasContext *ctx) +static void decode_opc(DisasContext *ctx) { /* check for compressed insn */ if (extract32(ctx->opcode, 0, 2) != 3) { - if (!riscv_has_ext(env, RVC)) { + if (!riscv_has_ext(ctx->env, RVC)) { gen_exception_illegal(ctx); } else { ctx->pc_succ_insn = ctx->base.pc_next + 2; - decode_RV32_64C(env, ctx); + decode_RV32_64C(ctx->env, ctx); } } else { ctx->pc_succ_insn = ctx->base.pc_next + 4; - decode_RV32_64G(env, ctx); + decode_RV32_64G(ctx->env, ctx); } } @@ -1846,10 +1847,10 @@ static bool riscv_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cpu, static void riscv_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu) { DisasContext *ctx = container_of(dcbase, DisasContext, base); - CPURISCVState *env = cpu->env_ptr; + ctx->env = cpu->env_ptr; - ctx->opcode = cpu_ldl_code(env, ctx->base.pc_next); - decode_opc(env, ctx); + ctx->opcode = cpu_ldl_code(ctx->env, ctx->base.pc_next); + decode_opc(ctx); ctx->base.pc_next = ctx->pc_succ_insn; if (ctx->base.is_jmp == DISAS_NEXT) {
CPURISCVState is rarely used, so there is no need to pass it to every translate function. This paves the way for decodetree which only passes DisasContext to translate functions. Signed-off-by: Bastian Koppelmann <kbastian@mail.uni-paderborn.de> --- target/riscv/translate.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-)