arm64: fix typo in I-cache policy detection

PATCH New Ard Biesheuvel 2 participants 9 messages Aug 5, 2014 – Aug 6, 2014 arm64

lore

Share & tools

lore

Ard Biesheuvel

author

2014-08-05 09:25 UTC #0 lore raw

PATCH v2 1/3 arm64: fix typo in I-cache policy detection

This removes an unfortunately placed semi-colon resulting in all instruction caches being classified as AIVIVT.

Signed-off-by Ard Biesheuvel <[email protected]>

Acked-by Mark Rutland <[email protected]>

---
v2: add ack

 arch/arm64/kernel/cpuinfo.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

arch/arm64/kernel/cpuinfo.c +1 -1

index f798f66634af..177169623026 100644
@@ -49,7 +49,7 @@ static void cpuinfo_detect_icache_policy(struct cpuinfo_arm64 *info)
 
 	if (l1ip != ICACHE_POLICY_PIPT)
 		set_bit(ICACHEF_ALIASING, &__icache_flags);
-	if (l1ip == ICACHE_POLICY_AIVIVT);
+	if (l1ip == ICACHE_POLICY_AIVIVT)
 		set_bit(ICACHEF_AIVIVT, &__icache_flags);
 
 	pr_info("Detected %s I-cache on CPU%d\n", icache_policy_str[l1ip], cpu);

Signature · 1 line

1.8.3.2

Ard Biesheuvel

author

2014-08-05 09:25 UTC #1 lore raw

PATCH v2 2/3 arm64: add helper functions to read I-cache attributes

This adds helper functions and #defines to <asm/cachetype.h> to read the line size and the number of sets from the level 1 instruction cache.

Signed-off-by Ard Biesheuvel <[email protected]>

---
v2: put () around macro args, use 64-bit types for asm() mrs/msr calls

 arch/arm64/include/asm/cachetype.h | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

arch/arm64/include/asm/cachetype.h +28 -0

index 7a2e0762cb40..e59c0c25b307 100644
@@ -39,6 +39,34 @@
 
 extern unsigned long __icache_flags;
 
+#define CCSIDR_EL1_LINESIZE_MASK	0x7
+#define CCSIDR_EL1_LINESIZE(x)		((x) & CCSIDR_EL1_LINESIZE_MASK)
+
+#define CCSIDR_EL1_NUMSETS_SHIFT	13
+#define CCSIDR_EL1_NUMSETS_MASK		(0x7fff << CCSIDR_EL1_NUMSETS_SHIFT)
+#define CCSIDR_EL1_NUMSETS(x) \
+	(((x) & CCSIDR_EL1_NUMSETS_MASK) >> CCSIDR_EL1_NUMSETS_SHIFT)
+
+static inline __attribute_const__ u64 icache_get_ccsidr(void)
+{
+	u64 ccsidr;
+
+	/* Select L1 I-cache and read its size ID register */
+	asm("msr csselr_el1, %1; isb; mrs %0, ccsidr_el1"
+	    : "=r"(ccsidr) : "r"(1L));
+	return ccsidr;
+}
+
+static inline int icache_get_linesize(void)
+{
+	return 16 << CCSIDR_EL1_LINESIZE(icache_get_ccsidr());
+}
+
+static inline int icache_get_numsets(void)
+{
+	return 1 + CCSIDR_EL1_NUMSETS(icache_get_ccsidr());
+}
+
 /*
  * Whilst the D-side always behaves as PIPT on AArch64, aliasing is
  * permitted in the I-cache.

Signature · 1 line

1.8.3.2

Will Deacon

2014-08-06 13:00 UTC #2 lore raw

PATCH v2 2/3 arm64: add helper functions to read I-cache attributes

Quoted 24 lines · Ard Biesheuvel

This adds helper functions and #defines to <asm/cachetype.h> to read the line size and the number of sets from the level 1 instruction cache.
Signed-off-by Ard Biesheuvel <[email protected]>
---
v2: put () around macro args, use 64-bit types for asm() mrs/msr calls
 arch/arm64/include/asm/cachetype.h | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)
arch/arm64/include/asm/cachetype.h +6 -0
index 7a2e0762cb40..e59c0c25b307 100644
@@ -39,6 +39,34 @@
 
 extern unsigned long __icache_flags;
 
+#define CCSIDR_EL1_LINESIZE_MASK	0x7
+#define CCSIDR_EL1_LINESIZE(x)		((x) & CCSIDR_EL1_LINESIZE_MASK)
+
+#define CCSIDR_EL1_NUMSETS_SHIFT	13
+#define CCSIDR_EL1_NUMSETS_MASK		(0x7fff << CCSIDR_EL1_NUMSETS_SHIFT)
+#define CCSIDR_EL1_NUMSETS(x) \

arch/arm64/include/asm/cachetype.h line 57

Quoted 10 lines

arch/arm64/include/asm/cachetype.h +10 -0

+	(((x) & CCSIDR_EL1_NUMSETS_MASK) >> CCSIDR_EL1_NUMSETS_SHIFT)
+
+static inline __attribute_const__ u64 icache_get_ccsidr(void)
+{
+	u64 ccsidr;
+
+	/* Select L1 I-cache and read its size ID register */
+	asm("msr csselr_el1, %1; isb; mrs %0, ccsidr_el1"
+	    : "=r"(ccsidr) : "r"(1L));
+	return ccsidr;

