[PATCH] arm64/cache: Fix cache_type_cwg() for register generation

[PATCH] arm64/cache: Fix cache_type_cwg() for register generation

[Mark Brown]

Ard noticed that since we converted CTR_EL0 to automatic generation we have
been seeing errors on some systems handling the value of cache_type_cwg()
such as

   CPU features: No Cache Writeback Granule information, assuming 128

This is because the manual definition of CTR_EL0_CWG_MASK was done without
a shift while our convention is to define the mask after shifting. This
means that the user in cache_type_cwg() was broken as it was written for
the manually written shift then mask. Fix this by reversing, we don't use
SYS_FIELD_GET() since that is defined in sysreg.h which we don't want to
include in cache.h.

The only other field where the _MASK for this register is used is IminLine
which is at offset 0 so unaffected.

Fixes: 9a3634d02301 ("arm64/sysreg: Convert CTR_EL0 to automatic generation")
Reported-by: Ard Biesheuvel <ardb@kernel.org>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 arch/arm64/include/asm/cache.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/include/asm/cache.h b/arch/arm64/include/asm/cache.h
index 119e4aa02eb1..8445a3213b2f 100644
--- a/arch/arm64/include/asm/cache.h
+++ b/arch/arm64/include/asm/cache.h
@@ -67,7 +67,7 @@ static __always_inline int icache_is_vpipt(void)
 
 static inline u32 cache_type_cwg(void)
 {
-	return (read_cpuid_cachetype() >> CTR_EL0_CWG_SHIFT) & CTR_EL0_CWG_MASK;
+	return (read_cpuid_cachetype() & CTR_EL0_CWG_MASK) >> CTR_EL0_CWG_SHIFT;
 }
 
 #define __read_mostly __section(".data..read_mostly")
-- 
2.30.2


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

Re: [PATCH] arm64/cache: Fix cache_type_cwg() for register generation

[Mark Rutland]

On Wed, Aug 17, 2022 at 05:02:46PM +0100, Mark Brown wrote:
> Ard noticed that since we converted CTR_EL0 to automatic generation we have
> been seeing errors on some systems handling the value of cache_type_cwg()
> such as
> 
>    CPU features: No Cache Writeback Granule information, assuming 128
> 
> This is because the manual definition of CTR_EL0_CWG_MASK was done without
> a shift while our convention is to define the mask after shifting. This
> means that the user in cache_type_cwg() was broken as it was written for
> the manually written shift then mask. Fix this by reversing, we don't use
> SYS_FIELD_GET() since that is defined in sysreg.h which we don't want to
> include in cache.h.
> 
> The only other field where the _MASK for this register is used is IminLine
> which is at offset 0 so unaffected.
> 
> Fixes: 9a3634d02301 ("arm64/sysreg: Convert CTR_EL0 to automatic generation")
> Reported-by: Ard Biesheuvel <ardb@kernel.org>
> Signed-off-by: Mark Brown <broonie@kernel.org>
> ---
>  arch/arm64/include/asm/cache.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/include/asm/cache.h b/arch/arm64/include/asm/cache.h
> index 119e4aa02eb1..8445a3213b2f 100644
> --- a/arch/arm64/include/asm/cache.h
> +++ b/arch/arm64/include/asm/cache.h
> @@ -67,7 +67,7 @@ static __always_inline int icache_is_vpipt(void)
>  
>  static inline u32 cache_type_cwg(void)
>  {
> -	return (read_cpuid_cachetype() >> CTR_EL0_CWG_SHIFT) & CTR_EL0_CWG_MASK;
> +	return (read_cpuid_cachetype() & CTR_EL0_CWG_MASK) >> CTR_EL0_CWG_SHIFT;
>  }

Can we follow the example of:

  5b345e39d3ebc213 ("arm64/sysreg: Standardise naming for CTR_EL0 fields")

... and instead have:

| #define CTR_CWG(ctr)          SYS_FIELD_GET(CTR_EL0, CWG, ctr)

... and:

| static inline u32 cache_type_cwg(void)
| {
| 	return CTR_CWG(read_cpuid_cachetype());
| }

... since AFAICT we had SYS_FIELD_GET() available in commit 

  9a3634d02301 ("arm64/sysreg: Convert CTR_EL0 to automatic generation")

... and then it's all consistent and there's no confusion about how to apply
MASK and SHIFT.

With that:

  Acked-by: Mark Rutland <mark.rutland@arm.com>

