Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1470619 > unrolled thread

[PATCH v2 9/9] arm64: Work around systems with mismatched cache line sizes

Started bySuzuki K Poulose <suzuki.poulose@arm.com>
First post2016-08-26 11:30 +0200
Last post2016-08-26 15:20 +0200
Articles 8 — 5 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v2 9/9] arm64: Work around systems with mismatched cache line sizes Suzuki K Poulose <suzuki.poulose@arm.com> - 2016-08-26 11:30 +0200
    Re: [PATCH v2 9/9] arm64: Work around systems with mismatched cache  line sizes Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2016-08-26 13:10 +0200
      Re: [PATCH v2 9/9] arm64: Work around systems with mismatched cache  line sizes Suzuki K Poulose <Suzuki.Poulose@arm.com> - 2016-08-26 15:10 +0200
        Re: [PATCH v2 9/9] arm64: Work around systems with mismatched cache  line sizes Will Deacon <will.deacon@arm.com> - 2016-08-26 18:20 +0200
          Re: [PATCH v2 9/9] arm64: Work around systems with mismatched cache  line sizes Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2016-08-26 19:00 +0200
          Re: [PATCH v2 9/9] arm64: Work around systems with mismatched cache  line sizes Catalin Marinas <catalin.marinas@arm.com> - 2016-08-26 19:20 +0200
            Re: [PATCH v2 9/9] arm64: Work around systems with mismatched cache  line sizes Suzuki K Poulose <Suzuki.Poulose@arm.com> - 2016-09-02 12:10 +0200
      Re: [PATCH v2 9/9] arm64: Work around systems with mismatched cache  line sizes Suzuki K Poulose <Suzuki.Poulose@arm.com> - 2016-08-26 15:20 +0200

#1470619 — [PATCH v2 9/9] arm64: Work around systems with mismatched cache line sizes

FromSuzuki K Poulose <suzuki.poulose@arm.com>
Date2016-08-26 11:30 +0200
Subject[PATCH v2 9/9] arm64: Work around systems with mismatched cache line sizes
Message-ID<salCy-57G-25@gated-at.bofh.it>
Systems with differing CPU i-cache/d-cache line sizes can cause
problems with the cache management by software when the execution
is migrated from one to another. Usually, the application reads
the cache size on a CPU and then uses that length to perform cache
operations. However, if it gets migrated to another CPU with a smaller
cache line size, things could go completely wrong. To prevent such
cases, always use the smallest cache line size among the CPUs. The
kernel CPU feature infrastructure already keeps track of the safe
value for all CPUID registers including CTR. This patch works around
the problem by :

For kernel, dynamically patch the kernel to read the cache size
from the system wide copy of CTR_EL0.

For applications, trap read accesses to CTR_EL0 (by clearing the SCTLR.UCT)
and emulate the mrs instruction to return the system wide safe value
of CTR_EL0.

For faster access (i.e, avoiding to lookup the system wide value of CTR_EL0
via read_system_reg), we keep track of the pointer to table entry for
CTR_EL0 in the CPU feature infrastructure.

Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Andre Przywara <andre.przywara@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
 arch/arm64/include/asm/assembler.h  | 25 +++++++++++++++++++++++--
 arch/arm64/include/asm/cpufeature.h |  4 +++-
 arch/arm64/include/asm/esr.h        |  8 ++++++++
 arch/arm64/include/asm/sysreg.h     |  1 +
 arch/arm64/kernel/asm-offsets.c     |  2 ++
 arch/arm64/kernel/cpu_errata.c      | 22 ++++++++++++++++++++++
 arch/arm64/kernel/cpufeature.c      |  9 +++++++++
 arch/arm64/kernel/traps.c           | 14 ++++++++++++++
 8 files changed, 82 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h
