> On 24 Apr 2023, at 15:05, Jan Beulich <jbeul...@suse.com> wrote:
>
> On 24.04.2023 16:00, Luca Fancellu wrote:
>>> On 24 Apr 2023, at 12:34, Jan Beulich <jbeul...@suse.com> 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
