Re: [PATCH 04/13] target/riscv: Replace riscv_cpu_is_32bit with riscv_cpu_mxl

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

On 10/13/21 9:46 AM, Frédéric Pétrot wrote:
> Hello,
> 
> Le 07/10/2021 à 19:47, Richard Henderson a écrit :
>> Shortly, the set of supported XL will not be just 32 and 64,
>> and representing that properly using the enumeration will be
>> imperative.
>>
>> Two places, booting and gdb, intentionally use misa_mxl_max
>> to emphasize the use of the reset value of misa.mxl, and not
>> the current cpu state.
>>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>>   target/riscv/cpu.h            |  9 ++++++++-
>>   hw/riscv/boot.c               |  2 +-
>>   semihosting/arm-compat-semi.c |  2 +-
>>   target/riscv/cpu.c            | 24 ++++++++++++++----------
>>   target/riscv/cpu_helper.c     | 12 ++++++------
>>   target/riscv/csr.c            | 24 ++++++++++++------------
>>   target/riscv/gdbstub.c        |  2 +-
>>   target/riscv/monitor.c        |  4 ++--
>>   8 files changed, 45 insertions(+), 34 deletions(-)
> 
> 
>> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
>> @@ -522,7 +522,7 @@ static RISCVException write_mstatus(CPURISCVState *env, int csrno,
>>   
>>       dirty = ((mstatus & MSTATUS_FS) == MSTATUS_FS) |
>>               ((mstatus & MSTATUS_XS) == MSTATUS_XS);
>> -    if (riscv_cpu_is_32bit(env)) {
>> +    if (riscv_cpu_mxl(env) == MXL_RV32) {
>>           mstatus = set_field(mstatus, MSTATUS32_SD, dirty);
>>       } else {
>>           mstatus = set_field(mstatus, MSTATUS64_SD, dirty);
> 
> I believe we miss the settings of the SXL and UXL fields that are needed
> by cpu_get_xl
> 
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 9c0753bc8b..c4a479ddd2 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -526,6 +526,9 @@ static RISCVException write_mstatus(CPURISCVState *env, int
> csrno,
>           mstatus = set_field(mstatus, MSTATUS32_SD, dirty);
>       } else {
>           mstatus = set_field(mstatus, MSTATUS64_SD, dirty);
> +        /* SXL and UXL fields are for now read only */
> +        mstatus = set_field(mstatus, MSTATUS64_SXL, MXL_RV64);
> +        mstatus = set_field(mstatus, MSTATUS64_UXL, MXL_RV64);
>       }
>       env->mstatus = mstatus;

Yes, I missed that.  I was relying too much on the UXL patch set which I did not 
incorporate here.  This is a good minimal addition to keep things bisectable.


r~