| Message ID | 20230424060248.1488859-8-luca.fancellu@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | SVE feature for arm guests | expand |
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. > --- 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. Jan
Hi Jan, > On 24 Apr 2023, at 12:34, Jan Beulich <jbeulich@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. > >> --- 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
On 24.04.2023 16:00, Luca Fancellu wrote: >> On 24 Apr 2023, at 12:34, Jan Beulich <jbeulich@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. >>> --- 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
> On 24 Apr 2023, at 15:05, Jan Beulich <jbeulich@suse.com> wrote: > > On 24.04.2023 16:00, Luca Fancellu wrote: >>> On 24 Apr 2023, at 12:34, Jan Beulich <jbeulich@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
On 24.04.2023 16:57, Luca Fancellu wrote: >> On 24 Apr 2023, at 15:05, Jan Beulich <jbeulich@suse.com> wrote: >> On 24.04.2023 16:00, Luca Fancellu wrote: >>>> On 24 Apr 2023, at 12:34, Jan Beulich <jbeulich@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? 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
> On 24 Apr 2023, at 16:06, Jan Beulich <jbeulich@suse.com> wrote: > > On 24.04.2023 16:57, Luca Fancellu wrote: >>> On 24 Apr 2023, at 15:05, Jan Beulich <jbeulich@suse.com> wrote: >>> On 24.04.2023 16:00, Luca Fancellu wrote: >>>>> On 24 Apr 2023, at 12:34, Jan Beulich <jbeulich@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? > > 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?" Oh ok, I don’t know, here what I get if for example I build arm32: arm-linux-gnueabihf-ld -EL -T arch/arm/xen.lds -N prelink.o \ ./common/symbols-dummy.o -o ./.xen-syms.0 arm-linux-gnueabihf-ld: prelink.o: in function `create_domUs': (.init.text+0x13464): undefined reference to `sve_domctl_vl_param' arm-linux-gnueabihf-ld: (.init.text+0x136b4): undefined reference to `sve_domctl_vl_param' arm-linux-gnueabihf-ld: ./.xen-syms.0: hidden symbol `sve_domctl_vl_param' isn't defined arm-linux-gnueabihf-ld: final link failed: bad value make[3]: *** [/data_sdc/lucfan01/kirkstone_xen/xen/xen/arch/arm/Makefile:95: xen-syms] Error 1 make[2]: *** [/data_sdc/lucfan01/kirkstone_xen/xen/xen/./build.mk:90: xen] Error 2 make[1]: *** [/data_sdc/lucfan01/kirkstone_xen/xen/xen/Makefile:590: xen] Error 2 make[1]: Leaving directory '/data_sdc/lucfan01/kirkstone_xen/build/xen-qemu-arm32' make: *** [Makefile:181: __sub-make] Error 2 make: Leaving directory '/data_sdc/lucfan01/kirkstone_xen/xen/xen’ These are the modification I’ve done: diff --git a/xen/arch/arm/include/asm/arm64/sve.h b/xen/arch/arm/include/asm/arm64/sve.h index 71bddb41f19c..330c47ea8864 100644 --- a/xen/arch/arm/include/asm/arm64/sve.h +++ b/xen/arch/arm/include/asm/arm64/sve.h @@ -24,6 +24,8 @@ static inline unsigned int sve_encode_vl(unsigned int sve_vl_bits) return sve_vl_bits / SVE_VL_MULTIPLE_VAL; } +bool sve_domctl_vl_param(int val, unsigned int *out); + #ifdef CONFIG_ARM64_SVE extern int opt_dom0_sve; @@ -37,7 +39,6 @@ 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 */ @@ -68,11 +69,6 @@ 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; -} - #endif /* CONFIG_ARM64_SVE */ #endif /* _ARM_ARM64_SVE_H */ > >>>> --- 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
On 24.04.2023 17:18, Luca Fancellu wrote: >> On 24 Apr 2023, at 16:06, Jan Beulich <jbeulich@suse.com> wrote: >> On 24.04.2023 16:57, Luca Fancellu wrote: >>>> On 24 Apr 2023, at 15:05, Jan Beulich <jbeulich@suse.com> wrote: >>>> On 24.04.2023 16:00, Luca Fancellu wrote: >>>>>> On 24 Apr 2023, at 12:34, Jan Beulich <jbeulich@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? >> >> 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?" > > Oh ok, I don’t know, here what I get if for example I build arm32: > > arm-linux-gnueabihf-ld -EL -T arch/arm/xen.lds -N prelink.o \ > ./common/symbols-dummy.o -o ./.xen-syms.0 > arm-linux-gnueabihf-ld: prelink.o: in function `create_domUs': > (.init.text+0x13464): undefined reference to `sve_domctl_vl_param' In particular with seeing this: What you copied here is a build with the series applied only up to this patch? I ask because the patch here adds a call only out of create_dom0(). Jan > arm-linux-gnueabihf-ld: (.init.text+0x136b4): undefined reference to `sve_domctl_vl_param' > arm-linux-gnueabihf-ld: ./.xen-syms.0: hidden symbol `sve_domctl_vl_param' isn't defined > arm-linux-gnueabihf-ld: final link failed: bad value > make[3]: *** [/data_sdc/lucfan01/kirkstone_xen/xen/xen/arch/arm/Makefile:95: xen-syms] Error 1 > make[2]: *** [/data_sdc/lucfan01/kirkstone_xen/xen/xen/./build.mk:90: xen] Error 2 > make[1]: *** [/data_sdc/lucfan01/kirkstone_xen/xen/xen/Makefile:590: xen] Error 2 > make[1]: Leaving directory '/data_sdc/lucfan01/kirkstone_xen/build/xen-qemu-arm32' > make: *** [Makefile:181: __sub-make] Error 2 > make: Leaving directory '/data_sdc/lucfan01/kirkstone_xen/xen/xen’ > > These are the modification I’ve done: > > diff --git a/xen/arch/arm/include/asm/arm64/sve.h b/xen/arch/arm/include/asm/arm64/sve.h > index 71bddb41f19c..330c47ea8864 100644 > --- a/xen/arch/arm/include/asm/arm64/sve.h > +++ b/xen/arch/arm/include/asm/arm64/sve.h > @@ -24,6 +24,8 @@ static inline unsigned int sve_encode_vl(unsigned int sve_vl_bits) > return sve_vl_bits / SVE_VL_MULTIPLE_VAL; > } > > +bool sve_domctl_vl_param(int val, unsigned int *out); > + > #ifdef CONFIG_ARM64_SVE > > extern int opt_dom0_sve; > @@ -37,7 +39,6 @@ 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 */ > > @@ -68,11 +69,6 @@ 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; > -} > - > #endif /* CONFIG_ARM64_SVE */ > > #endif /* _ARM_ARM64_SVE_H */
> On 24 Apr 2023, at 16:25, Jan Beulich <jbeulich@suse.com> wrote: > > On 24.04.2023 17:18, Luca Fancellu wrote: >>> On 24 Apr 2023, at 16:06, Jan Beulich <jbeulich@suse.com> wrote: >>> On 24.04.2023 16:57, Luca Fancellu wrote: >>>>> On 24 Apr 2023, at 15:05, Jan Beulich <jbeulich@suse.com> wrote: >>>>> On 24.04.2023 16:00, Luca Fancellu wrote: >>>>>>> On 24 Apr 2023, at 12:34, Jan Beulich <jbeulich@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? >>> >>> 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?" >> >> Oh ok, I don’t know, here what I get if for example I build arm32: >> >> arm-linux-gnueabihf-ld -EL -T arch/arm/xen.lds -N prelink.o \ >> ./common/symbols-dummy.o -o ./.xen-syms.0 >> arm-linux-gnueabihf-ld: prelink.o: in function `create_domUs': >> (.init.text+0x13464): undefined reference to `sve_domctl_vl_param' > > In particular with seeing this: What you copied here is a build with the > series applied only up to this patch? I ask because the patch here adds a > call only out of create_dom0(). No I’ve do the changes on top of the serie, I’ve tried it now, only to this patch and it builds correctly, It was my mistake to don’t read carefully the error output. Anyway I guess this change is not applicable because we don’t have a symbol that is plain 0 for domUs to be placed inside create_domUs. > > Jan > >> arm-linux-gnueabihf-ld: (.init.text+0x136b4): undefined reference to `sve_domctl_vl_param' >> arm-linux-gnueabihf-ld: ./.xen-syms.0: hidden symbol `sve_domctl_vl_param' isn't defined >> arm-linux-gnueabihf-ld: final link failed: bad value >> make[3]: *** [/data_sdc/lucfan01/kirkstone_xen/xen/xen/arch/arm/Makefile:95: xen-syms] Error 1 >> make[2]: *** [/data_sdc/lucfan01/kirkstone_xen/xen/xen/./build.mk:90: xen] Error 2 >> make[1]: *** [/data_sdc/lucfan01/kirkstone_xen/xen/xen/Makefile:590: xen] Error 2 >> make[1]: Leaving directory '/data_sdc/lucfan01/kirkstone_xen/build/xen-qemu-arm32' >> make: *** [Makefile:181: __sub-make] Error 2 >> make: Leaving directory '/data_sdc/lucfan01/kirkstone_xen/xen/xen’ >> >> These are the modification I’ve done: >> >> diff --git a/xen/arch/arm/include/asm/arm64/sve.h b/xen/arch/arm/include/asm/arm64/sve.h >> index 71bddb41f19c..330c47ea8864 100644 >> --- a/xen/arch/arm/include/asm/arm64/sve.h >> +++ b/xen/arch/arm/include/asm/arm64/sve.h >> @@ -24,6 +24,8 @@ static inline unsigned int sve_encode_vl(unsigned int sve_vl_bits) >> return sve_vl_bits / SVE_VL_MULTIPLE_VAL; >> } >> >> +bool sve_domctl_vl_param(int val, unsigned int *out); >> + >> #ifdef CONFIG_ARM64_SVE >> >> extern int opt_dom0_sve; >> @@ -37,7 +39,6 @@ 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 */ >> >> @@ -68,11 +69,6 @@ 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; >> -} >> - >> #endif /* CONFIG_ARM64_SVE */ >> >> #endif /* _ARM_ARM64_SVE_H */
On 24.04.2023 17:34, Luca Fancellu wrote: >> On 24 Apr 2023, at 16:25, Jan Beulich <jbeulich@suse.com> wrote: >> On 24.04.2023 17:18, Luca Fancellu wrote: >>>> On 24 Apr 2023, at 16:06, Jan Beulich <jbeulich@suse.com> wrote: >>>> On 24.04.2023 16:57, Luca Fancellu wrote: >>>>>> On 24 Apr 2023, at 15:05, Jan Beulich <jbeulich@suse.com> wrote: >>>>>> On 24.04.2023 16:00, Luca Fancellu wrote: >>>>>>>> On 24 Apr 2023, at 12:34, Jan Beulich <jbeulich@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? >>>> >>>> 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?" >>> >>> Oh ok, I don’t know, here what I get if for example I build arm32: >>> >>> arm-linux-gnueabihf-ld -EL -T arch/arm/xen.lds -N prelink.o \ >>> ./common/symbols-dummy.o -o ./.xen-syms.0 >>> arm-linux-gnueabihf-ld: prelink.o: in function `create_domUs': >>> (.init.text+0x13464): undefined reference to `sve_domctl_vl_param' >> >> In particular with seeing this: What you copied here is a build with the >> series applied only up to this patch? I ask because the patch here adds a >> call only out of create_dom0(). > > No I’ve do the changes on top of the serie, I’ve tried it now, only to this patch and it builds correctly, > It was my mistake to don’t read carefully the error output. > > Anyway I guess this change is not applicable because we don’t have a symbol that is plain 0 for domUs > to be placed inside create_domUs. Possible, but would you mind first telling me in which other patch(es) the further reference(s) are being introduced, so I could take a look without (again) digging through the entire series? Jan
> On 24 Apr 2023, at 16:41, Jan Beulich <jbeulich@suse.com> wrote: > > On 24.04.2023 17:34, Luca Fancellu wrote: >>> On 24 Apr 2023, at 16:25, Jan Beulich <jbeulich@suse.com> wrote: >>> On 24.04.2023 17:18, Luca Fancellu wrote: >>>>> On 24 Apr 2023, at 16:06, Jan Beulich <jbeulich@suse.com> wrote: >>>>> On 24.04.2023 16:57, Luca Fancellu wrote: >>>>>>> On 24 Apr 2023, at 15:05, Jan Beulich <jbeulich@suse.com> wrote: >>>>>>> On 24.04.2023 16:00, Luca Fancellu wrote: >>>>>>>>> On 24 Apr 2023, at 12:34, Jan Beulich <jbeulich@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? >>>>> >>>>> 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?" >>>> >>>> Oh ok, I don’t know, here what I get if for example I build arm32: >>>> >>>> arm-linux-gnueabihf-ld -EL -T arch/arm/xen.lds -N prelink.o \ >>>> ./common/symbols-dummy.o -o ./.xen-syms.0 >>>> arm-linux-gnueabihf-ld: prelink.o: in function `create_domUs': >>>> (.init.text+0x13464): undefined reference to `sve_domctl_vl_param' >>> >>> In particular with seeing this: What you copied here is a build with the >>> series applied only up to this patch? I ask because the patch here adds a >>> call only out of create_dom0(). >> >> No I’ve do the changes on top of the serie, I’ve tried it now, only to this patch and it builds correctly, >> It was my mistake to don’t read carefully the error output. >> >> Anyway I guess this change is not applicable because we don’t have a symbol that is plain 0 for domUs >> to be placed inside create_domUs. > > Possible, but would you mind first telling me in which other patch(es) the > further reference(s) are being introduced, so I could take a look without > (again) digging through the entire series? Sure, the other references to the function are introduced in "xen/arm: add sve property for dom0less domUs” patch 11 > > Jan
On 24.04.2023 17:43, Luca Fancellu wrote: >> On 24 Apr 2023, at 16:41, Jan Beulich <jbeulich@suse.com> wrote: >> On 24.04.2023 17:34, Luca Fancellu wrote: >>>> On 24 Apr 2023, at 16:25, Jan Beulich <jbeulich@suse.com> wrote: >>>> On 24.04.2023 17:18, Luca Fancellu wrote: >>>>> Oh ok, I don’t know, here what I get if for example I build arm32: >>>>> >>>>> arm-linux-gnueabihf-ld -EL -T arch/arm/xen.lds -N prelink.o \ >>>>> ./common/symbols-dummy.o -o ./.xen-syms.0 >>>>> arm-linux-gnueabihf-ld: prelink.o: in function `create_domUs': >>>>> (.init.text+0x13464): undefined reference to `sve_domctl_vl_param' >>>> >>>> In particular with seeing this: What you copied here is a build with the >>>> series applied only up to this patch? I ask because the patch here adds a >>>> call only out of create_dom0(). >>> >>> No I’ve do the changes on top of the serie, I’ve tried it now, only to this patch and it builds correctly, >>> It was my mistake to don’t read carefully the error output. >>> >>> Anyway I guess this change is not applicable because we don’t have a symbol that is plain 0 for domUs >>> to be placed inside create_domUs. >> >> Possible, but would you mind first telling me in which other patch(es) the >> further reference(s) are being introduced, so I could take a look without >> (again) digging through the entire series? > > Sure, the other references to the function are introduced in "xen/arm: add sve property for dom0less domUs” patch 11 Personally I'm inclined to suggest adding "#ifdef CONFIG_ARM64_SVE" there. But I guess that may again go against your desire to not ignore inapplicable options. Still I can't resist to at least ask how an "sve" node on Arm32 is different from an entirely unknown one. Jan
> On 24 Apr 2023, at 17:10, Jan Beulich <jbeulich@suse.com> wrote: > > On 24.04.2023 17:43, Luca Fancellu wrote: >>> On 24 Apr 2023, at 16:41, Jan Beulich <jbeulich@suse.com> wrote: >>> On 24.04.2023 17:34, Luca Fancellu wrote: >>>>> On 24 Apr 2023, at 16:25, Jan Beulich <jbeulich@suse.com> wrote: >>>>> On 24.04.2023 17:18, Luca Fancellu wrote: >>>>>> Oh ok, I don’t know, here what I get if for example I build arm32: >>>>>> >>>>>> arm-linux-gnueabihf-ld -EL -T arch/arm/xen.lds -N prelink.o \ >>>>>> ./common/symbols-dummy.o -o ./.xen-syms.0 >>>>>> arm-linux-gnueabihf-ld: prelink.o: in function `create_domUs': >>>>>> (.init.text+0x13464): undefined reference to `sve_domctl_vl_param' >>>>> >>>>> In particular with seeing this: What you copied here is a build with the >>>>> series applied only up to this patch? I ask because the patch here adds a >>>>> call only out of create_dom0(). >>>> >>>> No I’ve do the changes on top of the serie, I’ve tried it now, only to this patch and it builds correctly, >>>> It was my mistake to don’t read carefully the error output. >>>> >>>> Anyway I guess this change is not applicable because we don’t have a symbol that is plain 0 for domUs >>>> to be placed inside create_domUs. >>> >>> Possible, but would you mind first telling me in which other patch(es) the >>> further reference(s) are being introduced, so I could take a look without >>> (again) digging through the entire series? >> >> Sure, the other references to the function are introduced in "xen/arm: add sve property for dom0less domUs” patch 11 > > Personally I'm inclined to suggest adding "#ifdef CONFIG_ARM64_SVE" there. > But I guess that may again go against your desire to not ignore inapplicable > options. Still I can't resist to at least ask how an "sve" node on Arm32 is > different from an entirely unknown one. It would be ok for me to use #ifdef CONFIG_ARM64_SVE and fail in the #else branch, but I had the feeling in the past that Arm maintainers are not very happy with #ifdefs, I might be wrong so I’ll wait for them to give an opinion and then I will be happy to follow. > > Jan
Hi Luca, Sorry for the late reply. On 25/04/2023 07:04, Luca Fancellu wrote: > > >> On 24 Apr 2023, at 17:10, Jan Beulich <jbeulich@suse.com> wrote: >> >> On 24.04.2023 17:43, Luca Fancellu wrote: >>>> On 24 Apr 2023, at 16:41, Jan Beulich <jbeulich@suse.com> wrote: >>>> On 24.04.2023 17:34, Luca Fancellu wrote: >>>>>> On 24 Apr 2023, at 16:25, Jan Beulich <jbeulich@suse.com> wrote: >>>>>> On 24.04.2023 17:18, Luca Fancellu wrote: >>>>>>> Oh ok, I don’t know, here what I get if for example I build arm32: >>>>>>> >>>>>>> arm-linux-gnueabihf-ld -EL -T arch/arm/xen.lds -N prelink.o \ >>>>>>> ./common/symbols-dummy.o -o ./.xen-syms.0 >>>>>>> arm-linux-gnueabihf-ld: prelink.o: in function `create_domUs': >>>>>>> (.init.text+0x13464): undefined reference to `sve_domctl_vl_param' >>>>>> >>>>>> In particular with seeing this: What you copied here is a build with the >>>>>> series applied only up to this patch? I ask because the patch here adds a >>>>>> call only out of create_dom0(). >>>>> >>>>> No I’ve do the changes on top of the serie, I’ve tried it now, only to this patch and it builds correctly, >>>>> It was my mistake to don’t read carefully the error output. >>>>> >>>>> Anyway I guess this change is not applicable because we don’t have a symbol that is plain 0 for domUs >>>>> to be placed inside create_domUs. >>>> >>>> Possible, but would you mind first telling me in which other patch(es) the >>>> further reference(s) are being introduced, so I could take a look without >>>> (again) digging through the entire series? >>> >>> Sure, the other references to the function are introduced in "xen/arm: add sve property for dom0less domUs” patch 11 >> >> Personally I'm inclined to suggest adding "#ifdef CONFIG_ARM64_SVE" there. >> But I guess that may again go against your desire to not ignore inapplicable >> options. Still I can't resist to at least ask how an "sve" node on Arm32 is >> different from an entirely unknown one. > > It would be ok for me to use #ifdef CONFIG_ARM64_SVE and fail in the #else branch, > but I had the feeling in the past that Arm maintainers are not very happy with #ifdefs, I might > be wrong so I’ll wait for them to give an opinion and then I will be happy to follow. IIRC, your suggestion is for patch #11. In this case, my preference is the #ifdef + throwing an error in the #else branch. This would avoid to silently ignore the property if SVE is not enabled (both Bertrand and I agreed this should not be ignored, see [1]). Cheers, [1] https://lore.kernel.org/all/7614AE25-F59D-430A-9C3E-30B1CE0E1580@arm.com/
diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc index e0b89b7d3319..47e5b4eb6199 100644 --- a/docs/misc/xen-command-line.pandoc +++ b/docs/misc/xen-command-line.pandoc @@ -777,9 +777,9 @@ Specify the bit width of the DMA heap. ### dom0 = List of [ pv | pvh, shadow=<bool>, verbose=<bool>, - cpuid-faulting=<bool>, msr-relaxed=<bool> ] + cpuid-faulting=<bool>, msr-relaxed=<bool> ] (x86) - Applicability: x86 + = List of [ sve=<integer> ] (Arm) Controls for how dom0 is constructed on x86 systems. @@ -838,6 +838,22 @@ Controls for how dom0 is constructed on x86 systems. If using this option is necessary to fix an issue, please report a bug. +Enables features on dom0 on Arm systems. + +* The `sve` integer parameter enables Arm SVE usage for Dom0 domain and sets + the maximum SVE vector length, the option is applicable only to AArch64 + guests. + A value equal to 0 disables the feature, this is the default value. + Values below 0 means the feature uses the maximum SVE vector length + supported by hardware, if SVE is supported. + Values above 0 explicitly set the maximum SVE vector length for Dom0, + allowed values are from 128 to maximum 2048, being multiple of 128. + Please note that when the user explicitly specifies the value, if that value + is above the hardware supported maximum SVE vector length, the domain + creation will fail and the system will stop, the same will occur if the + option is provided with a non zero value, but the platform doesn't support + SVE. + ### dom0-cpuid = List of comma separated booleans diff --git a/xen/arch/arm/arm64/sve.c b/xen/arch/arm/arm64/sve.c index 064832b450ff..4d964f2b56b4 100644 --- a/xen/arch/arm/arm64/sve.c +++ b/xen/arch/arm/arm64/sve.c @@ -14,6 +14,9 @@ #include <asm/processor.h> #include <asm/system.h> +/* opt_dom0_sve: allow Dom0 to use SVE and set maximum vector length. */ +int __initdata opt_dom0_sve; + extern unsigned int sve_get_hw_vl(void); extern void sve_save_ctx(uint64_t *sve_ctx, uint64_t *pregs, int save_ffr); extern void sve_load_ctx(uint64_t const *sve_ctx, uint64_t const *pregs, @@ -123,3 +126,20 @@ void sve_restore_state(struct vcpu *v) sve_load_ctx(v->arch.vfp.sve_zreg_ctx_end, v->arch.vfp.fpregs, 1); } + +bool __init sve_domctl_vl_param(int val, unsigned int *out) +{ + /* + * Negative SVE parameter value means to use the maximum supported + * vector length, otherwise if a positive value is provided, check if the + * vector length is a multiple of 128 + */ + if ( val < 0 ) + *out = get_sys_vl_len(); + else if ( (val % SVE_VL_MULTIPLE_VAL) == 0 ) + *out = val; + else + return false; + + return true; +} diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index d9450416f665..4a6b73348594 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -62,6 +62,21 @@ custom_param("dom0_mem", parse_dom0_mem); int __init parse_arch_dom0_param(const char *s, const char *e) { + long long val; + + if ( !parse_signed_integer("sve", s, e, &val) ) + { +#ifdef CONFIG_ARM64_SVE + if ( (val >= INT_MIN) && (val <= INT_MAX) ) + opt_dom0_sve = val; + else + printk(XENLOG_INFO "'sve=%lld' value out of range!\n", val); +#else + no_config_param("ARM64_SVE", "sve", s, e); +#endif + return 0; + } + return -EINVAL; } @@ -4117,6 +4132,16 @@ void __init create_dom0(void) if ( iommu_enabled ) dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu; + if ( opt_dom0_sve ) + { + unsigned int vl; + + if ( sve_domctl_vl_param(opt_dom0_sve, &vl) ) + dom0_cfg.arch.sve_vl = sve_encode_vl(vl); + else + panic("SVE vector length error\n"); + } + dom0 = domain_create(0, &dom0_cfg, CDF_privileged | CDF_directmap); if ( IS_ERR(dom0) ) panic("Error creating domain 0 (rc = %ld)\n", PTR_ERR(dom0)); diff --git a/xen/arch/arm/include/asm/arm64/sve.h b/xen/arch/arm/include/asm/arm64/sve.h index 582405dfdf6a..71bddb41f19c 100644 --- a/xen/arch/arm/include/asm/arm64/sve.h +++ b/xen/arch/arm/include/asm/arm64/sve.h @@ -19,8 +19,15 @@ static inline unsigned int sve_decode_vl(unsigned int sve_vl) return sve_vl * SVE_VL_MULTIPLE_VAL; } +static inline unsigned int sve_encode_vl(unsigned int sve_vl_bits) +{ + return sve_vl_bits / SVE_VL_MULTIPLE_VAL; +} + #ifdef CONFIG_ARM64_SVE +extern int opt_dom0_sve; + #define is_sve_domain(d) ((d)->arch.sve_vl > 0) register_t compute_max_zcr(void); @@ -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; +} + #endif /* CONFIG_ARM64_SVE */ #endif /* _ARM_ARM64_SVE_H */ diff --git a/xen/common/kernel.c b/xen/common/kernel.c index f7b1f65f373c..b67d9056fec3 100644 --- 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); + 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; + + *val = pval; + + return 0; +} + int cmdline_strcmp(const char *frag, const char *name) { for ( ; ; frag++, name++ ) diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h index e914ccade095..5343ee7a944a 100644 --- a/xen/include/xen/lib.h +++ b/xen/include/xen/lib.h @@ -94,6 +94,16 @@ int parse_bool(const char *s, const char *e); */ int parse_boolean(const char *name, const char *s, const char *e); +/** + * Given a specific name, parses a string of the form: + * $NAME=<integer number> + * returning 0 and a value in val, for a recognised integer. + * Returns -1 for name not found, general errors, or -2 if name is found but + * not recognised number. + */ +int parse_signed_integer(const char *name, const char *s, const char *e, + long long *val); + /** * Very similar to strcmp(), but will declare a match if the NUL in 'name' * lines up with comma, colon, semicolon or equals in 'frag'. Designed for
Add a command line parameter to allow Dom0 the use of SVE resources, the command line parameter sve=<integer>, sub argument of dom0=, controls the feature on this domain and sets the maximum SVE vector length for Dom0. Add a new function, parse_signed_integer(), to parse an integer command line argument. Signed-off-by: Luca Fancellu <luca.fancellu@arm.com> --- Changes from v5: - stop the domain if VL error occurs (Julien, Bertrand) - update the documentation - Rename sve_sanitize_vl_param to sve_domctl_vl_param to mark the fact that we are sanitizing a parameter coming from the user before encoding it into sve_vl in domctl structure. (suggestion from Bertrand in a separate discussion) - update comment in parse_signed_integer, return boolean in sve_domctl_vl_param (Jan). Changes from v4: - Negative values as user param means max supported HW VL (Jan) - update documentation, make use of no_config_param(), rename parse_integer into parse_signed_integer and take long long *, also put a comment on the -2 return condition, update declaration comment to reflect the modifications (Jan) Changes from v3: - Don't use fixed len types when not needed (Jan) - renamed domainconfig_encode_vl to sve_encode_vl - Use a sub argument of dom0= to enable the feature (Jan) - Add parse_integer() function Changes from v2: - xen_domctl_createdomain field has changed into sve_vl and its value now is the VL / 128, create an helper function for that. Changes from v1: - No changes Changes from RFC: - Changed docs to explain that the domain won't be created if the requested vector length is above the supported one from the platform. --- docs/misc/xen-command-line.pandoc | 20 ++++++++++++++++++-- xen/arch/arm/arm64/sve.c | 20 ++++++++++++++++++++ xen/arch/arm/domain_build.c | 25 +++++++++++++++++++++++++ xen/arch/arm/include/asm/arm64/sve.h | 14 ++++++++++++++ xen/common/kernel.c | 25 +++++++++++++++++++++++++ xen/include/xen/lib.h | 10 ++++++++++ 6 files changed, 112 insertions(+), 2 deletions(-)