Re: [PATCH v4 02/12] xen/arm: add SVE vector length field to the domain

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.03.2023 12:59, Luca Fancellu wrote:
> @@ -43,8 +44,16 @@ register_t compute_max_zcr(void)
>  }
>  
>  /* Takes a vector length in bits and returns the ZCR_ELx encoding */
> -register_t vl_to_zcr(uint16_t vl)
> +register_t vl_to_zcr(unsigned int vl)
>  {
>      ASSERT(vl > 0);
>      return ((vl / SVE_VL_MULTIPLE_VAL) - 1U) & ZCR_ELx_LEN_MASK;
>  }
> +
> +/* Get the system sanitized value for VL in bits */
> +unsigned int get_sys_vl_len(void)
> +{
> +    /* ZCR_ELx len field is ((len+1) * 128) = vector bits length */
> +    return ((system_cpuinfo.zcr64.bits[0] & ZCR_ELx_LEN_MASK) + 1U) *
> +            SVE_VL_MULTIPLE_VAL;
> +}

Wouldn't this function better return 0 when !cpu_has_sve?

> @@ -602,6 +606,31 @@ int arch_sanitise_domain_config(struct 
> xen_domctl_createdomain *config)
>          return -EINVAL;
>      }
>  
> +    /* Check feature flags */
> +    if ( sve_vl_bits > 0 ) {

Nit: Style (brace placement).

> +        unsigned int zcr_max_bits = get_sys_vl_len();
> +
> +        if ( !cpu_has_sve )
> +        {
> +            dprintk(XENLOG_INFO, "SVE is unsupported on this machine.\n");
> +            return -EINVAL;
> +        }
> +        else if ( !is_vl_valid(sve_vl_bits) )

If this was code I'm the maintainer for, I'd ask for the "else" to be
dropped. Please consider doing so, as it makes more visible that the
earlier if() cannot "fall through". (This could then be further
supported by inserting blank lines, here and again right below.)

As to the check - this being the only user of is_vl_valid() (at this
point) I'd like to mention that half of what that function checks is
now pointless, as we're dealing with a decoded value. Future further
users may need the full value checked, but given that all interfaces
are now using encoded values this doesn't seem very likely. Hence the
respective part of the condition there may want to become an
assertion instead (or be dropped).

Yet another question is whether both range checks (against
SVE_VL_MAX_BITS and zcr_max_bits) are actually necessary / useful.
Iirc 2048 is a hard upper bound, so zcr_max_bits being higher than
that value should likely lead to not using SVE at all (if it doesn't
already; didn't check).

> +        {
> +            dprintk(XENLOG_INFO, "Unsupported SVE vector length (%u)\n",
> +                    sve_vl_bits);
> +            return -EINVAL;
> +        }
> +        else if ( sve_vl_bits > zcr_max_bits )
> +        {
> +            dprintk(XENLOG_INFO,
> +                    "The requested SVE vector length (%u) must be lower or 
> \n"
> +                    "equal to the platform supported vector length (%u)\n",
> +                    sve_vl_bits, zcr_max_bits);

Again, if I was the maintainer of this code, I'd ask for the message
to be shortened, such that it easily fits on one line. E.g.
"requested SVE vector length (%u) > supported length (%u)\n". This
would then also avoid the apparent grammar issue of "lower" not fitting
with "to" (i.e. a "than" needing inserting, or "below" being used
instead).

Jan