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

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

> On 24 Apr 2023, at 15:05, Jan Beulich <jbeulich@xxxxxxxx> wrote:
> 
> On 24.04.2023 16:00, Luca Fancellu wrote:
>>> On 24 Apr 2023, at 12:34, Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>> 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.
>> 
>> I’ve tried to do that, I’ve put the declaration outside the ifdef so that it 
>> was always included
>> and I removed the stub, but I got errors on compilation because of undefined 
>> function.
>> For that reason  I left that change out.
> 
> Interesting. I don't see where the reference would be coming from.

Could it be because the declaration is visible, outside the ifdef, but the 
definition is not compiled in? 

>>>> --- 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.
>> 
>> You are right, do you think this will be ok:
> 
> It'll do, I guess.
> 
>> --- a/xen/common/kernel.c
>> +++ b/xen/common/kernel.c
>> @@ -324,11 +324,14 @@ int __init parse_signed_integer(const char *name, 
>> const char *s, const char *e,
>>     slen = e ? ({ ASSERT(e >= s); e - s; }) : strlen(s);
>>     nlen = strlen(name);
>> 
>> +    if ( !e )
>> +        e = s + slen;
>> +
>>     /* Check that this is the name we're looking for and a value was 
>> provided */
>> -    if ( (slen <= nlen) || strncmp(s, name, nlen) || (s[nlen] != '=') )
>> +    if ( slen <= nlen || strncmp(s, name, nlen) || s[nlen] != '=' )
>>         return -1;
>> 
>> -    pval = simple_strtoll(&s[nlen + 1], &str, 0);
>> +    pval = simple_strtoll(&s[nlen + 1], &str, 10);
>> 
>>     /* Number not recognised */
>>     if ( str != e )
>> 
>> 
>> Please note that I’ve also included your comment about the base, which I 
>> forgot to add, apologies for that.
>> 
>> slen <= nlen doesn’t seems redundant to me, I have that because I’m 
>> accessing s[nlen] and I would like
>> the string s to be at least > nlen
> 
> Right, but doesn't strncmp() guarantee that already?

I thought strncmp() guarantees s contains at least nlen chars, meaning from 0 
to nlen-1, is my understanding wrong?

> 
> Jan