Thanks,
Mark.

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

Re: [PATCH] arm64/cache: Fix cache_type_cwg() for register generation

[Mark Rutland]

On Wed, Aug 17, 2022 at 05:56:24PM +0100, Mark Rutland wrote:
> On Wed, Aug 17, 2022 at 05:02:46PM +0100, Mark Brown wrote:
> > Ard noticed that since we converted CTR_EL0 to automatic generation we have
> > been seeing errors on some systems handling the value of cache_type_cwg()
> > such as
> > 
> >    CPU features: No Cache Writeback Granule information, assuming 128
> > 
> > This is because the manual definition of CTR_EL0_CWG_MASK was done without
> > a shift while our convention is to define the mask after shifting. This
> > means that the user in cache_type_cwg() was broken as it was written for
> > the manually written shift then mask. Fix this by reversing, we don't use
> > SYS_FIELD_GET() since that is defined in sysreg.h which we don't want to
> > include in cache.h.
> > 
> > The only other field where the _MASK for this register is used is IminLine
> > which is at offset 0 so unaffected.
> > 
> > Fixes: 9a3634d02301 ("arm64/sysreg: Convert CTR_EL0 to automatic generation")
> > Reported-by: Ard Biesheuvel <ardb@kernel.org>
> > Signed-off-by: Mark Brown <broonie@kernel.org>
> > ---
> >  arch/arm64/include/asm/cache.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/arch/arm64/include/asm/cache.h b/arch/arm64/include/asm/cache.h
> > index 119e4aa02eb1..8445a3213b2f 100644
> > --- a/arch/arm64/include/asm/cache.h
> > +++ b/arch/arm64/include/asm/cache.h
> > @@ -67,7 +67,7 @@ static __always_inline int icache_is_vpipt(void)
> >  
> >  static inline u32 cache_type_cwg(void)
> >  {
> > -	return (read_cpuid_cachetype() >> CTR_EL0_CWG_SHIFT) & CTR_EL0_CWG_MASK;
> > +	return (read_cpuid_cachetype() & CTR_EL0_CWG_MASK) >> CTR_EL0_CWG_SHIFT;
> >  }
> 
> Can we follow the example of:
> 
>   5b345e39d3ebc213 ("arm64/sysreg: Standardise naming for CTR_EL0 fields")
> 
> ... and instead have:
> 
> | #define CTR_CWG(ctr)          SYS_FIELD_GET(CTR_EL0, CWG, ctr)
> 
> ... and:
> 
> | static inline u32 cache_type_cwg(void)
> | {
> | 	return CTR_CWG(read_cpuid_cachetype());
> | }
> 
> ... since AFAICT we had SYS_FIELD_GET() available in commit 
> 
>   9a3634d02301 ("arm64/sysreg: Convert CTR_EL0 to automatic generation")
> 
> ... and then it's all consistent and there's no confusion about how to apply
> MASK and SHIFT.
> 
> With that:
> 
>   Acked-by: Mark Rutland <mark.rutland@arm.com>

Ah; I see that blows up because we're missing an include for
<linux/bitfield.h>. We'll need to fix sysreg.h too, e.g. as below.

Mark.

---->8----
diff --git a/arch/arm64/include/asm/cache.h b/arch/arm64/include/asm/cache.h
index ca9b487112ccb..794fe2652cded 100644
--- a/arch/arm64/include/asm/cache.h
+++ b/arch/arm64/include/asm/cache.h
@@ -50,6 +50,7 @@ static inline unsigned int arch_slab_minalign(void)
         CTR_EL0_IMINLINE_MASK << CTR_EL0_IMINLINE_SHIFT)
 
 #define CTR_L1IP(ctr)          SYS_FIELD_GET(CTR_EL0, L1Ip, ctr)
+#define CTR_CWG(ctr)           SYS_FIELD_GET(CTR_EL0, CWG, ctr)
 
 #define ICACHEF_ALIASING       0
 #define ICACHEF_VPIPT          1
@@ -71,7 +72,7 @@ static __always_inline int icache_is_vpipt(void)
 
 static inline u32 cache_type_cwg(void)
 {
-       return (read_cpuid_cachetype() >> CTR_EL0_CWG_SHIFT) & CTR_EL0_CWG_MASK;
+       return CTR_CWG(read_cpuid_cachetype());
 }
 
 #define __read_mostly __section(".data..read_mostly")
diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
index 7c71358d44c4a..7d806d1769d84 100644
--- a/arch/arm64/include/asm/sysreg.h
+++ b/arch/arm64/include/asm/sysreg.h
@@ -1116,6 +1116,7 @@
 
 #else
 
+#include <linux/bitfield.h>
 #include <linux/build_bug.h>
 #include <linux/types.h>
 #include <asm/alternative.h>
@@ -1209,8 +1210,6 @@
        par;                                                            \
 })
 
-#endif
-
 #define SYS_FIELD_GET(reg, field, val)         \
                 FIELD_GET(reg##_##field##_MASK, val)
 
@@ -1220,4 +1219,6 @@
 #define SYS_FIELD_PREP_ENUM(reg, field, val)           \
                 FIELD_PREP(reg##_##field##_MASK, reg##_##field##_##val)
 
+#endif /* __ASSEMBLY__ */
+
 #endif /* __ASM_SYSREG_H */

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

Re: [PATCH] arm64/cache: Fix cache_type_cwg() for register generation

[Mark Brown]

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

On Wed, Aug 17, 2022 at 05:56:24PM +0100, Mark Rutland wrote:

>   5b345e39d3ebc213 ("arm64/sysreg: Standardise naming for CTR_EL0 fields")

> ... and instead have:

> | #define CTR_CWG(ctr)          SYS_FIELD_GET(CTR_EL0, CWG, ctr)

I think if we're going to define that sort of per bitfield accessor
macro (which is certainly a valid and reasonable thing to do) we should
be having the script generate them rather than open coding them but
that's getting out of scope for a fix and should be done separately.

[-- 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/cache: Fix cache_type_cwg() for register generation

[Mark Rutland]

On Wed, Aug 17, 2022 at 07:22:26PM +0100, Mark Brown wrote:
> On Wed, Aug 17, 2022 at 05:56:24PM +0100, Mark Rutland wrote:
> 
> >   5b345e39d3ebc213 ("arm64/sysreg: Standardise naming for CTR_EL0 fields")
> 
> > ... and instead have:
> 
> > | #define CTR_CWG(ctr)          SYS_FIELD_GET(CTR_EL0, CWG, ctr)
> 
> I think if we're going to define that sort of per bitfield accessor
> macro (which is certainly a valid and reasonable thing to do) we should
> be having the script generate them rather than open coding them but
> that's getting out of scope for a fix and should be done separately.

I'm not asking for us to do that for *every* bitfield accessor, I'm just asking
for us to be locally consistent within cache.h.

I'm also happy to use SYS_FIELD_GET() directly within cache_type_cwg(), and not
define CTR_CWG().

Thanks,
Mark.

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

Re: [PATCH] arm64/cache: Fix cache_type_cwg() for register generation

[Mark Brown]

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

On Thu, Aug 18, 2022 at 02:20:18PM +0100, Mark Rutland wrote:
> On Wed, Aug 17, 2022 at 07:22:26PM +0100, Mark Brown wrote:
> > On Wed, Aug 17, 2022 at 05:56:24PM +0100, Mark Rutland wrote:

> > > | #define CTR_CWG(ctr)          SYS_FIELD_GET(CTR_EL0, CWG, ctr)

> > I think if we're going to define that sort of per bitfield accessor
> > macro (which is certainly a valid and reasonable thing to do) we should
> > be having the script generate them rather than open coding them but
> > that's getting out of scope for a fix and should be done separately.

> I'm not asking for us to do that for *every* bitfield accessor, I'm just asking
> for us to be locally consistent within cache.h.

If we're going to introduce that sort of rule it should probably be a
general thing rather than a per header thing, the CTR_L1IP() thing was
a preexisting thing that was just converted in place rather than a style
we were trying to say was a good idea that should be replicated.

> I'm also happy to use SYS_FIELD_GET() directly within cache_type_cwg(), and not
> define CTR_CWG().

Sure, that's less problematic and indeed currently going through my
testing.

[-- 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/cache: Fix cache_type_cwg() for register generation

[Mark Rutland]

On Thu, Aug 18, 2022 at 02:33:02PM +0100, Mark Brown wrote:
> On Thu, Aug 18, 2022 at 02:20:18PM +0100, Mark Rutland wrote:
> > I'm also happy to use SYS_FIELD_GET() directly within cache_type_cwg(), and not
> > define CTR_CWG().
> 
> Sure, that's less problematic and indeed currently going through my
> testing.

Perfect, thanks!

Mark.

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