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

Groups > linux.kernel > #1465303 > unrolled thread

[RESEND] [PATCH 0/8] arm64: Work around for mismatched cache line size

Back to article view | Back to linux.kernel

Contents

  [RESEND] [PATCH 0/8] arm64: Work around for mismatched cache line size Suzuki K Poulose <suzuki.poulose@arm.com> - 2016-08-18 15:20 +0200
    [PATCH 6/8] arm64: Introduce raw_{d,i}cache_line_size Suzuki K Poulose <suzuki.poulose@arm.com> - 2016-08-18 15:20 +0200
      Re: [PATCH 6/8] arm64: Introduce raw_{d,i}cache_line_size Geoff Levand <geoff@infradead.org> - 2016-08-19 03:20 +0200
      Re: [PATCH 6/8] arm64: Introduce raw_{d,i}cache_line_size Will Deacon <will.deacon@arm.com> - 2016-08-22 12:10 +0200
        Re: [PATCH 6/8] arm64: Introduce raw_{d,i}cache_line_size Suzuki K Poulose <Suzuki.Poulose@arm.com> - 2016-08-23 12:20 +0200
    [PATCH 4/8] arm64: insn: Add helpers for adrp offsets Suzuki K Poulose <suzuki.poulose@arm.com> - 2016-08-18 15:30 +0200
      Re: [PATCH 4/8] arm64: insn: Add helpers for adrp offsets Marc Zyngier <marc.zyngier@arm.com> - 2016-08-18 16:50 +0200
        Re: [PATCH 4/8] arm64: insn: Add helpers for adrp offsets Suzuki K Poulose <Suzuki.Poulose@arm.com> - 2016-08-19 03:00 +0200

#1465303 — [RESEND] [PATCH 0/8] arm64: Work around for mismatched cache line size

This series adds a work around for systems with mismatched {I,D}-cache
line sizes. When a thread of execution gets migrated to a different CPU,
the cache line size it had cached could be larger than that of the new
CPU. This could cause data corruption issues. We work around this by

 - Dynamically patching the kernel to use the smallest line size on the
   system (from the CPU feature infrastructure)
 - Trapping the userspace access to CTR_EL0 (by clearing SCTLR_EL1.UCT) and
   emulating it with the system wide safe value of CTR.

The series also adds support for alternative code patching of adrp
instructions by adjusting the PC-relative address offset to reflect
the new PC.

The series has been tested on Juno with a hack to forced enabling
of the capability.

Applies on aarch64: for-next/core. The tree is avaiable at :

	git://linux-arm.org/linux-skp.git ctr-emulation

Suzuki K Poulose (8):
  arm64: Set the safe value for L1 icache policy
  arm64: Use consistent naming for errata handling
  arm64: Rearrange CPU errata workaround checks
  arm64: insn: Add helpers for adrp offsets
  arm64: alternative: Add support for patching adrp instructions
  arm64: Introduce raw_{d,i}cache_line_size
  arm64: Refactor sysinstr exception handling
  arm64: Work around systems with mismatched cache line sizes

 arch/arm64/include/asm/assembler.h  | 45 +++++++++++++++++--
 arch/arm64/include/asm/cpufeature.h | 14 +++---
 arch/arm64/include/asm/esr.h        | 56 ++++++++++++++++++++++++
 arch/arm64/include/asm/insn.h       |  4 ++
 arch/arm64/include/asm/sysreg.h     |  1 +
 arch/arm64/kernel/alternative.c     | 13 ++++++
 arch/arm64/kernel/asm-offsets.c     |  2 +
 arch/arm64/kernel/cpu_errata.c      | 26 ++++++++++-
 arch/arm64/kernel/cpufeature.c      | 44 ++++++++++++++-----
 arch/arm64/kernel/cpuinfo.c         |  2 -
 arch/arm64/kernel/hibernate-asm.S   |  2 +-
 arch/arm64/kernel/insn.c            | 13 ++++++
 arch/arm64/kernel/relocate_kernel.S |  2 +-
 arch/arm64/kernel/smp.c             |  8 +++-
 arch/arm64/kernel/traps.c           | 87 ++++++++++++++++++++++++++-----------
 15 files changed, 264 insertions(+), 55 deletions(-)

