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

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

Hi Jan,

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

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

diff --git a/xen/common/kernel.c b/xen/common/kernel.c
index b67d9056fec3..7cd00a4c999a 100644
--- 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



> 
> Jan