[PATCH] arm64/fpsimd: Only provide the length to cpufeature for xCR registers

[PATCH] arm64/fpsimd: Only provide the length to cpufeature for xCR registers

[Mark Brown]

For both SVE and SME we abuse the generic register field comparison
support in the cpufeature code as part of our detection of unsupported
variations in the vector lengths available to PEs, reporting the maximum
vector lengths via ZCR_EL1.LEN and SMCR_EL1.LEN.  Since these are
configuration registers rather than identification registers the
assumptions the cpufeature code makes about how unknown bitfields behave
are invalid, leading to warnings when SME features like FA64 are enabled
and we hotplug a CPU:

  CPU features: SANITY CHECK: Unexpected variation in SYS_SMCR_EL1. Boot CPU: 0x0000000000000f, CPU3: 0x0000008000000f
  CPU features: Unsupported CPU feature variation detected.

SVE has no controls other than the vector length so is not yet impacted
but the same issue will apply there if any are defined.

Since the only field we are interested in having the cpufeature code
handle is the length field and we use a custom read function to obtain
the value we can avoid these warnings by filtering out all other bits
when we return the register value.

Fixes: 2e0f2478ea37eb ("arm64/sve: Probe SVE capabilities and usable vector lengths")
FixeS: b42990d3bf77cc ("arm64/sme: Identify supported SME vector lengths at boot")
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 arch/arm64/kernel/fpsimd.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
index 89d54a5242d1..c7fdeebd050c 100644
--- a/arch/arm64/kernel/fpsimd.c
+++ b/arch/arm64/kernel/fpsimd.c
@@ -1189,11 +1189,11 @@ u64 read_zcr_features(void)
 	write_sysreg_s(ZCR_ELx_LEN_MASK, SYS_ZCR_EL1);
 
 	zcr = read_sysreg_s(SYS_ZCR_EL1);
-	zcr &= ~(u64)ZCR_ELx_LEN_MASK; /* find sticky 1s outside LEN field */
+	zcr &= ~(u64)ZCR_ELx_LEN_MASK;
 	vq_max = sve_vq_from_vl(sve_get_vl());
 	zcr |= vq_max - 1; /* set LEN field to maximum effective value */
 
-	return zcr;
+	return SYS_FIELD_GET(ZCR_ELx, LEN, zcr);
 }
 
 void __init sve_setup(void)
@@ -1364,7 +1364,7 @@ u64 read_smcr_features(void)
 	vq_max = sve_vq_from_vl(sme_get_vl());
 	smcr |= vq_max - 1; /* set LEN field to maximum effective value */
 
-	return smcr;
+	return SYS_FIELD_GET(SMCR_ELx, LEN, smcr);
 }
 
 void __init sme_setup(void)

---
base-commit: 6eaae198076080886b9e7d57f4ae06fa782f90ef
change-id: 20230727-arm64-sme-fa64-hotplug-1e6896746f97

Best regards,
-- 
Mark Brown <broonie@kernel.org>


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64/fpsimd: Only provide the length to cpufeature for xCR registers

[Jonathan Cameron]

On Thu, 27 Jul 2023 22:31:44 +0100
Mark Brown <broonie@kernel.org> wrote:

> For both SVE and SME we abuse the generic register field comparison
> support in the cpufeature code as part of our detection of unsupported
> variations in the vector lengths available to PEs, reporting the maximum
> vector lengths via ZCR_EL1.LEN and SMCR_EL1.LEN.  Since these are
> configuration registers rather than identification registers the
> assumptions the cpufeature code makes about how unknown bitfields behave
> are invalid, leading to warnings when SME features like FA64 are enabled
> and we hotplug a CPU:
> 
>   CPU features: SANITY CHECK: Unexpected variation in SYS_SMCR_EL1. Boot CPU: 0x0000000000000f, CPU3: 0x0000008000000f
>   CPU features: Unsupported CPU feature variation detected.
> 
> SVE has no controls other than the vector length so is not yet impacted
> but the same issue will apply there if any are defined.
> 
> Since the only field we are interested in having the cpufeature code
> handle is the length field and we use a custom read function to obtain
> the value we can avoid these warnings by filtering out all other bits
> when we return the register value.
> 
> Fixes: 2e0f2478ea37eb ("arm64/sve: Probe SVE capabilities and usable vector lengths")
> FixeS: b42990d3bf77cc ("arm64/sme: Identify supported SME vector lengths at boot")