-- 
2.7.4

[toc] | [next] | [standalone]

#1465304 — [PATCH 6/8] arm64: Introduce raw_{d,i}cache_line_size

On systems with mismatched i/d cache min line sizes, we need to use
the smallest size possible across all CPUs. This will be done by fetching
the system wide safe value from CPU feature infrastructure.
However the some special users(e.g kexec, hibernate) would need the line
size on the CPU (rather than the system wide), when the system wide
feature may not be accessible. Provide another helper which will fetch
cache line size on the current CPU.

Cc: James Morse <james.morse@arm.com>
Cc: Geoff Levand <geoff@infradead.org>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
 arch/arm64/include/asm/assembler.h  | 24 ++++++++++++++++++++----
 arch/arm64/kernel/hibernate-asm.S   |  2 +-
 arch/arm64/kernel/relocate_kernel.S |  2 +-
 3 files changed, 22 insertions(+), 6 deletions(-)

diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h
index d5025c6..a4bb3f5 100644
--- a/arch/arm64/include/asm/assembler.h
+++ b/arch/arm64/include/asm/assembler.h
@@ -218,9 +218,10 @@ lr	.req	x30		// link register
 	.endm
 
 /*
- * dcache_line_size - get the minimum D-cache line size from the CTR register.
+ * raw_dcache_line_size - get the minimum D-cache line size on this CPU
+ * from the CTR register.
  */
-	.macro	dcache_line_size, reg, tmp
+	.macro	raw_dcache_line_size, reg, tmp
 	mrs	\tmp, ctr_el0			// read CTR
 	ubfm	\tmp, \tmp, #16, #19		// cache line size encoding
 	mov	\reg, #4			// bytes per word
@@ -228,9 +229,17 @@ lr	.req	x30		// link register
 	.endm
 
 /*
- * icache_line_size - get the minimum I-cache line size from the CTR register.
+ * dcache_line_size - get the safe D-cache line size across all CPUs
  */
-	.macro	icache_line_size, reg, tmp
+	.macro	dcache_line_size, reg, tmp
+	raw_dcache_line_size	\reg, \tmp
+	.endm
+
+/*
+ * raw_icache_line_size - get the minimum I-cache line size on this CPU
+ * from the CTR register.
+ */
+	.macro	raw_icache_line_size, reg, tmp
 	mrs	\tmp, ctr_el0			// read CTR
 	and	\tmp, \tmp, #0xf		// cache line size encoding
 	mov	\reg, #4			// bytes per word
@@ -238,6 +247,13 @@ lr	.req	x30		// link register
 	.endm
 
 /*
+ * 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
+	.endm
+
+/*
  * tcr_set_idmap_t0sz - update TCR.T0SZ so that we can load the ID map
  */
 	.macro	tcr_set_idmap_t0sz, valreg, tmpreg
diff --git a/arch/arm64/kernel/hibernate-asm.S b/arch/arm64/kernel/hibernate-asm.S
index 46f29b6..4ebc6a1 100644
--- a/arch/arm64/kernel/hibernate-asm.S
+++ b/arch/arm64/kernel/hibernate-asm.S
@@ -96,7 +96,7 @@ ENTRY(swsusp_arch_suspend_exit)
 
 	add	x1, x10, #PAGE_SIZE
 	/* Clean the copied page to PoU - based on flush_icache_range() */
-	dcache_line_size x2, x3
+	raw_dcache_line_size x2, x3
 	sub	x3, x2, #1
 	bic	x4, x10, x3
 2:	dc	cvau, x4	/* clean D line / unified line */
