| Message ID | 20230307081403.61950-3-liweiwei@iscas.ac.cn (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | support subsets of code size reduction extension | expand |
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. 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. - 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. 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 <liweiwei@iscas.ac.cn> > Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn> > Reviewed-by: Richard Henderson <richard.henderson@linaro.org> > Reviewed-by: Alistair Francis <alistair.francis@wdc.com> > Reviewed-by: Wilfred Mallawa <wilfred.mallawa@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 {
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 > 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: 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. > > 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. > As specified in Zc* spec, Zca is the subset of RVC. C & F include Zcf in RV32. C & D include Zcd. > > 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. 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 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. 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. The question is whether we need constrain the configuration for general cpu type. Regards, Weiwei Li > 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 <liweiwei@iscas.ac.cn> >> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn> >> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> >> Reviewed-by: Alistair Francis <alistair.francis@wdc.com> >> Reviewed-by: Wilfred Mallawa <wilfred.mallawa@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 {
On 2023/4/7 09: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 >> 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: > > 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. > >> >> 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. >> > As specified in Zc* spec, Zca is the subset of RVC. C & F include > Zcf in RV32. C & D include Zcd. >> >> 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. > > 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 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. > > 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. > > The question is whether we need constrain the configuration for > general cpu type. Subset extension for another extension is not a single case in RISC-V. such as zaamo is subset of A. Zfhmin is subset of Zfh. Maybe some of them don't have this problem. However, I think it's better to take the related work away from the developer. I think we can combine the two method if we want to constrain the configuration for general cpu type: - remain disable extensions due to privilege spec version mismatch before validation to disable the extensions manually set by users - mask the implicitly enabled extensions in isa_string to make them invisible to users (I have sent a new patch to do this, you can pick it into your patchset if this way is acceptable). Regards, Weiwei Li > > Regards, > > Weiwei Li > >> 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 <liweiwei@iscas.ac.cn> >>> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn> >>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> >>> Reviewed-by: Alistair Francis <alistair.francis@wdc.com> >>> Reviewed-by: Wilfred Mallawa <wilfred.mallawa@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 {
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 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: > > 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. > >> >> 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. >> > As specified in Zc* spec, Zca is the subset of RVC. C & F include Zcf in RV32. C & D include Zcd. >> >> 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. > > 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 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. > > 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 as well. > > 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 only named CPUs. Thanks, Daniel > > Regards, > > Weiwei Li > >> 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 <liweiwei@iscas.ac.cn> >>> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn> >>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> >>> Reviewed-by: Alistair Francis <alistair.francis@wdc.com> >>> Reviewed-by: Wilfred Mallawa <wilfred.mallawa@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 { >
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 >>> 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: >> >> 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. >> >>> >>> 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. >>> >> As specified in Zc* spec, Zca is the subset of RVC. C & F include >> Zcf in RV32. C & D include Zcd. >>> >>> 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. >> >> 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 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. >> >> 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 as > well. 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. > > >> >> 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 only > named CPUs. > > Yeah, the default priv version is PRIV_VERSION_LATEST, However User can specify the priv_version they needs. Regards, Weiwei Li > Thanks, > > Daniel > >> >> Regards, >> >> Weiwei Li >> >>> 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 <liweiwei@iscas.ac.cn> >>>> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn> >>>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> >>>> Reviewed-by: Alistair Francis <alistair.francis@wdc.com> >>>> Reviewed-by: Wilfred Mallawa <wilfred.mallawa@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 { >>
On 4/7/23 00:34, liweiwei wrote: > > On 2023/4/7 09: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 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: >> >> 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. >> >>> >>> 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. >>> >> As specified in Zc* spec, Zca is the subset of RVC. C & F include Zcf in RV32. C & D include Zcd. >>> >>> 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. >> >> 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 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. >> >> 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. >> >> The question is whether we need constrain the configuration for general cpu type. > > Subset extension for another extension is not a single case in RISC-V. such as zaamo is subset of A. Zfhmin is subset of Zfh. > > Maybe some of them don't have this problem. However, I think it's better to take the related work away from the developer. > > I think we can combine the two method if we want to constrain the configuration for general cpu type: > > - remain disable extensions due to privilege spec version mismatch before validation to disable the extensions manually set by users > > - mask the implicitly enabled extensions in isa_string to make them invisible to users (I have sent a new patch to do this, you can pick it into > > your patchset if this way is acceptable). I tested that patch with my series. If we keep the disable extension code to be executed before the validation, filtering the extensions that were user enabled only, it fixes the problem I reported here. It's worth noticing though that we would be making the intended, conscious decision of hiding extensions from the isa_string that are actually enabled in the hart. And CPUs such as SIFIVE_CPU will start working with Z extensions that are beyond their declared priv spec. This wouldn't be a problem if we could guarantee that userland would always read 'isa_string' before using an extension, but in reality we can't guarantee that. Firing an instruction for a given extension and capturing SIGILL to see if the hart supports it or not isn't forbidden by the ISA. All this said, I don't mind the proposed solution, as long as we're on the same boat w.r.t. the design changes and potential userspace impact this might have. Thanks, Daniel > > Regards, > > Weiwei Li > >> >> Regards, >> >> Weiwei Li >> >>> 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 <liweiwei@iscas.ac.cn> >>>> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn> >>>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> >>>> Reviewed-by: Alistair Francis <alistair.francis@wdc.com> >>>> Reviewed-by: Wilfred Mallawa <wilfred.mallawa@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 { >
On 2023/4/8 03:25, Daniel Henrique Barboza wrote: > > > On 4/7/23 00:34, liweiwei wrote: >> >> On 2023/4/7 09: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 >>>> 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: >>> >>> 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. >>> >>>> >>>> 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. >>>> >>> As specified in Zc* spec, Zca is the subset of RVC. C & F include >>> Zcf in RV32. C & D include Zcd. >>>> >>>> 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. >>> >>> 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 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. >>> >>> 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. >>> >>> The question is whether we need constrain the configuration for >>> general cpu type. >> >> Subset extension for another extension is not a single case in >> RISC-V. such as zaamo is subset of A. Zfhmin is subset of Zfh. >> >> Maybe some of them don't have this problem. However, I think it's >> better to take the related work away from the developer. >> >> I think we can combine the two method if we want to constrain the >> configuration for general cpu type: >> >> - remain disable extensions due to privilege spec version mismatch >> before validation to disable the extensions manually set by users >> >> - mask the implicitly enabled extensions in isa_string to make them >> invisible to users (I have sent a new patch to do this, you can pick >> it into >> >> your patchset if this way is acceptable). > > I tested that patch with my series. If we keep the disable extension > code to be executed > before the validation, filtering the extensions that were user enabled > only, it fixes > the problem I reported here. > > It's worth noticing though that we would be making the intended, > conscious decision of > hiding extensions from the isa_string that are actually enabled in the > hart. And CPUs > such as SIFIVE_CPU will start working with Z extensions that are > beyond their declared > priv spec. This wouldn't be a problem if we could guarantee that > userland would always > read 'isa_string' before using an extension, but in reality we can't > guarantee that. > Firing an instruction for a given extension and capturing SIGILL to > see if the hart supports > it or not isn't forbidden by the ISA. The implicitly enabled extensions are mostly subset of its super extension, except zfinx (I think it's visible to user, and we can change it to check zdinx/zhinx{min} requires it). So enabling them when their super subset are enabled will introduce no additional function to the cpu. So it's just another internal implementation(C is replaced by Zca, Zcf, Zcd) for SIFIVE_CPU without real function change (neither increase nor decrease). Implicitly enabled extensions may introduce new problem for write_misa: C/V function cannot be disabled by it (I have described before). However, this is another problem unrelated to priv version. Regards, Weiwei Li > > > All this said, I don't mind the proposed solution, as long as we're on > the same boat > w.r.t. the design changes and potential userspace impact this might have. > > > Thanks, > > > Daniel > > >> >> Regards, >> >> Weiwei Li >> >>> >>> Regards, >>> >>> Weiwei Li >>> >>>> 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 <liweiwei@iscas.ac.cn> >>>>> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn> >>>>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> >>>>> Reviewed-by: Alistair Francis <alistair.francis@wdc.com> >>>>> Reviewed-by: Wilfred Mallawa <wilfred.mallawa@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 { >>
On Fri, Apr 7, 2023 at 6:23 AM Daniel Henrique Barboza <dbarboza@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 <liweiwei@iscas.ac.cn> > > Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn> > > Reviewed-by: Richard Henderson <richard.henderson@linaro.org> > > Reviewed-by: Alistair Francis <alistair.francis@wdc.com> > > Reviewed-by: Wilfred Mallawa <wilfred.mallawa@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 { >
On 2023/4/12 10:12, Alistair Francis wrote: > On Fri, Apr 7, 2023 at 6:23 AM Daniel Henrique Barboza > <dbarboza@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 I had also implemented a patch for this solution. I'll sent it later. However, this will introduce additional check. i.e. check both Zca and C or , both Zcf/d and CF/CD. I think this is not very necessary. Implcitly-enabled extensions is often the subsets of existed extension. So enabling them will introduce no additional function to the cpus. We can just make them invisible to user(mask them in the isa string) instead of disabling them to be compatible with lower priv version. Regards, Weiwei Li > >> - 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 <liweiwei@iscas.ac.cn> >>> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn> >>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> >>> Reviewed-by: Alistair Francis <alistair.francis@wdc.com> >>> Reviewed-by: Wilfred Mallawa <wilfred.mallawa@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 {
On Wed, Apr 12, 2023 at 12:55 PM Weiwei Li <liweiwei@iscas.ac.cn> wrote: > > > On 2023/4/12 10:12, Alistair Francis wrote: > > On Fri, Apr 7, 2023 at 6:23 AM Daniel Henrique Barboza > > <dbarboza@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 > > I had also implemented a patch for this solution. I'll sent it later. Thanks! > > However, this will introduce additional check. i.e. check both Zca and C > or , both Zcf/d and CF/CD. Is there a large performance penalty for that? > > I think this is not very necessary. Implcitly-enabled extensions is > often the subsets of existed extension. Yeah, I see what you are saying. It just becomes difficult to manage though. It all worked fine until there are conflicts between the priv specs. > > So enabling them will introduce no additional function to the cpus. > > We can just make them invisible to user(mask them in the isa string) > instead of disabling them to be compatible with lower priv version. I'm open to other options, but masking them like this seems like more work and also seems confusing. The extension will end up enabled in QEMU and potentially visible to external tools, but then just not exposed to the guest. It seems prone to future bugs. Alistair > > Regards, > > Weiwei Li > > > > > >> - 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 <liweiwei@iscas.ac.cn> > >>> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn> > >>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> > >>> Reviewed-by: Alistair Francis <alistair.francis@wdc.com> > >>> Reviewed-by: Wilfred Mallawa <wilfred.mallawa@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 { >
On 2023/4/12 18:55, Alistair Francis wrote: > On Wed, Apr 12, 2023 at 12:55 PM Weiwei Li <liweiwei@iscas.ac.cn> wrote: >> >> On 2023/4/12 10:12, Alistair Francis wrote: >>> On Fri, Apr 7, 2023 at 6:23 AM Daniel Henrique Barboza >>> <dbarboza@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 >> I had also implemented a patch for this solution. I'll sent it later. > Thanks! > >> However, this will introduce additional check. i.e. check both Zca and C >> or , both Zcf/d and CF/CD. > Is there a large performance penalty for that? May not very large. Just one or two additional check in instruction translation. You can see the patch at: https://lore.kernel.org/qemu-devel/20230412030648.80470-1-liweiwei@iscas.ac.cn/ > >> I think this is not very necessary. Implcitly-enabled extensions is >> often the subsets of existed extension. > Yeah, I see what you are saying. It just becomes difficult to manage > though. It all worked fine until there are conflicts between the priv > specs. > >> So enabling them will introduce no additional function to the cpus. >> >> We can just make them invisible to user(mask them in the isa string) >> instead of disabling them to be compatible with lower priv version. > I'm open to other options, but masking them like this seems like more > work and also seems confusing. The extension will end up enabled in > QEMU and potentially visible to external tools, but then just not > exposed to the guest. It seems prone to future bugs. Yeah. they may be visible to external tools if they read cfg directly. Another way is to introduce another parameter instead of cfg.ext_zca to indicate whether Zca/Zcf/Zcd instructions are enabled. This is be done by patchset: https://lore.kernel.org/qemu-devel/20230410033526.31708-1-liweiwei@iscas.ac.cn/ All of the three ways are acceptable to me. Regards, Weiwei Li > Alistair > >> Regards, >> >> Weiwei Li >> >> >>>> - 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 <liweiwei@iscas.ac.cn> >>>>> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn> >>>>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> >>>>> Reviewed-by: Alistair Francis <alistair.francis@wdc.com> >>>>> Reviewed-by: Wilfred Mallawa <wilfred.mallawa@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 {
On 4/12/23 08:35, Weiwei Li wrote: > > On 2023/4/12 18:55, Alistair Francis wrote: >> On Wed, Apr 12, 2023 at 12:55 PM Weiwei Li <liweiwei@iscas.ac.cn> wrote: >>> >>> On 2023/4/12 10:12, Alistair Francis wrote: >>>> On Fri, Apr 7, 2023 at 6:23 AM Daniel Henrique Barboza >>>> <dbarboza@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 >>> I had also implemented a patch for this solution. I'll sent it later. >> Thanks! >> >>> However, this will introduce additional check. i.e. check both Zca and C >>> or , both Zcf/d and CF/CD. >> Is there a large performance penalty for that? > > May not very large. Just one or two additional check in instruction translation. You can see the patch at: > > https://lore.kernel.org/qemu-devel/20230412030648.80470-1-liweiwei@iscas.ac.cn/ > >> >>> I think this is not very necessary. Implcitly-enabled extensions is >>> often the subsets of existed extension. >> Yeah, I see what you are saying. It just becomes difficult to manage >> though. It all worked fine until there are conflicts between the priv >> specs. >> >>> So enabling them will introduce no additional function to the cpus. >>> >>> We can just make them invisible to user(mask them in the isa string) >>> instead of disabling them to be compatible with lower priv version. >> I'm open to other options, but masking them like this seems like more >> work and also seems confusing. The extension will end up enabled in >> QEMU and potentially visible to external tools, but then just not >> exposed to the guest. It seems prone to future bugs. > > Yeah. they may be visible to external tools if they read cfg directly. > > Another way is to introduce another parameter instead of cfg.ext_zca to indicate whether Zca/Zcf/Zcd > > instructions are enabled. This is be done by patchset: > > https://lore.kernel.org/qemu-devel/20230410033526.31708-1-liweiwei@iscas.ac.cn/ > > All of the three ways are acceptable to me. Earlier today I grabbed two Weiwei Li patches (isa_string changes and Zca/Zcf/Zcd changes) in the "rework CPU extensions validation" series, but after reading these last messages I guess I jumped the gun. Alistair, I'm considering drop the patch that disables extensions via priv_spec later during realize() (the one I mentioned in my first message) from that series until we figure the best way of dealing with priv spec and Z-extensions being used as MISA bits and so on. We can merge those cleanups and write_misa() changes that are already reviewed in the meantime. Are you ok with that? Thanks, Daniel > > Regards, > > Weiwei Li > > >> Alistair >> >>> Regards, >>> >>> Weiwei Li >>> >>> >>>>> - 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 <liweiwei@iscas.ac.cn> >>>>>> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn> >>>>>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> >>>>>> Reviewed-by: Alistair Francis <alistair.francis@wdc.com> >>>>>> Reviewed-by: Wilfred Mallawa <wilfred.mallawa@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 { >
On Thu, Apr 13, 2023 at 3:24 AM Daniel Henrique Barboza <dbarboza@ventanamicro.com> wrote: > > > > On 4/12/23 08:35, Weiwei Li wrote: > > > > On 2023/4/12 18:55, Alistair Francis wrote: > >> On Wed, Apr 12, 2023 at 12:55 PM Weiwei Li <liweiwei@iscas.ac.cn> wrote: > >>> > >>> On 2023/4/12 10:12, Alistair Francis wrote: > >>>> On Fri, Apr 7, 2023 at 6:23 AM Daniel Henrique Barboza > >>>> <dbarboza@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 > >>> I had also implemented a patch for this solution. I'll sent it later. > >> Thanks! > >> > >>> However, this will introduce additional check. i.e. check both Zca and C > >>> or , both Zcf/d and CF/CD. > >> Is there a large performance penalty for that? > > > > May not very large. Just one or two additional check in instruction translation. You can see the patch at: > > > > https://lore.kernel.org/qemu-devel/20230412030648.80470-1-liweiwei@iscas.ac.cn/ This is my prefered way. I think it's pretty simple and explicitly follows the spec. > > > >> > >>> I think this is not very necessary. Implcitly-enabled extensions is > >>> often the subsets of existed extension. > >> Yeah, I see what you are saying. It just becomes difficult to manage > >> though. It all worked fine until there are conflicts between the priv > >> specs. > >> > >>> So enabling them will introduce no additional function to the cpus. > >>> > >>> We can just make them invisible to user(mask them in the isa string) > >>> instead of disabling them to be compatible with lower priv version. > >> I'm open to other options, but masking them like this seems like more > >> work and also seems confusing. The extension will end up enabled in > >> QEMU and potentially visible to external tools, but then just not > >> exposed to the guest. It seems prone to future bugs. > > > > Yeah. they may be visible to external tools if they read cfg directly. > > > > Another way is to introduce another parameter instead of cfg.ext_zca to indicate whether Zca/Zcf/Zcd > > > > instructions are enabled. This is be done by patchset: > > > > https://lore.kernel.org/qemu-devel/20230410033526.31708-1-liweiwei@iscas.ac.cn/ I don't prefer this option, but if others feel strongly I'm not completely opposed to it. > > > > All of the three ways are acceptable to me. > > Earlier today I grabbed two Weiwei Li patches (isa_string changes and Zca/Zcf/Zcd > changes) in the "rework CPU extensions validation" series, but after reading these > last messages I guess I jumped the gun. My preference would be the "target/riscv: Update check for Zca/zcf/zcd" patch, which I think is what you picked. So that seems like the way to go > > Alistair, I'm considering drop the patch that disables extensions via priv_spec later > during realize() (the one I mentioned in my first message) from that series until we > figure the best way of dealing with priv spec and Z-extensions being used as MISA bits > and so on. We can merge those cleanups and write_misa() changes that are already reviewed > in the meantime. Are you ok with that? That's also fine Alistair > > > Thanks, > > > Daniel > > > > > > Regards, > > > > Weiwei Li > > > > > >> Alistair > >> > >>> Regards, > >>> > >>> Weiwei Li > >>> > >>> > >>>>> - 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 <liweiwei@iscas.ac.cn> > >>>>>> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn> > >>>>>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> > >>>>>> Reviewed-by: Alistair Francis <alistair.francis@wdc.com> > >>>>>> Reviewed-by: Wilfred Mallawa <wilfred.mallawa@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 { > >
On 4/16/23 23:35, Alistair Francis wrote: > On Thu, Apr 13, 2023 at 3:24 AM Daniel Henrique Barboza > <dbarboza@ventanamicro.com> wrote: >> >> >> >> On 4/12/23 08:35, Weiwei Li wrote: >>> >>> On 2023/4/12 18:55, Alistair Francis wrote: >>>> On Wed, Apr 12, 2023 at 12:55 PM Weiwei Li <liweiwei@iscas.ac.cn> wrote: >>>>> >>>>> On 2023/4/12 10:12, Alistair Francis wrote: >>>>>> On Fri, Apr 7, 2023 at 6:23 AM Daniel Henrique Barboza >>>>>> <dbarboza@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 >>>>> I had also implemented a patch for this solution. I'll sent it later. >>>> Thanks! >>>> >>>>> However, this will introduce additional check. i.e. check both Zca and C >>>>> or , both Zcf/d and CF/CD. >>>> Is there a large performance penalty for that? >>> >>> May not very large. Just one or two additional check in instruction translation. You can see the patch at: >>> >>> https://lore.kernel.org/qemu-devel/20230412030648.80470-1-liweiwei@iscas.ac.cn/ > > This is my prefered way. I think it's pretty simple and explicitly > follows the spec. > >>> >>>> >>>>> I think this is not very necessary. Implcitly-enabled extensions is >>>>> often the subsets of existed extension. >>>> Yeah, I see what you are saying. It just becomes difficult to manage >>>> though. It all worked fine until there are conflicts between the priv >>>> specs. >>>> >>>>> So enabling them will introduce no additional function to the cpus. >>>>> >>>>> We can just make them invisible to user(mask them in the isa string) >>>>> instead of disabling them to be compatible with lower priv version. >>>> I'm open to other options, but masking them like this seems like more >>>> work and also seems confusing. The extension will end up enabled in >>>> QEMU and potentially visible to external tools, but then just not >>>> exposed to the guest. It seems prone to future bugs. >>> >>> Yeah. they may be visible to external tools if they read cfg directly. >>> >>> Another way is to introduce another parameter instead of cfg.ext_zca to indicate whether Zca/Zcf/Zcd >>> >>> instructions are enabled. This is be done by patchset: >>> >>> https://lore.kernel.org/qemu-devel/20230410033526.31708-1-liweiwei@iscas.ac.cn/ > > I don't prefer this option, but if others feel strongly I'm not > completely opposed to it. > >>> >>> All of the three ways are acceptable to me. >> >> Earlier today I grabbed two Weiwei Li patches (isa_string changes and Zca/Zcf/Zcd >> changes) in the "rework CPU extensions validation" series, but after reading these >> last messages I guess I jumped the gun. > > My preference would be the "target/riscv: Update check for > Zca/zcf/zcd" patch, which I think is what you picked. So that seems > like the way to go Ok! I'll send it over with Weiwei's v2. Thanks, Daniel > >> >> Alistair, I'm considering drop the patch that disables extensions via priv_spec later >> during realize() (the one I mentioned in my first message) from that series until we >> figure the best way of dealing with priv spec and Z-extensions being used as MISA bits >> and so on. We can merge those cleanups and write_misa() changes that are already reviewed >> in the meantime. Are you ok with that? > > That's also fine > > Alistair > >> >> >> Thanks, >> >> >> Daniel >> >> >>> >>> Regards, >>> >>> Weiwei Li >>> >>> >>>> Alistair >>>> >>>>> Regards, >>>>> >>>>> Weiwei Li >>>>> >>>>> >>>>>>> - 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 <liweiwei@iscas.ac.cn> >>>>>>>> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn> >>>>>>>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> >>>>>>>> Reviewed-by: Alistair Francis <alistair.francis@wdc.com> >>>>>>>> Reviewed-by: Wilfred Mallawa <wilfred.mallawa@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 { >>>
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 {