arm64: replace open-coded {msr, mrs} with {read, write}_sysreg()

PATCH New Mark Rutland 3 participants 9 messages Sep 8, 2016

Subsystems (4)

arm64 KVM arm64 Arm PMU profiling and debugging Arm architected timer driver

lore

Share & tools

lore

Mark Rutland

author maintainer

2016-09-08 12:55 UTC #0 lore raw

cover arm64: replace open-coded {msr, mrs} with {read, write}_sysreg()

A while back we added {read,write}_sysreg accessors to handle accesses to system registers, without the usual boilerplate asm volatile, temporary variable, etc.

These patches replace asm blocks that perform mrs/msr with {read,write}_sysreg(), and remove code and comments that this change makes unnecessary. Where {read,write}_sysreg() are not appropriate, comments are added to explain why.

This gets rid of a reasonable amount of code, and makes what remains easier to follow.

Thanks,
Mark.

Mark Rutland (6):
arm64: sysreg: allow write_sysreg to use XZR
arm64: arch_timer: simplify accessors
arm64: dcc: simplify accessors
arm64/kvm: use {read,write}_sysreg()
arm64: simplify sysreg manipulation
arm64: simplify contextidr_thread_switch

 arch/arm64/include/asm/arch_timer.h    | 41 ++++++++++++----------------------
 arch/arm64/include/asm/dcc.h           | 14 ++++--------
 arch/arm64/include/asm/hw_breakpoint.h | 13 ++++++-----
 arch/arm64/include/asm/mmu_context.h   | 36 +++++++++++------------------
 arch/arm64/include/asm/pgtable-hwdef.h |  1 +
 arch/arm64/include/asm/sysreg.h        | 28 +++++++++++++----------
 arch/arm64/include/asm/thread_info.h   |  3 +++
 arch/arm64/include/asm/virt.h          |  6 ++---
 arch/arm64/kernel/cacheinfo.c          |  8 +++----
 arch/arm64/kernel/debug-monitors.c     |  8 +++----
 arch/arm64/kernel/process.c            | 14 +++++-------
 arch/arm64/kernel/sys_compat.c         |  2 +-
 arch/arm64/kvm/sys_regs.c              | 31 +++++++++----------------
 arch/arm64/kvm/sys_regs_generic_v8.c   |  6 ++---
 14 files changed, 86 insertions(+), 125 deletions(-)

Signature · 1 line

1.9.1

Mark Rutland

author maintainer

2016-09-08 12:55 UTC #1 lore raw

PATCH 1/6 arm64: sysreg: allow write_sysreg to use XZR

Currently write_sysreg has to allocate a temporary register to write zero to a system register, which is unfortunate given that the MSR instruction accepts XZR as an operand.

Allow XZR to be used when appropriate by fiddling with the assembly constraints.

Signed-off-by Mark Rutland <[email protected]>

Cc Catalin Marinas <[email protected]>

Cc Marc Zyngier <[email protected]>

Cc Robin Murphy <[email protected]>

Cc Suzuki K Poulose <[email protected]>

Cc Will Deacon <[email protected]>

---
 arch/arm64/include/asm/sysreg.h | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

arch/arm64/include/asm/sysreg.h +6 -2

index cc06794..39fed2e 100644
@@ -273,10 +273,14 @@ static inline void config_sctlr_el1(u32 clear, u32 set)
 	__val;							\
 })
 
+/*
+ * The "Z" constraint normally means a zero immediate, but when combined with
+ * the "%x0" template means XZR.
+ */
 #define write_sysreg(v, r) do {					\
 	u64 __val = (u64)v;					\
-	asm volatile("msr " __stringify(r) ", %0"		\
-		     : : "r" (__val));				\
+	asm volatile("msr " __stringify(r) ", %x0"		\
+		     : : "rZ" (__val));				\
 } while (0)
 
 #endif

Signature · 1 line

1.9.1

Robin Murphy

2016-09-08 14:12 UTC #2 lore raw

PATCH 1/6 arm64: sysreg: allow write_sysreg to use XZR

Quoted 22 lines · Mark Rutland

Currently write_sysreg has to allocate a temporary register to write zero to a system register, which is unfortunate given that the MSR instruction accepts XZR as an operand.
Allow XZR to be used when appropriate by fiddling with the assembly constraints.
Signed-off-by Mark Rutland <[email protected]>
Cc Catalin Marinas <[email protected]>
Cc Marc Zyngier <[email protected]>
Cc Robin Murphy <[email protected]>
Cc Suzuki K Poulose <[email protected]>
Cc Will Deacon <[email protected]>
---
 arch/arm64/include/asm/sysreg.h | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)
arch/arm64/include/asm/sysreg.h +0 -0
index cc06794..39fed2e 100644

arch/arm64/include/asm/sysreg.h static inline void config_sctlr_el1(u32 clear, u32 set) · line 279

Quoted 7 lines

arch/arm64/include/asm/sysreg.h +4 -0

@@ -273,10 +273,14 @@ static inline void config_sctlr_el1(u32 clear, u32 set)
 	__val;							\
 })
 
+/*
+ * The "Z" constraint normally means a zero immediate, but when combined with
+ * the "%x0" template means XZR.
+ */

Personally I find that comment more confusing than useful - "Z" is documented for AArch64 as "Integer constant zero", which to me doesn't necessarily imply an immediate; it's just not necessarily immediately intuitive that that can be an immediate constant or a 'register constant' depending on exactly how it is referenced.

Either way, though:

Reviewed-by Robin Murphy <[email protected]>

Quoted 10 lines

arch/arm64/include/asm/sysreg.h +2 -2

 #define write_sysreg(v, r) do {					\
 	u64 __val = (u64)v;					\
-	asm volatile("msr " __stringify(r) ", %0"		\
-		     : : "r" (__val));				\
+	asm volatile("msr " __stringify(r) ", %x0"		\
+		     : : "rZ" (__val));				\
 } while (0)
 
 #endif

Mark Rutland

author maintainer

2016-09-08 12:55 UTC #3 lore raw

PATCH 2/6 arm64: arch_timer: simplify accessors

In reply to Mark Rutland

A while back we added {read,write}_sysreg accessors to handle accesses to system registers, without the usual boilerplate asm volatile, temporary variable, etc.

This patch makes use of these in the arm64 arch timer accessors to make the code shorter and clearer.

Signed-off-by Mark Rutland <[email protected]>

Cc Catalin Marinas <[email protected]>

Cc Marc Zyngier <[email protected]>

Cc Will Deacon <[email protected]>

---
 arch/arm64/include/asm/arch_timer.h | 41 +++++++++++++------------------------
 1 file changed, 14 insertions(+), 27 deletions(-)

arch/arm64/include/asm/arch_timer.h +14 ~~-27~~

index fbe0ca3..7ff386c 100644
@@ -20,6 +20,7 @@
 #define __ASM_ARCH_TIMER_H
 
 #include <asm/barrier.h>
+#include <asm/sysreg.h>
 
 #include <linux/bug.h>
 #include <linux/init.h>

@@ -38,19 +39,19 @@ void arch_timer_reg_write_cp15(int access, enum arch_timer_reg reg, u32 val)
 	if (access == ARCH_TIMER_PHYS_ACCESS) {
 		switch (reg) {
 		case ARCH_TIMER_REG_CTRL:
-			asm volatile("msr cntp_ctl_el0,  %0" : : "r" (val));
+			write_sysreg(val, cntp_ctl_el0);
 			break;
 		case ARCH_TIMER_REG_TVAL:
-			asm volatile("msr cntp_tval_el0, %0" : : "r" (val));
+			write_sysreg(val, cntp_tval_el0);
 			break;
 		}
 	} else if (access == ARCH_TIMER_VIRT_ACCESS) {
 		switch (reg) {
 		case ARCH_TIMER_REG_CTRL:
-			asm volatile("msr cntv_ctl_el0,  %0" : : "r" (val));
+			write_sysreg(val, cntv_ctl_el0);
 			break;
 		case ARCH_TIMER_REG_TVAL:
-			asm volatile("msr cntv_tval_el0, %0" : : "r" (val));
+			write_sysreg(val, cntv_tval_el0);
 			break;
 		}
 	}

@@ -61,48 +62,38 @@ void arch_timer_reg_write_cp15(int access, enum arch_timer_reg reg, u32 val)
 static __always_inline
 u32 arch_timer_reg_read_cp15(int access, enum arch_timer_reg reg)
 {
-	u32 val;
-
 	if (access == ARCH_TIMER_PHYS_ACCESS) {
 		switch (reg) {
 		case ARCH_TIMER_REG_CTRL:
-			asm volatile("mrs %0,  cntp_ctl_el0" : "=r" (val));
-			break;
+			return read_sysreg(cntp_ctl_el0);
 		case ARCH_TIMER_REG_TVAL:
-			asm volatile("mrs %0, cntp_tval_el0" : "=r" (val));
-			break;
+			return read_sysreg(cntp_tval_el0);
 		}
 	} else if (access == ARCH_TIMER_VIRT_ACCESS) {
 		switch (reg) {
 		case ARCH_TIMER_REG_CTRL:
-			asm volatile("mrs %0,  cntv_ctl_el0" : "=r" (val));
-			break;
+			return read_sysreg(cntv_ctl_el0);
 		case ARCH_TIMER_REG_TVAL:
-			asm volatile("mrs %0, cntv_tval_el0" : "=r" (val));
-			break;
+			return read_sysreg(cntv_tval_el0);
 		}
 	}
 
-	return val;
+	BUG();
 }
 
 static inline u32 arch_timer_get_cntfrq(void)
 {
-	u32 val;
-	asm volatile("mrs %0,   cntfrq_el0" : "=r" (val));
-	return val;
+	return read_sysreg(cntfrq_el0);
 }
 
 static inline u32 arch_timer_get_cntkctl(void)
 {
-	u32 cntkctl;
-	asm volatile("mrs	%0, cntkctl_el1" : "=r" (cntkctl));
-	return cntkctl;
+	return read_sysreg(cntkctl_el1);
 }
 
 static inline void arch_timer_set_cntkctl(u32 cntkctl)
 {
-	asm volatile("msr	cntkctl_el1, %0" : : "r" (cntkctl));
+	write_sysreg(cntkctl, cntkctl_el1);
 }
 
 static inline u64 arch_counter_get_cntpct(void)

@@ -116,12 +107,8 @@ static inline u64 arch_counter_get_cntpct(void)
 
 static inline u64 arch_counter_get_cntvct(void)
 {
-	u64 cval;
-
 	isb();
-	asm volatile("mrs %0, cntvct_el0" : "=r" (cval));
-
-	return cval;
+	return read_sysreg(cntvct_el0);
 }
 
 static inline int arch_timer_arch_init(void)

Signature · 1 line

1.9.1

Mark Rutland

author maintainer

2016-09-08 12:55 UTC #4 lore raw

PATCH 3/6 arm64: dcc: simplify accessors

In reply to Mark Rutland

A while back we added {read,write}_sysreg accessors to handle accesses to system registers, without the usual boilerplate asm volatile, temporary variable, etc.

This patch makes use of these in the arm64 DCC accessors to make the code shorter and clearer.

Signed-off-by Mark Rutland <[email protected]>

Cc Catalin Marinas <[email protected]>

Cc Will Deacon <[email protected]>

---
 arch/arm64/include/asm/dcc.h | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

arch/arm64/include/asm/dcc.h +4 ~~-10~~

index 65e0190..836b056 100644
@@ -21,21 +21,16 @@
 #define __ASM_DCC_H
 
 #include <asm/barrier.h>
+#include <asm/sysreg.h>
 
 static inline u32 __dcc_getstatus(void)
 {
-	u32 ret;
-
-	asm volatile("mrs %0, mdccsr_el0" : "=r" (ret));
-
-	return ret;
+	return read_sysreg(mdccsr_el0);
 }
 
 static inline char __dcc_getchar(void)
 {
-	char c;
-
-	asm volatile("mrs %0, dbgdtrrx_el0" : "=r" (c));
+	char c = read_sysreg(dbgdtrrx_el0);
 	isb();
 
 	return c;

@@ -47,8 +42,7 @@ static inline void __dcc_putchar(char c)
 	 * The typecast is to make absolutely certain that 'c' is
 	 * zero-extended.
 	 */
-	asm volatile("msr dbgdtrtx_el0, %0"
-			: : "r" ((unsigned long)(unsigned char)c));
+	write_sysreg((unsigned char)c, dbgdtrtx_el0);
 	isb();
 }

Signature · 1 line

1.9.1

Mark Rutland

author maintainer

2016-09-08 12:55 UTC #5 lore raw

PATCH 4/6 arm64/kvm: use {read,write}_sysreg()

In reply to Mark Rutland

A while back we added {read,write}_sysreg accessors to handle accesses to system registers, without the usual boilerplate asm volatile, temporary variable, etc.

This patch makes use of these in the arm64 KVM code to make the code shorter and clearer.

At the same time, a comment style violation next to a system register access is fixed up in reset_pmcr, and comments describing whether operations are reads or writes are removed as this is now painfully obvious.

Signed-off-by Mark Rutland <[email protected]>

Cc Catalin Marinas <[email protected]>

Cc Christoffer Dall <[email protected]>

Cc Marc Zyngier <[email protected]>

Cc Will Deacon <[email protected]>

---
 arch/arm64/include/asm/virt.h        |  6 ++----
 arch/arm64/kvm/sys_regs.c            | 31 +++++++++++--------------------
 arch/arm64/kvm/sys_regs_generic_v8.c |  6 ++----
 3 files changed, 15 insertions(+), 28 deletions(-)

arch/arm64/include/asm/virt.h +2 -4

index 1788545..c5cc940 100644
@@ -45,6 +45,7 @@
 #ifndef __ASSEMBLY__
 
 #include <asm/ptrace.h>
+#include <asm/sysreg.h>
 
 /*
  * __boot_cpu_mode records what mode CPUs were booted in.

@@ -75,10 +76,7 @@ static inline bool is_hyp_mode_mismatched(void)
 
 static inline bool is_kernel_in_hyp_mode(void)
 {
-	u64 el;
-
-	asm("mrs %0, CurrentEL" : "=r" (el));
-	return el == CurrentEL_EL2;
+	return read_sysreg(CurrentEL) == CurrentEL_EL2;
 }
 
 #ifdef CONFIG_ARM64_VHE

arch/arm64/kvm/sys_regs.c +11 ~~-20~~

index e51367d..f302fdb 100644
@@ -36,6 +36,7 @@
 #include <asm/kvm_host.h>
 #include <asm/kvm_mmu.h>
 #include <asm/perf_event.h>
+#include <asm/sysreg.h>
 
 #include <trace/events/kvm.h>

@@ -67,11 +68,9 @@ static u32 get_ccsidr(u32 csselr)
 
 	/* Make sure noone else changes CSSELR during this! */
 	local_irq_disable();
-	/* Put value into CSSELR */
-	asm volatile("msr csselr_el1, %x0" : : "r" (csselr));
+	write_sysreg(csselr, csselr_el1);
 	isb();
-	/* Read result out of CCSIDR */
-	asm volatile("mrs %0, ccsidr_el1" : "=r" (ccsidr));
+	ccsidr = read_sysreg(ccsidr_el1);
 	local_irq_enable();
 
 	return ccsidr;

@@ -174,9 +173,7 @@ static bool trap_dbgauthstatus_el1(struct kvm_vcpu *vcpu,
 	if (p->is_write) {
 		return ignore_write(vcpu, p);
 	} else {
-		u32 val;
-		asm volatile("mrs %0, dbgauthstatus_el1" : "=r" (val));
-		p->regval = val;
+		p->regval = read_sysreg(dbgauthstatus_el1);
 		return true;
 	}
 }

@@ -429,10 +426,7 @@ static void reset_wcr(struct kvm_vcpu *vcpu,
 
 static void reset_amair_el1(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
 {
-	u64 amair;
-
-	asm volatile("mrs %0, amair_el1\n" : "=r" (amair));
-	vcpu_sys_reg(vcpu, AMAIR_EL1) = amair;
+	vcpu_sys_reg(vcpu, AMAIR_EL1) = read_sysreg(amair_el1);
 }
 
 static void reset_mpidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)

@@ -456,8 +450,9 @@ static void reset_pmcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
 {
 	u64 pmcr, val;
 
-	asm volatile("mrs %0, pmcr_el0\n" : "=r" (pmcr));
-	/* Writable bits of PMCR_EL0 (ARMV8_PMU_PMCR_MASK) is reset to UNKNOWN
+	pmcr = read_sysreg(pmcr_el0);
+	/*
+	 * Writable bits of PMCR_EL0 (ARMV8_PMU_PMCR_MASK) are reset to UNKNOWN
 	 * except PMCR.E resetting to zero.
 	 */
 	val = ((pmcr & ~ARMV8_PMU_PMCR_MASK)

@@ -557,9 +552,9 @@ static bool access_pmceid(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
 		return false;
 
 	if (!(p->Op2 & 1))
-		asm volatile("mrs %0, pmceid0_el0\n" : "=r" (pmceid));
+		pmceid = read_sysreg(pmceid0_el0);
 	else
-		asm volatile("mrs %0, pmceid1_el0\n" : "=r" (pmceid));
+		pmceid = read_sysreg(pmceid1_el0);
 
 	p->regval = pmceid;

@@ -1833,11 +1828,7 @@ static const struct sys_reg_desc *index_to_sys_reg_desc(struct kvm_vcpu *vcpu,
 	static void get_##reg(struct kvm_vcpu *v,			\
 			      const struct sys_reg_desc *r)		\
 	{								\
-		u64 val;						\
-									\
-		asm volatile("mrs %0, " __stringify(reg) "\n"		\
-			     : "=r" (val));				\
-		((struct sys_reg_desc *)r)->val = val;			\
+		((struct sys_reg_desc *)r)->val = read_sysreg(reg);	\
 	}
 
 FUNCTION_INVARIANT(midr_el1)

arch/arm64/kvm/sys_regs_generic_v8.c +2 -4

index ed90578..46af718 100644
@@ -26,6 +26,7 @@
 #include <asm/kvm_host.h>
 #include <asm/kvm_emulate.h>
 #include <asm/kvm_coproc.h>
+#include <asm/sysreg.h>
 #include <linux/init.h>
 
 #include "sys_regs.h"

@@ -43,10 +44,7 @@ static bool access_actlr(struct kvm_vcpu *vcpu,
 
 static void reset_actlr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
 {
-	u64 actlr;
-
-	asm volatile("mrs %0, actlr_el1\n" : "=r" (actlr));
-	vcpu_sys_reg(vcpu, ACTLR_EL1) = actlr;
+	vcpu_sys_reg(vcpu, ACTLR_EL1) = read_sysreg(actlr_el1);
 }
 
 /*

Signature · 1 line

1.9.1

Christoffer Dall

2016-09-08 13:34 UTC #6 lore raw

PATCH 4/6 arm64/kvm: use {read,write}_sysreg()

Quoted 11 lines · Mark Rutland

A while back we added {read,write}_sysreg accessors to handle accesses to system registers, without the usual boilerplate asm volatile, temporary variable, etc.
This patch makes use of these in the arm64 KVM code to make the code shorter and clearer.
At the same time, a comment style violation next to a system register access is fixed up in reset_pmcr, and comments describing whether operations are reads or writes are removed as this is now painfully obvious.

Acked-by Christoffer Dall <[email protected]>

Quoted 149 lines

Signed-off-by Mark Rutland <[email protected]>

Cc Catalin Marinas <[email protected]>

Cc Christoffer Dall <[email protected]>

Cc Marc Zyngier <[email protected]>

Cc Will Deacon <[email protected]>

---
 arch/arm64/include/asm/virt.h        |  6 ++----
 arch/arm64/kvm/sys_regs.c            | 31 +++++++++++--------------------
 arch/arm64/kvm/sys_regs_generic_v8.c |  6 ++----
 3 files changed, 15 insertions(+), 28 deletions(-)

arch/arm64/include/asm/virt.h +2 -4

index 1788545..c5cc940 100644
@@ -45,6 +45,7 @@
 #ifndef __ASSEMBLY__
 
 #include <asm/ptrace.h>
+#include <asm/sysreg.h>
 
 /*
  * __boot_cpu_mode records what mode CPUs were booted in.

@@ -75,10 +76,7 @@ static inline bool is_hyp_mode_mismatched(void)
 
 static inline bool is_kernel_in_hyp_mode(void)
 {
-	u64 el;
-
-	asm("mrs %0, CurrentEL" : "=r" (el));
-	return el == CurrentEL_EL2;
+	return read_sysreg(CurrentEL) == CurrentEL_EL2;
 }
 
 #ifdef CONFIG_ARM64_VHE

arch/arm64/kvm/sys_regs.c +11 ~~-20~~

index e51367d..f302fdb 100644
@@ -36,6 +36,7 @@
 #include <asm/kvm_host.h>
 #include <asm/kvm_mmu.h>
 #include <asm/perf_event.h>
+#include <asm/sysreg.h>
 
 #include <trace/events/kvm.h>

@@ -67,11 +68,9 @@ static u32 get_ccsidr(u32 csselr)
 
 	/* Make sure noone else changes CSSELR during this! */
 	local_irq_disable();
-	/* Put value into CSSELR */
-	asm volatile("msr csselr_el1, %x0" : : "r" (csselr));
+	write_sysreg(csselr, csselr_el1);
 	isb();
-	/* Read result out of CCSIDR */
-	asm volatile("mrs %0, ccsidr_el1" : "=r" (ccsidr));
+	ccsidr = read_sysreg(ccsidr_el1);
 	local_irq_enable();
 
 	return ccsidr;

@@ -174,9 +173,7 @@ static bool trap_dbgauthstatus_el1(struct kvm_vcpu *vcpu,
 	if (p->is_write) {
 		return ignore_write(vcpu, p);
 	} else {
-		u32 val;
-		asm volatile("mrs %0, dbgauthstatus_el1" : "=r" (val));
-		p->regval = val;
+		p->regval = read_sysreg(dbgauthstatus_el1);
 		return true;
 	}
 }

@@ -429,10 +426,7 @@ static void reset_wcr(struct kvm_vcpu *vcpu,
 
 static void reset_amair_el1(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
 {
-	u64 amair;
-
-	asm volatile("mrs %0, amair_el1\n" : "=r" (amair));
-	vcpu_sys_reg(vcpu, AMAIR_EL1) = amair;
+	vcpu_sys_reg(vcpu, AMAIR_EL1) = read_sysreg(amair_el1);
 }
 
 static void reset_mpidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)

@@ -456,8 +450,9 @@ static void reset_pmcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
 {
 	u64 pmcr, val;
 
-	asm volatile("mrs %0, pmcr_el0\n" : "=r" (pmcr));
-	/* Writable bits of PMCR_EL0 (ARMV8_PMU_PMCR_MASK) is reset to UNKNOWN
+	pmcr = read_sysreg(pmcr_el0);
+	/*
+	 * Writable bits of PMCR_EL0 (ARMV8_PMU_PMCR_MASK) are reset to UNKNOWN
 	 * except PMCR.E resetting to zero.
 	 */
 	val = ((pmcr & ~ARMV8_PMU_PMCR_MASK)

@@ -557,9 +552,9 @@ static bool access_pmceid(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
 		return false;
 
 	if (!(p->Op2 & 1))
-		asm volatile("mrs %0, pmceid0_el0\n" : "=r" (pmceid));
+		pmceid = read_sysreg(pmceid0_el0);
 	else
-		asm volatile("mrs %0, pmceid1_el0\n" : "=r" (pmceid));
+		pmceid = read_sysreg(pmceid1_el0);
 
 	p->regval = pmceid;

@@ -1833,11 +1828,7 @@ static const struct sys_reg_desc *index_to_sys_reg_desc(struct kvm_vcpu *vcpu,
 	static void get_##reg(struct kvm_vcpu *v,			\
 			      const struct sys_reg_desc *r)		\
 	{								\
-		u64 val;						\
-									\
-		asm volatile("mrs %0, " __stringify(reg) "\n"		\
-			     : "=r" (val));				\
-		((struct sys_reg_desc *)r)->val = val;			\
+		((struct sys_reg_desc *)r)->val = read_sysreg(reg);	\
 	}
 
 FUNCTION_INVARIANT(midr_el1)

arch/arm64/kvm/sys_regs_generic_v8.c +2 -4

index ed90578..46af718 100644
@@ -26,6 +26,7 @@
 #include <asm/kvm_host.h>
 #include <asm/kvm_emulate.h>
 #include <asm/kvm_coproc.h>
+#include <asm/sysreg.h>
 #include <linux/init.h>
 
 #include "sys_regs.h"

@@ -43,10 +44,7 @@ static bool access_actlr(struct kvm_vcpu *vcpu,
 
 static void reset_actlr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
 {
-	u64 actlr;
-
-	asm volatile("mrs %0, actlr_el1\n" : "=r" (actlr));
-	vcpu_sys_reg(vcpu, ACTLR_EL1) = actlr;
+	vcpu_sys_reg(vcpu, ACTLR_EL1) = read_sysreg(actlr_el1);
 }
 
 /*

Signature · 1 line

1.9.1

Mark Rutland

author maintainer

2016-09-08 12:55 UTC #7 lore raw

PATCH 5/6 arm64: simplify sysreg manipulation

In reply to Mark Rutland

A while back we added {read,write}_sysreg accessors to handle accesses to system registers, without the usual boilerplate asm volatile, temporary variable, etc.

This patch makes use of these across arm64 to make code shorter and clearer. For sequences with a trailing ISB, the existing isb() macro is also used so that asm blocks can be removed entirely.

A few uses of inline assembly for msr/mrs are left as-is. Those manipulating sp_el0 for the current thread_info value have special clobber requiremends.

Signed-off-by Mark Rutland <[email protected]>

Cc Catalin Marinas <[email protected]>

Cc Will Deacon <[email protected]>

---
 arch/arm64/include/asm/hw_breakpoint.h | 13 +++++++------
 arch/arm64/include/asm/mmu_context.h   | 27 ++++++++++-----------------
 arch/arm64/include/asm/pgtable-hwdef.h |  1 +
 arch/arm64/include/asm/sysreg.h        | 20 ++++++++++----------
 arch/arm64/include/asm/thread_info.h   |  3 +++
 arch/arm64/kernel/cacheinfo.c          |  8 +++-----
 arch/arm64/kernel/debug-monitors.c     |  8 +++-----
 arch/arm64/kernel/process.c            | 14 ++++++--------
 arch/arm64/kernel/sys_compat.c         |  2 +-
 9 files changed, 44 insertions(+), 52 deletions(-)

arch/arm64/include/asm/hw_breakpoint.h +7 -6

index 115ea2a..783ba7d 100644
@@ -18,6 +18,7 @@
 
 #include <asm/cputype.h>
 #include <asm/cpufeature.h>
+#include <asm/sysreg.h>
 #include <asm/virt.h>
 
 #ifdef __KERNEL__

@@ -98,18 +99,18 @@ static inline void decode_ctrl_reg(u32 reg,
 #define AARCH64_DBG_REG_WCR	(AARCH64_DBG_REG_WVR + ARM_MAX_WRP)
 
 /* Debug register names. */
-#define AARCH64_DBG_REG_NAME_BVR	"bvr"
-#define AARCH64_DBG_REG_NAME_BCR	"bcr"
-#define AARCH64_DBG_REG_NAME_WVR	"wvr"
-#define AARCH64_DBG_REG_NAME_WCR	"wcr"
+#define AARCH64_DBG_REG_NAME_BVR	bvr
+#define AARCH64_DBG_REG_NAME_BCR	bcr
+#define AARCH64_DBG_REG_NAME_WVR	wvr
+#define AARCH64_DBG_REG_NAME_WCR	wcr
 
 /* Accessor macros for the debug registers. */
 #define AARCH64_DBG_READ(N, REG, VAL) do {\
-	asm volatile("mrs %0, dbg" REG #N "_el1" : "=r" (VAL));\
+	VAL = read_sysreg(dbg##REG##N##_el1);\
 } while (0)
 
 #define AARCH64_DBG_WRITE(N, REG, VAL) do {\
-	asm volatile("msr dbg" REG #N "_el1, %0" :: "r" (VAL));\
+	write_sysreg(VAL, dbg##REG##N##_el1);\
 } while (0)
 
 struct task_struct;

arch/arm64/include/asm/mmu_context.h +10 ~~-17~~

index b1892a0..e5c24b4 100644
@@ -27,16 +27,14 @@
 #include <asm-generic/mm_hooks.h>
 #include <asm/cputype.h>
 #include <asm/pgtable.h>
+#include <asm/sysreg.h>
 #include <asm/tlbflush.h>
 
 #ifdef CONFIG_PID_IN_CONTEXTIDR
 static inline void contextidr_thread_switch(struct task_struct *next)
 {
-	asm(
-	"	msr	contextidr_el1, %0\n"
-	"	isb"
-	:
-	: "r" (task_pid_nr(next)));
+	write_sysreg(task_pid_nr(next), contextidr_el1);
+	isb();
 }
 #else
 static inline void contextidr_thread_switch(struct task_struct *next)

@@ -51,11 +49,8 @@ static inline void cpu_set_reserved_ttbr0(void)
 {
 	unsigned long ttbr = virt_to_phys(empty_zero_page);
 
-	asm(
-	"	msr	ttbr0_el1, %0			// set TTBR0\n"
-	"	isb"
-	:
-	: "r" (ttbr));
+	write_sysreg(ttbr, ttbr0_el1);
+	isb();
 }
 
 /*

@@ -81,13 +76,11 @@ static inline void __cpu_set_tcr_t0sz(unsigned long t0sz)
 	if (!__cpu_uses_extended_idmap())
 		return;
 
-	asm volatile (
-	"	mrs	%0, tcr_el1	;"
-	"	bfi	%0, %1, %2, %3	;"
-	"	msr	tcr_el1, %0	;"
-	"	isb"
-	: "=&r" (tcr)
-	: "r"(t0sz), "I"(TCR_T0SZ_OFFSET), "I"(TCR_TxSZ_WIDTH));
+	tcr = read_sysreg(tcr_el1);
+	tcr &= ~TCR_T0SZ_MASK;
+	tcr |= t0sz << TCR_T0SZ_OFFSET;
+	write_sysreg(tcr, tcr_el1);
+	isb();
 }
 
 #define cpu_set_default_tcr_t0sz()	__cpu_set_tcr_t0sz(TCR_T0SZ(VA_BITS))

arch/arm64/include/asm/pgtable-hwdef.h +1 -0

index c3ae239..eb0c2bd 100644
@@ -208,6 +208,7 @@
 #define TCR_T1SZ(x)		((UL(64) - (x)) << TCR_T1SZ_OFFSET)
 #define TCR_TxSZ(x)		(TCR_T0SZ(x) | TCR_T1SZ(x))
 #define TCR_TxSZ_WIDTH		6
+#define TCR_T0SZ_MASK		(((UL(1) << TCR_TxSZ_WIDTH) - 1) << TCR_T0SZ_OFFSET)
 
 #define TCR_IRGN0_SHIFT		8
 #define TCR_IRGN0_MASK		(UL(3) << TCR_IRGN0_SHIFT)

arch/arm64/include/asm/sysreg.h +10 ~~-10~~

index 39fed2e..e91aef2b 100644
@@ -253,16 +253,6 @@ asm(
 "	.endm\n"
 );
 
-static inline void config_sctlr_el1(u32 clear, u32 set)
-{
-	u32 val;
-
-	asm volatile("mrs %0, sctlr_el1" : "=r" (val));
-	val &= ~clear;
-	val |= set;
-	asm volatile("msr sctlr_el1, %0" : : "r" (val));
-}
-
 /*
  * Unlike read_cpuid, calls to read_sysreg are never expected to be
  * optimized away or replaced with synthetic values.

@@ -283,6 +273,16 @@ static inline void config_sctlr_el1(u32 clear, u32 set)
 		     : : "rZ" (__val));				\
 } while (0)
 
+static inline void config_sctlr_el1(u32 clear, u32 set)
+{
+	u32 val;
+
+	val = read_sysreg(sctlr_el1);
+	val &= ~clear;
+	val |= set;
+	write_sysreg(val, sctlr_el1);
+}
+
 #endif
 
 #endif	/* __ASM_SYSREG_H */

arch/arm64/include/asm/thread_info.h +3 -0

index abd64bd..e9ea5a6 100644
@@ -75,6 +75,9 @@ static inline struct thread_info *current_thread_info(void) __attribute_const__;
 
 /*
  * struct thread_info can be accessed directly via sp_el0.
+ *
+ * We don't use read_sysreg() as we want the compiler to cache the value where
+ * possible.
  */
 static inline struct thread_info *current_thread_info(void)
 {

arch/arm64/kernel/cacheinfo.c +3 -5

index b8629d5..9617301 100644
@@ -39,7 +39,7 @@ static inline enum cache_type get_cache_type(int level)
 
 	if (level > MAX_CACHE_LEVEL)
 		return CACHE_TYPE_NOCACHE;
-	asm volatile ("mrs     %x0, clidr_el1" : "=r" (clidr));
+	clidr = read_sysreg(clidr_el1);
 	return CLIDR_CTYPE(clidr, level);
 }

@@ -55,11 +55,9 @@ u64 __attribute_const__ cache_get_ccsidr(u64 csselr)
 
 	WARN_ON(preemptible());
 
-	/* Put value into CSSELR */
-	asm volatile("msr csselr_el1, %x0" : : "r" (csselr));
+	write_sysreg(csselr, csselr_el1);
 	isb();
-	/* Read result out of CCSIDR */
-	asm volatile("mrs %x0, ccsidr_el1" : "=r" (ccsidr));
+	ccsidr = read_sysreg(ccsidr_el1);
 
 	return ccsidr;
 }

arch/arm64/kernel/debug-monitors.c +3 -5

index 91fff48..c2cd1c3 100644
@@ -46,16 +46,14 @@ static void mdscr_write(u32 mdscr)
 {
 	unsigned long flags;
 	local_dbg_save(flags);
-	asm volatile("msr mdscr_el1, %0" :: "r" (mdscr));
+	write_sysreg(mdscr, mdscr_el1);
 	local_dbg_restore(flags);
 }
 NOKPROBE_SYMBOL(mdscr_write);
 
 static u32 mdscr_read(void)
 {
-	u32 mdscr;
-	asm volatile("mrs %0, mdscr_el1" : "=r" (mdscr));
-	return mdscr;
+	return read_sysreg(mdscr_el1);
 }
 NOKPROBE_SYMBOL(mdscr_read);

@@ -134,7 +132,7 @@ NOKPROBE_SYMBOL(disable_debug_monitors);
  */
 static void clear_os_lock(void *unused)
 {
-	asm volatile("msr oslar_el1, %0" : : "r" (0));
+	write_sysreg(0, oslar_el1);
 }
 
 static int os_lock_notify(struct notifier_block *self,

arch/arm64/kernel/process.c +6 -8

index 6cd2612..a4f5f76 100644
@@ -202,7 +202,7 @@ void show_regs(struct pt_regs * regs)
 
 static void tls_thread_flush(void)
 {
-	asm ("msr tpidr_el0, xzr");
+	write_sysreg(0, tpidr_el0);
 
 	if (is_compat_task()) {
 		current->thread.tp_value = 0;

@@ -213,7 +213,7 @@ static void tls_thread_flush(void)
 		 * with a stale shadow state during context switch.
 		 */
 		barrier();
-		asm ("msr tpidrro_el0, xzr");
+		write_sysreg(0, tpidrro_el0);
 	}
 }

@@ -253,7 +253,7 @@ int copy_thread(unsigned long clone_flags, unsigned long stack_start,
 		 * Read the current TLS pointer from tpidr_el0 as it may be
 		 * out-of-sync with the saved value.
 		 */
-		asm("mrs %0, tpidr_el0" : "=r" (*task_user_tls(p)));
+		*task_user_tls(p) = read_sysreg(tpidr_el0);
 
 		if (stack_start) {
 			if (is_compat_thread(task_thread_info(p)))

@@ -289,17 +289,15 @@ static void tls_thread_switch(struct task_struct *next)
 {
 	unsigned long tpidr, tpidrro;
 
-	asm("mrs %0, tpidr_el0" : "=r" (tpidr));
+	tpidr = read_sysreg(tpidr_el0);
 	*task_user_tls(current) = tpidr;
 
 	tpidr = *task_user_tls(next);
 	tpidrro = is_compat_thread(task_thread_info(next)) ?
 		  next->thread.tp_value : 0;
 
-	asm(
-	"	msr	tpidr_el0, %0\n"
-	"	msr	tpidrro_el0, %1"
-	: : "r" (tpidr), "r" (tpidrro));
+	write_sysreg(tpidr, tpidr_el0);
+	write_sysreg(tpidrro, tpidrro_el0);
 }
 
 /* Restore the UAO state depending on next's addr_limit */

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

index 28c511b..abaf582 100644
@@ -94,7 +94,7 @@ long compat_arm_syscall(struct pt_regs *regs)
 		 * See comment in tls_thread_flush.
 		 */
 		barrier();
-		asm ("msr tpidrro_el0, %0" : : "r" (regs->regs[0]));
+		write_sysreg(regs->regs[0], tpidrro_el0);
 		return 0;
 
 	default:

Signature · 1 line

1.9.1

Mark Rutland

author maintainer

2016-09-08 12:55 UTC #8 lore raw

PATCH 6/6 arm64: simplify contextidr_thread_switch

In reply to Mark Rutland

When CONFIG_PID_IN_CONTEXTIDR is not selected, we use an empty stub definition of contextidr_thread_switch(). As everything we rely upon exists regardless of CONFIG_PID_IN_CONTEXTIDR, we don't strictly require an empty stub.

By using IS_ENABLED() rather than ifdeffery, we avoid duplication, and get compiler coverage on all the code even when CONFIG_PID_IN_CONTEXTIDR is not selected and the code is optimised away.

Signed-off-by Mark Rutland <[email protected]>

Cc Catalin Marinas <[email protected]>

Cc Will Deacon <[email protected]>

---
 arch/arm64/include/asm/mmu_context.h | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

arch/arm64/include/asm/mmu_context.h +3 -6

index e5c24b4..a501853 100644
@@ -30,17 +30,14 @@
 #include <asm/sysreg.h>
 #include <asm/tlbflush.h>
 
-#ifdef CONFIG_PID_IN_CONTEXTIDR
 static inline void contextidr_thread_switch(struct task_struct *next)
 {
+	if (!IS_ENABLED(CONFIG_PID_IN_CONTEXTIDR))
+		return;
+
 	write_sysreg(task_pid_nr(next), contextidr_el1);
 	isb();
 }
-#else
-static inline void contextidr_thread_switch(struct task_struct *next)
-{
-}
-#endif
 
 /*
  * Set TTBR0 to empty_zero_page. No translations will be possible via TTBR0.

Signature · 1 line

1.9.1