index a4bb3f5..66bd268 100644
--- a/arch/arm64/include/asm/assembler.h
+++ b/arch/arm64/include/asm/assembler.h
@@ -216,6 +216,21 @@ lr	.req	x30		// link register
 	.macro	mmid, rd, rn
 	ldr	\rd, [\rn, #MM_CONTEXT_ID]
 	.endm
+/*
+ * read_ctr - read CTR_EL0. If the system has mismatched
+ * cache line sizes, provide the system wide safe value.
+ */
+	.macro	read_ctr, reg
+alternative_if_not ARM64_MISMATCHED_CACHE_LINE_SIZE
+	mrs	\reg, ctr_el0			// read CTR
+	nop
+	nop
+alternative_else
+	ldr_l	\reg, sys_ctr_ftr		// Read system wide safe CTR value
+	ldr	\reg, [\reg, #ARM64_FTR_SYSVAL] // from sys_ctr_ftr->sys_val
+alternative_endif
+	.endm
+
 
 /*
  * raw_dcache_line_size - get the minimum D-cache line size on this CPU
@@ -232,7 +247,10 @@ lr	.req	x30		// link register
  * dcache_line_size - get the safe D-cache line size across all CPUs
  */
 	.macro	dcache_line_size, reg, tmp
-	raw_dcache_line_size	\reg, \tmp
+	read_ctr	\tmp
+	ubfm		\tmp, \tmp, #16, #19	// cache line size encoding
+	mov		\reg, #4		// bytes per word
+	lsl		\reg, \reg, \tmp	// actual cache line size
 	.endm
 
 /*
@@ -250,7 +268,10 @@ lr	.req	x30		// link register
  * icache_line_size - get the safe I-cache line size across all CPUs
  */
 	.macro	icache_line_size, reg, tmp
-	raw_icache_line_size	\reg, \tmp
+	read_ctr	\tmp
+	and		\tmp, \tmp, #0xf	// cache line size encoding
+	mov		\reg, #4		// bytes per word
+	lsl		\reg, \reg, \tmp	// actual cache line size
 	.endm
 
 /*
diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
index 692b8d3..e99f2af 100644
--- a/arch/arm64/include/asm/cpufeature.h
+++ b/arch/arm64/include/asm/cpufeature.h
@@ -37,8 +37,9 @@
 #define ARM64_WORKAROUND_CAVIUM_27456		12
 #define ARM64_HAS_32BIT_EL0			13
 #define ARM64_HYP_OFFSET_LOW			14
+#define ARM64_MISMATCHED_CACHE_LINE_SIZE	15
 
-#define ARM64_NCAPS				15
+#define ARM64_NCAPS				16
 
 #ifndef __ASSEMBLY__
 
@@ -109,6 +110,7 @@ struct arm64_cpu_capabilities {
 };
 
 extern DECLARE_BITMAP(cpu_hwcaps, ARM64_NCAPS);
+extern struct arm64_ftr_reg *sys_ctr_ftr;
 
 bool this_cpu_has_cap(unsigned int cap);
 
diff --git a/arch/arm64/include/asm/esr.h b/arch/arm64/include/asm/esr.h
index 9875b32..d14c478 100644
--- a/arch/arm64/include/asm/esr.h
+++ b/arch/arm64/include/asm/esr.h
@@ -149,6 +149,9 @@
 					 ((op2) << ESR_ELx_SYS64_ISS_OP2_SHIFT) | \
 					 ((crn) << ESR_ELx_SYS64_ISS_CRN_SHIFT) | \
 					 ((crm) << ESR_ELx_SYS64_ISS_CRM_SHIFT))
+
+#define ESR_ELx_SYS64_ISS_SYS_OP_MASK	(ESR_ELx_SYS64_ISS_SYS_MASK | \
+					 ESR_ELx_SYS64_ISS_DIR_MASK)
 /*
  * User space cache operations have the following sysreg encoding
  * in System instructions.
@@ -167,6 +170,11 @@
 #define ESR_ELx_SYS64_ISS_EL0_CACHE_OP_VAL \
 				(ESR_ELx_SYS64_ISS_SYS_VAL(1, 3, 1, 7, 0) | \
 				 ESR_ELx_SYS64_ISS_DIR_WRITE)
+
+#define ESR_ELx_SYS64_ISS_SYS_CTR	ESR_ELx_SYS64_ISS_SYS_VAL(3, 3, 1, 0, 0)
+#define ESR_ELx_SYS64_ISS_SYS_CTR_READ	(ESR_ELx_SYS64_ISS_SYS_CTR | \
+					 ESR_ELx_SYS64_ISS_DIR_READ)
+
 #ifndef __ASSEMBLY__
 #include <asm/types.h>
 
diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
index cc06794..4f26ad5 100644
--- a/arch/arm64/include/asm/sysreg.h
+++ b/arch/arm64/include/asm/sysreg.h
@@ -100,6 +100,7 @@
 /* SCTLR_EL1 specific flags. */
 #define SCTLR_EL1_UCI		(1 << 26)
 #define SCTLR_EL1_SPAN		(1 << 23)
+#define SCTLR_EL1_UCT		(1 << 15)
 #define SCTLR_EL1_SED		(1 << 8)
 #define SCTLR_EL1_CP15BEN	(1 << 5)
 
diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c
index 05070b7..4a2f0f0 100644
--- a/arch/arm64/kernel/asm-offsets.c
+++ b/arch/arm64/kernel/asm-offsets.c
@@ -23,6 +23,7 @@
 #include <linux/dma-mapping.h>
 #include <linux/kvm_host.h>
 #include <linux/suspend.h>
+#include <asm/cpufeature.h>
 #include <asm/thread_info.h>
 #include <asm/memory.h>
 #include <asm/smp_plat.h>
@@ -145,5 +146,6 @@ int main(void)
   DEFINE(HIBERN_PBE_ORIG,	offsetof(struct pbe, orig_address));
   DEFINE(HIBERN_PBE_ADDR,	offsetof(struct pbe, address));
   DEFINE(HIBERN_PBE_NEXT,	offsetof(struct pbe, next));
+  DEFINE(ARM64_FTR_SYSVAL,	offsetof(struct arm64_ftr_reg, sys_val));
   return 0;
 }
diff --git a/arch/arm64/kernel/cpu_errata.c b/arch/arm64/kernel/cpu_errata.c
index 5836b3d..621fa7f 100644
--- a/arch/arm64/kernel/cpu_errata.c
+++ b/arch/arm64/kernel/cpu_errata.c
@@ -30,6 +30,21 @@ is_affected_midr_range(const struct arm64_cpu_capabilities *entry, int scope)
 				       entry->midr_range_max);
 }
 
