Re: [PATCH v4 02/12] xen/arm: add SVE vector length field to the domain

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

Hi Jan,

Thank you for your review, very appreciated,

> On 28 Mar 2023, at 10:36, Jan Beulich <jbeulich@xxxxxxxx> wrote:
> 
> On 27.03.2023 12:59, Luca Fancellu wrote:
>> @@ -43,8 +44,16 @@ register_t compute_max_zcr(void)
>> }
>> 
>> /* Takes a vector length in bits and returns the ZCR_ELx encoding */
>> -register_t vl_to_zcr(uint16_t vl)
>> +register_t vl_to_zcr(unsigned int vl)
>> {
>>     ASSERT(vl > 0);
>>     return ((vl / SVE_VL_MULTIPLE_VAL) - 1U) & ZCR_ELx_LEN_MASK;
>> }
>> +
>> +/* Get the system sanitized value for VL in bits */
>> +unsigned int get_sys_vl_len(void)
>> +{
>> +    /* ZCR_ELx len field is ((len+1) * 128) = vector bits length */
>> +    return ((system_cpuinfo.zcr64.bits[0] & ZCR_ELx_LEN_MASK) + 1U) *
>> +            SVE_VL_MULTIPLE_VAL;
>> +}
> 
> Wouldn't this function better return 0 when !cpu_has_sve?

Yes I agree

> 
>> @@ -602,6 +606,31 @@ int arch_sanitise_domain_config(struct 
>> xen_domctl_createdomain *config)
>>         return -EINVAL;
>>     }
>> 
>> +    /* Check feature flags */
>> +    if ( sve_vl_bits > 0 ) {
> 
> Nit: Style (brace placement).

Will fix

> 
>> +        unsigned int zcr_max_bits = get_sys_vl_len();
>> +
>> +        if ( !cpu_has_sve )
>> +        {
>> +            dprintk(XENLOG_INFO, "SVE is unsupported on this machine.\n");
>> +            return -EINVAL;
>> +        }
>> +        else if ( !is_vl_valid(sve_vl_bits) )
> 
> If this was code I'm the maintainer for, I'd ask for the "else" to be
> dropped. Please consider doing so, as it makes more visible that the
> earlier if() cannot "fall through". (This could then be further
> supported by inserting blank lines, here and again right below.)
> 
> As to the check - this being the only user of is_vl_valid() (at this
> point) I'd like to mention that half of what that function checks is
> now pointless, as we're dealing with a decoded value. Future further
> users may need the full value checked, but given that all interfaces
> are now using encoded values this doesn't seem very likely. Hence the
> respective part of the condition there may want to become an
> assertion instead (or be dropped).

Yes I can do that

> 
> Yet another question is whether both range checks (against
> SVE_VL_MAX_BITS and zcr_max_bits) are actually necessary / useful.
> Iirc 2048 is a hard upper bound, so zcr_max_bits being higher than
> that value should likely lead to not using SVE at all (if it doesn't
> already; didn't check).

I think the check sve_vl_bits > zcr_max_bits is needed because from 
sve_vl_bits = sve_decode_vl(config->arch.sve_vl); I can get values above the
maximum supported bits (zcr_max_bits), later on I will use the struct 
arch_domain
field sve_vl to compute the size of the registers to be saved/restore

Is there something I’ve missed from your comment?

> 
>> +        {
>> +            dprintk(XENLOG_INFO, "Unsupported SVE vector length (%u)\n",
>> +                    sve_vl_bits);
>> +            return -EINVAL;
>> +        }
>> +        else if ( sve_vl_bits > zcr_max_bits )
>> +        {
>> +            dprintk(XENLOG_INFO,
>> +                    "The requested SVE vector length (%u) must be lower or 
>> \n"
>> +                    "equal to the platform supported vector length (%u)\n",
>> +                    sve_vl_bits, zcr_max_bits);
> 
> Again, if I was the maintainer of this code, I'd ask for the message
> to be shortened, such that it easily fits on one line. E.g.
> "requested SVE vector length (%u) > supported length (%u)\n". This
> would then also avoid the apparent grammar issue of "lower" not fitting
> with "to" (i.e. a "than" needing inserting, or "below" being used
> instead).

Yes this suggestion makes sense to me.


To address your comments I’m doing these modifications to the patch, I hope
they caption all your feedbacks:

diff --git a/xen/arch/arm/arm64/sve.c b/xen/arch/arm/arm64/sve.c
index 3c3adfb5c6bd..78f7482619da 100644
--- a/xen/arch/arm/arm64/sve.c
+++ b/xen/arch/arm/arm64/sve.c
@@ -53,6 +53,9 @@ register_t vl_to_zcr(unsigned int vl)
 /* Get the system sanitized value for VL in bits */
 unsigned int get_sys_vl_len(void)
 {
+    if ( !cpu_has_sve )
+        return 0;
+
     /* ZCR_ELx len field is ((len+1) * 128) = vector bits length */
     return ((system_cpuinfo.zcr64.bits[0] & ZCR_ELx_LEN_MASK) + 1U) *
             SVE_VL_MULTIPLE_VAL;
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 7182d4567bf0..c1fb30dfc5ef 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -607,7 +607,8 @@ int arch_sanitise_domain_config(struct 
xen_domctl_createdomain *config)
     }
 
     /* Check feature flags */
-    if ( sve_vl_bits > 0 ) {
+    if ( sve_vl_bits > 0 )
+    {
         unsigned int zcr_max_bits = get_sys_vl_len();
 
         if ( !cpu_has_sve )
@@ -615,17 +616,18 @@ int arch_sanitise_domain_config(struct 
xen_domctl_createdomain *config)
             dprintk(XENLOG_INFO, "SVE is unsupported on this machine.\n");
             return -EINVAL;
         }
-        else if ( !is_vl_valid(sve_vl_bits) )
+
+        if ( !is_vl_valid(sve_vl_bits) )
         {
             dprintk(XENLOG_INFO, "Unsupported SVE vector length (%u)\n",
                     sve_vl_bits);
             return -EINVAL;
         }
-        else if ( sve_vl_bits > zcr_max_bits )
+
+        if ( sve_vl_bits > zcr_max_bits )
         {
             dprintk(XENLOG_INFO,
-                    "The requested SVE vector length (%u) must be lower or \n"
-                    "equal to the platform supported vector length (%u)\n",
+                    "Requested SVE vector length (%u) > supported length 
(%u)\n",
                     sve_vl_bits, zcr_max_bits);
             return -EINVAL;
         }
diff --git a/xen/arch/arm/include/asm/arm64/sve.h 
b/xen/arch/arm/include/asm/arm64/sve.h
index 8037f09b5b0a..e8c01a24e6da 100644
--- a/xen/arch/arm/include/asm/arm64/sve.h
+++ b/xen/arch/arm/include/asm/arm64/sve.h
@@ -16,7 +16,9 @@
 static inline bool is_vl_valid(unsigned int vl)
 {
     /* SVE vector length is multiple of 128 and maximum 2048 */
-    return ((vl % SVE_VL_MULTIPLE_VAL) == 0) && (vl <= SVE_VL_MAX_BITS);
+    ASSERT((vl % SVE_VL_MULTIPLE_VAL) == 0);
+
+    return vl <= SVE_VL_MAX_BITS;
 }
 
 static inline unsigned int sve_decode_vl(unsigned int sve_vl)

> 
> Jan