Re: [PATCH v5 07/12] xen: enable Dom0 to use SVE feature

[Jan Beulich <jbeulich@xxxxxxxx>]

On 12.04.2023 11:49, Luca Fancellu wrote:
> @@ -118,3 +121,21 @@ void sve_restore_state(struct vcpu *v)
>  
>      sve_load_ctx(sve_ctx_zreg_end, v->arch.vfp.fpregs, 1);
>  }
> +
> +int __init sve_sanitize_vl_param(int val, unsigned int *out)
> +{
> +    /*
> +     * Negative SVE parameter value means to use the maximum supported
> +     * vector length, otherwise if a positive value is provided, check if the
> +     * vector length is a multiple of 128 and not bigger than the maximum 
> value
> +     * 2048
> +     */
> +    if ( val < 0 )
> +        *out = get_sys_vl_len();
> +    else if ( ((val % SVE_VL_MULTIPLE_VAL) == 0) && (val <= SVE_VL_MAX_BITS) 
> )
> +        *out = val;
> +    else
> +        return -1;
> +
> +    return 0;
> +}

I think such a function wants to either return boolean, or -E... in the
error case. Boolean would ...

> @@ -4109,6 +4125,17 @@ void __init create_dom0(void)
>      if ( iommu_enabled )
>          dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu;
>  
> +    if ( opt_dom0_sve )
> +    {
> +        unsigned int vl;
> +
> +        if ( !sve_sanitize_vl_param(opt_dom0_sve, &vl) )

... yield a slightly better call site here, imo.

> +            dom0_cfg.arch.sve_vl = sve_encode_vl(vl);
> +        else
> +            printk(XENLOG_WARNING
> +                   "SVE vector length error, disable feature for Dom0\n");

I appreciate the now better behavior here, but I think the respective part
of the doc is now stale?

> @@ -28,9 +35,12 @@ int sve_context_init(struct vcpu *v);
>  void sve_context_free(struct vcpu *v);
>  void sve_save_state(struct vcpu *v);
>  void sve_restore_state(struct vcpu *v);
> +int sve_sanitize_vl_param(int val, unsigned int *out);
>  
>  #else /* !CONFIG_ARM64_SVE */
>  
> +#define opt_dom0_sve (0)

With this I don't think you need ...

> @@ -55,6 +65,11 @@ static inline void sve_context_free(struct vcpu *v) {}
>  static inline void sve_save_state(struct vcpu *v) {}
>  static inline void sve_restore_state(struct vcpu *v) {}
>  
> +static inline int sve_sanitize_vl_param(int val, unsigned int *out)
> +{
> +    return -1;
> +}

... such a stub function; having the declaration visible should be
enough for things to build (thanks to DCE, which we use for similar
purposes on many other places).

> --- a/xen/common/kernel.c
> +++ b/xen/common/kernel.c
> @@ -314,6 +314,31 @@ int parse_boolean(const char *name, const char *s, const 
> char *e)
>      return -1;
>  }
>  
> +int __init parse_signed_integer(const char *name, const char *s, const char 
> *e,
> +                                long long *val)
> +{
> +    size_t slen, nlen;
> +    const char *str;
> +    long long pval;

What use is this extra variable?

> +    slen = e ? ({ ASSERT(e >= s); e - s; }) : strlen(s);
> +    nlen = strlen(name);
> +
> +    /* Does s start with name or contains only the name? */
> +    if ( (slen <= nlen) || strncmp(s, name, nlen) || (s[nlen] != '=') )
> +        return -1;

The comment imo wants wording consistently positive or consistently
negative. IOW either you say what you're looking for, or you say
what you're meaning to reject.

> +    pval = simple_strtoll(&s[nlen + 1], &str, 0);

I wonder whether, when potentially expecting negative numbers,
accepting other than decimal numbers here is useful.

Jan