| Message ID | 20230523074326.3035745-8-luca.fancellu@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | SVE feature for arm guests | expand |
On 23.05.2023 09:43, Luca Fancellu 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> Reviewed-by: Jan Beulich <jbeulich@suse.com> # !arm > --- 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) While in the text below you mention this is Arm64 only, I think the tag here would better express this as well. > @@ -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. Why "guests"? Does the option affect more than Dom0? > + 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. Assuming this also covers the -1 case, I wonder if that isn't a little too strict. "Maximum supported" imo can very well be 0. Jan
> On 23 May 2023, at 11:02, Jan Beulich <jbeulich@suse.com> wrote: > > On 23.05.2023 09:43, Luca Fancellu 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> > > Reviewed-by: Jan Beulich <jbeulich@suse.com> # !arm > >> --- 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) > > While in the text below you mention this is Arm64 only, I think the tag > here would better express this as well. Ok I can use Arm64 instead if there is no opposition from others > >> @@ -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. > > Why "guests"? Does the option affect more than Dom0? I used “guests” because in my mind I was referring to all the aarch64 OS that can be used as control domain, I can change it if it sounds bad. > >> + 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. > > Assuming this also covers the -1 case, I wonder if that isn't a little too > strict. "Maximum supported" imo can very well be 0. Maximum supported, when platforms uses SVE, can be at minimum 128 by arm specs. > > Jan
On 23.05.2023 12:21, Luca Fancellu wrote: >> On 23 May 2023, at 11:02, Jan Beulich <jbeulich@suse.com> wrote: >> On 23.05.2023 09:43, Luca Fancellu wrote: >>> @@ -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. >> >> Why "guests"? Does the option affect more than Dom0? > > I used “guests” because in my mind I was referring to all the aarch64 OS that can be used > as control domain, I can change it if it sounds bad. If you means OSes then better also say OSes. But maybe this doesn't need specifically expressing, by saying e.g. "..., the option is applicable only on AArch64"? Or can a Dom0 be 32-bit on Arm64 Xen? >>> + 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. >> >> Assuming this also covers the -1 case, I wonder if that isn't a little too >> strict. "Maximum supported" imo can very well be 0. > > Maximum supported, when platforms uses SVE, can be at minimum 128 by arm specs. When there is SVE - sure. But when there's no SVE, 0 is kind of the implied length. And I'd view a command line option value of -1 quite okay in that case: They've asked for the maximum supported, so they'll get 0. No reason to crash the system during boot. Jan
> On 23 May 2023, at 11:31, Jan Beulich <jbeulich@suse.com> wrote: > > On 23.05.2023 12:21, Luca Fancellu wrote: >>> On 23 May 2023, at 11:02, Jan Beulich <jbeulich@suse.com> wrote: >>> On 23.05.2023 09:43, Luca Fancellu wrote: >>>> @@ -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. >>> >>> Why "guests"? Does the option affect more than Dom0? >> >> I used “guests” because in my mind I was referring to all the aarch64 OS that can be used >> as control domain, I can change it if it sounds bad. > > If you means OSes then better also say OSes. But maybe this doesn't need > specifically expressing, by saying e.g. "..., the option is applicable > only on AArch64"? Or can a Dom0 be 32-bit on Arm64 Xen? I think there is no limitation so Dom0 can be 32 bit or 64. Maybe I can say “... AArch64 kernel guests.”? > >>>> + 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. >>> >>> Assuming this also covers the -1 case, I wonder if that isn't a little too >>> strict. "Maximum supported" imo can very well be 0. >> >> Maximum supported, when platforms uses SVE, can be at minimum 128 by arm specs. > > When there is SVE - sure. But when there's no SVE, 0 is kind of the implied > length. And I'd view a command line option value of -1 quite okay in that > case: They've asked for the maximum supported, so they'll get 0. No reason > to crash the system during boot. Ok I see what you mean, for example when Kconfig SVE is enabled, but the platform doesn’t have SVE feature, requesting sve=-1 will keep the value to 0, and no system will be stopped. Maybe I can say: “... the same will occur if the option is provided with a positive non zero value, but the platform doesn't support SVE." > > Jan
On 23.05.2023 13:50, Luca Fancellu wrote: >> On 23 May 2023, at 11:31, Jan Beulich <jbeulich@suse.com> wrote: >> On 23.05.2023 12:21, Luca Fancellu wrote: >>>> On 23 May 2023, at 11:02, Jan Beulich <jbeulich@suse.com> wrote: >>>> On 23.05.2023 09:43, Luca Fancellu wrote: >>>>> @@ -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. >>>> >>>> Why "guests"? Does the option affect more than Dom0? >>> >>> I used “guests” because in my mind I was referring to all the aarch64 OS that can be used >>> as control domain, I can change it if it sounds bad. >> >> If you means OSes then better also say OSes. But maybe this doesn't need >> specifically expressing, by saying e.g. "..., the option is applicable >> only on AArch64"? Or can a Dom0 be 32-bit on Arm64 Xen? > > I think there is no limitation so Dom0 can be 32 bit or 64. Maybe I can say > “... AArch64 kernel guests.”? I'd recommend to avoid the term "guest" when you talk about Dom0 alone. Commonly "guest" means ordinary domains only, i.e. in particular excluding Dom0. What's wrong with "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. >>>> >>>> Assuming this also covers the -1 case, I wonder if that isn't a little too >>>> strict. "Maximum supported" imo can very well be 0. >>> >>> Maximum supported, when platforms uses SVE, can be at minimum 128 by arm specs. >> >> When there is SVE - sure. But when there's no SVE, 0 is kind of the implied >> length. And I'd view a command line option value of -1 quite okay in that >> case: They've asked for the maximum supported, so they'll get 0. No reason >> to crash the system during boot. > > Ok I see what you mean, for example when Kconfig SVE is enabled, but the platform doesn’t > have SVE feature, requesting sve=-1 will keep the value to 0, and no system will be stopped. > > Maybe I can say: > > “... the same will occur if the option is provided with a positive non zero value, > but the platform doesn't support SVE." Right, provided that matches the implementation. Jan
> On 23 May 2023, at 12:53, Jan Beulich <jbeulich@suse.com> wrote: > > On 23.05.2023 13:50, Luca Fancellu wrote: >>> On 23 May 2023, at 11:31, Jan Beulich <jbeulich@suse.com> wrote: >>> On 23.05.2023 12:21, Luca Fancellu wrote: >>>>> On 23 May 2023, at 11:02, Jan Beulich <jbeulich@suse.com> wrote: >>>>> On 23.05.2023 09:43, Luca Fancellu wrote: >>>>>> @@ -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. >>>>> >>>>> Why "guests"? Does the option affect more than Dom0? >>>> >>>> I used “guests” because in my mind I was referring to all the aarch64 OS that can be used >>>> as control domain, I can change it if it sounds bad. >>> >>> If you means OSes then better also say OSes. But maybe this doesn't need >>> specifically expressing, by saying e.g. "..., the option is applicable >>> only on AArch64"? Or can a Dom0 be 32-bit on Arm64 Xen? >> >> I think there is no limitation so Dom0 can be 32 bit or 64. Maybe I can say >> “... AArch64 kernel guests.”? > > I'd recommend to avoid the term "guest" when you talk about Dom0 alone. > Commonly "guest" means ordinary domains only, i.e. in particular excluding > Dom0. What's wrong with "AArch64 Dom0 kernels"? Ok works for me, I will use “AArch64 Dom0 kernels", I thought “guests” were a generic category and then we have “privileged guests”, for example Dom0 or driver domain, and “unprivileged guests” like DomUs. > >>>>>> + 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. >>>>> >>>>> Assuming this also covers the -1 case, I wonder if that isn't a little too >>>>> strict. "Maximum supported" imo can very well be 0. >>>> >>>> Maximum supported, when platforms uses SVE, can be at minimum 128 by arm specs. >>> >>> When there is SVE - sure. But when there's no SVE, 0 is kind of the implied >>> length. And I'd view a command line option value of -1 quite okay in that >>> case: They've asked for the maximum supported, so they'll get 0. No reason >>> to crash the system during boot. >> >> Ok I see what you mean, for example when Kconfig SVE is enabled, but the platform doesn’t >> have SVE feature, requesting sve=-1 will keep the value to 0, and no system will be stopped. >> >> Maybe I can say: >> >> “... the same will occur if the option is provided with a positive non zero value, >> but the platform doesn't support SVE." > > Right, provided that matches the implementation. Ok I will do the changes, can I retain your R-by? I suppose it covers also documentation right? > > Jan
On 23.05.2023 13:57, Luca Fancellu wrote: >> On 23 May 2023, at 12:53, Jan Beulich <jbeulich@suse.com> wrote: >> On 23.05.2023 13:50, Luca Fancellu wrote: >>>> On 23 May 2023, at 11:31, Jan Beulich <jbeulich@suse.com> wrote: >>>> On 23.05.2023 12:21, Luca Fancellu wrote: >>>>>> On 23 May 2023, at 11:02, Jan Beulich <jbeulich@suse.com> wrote: >>>>>> On 23.05.2023 09:43, Luca Fancellu wrote: >>>>>>> @@ -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. >>>>>> >>>>>> Why "guests"? Does the option affect more than Dom0? >>>>> >>>>> I used “guests” because in my mind I was referring to all the aarch64 OS that can be used >>>>> as control domain, I can change it if it sounds bad. >>>> >>>> If you means OSes then better also say OSes. But maybe this doesn't need >>>> specifically expressing, by saying e.g. "..., the option is applicable >>>> only on AArch64"? Or can a Dom0 be 32-bit on Arm64 Xen? >>> >>> I think there is no limitation so Dom0 can be 32 bit or 64. Maybe I can say >>> “... AArch64 kernel guests.”? >> >> I'd recommend to avoid the term "guest" when you talk about Dom0 alone. >> Commonly "guest" means ordinary domains only, i.e. in particular excluding >> Dom0. What's wrong with "AArch64 Dom0 kernels"? > > Ok works for me, I will use “AArch64 Dom0 kernels", I thought “guests” were a generic category > and then we have “privileged guests”, for example Dom0 or driver domain, and “unprivileged guests” > like DomUs. Well, yes - "commonly" doesn't mean "always". >>>>>>> + 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. >>>>>> >>>>>> Assuming this also covers the -1 case, I wonder if that isn't a little too >>>>>> strict. "Maximum supported" imo can very well be 0. >>>>> >>>>> Maximum supported, when platforms uses SVE, can be at minimum 128 by arm specs. >>>> >>>> When there is SVE - sure. But when there's no SVE, 0 is kind of the implied >>>> length. And I'd view a command line option value of -1 quite okay in that >>>> case: They've asked for the maximum supported, so they'll get 0. No reason >>>> to crash the system during boot. >>> >>> Ok I see what you mean, for example when Kconfig SVE is enabled, but the platform doesn’t >>> have SVE feature, requesting sve=-1 will keep the value to 0, and no system will be stopped. >>> >>> Maybe I can say: >>> >>> “... the same will occur if the option is provided with a positive non zero value, >>> but the platform doesn't support SVE." >> >> Right, provided that matches the implementation. > > Ok I will do the changes, can I retain your R-by? I suppose it covers also documentation right? I guess whether doc is covered is fuzzy. Since the doc part is Arm- specific, I'd probably consider it not covered with the "!arm" that I appended. But whichever way you look at it, you can keep the tag in place. Jan
Hi Luca, > On 23 May 2023, at 09:43, 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> Reviewed-by: Bertrand Marquis <bertrand.marquis@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-30B1CE0E1580@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 >
On 24/05/2023 11:05, Bertrand Marquis wrote: > Hi Luca, Hi, >> On 23 May 2023, at 09:43, 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> > > Reviewed-by: Bertrand Marquis <bertrand.marquis@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-30B1CE0E1580@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 NIT: "Domain" is bit redundant here. >> + 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" So far we have no use of AArch64 in our documentation. We have a few use of Arm64 (with various uppercase). In the code base, we seem to have a mix of AArch64 and Arm64. At the moment, I am not going to ask for consistency in the code. But we should aim to not introduce inconsistency in the documentation. I don't have a strong opinion whether we should use aarch64 or arm64. My only request is to be consistent. Cheers,
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. + 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. + ### 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
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 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-30B1CE0E1580@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(-)