On 2023/4/7 18:28, Daniel Henrique Barboza wrote:
On 4/6/23 22:14, liweiwei wrote:On 2023/4/7 04:22, Daniel Henrique Barboza 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 enablingRVC. 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. Andthis will happen:Yeah, I didn't take priv_version into consideration before.This is a new problem if we disable them at the end and was not triggered in my previous tests.Not only Zca and Zcd, Zcf also has the same problem.As specified in Zc* spec, Zca is the subset of RVC. C & F include Zcf in RV32. C & D include Zcd.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 nolonger valid, and all these translations won't work.Some possible solutions:- Do not use Zca as a synonym for RVC, i.e. drop this patch. We would need to convertall Zca checks to RVC checks in all translation code.We should check both Zca and RVC in this way.Similarly, we also should check both C&F and Zcf for Zcf instructions, C&D and Zcd for Zcd instructions.I can update this patchset or add a new patch for it if needed.- 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 theproblem that the patch addresses).- change the priv spec of the sifive CPUs - and everyone that uses RVC - to 1_12_0. Notsure 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, butit's also a possibility.I prefer this way. For vendor-specific cpu types, the implicitly implied extensions will have no effect on its function,and this can be invisible to user if we mask them in isa_string exposed to the kernel.Problem is that, at least for now, we can't say whether a Z extension was enabled by the user or by us. We'll end up masking user selection in the isa_string aswell.
No, for vendor-specific cpu, extension support is stable, and the Z* extension related property isn't registered for them.
So they cannot be enabled by user.
Yeah, the default priv version is PRIV_VERSION_LATEST, However User can specify the priv_version they needs.The question is whether we need constrain the configuration for general cpu type.General CPU types aren't affected at all by these changes because they'll always run with PRIV_VERSION_LATEST. This particular problem is something that affects onlynamed CPUs.
Regards, Weiwei Li
Thanks, DanielRegards, Weiwei LiI'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.incindex 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 {