Re: [PATCH v4 4/4] target/riscv: add vector configure instruction

[LIU Zhiwei]

On 2020/2/12 0:56, Richard Henderson wrote:
> On 2/10/20 8:12 AM, LIU Zhiwei wrote:
> >   static inline void cpu_get_tb_cpu_state(CPURISCVState *env, target_ulong *pc,
> > -                                        target_ulong *cs_base, uint32_t *flags)
> > +                                        target_ulong *cs_base, uint32_t 
> > *pflags)
> >   {
> > +    uint32_t flags = 0;
> > +    uint32_t vlmax;
> > +    uint8_t vl_eq_vlmax;
> bool.
OK.

Is it clearer to use "bool" here? Or it's wrong to use "uint8_t "?
> > +
> >       *pc = env->pc;
> >       *cs_base = 0;
> > +
> > +    if (env->misa & RVV) {
> > +        vlmax = vext_get_vlmax(env_archcpu(env), env->vext.vtype);
> > +        vl_eq_vlmax = (env->vext.vstart == 0) && (vlmax == env->vext.vl);
> You might as well move the variable declarations inside this block.
OK.
> > +target_ulong HELPER(vsetvl)(CPURISCVState *env, target_ulong s1,
> > +     target_ulong s2)
> Indentation.
OK.
> > +{
> > +    int vlmax, vl;
> > +    RISCVCPU *cpu = env_archcpu(env);
> > +    uint16_t sew = 1 << FIELD_EX64(s2, VTYPE, SEW);
> > +
> > +    if (sew > cpu->cfg.elen) { /* only set vill bit. */
> > +        env->vext.vtype = FIELD_DP64(0, VTYPE, VILL, 1);
> > +        env->vext.vl = 0;
> > +        env->vext.vstart = 0;
> > +        return 0;
> > +    }
> You're missing checks against EDIV, VILL and the RESERVED field == 0.
This implementation does not support "Zvediv" . So I did not check it. 
I'm not sure if I should check(ediv==0).

I missed check  "VILL" filed.  Fix up it next patch.

I'm not quite sure if I should set VILL if  the RESERVED field != 0.

> > +
> > +    vlmax = vext_get_vlmax(cpu, s2);
> > +    if (s1 <= vlmax) {
> > +        vl = s1;
> > +    } else {
> > +        vl = vlmax;
> > +    }
> > +    env->vext.vl = vl;
> > +    env->vext.vtype = s2;
> > +    env->vext.vstart = 0;
> > +    return vl;
> > +}
>
> r~