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(-)

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
-- 
2.34.1

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

> 
>> @@ -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

> 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