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

[Richard Henderson]

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~