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

[liweiwei <liweiwei@iscas.ac.cn>]

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.

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 {