Re: [PATCH v2 12/14] target/riscv: Split out the vill from vtype

[LIU Zhiwei]

On 2021/11/10 下午7:23, Richard Henderson wrote:
> On 11/10/21 8:04 AM, LIU Zhiwei wrote:
> > We need not specially process vtype when XLEN changes.
> > Signed-off-by: LIU Zhiwei <zhiwei_liu@c-sky.com>
> > ---
> >   target/riscv/cpu.h           |  1 +
> >   target/riscv/csr.c           | 15 ++++++++++++++-
> >   target/riscv/machine.c       |  1 +
> >   target/riscv/vector_helper.c |  7 ++-----
> >   4 files changed, 18 insertions(+), 6 deletions(-)
>
> This patch should come before patch 6, which is over-complicated.

Agree.

One question here. Even come before patch 6, we don't have a simple way 
to choose vill and reserved fields from s2 register in patch 6.

> > +    target_ulong vill;
>
> This could be bool, though there's no good place to slot it that does 
> not result in unused padding.

As env->vill will be used in read_vtype,  we still need to covert 
env->vill type to target_ulong there.
Is there any benefit to use bool instead of target_ulong?

> Comments should be added to show that this bit is now missing from vtype.
>
> > diff --git a/target/riscv/machine.c b/target/riscv/machine.c
> > index 19e982d3f0..cc4dda4b93 100644
> > --- a/target/riscv/machine.c
> > +++ b/target/riscv/machine.c
> > @@ -104,6 +104,7 @@ static const VMStateDescription vmstate_vector = {
> >               VMSTATE_UINTTL(env.vl, RISCVCPU),
> >               VMSTATE_UINTTL(env.vstart, RISCVCPU),
> >               VMSTATE_UINTTL(env.vtype, RISCVCPU),
> > +            VMSTATE_UINTTL(env.vill, RISCVCPU),
> >               VMSTATE_END_OF_LIST()
>
> This will need a bump to version.
>
> > @@ -45,11 +45,8 @@ target_ulong HELPER(vsetvl)(CPURISCVState *env, 
> > target_ulong s1,
> >       }
> >       if ((sew > cpu->cfg.elen) || vill || (ediv != 0) || (reserved 
> > != 0)) {
> >           /* only set vill bit. */
> > -        if (xlen < TARGET_LONG_BITS) {
> > -            env->vtype = FIELD_DP64(0, VTYPE, VILL_XLEN32, 1);
> > -        } else {
> > -            env->vtype = FIELD_DP64(0, VTYPE, VILL, 1);
> > -        }
> > +        env->vill = 1;
> > +        env->vtype = 0;
>
> This is fine.
>
> You're missing the updates to cpu_get_tb_cpu_state.

Yes.

Thanks,
Zhiwei

> r~