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

[Alistair Francis]

On Thu, Apr 13, 2023 at 3:24 AM Daniel Henrique Barboza
<dbarboza@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 <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?
> >
> > May not very large.  Just one or two additional check in instruction 
> > translation. You can see the patch at:
> >
> > 20230412030648.80470-1-liweiwei@iscas.ac.cn/">https://lore.kernel.org/qemu-devel/20230412030648.80470-1-liweiwei@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:
> >
> > 20230410033526.31708-1-liweiwei@iscas.ac.cn/">https://lore.kernel.org/qemu-devel/20230410033526.31708-1-liweiwei@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 <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 {
> >