On Thu, Apr 13, 2023 at 3:24 AM Daniel Henrique Barboza <dbarb...@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 <liwei...@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 > >>>> <dbarb...@ventanamicro.com> wrote: > >>>>> Hi, > >>>>> > >>>>> This patch is going to break the sifive_u boot if I rebase > >>>>> > >>>>> "[PATCH v6 0/9] target/riscv: rework CPU extensions validation" > >>>>> > >>>>> on top of it, as it is the case today with the current > >>>>> riscv-to-apply.next. > >>>>> > >>>>> The reason is that the priv spec version for Zca is marked as 1_12_0, > >>>>> and > >>>>> the priv spec version for both sifive CPUs is 1_10_0, and both are > >>>>> enabling > >>>>> RVC. > >>>>> > >>>>> This patch from that series above: > >>>>> > >>>>> "[PATCH v6 5/9] target/riscv/cpu.c: add priv_spec validate/disable_exts > >>>>> helpers" > >>>>> > >>>>> Makes the disabling of the extension based on priv version to happen > >>>>> *after* we > >>>>> do all the validations, instead of before as we're doing today. Zca > >>>>> (and Zcd) will > >>>>> be manually enabled just to be disabled shortly after by the priv spec > >>>>> code. And > >>>>> this will happen: > >>>>> > >>>>> qemu-system-riscv64: warning: disabling zca extension for hart > >>>>> 0x0000000000000000 because privilege spec version does not match > >>>>> qemu-system-riscv64: warning: disabling zca extension for hart > >>>>> 0x0000000000000001 because privilege spec version does not match > >>>>> qemu-system-riscv64: warning: disabling zcd extension for hart > >>>>> 0x0000000000000001 because privilege spec version does not match > >>>>> (--- hangs ---) > >>>>> > >>>>> This means that the assumption made in this patch - that Zca implies > >>>>> RVC - is no > >>>>> longer valid, and all these translations won't work. > >>>> Thanks for catching and reporting this > >>>> > >>>>> Some possible solutions: > >>>>> > >>>>> - Do not use Zca as a synonym for RVC, i.e. drop this patch. We would > >>>>> need to convert > >>>>> all Zca checks to RVC checks in all translation code. > >>>> This to me seems like the best solution > >>> 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-liwei...@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-liwei...@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 <liwei...@iscas.ac.cn> > >>>>>> Signed-off-by: Junqiang Wang <wangjunqi...@iscas.ac.cn> > >>>>>> Reviewed-by: Richard Henderson <richard.hender...@linaro.org> > >>>>>> Reviewed-by: Alistair Francis <alistair.fran...@wdc.com> > >>>>>> Reviewed-by: Wilfred Mallawa <wilfred.mall...@wdc.com> > >>>>>> --- > >>>>>> target/riscv/insn_trans/trans_rvi.c.inc | 4 ++-- > >>>>>> target/riscv/translate.c | 8 ++++++-- > >>>>>> 2 files changed, 8 insertions(+), 4 deletions(-) > >>>>>> > >>>>>> diff --git a/target/riscv/insn_trans/trans_rvi.c.inc > >>>>>> b/target/riscv/insn_trans/trans_rvi.c.inc > >>>>>> index 4ad54e8a49..c70c495fc5 100644 > >>>>>> --- a/target/riscv/insn_trans/trans_rvi.c.inc > >>>>>> +++ b/target/riscv/insn_trans/trans_rvi.c.inc > >>>>>> @@ -56,7 +56,7 @@ static bool trans_jalr(DisasContext *ctx, arg_jalr > >>>>>> *a) > >>>>>> tcg_gen_andi_tl(cpu_pc, cpu_pc, (target_ulong)-2); > >>>>>> > >>>>>> gen_set_pc(ctx, cpu_pc); > >>>>>> - if (!has_ext(ctx, RVC)) { > >>>>>> + if (!ctx->cfg_ptr->ext_zca) { > >>>>>> TCGv t0 = tcg_temp_new(); > >>>>>> > >>>>>> misaligned = gen_new_label(); > >>>>>> @@ -169,7 +169,7 @@ static bool gen_branch(DisasContext *ctx, arg_b > >>>>>> *a, TCGCond cond) > >>>>>> > >>>>>> gen_set_label(l); /* branch taken */ > >>>>>> > >>>>>> - if (!has_ext(ctx, RVC) && ((ctx->base.pc_next + a->imm) & 0x3)) { > >>>>>> + if (!ctx->cfg_ptr->ext_zca && ((ctx->base.pc_next + a->imm) & > >>>>>> 0x3)) { > >>>>>> /* misaligned */ > >>>>>> gen_exception_inst_addr_mis(ctx); > >>>>>> } else { > >>>>>> diff --git a/target/riscv/translate.c b/target/riscv/translate.c > >>>>>> index 0ee8ee147d..d1fdd0c2d7 100644 > >>>>>> --- a/target/riscv/translate.c > >>>>>> +++ b/target/riscv/translate.c > >>>>>> @@ -549,7 +549,7 @@ static void gen_jal(DisasContext *ctx, int rd, > >>>>>> target_ulong imm) > >>>>>> > >>>>>> /* check misaligned: */ > >>>>>> next_pc = ctx->base.pc_next + imm; > >>>>>> - if (!has_ext(ctx, RVC)) { > >>>>>> + if (!ctx->cfg_ptr->ext_zca) { > >>>>>> if ((next_pc & 0x3) != 0) { > >>>>>> gen_exception_inst_addr_mis(ctx); > >>>>>> return; > >>>>>> @@ -1122,7 +1122,11 @@ static void decode_opc(CPURISCVState *env, > >>>>>> DisasContext *ctx, uint16_t opcode) > >>>>>> if (insn_len(opcode) == 2) { > >>>>>> ctx->opcode = opcode; > >>>>>> ctx->pc_succ_insn = ctx->base.pc_next + 2; > >>>>>> - if (has_ext(ctx, RVC) && decode_insn16(ctx, opcode)) { > >>>>>> + /* > >>>>>> + * The Zca extension is added as way to refer to instructions > >>>>>> in the C > >>>>>> + * extension that do not include the floating-point loads and > >>>>>> stores > >>>>>> + */ > >>>>>> + if (ctx->cfg_ptr->ext_zca && decode_insn16(ctx, opcode)) { > >>>>>> return; > >>>>>> } > >>>>>> } else { > >
