Hi Jan, Sorry for the late reply, I’ve missed this one,
> On 17 Apr 2023, at 10:41, Jan Beulich <jbeul...@suse.com> wrote: > > On 12.04.2023 11:49, Luca Fancellu wrote: >> @@ -118,3 +121,21 @@ void sve_restore_state(struct vcpu *v) >> >> sve_load_ctx(sve_ctx_zreg_end, v->arch.vfp.fpregs, 1); >> } >> + >> +int __init sve_sanitize_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 and not bigger than the maximum >> value >> + * 2048 >> + */ >> + if ( val < 0 ) >> + *out = get_sys_vl_len(); >> + else if ( ((val % SVE_VL_MULTIPLE_VAL) == 0) && (val <= >> SVE_VL_MAX_BITS) ) >> + *out = val; >> + else >> + return -1; >> + >> + return 0; >> +} > > I think such a function wants to either return boolean, or -E... in the > error case. Boolean would ... > >> @@ -4109,6 +4125,17 @@ void __init create_dom0(void) >> if ( iommu_enabled ) >> dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu; >> >> + if ( opt_dom0_sve ) >> + { >> + unsigned int vl; >> + >> + if ( !sve_sanitize_vl_param(opt_dom0_sve, &vl) ) > > ... yield a slightly better call site here, imo. Ok I’ll chose one of the two, depending on the discussion with Bertrand about the parameter checking > >> + dom0_cfg.arch.sve_vl = sve_encode_vl(vl); >> + else >> + printk(XENLOG_WARNING >> + "SVE vector length error, disable feature for Dom0\n"); > > I appreciate the now better behavior here, but I think the respective part > of the doc is now stale? > >> @@ -28,9 +35,12 @@ 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); >> +int sve_sanitize_vl_param(int val, unsigned int *out); >> >> #else /* !CONFIG_ARM64_SVE */ >> >> +#define opt_dom0_sve (0) > > With this I don't think you need ... > >> @@ -55,6 +65,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 int sve_sanitize_vl_param(int val, unsigned int *out) >> +{ >> + return -1; >> +} > > ... such a stub function; having the declaration visible should be > enough for things to build (thanks to DCE, which we use for similar > purposes on many other places). Ok I’ll try this approach and I’ll change if I don’t find any issue, thanks for suggesting that > >> --- 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; > > What use is this extra variable? I’m using pval to avoid using *val in the case we find that the parsed number is not good, I think it’s better to don’t change the *val if any error come out, what do you think? > >> + slen = e ? ({ ASSERT(e >= s); e - s; }) : strlen(s); >> + nlen = strlen(name); >> + >> + /* Does s start with name or contains only the name? */ >> + if ( (slen <= nlen) || strncmp(s, name, nlen) || (s[nlen] != '=') ) >> + return -1; > > The comment imo wants wording consistently positive or consistently > negative. IOW either you say what you're looking for, or you say > what you're meaning to reject. Ok I’ll rephrase to: /* Check that this is the name we are looking for and the syntax is right */ Is that better? > >> + pval = simple_strtoll(&s[nlen + 1], &str, 0); > > I wonder whether, when potentially expecting negative numbers, > accepting other than decimal numbers here is useful. You are right, I’ll change to 10 base > > Jan