Fixes

> Signed-off-by: Mark Brown <broonie@kernel.org>


> ---
>  arch/arm64/kernel/fpsimd.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
> index 89d54a5242d1..c7fdeebd050c 100644
> --- a/arch/arm64/kernel/fpsimd.c
> +++ b/arch/arm64/kernel/fpsimd.c
> @@ -1189,11 +1189,11 @@ u64 read_zcr_features(void)
>  	write_sysreg_s(ZCR_ELx_LEN_MASK, SYS_ZCR_EL1);
>  
>  	zcr = read_sysreg_s(SYS_ZCR_EL1);
> -	zcr &= ~(u64)ZCR_ELx_LEN_MASK; /* find sticky 1s outside LEN field */
> +	zcr &= ~(u64)ZCR_ELx_LEN_MASK;
>  	vq_max = sve_vq_from_vl(sve_get_vl());
>  	zcr |= vq_max - 1; /* set LEN field to maximum effective value */
>  
> -	return zcr;
> +	return SYS_FIELD_GET(ZCR_ELx, LEN, zcr);

Isn't that overly complex if we only end up with the length? (if I'm reading this right)
Perhaps it is more logical to build the register then pull the
field out of it, but it would be simpler as something like...

	return sve_vq_from_vl(sve_get_vl()) - 1;

>  }
>  
>  void __init sve_setup(void)
> @@ -1364,7 +1364,7 @@ u64 read_smcr_features(void)
>  	vq_max = sve_vq_from_vl(sme_get_vl());
>  	smcr |= vq_max - 1; /* set LEN field to maximum effective value */
>  
> -	return smcr;
> +	return SYS_FIELD_GET(SMCR_ELx, LEN, smcr);
>  }
>  
>  void __init sme_setup(void)
> 
> ---
> base-commit: 6eaae198076080886b9e7d57f4ae06fa782f90ef
> change-id: 20230727-arm64-sme-fa64-hotplug-1e6896746f97
> 
> Best regards,


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64/fpsimd: Only provide the length to cpufeature for xCR registers

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 861 bytes --]

On Fri, Jul 28, 2023 at 11:27:20AM +0100, Jonathan Cameron wrote:
> Mark Brown <broonie@kernel.org> wrote:

> >  	zcr = read_sysreg_s(SYS_ZCR_EL1);
> > -	zcr &= ~(u64)ZCR_ELx_LEN_MASK; /* find sticky 1s outside LEN field */
> > +	zcr &= ~(u64)ZCR_ELx_LEN_MASK;
> >  	vq_max = sve_vq_from_vl(sve_get_vl());
> >  	zcr |= vq_max - 1; /* set LEN field to maximum effective value */

> > -	return zcr;
> > +	return SYS_FIELD_GET(ZCR_ELx, LEN, zcr);

> Isn't that overly complex if we only end up with the length? (if I'm reading this right)
> Perhaps it is more logical to build the register then pull the
> field out of it, but it would be simpler as something like...

> 	return sve_vq_from_vl(sve_get_vl()) - 1;

We could, yes - I did prefer to keep it clear that this is an actual
if modified register value we're returning, though that could've been a
comment.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64/fpsimd: Only provide the length to cpufeature for xCR registers

[Will Deacon]

