Hi Luca, > On 23 May 2023, at 09:43, Luca Fancellu <luca.fance...@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.fance...@arm.com>
Reviewed-by: Bertrand Marquis <bertrand.marq...@arm.com> with ... > --- > Changes from v6: > - Fixed case for e==NULL in parse_signed_integer, drop parenthesis > from if conditions, delete inline sve_domctl_vl_param and rely on > DCE from the compiler (Jan) > - Drop parenthesis from opt_dom0_sve (Julien) > - Do not continue if 'sve' is in command line args but > CONFIG_ARM64_SVE is not selected: > https://lore.kernel.org/all/7614ae25-f59d-430a-9c3e-30b1ce0e1...@arm.com/ > Changes from v5: > - stop the domain if VL error occurs (Julien, Bertrand) > - update the documentation > - Rename sve_sanitize_vl_param to sve_domctl_vl_param to > mark the fact that we are sanitizing a parameter coming from > the user before encoding it into sve_vl in domctl structure. > (suggestion from Bertrand in a separate discussion) > - update comment in parse_signed_integer, return boolean in > sve_domctl_vl_param (Jan). > 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 | 20 ++++++++++++++++++-- > xen/arch/arm/arm64/sve.c | 20 ++++++++++++++++++++ > xen/arch/arm/domain_build.c | 26 ++++++++++++++++++++++++++ > xen/arch/arm/include/asm/arm64/sve.h | 10 ++++++++++ > xen/common/kernel.c | 28 ++++++++++++++++++++++++++++ > xen/include/xen/lib.h | 10 ++++++++++ > 6 files changed, 112 insertions(+), 2 deletions(-) > > diff --git a/docs/misc/xen-command-line.pandoc > b/docs/misc/xen-command-line.pandoc > index e0b89b7d3319..47e5b4eb6199 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,22 @@ 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, the option is applicable only to AArch64 > + guests. Here i would just remove "guests", just AArch64 is enough. I am ok if you choose to use "AArch64 Dom0 kernels" > + 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, if SVE is supported. > + 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 specifies the value, if that > value > + is above the hardware supported maximum SVE vector length, the domain > + creation will fail and the system will stop, the same will occur if the > + option is provided with a non zero value, but the platform doesn't > support > + SVE. > + I agree on the discussion with Jan here so you can keep my R-b if modified as discussed. Cheers Bertrand > ### dom0-cpuid > = List of comma separated booleans > > diff --git a/xen/arch/arm/arm64/sve.c b/xen/arch/arm/arm64/sve.c > index 84a6dedc1fd7..feaca2cf647d 100644 > --- a/xen/arch/arm/arm64/sve.c > +++ b/xen/arch/arm/arm64/sve.c > @@ -13,6 +13,9 @@ > #include <asm/processor.h> > #include <asm/system.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); > > /* > @@ -152,6 +155,23 @@ void sve_restore_state(struct vcpu *v) > sve_load_ctx(v->arch.vfp.sve_zreg_ctx_end, v->arch.vfp.fpregs, 1); > } > > +bool __init sve_domctl_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 > + */ > + if ( val < 0 ) > + *out = get_sys_vl_len(); > + else if ( (val % SVE_VL_MULTIPLE_VAL) == 0 ) > + *out = val; > + else > + return false; > + > + return true; > +} > + > /* > * Local variables: > * mode: C > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > index f373a5024783..9202a96d9c28 100644 > --- a/xen/arch/arm/domain_build.c > +++ b/xen/arch/arm/domain_build.c > @@ -62,6 +62,22 @@ 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); > + > + return 0; > +#else > + panic("'sve' property found, but CONFIG_ARM64_SVE not selected"); > +#endif > + } > + > return -EINVAL; > } > > @@ -4113,6 +4129,16 @@ void __init create_dom0(void) > if ( iommu_enabled ) > dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu; > > + if ( opt_dom0_sve ) > + { > + unsigned int vl; > + > + if ( sve_domctl_vl_param(opt_dom0_sve, &vl) ) > + dom0_cfg.arch.sve_vl = sve_encode_vl(vl); > + else > + panic("SVE vector length error\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 65b46685d263..a71d6a295dcc 100644 > --- a/xen/arch/arm/include/asm/arm64/sve.h > +++ b/xen/arch/arm/include/asm/arm64/sve.h > @@ -21,14 +21,22 @@ 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; > +} > + > register_t compute_max_zcr(void); > 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); > +bool sve_domctl_vl_param(int val, unsigned int *out); > > #ifdef CONFIG_ARM64_SVE > > +extern int opt_dom0_sve; > + > static inline bool is_sve_domain(const struct domain *d) > { > return d->arch.sve_vl > 0; > @@ -38,6 +46,8 @@ unsigned int get_sys_vl_len(void); > > #else /* !CONFIG_ARM64_SVE */ > > +#define opt_dom0_sve 0 > + > static inline bool is_sve_domain(const struct domain *d) > { > return false; > diff --git a/xen/common/kernel.c b/xen/common/kernel.c > index f7b1f65f373c..7cd00a4c999a 100644 > --- a/xen/common/kernel.c > +++ b/xen/common/kernel.c > @@ -314,6 +314,34 @@ 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); > + > + if ( !e ) > + e = s + slen; > + > + /* Check that this is the name we're looking for and a value was > provided */ > + if ( slen <= nlen || strncmp(s, name, nlen) || s[nlen] != '=' ) > + return -1; > + > + pval = simple_strtoll(&s[nlen + 1], &str, 10); > + > + /* 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 >
