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

[liweiwei]

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 {