Re: [PATCH 11/13] target/riscv: Switch context in exception return

[LIU Zhiwei <zhiwei_liu@c-sky.com>]

On 2021/11/2 上午12:43, Richard Henderson wrote:
> On 11/1/21 6:01 AM, LIU Zhiwei wrote:
>> After excpetion return, we should give a xlen view of context in new
>> priveledge, including the general registers, pc, and CSRs.
>>
>> Signed-off-by: LIU Zhiwei <zhiwei_liu@c-sky.com>
>> ---
>>   target/riscv/helper.h                         |  1 +
>>   .../riscv/insn_trans/trans_privileged.c.inc   |  2 ++
>>   target/riscv/op_helper.c                      | 26 +++++++++++++++++++
>>   3 files changed, 29 insertions(+)
>>
>> diff --git a/target/riscv/helper.h b/target/riscv/helper.h
>> index e198d43981..9b379d7232 100644
>> --- a/target/riscv/helper.h
>> +++ b/target/riscv/helper.h
>> @@ -71,6 +71,7 @@ DEF_HELPER_2(sret, tl, env, tl)
>>   DEF_HELPER_2(mret, tl, env, tl)
>>   DEF_HELPER_1(wfi, void, env)
>>   DEF_HELPER_1(tlb_flush, void, env)
>> +DEF_HELPER_1(switch_context_xl, void, env)
>>   #endif
>>     /* Hypervisor functions */
>> diff --git a/target/riscv/insn_trans/trans_privileged.c.inc 
>> b/target/riscv/insn_trans/trans_privileged.c.inc
>> index 75c6ef80a6..6e39632f83 100644
>> --- a/target/riscv/insn_trans/trans_privileged.c.inc
>> +++ b/target/riscv/insn_trans/trans_privileged.c.inc
>> @@ -78,6 +78,7 @@ static bool trans_sret(DisasContext *ctx, arg_sret *a)
>>         if (has_ext(ctx, RVS)) {
>>           gen_helper_sret(cpu_pc, cpu_env, cpu_pc);
>> +        gen_helper_switch_context_xl(cpu_env);
>>           tcg_gen_exit_tb(NULL, 0); /* no chaining */
>>           ctx->base.is_jmp = DISAS_NORETURN;
>>       } else {
>> @@ -94,6 +95,7 @@ static bool trans_mret(DisasContext *ctx, arg_mret *a)
>>   #ifndef CONFIG_USER_ONLY
>>       tcg_gen_movi_tl(cpu_pc, ctx->base.pc_next);
>>       gen_helper_mret(cpu_pc, cpu_env, cpu_pc);
>> +    gen_helper_switch_context_xl(cpu_env);
>>       tcg_gen_exit_tb(NULL, 0); /* no chaining */
>>       ctx->base.is_jmp = DISAS_NORETURN;
>>       return true;
>> diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
>> index ee7c24efe7..20cf8ad883 100644
>> --- a/target/riscv/op_helper.c
>> +++ b/target/riscv/op_helper.c
>> @@ -70,6 +70,32 @@ target_ulong helper_csrrw(CPURISCVState *env, int 
>> csr,
>>   }
>>     #ifndef CONFIG_USER_ONLY
>> +void helper_switch_context_xl(CPURISCVState *env)
>> +{
>> +    RISCVMXL xl = cpu_get_xl(env);
>> +    if (xl == env->misa_mxl_max) {
>> +        return;
>> +    }
>> +    assert(xl < env->misa_mxl_max);
>> +    switch (xl) {
>> +    case MXL_RV32:
>> +        for (int i = 1; i < 32; i++) {
>> +            env->gpr[i] = (int32_t)env->gpr[i];
>> +        }
>
> I think this is wrong.  As I read the spec, the new context ignores 
> high bits and writes sign-extended values, but registers that are not 
> written should not be changed.
I think so.
>
> I think that a unit test with SXLEN == 64 and UXLEN == 32, where the 
> U-mode program executes only the ECALL instruction, should leave the 
> high 32 bits of all gprs unchanged on re-entry to S-mode.
Re-entry to U-mode?  I think you are right.  But currently I  don't have 
a hardware has implemented the UXL32.
>
>> +        env->pc = (int32_t)env->pc;
>
> I think this will happen naturally via patch 3.
Maybe we have to sign extend here as clarified in other reply.
>
>> +        /*
>> +         * For the read-only bits of the previous-width CSR, the 
>> bits at the
>> +         * same positions in the temporary register are set to zeros.
>> +         */
>> +        if ((env->priv == PRV_U) && (env->misa_ext & RVV)) {
>> +            env->vl = 0;
>> +            env->vtype = 0;
>> +        }
>
> I don't understand this.  The return from the S-mode interrupt handler 
> to the U-mode program should preserve the U-mode VTYPE.
>
>
According to the privileged architecture specification,  section 2.5,  
if the width of a CSR is changed, the read-only bits of the 
previous-width CSR are zeroed.
More general,  there is an algorithm how to calculate the new CSR value 
from the previous CSR.

I am not sure which is the right time  to do this context switch. The 
write time of  UXL field , or return to the U-mode?


Thanks,
Zhiwei

> r~