Re: [PATCH v12 02/10] target/riscv: add support for Zca extension

[Alistair Francis]

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 {
>