Re: [PATCH v2 05/13] target/riscv: Add MXL/SXL/UXL to TB_FLAGS

[Richard Henderson <richard.henderson@linaro.org>]

On 10/14/21 1:20 AM, LIU Zhiwei wrote:
> 
> On 2021/10/14 上午4:50, Richard Henderson wrote:
>> Begin adding support for switching XLEN at runtime.  Extract the
>> effective XLEN from MISA and MSTATUS and store for use during translation.
>>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>> v2: Force SXL and UXL to valid values.
>> ---
>>   target/riscv/cpu.h        |  2 ++
>>   target/riscv/cpu.c        |  8 ++++++++
>>   target/riscv/cpu_helper.c | 33 +++++++++++++++++++++++++++++++++
>>   target/riscv/csr.c        |  3 +++
>>   target/riscv/translate.c  |  2 +-
>>   5 files changed, 47 insertions(+), 1 deletion(-)
>>
>> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
>> index 87248b562a..445ba5b395 100644
>> --- a/target/riscv/cpu.h
>> +++ b/target/riscv/cpu.h
>> @@ -395,6 +395,8 @@ FIELD(TB_FLAGS, VILL, 8, 1)
>>   /* Is a Hypervisor instruction load/store allowed? */
>>   FIELD(TB_FLAGS, HLSX, 9, 1)
>>   FIELD(TB_FLAGS, MSTATUS_HS_FS, 10, 2)
>> +/* The combination of MXL/SXL/UXL that applies to the current cpu mode. */
>> +FIELD(TB_FLAGS, XL, 12, 2)
>>   #ifdef CONFIG_RISCV32
>>   #define riscv_cpu_mxl(env)      MXL_RV32
>> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
>> index 1857670a69..840edd66f8 100644
>> --- a/target/riscv/cpu.c
>> +++ b/target/riscv/cpu.c
>> @@ -355,6 +355,14 @@ static void riscv_cpu_reset(DeviceState *dev)
>>       env->misa_mxl = env->misa_mxl_max;
>>       env->priv = PRV_M;
>>       env->mstatus &= ~(MSTATUS_MIE | MSTATUS_MPRV);
>> +    if (env->misa_mxl > MXL_RV32) {
>> +        /*
>> +         * The reset status of SXL/UXL is officially undefined,
>> +         * but invalid settings would result in a tcg assert.
>> +         */
>> +        env->mstatus = set_field(env->mstatus, MSTATUS64_SXL, env->misa_mxl);
>> +        env->mstatus = set_field(env->mstatus, MSTATUS64_UXL, env->misa_mxl);
>> +    }
> 
> Can you give more explanation about the assert? As the cpu will always reset to M mode, I 
> think we can omit the the setting of UXL or SXL.

The mstatus csr is WARL, which means that we should always be able to read a valid value. 
  On init, these fields will still be 0, which isn't right.

I guess the assert that I was considering can't really happen, because we'd need to write 
to mstatus to exit M-mode, and write_mstatus will force these fields to the correct value 
(as found by Frederic).

r~