Is it worth having a WARN_ON(preemptible()) here?

Will

Ard Biesheuvel

author

2014-08-06 13:17 UTC #3 lore raw

PATCH v2 2/3 arm64: add helper functions to read I-cache attributes

Quoted 29 lines · Will Deacon

Quoted 28 lines · Ard Biesheuvel
This adds helper functions and #defines to <asm/cachetype.h> to read the line size and the number of sets from the level 1 instruction cache.
Signed-off-by Ard Biesheuvel <[email protected]>
---
v2: put () around macro args, use 64-bit types for asm() mrs/msr calls
 arch/arm64/include/asm/cachetype.h | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)
arch/arm64/include/asm/cachetype.h +10 -0
index 7a2e0762cb40..e59c0c25b307 100644
@@ -39,6 +39,34 @@
 
 extern unsigned long __icache_flags;
 
+#define CCSIDR_EL1_LINESIZE_MASK     0x7
+#define CCSIDR_EL1_LINESIZE(x)               ((x) & CCSIDR_EL1_LINESIZE_MASK)
+
+#define CCSIDR_EL1_NUMSETS_SHIFT     13
+#define CCSIDR_EL1_NUMSETS_MASK              (0x7fff << CCSIDR_EL1_NUMSETS_SHIFT)
+#define CCSIDR_EL1_NUMSETS(x) \
+     (((x) & CCSIDR_EL1_NUMSETS_MASK) >> CCSIDR_EL1_NUMSETS_SHIFT)
+
+static inline __attribute_const__ u64 icache_get_ccsidr(void)
+{

Quoted 9 lines

arch/arm64/include/asm/cachetype.h line 57
Quoted 6 lines
arch/arm64/include/asm/cachetype.h +6 -0
+     u64 ccsidr;
+
+     /* Select L1 I-cache and read its size ID register */
+     asm("msr csselr_el1, %1; isb; mrs %0, ccsidr_el1"
+         : "=r"(ccsidr) : "r"(1L));
+     return ccsidr;
Is it worth having a WARN_ON(preemptible()) here?

Sure, why not.

Ard Biesheuvel

author

2014-08-06 13:27 UTC #4 lore raw

PATCH v2 2/3 arm64: add helper functions to read I-cache attributes

Quoted 40 lines · Ard Biesheuvel

Quoted 29 lines · Will Deacon
Quoted 28 lines · Ard Biesheuvel
This adds helper functions and #defines to <asm/cachetype.h> to read the line size and the number of sets from the level 1 instruction cache.
Signed-off-by Ard Biesheuvel <[email protected]>
---
v2: put () around macro args, use 64-bit types for asm() mrs/msr calls
 arch/arm64/include/asm/cachetype.h | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)
arch/arm64/include/asm/cachetype.h +10 -0
index 7a2e0762cb40..e59c0c25b307 100644
@@ -39,6 +39,34 @@
 
 extern unsigned long __icache_flags;
 