On Thu, Jul 27, 2023 at 10:31:44PM +0100, Mark Brown wrote:
> For both SVE and SME we abuse the generic register field comparison
> support in the cpufeature code as part of our detection of unsupported
> variations in the vector lengths available to PEs, reporting the maximum
> vector lengths via ZCR_EL1.LEN and SMCR_EL1.LEN.  Since these are
> configuration registers rather than identification registers the
> assumptions the cpufeature code makes about how unknown bitfields behave
> are invalid, leading to warnings when SME features like FA64 are enabled
> and we hotplug a CPU:
> 
>   CPU features: SANITY CHECK: Unexpected variation in SYS_SMCR_EL1. Boot CPU: 0x0000000000000f, CPU3: 0x0000008000000f
>   CPU features: Unsupported CPU feature variation detected.
> 
> SVE has no controls other than the vector length so is not yet impacted
> but the same issue will apply there if any are defined.
> 
> Since the only field we are interested in having the cpufeature code
> handle is the length field and we use a custom read function to obtain
> the value we can avoid these warnings by filtering out all other bits
> when we return the register value.
> 
> Fixes: 2e0f2478ea37eb ("arm64/sve: Probe SVE capabilities and usable vector lengths")
> FixeS: b42990d3bf77cc ("arm64/sme: Identify supported SME vector lengths at boot")
> Signed-off-by: Mark Brown <broonie@kernel.org>
> ---
>  arch/arm64/kernel/fpsimd.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
> index 89d54a5242d1..c7fdeebd050c 100644
> --- a/arch/arm64/kernel/fpsimd.c
> +++ b/arch/arm64/kernel/fpsimd.c
> @@ -1189,11 +1189,11 @@ u64 read_zcr_features(void)
>  	write_sysreg_s(ZCR_ELx_LEN_MASK, SYS_ZCR_EL1);
>  
>  	zcr = read_sysreg_s(SYS_ZCR_EL1);
> -	zcr &= ~(u64)ZCR_ELx_LEN_MASK; /* find sticky 1s outside LEN field */
> +	zcr &= ~(u64)ZCR_ELx_LEN_MASK;
>  	vq_max = sve_vq_from_vl(sve_get_vl());
>  	zcr |= vq_max - 1; /* set LEN field to maximum effective value */
>  
> -	return zcr;
> +	return SYS_FIELD_GET(ZCR_ELx, LEN, zcr);

Hmm, now this function looks like a mixture of code which relies on the
LEN field living at the bottom of the register and code which is agnostic
to that.

Can we update the 'zcr |= vq_max - 1' part to use something like
FIELD_PREP() instead?

>  }
>  
>  void __init sve_setup(void)
> @@ -1364,7 +1364,7 @@ u64 read_smcr_features(void)
>  	vq_max = sve_vq_from_vl(sme_get_vl());
>  	smcr |= vq_max - 1; /* set LEN field to maximum effective value */
>  
> -	return smcr;
> +	return SYS_FIELD_GET(SMCR_ELx, LEN, smcr);

It looks like there's a similar thing here.

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64/fpsimd: Only provide the length to cpufeature for xCR registers

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 705 bytes --]

On Wed, Aug 02, 2023 at 12:21:23PM +0100, Will Deacon wrote:
> On Thu, Jul 27, 2023 at 10:31:44PM +0100, Mark Brown wrote:

> > -	return zcr;
> > +	return SYS_FIELD_GET(ZCR_ELx, LEN, zcr);

> Hmm, now this function looks like a mixture of code which relies on the
> LEN field living at the bottom of the register and code which is agnostic
> to that.

> Can we update the 'zcr |= vq_max - 1' part to use something like
> FIELD_PREP() instead?

There was a version 2 that was sent already which goes in the opposite
direction and just returns the value we would munge in without use of
any FIELD_ macros:

   https://lore.kernel.org/r/20230731-arm64-sme-fa64-hotplug-v2-1

which also addresses your issue?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel