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

Groups > linux.kernel > #1465277 > unrolled thread

[PATCH 7/8] arm64: Refactor sysinstr exception handling

Started bySuzuki K Poulose <suzuki.poulose@arm.com>
First post2016-08-18 15:20 +0200
Last post2016-08-23 12:20 +0200
Articles 3 — 3 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 7/8] arm64: Refactor sysinstr exception handling Suzuki K Poulose <suzuki.poulose@arm.com> - 2016-08-18 15:20 +0200
    Re: [PATCH 7/8] arm64: Refactor sysinstr exception handling Will Deacon <will.deacon@arm.com> - 2016-08-22 15:00 +0200
      Re: [PATCH 7/8] arm64: Refactor sysinstr exception handling Suzuki K Poulose <Suzuki.Poulose@arm.com> - 2016-08-23 12:20 +0200

#1465277 — [PATCH 7/8] arm64: Refactor sysinstr exception handling

FromSuzuki K Poulose <suzuki.poulose@arm.com>
Date2016-08-18 15:20 +0200
Subject[PATCH 7/8] arm64: Refactor sysinstr exception handling
Message-ID<s7voK-ho-25@gated-at.bofh.it>
Right now we trap some of the user space data cache operations
based on a few Errata (ARM 819472, 826319, 827319 and 824069).
We need to trap userspace access to CTR_EL0, if we detect mismatched
cache line size. Since both these traps share the EC, refactor
the handler a little bit to make it a bit more reader friendly.

Cc: Andre Przywara <andre.przywara@arm.com>
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/esr.h | 48 +++++++++++++++++++++++++++++
 arch/arm64/kernel/traps.c    | 73 ++++++++++++++++++++++++++++----------------
 2 files changed, 95 insertions(+), 26 deletions(-)

diff --git a/arch/arm64/include/asm/esr.h b/arch/arm64/include/asm/esr.h
index f772e15..2a8f6c3 100644
--- a/arch/arm64/include/asm/esr.h
+++ b/arch/arm64/include/asm/esr.h
@@ -109,6 +109,54 @@
 	((ESR_ELx_EC_BRK64 << ESR_ELx_EC_SHIFT) | ESR_ELx_IL |	\
 	 ((imm) & 0xffff))
 
+/* ISS field definitions for System instruction traps */
+#define ESR_ELx_SYS64_ISS_RES0_SHIFT	22
+#define ESR_ELx_SYS64_ISS_RES0_MASK	(UL(0x7) << ESR_ELx_SYS64_ISS_RES0_SHIFT)
+#define ESR_ELx_SYS64_ISS_DIR_MASK	0x1
+#define ESR_ELx_SYS64_ISS_DIR_READ	0x1
+#define ESR_ELx_SYS64_ISS_DIR_WRITE	0x0
+
+#define ESR_ELx_SYS64_ISS_RT_SHIFT	5
+#define ESR_ELx_SYS64_ISS_RT_MASK	(UL(0x1f) << ESR_ELx_SYS64_ISS_RT_SHIFT)
+#define ESR_ELx_SYS64_ISS_CRm_SHIFT	1
+#define ESR_ELx_SYS64_ISS_CRm_MASK	(UL(0xf) << ESR_ELx_SYS64_ISS_CRm_SHIFT)
+#define ESR_ELx_SYS64_ISS_CRn_SHIFT	10
+#define ESR_ELx_SYS64_ISS_CRn_MASK	(UL(0xf) << ESR_ELx_SYS64_ISS_CRn_SHIFT)
+#define ESR_ELx_SYS64_ISS_Op1_SHIFT	14
+#define ESR_ELx_SYS64_ISS_Op1_MASK	(UL(0x7) << ESR_ELx_SYS64_ISS_Op1_SHIFT)
+#define ESR_ELx_SYS64_ISS_Op2_SHIFT	17
+#define ESR_ELx_SYS64_ISS_Op2_MASK	(UL(0x7) << ESR_ELx_SYS64_ISS_Op2_SHIFT)
+#define ESR_ELx_SYS64_ISS_Op0_SHIFT	20
+#define ESR_ELx_SYS64_ISS_Op0_MASK	(UL(0x3) << ESR_ELx_SYS64_ISS_Op0_SHIFT)
+#define ESR_ELx_SYS64_ISS_SYS_MASK	(ESR_ELx_SYS64_ISS_Op0_MASK | \
+					 ESR_ELx_SYS64_ISS_Op1_MASK | \
+					 ESR_ELx_SYS64_ISS_Op2_MASK | \
+					 ESR_ELx_SYS64_ISS_CRn_MASK | \
+					 ESR_ELx_SYS64_ISS_CRm_MASK)
+#define ESR_ELx_SYS64_ISS_SYS_VAL(Op0, Op1, Op2, CRn, CRm) \
+					(((Op0) << ESR_ELx_SYS64_ISS_Op0_SHIFT) | \
+					 ((Op1) << ESR_ELx_SYS64_ISS_Op1_SHIFT) | \
+					 ((Op2) << ESR_ELx_SYS64_ISS_Op2_SHIFT) | \
+					 ((CRn) << ESR_ELx_SYS64_ISS_CRn_SHIFT) | \
+					 ((CRm) << ESR_ELx_SYS64_ISS_CRm_SHIFT))
+/*
+ * User space cache operations have the following sysreg encoding
+ * in System instructions.
+ * Op0=1, Op1=3, Op2=1, CRn=7, CRm={ 5, 10, 11, 14 }, WRITE (L=0)
+ */
+#define ESR_ELx_SYS64_ISS_CRm_DC_CIVAC	14
+#define ESR_ELx_SYS64_ISS_CRm_DC_CVAU	11
+#define ESR_ELx_SYS64_ISS_CRm_DC_CVAC	10
+#define ESR_ELx_SYS64_ISS_CRm_IC_IVAU	5
+
+#define ESR_ELx_SYS64_ISS_U_CACHE_OP_MASK	(ESR_ELx_SYS64_ISS_Op0_MASK | \
+						 ESR_ELx_SYS64_ISS_Op1_MASK | \
+						 ESR_ELx_SYS64_ISS_Op2_MASK | \
+						 ESR_ELx_SYS64_ISS_CRn_MASK | \
+						 ESR_ELx_SYS64_ISS_DIR_MASK)
+#define ESR_ELx_SYS64_ISS_U_CACHE_OP_VAL \
+				(ESR_ELx_SYS64_ISS_SYS_VAL(1, 3, 1, 7, 0) | \
+				 ESR_ELx_SYS64_ISS_DIR_WRITE)
 #ifndef __ASSEMBLY__
 #include <asm/types.h>
 
diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
index e04f838..93c5287 100644
--- a/arch/arm64/kernel/traps.c
+++ b/arch/arm64/kernel/traps.c
@@ -447,36 +447,29 @@ void cpu_enable_cache_maint_trap(void *__unused)
 		: "=r" (res)					\
 		: "r" (address), "i" (-EFAULT) )
 