+#define CCSIDR_EL1_LINESIZE_MASK     0x7
+#define CCSIDR_EL1_LINESIZE(x)               ((x) & CCSIDR_EL1_LINESIZE_MASK)
+
+#define CCSIDR_EL1_NUMSETS_SHIFT     13
+#define CCSIDR_EL1_NUMSETS_MASK              (0x7fff << CCSIDR_EL1_NUMSETS_SHIFT)
+#define CCSIDR_EL1_NUMSETS(x) \
+     (((x) & CCSIDR_EL1_NUMSETS_MASK) >> CCSIDR_EL1_NUMSETS_SHIFT)
+
+static inline __attribute_const__ u64 icache_get_ccsidr(void)
+{
Quoted 9 lines
arch/arm64/include/asm/cachetype.h line 57
Quoted 6 lines
arch/arm64/include/asm/cachetype.h +6 -0
+     u64 ccsidr;
+
+     /* Select L1 I-cache and read its size ID register */
+     asm("msr csselr_el1, %1; isb; mrs %0, ccsidr_el1"
+         : "=r"(ccsidr) : "r"(1L));
+     return ccsidr;
Is it worth having a WARN_ON(preemptible()) here?

Quoted 1 line

Sure, why not.

... if it weren't for the fact that this triggers recursive header inclusion hell

CC kernel/bounds.s
In file included from /home/ard/linux-2.6/include/asm-generic/preempt.h:4:0,

                 from arch/arm64/include/generated/asm/preempt.h:1,
                 from /home/ard/linux-2.6/include/linux/preempt.h:18,
                 from /home/ard/linux-2.6/arch/arm64/include/asm/cachetype.h:21,
                 from /home/ard/linux-2.6/arch/arm64/include/asm/cache.h:19,
                 from /home/ard/linux-2.6/include/linux/cache.h:5,
                 from /home/ard/linux-2.6/include/linux/printk.h:8,
                 from /home/ard/linux-2.6/include/linux/kernel.h:13,
                 from /home/ard/linux-2.6/include/asm-generic/bug.h:13,
                 from arch/arm64/include/generated/asm/bug.h:1,
                 from /home/ard/linux-2.6/include/linux/bug.h:4,
                 from /home/ard/linux-2.6/include/linux/page-flags.h:9,
                 from /home/ard/linux-2.6/kernel/bounds.c:9:

/home/ard/linux-2.6/include/linux/thread_info.h: In function ?set_restore_sigmask?:
/home/ard/linux-2.6/include/linux/thread_info.h:128:2: error: implicit declaration of function ?WARN_ON? [-Werror=implicit-function-declaration]
WARN_ON(!test_thread_flag(TIF_SIGPENDING));
^
In file included from /home/ard/linux-2.6/arch/arm64/include/asm/cache.h:19:0,

                 from /home/ard/linux-2.6/include/linux/cache.h:5,
                 from /home/ard/linux-2.6/include/linux/printk.h:8,
                 from /home/ard/linux-2.6/include/linux/kernel.h:13,
                 from /home/ard/linux-2.6/include/asm-generic/bug.h:13,
                 from arch/arm64/include/generated/asm/bug.h:1,
                 from /home/ard/linux-2.6/include/linux/bug.h:4,
                 from /home/ard/linux-2.6/include/linux/page-flags.h:9,
                 from /home/ard/linux-2.6/kernel/bounds.c:9:

/home/ard/linux-2.6/arch/arm64/include/asm/cachetype.h: In function ?icache_get_ccsidr?:
/home/ard/linux-2.6/arch/arm64/include/asm/cachetype.h:59:2: error:
implicit declaration of function ?preemptible? [-Werror=implicit-function-declaration]
WARN_ON(preemptible());
^

i.e., linux/bug,h and linux/preempt.h already implicitly #include cachetype.h, so including the former from the latter to import the declaration of WARN_ON() and/or preemptible respectively produces this error.

Signature · 1 line

Ard.

Will Deacon

2014-08-06 14:34 UTC #5 lore raw

PATCH v2 2/3 arm64: add helper functions to read I-cache attributes

Quoted 8 lines · Ard Biesheuvel

Quoted 7 lines · Ard Biesheuvel
Quoted 6 lines · Will Deacon
Quoted 5 lines · Ard Biesheuvel
This adds helper functions and #defines to <asm/cachetype.h> to read the line size and the number of sets from the level 1 instruction cache.
Signed-off-by Ard Biesheuvel <[email protected]>
---

[...]

Quoted 22 lines

Quoted 12 lines
Quoted 11 lines
arch/arm64/include/asm/cachetype.h line 57
Quoted 8 lines
arch/arm64/include/asm/cachetype.h +8 -0
+static inline __attribute_const__ u64 icache_get_ccsidr(void)
+{
+     u64 ccsidr;
+
+     /* Select L1 I-cache and read its size ID register */
+     asm("msr csselr_el1, %1; isb; mrs %0, ccsidr_el1"
+         : "=r"(ccsidr) : "r"(1L));
+     return ccsidr;
Is it worth having a WARN_ON(preemptible()) here?
Quoted 1 line
Sure, why not.
... if it weren't for the fact that this triggers recursive header inclusion hell
CC kernel/bounds.s
In file included from /home/ard/linux-2.6/include/asm-generic/preempt.h:4:0,
                 from arch/arm64/include/generated/asm/preempt.h:1,
                 from /home/ard/linux-2.6/include/linux/preempt.h:18,
                 from /home/ard/linux-2.6/arch/arm64/include/asm/cachetype.h:21,

[...]

Quoted 4 lines

i.e., linux/bug,h and linux/preempt.h already implicitly #include cachetype.h, so including the former from the latter to import the declaration of WARN_ON() and/or preemptible respectively produces this error.

Damn, that's a real shame. I'm always dubious about adding code like this which isn't obviously broken from preemptible context when you're just looking at the function name.

Ho-hum.

Will

Ard Biesheuvel

author

2014-08-06 14:38 UTC #6 lore raw

PATCH v2 2/3 arm64: add helper functions to read I-cache attributes

Quoted 42 lines · Will Deacon

Quoted 8 lines · Ard Biesheuvel
Quoted 7 lines · Ard Biesheuvel
Quoted 6 lines · Will Deacon
Quoted 5 lines · Ard Biesheuvel
This adds helper functions and #defines to <asm/cachetype.h> to read the line size and the number of sets from the level 1 instruction cache.
Signed-off-by Ard Biesheuvel <[email protected]>
---
[...]
Quoted 22 lines
Quoted 12 lines
Quoted 11 lines
Review
Quoted 8 lines
(quoted diff) +8 -0
+static inline __attribute_const__ u64 icache_get_ccsidr(void)
+{
+     u64 ccsidr;
+
+     /* Select L1 I-cache and read its size ID register */
+     asm("msr csselr_el1, %1; isb; mrs %0, ccsidr_el1"
+         : "=r"(ccsidr) : "r"(1L));
+     return ccsidr;
Is it worth having a WARN_ON(preemptible()) here?
Quoted 1 line
Sure, why not.
... if it weren't for the fact that this triggers recursive header inclusion hell
CC kernel/bounds.s
In file included from /home/ard/linux-2.6/include/asm-generic/preempt.h:4:0,
                 from arch/arm64/include/generated/asm/preempt.h:1,
                 from /home/ard/linux-2.6/include/linux/preempt.h:18,
                 from /home/ard/linux-2.6/arch/arm64/include/asm/cachetype.h:21,
[...]
Quoted 4 lines
i.e., linux/bug,h and linux/preempt.h already implicitly #include cachetype.h, so including the former from the latter to import the declaration of WARN_ON() and/or preemptible respectively produces this error.

Quoted 6 lines

Damn, that's a real shame. I'm always dubious about adding code like this which isn't obviously broken from preemptible context when you're just looking at the function name.
Ho-hum.

Moving icache_get_ccsidr() into cpuinfo.c does work, as in the patch below

(quoted diff) +0 -0

b/arch/arm64/include/asm/cachetype.h
index 7a2e0762cb40..4c631a0a3609 100644

arch/arm64/include/asm/cachetype.h +20 -0

@@ -39,6 +39,26 @@
 
 extern unsigned long __icache_flags;
 
+#define CCSIDR_EL1_LINESIZE_MASK 0x7
+#define CCSIDR_EL1_LINESIZE(x) ((x) & CCSIDR_EL1_LINESIZE_MASK)
+
+#define CCSIDR_EL1_NUMSETS_SHIFT 13
+#define CCSIDR_EL1_NUMSETS_MASK (0x7fff << CCSIDR_EL1_NUMSETS_SHIFT)
+#define CCSIDR_EL1_NUMSETS(x) \
+ (((x) & CCSIDR_EL1_NUMSETS_MASK) >> CCSIDR_EL1_NUMSETS_SHIFT)
+
+extern u64 __attribute_const__ icache_get_ccsidr(void);
+
+static inline int icache_get_linesize(void)
+{
+ return 16 << CCSIDR_EL1_LINESIZE(icache_get_ccsidr());
+}
+
+static inline int icache_get_numsets(void)
+{
+ return 1 + CCSIDR_EL1_NUMSETS(icache_get_ccsidr());
+}
+
 /*
  * Whilst the D-side always behaves as PIPT on AArch64, aliasing is
  * permitted in the I-cache.

arch/arm64/kernel/cpuinfo.c +14 -0

index f798f66634af..319255ff536d 100644
@@ -20,8 +20,10 @@
 #include <asm/cputype.h>
 
 #include <linux/bitops.h>
+#include <linux/bug.h>
 #include <linux/init.h>
 #include <linux/kernel.h>
+#include <linux/preempt.h>
 #include <linux/printk.h>
 #include <linux/smp.h>

@@ -190,3 +192,15 @@ void __init cpuinfo_store_boot_cpu(void)
 
  boot_cpu_data = *info;
 }
+
+u64 __attribute_const__ icache_get_ccsidr(void)
+{
+ u64 ccsidr;
+
+ WARN_ON(preemptible());
+
+ /* Select L1 I-cache and read its size ID register */
+ asm("msr csselr_el1, %1; isb; mrs %0, ccsidr_el1"
+    : "=r"(ccsidr) : "r"(1L));
+ return ccsidr;
+}

Ard Biesheuvel

author

2014-08-05 09:25 UTC #7 lore raw

PATCH v2 3/3 arm64: don't flag non-aliasing VIPT I-caches as aliasing

In reply to Ard Biesheuvel

VIPT caches are non-aliasing if the index is derived from address bits that are always equal between VA and PA. Classifying these as aliasing results in unnecessary flushing which may hurt performance.

Signed-off-by Ard Biesheuvel <[email protected]>

---
 arch/arm64/kernel/cpuinfo.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

arch/arm64/kernel/cpuinfo.c +7 -1

index 177169623026..eb993cbdacf7 100644
@@ -47,7 +47,13 @@ static void cpuinfo_detect_icache_policy(struct cpuinfo_arm64 *info)
 	unsigned int cpu = smp_processor_id();
 	u32 l1ip = CTR_L1IP(info->reg_ctr);
 
-	if (l1ip != ICACHE_POLICY_PIPT)
+	/*
+	 * VIPT caches are non-aliasing if the VA always equals the PA in all
+	 * bit positions that are covered by the index, i.e., if num_sets_shift
+	 * is less than or equal to PAGE_SHIFT minus line_size_shift.
+	 */
+	if (l1ip != ICACHE_POLICY_PIPT && !(l1ip == ICACHE_POLICY_VIPT &&
+	    icache_get_linesize() * icache_get_numsets() <= PAGE_SIZE))
 		set_bit(ICACHEF_ALIASING, &__icache_flags);
 	if (l1ip == ICACHE_POLICY_AIVIVT)
 		set_bit(ICACHEF_AIVIVT, &__icache_flags);

Signature · 1 line

1.8.3.2

Will Deacon

2014-08-06 12:43 UTC #8 lore raw

PATCH v2 1/3 arm64: fix typo in I-cache policy detection

In reply to Ard Biesheuvel

Quoted 7 lines · Ard Biesheuvel

This removes an unfortunately placed semi-colon resulting in all instruction caches being classified as AIVIVT.
Signed-off-by Ard Biesheuvel <[email protected]>
Acked-by Mark Rutland <[email protected]>
---
v2: add ack

Cheers Ard, I'll take this patch into our fixes branch.

Will