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

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

> On 28 Mar 2023, at 11:08, Jan Beulich <jbeulich@xxxxxxxx> wrote:
> 
> 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".

This is a very good suggestion, do you think I should restrict only to one 
negative value,
for example -1, instead of every negative value?

> 
>> +    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.

I will add it

> 
>> +    Please note that the platform can supports a lower value, if the 
>> requested
> 
> Maybe better "... may only support ..."?

ok

> 
>> +    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.

I guess that with the introduction of a negative value meaning max supported
VL, it is ok to stop the system if the user provides a custom VL value that is
not OK. I remember Julien asked to stop the creation of Dom0 in such a case on
the RFC serie.

> 
>> @@ -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, ..."

Ok I will address all the comments above

> 
> Jan