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

[liweiwei]

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 {