+static bool
+has_mismatched_cache_line_size(const struct arm64_cpu_capabilities *entry,
+				int scope)
+{
+	WARN_ON(scope != SCOPE_LOCAL_CPU || preemptible());
+	return (read_cpuid_cachetype() & sys_ctr_ftr->strict_mask) !=
+		 (sys_ctr_ftr->sys_val & sys_ctr_ftr->strict_mask);
+}
+
+static void cpu_enable_trap_ctr_access(void *__unused)
+{
+	/* Clear SCTLR_EL1.UCT */
+	config_sctlr_el1(SCTLR_EL1_UCT, 0);
+}
+
 #define MIDR_RANGE(model, min, max) \
 	.def_scope = SCOPE_LOCAL_CPU, \
 	.matches = is_affected_midr_range, \
@@ -108,6 +123,13 @@ const struct arm64_cpu_capabilities arm64_errata[] = {
 	},
 #endif
 	{
+		.desc = "Mismatched cache line size",
+		.capability = ARM64_MISMATCHED_CACHE_LINE_SIZE,
+		.matches = has_mismatched_cache_line_size,
+		.def_scope = SCOPE_LOCAL_CPU,
+		.enable = cpu_enable_trap_ctr_access,
+	},
+	{
 	}
 };
 
diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index fcf87ca..c1db313 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -46,6 +46,13 @@ unsigned int compat_elf_hwcap2 __read_mostly;
 
 DECLARE_BITMAP(cpu_hwcaps, ARM64_NCAPS);
 
+/*
+ * If we have mismatched cache line sizes, we need to emulate access to
+ * CTRL_EL0. To avoid a lookup everytime (via read_system_reg()), cache
+ * the table entry for CTR_EL0.
+ */
+struct arm64_ftr_reg *sys_ctr_ftr;
+
 #define __ARM64_FTR_BITS(SIGNED, STRICT, TYPE, SHIFT, WIDTH, SAFE_VAL) \
 	{						\
 		.sign = SIGNED,				\
@@ -461,6 +468,8 @@ void __init init_cpu_features(struct cpuinfo_arm64 *info)
 		init_cpu_ftr_reg(SYS_MVFR2_EL1, info->reg_mvfr2);
 	}
 
+	/* Keep track of the CTR feature register */
+	sys_ctr_ftr = get_arm64_ftr_reg(SYS_CTR_EL0);
 }
 
 static void update_cpu_ftr_reg(struct arm64_ftr_reg *reg, u64 new)
diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
index 224f64e..7c28898 100644
--- a/arch/arm64/kernel/traps.c
+++ b/arch/arm64/kernel/traps.c
@@ -480,6 +480,14 @@ static void user_cache_maint_handler(unsigned int esr, struct pt_regs *regs)
 		regs->pc += 4;
 }
 
+static void ctr_read_handler(unsigned int esr, struct pt_regs *regs)
+{
+	int rt = (esr & ESR_ELx_SYS64_ISS_RT_MASK) >> ESR_ELx_SYS64_ISS_RT_SHIFT;
+
+	regs->regs[rt] = sys_ctr_ftr->sys_val;
+	regs->pc += 4;
+}
+
 struct sys64_hook {
 	unsigned int esr_mask;
 	unsigned int esr_val;
@@ -492,6 +500,12 @@ static struct sys64_hook sys64_hooks[] = {
 		.esr_val = ESR_ELx_SYS64_ISS_EL0_CACHE_OP_VAL,
 		.handler = user_cache_maint_handler,
 	},
+	{
+		/* Trap read access to CTR_EL0 */
+		.esr_mask = ESR_ELx_SYS64_ISS_SYS_OP_MASK,
+		.esr_val = ESR_ELx_SYS64_ISS_SYS_CTR_READ,
+		.handler = ctr_read_handler,
+	},
 	{},
 };
 
-- 
2.7.4

[toc] | [next] | [standalone]

#1470656 — Re: [PATCH v2 9/9] arm64: Work around systems with mismatched cache line sizes

FromArd Biesheuvel <ard.biesheuvel@linaro.org>
Date2016-08-26 13:10 +0200
SubjectRe: [PATCH v2 9/9] arm64: Work around systems with mismatched cache line sizes
Message-ID<sanbj-6aB-1@gated-at.bofh.it>
In reply to#1470619
Hello Suzuki,

On 26 August 2016 at 11:23, Suzuki K Poulose <suzuki.poulose@arm.com> wrote:
> Systems with differing CPU i-cache/d-cache line sizes can cause
> problems with the cache management by software when the execution
> is migrated from one to another. Usually, the application reads
> the cache size on a CPU and then uses that length to perform cache
> operations. However, if it gets migrated to another CPU with a smaller
> cache line size, things could go completely wrong. To prevent such
> cases, always use the smallest cache line size among the CPUs. The
> kernel CPU feature infrastructure already keeps track of the safe
> value for all CPUID registers including CTR. This patch works around
> the problem by :
>
> For kernel, dynamically patch the kernel to read the cache size
> from the system wide copy of CTR_EL0.
>
> For applications, trap read accesses to CTR_EL0 (by clearing the SCTLR.UCT)
> and emulate the mrs instruction to return the system wide safe value
> of CTR_EL0.
>
> For faster access (i.e, avoiding to lookup the system wide value of CTR_EL0
> via read_system_reg), we keep track of the pointer to table entry for
> CTR_EL0 in the CPU feature infrastructure.
>

IIUC it is the runtime sorting of the arm64_ftr_reg array that
requires you to stash a pointer to CTR_EL0's entry somewhere, so that
you can dereference it without doing the bsearch.

IMO, this is a pattern that we should avoid: you are introducing one
instance now, which will make it hard to say no to the next one in the
future. Isn't there a better way to organize the arm64_ftr_reg array
that allows us to reference entries directly? Ideally, a way that gets
rid of the runtime sorting, since I don't think that is a good
replacement for developer discipline anyway (although I should have
spoken up when that was first introduced) Or am I missing something
here?

-- 
Ard.

> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Andre Przywara <andre.przywara@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> ---
>  arch/arm64/include/asm/assembler.h  | 25 +++++++++++++++++++++++--
>  arch/arm64/include/asm/cpufeature.h |  4 +++-
>  arch/arm64/include/asm/esr.h        |  8 ++++++++
>  arch/arm64/include/asm/sysreg.h     |  1 +
>  arch/arm64/kernel/asm-offsets.c     |  2 ++
>  arch/arm64/kernel/cpu_errata.c      | 22 ++++++++++++++++++++++
>  arch/arm64/kernel/cpufeature.c      |  9 +++++++++
>  arch/arm64/kernel/traps.c           | 14 ++++++++++++++
>  8 files changed, 82 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h
> index a4bb3f5..66bd268 100644
> --- a/arch/arm64/include/asm/assembler.h
> +++ b/arch/arm64/include/asm/assembler.h
> @@ -216,6 +216,21 @@ lr .req    x30             // link register
>         .macro  mmid, rd, rn
>         ldr     \rd, [\rn, #MM_CONTEXT_ID]
>         .endm
> +/*
> + * read_ctr - read CTR_EL0. If the system has mismatched
> + * cache line sizes, provide the system wide safe value.
> + */
> +       .macro  read_ctr, reg
> +alternative_if_not ARM64_MISMATCHED_CACHE_LINE_SIZE
> +       mrs     \reg, ctr_el0                   // read CTR
> +       nop
> +       nop
> +alternative_else
> +       ldr_l   \reg, sys_ctr_ftr               // Read system wide safe CTR value
> +       ldr     \reg, [\reg, #ARM64_FTR_SYSVAL] // from sys_ctr_ftr->sys_val
> +alternative_endif
> +       .endm
> +
>
>  /*
>   * raw_dcache_line_size - get the minimum D-cache line size on this CPU
> @@ -232,7 +247,10 @@ lr .req    x30             // link register
>   * dcache_line_size - get the safe D-cache line size across all CPUs
>   */
>         .macro  dcache_line_size, reg, tmp
> -       raw_dcache_line_size    \reg, \tmp
> +       read_ctr        \tmp
> +       ubfm            \tmp, \tmp, #16, #19    // cache line size encoding
> +       mov             \reg, #4                // bytes per word
> +       lsl             \reg, \reg, \tmp        // actual cache line size
>         .endm
>
>  /*
> @@ -250,7 +268,10 @@ lr .req    x30             // link register
>   * icache_line_size - get the safe I-cache line size across all CPUs
>   */
>         .macro  icache_line_size, reg, tmp
> -       raw_icache_line_size    \reg, \tmp
> +       read_ctr        \tmp
> +       and             \tmp, \tmp, #0xf        // cache line size encoding
> +       mov             \reg, #4                // bytes per word
> +       lsl             \reg, \reg, \tmp        // actual cache line size
>         .endm
>
>  /*
> diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
> index 692b8d3..e99f2af 100644
> --- a/arch/arm64/include/asm/cpufeature.h
> +++ b/arch/arm64/include/asm/cpufeature.h
> @@ -37,8 +37,9 @@
>  #define ARM64_WORKAROUND_CAVIUM_27456          12
>  #define ARM64_HAS_32BIT_EL0                    13
>  #define ARM64_HYP_OFFSET_LOW                   14
> +#define ARM64_MISMATCHED_CACHE_LINE_SIZE       15
>
> -#define ARM64_NCAPS                            15
> +#define ARM64_NCAPS                            16
>
>  #ifndef __ASSEMBLY__
>
> @@ -109,6 +110,7 @@ struct arm64_cpu_capabilities {
>  };
>
>  extern DECLARE_BITMAP(cpu_hwcaps, ARM64_NCAPS);
> +extern struct arm64_ftr_reg *sys_ctr_ftr;
>
>  bool this_cpu_has_cap(unsigned int cap);
>
> diff --git a/arch/arm64/include/asm/esr.h b/arch/arm64/include/asm/esr.h
> index 9875b32..d14c478 100644
> --- a/arch/arm64/include/asm/esr.h
> +++ b/arch/arm64/include/asm/esr.h
> @@ -149,6 +149,9 @@
>                                          ((op2) << ESR_ELx_SYS64_ISS_OP2_SHIFT) | \
>                                          ((crn) << ESR_ELx_SYS64_ISS_CRN_SHIFT) | \
>                                          ((crm) << ESR_ELx_SYS64_ISS_CRM_SHIFT))
> +
> +#define ESR_ELx_SYS64_ISS_SYS_OP_MASK  (ESR_ELx_SYS64_ISS_SYS_MASK | \
> +                                        ESR_ELx_SYS64_ISS_DIR_MASK)
>  /*
>   * User space cache operations have the following sysreg encoding
>   * in System instructions.
> @@ -167,6 +170,11 @@
>  #define ESR_ELx_SYS64_ISS_EL0_CACHE_OP_VAL \
>                                 (ESR_ELx_SYS64_ISS_SYS_VAL(1, 3, 1, 7, 0) | \
>                                  ESR_ELx_SYS64_ISS_DIR_WRITE)
> +
> +#define ESR_ELx_SYS64_ISS_SYS_CTR      ESR_ELx_SYS64_ISS_SYS_VAL(3, 3, 1, 0, 0)
> +#define ESR_ELx_SYS64_ISS_SYS_CTR_READ (ESR_ELx_SYS64_ISS_SYS_CTR | \
> +                                        ESR_ELx_SYS64_ISS_DIR_READ)
> +
>  #ifndef __ASSEMBLY__
>  #include <asm/types.h>
>
> diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
> index cc06794..4f26ad5 100644
> --- a/arch/arm64/include/asm/sysreg.h
> +++ b/arch/arm64/include/asm/sysreg.h
> @@ -100,6 +100,7 @@
>  /* SCTLR_EL1 specific flags. */
>  #define SCTLR_EL1_UCI          (1 << 26)
>  #define SCTLR_EL1_SPAN         (1 << 23)
> +#define SCTLR_EL1_UCT          (1 << 15)
>  #define SCTLR_EL1_SED          (1 << 8)
>  #define SCTLR_EL1_CP15BEN      (1 << 5)
>
> diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c
> index 05070b7..4a2f0f0 100644
> --- a/arch/arm64/kernel/asm-offsets.c
> +++ b/arch/arm64/kernel/asm-offsets.c
> @@ -23,6 +23,7 @@
>  #include <linux/dma-mapping.h>
>  #include <linux/kvm_host.h>
>  #include <linux/suspend.h>
> +#include <asm/cpufeature.h>
>  #include <asm/thread_info.h>
>  #include <asm/memory.h>
>  #include <asm/smp_plat.h>
> @@ -145,5 +146,6 @@ int main(void)
>    DEFINE(HIBERN_PBE_ORIG,      offsetof(struct pbe, orig_address));
>    DEFINE(HIBERN_PBE_ADDR,      offsetof(struct pbe, address));
>    DEFINE(HIBERN_PBE_NEXT,      offsetof(struct pbe, next));
> +  DEFINE(ARM64_FTR_SYSVAL,     offsetof(struct arm64_ftr_reg, sys_val));
>    return 0;
>  }
> diff --git a/arch/arm64/kernel/cpu_errata.c b/arch/arm64/kernel/cpu_errata.c
> index 5836b3d..621fa7f 100644
> --- a/arch/arm64/kernel/cpu_errata.c
> +++ b/arch/arm64/kernel/cpu_errata.c
> @@ -30,6 +30,21 @@ is_affected_midr_range(const struct arm64_cpu_capabilities *entry, int scope)
>                                        entry->midr_range_max);
>  }
>
> +static bool
> +has_mismatched_cache_line_size(const struct arm64_cpu_capabilities *entry,
> +                               int scope)
> +{
> +       WARN_ON(scope != SCOPE_LOCAL_CPU || preemptible());
> +       return (read_cpuid_cachetype() & sys_ctr_ftr->strict_mask) !=
> +                (sys_ctr_ftr->sys_val & sys_ctr_ftr->strict_mask);
> +}
> +
> +static void cpu_enable_trap_ctr_access(void *__unused)
> +{
> +       /* Clear SCTLR_EL1.UCT */
> +       config_sctlr_el1(SCTLR_EL1_UCT, 0);
> +}
> +
>  #define MIDR_RANGE(model, min, max) \
>         .def_scope = SCOPE_LOCAL_CPU, \
>         .matches = is_affected_midr_range, \
> @@ -108,6 +123,13 @@ const struct arm64_cpu_capabilities arm64_errata[] = {
>         },
>  #endif
>         {
> +               .desc = "Mismatched cache line size",
> +               .capability = ARM64_MISMATCHED_CACHE_LINE_SIZE,
> +               .matches = has_mismatched_cache_line_size,
> +               .def_scope = SCOPE_LOCAL_CPU,
> +               .enable = cpu_enable_trap_ctr_access,
> +       },
> +       {
>         }
>  };
>
> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> index fcf87ca..c1db313 100644
> --- a/arch/arm64/kernel/cpufeature.c
> +++ b/arch/arm64/kernel/cpufeature.c
> @@ -46,6 +46,13 @@ unsigned int compat_elf_hwcap2 __read_mostly;
>
>  DECLARE_BITMAP(cpu_hwcaps, ARM64_NCAPS);
>
> +/*
> + * If we have mismatched cache line sizes, we need to emulate access to
> + * CTRL_EL0. To avoid a lookup everytime (via read_system_reg()), cache
> + * the table entry for CTR_EL0.
> + */
> +struct arm64_ftr_reg *sys_ctr_ftr;
> +
>  #define __ARM64_FTR_BITS(SIGNED, STRICT, TYPE, SHIFT, WIDTH, SAFE_VAL) \
>         {                                               \
>                 .sign = SIGNED,                         \
> @@ -461,6 +468,8 @@ void __init init_cpu_features(struct cpuinfo_arm64 *info)
>                 init_cpu_ftr_reg(SYS_MVFR2_EL1, info->reg_mvfr2);
>         }
>
> +       /* Keep track of the CTR feature register */
> +       sys_ctr_ftr = get_arm64_ftr_reg(SYS_CTR_EL0);
>  }
>
>  static void update_cpu_ftr_reg(struct arm64_ftr_reg *reg, u64 new)
> diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
> index 224f64e..7c28898 100644
> --- a/arch/arm64/kernel/traps.c
> +++ b/arch/arm64/kernel/traps.c
> @@ -480,6 +480,14 @@ static void user_cache_maint_handler(unsigned int esr, struct pt_regs *regs)
>                 regs->pc += 4;
>  }
>
> +static void ctr_read_handler(unsigned int esr, struct pt_regs *regs)
> +{
> +       int rt = (esr & ESR_ELx_SYS64_ISS_RT_MASK) >> ESR_ELx_SYS64_ISS_RT_SHIFT;
> +
> +       regs->regs[rt] = sys_ctr_ftr->sys_val;
> +       regs->pc += 4;
> +}
> +
>  struct sys64_hook {
>         unsigned int esr_mask;
>         unsigned int esr_val;
> @@ -492,6 +500,12 @@ static struct sys64_hook sys64_hooks[] = {
>                 .esr_val = ESR_ELx_SYS64_ISS_EL0_CACHE_OP_VAL,
>                 .handler = user_cache_maint_handler,
>         },
> +       {
> +               /* Trap read access to CTR_EL0 */
> +               .esr_mask = ESR_ELx_SYS64_ISS_SYS_OP_MASK,
> +               .esr_val = ESR_ELx_SYS64_ISS_SYS_CTR_READ,
> +               .handler = ctr_read_handler,
> +       },
>         {},
>  };
>
> --
> 2.7.4
>

[toc] | [prev] | [next] | [standalone]

#1470710 — Re: [PATCH v2 9/9] arm64: Work around systems with mismatched cache line sizes

FromSuzuki K Poulose <Suzuki.Poulose@arm.com>
Date2016-08-26 15:10 +0200
SubjectRe: [PATCH v2 9/9] arm64: Work around systems with mismatched cache line sizes
Message-ID<sap3s-7uZ-33@gated-at.bofh.it>
In reply to#1470656
On 26/08/16 14:04, Suzuki K Poulose wrote:
> On 26/08/16 12:03, Ard Biesheuvel wrote:
>> Hello Suzuki,
>>
>
>
>>> For faster access (i.e, avoiding to lookup the system wide value of CTR_EL0
>>> via read_system_reg), we keep track of the pointer to table entry for
>>> CTR_EL0 in the CPU feature infrastructure.
>>>
>>
>> IIUC it is the runtime sorting of the arm64_ftr_reg array that
>> requires you to stash a pointer to CTR_EL0's entry somewhere, so that
>> you can dereference it without doing the bsearch.
>
> Correct.



>
>>
>> IMO, this is a pattern that we should avoid: you are introducing one
>> instance now, which will make it hard to say no to the next one in the
>> future. Isn't there a better way to organize the arm64_ftr_reg array
>> that allows us to reference entries directly? Ideally, a way that gets
>> rid of the runtime sorting, since I don't think that is a good
>> replacement for developer discipline anyway (although I should have
>> spoken up when that was first introduced) Or am I missing something
>> here?
>
> I had some form of direct access to the feature register in one of
> the versions [0], but was dropped based on Catalin's suggestion at [1].

Forgot to add, [0] wouldn't solve this issue cleanly either. It would simply
speed up the read_system_reg(). So we do need a call to read_system_reg()
from assembly code, which makes it a little bit tricky.

Suzuki

>
>
> [0] https://lkml.org/lkml/2015/10/5/504
> [1] https://lkml.org/lkml/2015/10/7/558
>
>
> Suzuki
>
>

[toc] | [prev] | [next] | [standalone]

