Hi Jan,
Thank you for your review, very appreciated,
> On 28 Mar 2023, at 10:36, Jan Beulich <jbeul...@suse.com> 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
