* [PATCH] arm64/cache: Fix cache_type_cwg() for register generation
@ 2022-08-17 16:02 Mark Brown
2022-08-17 16:56 ` Mark Rutland
0 siblings, 1 reply; 7+ messages in thread
From: Mark Brown @ 2022-08-17 16:02 UTC (permalink / raw)
To: Will Deacon, Catalin Marinas; +Cc: linux-arm-kernel, Mark Brown, Ard Biesheuvel
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] arm64/cache: Fix cache_type_cwg() for register generation
2022-08-17 16:02 [PATCH] arm64/cache: Fix cache_type_cwg() for register generation Mark Brown
@ 2022-08-17 16:56 ` Mark Rutland
2022-08-17 17:25 ` Mark Rutland
2022-08-17 18:22 ` Mark Brown
0 siblings, 2 replies; 7+ messages in thread
From: Mark Rutland @ 2022-08-17 16:56 UTC (permalink / raw)
To: Mark Brown; +Cc: Will Deacon, Catalin Marinas, linux-arm-kernel, Ard Biesheuvel
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] arm64/cache: Fix cache_type_cwg() for register generation
2022-08-17 16:56 ` Mark Rutland
@ 2022-08-17 17:25 ` Mark Rutland
2022-08-17 18:22 ` Mark Brown
1 sibling, 0 replies; 7+ messages in thread
From: Mark Rutland @ 2022-08-17 17:25 UTC (permalink / raw)
To: Mark Brown; +Cc: Will Deacon, Catalin Marinas, linux-arm-kernel, Ard Biesheuvel
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] arm64/cache: Fix cache_type_cwg() for register generation
2022-08-17 16:56 ` Mark Rutland
2022-08-17 17:25 ` Mark Rutland
@ 2022-08-17 18:22 ` Mark Brown
2022-08-18 13:20 ` Mark Rutland
1 sibling, 1 reply; 7+ messages in thread
From: Mark Brown @ 2022-08-17 18:22 UTC (permalink / raw)
To: Mark Rutland
Cc: Will Deacon, Catalin Marinas, linux-arm-kernel, Ard Biesheuvel
[-- 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] arm64/cache: Fix cache_type_cwg() for register generation
2022-08-17 18:22 ` Mark Brown
@ 2022-08-18 13:20 ` Mark Rutland
2022-08-18 13:33 ` Mark Brown
0 siblings, 1 reply; 7+ messages in thread
From: Mark Rutland @ 2022-08-18 13:20 UTC (permalink / raw)
To: Mark Brown; +Cc: Will Deacon, Catalin Marinas, linux-arm-kernel, Ard Biesheuvel
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] arm64/cache: Fix cache_type_cwg() for register generation
2022-08-18 13:20 ` Mark Rutland
@ 2022-08-18 13:33 ` Mark Brown
2022-08-18 13:40 ` Mark Rutland
0 siblings, 1 reply; 7+ messages in thread
From: Mark Brown @ 2022-08-18 13:33 UTC (permalink / raw)
To: Mark Rutland
Cc: Will Deacon, Catalin Marinas, linux-arm-kernel, Ard Biesheuvel
[-- 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] arm64/cache: Fix cache_type_cwg() for register generation
2022-08-18 13:33 ` Mark Brown
@ 2022-08-18 13:40 ` Mark Rutland
0 siblings, 0 replies; 7+ messages in thread
From: Mark Rutland @ 2022-08-18 13:40 UTC (permalink / raw)
To: Mark Brown; +Cc: Will Deacon, Catalin Marinas, linux-arm-kernel, Ard Biesheuvel
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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-08-18 13:50 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-17 16:02 [PATCH] arm64/cache: Fix cache_type_cwg() for register generation Mark Brown
2022-08-17 16:56 ` Mark Rutland
2022-08-17 17:25 ` Mark Rutland
2022-08-17 18:22 ` Mark Brown
2022-08-18 13:20 ` Mark Rutland
2022-08-18 13:33 ` Mark Brown
2022-08-18 13:40 ` Mark Rutland
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.