| Message ID | 20230412094938.2693890-8-luca.fancellu@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | SVE feature for arm guests | expand |
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
Hi Luca, > On 12 Apr 2023, at 11:49, Luca Fancellu <Luca.Fancellu@arm.com> wrote: > > Add a command line parameter to allow Dom0 the use of SVE resources, > the command line parameter sve=<integer>, sub argument of dom0=, > controls the feature on this domain and sets the maximum SVE vector > length for Dom0. > > Add a new function, parse_signed_integer(), to parse an integer > command line argument. > > Signed-off-by: Luca Fancellu <luca.fancellu@arm.com> > --- > Changes from v4: > - Negative values as user param means max supported HW VL (Jan) > - update documentation, make use of no_config_param(), rename > parse_integer into parse_signed_integer and take long long *, > also put a comment on the -2 return condition, update > declaration comment to reflect the modifications (Jan) > Changes from v3: > - Don't use fixed len types when not needed (Jan) > - renamed domainconfig_encode_vl to sve_encode_vl > - Use a sub argument of dom0= to enable the feature (Jan) > - Add parse_integer() function > Changes from v2: > - xen_domctl_createdomain field has changed into sve_vl and its > value now is the VL / 128, create an helper function for that. > Changes from v1: > - No changes > Changes from RFC: > - Changed docs to explain that the domain won't be created if the > requested vector length is above the supported one from the > platform. > --- > docs/misc/xen-command-line.pandoc | 18 ++++++++++++++++-- > xen/arch/arm/arm64/sve.c | 21 +++++++++++++++++++++ > xen/arch/arm/domain_build.c | 27 +++++++++++++++++++++++++++ > xen/arch/arm/include/asm/arm64/sve.h | 15 +++++++++++++++ > xen/common/kernel.c | 25 +++++++++++++++++++++++++ > xen/include/xen/lib.h | 10 ++++++++++ > 6 files changed, 114 insertions(+), 2 deletions(-) > > diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc > index e0b89b7d3319..9c0790ce6c7c 100644 > --- a/docs/misc/xen-command-line.pandoc > +++ b/docs/misc/xen-command-line.pandoc > @@ -777,9 +777,9 @@ Specify the bit width of the DMA heap. > > ### dom0 > = List of [ pv | pvh, shadow=<bool>, verbose=<bool>, > - cpuid-faulting=<bool>, msr-relaxed=<bool> ] > + cpuid-faulting=<bool>, msr-relaxed=<bool> ] (x86) > > - Applicability: x86 > + = List of [ sve=<integer> ] (Arm) > > Controls for how dom0 is constructed on x86 systems. > > @@ -838,6 +838,20 @@ Controls for how dom0 is constructed on x86 systems. > > If using this option is necessary to fix an issue, please report a bug. > > +Enables features on dom0 on Arm systems. > + > +* The `sve` integer parameter enables Arm SVE usage for Dom0 domain and sets > + the maximum SVE vector length. > + A value equal to 0 disables the feature, this is the default value. > + Values below 0 means the feature uses the maximum SVE vector length > + supported by hardware, please be aware that if the hardware doesn't supports > + SVE, the feature remains disabled. > + Values above 0 explicitly set the maximum SVE vector length for Dom0, > + allowed values are from 128 to maximum 2048, being multiple of 128. > + Please note that when the user explicitly specify the value, if that value > + is above the hardware supported maximum SVE vector length, the domain > + creation will fail and the system will stop. > + > ### dom0-cpuid > = List of comma separated booleans > > diff --git a/xen/arch/arm/arm64/sve.c b/xen/arch/arm/arm64/sve.c > index 5485648850a0..ad5db62e1805 100644 > --- a/xen/arch/arm/arm64/sve.c > +++ b/xen/arch/arm/arm64/sve.c > @@ -9,6 +9,9 @@ > #include <xen/sizes.h> > #include <asm/arm64/sve.h> > > +/* opt_dom0_sve: allow Dom0 to use SVE and set maximum vector length. */ > +int __initdata opt_dom0_sve; > + > extern unsigned int sve_get_hw_vl(void); > extern void sve_save_ctx(uint64_t *sve_ctx, uint64_t *pregs, int save_ffr); > extern void sve_load_ctx(uint64_t const *sve_ctx, uint64_t const *pregs, > @@ -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; Shouldn't you also check if it is not greater than the maximum vector length ? > + else > + return -1; > + > + return 0; > +} > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > index eeb4662f0eee..3f30ef5c37b6 100644 > --- a/xen/arch/arm/domain_build.c > +++ b/xen/arch/arm/domain_build.c > @@ -26,6 +26,7 @@ > #include <asm/platform.h> > #include <asm/psci.h> > #include <asm/setup.h> > +#include <asm/arm64/sve.h> > #include <asm/cpufeature.h> > #include <asm/domain_build.h> > #include <xen/event.h> > @@ -61,6 +62,21 @@ custom_param("dom0_mem", parse_dom0_mem); > > int __init parse_arch_dom0_param(const char *s, const char *e) > { > + long long val; > + > + if ( !parse_signed_integer("sve", s, e, &val) ) > + { > +#ifdef CONFIG_ARM64_SVE > + if ( (val >= INT_MIN) && (val <= INT_MAX) ) > + opt_dom0_sve = val; > + else > + printk(XENLOG_INFO "'sve=%lld' value out of range!\n", val); > +#else > + no_config_param("ARM64_SVE", "sve", s, e); > +#endif Correct me if my understanding is wrong but here you just ignore the sve parameter if SVE is not supported by Xen ? I am a bit wondering if we should not just refuse it here as the user might wrongly think that his parameter had some effect. Or is it a usual way to handle this case ? > + return 0; > + } > + > return -EINVAL; > } > > @@ -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) ) > + dom0_cfg.arch.sve_vl = sve_encode_vl(vl); > + else > + printk(XENLOG_WARNING > + "SVE vector length error, disable feature for Dom0\n"); > + } > + > dom0 = domain_create(0, &dom0_cfg, CDF_privileged | CDF_directmap); > if ( IS_ERR(dom0) ) > panic("Error creating domain 0 (rc = %ld)\n", PTR_ERR(dom0)); > diff --git a/xen/arch/arm/include/asm/arm64/sve.h b/xen/arch/arm/include/asm/arm64/sve.h > index fc162c9d2cf7..f1801876b5de 100644 > --- a/xen/arch/arm/include/asm/arm64/sve.h > +++ b/xen/arch/arm/include/asm/arm64/sve.h > @@ -19,8 +19,15 @@ static inline unsigned int sve_decode_vl(unsigned int sve_vl) > return sve_vl * SVE_VL_MULTIPLE_VAL; > } > > +static inline unsigned int sve_encode_vl(unsigned int sve_vl_bits) > +{ > + return sve_vl_bits / SVE_VL_MULTIPLE_VAL; > +} > + > #ifdef CONFIG_ARM64_SVE > > +extern int opt_dom0_sve; > + > register_t compute_max_zcr(void); > register_t vl_to_zcr(unsigned int vl); > unsigned int get_sys_vl_len(void); > @@ -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) > + > static inline register_t compute_max_zcr(void) > { > return 0; > @@ -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; > +} > + > #endif /* CONFIG_ARM64_SVE */ > > #endif /* _ARM_ARM64_SVE_H */ > diff --git a/xen/common/kernel.c b/xen/common/kernel.c > index f7b1f65f373c..29d05282c8bb 100644 > --- 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); > + nlen = strlen(name); > + > + /* Does s start with name or contains only the name? */ > + 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; > + > + *val = pval; > + > + return 0; > +} > + > int cmdline_strcmp(const char *frag, const char *name) > { > for ( ; ; frag++, name++ ) > diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h > index e914ccade095..5343ee7a944a 100644 > --- a/xen/include/xen/lib.h > +++ b/xen/include/xen/lib.h > @@ -94,6 +94,16 @@ int parse_bool(const char *s, const char *e); > */ > int parse_boolean(const char *name, const char *s, const char *e); > > +/** > + * Given a specific name, parses a string of the form: > + * $NAME=<integer number> > + * returning 0 and a value in val, for a recognised integer. > + * Returns -1 for name not found, general errors, or -2 if name is found but > + * not recognised number. > + */ > +int parse_signed_integer(const char *name, const char *s, const char *e, > + long long *val); > + > /** > * Very similar to strcmp(), but will declare a match if the NUL in 'name' > * lines up with comma, colon, semicolon or equals in 'frag'. Designed for > -- > 2.34.1 > Cheers Bertrand
On 18.04.2023 14:47, Bertrand Marquis wrote: >> On 12 Apr 2023, at 11:49, Luca Fancellu <Luca.Fancellu@arm.com> 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; > > Shouldn't you also check if it is not greater than the maximum vector length ? Perhaps not "also" but "instead", because the supported length shouldn't be larger than SVE_VL_MAX_BITS (or if there was a risk that it might be, that should be taken care of elsewhere, e.g. in get_sys_vl_len(), not here). >> @@ -61,6 +62,21 @@ custom_param("dom0_mem", parse_dom0_mem); >> >> int __init parse_arch_dom0_param(const char *s, const char *e) >> { >> + long long val; >> + >> + if ( !parse_signed_integer("sve", s, e, &val) ) >> + { >> +#ifdef CONFIG_ARM64_SVE >> + if ( (val >= INT_MIN) && (val <= INT_MAX) ) >> + opt_dom0_sve = val; >> + else >> + printk(XENLOG_INFO "'sve=%lld' value out of range!\n", val); >> +#else >> + no_config_param("ARM64_SVE", "sve", s, e); >> +#endif > > Correct me if my understanding is wrong but here you just ignore the sve > parameter if SVE is not supported by Xen ? > > I am a bit wondering if we should not just refuse it here as the user might > wrongly think that his parameter had some effect. > > Or is it a usual way to handle this case ? It is, or else we'd need to alter what no_config_param() does. Plus ignoring the argument when !ARM64_SVE is no different from passing the same argument to an older Xen version, or from passing any unknown one. Jan
Hi Bertrand, >> >> diff --git a/xen/arch/arm/arm64/sve.c b/xen/arch/arm/arm64/sve.c >> index 5485648850a0..ad5db62e1805 100644 >> --- a/xen/arch/arm/arm64/sve.c >> +++ b/xen/arch/arm/arm64/sve.c >> @@ -9,6 +9,9 @@ >> #include <xen/sizes.h> >> #include <asm/arm64/sve.h> >> >> +/* opt_dom0_sve: allow Dom0 to use SVE and set maximum vector length. */ >> +int __initdata opt_dom0_sve; >> + >> extern unsigned int sve_get_hw_vl(void); >> extern void sve_save_ctx(uint64_t *sve_ctx, uint64_t *pregs, int save_ffr); >> extern void sve_load_ctx(uint64_t const *sve_ctx, uint64_t const *pregs, >> @@ -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; > > Shouldn't you also check if it is not greater than the maximum vector length ? I don’t understand, I am checking that the value is below or equal to SVE_VL_MAX_BITS, If you mean if it should be checked also against the maximum supported length by the platform, I think this is not the right place, the check is already in arch_sanitise_domain_config(), introduced in patch #2 > >> + else >> + return -1; >> + >> + return 0; >> +} >> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c >> index eeb4662f0eee..3f30ef5c37b6 100644 >> --- a/xen/arch/arm/domain_build.c >> +++ b/xen/arch/arm/domain_build.c >> @@ -26,6 +26,7 @@ >> #include <asm/platform.h> >> #include <asm/psci.h> >> #include <asm/setup.h> >> +#include <asm/arm64/sve.h> >> #include <asm/cpufeature.h> >> #include <asm/domain_build.h> >> #include <xen/event.h> >> @@ -61,6 +62,21 @@ custom_param("dom0_mem", parse_dom0_mem); >> >> int __init parse_arch_dom0_param(const char *s, const char *e) >> { >> + long long val; >> + >> + if ( !parse_signed_integer("sve", s, e, &val) ) >> + { >> +#ifdef CONFIG_ARM64_SVE >> + if ( (val >= INT_MIN) && (val <= INT_MAX) ) >> + opt_dom0_sve = val; >> + else >> + printk(XENLOG_INFO "'sve=%lld' value out of range!\n", val); >> +#else >> + no_config_param("ARM64_SVE", "sve", s, e); >> +#endif > > Correct me if my understanding is wrong but here you just ignore the sve > parameter if SVE is not supported by Xen ? > > I am a bit wondering if we should not just refuse it here as the user might > wrongly think that his parameter had some effect. > > Or is it a usual way to handle this case ? Jan suggested this approach, as it should be the preferred way to handle the case, looking into the x86 code it seems so.
Hi Luca, > On 19 Apr 2023, at 09:15, Luca Fancellu <Luca.Fancellu@arm.com> wrote: > > Hi Bertrand, > >>> >>> diff --git a/xen/arch/arm/arm64/sve.c b/xen/arch/arm/arm64/sve.c >>> index 5485648850a0..ad5db62e1805 100644 >>> --- a/xen/arch/arm/arm64/sve.c >>> +++ b/xen/arch/arm/arm64/sve.c >>> @@ -9,6 +9,9 @@ >>> #include <xen/sizes.h> >>> #include <asm/arm64/sve.h> >>> >>> +/* opt_dom0_sve: allow Dom0 to use SVE and set maximum vector length. */ >>> +int __initdata opt_dom0_sve; >>> + >>> extern unsigned int sve_get_hw_vl(void); >>> extern void sve_save_ctx(uint64_t *sve_ctx, uint64_t *pregs, int save_ffr); >>> extern void sve_load_ctx(uint64_t const *sve_ctx, uint64_t const *pregs, >>> @@ -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; >> >> Shouldn't you also check if it is not greater than the maximum vector length ? > > I don’t understand, I am checking that the value is below or equal to SVE_VL_MAX_BITS, > If you mean if it should be checked also against the maximum supported length by the platform, > I think this is not the right place, the check is already in arch_sanitise_domain_config(), introduced > in patch #2 If this is not the right place to check it then why checking the rest here ? From a user or a developer point of view I would expect the validity of the input to be checked only in one place. If here is not the place for that it is ok but then i would check everything in arch_sanitise_domain_config (multiple, min and supported) instead of doing it partly in 2 places. > >> >>> + else >>> + return -1; >>> + >>> + return 0; >>> +} >>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c >>> index eeb4662f0eee..3f30ef5c37b6 100644 >>> --- a/xen/arch/arm/domain_build.c >>> +++ b/xen/arch/arm/domain_build.c >>> @@ -26,6 +26,7 @@ >>> #include <asm/platform.h> >>> #include <asm/psci.h> >>> #include <asm/setup.h> >>> +#include <asm/arm64/sve.h> >>> #include <asm/cpufeature.h> >>> #include <asm/domain_build.h> >>> #include <xen/event.h> >>> @@ -61,6 +62,21 @@ custom_param("dom0_mem", parse_dom0_mem); >>> >>> int __init parse_arch_dom0_param(const char *s, const char *e) >>> { >>> + long long val; >>> + >>> + if ( !parse_signed_integer("sve", s, e, &val) ) >>> + { >>> +#ifdef CONFIG_ARM64_SVE >>> + if ( (val >= INT_MIN) && (val <= INT_MAX) ) >>> + opt_dom0_sve = val; >>> + else >>> + printk(XENLOG_INFO "'sve=%lld' value out of range!\n", val); >>> +#else >>> + no_config_param("ARM64_SVE", "sve", s, e); >>> +#endif >> >> Correct me if my understanding is wrong but here you just ignore the sve >> parameter if SVE is not supported by Xen ? >> >> I am a bit wondering if we should not just refuse it here as the user might >> wrongly think that his parameter had some effect. >> >> Or is it a usual way to handle this case ? > > Jan suggested this approach, as it should be the preferred way to handle the case, > looking into the x86 code it seems so. > This is somehow going around the global discussion: is it really ok to ignore sve param if it is not supported. Let's have this discussion on the other thread instead. Cheers Bertrand
> On 19 Apr 2023, at 08:21, Bertrand Marquis <Bertrand.Marquis@arm.com> wrote: > > Hi Luca, > >> On 19 Apr 2023, at 09:15, Luca Fancellu <Luca.Fancellu@arm.com> wrote: >> >> Hi Bertrand, >> >>>> >>>> diff --git a/xen/arch/arm/arm64/sve.c b/xen/arch/arm/arm64/sve.c >>>> index 5485648850a0..ad5db62e1805 100644 >>>> --- a/xen/arch/arm/arm64/sve.c >>>> +++ b/xen/arch/arm/arm64/sve.c >>>> @@ -9,6 +9,9 @@ >>>> #include <xen/sizes.h> >>>> #include <asm/arm64/sve.h> >>>> >>>> +/* opt_dom0_sve: allow Dom0 to use SVE and set maximum vector length. */ >>>> +int __initdata opt_dom0_sve; >>>> + >>>> extern unsigned int sve_get_hw_vl(void); >>>> extern void sve_save_ctx(uint64_t *sve_ctx, uint64_t *pregs, int save_ffr); >>>> extern void sve_load_ctx(uint64_t const *sve_ctx, uint64_t const *pregs, >>>> @@ -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; >>> >>> Shouldn't you also check if it is not greater than the maximum vector length ? >> >> I don’t understand, I am checking that the value is below or equal to SVE_VL_MAX_BITS, >> If you mean if it should be checked also against the maximum supported length by the platform, >> I think this is not the right place, the check is already in arch_sanitise_domain_config(), introduced >> in patch #2 > > If this is not the right place to check it then why checking the rest here ? > > From a user or a developer point of view I would expect the validity of the input to be checked only > in one place. > If here is not the place for that it is ok but then i would check everything in arch_sanitise_domain_config > (multiple, min and supported) instead of doing it partly in 2 places. Ok, given the way we encoded the value in xen_domctl_createdomain structure, we have that the value takes very little space, but a small issue is that when we encode it, we are dividing it by 128, which is fine for user params that are multiple of 128, but it’s less fine if the user passes “129”. To overcome this issue we are checking the value when it is not already encoded. Now, thinking about it, the check "&& (val <= SVE_VL_MAX_BITS)” is not really needed, because even if the value is above, then in arch_sanitise_domain_config we will hit the top limit of the platform maximum VL. int arch_sanitise_domain_config(struct xen_domctl_createdomain *config) { unsigned int max_vcpus; unsigned int flags_required = (XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap); unsigned int flags_optional = (XEN_DOMCTL_CDF_iommu | XEN_DOMCTL_CDF_vpmu); unsigned int sve_vl_bits = sve_decode_vl(config->arch.sve_vl); if ( (config->flags & ~flags_optional) != flags_required ) { dprintk(XENLOG_INFO, "Unsupported configuration %#x\n", config->flags); return -EINVAL; } /* Check feature flags */ if ( sve_vl_bits > 0 ) { unsigned int zcr_max_bits = get_sys_vl_len(); if ( !zcr_max_bits ) { dprintk(XENLOG_INFO, "SVE is unsupported on this machine.\n"); return -EINVAL; } if ( sve_vl_bits > zcr_max_bits ) { dprintk(XENLOG_INFO, "Requested SVE vector length (%u) > supported length (%u)\n", sve_vl_bits, zcr_max_bits); return -EINVAL; } } [...] Now, I understand your point, we could check everything in sve_sanitize_vl_param(), but it would leave a problem for domains created by hypercalls if I am not wrong. What do you think?
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
On 20.04.2023 08:25, Luca Fancellu wrote: >> On 17 Apr 2023, at 10:41, Jan Beulich <jbeulich@suse.com> wrote: >> On 12.04.2023 11:49, Luca Fancellu wrote: >>> --- 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? Caller ought to check the return value before even considering to look at the value. Then again I can see how, if the address of a global variable was passed in, that global may be unduly affected. So I guess what you have is actually okay. >>> + 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? It is, thanks. Alternatively how about "... and a value was provided"? Jan
>>>>> +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; >>>> >>>> Shouldn't you also check if it is not greater than the maximum vector length ? >>> >>> I don’t understand, I am checking that the value is below or equal to SVE_VL_MAX_BITS, >>> If you mean if it should be checked also against the maximum supported length by the platform, >>> I think this is not the right place, the check is already in arch_sanitise_domain_config(), introduced >>> in patch #2 >> >> If this is not the right place to check it then why checking the rest here ? >> >> From a user or a developer point of view I would expect the validity of the input to be checked only >> in one place. >> If here is not the place for that it is ok but then i would check everything in arch_sanitise_domain_config >> (multiple, min and supported) instead of doing it partly in 2 places. > > Ok, given the way we encoded the value in xen_domctl_createdomain structure, we have that the value takes > very little space, but a small issue is that when we encode it, we are dividing it by 128, which is fine for user params > that are multiple of 128, but it’s less fine if the user passes “129”. > > To overcome this issue we are checking the value when it is not already encoded. Now, thinking about it, the check > "&& (val <= SVE_VL_MAX_BITS)” is not really needed, because even if the value is above, then in arch_sanitise_domain_config > we will hit the top limit of the platform maximum VL. > > int arch_sanitise_domain_config(struct xen_domctl_createdomain *config) > { > unsigned int max_vcpus; > unsigned int flags_required = (XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap); > unsigned int flags_optional = (XEN_DOMCTL_CDF_iommu | XEN_DOMCTL_CDF_vpmu); > unsigned int sve_vl_bits = sve_decode_vl(config->arch.sve_vl); > > if ( (config->flags & ~flags_optional) != flags_required ) > { > dprintk(XENLOG_INFO, "Unsupported configuration %#x\n", > config->flags); > return -EINVAL; > } > > /* Check feature flags */ > if ( sve_vl_bits > 0 ) > { > unsigned int zcr_max_bits = get_sys_vl_len(); > > if ( !zcr_max_bits ) > { > dprintk(XENLOG_INFO, "SVE is unsupported on this machine.\n"); > return -EINVAL; > } > > if ( sve_vl_bits > zcr_max_bits ) > { > dprintk(XENLOG_INFO, > "Requested SVE vector length (%u) > supported length (%u)\n", > sve_vl_bits, zcr_max_bits); > return -EINVAL; > } > } > [...] > > Now, I understand your point, we could check everything in sve_sanitize_vl_param(), but it would leave a problem > for domains created by hypercalls if I am not wrong. > > What do you think? I thought about that and another possibility is to store “sve_vl” as uint16_t inside struct xen_arch_domainconfig, and check it inside arch_sanitise_domain_config() for it to be mod 128 and less than the max supported VL, this will allow to have all the checks in one place, taking a bit more space, anyway we would take the space from the implicit padding as this is the current status: struct arch_domain { enum domain_type type; /* 0 4 */ uint8_t sve_vl; /* 4 1 */ /* XXX 3 bytes hole, try to pack */ struct p2m_domain p2m; /* 8 328 */ /* --- cacheline 5 boundary (320 bytes) was 16 bytes ago --- */ struct hvm_domain hvm; /* 336 312 */ /* --- cacheline 10 boundary (640 bytes) was 8 bytes ago --- */ struct paging_domain paging; /* 648 32 */ struct vmmio vmmio; /* 680 32 */ /* --- cacheline 11 boundary (704 bytes) was 8 bytes ago --- */ unsigned int rel_priv; /* 712 4 */ /* XXX 4 bytes hole, try to pack */ struct { uint64_t offset; /* 720 8 */ s_time_t nanoseconds; /* 728 8 */ } virt_timer_base; /* 720 16 */ struct vgic_dist vgic; /* 736 200 */ /* XXX last struct has 2 bytes of padding */ /* --- cacheline 14 boundary (896 bytes) was 40 bytes ago --- */ struct vuart vuart; /* 936 32 */ /* --- cacheline 15 boundary (960 bytes) was 8 bytes ago --- */ unsigned int evtchn_irq; /* 968 4 */ struct { uint8_t privileged_call_enabled:1; /* 972: 0 1 */ } monitor; /* 972 1 */ /* XXX 3 bytes hole, try to pack */ struct vpl011 vpl011; /* 976 72 */ /* size: 1152, cachelines: 18, members: 13 */ /* sum members: 1038, holes: 3, sum holes: 10 */ /* padding: 104 */ /* paddings: 1, sum paddings: 2 */ } __attribute__((__aligned__(128)));
Hi Luca, > On 20 Apr 2023, at 10:46, Luca Fancellu <Luca.Fancellu@arm.com> wrote: > >>>>>> >>>>>> +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; >>>>> >>>>> Shouldn't you also check if it is not greater than the maximum vector length ? >>>> >>>> I don’t understand, I am checking that the value is below or equal to SVE_VL_MAX_BITS, >>>> If you mean if it should be checked also against the maximum supported length by the platform, >>>> I think this is not the right place, the check is already in arch_sanitise_domain_config(), introduced >>>> in patch #2 >>> >>> If this is not the right place to check it then why checking the rest here ? >>> >>> From a user or a developer point of view I would expect the validity of the input to be checked only >>> in one place. >>> If here is not the place for that it is ok but then i would check everything in arch_sanitise_domain_config >>> (multiple, min and supported) instead of doing it partly in 2 places. >> >> Ok, given the way we encoded the value in xen_domctl_createdomain structure, we have that the value takes >> very little space, but a small issue is that when we encode it, we are dividing it by 128, which is fine for user params >> that are multiple of 128, but it’s less fine if the user passes “129”. >> >> To overcome this issue we are checking the value when it is not already encoded. Now, thinking about it, the check >> "&& (val <= SVE_VL_MAX_BITS)” is not really needed, because even if the value is above, then in arch_sanitise_domain_config >> we will hit the top limit of the platform maximum VL. >> >> int arch_sanitise_domain_config(struct xen_domctl_createdomain *config) >> { >> unsigned int max_vcpus; >> unsigned int flags_required = (XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap); >> unsigned int flags_optional = (XEN_DOMCTL_CDF_iommu | XEN_DOMCTL_CDF_vpmu); >> unsigned int sve_vl_bits = sve_decode_vl(config->arch.sve_vl); >> >> if ( (config->flags & ~flags_optional) != flags_required ) >> { >> dprintk(XENLOG_INFO, "Unsupported configuration %#x\n", >> config->flags); >> return -EINVAL; >> } >> >> /* Check feature flags */ >> if ( sve_vl_bits > 0 ) >> { >> unsigned int zcr_max_bits = get_sys_vl_len(); >> >> if ( !zcr_max_bits ) >> { >> dprintk(XENLOG_INFO, "SVE is unsupported on this machine.\n"); >> return -EINVAL; >> } >> >> if ( sve_vl_bits > zcr_max_bits ) >> { >> dprintk(XENLOG_INFO, >> "Requested SVE vector length (%u) > supported length (%u)\n", >> sve_vl_bits, zcr_max_bits); >> return -EINVAL; >> } >> } >> [...] >> >> Now, I understand your point, we could check everything in sve_sanitize_vl_param(), but it would leave a problem >> for domains created by hypercalls if I am not wrong. >> >> What do you think? Sorry i missed that answer. Yes i agree, maybe we could factorize the checks in one function and use it in several places ? > > I thought about that and another possibility is to store “sve_vl” as uint16_t inside struct xen_arch_domainconfig, and > check it inside arch_sanitise_domain_config() for it to be mod 128 and less than the max supported VL, this will > allow to have all the checks in one place, taking a bit more space, anyway we would take the space from the implicit > padding as this is the current status: > > struct arch_domain { > enum domain_type type; /* 0 4 */ > uint8_t sve_vl; /* 4 1 */ > > /* XXX 3 bytes hole, try to pack */ > > struct p2m_domain p2m; /* 8 328 */ > /* --- cacheline 5 boundary (320 bytes) was 16 bytes ago --- */ > struct hvm_domain hvm; /* 336 312 */ > /* --- cacheline 10 boundary (640 bytes) was 8 bytes ago --- */ > struct paging_domain paging; /* 648 32 */ > struct vmmio vmmio; /* 680 32 */ > /* --- cacheline 11 boundary (704 bytes) was 8 bytes ago --- */ > unsigned int rel_priv; /* 712 4 */ > > /* XXX 4 bytes hole, try to pack */ > > struct { > uint64_t offset; /* 720 8 */ > s_time_t nanoseconds; /* 728 8 */ > } virt_timer_base; /* 720 16 */ > struct vgic_dist vgic; /* 736 200 */ > > /* XXX last struct has 2 bytes of padding */ > > /* --- cacheline 14 boundary (896 bytes) was 40 bytes ago --- */ > struct vuart vuart; /* 936 32 */ > /* --- cacheline 15 boundary (960 bytes) was 8 bytes ago --- */ > unsigned int evtchn_irq; /* 968 4 */ > struct { > uint8_t privileged_call_enabled:1; /* 972: 0 1 */ > } monitor; /* 972 1 */ > > /* XXX 3 bytes hole, try to pack */ > > struct vpl011 vpl011; /* 976 72 */ > > /* size: 1152, cachelines: 18, members: 13 */ > /* sum members: 1038, holes: 3, sum holes: 10 */ > /* padding: 104 */ > /* paddings: 1, sum paddings: 2 */ > } __attribute__((__aligned__(128))); That would work but it is a bit odd to save a 16bit value just so you could save invalid values and give an error. Cheers Bertrand
Hi Luca, On 20/04/2023 09:46, Luca Fancellu wrote: > >>>>>> +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; >>>>> >>>>> Shouldn't you also check if it is not greater than the maximum vector length ? >>>> >>>> I don’t understand, I am checking that the value is below or equal to SVE_VL_MAX_BITS, >>>> If you mean if it should be checked also against the maximum supported length by the platform, >>>> I think this is not the right place, the check is already in arch_sanitise_domain_config(), introduced >>>> in patch #2 >>> >>> If this is not the right place to check it then why checking the rest here ? >>> >>> From a user or a developer point of view I would expect the validity of the input to be checked only >>> in one place. >>> If here is not the place for that it is ok but then i would check everything in arch_sanitise_domain_config >>> (multiple, min and supported) instead of doing it partly in 2 places. >> >> Ok, given the way we encoded the value in xen_domctl_createdomain structure, we have that the value takes >> very little space, but a small issue is that when we encode it, we are dividing it by 128, which is fine for user params >> that are multiple of 128, but it’s less fine if the user passes “129”. >> >> To overcome this issue we are checking the value when it is not already encoded. Now, thinking about it, the check >> "&& (val <= SVE_VL_MAX_BITS)” is not really needed, because even if the value is above, then in arch_sanitise_domain_config >> we will hit the top limit of the platform maximum VL. >> >> int arch_sanitise_domain_config(struct xen_domctl_createdomain *config) >> { >> unsigned int max_vcpus; >> unsigned int flags_required = (XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap); >> unsigned int flags_optional = (XEN_DOMCTL_CDF_iommu | XEN_DOMCTL_CDF_vpmu); >> unsigned int sve_vl_bits = sve_decode_vl(config->arch.sve_vl); >> >> if ( (config->flags & ~flags_optional) != flags_required ) >> { >> dprintk(XENLOG_INFO, "Unsupported configuration %#x\n", >> config->flags); >> return -EINVAL; >> } >> >> /* Check feature flags */ >> if ( sve_vl_bits > 0 ) >> { >> unsigned int zcr_max_bits = get_sys_vl_len(); >> >> if ( !zcr_max_bits ) >> { >> dprintk(XENLOG_INFO, "SVE is unsupported on this machine.\n"); >> return -EINVAL; >> } >> >> if ( sve_vl_bits > zcr_max_bits ) >> { >> dprintk(XENLOG_INFO, >> "Requested SVE vector length (%u) > supported length (%u)\n", >> sve_vl_bits, zcr_max_bits); >> return -EINVAL; >> } >> } >> [...] >> >> Now, I understand your point, we could check everything in sve_sanitize_vl_param(), but it would leave a problem >> for domains created by hypercalls if I am not wrong. >> >> What do you think? > > I thought about that and another possibility is to store “sve_vl” as uint16_t inside struct xen_arch_domainconfig, and > check it inside arch_sanitise_domain_config() for it to be mod 128 and less than the max supported VL, this will > allow to have all the checks in one place, taking a bit more space, anyway we would take the space from the implicit > padding as this is the current status: Sorry, I am having trouble to follow the discussion. If you are checking the value in arch_sanitise_domain_config(), then why do you need to take more space in arch_domain? Cheers,
> On 20 Apr 2023, at 13:29, Julien Grall <julien@xen.org> wrote: > > Hi Luca, > > On 20/04/2023 09:46, Luca Fancellu wrote: >>>>>>> +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; >>>>>> >>>>>> Shouldn't you also check if it is not greater than the maximum vector length ? >>>>> >>>>> I don’t understand, I am checking that the value is below or equal to SVE_VL_MAX_BITS, >>>>> If you mean if it should be checked also against the maximum supported length by the platform, >>>>> I think this is not the right place, the check is already in arch_sanitise_domain_config(), introduced >>>>> in patch #2 >>>> >>>> If this is not the right place to check it then why checking the rest here ? >>>> >>>> From a user or a developer point of view I would expect the validity of the input to be checked only >>>> in one place. >>>> If here is not the place for that it is ok but then i would check everything in arch_sanitise_domain_config >>>> (multiple, min and supported) instead of doing it partly in 2 places. >>> >>> Ok, given the way we encoded the value in xen_domctl_createdomain structure, we have that the value takes >>> very little space, but a small issue is that when we encode it, we are dividing it by 128, which is fine for user params >>> that are multiple of 128, but it’s less fine if the user passes “129”. >>> >>> To overcome this issue we are checking the value when it is not already encoded. Now, thinking about it, the check >>> "&& (val <= SVE_VL_MAX_BITS)” is not really needed, because even if the value is above, then in arch_sanitise_domain_config >>> we will hit the top limit of the platform maximum VL. >>> >>> int arch_sanitise_domain_config(struct xen_domctl_createdomain *config) >>> { >>> unsigned int max_vcpus; >>> unsigned int flags_required = (XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap); >>> unsigned int flags_optional = (XEN_DOMCTL_CDF_iommu | XEN_DOMCTL_CDF_vpmu); >>> unsigned int sve_vl_bits = sve_decode_vl(config->arch.sve_vl); >>> >>> if ( (config->flags & ~flags_optional) != flags_required ) >>> { >>> dprintk(XENLOG_INFO, "Unsupported configuration %#x\n", >>> config->flags); >>> return -EINVAL; >>> } >>> >>> /* Check feature flags */ >>> if ( sve_vl_bits > 0 ) >>> { >>> unsigned int zcr_max_bits = get_sys_vl_len(); >>> >>> if ( !zcr_max_bits ) >>> { >>> dprintk(XENLOG_INFO, "SVE is unsupported on this machine.\n"); >>> return -EINVAL; >>> } >>> >>> if ( sve_vl_bits > zcr_max_bits ) >>> { >>> dprintk(XENLOG_INFO, >>> "Requested SVE vector length (%u) > supported length (%u)\n", >>> sve_vl_bits, zcr_max_bits); >>> return -EINVAL; >>> } >>> } >>> [...] >>> >>> Now, I understand your point, we could check everything in sve_sanitize_vl_param(), but it would leave a problem >>> for domains created by hypercalls if I am not wrong. >>> >>> What do you think? >> I thought about that and another possibility is to store “sve_vl” as uint16_t inside struct xen_arch_domainconfig, and >> check it inside arch_sanitise_domain_config() for it to be mod 128 and less than the max supported VL, this will >> allow to have all the checks in one place, taking a bit more space, anyway we would take the space from the implicit >> padding as this is the current status: Hi Julien, > > Sorry, I am having trouble to follow the discussion. If you are checking the value in arch_sanitise_domain_config(), then why do you need to take more space in arch_domain? Yes I am checking the value in arch_sanitise_domain_config, the value checked is from arch_domain and it is the vector length divided by 128, so an encoded value. Bertrand was puzzled by the fact that I also put a check in sve_sanitize_vl_param to see if the vector length passed by the user is mod 128, his point is that we should check a value only in one place and not in two, and it is a valid point but in this case we can’t put all the check into arch_sanitise_domain_config because we don’t have the “full” value from arch_domain, and we can’t put all the checks in sve_sanitize_vl_param because it will leave out from the check domains created by hyper calls. So to have only one point where the checks are done (mod 128 and <= HW supported VL), we might need to have a full resolution VL value in struct xen_arch_domainconfig (16 bit). But if we want to save space for the future, we could leave the code as it is and rely on the fact that the tools (or Xen) when creating the domain configuration, will check that the SVE VL parameter is mod 128. In this last case what is in struct xen_arch_domainconfig is implicitly mod 128 and only the upper boundary of the max supported VL will be checked by Xen inside arch_sanitise_domain_config. > > Cheers, > > -- > Julien Grall
Hi Luca, On 20/04/2023 13:43, Luca Fancellu wrote: >> On 20 Apr 2023, at 13:29, Julien Grall <julien@xen.org> wrote: >> >> Hi Luca, >> >> On 20/04/2023 09:46, Luca Fancellu wrote: >>>>>>>> +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; >>>>>>> >>>>>>> Shouldn't you also check if it is not greater than the maximum vector length ? >>>>>> >>>>>> I don’t understand, I am checking that the value is below or equal to SVE_VL_MAX_BITS, >>>>>> If you mean if it should be checked also against the maximum supported length by the platform, >>>>>> I think this is not the right place, the check is already in arch_sanitise_domain_config(), introduced >>>>>> in patch #2 >>>>> >>>>> If this is not the right place to check it then why checking the rest here ? >>>>> >>>>> From a user or a developer point of view I would expect the validity of the input to be checked only >>>>> in one place. >>>>> If here is not the place for that it is ok but then i would check everything in arch_sanitise_domain_config >>>>> (multiple, min and supported) instead of doing it partly in 2 places. >>>> >>>> Ok, given the way we encoded the value in xen_domctl_createdomain structure, we have that the value takes >>>> very little space, but a small issue is that when we encode it, we are dividing it by 128, which is fine for user params >>>> that are multiple of 128, but it’s less fine if the user passes “129”. >>>> >>>> To overcome this issue we are checking the value when it is not already encoded. Now, thinking about it, the check >>>> "&& (val <= SVE_VL_MAX_BITS)” is not really needed, because even if the value is above, then in arch_sanitise_domain_config >>>> we will hit the top limit of the platform maximum VL. >>>> >>>> int arch_sanitise_domain_config(struct xen_domctl_createdomain *config) >>>> { >>>> unsigned int max_vcpus; >>>> unsigned int flags_required = (XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap); >>>> unsigned int flags_optional = (XEN_DOMCTL_CDF_iommu | XEN_DOMCTL_CDF_vpmu); >>>> unsigned int sve_vl_bits = sve_decode_vl(config->arch.sve_vl); >>>> >>>> if ( (config->flags & ~flags_optional) != flags_required ) >>>> { >>>> dprintk(XENLOG_INFO, "Unsupported configuration %#x\n", >>>> config->flags); >>>> return -EINVAL; >>>> } >>>> >>>> /* Check feature flags */ >>>> if ( sve_vl_bits > 0 ) >>>> { >>>> unsigned int zcr_max_bits = get_sys_vl_len(); >>>> >>>> if ( !zcr_max_bits ) >>>> { >>>> dprintk(XENLOG_INFO, "SVE is unsupported on this machine.\n"); >>>> return -EINVAL; >>>> } >>>> >>>> if ( sve_vl_bits > zcr_max_bits ) >>>> { >>>> dprintk(XENLOG_INFO, >>>> "Requested SVE vector length (%u) > supported length (%u)\n", >>>> sve_vl_bits, zcr_max_bits); >>>> return -EINVAL; >>>> } >>>> } >>>> [...] >>>> >>>> Now, I understand your point, we could check everything in sve_sanitize_vl_param(), but it would leave a problem >>>> for domains created by hypercalls if I am not wrong. >>>> >>>> What do you think? >>> I thought about that and another possibility is to store “sve_vl” as uint16_t inside struct xen_arch_domainconfig, and >>> check it inside arch_sanitise_domain_config() for it to be mod 128 and less than the max supported VL, this will >>> allow to have all the checks in one place, taking a bit more space, anyway we would take the space from the implicit >>> padding as this is the current status: > > Hi Julien, > >> >> Sorry, I am having trouble to follow the discussion. If you are checking the value in arch_sanitise_domain_config(), then why do you need to take more space in arch_domain? > > Yes I am checking the value in arch_sanitise_domain_config, the value checked is from arch_domain and it is the vector length divided by 128, so an encoded value. I think this is where the disconnect is coming from. arch_sanitise_domain_config() doesn't use struct arch_domain because the domain itself is not yet allocated. Instead, it is using xen_arch_domainconfig. I care less about the increase of xen_arch_domainconfig because this is a one off structure. > > Bertrand was puzzled by the fact that I also put a check in sve_sanitize_vl_param to see if the vector length passed by the user is mod 128, his point is that we should check a value only in one place and not in two, and it is a valid point but in this case we can’t put all the check into arch_sanitise_domain_config because we don’t have the “full” value from arch_domain, and we can’t put all the checks in sve_sanitize_vl_param because it will leave out from the check domains created by hyper calls. > > So to have only one point where the checks are done (mod 128 and <= HW supported VL), we might need to have a full resolution VL value in struct xen_arch_domainconfig (16 bit). > > But if we want to save space for the future, we could leave the code as it is and rely on the fact that the tools (or Xen) when creating the domain configuration, will check that the SVE VL parameter is mod 128. > In this last case what is in struct xen_arch_domainconfig is implicitly mod 128 and only the upper boundary of the max supported VL will be checked by Xen inside arch_sanitise_domain_config. Before answering to the approach, can you explain why you ask the user to pass a value that is a multiple of 128 rather than the already "divided" value? Is it more natural for the user? Cheers,
> On 20 Apr 2023, at 14:08, Julien Grall <julien@xen.org> wrote: > > Hi Luca, > > On 20/04/2023 13:43, Luca Fancellu wrote: >>> On 20 Apr 2023, at 13:29, Julien Grall <julien@xen.org> wrote: >>> >>> Hi Luca, >>> >>> On 20/04/2023 09:46, Luca Fancellu wrote: >>>>>>>>> +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; >>>>>>>> >>>>>>>> Shouldn't you also check if it is not greater than the maximum vector length ? >>>>>>> >>>>>>> I don’t understand, I am checking that the value is below or equal to SVE_VL_MAX_BITS, >>>>>>> If you mean if it should be checked also against the maximum supported length by the platform, >>>>>>> I think this is not the right place, the check is already in arch_sanitise_domain_config(), introduced >>>>>>> in patch #2 >>>>>> >>>>>> If this is not the right place to check it then why checking the rest here ? >>>>>> >>>>>> From a user or a developer point of view I would expect the validity of the input to be checked only >>>>>> in one place. >>>>>> If here is not the place for that it is ok but then i would check everything in arch_sanitise_domain_config >>>>>> (multiple, min and supported) instead of doing it partly in 2 places. >>>>> >>>>> Ok, given the way we encoded the value in xen_domctl_createdomain structure, we have that the value takes >>>>> very little space, but a small issue is that when we encode it, we are dividing it by 128, which is fine for user params >>>>> that are multiple of 128, but it’s less fine if the user passes “129”. >>>>> >>>>> To overcome this issue we are checking the value when it is not already encoded. Now, thinking about it, the check >>>>> "&& (val <= SVE_VL_MAX_BITS)” is not really needed, because even if the value is above, then in arch_sanitise_domain_config >>>>> we will hit the top limit of the platform maximum VL. >>>>> >>>>> int arch_sanitise_domain_config(struct xen_domctl_createdomain *config) >>>>> { >>>>> unsigned int max_vcpus; >>>>> unsigned int flags_required = (XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap); >>>>> unsigned int flags_optional = (XEN_DOMCTL_CDF_iommu | XEN_DOMCTL_CDF_vpmu); >>>>> unsigned int sve_vl_bits = sve_decode_vl(config->arch.sve_vl); >>>>> >>>>> if ( (config->flags & ~flags_optional) != flags_required ) >>>>> { >>>>> dprintk(XENLOG_INFO, "Unsupported configuration %#x\n", >>>>> config->flags); >>>>> return -EINVAL; >>>>> } >>>>> >>>>> /* Check feature flags */ >>>>> if ( sve_vl_bits > 0 ) >>>>> { >>>>> unsigned int zcr_max_bits = get_sys_vl_len(); >>>>> >>>>> if ( !zcr_max_bits ) >>>>> { >>>>> dprintk(XENLOG_INFO, "SVE is unsupported on this machine.\n"); >>>>> return -EINVAL; >>>>> } >>>>> >>>>> if ( sve_vl_bits > zcr_max_bits ) >>>>> { >>>>> dprintk(XENLOG_INFO, >>>>> "Requested SVE vector length (%u) > supported length (%u)\n", >>>>> sve_vl_bits, zcr_max_bits); >>>>> return -EINVAL; >>>>> } >>>>> } >>>>> [...] >>>>> >>>>> Now, I understand your point, we could check everything in sve_sanitize_vl_param(), but it would leave a problem >>>>> for domains created by hypercalls if I am not wrong. >>>>> >>>>> What do you think? >>>> I thought about that and another possibility is to store “sve_vl” as uint16_t inside struct xen_arch_domainconfig, and >>>> check it inside arch_sanitise_domain_config() for it to be mod 128 and less than the max supported VL, this will >>>> allow to have all the checks in one place, taking a bit more space, anyway we would take the space from the implicit >>>> padding as this is the current status: >> Hi Julien, >>> >>> Sorry, I am having trouble to follow the discussion. If you are checking the value in arch_sanitise_domain_config(), then why do you need to take more space in arch_domain? >> Yes I am checking the value in arch_sanitise_domain_config, the value checked is from arch_domain and it is the vector length divided by 128, so an encoded value. > > I think this is where the disconnect is coming from. arch_sanitise_domain_config() doesn't use struct arch_domain because the domain itself is not yet allocated. Instead, it is using xen_arch_domainconfig. > > I care less about the increase of xen_arch_domainconfig because this is a one off structure. I’m sorry, yes, I meant struct xen_domctl_createdomain, sorry I got confused copying from this thread > >> Bertrand was puzzled by the fact that I also put a check in sve_sanitize_vl_param to see if the vector length passed by the user is mod 128, his point is that we should check a value only in one place and not in two, and it is a valid point but in this case we can’t put all the check into arch_sanitise_domain_config because we don’t have the “full” value from arch_domain, and we can’t put all the checks in sve_sanitize_vl_param because it will leave out from the check domains created by hyper calls. >> So to have only one point where the checks are done (mod 128 and <= HW supported VL), we might need to have a full resolution VL value in struct xen_arch_domainconfig (16 bit). >> But if we want to save space for the future, we could leave the code as it is and rely on the fact that the tools (or Xen) when creating the domain configuration, will check that the SVE VL parameter is mod 128. >> In this last case what is in struct xen_arch_domainconfig is implicitly mod 128 and only the upper boundary of the max supported VL will be checked by Xen inside arch_sanitise_domain_config. > > Before answering to the approach, can you explain why you ask the user to pass a value that is a multiple of 128 rather than the already "divided" value? Is it more natural for the user? Yes I thought is was more natural for the user to think about number of bits (for example 512) instead of an encoded value (4 in this case). > > Cheers, > > -- > Julien Grall
diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc index e0b89b7d3319..9c0790ce6c7c 100644 --- a/docs/misc/xen-command-line.pandoc +++ b/docs/misc/xen-command-line.pandoc @@ -777,9 +777,9 @@ Specify the bit width of the DMA heap. ### dom0 = List of [ pv | pvh, shadow=<bool>, verbose=<bool>, - cpuid-faulting=<bool>, msr-relaxed=<bool> ] + cpuid-faulting=<bool>, msr-relaxed=<bool> ] (x86) - Applicability: x86 + = List of [ sve=<integer> ] (Arm) Controls for how dom0 is constructed on x86 systems. @@ -838,6 +838,20 @@ Controls for how dom0 is constructed on x86 systems. If using this option is necessary to fix an issue, please report a bug. +Enables features on dom0 on Arm systems. + +* The `sve` integer parameter enables Arm SVE usage for Dom0 domain and sets + the maximum SVE vector length. + A value equal to 0 disables the feature, this is the default value. + Values below 0 means the feature uses the maximum SVE vector length + supported by hardware, please be aware that if the hardware doesn't supports + SVE, the feature remains disabled. + Values above 0 explicitly set the maximum SVE vector length for Dom0, + allowed values are from 128 to maximum 2048, being multiple of 128. + Please note that when the user explicitly specify the value, if that value + is above the hardware supported maximum SVE vector length, the domain + creation will fail and the system will stop. + ### dom0-cpuid = List of comma separated booleans diff --git a/xen/arch/arm/arm64/sve.c b/xen/arch/arm/arm64/sve.c index 5485648850a0..ad5db62e1805 100644 --- a/xen/arch/arm/arm64/sve.c +++ b/xen/arch/arm/arm64/sve.c @@ -9,6 +9,9 @@ #include <xen/sizes.h> #include <asm/arm64/sve.h> +/* opt_dom0_sve: allow Dom0 to use SVE and set maximum vector length. */ +int __initdata opt_dom0_sve; + extern unsigned int sve_get_hw_vl(void); extern void sve_save_ctx(uint64_t *sve_ctx, uint64_t *pregs, int save_ffr); extern void sve_load_ctx(uint64_t const *sve_ctx, uint64_t const *pregs, @@ -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; +} diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index eeb4662f0eee..3f30ef5c37b6 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -26,6 +26,7 @@ #include <asm/platform.h> #include <asm/psci.h> #include <asm/setup.h> +#include <asm/arm64/sve.h> #include <asm/cpufeature.h> #include <asm/domain_build.h> #include <xen/event.h> @@ -61,6 +62,21 @@ custom_param("dom0_mem", parse_dom0_mem); int __init parse_arch_dom0_param(const char *s, const char *e) { + long long val; + + if ( !parse_signed_integer("sve", s, e, &val) ) + { +#ifdef CONFIG_ARM64_SVE + if ( (val >= INT_MIN) && (val <= INT_MAX) ) + opt_dom0_sve = val; + else + printk(XENLOG_INFO "'sve=%lld' value out of range!\n", val); +#else + no_config_param("ARM64_SVE", "sve", s, e); +#endif + return 0; + } + return -EINVAL; } @@ -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) ) + dom0_cfg.arch.sve_vl = sve_encode_vl(vl); + else + printk(XENLOG_WARNING + "SVE vector length error, disable feature for Dom0\n"); + } + dom0 = domain_create(0, &dom0_cfg, CDF_privileged | CDF_directmap); if ( IS_ERR(dom0) ) panic("Error creating domain 0 (rc = %ld)\n", PTR_ERR(dom0)); diff --git a/xen/arch/arm/include/asm/arm64/sve.h b/xen/arch/arm/include/asm/arm64/sve.h index fc162c9d2cf7..f1801876b5de 100644 --- a/xen/arch/arm/include/asm/arm64/sve.h +++ b/xen/arch/arm/include/asm/arm64/sve.h @@ -19,8 +19,15 @@ static inline unsigned int sve_decode_vl(unsigned int sve_vl) return sve_vl * SVE_VL_MULTIPLE_VAL; } +static inline unsigned int sve_encode_vl(unsigned int sve_vl_bits) +{ + return sve_vl_bits / SVE_VL_MULTIPLE_VAL; +} + #ifdef CONFIG_ARM64_SVE +extern int opt_dom0_sve; + register_t compute_max_zcr(void); register_t vl_to_zcr(unsigned int vl); unsigned int get_sys_vl_len(void); @@ -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) + static inline register_t compute_max_zcr(void) { return 0; @@ -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; +} + #endif /* CONFIG_ARM64_SVE */ #endif /* _ARM_ARM64_SVE_H */ diff --git a/xen/common/kernel.c b/xen/common/kernel.c index f7b1f65f373c..29d05282c8bb 100644 --- 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); + nlen = strlen(name); + + /* Does s start with name or contains only the name? */ + 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; + + *val = pval; + + return 0; +} + int cmdline_strcmp(const char *frag, const char *name) { for ( ; ; frag++, name++ ) diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h index e914ccade095..5343ee7a944a 100644 --- a/xen/include/xen/lib.h +++ b/xen/include/xen/lib.h @@ -94,6 +94,16 @@ int parse_bool(const char *s, const char *e); */ int parse_boolean(const char *name, const char *s, const char *e); +/** + * Given a specific name, parses a string of the form: + * $NAME=<integer number> + * returning 0 and a value in val, for a recognised integer. + * Returns -1 for name not found, general errors, or -2 if name is found but + * not recognised number. + */ +int parse_signed_integer(const char *name, const char *s, const char *e, + long long *val); + /** * Very similar to strcmp(), but will declare a match if the NUL in 'name' * lines up with comma, colon, semicolon or equals in 'frag'. Designed for
Add a command line parameter to allow Dom0 the use of SVE resources, the command line parameter sve=<integer>, sub argument of dom0=, controls the feature on this domain and sets the maximum SVE vector length for Dom0. Add a new function, parse_signed_integer(), to parse an integer command line argument. Signed-off-by: Luca Fancellu <luca.fancellu@arm.com> --- Changes from v4: - Negative values as user param means max supported HW VL (Jan) - update documentation, make use of no_config_param(), rename parse_integer into parse_signed_integer and take long long *, also put a comment on the -2 return condition, update declaration comment to reflect the modifications (Jan) Changes from v3: - Don't use fixed len types when not needed (Jan) - renamed domainconfig_encode_vl to sve_encode_vl - Use a sub argument of dom0= to enable the feature (Jan) - Add parse_integer() function Changes from v2: - xen_domctl_createdomain field has changed into sve_vl and its value now is the VL / 128, create an helper function for that. Changes from v1: - No changes Changes from RFC: - Changed docs to explain that the domain won't be created if the requested vector length is above the supported one from the platform. --- docs/misc/xen-command-line.pandoc | 18 ++++++++++++++++-- xen/arch/arm/arm64/sve.c | 21 +++++++++++++++++++++ xen/arch/arm/domain_build.c | 27 +++++++++++++++++++++++++++ xen/arch/arm/include/asm/arm64/sve.h | 15 +++++++++++++++ xen/common/kernel.c | 25 +++++++++++++++++++++++++ xen/include/xen/lib.h | 10 ++++++++++ 6 files changed, 114 insertions(+), 2 deletions(-)