diff --git a/arch/arm64/kernel/relocate_kernel.S b/arch/arm64/kernel/relocate_kernel.S
index 51b73cd..ce704a4 100644
--- a/arch/arm64/kernel/relocate_kernel.S
+++ b/arch/arm64/kernel/relocate_kernel.S
@@ -34,7 +34,7 @@ ENTRY(arm64_relocate_new_kernel)
 	/* Setup the list loop variables. */
 	mov	x17, x1				/* x17 = kimage_start */
 	mov	x16, x0				/* x16 = kimage_head */
-	dcache_line_size x15, x0		/* x15 = dcache line size */
+	raw_dcache_line_size x15, x0		/* x15 = dcache line size */
 	mov	x14, xzr			/* x14 = entry ptr */
 	mov	x13, xzr			/* x13 = copy dest */
 
-- 
2.7.4

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

#1465744 — Re: [PATCH 6/8] arm64: Introduce raw_{d,i}cache_line_size

On Thu, 2016-08-18 at 14:10 +0100, Suzuki K Poulose wrote:
> On systems with mismatched i/d cache min line sizes, we need to use
> the smallest size possible across all CPUs. This will be done by fetching
> the system wide safe value from CPU feature infrastructure.
> However the some special users(e.g kexec, hibernate) would need the line
> size on the CPU (rather than the system wide), when the system wide
> feature may not be accessible. Provide another helper which will fetch
> cache line size on the current CPU.
> 
> Cc: James Morse <james.morse@arm.com>
> Cc: Geoff Levand <geoff@infradead.org>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> ---
>  arch/arm64/include/asm/assembler.h  | 24 ++++++++++++++++++++----
>  arch/arm64/kernel/hibernate-asm.S   |  2 +-
>  arch/arm64/kernel/relocate_kernel.S |  2 +-
>  3 files changed, 22 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h
> index d5025c6..a4bb3f5 100644
> --- a/arch/arm64/include/asm/assembler.h
> +++ b/arch/arm64/include/asm/assembler.h
> @@ -218,9 +218,10 @@ lr	.req	x30		// link register
>  	.endm
>  
>  /*
> - * dcache_line_size - get the minimum D-cache line size from the CTR register.
> + * raw_dcache_line_size - get the minimum D-cache line size on this CPU
> + * from the CTR register.
>   */
> -	.macro	dcache_line_size, reg, tmp
> +	.macro	raw_dcache_line_size, reg, tmp
>  	mrs	\tmp, ctr_el0			// read CTR
>  	ubfm	\tmp, \tmp, #16, #19		// cache line size encoding
>  	mov	\reg, #4			// bytes per word
> @@ -228,9 +229,17 @@ lr	.req	x30		// link register
>  	.endm

...

> +++ b/arch/arm64/kernel/relocate_kernel.S
> @@ -34,7 +34,7 @@ ENTRY(arm64_relocate_new_kernel)
>  	/* Setup the list loop variables. */
>  	mov	x17, x1				/* x17 = kimage_start */
>  	mov	x16, x0				/* x16 = kimage_head */
> -	dcache_line_size x15, x0		/* x15 = dcache line size */
> +	raw_dcache_line_size x15, x0		/* x15 = dcache line size */
>  	mov	x14, xzr			/* x14 = entry ptr */
>  	mov	x13, xzr			/* x13 = copy dest */

Since this is just renaming dcache_line_size to raw_dcache_line_size,
and for kexec's relocate_kernel we need to know about the CPU we are
running on, this part of the change looks good.

Reviewed by: Geoff Levand <geoff@infradead.org>

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

#1467521 — Re: [PATCH 6/8] arm64: Introduce raw_{d,i}cache_line_size

On Thu, Aug 18, 2016 at 02:10:30PM +0100, Suzuki K Poulose wrote:
> On systems with mismatched i/d cache min line sizes, we need to use
> the smallest size possible across all CPUs. This will be done by fetching
> the system wide safe value from CPU feature infrastructure.
> However the some special users(e.g kexec, hibernate) would need the line
> size on the CPU (rather than the system wide), when the system wide
> feature may not be accessible. Provide another helper which will fetch
> cache line size on the current CPU.

