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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.04.2023 16:57, Luca Fancellu wrote:
>> 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? 

Well, yes, likely. But the question isn't that but "Why did the reference
not get removed, when it's inside an if(0) block?"

>>> --- 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?

That's my understanding too. Translated to C this means "slen >= nlen",
i.e. the "slen < nlen" case is covered. The "slen == nlen" case is then
covered by "s[nlen] != '='", which - due to the earlier guarantee - is
going to be in bounds. That's because even when e is non-NULL and points
at non-nul, it still points into a valid nul-terminated string. (But yes,
I see now that the "slen == nlen" case is a little hairy, so perhaps
indeed best to keep the check as you have it.)

Jan