Re: [PATCH v5 07/12] xen: enable Dom0 to use SVE feature

[Luca Fancellu <Luca.Fancellu@arm.com>]

Hi Jan,

Sorry for the late reply, I’ve missed this one,

> On 17 Apr 2023, at 10:41, Jan Beulich <jbeulich@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