-asmlinkage void __exception do_sysinstr(unsigned int esr, struct pt_regs *regs)
+static void user_cache_maint_handler(unsigned int esr, struct pt_regs *regs)
 {
 	unsigned long address;
-	int ret;
+	int rt = (esr & ESR_ELx_SYS64_ISS_RT_MASK) >> ESR_ELx_SYS64_ISS_RT_SHIFT;
+	int crm = (esr & ESR_ELx_SYS64_ISS_CRm_MASK) >> ESR_ELx_SYS64_ISS_CRm_SHIFT;
+	int ret = 0;
 
-	/* if this is a write with: Op0=1, Op2=1, Op1=3, CRn=7 */
-	if ((esr & 0x01fffc01) == 0x0012dc00) {
-		int rt = (esr >> 5) & 0x1f;
-		int crm = (esr >> 1) & 0x0f;
+	address = (rt == 31) ? 0 : regs->regs[rt];
 
-		address = (rt == 31) ? 0 : regs->regs[rt];
-
-		switch (crm) {
-		case 11:		/* DC CVAU, gets promoted */
-			__user_cache_maint("dc civac", address, ret);
-			break;
-		case 10:		/* DC CVAC, gets promoted */
-			__user_cache_maint("dc civac", address, ret);
-			break;
-		case 14:		/* DC CIVAC */
-			__user_cache_maint("dc civac", address, ret);
-			break;
-		case 5:			/* IC IVAU */
-			__user_cache_maint("ic ivau", address, ret);
-			break;
-		default:
-			force_signal_inject(SIGILL, ILL_ILLOPC, regs, 0);
-			return;
-		}
-	} else {
+	switch (crm) {
+	case ESR_ELx_SYS64_ISS_CRm_DC_CVAU:	/* DC CVAU, gets promoted */
+		__user_cache_maint("dc civac", address, ret);
+		break;
+	case ESR_ELx_SYS64_ISS_CRm_DC_CVAC:	/* DC CVAC, gets promoted */
+		__user_cache_maint("dc civac", address, ret);
+		break;
+	case ESR_ELx_SYS64_ISS_CRm_DC_CIVAC:	/* DC CIVAC */
+		__user_cache_maint("dc civac", address, ret);
+		break;
+	case ESR_ELx_SYS64_ISS_CRm_IC_IVAU:	/* IC IVAU */
+		__user_cache_maint("ic ivau", address, ret);
+		break;
+	default:
 		force_signal_inject(SIGILL, ILL_ILLOPC, regs, 0);
 		return;
 	}
@@ -487,6 +480,34 @@ asmlinkage void __exception do_sysinstr(unsigned int esr, struct pt_regs *regs)
 		regs->pc += 4;
 }
 
+struct sys64_hook {
+	unsigned int esr_mask;
+	unsigned int esr_val;
+	void (*handler)(unsigned int esr, struct pt_regs *regs);
+};
+
+static struct sys64_hook sys64_hooks[] = {
+	{
+		.esr_mask = ESR_ELx_SYS64_ISS_U_CACHE_OP_MASK,
+		.esr_val = ESR_ELx_SYS64_ISS_U_CACHE_OP_VAL,
+		.handler = user_cache_maint_handler,
+	},
+	{},
+};
+
+asmlinkage void __exception do_sysinstr(unsigned int esr, struct pt_regs *regs)
+{
+	struct sys64_hook *hook;
+
+	for (hook = sys64_hooks; hook->handler; hook++)
+		if ((hook->esr_mask & esr) == hook->esr_val) {
+			hook->handler(esr, regs);
+			return;
+		}
+
+	force_signal_inject(SIGILL, ILL_ILLOPC, regs, 0);
+}
+
 long compat_arm_syscall(struct pt_regs *regs);
 
 asmlinkage long do_ni_syscall(struct pt_regs *regs)
-- 
2.7.4

[toc] | [next] | [standalone]

#1467595

FromWill Deacon <will.deacon@arm.com>
Date2016-08-22 15:00 +0200
Message-ID<s8WZA-6pI-31@gated-at.bofh.it>
In reply to#1465277
On Thu, Aug 18, 2016 at 02:10:31PM +0100, Suzuki K Poulose wrote:
> Right now we trap some of the user space data cache operations
> based on a few Errata (ARM 819472, 826319, 827319 and 824069).
> We need to trap userspace access to CTR_EL0, if we detect mismatched
> cache line size. Since both these traps share the EC, refactor
> the handler a little bit to make it a bit more reader friendly.
> 
> Cc: Andre Przywara <andre.przywara@arm.com>
> 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/esr.h | 48 +++++++++++++++++++++++++++++
>  arch/arm64/kernel/traps.c    | 73 ++++++++++++++++++++++++++++----------------
>  2 files changed, 95 insertions(+), 26 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/esr.h b/arch/arm64/include/asm/esr.h
> index f772e15..2a8f6c3 100644
> --- a/arch/arm64/include/asm/esr.h
> +++ b/arch/arm64/include/asm/esr.h
> @@ -109,6 +109,54 @@
>  	((ESR_ELx_EC_BRK64 << ESR_ELx_EC_SHIFT) | ESR_ELx_IL |	\
>  	 ((imm) & 0xffff))
>  
> +/* ISS field definitions for System instruction traps */

Can you add a similar comment for the ESR_ELx_* encodings that we already
have, please? Unfortunately, we've not namespaced things, so the
data/instruction abort encodings are described as e.g. ESR_ELx_ISV.

> +#define ESR_ELx_SYS64_ISS_RES0_SHIFT	22
> +#define ESR_ELx_SYS64_ISS_RES0_MASK	(UL(0x7) << ESR_ELx_SYS64_ISS_RES0_SHIFT)
> +#define ESR_ELx_SYS64_ISS_DIR_MASK	0x1
> +#define ESR_ELx_SYS64_ISS_DIR_READ	0x1
> +#define ESR_ELx_SYS64_ISS_DIR_WRITE	0x0
> +
> +#define ESR_ELx_SYS64_ISS_RT_SHIFT	5
> +#define ESR_ELx_SYS64_ISS_RT_MASK	(UL(0x1f) << ESR_ELx_SYS64_ISS_RT_SHIFT)
> +#define ESR_ELx_SYS64_ISS_CRm_SHIFT	1
> +#define ESR_ELx_SYS64_ISS_CRm_MASK	(UL(0xf) << ESR_ELx_SYS64_ISS_CRm_SHIFT)
> +#define ESR_ELx_SYS64_ISS_CRn_SHIFT	10
> +#define ESR_ELx_SYS64_ISS_CRn_MASK	(UL(0xf) << ESR_ELx_SYS64_ISS_CRn_SHIFT)
> +#define ESR_ELx_SYS64_ISS_Op1_SHIFT	14
> +#define ESR_ELx_SYS64_ISS_Op1_MASK	(UL(0x7) << ESR_ELx_SYS64_ISS_Op1_SHIFT)
> +#define ESR_ELx_SYS64_ISS_Op2_SHIFT	17
> +#define ESR_ELx_SYS64_ISS_Op2_MASK	(UL(0x7) << ESR_ELx_SYS64_ISS_Op2_SHIFT)
> +#define ESR_ELx_SYS64_ISS_Op0_SHIFT	20
> +#define ESR_ELx_SYS64_ISS_Op0_MASK	(UL(0x3) << ESR_ELx_SYS64_ISS_Op0_SHIFT)

Inconsistent capitalisation in your macro naming (e.g. RT vs Op1). Maybe
just stick to uppercase for #defines?

> +#define ESR_ELx_SYS64_ISS_SYS_MASK	(ESR_ELx_SYS64_ISS_Op0_MASK | \
> +					 ESR_ELx_SYS64_ISS_Op1_MASK | \
> +					 ESR_ELx_SYS64_ISS_Op2_MASK | \
> +					 ESR_ELx_SYS64_ISS_CRn_MASK | \
> +					 ESR_ELx_SYS64_ISS_CRm_MASK)
> +#define ESR_ELx_SYS64_ISS_SYS_VAL(Op0, Op1, Op2, CRn, CRm) \
> +					(((Op0) << ESR_ELx_SYS64_ISS_Op0_SHIFT) | \
> +					 ((Op1) << ESR_ELx_SYS64_ISS_Op1_SHIFT) | \
> +					 ((Op2) << ESR_ELx_SYS64_ISS_Op2_SHIFT) | \
> +					 ((CRn) << ESR_ELx_SYS64_ISS_CRn_SHIFT) | \
> +					 ((CRm) << ESR_ELx_SYS64_ISS_CRm_SHIFT))
> +/*
> + * User space cache operations have the following sysreg encoding
> + * in System instructions.
> + * Op0=1, Op1=3, Op2=1, CRn=7, CRm={ 5, 10, 11, 14 }, WRITE (L=0)
> + */
> +#define ESR_ELx_SYS64_ISS_CRm_DC_CIVAC	14
> +#define ESR_ELx_SYS64_ISS_CRm_DC_CVAU	11
> +#define ESR_ELx_SYS64_ISS_CRm_DC_CVAC	10
> +#define ESR_ELx_SYS64_ISS_CRm_IC_IVAU	5
> +
> +#define ESR_ELx_SYS64_ISS_U_CACHE_OP_MASK	(ESR_ELx_SYS64_ISS_Op0_MASK | \
> +						 ESR_ELx_SYS64_ISS_Op1_MASK | \
> +						 ESR_ELx_SYS64_ISS_Op2_MASK | \
> +						 ESR_ELx_SYS64_ISS_CRn_MASK | \
> +						 ESR_ELx_SYS64_ISS_DIR_MASK)
> +#define ESR_ELx_SYS64_ISS_U_CACHE_OP_VAL \
> +				(ESR_ELx_SYS64_ISS_SYS_VAL(1, 3, 1, 7, 0) | \
> +				 ESR_ELx_SYS64_ISS_DIR_WRITE)

What is the _U_ for? Unified? User? If it's user, EL0 may be more
appropriate.

Will

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

#1468465

FromSuzuki K Poulose <Suzuki.Poulose@arm.com>
Date2016-08-23 12:20 +0200
Message-ID<s9gYi-2wu-15@gated-at.bofh.it>
In reply to#1467595
On 22/08/16 13:53, Will Deacon wrote:
> On Thu, Aug 18, 2016 at 02:10:31PM +0100, Suzuki K Poulose wrote:
>> Right now we trap some of the user space data cache operations
>> based on a few Errata (ARM 819472, 826319, 827319 and 824069).
>> We need to trap userspace access to CTR_EL0, if we detect mismatched
>> cache line size. Since both these traps share the EC, refactor
>> the handler a little bit to make it a bit more reader friendly.
>>
>> Cc: Andre Przywara <andre.przywara@arm.com>
>> 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/esr.h | 48 +++++++++++++++++++++++++++++
>>  arch/arm64/kernel/traps.c    | 73 ++++++++++++++++++++++++++++----------------
>>  2 files changed, 95 insertions(+), 26 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/esr.h b/arch/arm64/include/asm/esr.h
>> index f772e15..2a8f6c3 100644
>> --- a/arch/arm64/include/asm/esr.h
>> +++ b/arch/arm64/include/asm/esr.h
>> @@ -109,6 +109,54 @@
>>  	((ESR_ELx_EC_BRK64 << ESR_ELx_EC_SHIFT) | ESR_ELx_IL |	\
>>  	 ((imm) & 0xffff))
>>
>> +/* ISS field definitions for System instruction traps */
>
> Can you add a similar comment for the ESR_ELx_* encodings that we already
> have, please? Unfortunately, we've not namespaced things, so the
> data/instruction abort encodings are described as e.g. ESR_ELx_ISV.

Will do.


>> +#define ESR_ELx_SYS64_ISS_Op0_SHIFT	20
>> +#define ESR_ELx_SYS64_ISS_Op0_MASK	(UL(0x3) << ESR_ELx_SYS64_ISS_Op0_SHIFT)
>
> Inconsistent capitalisation in your macro naming (e.g. RT vs Op1). Maybe
> just stick to uppercase for #defines?

Sure

>> +
>> +#define ESR_ELx_SYS64_ISS_U_CACHE_OP_MASK	(ESR_ELx_SYS64_ISS_Op0_MASK | \
>> +						 ESR_ELx_SYS64_ISS_Op1_MASK | \
>> +						 ESR_ELx_SYS64_ISS_Op2_MASK | \
>> +						 ESR_ELx_SYS64_ISS_CRn_MASK | \
>> +						 ESR_ELx_SYS64_ISS_DIR_MASK)
>> +#define ESR_ELx_SYS64_ISS_U_CACHE_OP_VAL \
>> +				(ESR_ELx_SYS64_ISS_SYS_VAL(1, 3, 1, 7, 0) | \
>> +				 ESR_ELx_SYS64_ISS_DIR_WRITE)
>
> What is the _U_ for? Unified? User? If it's user, EL0 may be more
> appropriate.

Ok.
  


Thanks
Suzuki

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web