#1470881 — Re: [PATCH v2 9/9] arm64: Work around systems with mismatched cache line sizes

FromWill Deacon <will.deacon@arm.com>
Date2016-08-26 18:20 +0200
SubjectRe: [PATCH v2 9/9] arm64: Work around systems with mismatched cache line sizes
Message-ID<sas1j-XY-9@gated-at.bofh.it>
In reply to#1470710
On Fri, Aug 26, 2016 at 02:08:01PM +0100, Suzuki K Poulose wrote:
> On 26/08/16 14:04, Suzuki K Poulose wrote:
> >On 26/08/16 12:03, Ard Biesheuvel wrote:
> >>IMO, this is a pattern that we should avoid: you are introducing one
> >>instance now, which will make it hard to say no to the next one in the
> >>future. Isn't there a better way to organize the arm64_ftr_reg array
> >>that allows us to reference entries directly? Ideally, a way that gets
> >>rid of the runtime sorting, since I don't think that is a good
> >>replacement for developer discipline anyway (although I should have
> >>spoken up when that was first introduced) Or am I missing something
> >>here?
> >
> >I had some form of direct access to the feature register in one of
> >the versions [0], but was dropped based on Catalin's suggestion at [1].
> 
> Forgot to add, [0] wouldn't solve this issue cleanly either. It would simply
> speed up the read_system_reg(). So we do need a call to read_system_reg()
> from assembly code, which makes it a little bit tricky.

It might be worth looking to see if we can pass the ctr as an extra
parameter to the assembly routines that need it. Then you can access it
easily from C code, and if you pass it as 0 that could result in the asm
code reading it from the h/w register, removing the need for the _raw
stuff you add.

Of course, it could also be a complete mess fixing up all the callers,
but it's probably worth investigating to see what the trade-off is.

Will

[toc] | [prev] | [next] | [standalone]

#1470894 — Re: [PATCH v2 9/9] arm64: Work around systems with mismatched cache line sizes

FromArd Biesheuvel <ard.biesheuvel@linaro.org>
Date2016-08-26 19:00 +0200
SubjectRe: [PATCH v2 9/9] arm64: Work around systems with mismatched cache line sizes
Message-ID<sasE1-1bF-3@gated-at.bofh.it>
In reply to#1470881
On 26 August 2016 at 17:16, Will Deacon <will.deacon@arm.com> wrote:
> On Fri, Aug 26, 2016 at 02:08:01PM +0100, Suzuki K Poulose wrote:
>> On 26/08/16 14:04, Suzuki K Poulose wrote:
>> >On 26/08/16 12:03, Ard Biesheuvel wrote:
>> >>IMO, this is a pattern that we should avoid: you are introducing one
>> >>instance now, which will make it hard to say no to the next one in the
>> >>future. Isn't there a better way to organize the arm64_ftr_reg array
>> >>that allows us to reference entries directly? Ideally, a way that gets
>> >>rid of the runtime sorting, since I don't think that is a good
>> >>replacement for developer discipline anyway (although I should have
>> >>spoken up when that was first introduced) Or am I missing something
>> >>here?
>> >
>> >I had some form of direct access to the feature register in one of
>> >the versions [0], but was dropped based on Catalin's suggestion at [1].
>>
>> Forgot to add, [0] wouldn't solve this issue cleanly either. It would simply
>> speed up the read_system_reg(). So we do need a call to read_system_reg()
>> from assembly code, which makes it a little bit tricky.
>
> It might be worth looking to see if we can pass the ctr as an extra
> parameter to the assembly routines that need it. Then you can access it
> easily from C code, and if you pass it as 0 that could result in the asm
> code reading it from the h/w register, removing the need for the _raw
> stuff you add.
>
> Of course, it could also be a complete mess fixing up all the callers,
> but it's probably worth investigating to see what the trade-off is.
>

The point Catalin made was under the assumption that this code is
never called on a hot path, and now you are finding yourself
optimizing the access, which either means you are doing so
prematurely, or it is on a hot path.

I will follow up with a separate suggestion. Feel free to incorporate
it, or completely ignore it. Just my 2 cents.

-- 
Ard.

[toc] | [prev] | [next] | [standalone]

#1470900 — Re: [PATCH v2 9/9] arm64: Work around systems with mismatched cache line sizes

FromCatalin Marinas <catalin.marinas@arm.com>
Date2016-08-26 19:20 +0200
SubjectRe: [PATCH v2 9/9] arm64: Work around systems with mismatched cache line sizes
Message-ID<sasXn-1xo-3@gated-at.bofh.it>
In reply to#1470881
On Fri, Aug 26, 2016 at 05:16:27PM +0100, Will Deacon wrote:
> On Fri, Aug 26, 2016 at 02:08:01PM +0100, Suzuki K Poulose wrote:
> > On 26/08/16 14:04, Suzuki K Poulose wrote:
> > >On 26/08/16 12:03, Ard Biesheuvel wrote:
> > >>IMO, this is a pattern that we should avoid: you are introducing one
> > >>instance now, which will make it hard to say no to the next one in the
> > >>future. Isn't there a better way to organize the arm64_ftr_reg array
> > >>that allows us to reference entries directly? Ideally, a way that gets
> > >>rid of the runtime sorting, since I don't think that is a good
> > >>replacement for developer discipline anyway (although I should have
> > >>spoken up when that was first introduced) Or am I missing something
> > >>here?

