On Fri, Apr 7, 2023 at 6:23 AM Daniel Henrique Barboza <dbarb...@ventanamicro.com> wrote: > > Hi, > > This patch is going to break the sifive_u boot if I rebase > > "[PATCH v6 0/9] target/riscv: rework CPU extensions validation" > > on top of it, as it is the case today with the current riscv-to-apply.next. > > The reason is that the priv spec version for Zca is marked as 1_12_0, and > the priv spec version for both sifive CPUs is 1_10_0, and both are enabling > RVC. > > This patch from that series above: > > "[PATCH v6 5/9] target/riscv/cpu.c: add priv_spec validate/disable_exts > helpers" > > Makes the disabling of the extension based on priv version to happen *after* > we > do all the validations, instead of before as we're doing today. Zca (and Zcd) > will > be manually enabled just to be disabled shortly after by the priv spec code. > And > this will happen: > > qemu-system-riscv64: warning: disabling zca extension for hart > 0x0000000000000000 because privilege spec version does not match > qemu-system-riscv64: warning: disabling zca extension for hart > 0x0000000000000001 because privilege spec version does not match > qemu-system-riscv64: warning: disabling zcd extension for hart > 0x0000000000000001 because privilege spec version does not match > (--- hangs ---) > > This means that the assumption made in this patch - that Zca implies RVC - is > no > longer valid, and all these translations won't work.
Thanks for catching and reporting this > > > Some possible solutions: > > - Do not use Zca as a synonym for RVC, i.e. drop this patch. We would need to > convert > all Zca checks to RVC checks in all translation code. This to me seems like the best solution > > - Do not apply patch 5/9 from that series that moves the disable_ext code to > the end > of validation. Also a possibility, but we would be sweeping the problem under > the rug. > Zca still can't be used as a RVC replacement due to priv spec version > constraints, but > we just won't disable Zca because we'll keep validating exts too early (which > is the > problem that the patch addresses). > > - change the priv spec of the sifive CPUs - and everyone that uses RVC - to > 1_12_0. Not > sure if this makes sense. > > - do not disable any extensions due to privilege spec version mismatch. This > would make > all the priv_version related artifacts to be more "educational" than to be an > actual > configuration we want to enforce. Not sure if that would do any good in the > end, but > it's also a possibility. This also seems like something we can do. Printing a warning but continuing on seems reasonable to me. That allows users to enable/disable features even if the versions don't match. I don't think this is the solution for this problem though Alistair > > > I'll hold the rebase of that series until we sort this out. Thanks, > > > Daniel > > > > On 3/7/23 05:13, Weiwei Li wrote: > > Modify the check for C extension to Zca (C implies Zca). > > > > Signed-off-by: Weiwei Li <liwei...@iscas.ac.cn> > > Signed-off-by: Junqiang Wang <wangjunqi...@iscas.ac.cn> > > Reviewed-by: Richard Henderson <richard.hender...@linaro.org> > > Reviewed-by: Alistair Francis <alistair.fran...@wdc.com> > > Reviewed-by: Wilfred Mallawa <wilfred.mall...@wdc.com> > > --- > > target/riscv/insn_trans/trans_rvi.c.inc | 4 ++-- > > target/riscv/translate.c | 8 ++++++-- > > 2 files changed, 8 insertions(+), 4 deletions(-) > > > > diff --git a/target/riscv/insn_trans/trans_rvi.c.inc > > b/target/riscv/insn_trans/trans_rvi.c.inc > > index 4ad54e8a49..c70c495fc5 100644 > > --- a/target/riscv/insn_trans/trans_rvi.c.inc > > +++ b/target/riscv/insn_trans/trans_rvi.c.inc > > @@ -56,7 +56,7 @@ static bool trans_jalr(DisasContext *ctx, arg_jalr *a) > > tcg_gen_andi_tl(cpu_pc, cpu_pc, (target_ulong)-2); > > > > gen_set_pc(ctx, cpu_pc); > > - if (!has_ext(ctx, RVC)) { > > + if (!ctx->cfg_ptr->ext_zca) { > > TCGv t0 = tcg_temp_new(); > > > > misaligned = gen_new_label(); > > @@ -169,7 +169,7 @@ static bool gen_branch(DisasContext *ctx, arg_b *a, > > TCGCond cond) > > > > gen_set_label(l); /* branch taken */ > > > > - if (!has_ext(ctx, RVC) && ((ctx->base.pc_next + a->imm) & 0x3)) { > > + if (!ctx->cfg_ptr->ext_zca && ((ctx->base.pc_next + a->imm) & 0x3)) { > > /* misaligned */ > > gen_exception_inst_addr_mis(ctx); > > } else { > > diff --git a/target/riscv/translate.c b/target/riscv/translate.c > > index 0ee8ee147d..d1fdd0c2d7 100644 > > --- a/target/riscv/translate.c > > +++ b/target/riscv/translate.c > > @@ -549,7 +549,7 @@ static void gen_jal(DisasContext *ctx, int rd, > > target_ulong imm) > > > > /* check misaligned: */ > > next_pc = ctx->base.pc_next + imm; > > - if (!has_ext(ctx, RVC)) { > > + if (!ctx->cfg_ptr->ext_zca) { > > if ((next_pc & 0x3) != 0) { > > gen_exception_inst_addr_mis(ctx); > > return; > > @@ -1122,7 +1122,11 @@ static void decode_opc(CPURISCVState *env, > > DisasContext *ctx, uint16_t opcode) > > if (insn_len(opcode) == 2) { > > ctx->opcode = opcode; > > ctx->pc_succ_insn = ctx->base.pc_next + 2; > > - if (has_ext(ctx, RVC) && decode_insn16(ctx, opcode)) { > > + /* > > + * The Zca extension is added as way to refer to instructions in > > the C > > + * extension that do not include the floating-point loads and > > stores > > + */ > > + if (ctx->cfg_ptr->ext_zca && decode_insn16(ctx, opcode)) { > > return; > > } > > } else { >