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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.04.2023 08:02, Luca Fancellu wrote:
> @@ -30,9 +37,11 @@ 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);
> +bool sve_domctl_vl_param(int val, unsigned int *out);
>  
>  #else /* !CONFIG_ARM64_SVE */
>  
> +#define opt_dom0_sve     (0)
>  #define is_sve_domain(d) (0)
>  
>  static inline register_t compute_max_zcr(void)
> @@ -59,6 +68,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 bool sve_domctl_vl_param(int val, unsigned int *out)
> +{
> +    return false;
> +}

Once again I don't see the need for this stub: opt_dom0_sve is #define-d
to plain zero when !ARM64_SVE, so the only call site merely requires a
visible declaration, and DCE will take care of eliminating the actual call.

> --- 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;
> +
> +    slen = e ? ({ ASSERT(e >= s); e - s; }) : strlen(s);

As per this "e" may come in as NULL, meaning that ...

> +    nlen = strlen(name);
> +
> +    /* Check that this is the name we're looking for and a value was 
> provided */
> +    if ( (slen <= nlen) || strncmp(s, name, nlen) || (s[nlen] != '=') )
> +        return -1;
> +
> +    pval = simple_strtoll(&s[nlen + 1], &str, 0);
> +
> +    /* Number not recognised */
> +    if ( str != e )
> +        return -2;

... this is always going to lead to failure in that case. (I guess I could
have spotted this earlier, sorry.)

As a nit, I'd also appreciate if style here (parenthesization in particular)
could match that of parse_boolean(), which doesn't put parentheses around
the operands of comparison operators (a few lines up from here). With the
other function in mind, I'm then not going to pick on the seemingly
redundant (with the subsequent strncmp()) "slen <= nlen", which has an
equivalent there as well.

Jan