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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.03.2023 12:59, Luca Fancellu wrote:
> @@ -838,6 +838,18 @@ Controls for how dom0 is constructed on x86 systems.
>  
>      If using this option is necessary to fix an issue, please report a bug.
>  
> +Enables features on dom0 on Arm systems.
> +
> +*   The `sve` integer parameter enables Arm SVE usage for Dom0 domain and 
> sets
> +    the maximum SVE vector length.
> +    Values above 0 means feature is enabled for Dom0, otherwise feature is
> +    disabled.

Nit: "above" suggests negative values may also enable the feature, which
I'm sure isn't intended. You may want to consider using negative values
to signal "use length supported by hardware".

> +    Possible values are from 0 to maximum 2048, being multiple of 128, that 
> will
> +    be the maximum vector length.

It may be advisable to also state the default here.

> +    Please note that the platform can supports a lower value, if the 
> requested

Maybe better "... may only support ..."?

> +    value is above the supported one, the domain creation will fail and the
> +    system will stop.

Such behavior may be acceptable for DomU-s which aren't essential for the
system (i.e. possibly excluding ones in dom0less scenarios), but I don't
think that's very nice for Dom0. I'd rather suggest falling back to no
SVE in such an event.

> @@ -115,3 +119,8 @@ void sve_restore_state(struct vcpu *v)
>  
>      sve_load_ctx(sve_ctx_zreg_end, v->arch.vfp.fpregs, 1);
>  }
> +
> +int __init sve_parse_dom0_param(const char *str_begin, const char *str_end)
> +{
> +    return parse_integer("sve", str_begin, str_end, (int*)&opt_dom0_sve);

Please can you avoid introducing casts like this? If you're after an unsigned
value, make a function which only parses (and returns) an unsigned one. Also
why ...

> @@ -61,7 +62,12 @@ custom_param("dom0_mem", parse_dom0_mem);
>  
>  int __init parse_arch_dom0_param(const char *str_begin, const char *str_end)
>  {
> -    return -1;
> +    int rc = 0;
> +
> +    if ( sve_parse_dom0_param(str_begin, str_end) < 0 )
> +        rc = -EINVAL;

... can't you call parse_integer() right here? opt_dom0_sve isn't static,
so ought to be accessible here (provided the necessary header was included).

> --- a/xen/common/kernel.c
> +++ b/xen/common/kernel.c
> @@ -314,6 +314,30 @@ int parse_boolean(const char *name, const char *s, const 
> char *e)
>      return -1;
>  }
>  
> +int parse_integer(const char *name, const char *s, const char *e,
> +                  int *val)
> +{
> +    size_t slen, nlen;
> +    const char *str;
> +    long long pval;
> +
> +    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;
> +
> +    pval = simple_strtoll(&s[nlen + 1], &str, 0);
> +
> +    if ( (str != e) || (pval < INT_MIN) || (pval > INT_MAX) )
> +        return -2;

Like its counterpart in parse_boolean() (which I understand you've
derived parts of the function from) this if+return wants a comment.
Also - why strtoll() when you're only after an int? Yet then another
question is whether we really want to gain parse_long() and
parse_longlong() functions subsequently, or whether instead we
limit ourselves to (e.g.) parse_signed_integer() and
parse_unsigned_integer(), taking long long * and unsigned long long *
respectively to store their outputs. (Of course right now you'd
implement only one of the two.)

Finally, for the purposes right now the function can (and should) be
__init.

> --- a/xen/include/xen/lib.h
> +++ b/xen/include/xen/lib.h
> @@ -94,6 +94,16 @@ int parse_bool(const char *s, const char *e);
>   */
>  int parse_boolean(const char *name, const char *s, const char *e);
>  
> +/**
> + * Given a specific name, parses a string of the form:
> + *   $NAME[=...]
> + * returning 0 and a value in val, for a recognised integer.
> + * Returns -1 for name not found, general errors, or -2 if name is found but
> + * not recognised/overflow/underflow value.
> + */
> +int parse_integer(const char *name, const char *s, const char *e,
> +                  int *val);

The comment wants to match function behavior: The '=' and the value
aren't optional as per the implementation, unlike for parse_boolean().
Also please be precise and say "... and a value in *val, ..."

Jan