arm64: sys_reg() : Fix encoding of system registers

Commit Message

Suzuki K Poulose July 22, 2015, 9:54 a.m. UTC

From: "Suzuki K. Poulose" <[e-mail]>

sys_reg() macro encodes Op0 as (Op0 - 2) and pushes it to Bit 19,
leaving Bit 20 uninitialised. As per ARMv8 ARM, (Ref: ARMv8 ARM,
Section: "System instruction class encoding overview", C5.2,
version:ARM DDI 0487A.f), the instruction encoding reserves
bits [20-19] for Op0. Especially, the sys_reg could give wrong
values for using with MSR(Immediate) form, which is only supported
with Op0 == 0 (e.g, PSTATE.x). Fix this to avoid users getting the
wrong encoding.

Cc: James Morse <[e-mail]>
Cc: Catalin Marinas <[e-mail]>
Signed-off-by: Suzuki K. Poulose <[e-mail]>
---
 arch/arm64/include/asm/sysreg.h |   20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

Comments

Catalin Marinas July 22, 2015, 10:07 a.m. UTC | #1

On Wed, Jul 22, 2015 at 10:54:17AM +0100, Suzuki K. Poulose wrote:
> From: "Suzuki K. Poulose" <[e-mail]>
> 
> sys_reg() macro encodes Op0 as (Op0 - 2) and pushes it to Bit 19,
> leaving Bit 20 uninitialised.

A value of 0 doesn't mean uninitialised and as you noticed, it's set by
the value or'ed onto the sys_reg() returned value in the mrs_s etc.
macros.

So I would rather change the subject to "Generalise encoding of system
registers" and drop the uninitialised paragraph above, maybe replace it
with a statement that current sys_reg() was only meant for MSR/MRS
(register) encodings.

Otherwise, the patch itself is fine.

Reviewed-by: Catalin Marinas <[e-mail]>

Suzuki K Poulose July 22, 2015, 10:20 a.m. UTC | #2

On 22/07/15 11:07, Catalin Marinas wrote:
> On Wed, Jul 22, 2015 at 10:54:17AM +0100, Suzuki K. Poulose wrote:
>> From: "Suzuki K. Poulose" <[e-mail]>
>>
>> sys_reg() macro encodes Op0 as (Op0 - 2) and pushes it to Bit 19,
>> leaving Bit 20 uninitialised.
>
> A value of 0 doesn't mean uninitialised and as you noticed, it's set by
> the value or'ed onto the sys_reg() returned value in the mrs_s etc.
> macros.
>
> So I would rather change the subject to "Generalise encoding of system
> registers" and drop the uninitialised paragraph above, maybe replace it
> with a statement that current sys_reg() was only meant for MSR/MRS
> (register) encodings.

OK. Will send the updated versoin.

>
> Otherwise, the patch itself is fine.
>
> Reviewed-by: Catalin Marinas <[e-mail]>

Thanks
Suzuki

Patch

diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
index 0a579b2..9c5f0f1 100644
--- a/arch/arm64/include/asm/sysreg.h
+++ b/arch/arm64/include/asm/sysreg.h
@@ -20,8 +20,18 @@ 
 #ifndef __ASM_SYSREG_H
 #define __ASM_SYSREG_H
 
+/*
+ * ARMv8 ARM reserves the following encoding for system registers:
+ * (Ref: ARMv8 ARM, Section: "System instruction class encoding overview",
+ *  C5.2, version:ARM DDI 0487A.f)
+ *	[20-19] : Op0
+ *	[18-16] : Op1
+ *	[15-12] : CRn
+ *	[11-8]  : CRm
+ *	[7-5]   : Op2
+ */
 #define sys_reg(op0, op1, crn, crm, op2) \
-	((((op0)-2)<<19)|((op1)<<16)|((crn)<<12)|((crm)<<8)|((op2)<<5))
+	((((op0)&3)<<19)|((op1)<<16)|((crn)<<12)|((crm)<<8)|((op2)<<5))
 
 #ifdef __ASSEMBLY__
 
@@ -31,11 +41,11 @@ 
 	.equ	__reg_num_xzr, 31
 
 	.macro	mrs_s, rt, sreg
-	.inst	0xd5300000|(\sreg)|(__reg_num_\rt)
+	.inst	0xd5200000|(\sreg)|(__reg_num_\rt)
 	.endm
 
 	.macro	msr_s, sreg, rt
-	.inst	0xd5100000|(\sreg)|(__reg_num_\rt)
+	.inst	0xd5000000|(\sreg)|(__reg_num_\rt)
 	.endm
 
 #else
@@ -47,11 +57,11 @@  asm(
 "	.equ	__reg_num_xzr, 31\n"
 "\n"
 "	.macro	mrs_s, rt, sreg\n"
-"	.inst	0xd5300000|(\\sreg)|(__reg_num_\\rt)\n"
+"	.inst	0xd5200000|(\\sreg)|(__reg_num_\\rt)\n"
 "	.endm\n"
 "\n"
 "	.macro	msr_s, sreg, rt\n"
-"	.inst	0xd5100000|(\\sreg)|(__reg_num_\\rt)\n"
+"	.inst	0xd5000000|(\\sreg)|(__reg_num_\\rt)\n"
 "	.endm\n"
 );