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

[liweiwei]

On 2023/4/7 03:33, Richard Henderson wrote:
> 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.
I agree. Maybe PRV_RESERVED is more readable in some cases.
> 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;
OK. This is more readable. I'll update this.

Regards,

Weiwei Li

> r~