[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Luca Fancellu <Luca.Fancellu@xxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Mon, 24 Apr 2023 16:05:42 +0200
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=suse.com; dmarc=pass action=none header.from=suse.com; dkim=pass header.d=suse.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=TpcbWidt73BpmMU/JDX2CpCaPV4QH4c9ciMRnj8u0aE=; b=DrVFrxrTtGu794PW/SfTJ0b2U2E6oJRFE9nbh78FFXJk6fi7yJOdyDpBR68q316+cdgrRTl7WZKabwHW7PKKNUwyXKuH9yKZ4drwMrz09AT8yzDRri0Huv2b3aXHxsJFtODyvlhcjkt7Fh/qg0hORpAmwP8Xbzc8AtgrQOWt8iJ6EL8tgk94GXFIzLqA2KJcHFppC4/RyisGLl/XLzlPnZPzxLze1IIYwx4rTXRnomgSjjkdreqj6c0gGZMM/8adUwk7aAU8n878YgI1MOB/h/tAV/Dvoe263g5XI10N3+3x+x+86cSQ240DY70x3A4GBh6XLHBYpqvocwD5KTE2Og==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=E1OAGF7HyFFvIXa6PE3mXjKFp50KLs3K9fMsnvp9LNpjlhTqZMXiepW2/YX437n3VdO72ZNRtkYSunk560xWBZJxsqwEal0y8XMNeGrcd+OXtEKu/u93BxSvTPqJxjJ5rR0/aRQJJBAjJMERSRKAIl7dz0pt0pffGePUXENTRotkaWgaSdq/g3B9moNVg/IANa1IT6dUtKyYTNHagGSnBGkPk6KnaQ9c9cHWt+bx/SKUtjhl3Z3VQqtHsuW/etkrMlGLaZJGWFjCDFnAj7y3DBg2zrNmMnr2is9PH2oIFBjWmaaNKKsEWsrvqRIOUOO6RqbV6sR0NUi0vcNG/5GrDg==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=suse.com;
  • Cc: Bertrand Marquis <Bertrand.Marquis@xxxxxxx>, Wei Chen <Wei.Chen@xxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, George Dunlap <george.dunlap@xxxxxxxxxx>, Julien Grall <julien@xxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Mon, 24 Apr 2023 14:06:01 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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.

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

Jan



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.