Why are these users "special"? Using a smaller line size shouldn't affect
correctness, and I don't see kexec and hibernate as being performance
critical in their cache maintenance.

Will

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

#1468467 — Re: [PATCH 6/8] arm64: Introduce raw_{d,i}cache_line_size

On 22/08/16 11:00, Will Deacon wrote:
> On Thu, Aug 18, 2016 at 02:10:30PM +0100, Suzuki K Poulose wrote:
>> On systems with mismatched i/d cache min line sizes, we need to use
>> the smallest size possible across all CPUs. This will be done by fetching
>> the system wide safe value from CPU feature infrastructure.
>> However the some special users(e.g kexec, hibernate) would need the line
>> size on the CPU (rather than the system wide), when the system wide
>> feature may not be accessible. Provide another helper which will fetch
>> cache line size on the current CPU.
>
> Why are these users "special"? Using a smaller line size shouldn't affect

With the alternate patched code, we refer to the kernel data structure for
CTR value. At least for kexec, it may overwrite the existing kernel image/data where
our data was stored and could possibly end up in receiving corrupted code.

For all special cases where it is ensured that the code is run on a
single CPU and will not be migrated to another CPU they can rely on
the raw value of CTR, hence the change.

> correctness, and I don't see kexec and hibernate as being performance
> critical in their cache maintenance.

Its not for performance, but for the safety.

Suzuki

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

#1465335 — [PATCH 4/8] arm64: insn: Add helpers for adrp offsets

Adds helpers for decoding/encoding the PC relative addresses for adrp.
This will be used for handling dynamic patching of 'adrp' instructions
in alternative code patching.

Cc: Mark Rutland <mark.rutland@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/insn.h |  4 ++++
 arch/arm64/kernel/insn.c      | 13 +++++++++++++
 2 files changed, 17 insertions(+)

diff --git a/arch/arm64/include/asm/insn.h b/arch/arm64/include/asm/insn.h
index 1dbaa90..dffb0364 100644
--- a/arch/arm64/include/asm/insn.h
+++ b/arch/arm64/include/asm/insn.h
@@ -247,6 +247,7 @@ static __always_inline u32 aarch64_insn_get_##abbr##_value(void) \
 { return (val); }
 
 __AARCH64_INSN_FUNCS(adr_adrp,	0x1F000000, 0x10000000)
+__AARCH64_INSN_FUNCS(adrp,	0x9F000000, 0x90000000)
 __AARCH64_INSN_FUNCS(prfm_lit,	0xFF000000, 0xD8000000)
 __AARCH64_INSN_FUNCS(str_reg,	0x3FE0EC00, 0x38206800)
 __AARCH64_INSN_FUNCS(ldr_reg,	0x3FE0EC00, 0x38606800)
@@ -398,6 +399,9 @@ int aarch64_insn_patch_text_nosync(void *addr, u32 insn);
 int aarch64_insn_patch_text_sync(void *addrs[], u32 insns[], int cnt);
 int aarch64_insn_patch_text(void *addrs[], u32 insns[], int cnt);
 
+s32 aarch64_insn_adrp_get_offset(u32 insn);
+u32 aarch64_insn_adrp_set_offset(u32 insn, s32 offset);
+
 bool aarch32_insn_is_wide(u32 insn);
 
 #define A32_RN_OFFSET	16
diff --git a/arch/arm64/kernel/insn.c b/arch/arm64/kernel/insn.c
index 63f9432..f022af4 100644
--- a/arch/arm64/kernel/insn.c
+++ b/arch/arm64/kernel/insn.c
@@ -1202,6 +1202,19 @@ u32 aarch64_set_branch_offset(u32 insn, s32 offset)
 	BUG();
 }
 
+s32 aarch64_insn_adrp_get_offset(u32 insn)
+{
+	BUG_ON(!aarch64_insn_is_adrp(insn));
+	return aarch64_insn_decode_immediate(AARCH64_INSN_IMM_ADR, insn) << 12;
+}
+
+u32 aarch64_insn_adrp_set_offset(u32 insn, s32 offset)
+{
+	BUG_ON(!aarch64_insn_is_adrp(insn));
+	return aarch64_insn_encode_immediate(AARCH64_INSN_IMM_ADR, insn,
+						offset >> 12);
+}
+
 /*
  * Extract the Op/CR data from a msr/mrs instruction.
  */
-- 
2.7.4

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

#1465593 — Re: [PATCH 4/8] arm64: insn: Add helpers for adrp offsets

Hi Suzuki,

On 18/08/16 14:10, Suzuki K Poulose wrote:
> Adds helpers for decoding/encoding the PC relative addresses for adrp.
> This will be used for handling dynamic patching of 'adrp' instructions
> in alternative code patching.
> 
> Cc: Mark Rutland <mark.rutland@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/insn.h |  4 ++++
>  arch/arm64/kernel/insn.c      | 13 +++++++++++++
>  2 files changed, 17 insertions(+)
> 
> diff --git a/arch/arm64/include/asm/insn.h b/arch/arm64/include/asm/insn.h
> index 1dbaa90..dffb0364 100644
> --- a/arch/arm64/include/asm/insn.h
> +++ b/arch/arm64/include/asm/insn.h
> @@ -247,6 +247,7 @@ static __always_inline u32 aarch64_insn_get_##abbr##_value(void) \
>  { return (val); }
>  
>  __AARCH64_INSN_FUNCS(adr_adrp,	0x1F000000, 0x10000000)
> +__AARCH64_INSN_FUNCS(adrp,	0x9F000000, 0x90000000)

I'm a bit bothered by this one. We end-up with both
aarch64_insn_is_adr_adrp() *and* aarch64_insn_is_adrp() (and their
respective getters).

How about dropping adr_adrp, and explicitly having adr and adrp? There
is only two users in the tree, so that should be easy to address.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

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

#1465684 — Re: [PATCH 4/8] arm64: insn: Add helpers for adrp offsets

On 18/08/16 15:47, Marc Zyngier wrote:
> Hi Suzuki,
>
> On 18/08/16 14:10, Suzuki K Poulose wrote:
>> Adds helpers for decoding/encoding the PC relative addresses for adrp.
>> This will be used for handling dynamic patching of 'adrp' instructions
>> in alternative code patching.
>>
>> Cc: Mark Rutland <mark.rutland@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/insn.h |  4 ++++
>>  arch/arm64/kernel/insn.c      | 13 +++++++++++++
>>  2 files changed, 17 insertions(+)
>>
>> diff --git a/arch/arm64/include/asm/insn.h b/arch/arm64/include/asm/insn.h
>> index 1dbaa90..dffb0364 100644
>> --- a/arch/arm64/include/asm/insn.h
>> +++ b/arch/arm64/include/asm/insn.h
>> @@ -247,6 +247,7 @@ static __always_inline u32 aarch64_insn_get_##abbr##_value(void) \
>>  { return (val); }
>>
>>  __AARCH64_INSN_FUNCS(adr_adrp,	0x1F000000, 0x10000000)
>> +__AARCH64_INSN_FUNCS(adrp,	0x9F000000, 0x90000000)
>
> I'm a bit bothered by this one. We end-up with both
> aarch64_insn_is_adr_adrp() *and* aarch64_insn_is_adrp() (and their
> respective getters).

You're right. It doesn't look good.  

> How about dropping adr_adrp, and explicitly having adr and adrp? There
> is only two users in the tree, so that should be easy to address.

Sounds good, will update if for v2.

Cheers
Suzuki

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web