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.
> + 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).
> --- 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?
> + 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.
> + pval = simple_strtoll(&s[nlen + 1], &str, 0);
I wonder whether, when potentially expecting negative numbers,
accepting other than decimal numbers here is useful.
Jan