I'm not sure we can have some simple direct access. Suzuki did some
grouping originally but I wouldn't call that a direct access either,
more like iterating through several groups.

A possibility would be to generate global variables for each
arm64_ftr_reg with the ARM64_FTR_REG macro extended to also place a
pointer in a dedicated section to be used as array. Assembly code would
access the global variable directly. But I'm not really sure it's worth
it.

> > >I had some form of direct access to the feature register in one of
> > >the versions [0], but was dropped based on Catalin's suggestion at [1].
> > 
> > Forgot to add, [0] wouldn't solve this issue cleanly either. It would simply
> > speed up the read_system_reg(). So we do need a call to read_system_reg()
> > from assembly code, which makes it a little bit tricky.
> 
> It might be worth looking to see if we can pass the ctr as an extra
> parameter to the assembly routines that need it. Then you can access it
> easily from C code, and if you pass it as 0 that could result in the asm
> code reading it from the h/w register, removing the need for the _raw
> stuff you add.

How often to we need to access a sanitised sysreg from assembly? AFAICT,
CTR_EL0 is the first. If we only need it to infer the minimum cache line
size, we could as well store the latter in a global variable and access
it directly. If we feel brave, we could patch a "mov \reg, #x"
instruction in the ?cache_line_size macros (starting with 32 by default,
though to make it less cumbersome we'd have to improve the run-time
patching code a bit).

-- 
Catalin

[toc] | [prev] | [next] | [standalone]

#1475000 — Re: [PATCH v2 9/9] arm64: Work around systems with mismatched cache line sizes

FromSuzuki K Poulose <Suzuki.Poulose@arm.com>
Date2016-09-02 12:10 +0200
SubjectRe: [PATCH v2 9/9] arm64: Work around systems with mismatched cache line sizes
Message-ID<scTA7-6xn-91@gated-at.bofh.it>
In reply to#1470900
On 26/08/16 18:00, Catalin Marinas wrote:
> On Fri, Aug 26, 2016 at 05:16:27PM +0100, Will Deacon wrote:
>> On Fri, Aug 26, 2016 at 02:08:01PM +0100, Suzuki K Poulose wrote:
>>> On 26/08/16 14:04, Suzuki K Poulose wrote:

>> It might be worth looking to see if we can pass the ctr as an extra
>> parameter to the assembly routines that need it. Then you can access it
>> easily from C code, and if you pass it as 0 that could result in the asm
>> code reading it from the h/w register, removing the need for the _raw
>> stuff you add.
>
> How often to we need to access a sanitised sysreg from assembly? AFAICT,
> CTR_EL0 is the first. If we only need it to infer the minimum cache line
> size, we could as well store the latter in a global variable and access
> it directly. If we feel brave, we could patch a "mov \reg, #x"
> instruction in the ?cache_line_size macros (starting with 32 by default,
> though to make it less cumbersome we'd have to improve the run-time
> patching code a bit).


With Ard's patches [1] to refactor the feature array, we can refer to named
CTR_EL0 feature register cleanly. I can rebase this series on top of that
if nobody has any objection.

[1] http://marc.info/?l=linux-arm-kernel&m=147263959504998&w=2


Suzuki

[toc] | [prev] | [next] | [standalone]

#1470721 — Re: [PATCH v2 9/9] arm64: Work around systems with mismatched cache line sizes

FromSuzuki K Poulose <Suzuki.Poulose@arm.com>
Date2016-08-26 15:20 +0200
SubjectRe: [PATCH v2 9/9] arm64: Work around systems with mismatched cache line sizes
Message-ID<sap3s-7uZ-35@gated-at.bofh.it>
In reply to#1470656
On 26/08/16 12:03, Ard Biesheuvel wrote:
> Hello Suzuki,
>


>> For faster access (i.e, avoiding to lookup the system wide value of CTR_EL0
>> via read_system_reg), we keep track of the pointer to table entry for
>> CTR_EL0 in the CPU feature infrastructure.
>>
>
> IIUC it is the runtime sorting of the arm64_ftr_reg array that
> requires you to stash a pointer to CTR_EL0's entry somewhere, so that
> you can dereference it without doing the bsearch.

Correct.

>
> IMO, this is a pattern that we should avoid: you are introducing one
> instance now, which will make it hard to say no to the next one in the
> future. Isn't there a better way to organize the arm64_ftr_reg array
> that allows us to reference entries directly? Ideally, a way that gets
> rid of the runtime sorting, since I don't think that is a good
> replacement for developer discipline anyway (although I should have
> spoken up when that was first introduced) Or am I missing something
> here?

I had some form of direct access to the feature register in one of
the versions [0], but was dropped based on Catalin's suggestion at [1].


[0] https://lkml.org/lkml/2015/10/5/504
[1] https://lkml.org/lkml/2015/10/7/558


Suzuki

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web