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
