| Message ID | 20230327105944.1360856-8-luca.fancellu@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | SVE feature for arm guests | expand |
On 27.03.2023 12:59, Luca Fancellu wrote: > @@ -838,6 +838,18 @@ 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. > + Values above 0 means feature is enabled for Dom0, otherwise feature is > + disabled. Nit: "above" suggests negative values may also enable the feature, which I'm sure isn't intended. You may want to consider using negative values to signal "use length supported by hardware". > + Possible values are from 0 to maximum 2048, being multiple of 128, that will > + be the maximum vector length. It may be advisable to also state the default here. > + Please note that the platform can supports a lower value, if the requested Maybe better "... may only support ..."? > + value is above the supported one, the domain creation will fail and the > + system will stop. Such behavior may be acceptable for DomU-s which aren't essential for the system (i.e. possibly excluding ones in dom0less scenarios), but I don't think that's very nice for Dom0. I'd rather suggest falling back to no SVE in such an event. > @@ -115,3 +119,8 @@ void sve_restore_state(struct vcpu *v) > > sve_load_ctx(sve_ctx_zreg_end, v->arch.vfp.fpregs, 1); > } > + > +int __init sve_parse_dom0_param(const char *str_begin, const char *str_end) > +{ > + return parse_integer("sve", str_begin, str_end, (int*)&opt_dom0_sve); Please can you avoid introducing casts like this? If you're after an unsigned value, make a function which only parses (and returns) an unsigned one. Also why ... > @@ -61,7 +62,12 @@ custom_param("dom0_mem", parse_dom0_mem); > > int __init parse_arch_dom0_param(const char *str_begin, const char *str_end) > { > - return -1; > + int rc = 0; > + > + if ( sve_parse_dom0_param(str_begin, str_end) < 0 ) > + rc = -EINVAL; ... can't you call parse_integer() right here? opt_dom0_sve isn't static, so ought to be accessible here (provided the necessary header was included). > --- a/xen/common/kernel.c > +++ b/xen/common/kernel.c > @@ -314,6 +314,30 @@ int parse_boolean(const char *name, const char *s, const char *e) > return -1; > } > > +int parse_integer(const char *name, const char *s, const char *e, > + int *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); > + > + if ( (str != e) || (pval < INT_MIN) || (pval > INT_MAX) ) > + return -2; Like its counterpart in parse_boolean() (which I understand you've derived parts of the function from) this if+return wants a comment. Also - why strtoll() when you're only after an int? Yet then another question is whether we really want to gain parse_long() and parse_longlong() functions subsequently, or whether instead we limit ourselves to (e.g.) parse_signed_integer() and parse_unsigned_integer(), taking long long * and unsigned long long * respectively to store their outputs. (Of course right now you'd implement only one of the two.) Finally, for the purposes right now the function can (and should) be __init. > --- 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[=...] > + * 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/overflow/underflow value. > + */ > +int parse_integer(const char *name, const char *s, const char *e, > + int *val); The comment wants to match function behavior: The '=' and the value aren't optional as per the implementation, unlike for parse_boolean(). Also please be precise and say "... and a value in *val, ..." Jan
> On 28 Mar 2023, at 11:08, Jan Beulich <jbeulich@suse.com> wrote: > > On 27.03.2023 12:59, Luca Fancellu wrote: >> @@ -838,6 +838,18 @@ 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. >> + Values above 0 means feature is enabled for Dom0, otherwise feature is >> + disabled. > > Nit: "above" suggests negative values may also enable the feature, which > I'm sure isn't intended. You may want to consider using negative values > to signal "use length supported by hardware". This is a very good suggestion, do you think I should restrict only to one negative value, for example -1, instead of every negative value? > >> + Possible values are from 0 to maximum 2048, being multiple of 128, that will >> + be the maximum vector length. > > It may be advisable to also state the default here. I will add it > >> + Please note that the platform can supports a lower value, if the requested > > Maybe better "... may only support ..."? ok > >> + value is above the supported one, the domain creation will fail and the >> + system will stop. > > Such behavior may be acceptable for DomU-s which aren't essential for the > system (i.e. possibly excluding ones in dom0less scenarios), but I don't > think that's very nice for Dom0. I'd rather suggest falling back to no > SVE in such an event. I guess that with the introduction of a negative value meaning max supported VL, it is ok to stop the system if the user provides a custom VL value that is not OK. I remember Julien asked to stop the creation of Dom0 in such a case on the RFC serie. > >> @@ -115,3 +119,8 @@ void sve_restore_state(struct vcpu *v) >> >> sve_load_ctx(sve_ctx_zreg_end, v->arch.vfp.fpregs, 1); >> } >> + >> +int __init sve_parse_dom0_param(const char *str_begin, const char *str_end) >> +{ >> + return parse_integer("sve", str_begin, str_end, (int*)&opt_dom0_sve); > > Please can you avoid introducing casts like this? If you're after an unsigned > value, make a function which only parses (and returns) an unsigned one. Also > why ... > >> @@ -61,7 +62,12 @@ custom_param("dom0_mem", parse_dom0_mem); >> >> int __init parse_arch_dom0_param(const char *str_begin, const char *str_end) >> { >> - return -1; >> + int rc = 0; >> + >> + if ( sve_parse_dom0_param(str_begin, str_end) < 0 ) >> + rc = -EINVAL; > > ... can't you call parse_integer() right here? opt_dom0_sve isn't static, > so ought to be accessible here (provided the necessary header was included). > >> --- a/xen/common/kernel.c >> +++ b/xen/common/kernel.c >> @@ -314,6 +314,30 @@ int parse_boolean(const char *name, const char *s, const char *e) >> return -1; >> } >> >> +int parse_integer(const char *name, const char *s, const char *e, >> + int *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); >> + >> + if ( (str != e) || (pval < INT_MIN) || (pval > INT_MAX) ) >> + return -2; > > Like its counterpart in parse_boolean() (which I understand you've > derived parts of the function from) this if+return wants a comment. > Also - why strtoll() when you're only after an int? Yet then another > question is whether we really want to gain parse_long() and > parse_longlong() functions subsequently, or whether instead we > limit ourselves to (e.g.) parse_signed_integer() and > parse_unsigned_integer(), taking long long * and unsigned long long * > respectively to store their outputs. (Of course right now you'd > implement only one of the two.) > > Finally, for the purposes right now the function can (and should) be > __init. > >> --- 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[=...] >> + * 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/overflow/underflow value. >> + */ >> +int parse_integer(const char *name, const char *s, const char *e, >> + int *val); > > The comment wants to match function behavior: The '=' and the value > aren't optional as per the implementation, unlike for parse_boolean(). > Also please be precise and say "... and a value in *val, ..." Ok I will address all the comments above > > Jan
On 29.03.2023 13:48, Luca Fancellu wrote: >> On 28 Mar 2023, at 11:08, Jan Beulich <jbeulich@suse.com> wrote: >> On 27.03.2023 12:59, Luca Fancellu wrote: >>> @@ -838,6 +838,18 @@ 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. >>> + Values above 0 means feature is enabled for Dom0, otherwise feature is >>> + disabled. >> >> Nit: "above" suggests negative values may also enable the feature, which >> I'm sure isn't intended. You may want to consider using negative values >> to signal "use length supported by hardware". > > This is a very good suggestion, do you think I should restrict only to one negative value, > for example -1, instead of every negative value? That highly depends on whether there's any foreseeable use for other negative values. I can't imagine such, so I'm inclined to say that "just negative" is all that matters. >>> + Please note that the platform can supports a lower value, if the requested >> >> Maybe better "... may only support ..."? > > ok > >> >>> + value is above the supported one, the domain creation will fail and the >>> + system will stop. >> >> Such behavior may be acceptable for DomU-s which aren't essential for the >> system (i.e. possibly excluding ones in dom0less scenarios), but I don't >> think that's very nice for Dom0. I'd rather suggest falling back to no >> SVE in such an event. > > I guess that with the introduction of a negative value meaning max supported > VL, it is ok to stop the system if the user provides a custom VL value that is > not OK. I remember Julien asked to stop the creation of Dom0 in such a case on > the RFC serie. Oh, okay. I don't mean to override a maintainer's view. I don't see the connection to assigning meaning to negative values though - preventing successful (even if functionally restricted) boot is imo never a good thing, when it can easily be avoided. Jan
> >> @@ -61,7 +62,12 @@ custom_param("dom0_mem", parse_dom0_mem); >> >> int __init parse_arch_dom0_param(const char *str_begin, const char *str_end) >> { >> - return -1; >> + int rc = 0; >> + >> + if ( sve_parse_dom0_param(str_begin, str_end) < 0 ) >> + rc = -EINVAL; > > ... can't you call parse_integer() right here? opt_dom0_sve isn't static, > so ought to be accessible here (provided the necessary header was included). > Oh ok now I’ve seen why I’m doing this, because ops_dom0_sve is compiled only when CONFIG_ARM64_SVE is enabled, so I’m using sve_parse_dom0_param() that returns negative if that option is not enabled. Otherwise I should declare ops_dom0_sve anyway, but I should not accept user customization of it if the option is not enabled. So I thought the use of sve_parse_dom0_param() was the best way to handle that
On 29.03.2023 14:06, Luca Fancellu wrote: >>> @@ -61,7 +62,12 @@ custom_param("dom0_mem", parse_dom0_mem); >>> >>> int __init parse_arch_dom0_param(const char *str_begin, const char *str_end) >>> { >>> - return -1; >>> + int rc = 0; >>> + >>> + if ( sve_parse_dom0_param(str_begin, str_end) < 0 ) >>> + rc = -EINVAL; >> >> ... can't you call parse_integer() right here? opt_dom0_sve isn't static, >> so ought to be accessible here (provided the necessary header was included). >> > > Oh ok now I’ve seen why I’m doing this, because ops_dom0_sve is compiled only > when CONFIG_ARM64_SVE is enabled, so I’m using sve_parse_dom0_param() > that returns negative if that option is not enabled. > > Otherwise I should declare ops_dom0_sve anyway, but I should not accept user > customization of it if the option is not enabled. > > So I thought the use of sve_parse_dom0_param() was the best way to handle that Maybe. But please also pay attention to the existence of no_config_param() (as in: consider using it here, which would require the code to live outside of sve.c). Jan
> On 29 Mar 2023, at 13:24, Jan Beulich <jbeulich@suse.com> wrote: > > On 29.03.2023 14:06, Luca Fancellu wrote: >>>> @@ -61,7 +62,12 @@ custom_param("dom0_mem", parse_dom0_mem); >>>> >>>> int __init parse_arch_dom0_param(const char *str_begin, const char *str_end) >>>> { >>>> - return -1; >>>> + int rc = 0; >>>> + >>>> + if ( sve_parse_dom0_param(str_begin, str_end) < 0 ) >>>> + rc = -EINVAL; >>> >>> ... can't you call parse_integer() right here? opt_dom0_sve isn't static, >>> so ought to be accessible here (provided the necessary header was included). >>> >> >> Oh ok now I’ve seen why I’m doing this, because ops_dom0_sve is compiled only >> when CONFIG_ARM64_SVE is enabled, so I’m using sve_parse_dom0_param() >> that returns negative if that option is not enabled. >> >> Otherwise I should declare ops_dom0_sve anyway, but I should not accept user >> customization of it if the option is not enabled. >> >> So I thought the use of sve_parse_dom0_param() was the best way to handle that > > Maybe. But please also pay attention to the existence of no_config_param() > (as in: consider using it here, which would require the code to live outside > of sve.c). Thank you, I didn’t know the existence of no_config_param(), I’ve had a look on the approach, for example in static int __init cf_check parse_cet(const char *s), and I’ll do something similar > > Jan
diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc index e0b89b7d3319..06c1eb4e6d6f 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,18 @@ 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. + Values above 0 means feature is enabled for Dom0, otherwise feature is + disabled. + Possible values are from 0 to maximum 2048, being multiple of 128, that will + be the maximum vector length. + Please note that the platform can supports a lower value, if the requested + value is above the supported one, 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 696a97811cac..6416403817e3 100644 --- a/xen/arch/arm/arm64/sve.c +++ b/xen/arch/arm/arm64/sve.c @@ -5,10 +5,14 @@ * Copyright (C) 2022 ARM Ltd. */ +#include <xen/param.h> #include <xen/sched.h> #include <xen/sizes.h> #include <asm/arm64/sve.h> +/* opt_dom0_sve: allow Dom0 to use SVE and set maximum vector length. */ +unsigned 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, @@ -115,3 +119,8 @@ void sve_restore_state(struct vcpu *v) sve_load_ctx(sve_ctx_zreg_end, v->arch.vfp.fpregs, 1); } + +int __init sve_parse_dom0_param(const char *str_begin, const char *str_end) +{ + return parse_integer("sve", str_begin, str_end, (int*)&opt_dom0_sve); +} diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index 35dbe964fc8b..f6019ce30149 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,7 +62,12 @@ custom_param("dom0_mem", parse_dom0_mem); int __init parse_arch_dom0_param(const char *str_begin, const char *str_end) { - return -1; + int rc = 0; + + if ( sve_parse_dom0_param(str_begin, str_end) < 0 ) + rc = -EINVAL; + + return rc; } /* Override macros from asm/page.h to make them work with mfn_t */ @@ -4089,6 +4095,9 @@ void __init create_dom0(void) if ( iommu_enabled ) dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu; + if ( opt_dom0_sve > 0 ) + dom0_cfg.arch.sve_vl = sve_encode_vl(opt_dom0_sve); + 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 d38a37408439..69a1044c37d9 100644 --- a/xen/arch/arm/include/asm/arm64/sve.h +++ b/xen/arch/arm/include/asm/arm64/sve.h @@ -25,8 +25,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 unsigned 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); @@ -34,9 +41,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_parse_dom0_param(const char *str_begin, const char *str_end); #else /* !CONFIG_ARM64_SVE */ +#define opt_dom0_sve (0) + static inline register_t compute_max_zcr(void) { return 0; @@ -61,6 +71,12 @@ 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_parse_dom0_param(const char *str_begin, + const char *str_end) +{ + return -1; +} + #endif #endif /* _ARM_ARM64_SVE_H */ diff --git a/xen/common/kernel.c b/xen/common/kernel.c index f7b1f65f373c..97b460f5a5c2 100644 --- a/xen/common/kernel.c +++ b/xen/common/kernel.c @@ -314,6 +314,30 @@ int parse_boolean(const char *name, const char *s, const char *e) return -1; } +int parse_integer(const char *name, const char *s, const char *e, + int *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); + + if ( (str != e) || (pval < INT_MIN) || (pval > INT_MAX) ) + 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 05ee1e18af6b..900f1257acb4 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[=...] + * 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/overflow/underflow value. + */ +int parse_integer(const char *name, const char *s, const char *e, + int *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_integer(), to parse an integer command line argument. Signed-off-by: Luca Fancellu <luca.fancellu@arm.com> --- 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 | 16 ++++++++++++++-- xen/arch/arm/arm64/sve.c | 9 +++++++++ xen/arch/arm/domain_build.c | 11 ++++++++++- xen/arch/arm/include/asm/arm64/sve.h | 16 ++++++++++++++++ xen/common/kernel.c | 24 ++++++++++++++++++++++++ xen/include/xen/lib.h | 10 ++++++++++ 6 files changed, 83 insertions(+), 3 deletions(-)