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

[Alistair Francis]

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