Re: [PATCH v4 4/9] target/arm/cpu64: max cpu: Introduce sve<N> properties

[Andrew Jones]

On Thu, Sep 26, 2019 at 12:07:54PM -0700, Richard Henderson wrote:
> On 9/24/19 4:31 AM, Andrew Jones wrote:
> > +static uint32_t sve_zcr_get_valid_len(ARMCPU *cpu, uint32_t start_len)
> > +{
> > +    uint32_t start_vq = (start_len & 0xf) + 1;
> > +
> > +    return arm_cpu_vq_map_next_smaller(cpu, start_vq + 1) - 1;
> > +}
> > +
> >  /*
> >   * Given that SVE is enabled, return the vector length for EL.
> >   */
> > @@ -5360,13 +5367,13 @@ uint32_t sve_zcr_len_for_el(CPUARMState *env, int 
> > el)
> >      uint32_t zcr_len = cpu->sve_max_vq - 1;
> >  
> >      if (el <= 1) {
> > -        zcr_len = MIN(zcr_len, 0xf & (uint32_t)env->vfp.zcr_el[1]);
> > +        zcr_len = sve_zcr_get_valid_len(cpu, env->vfp.zcr_el[1]);
> >      }
> >      if (el <= 2 && arm_feature(env, ARM_FEATURE_EL2)) {
> > -        zcr_len = MIN(zcr_len, 0xf & (uint32_t)env->vfp.zcr_el[2]);
> > +        zcr_len = sve_zcr_get_valid_len(cpu, env->vfp.zcr_el[2]);
> >      }
> >      if (arm_feature(env, ARM_FEATURE_EL3)) {
> > -        zcr_len = MIN(zcr_len, 0xf & (uint32_t)env->vfp.zcr_el[3]);
> > +        zcr_len = sve_zcr_get_valid_len(cpu, env->vfp.zcr_el[3]);
> 
> This has lost the MIN relation between these 3 registers.
> One possible solution is to keep these 3 if statements as-is,
> but make one call
> 
>     zcr_len = arm_cpu_vq_map_next_smaller(cpu, zcr_len);
> 
> at the end.

Argh. I obviously didn't look closely enough at this function when I was
modifying it. I must have interpreted it as an if-else-if sequence...

I'll fix this as you suggest.

> 
> > +#if __SIZEOF_LONG__ == 8
> > +#define BIT(n) (1UL << (n))
> > +#else
> > +#define BIT(n) (1ULL << (n))
> > +#endif
> 
> There's no reason not to always use 1ULL is there?

Indeed ULL == UL for AArch64. I'll drop the #if... stuff.

Thanks,
drew