Re: [PATCH v2 2/2] target/riscv: Legalize MPP value in write_mstatus

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

On 4/6/23 00:25, Weiwei Li wrote:
> +static target_ulong legalize_mpp(CPURISCVState *env, target_ulong old_mpp,
> +                                 target_ulong val)
> +{
> +    target_ulong new_mpp = get_field(val, MSTATUS_MPP);
> +    bool mpp_invalid = (new_mpp == PRV_S && !riscv_has_ext(env, RVS)) ||
> +                       (new_mpp == PRV_U && !riscv_has_ext(env, RVU)) ||
> +                       (new_mpp == PRV_H);
> +
> +    /* Remain field unchanged if new_mpp value is invalid */
> +    return mpp_invalid ? set_field(val, MSTATUS_MPP, old_mpp) : val;
> +}

Does anyone find PRV_H confusing, since that's not what it is?
I think it would be nice to remove it entirely.

This function might be better as

     bool valid = false;

     switch (new_mpp) {
     case PRV_M:
         valid = true;
         break;
     case PRV_S:
         valid = riscv_has_ext(env, RVS);
         break;
     case PRV_U:
         valid = riscv_has_ext(env, RVU);
         break;
     }
     if (!valid) {
         val = set_field(...);
     }
     return val;


r~