[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v6 07/12] xen: enable Dom0 to use SVE feature
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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |