Add generic ARM ISA condition code check

arch/arm/include/asm/opcodes.h new file +4 -0

new file mode 100644
index 0000000..aea97bf
@@ -0,0 +1,20 @@
+/*
+ *  arch/arm/include/asm/opcodes.h
+ *
+ * This program is free software; you can redistribute it and/or modify

arch/arm/kernel/opcodes.c new file +45 -0

new file mode 100644
index 0000000..ad966fe
@@ -0,0 +1,69 @@
+/*
+ *  linux/arch/arm/kernel/opcodes.c
+ *
+ *  A32 condition code lookup feature moved from nwfpe/fpopcode.c
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/module.h>
+#include <asm/opcodes.h>
+
+#define ARM_OPCODE_CONDITION_UNCOND 0xf
+
+/*
+ * condition code lookup table
+ * index into the table is test code: EQ, NE, ... LT, GT, AL, NV
+ *
+ * bit position in short is condition code: NZCV
+ */
+static const unsigned short cc_map[16] = {
+	0xF0F0,			/* EQ == Z set            */
+	0x0F0F,			/* NE                     */
+	0xCCCC,			/* CS == C set            */
+	0x3333,			/* CC                     */
+	0xFF00,			/* MI == N set            */
+	0x00FF,			/* PL                     */
+	0xAAAA,			/* VS == V set            */
+	0x5555,			/* VC                     */
+	0x0C0C,			/* HI == C set && Z clear */
+	0xF3F3,			/* LS == C clear || Z set */
+	0xAA55,			/* GE == (N==V)           */
+	0x55AA,			/* LT == (N!=V)           */
+	0x0A05,			/* GT == (!Z && (N==V))   */
+	0xF5FA,			/* LE == (Z || (N!=V))    */
+	0xFFFF,			/* AL always              */
+	0			/* NV                     */
+};
+
+/*
+ * Returns:
+ * ARM_OPCODE_CONDTEST_FAIL   - if condition fails
+ * ARM_OPCODE_CONDTEST_PASS   - if condition passes (including AL)
+ * ARM_OPCODE_CONDTEST_UNCOND - if NV condition, or separate unconditional

Hi Leif,

This looks largely there, but I have some small comments inline.

arch/arm/include/asm/opcodes.h line 14

Quoted 3 lines · Leif Lindholm

arch/arm/include/asm/opcodes.h new file +3 -0

+#ifndef __ASSEMBLY__
+extern unsigned int arm_check_condition(unsigned int opcode, unsigned int psr);
+#endif

You could use u32 for the parameters to reinforce the point that they're words.

Quoted 1 line

+unsigned int arm_check_condition(unsigned int opcode, unsigned int psr)

I think you want asmlinkage for this function.

Quoted 8 lines

arch/arm/kernel/opcodes.c new file +8 -0

+{
+    unsigned int cc_bits  = opcode>>  28;
+    unsigned int psr_cond = psr>>  28;
+
+    if (cc_bits != ARM_OPCODE_CONDITION_UNCOND) {
+            if ((cc_map[cc_bits]>>  (psr_cond))&  1)
+                    return ARM_OPCODE_CONDTEST_PASS;
+            else

arch/arm/kernel/opcodes.c line 68

Quoted 5 lines

arch/arm/kernel/opcodes.c new file +5 -0

+                    return ARM_OPCODE_CONDTEST_FAIL;
+    } else {
+            return ARM_OPCODE_CONDTEST_UNCOND;
+    }
+}

Maybe have a single return here returning a ret value which you assign in the conditional blocks.

Quoted 1 line

+EXPORT_SYMBOL(arm_check_condition);

EXPORT_SYMBOL_GPL?

arch/arm/kernel/swp_emulate.c +2 -0

index 5f452f8..6a5210e 100644
@@ -25,6 +25,7 @@
 #include <linux/syscalls.h>
 #include <linux/perf_event.h>
 
+#include <asm/opcodes.h>
 #include <asm/traps.h>
 #include <asm/uaccess.h>
 

@@ -185,6 +186,17 @@ static int swp_handler(struct pt_regs *regs, unsigned int instr)
 
 	perf_sw_event(PERF_COUNT_SW_EMULATION_FAULTS, 1, regs, regs->ARM_pc);
 
+	res = arm_check_condition(instr, regs->ARM_cpsr);

arch/arm/kernel/kprobes-test.c +2 -0

index e17cdd6..f81c2b4 100644
@@ -202,6 +202,8 @@
 #include <linux/slab.h>
 #include <linux/kprobes.h>
 
+#include <asm/opcodes.h>
+

{

arch/arm/kernel/kprobes-test.c +0 -17

-	unsigned long temp;
-
-	switch (cc) {
-	case 0x0: /* eq */
-		return cpsr & PSR_Z_BIT;
-
-	case 0x1: /* ne */
-		return (~cpsr) & PSR_Z_BIT;
-
-	case 0x2: /* cs */
-		return cpsr & PSR_C_BIT;
-
-	case 0x3: /* cc */
-		return (~cpsr) & PSR_C_BIT;
-
-	case 0x4: /* mi */
-		return cpsr & PSR_N_BIT;

arch/arm/kernel/kprobes-test.c static int test_instance; · line 1053

Quoted 2 lines · Leif Lindholm

arch/arm/kernel/kprobes-test.c +1 -1

-static unsigned long test_check_cc(int cc, unsigned long cpsr)
+inline unsigned long test_check_cc(int cc, unsigned long cpsr)

Not sure why you make this change, surely you can just leave it as static?

Maybe it's best just to change all of the callers to call arm_check_condition directly, like you have done below for the ARM case. For the Thumb cases will it work if you make sure that you put the condition code in the top bits?

Quoted 5 lines · Will Deacon

arch/arm/kernel/kprobes-test.c static int test_instance; · line 1053

Quoted 2 lines · Leif Lindholm

arch/arm/kernel/kprobes-test.c +1 -1

-static unsigned long test_check_cc(int cc, unsigned long cpsr)
+inline unsigned long test_check_cc(int cc, unsigned long cpsr)

Not sure why you make this change, surely you can just leave it as static?

As the function is now only a wrapper on arm_check_condition (with an inline shift), it made more sense to me. This was also suggested by Tixy, and he's since ACKed this patch.

Quoted 4 lines

Maybe it's best just to change all of the callers to call arm_check_condition directly, like you have done below for the ARM case. For the Thumb cases will it work if you make sure that you put the condition code in the top bits?

Yes, that is functionally equivalent, and what I did in the RFC version, but it ended up looking messy at the calling point.

Any sane compiler should generate pretty identical code for these two alternative solutions (unless it neglected to auto-inline the static variant).

Hmm, I still don't see why you should change the linkage. Make it static inline if you really want the inline, but that seems weird outside of a header file stub.

Quoted 7 lines

Quoted 4 lines

Maybe it's best just to change all of the callers to call arm_check_condition directly, like you have done below for the ARM case. For the Thumb cases will it work if you make sure that you put the condition code in the top bits?

Yes, that is functionally equivalent, and what I did in the RFC version, but it ended up looking messy at the calling point.

Ok, then could you route the ARM variant through the wrapper too?

Quoted 7 lines · Leif Lindholm

Quoted 5 lines · Will Deacon

arch/arm/kernel/kprobes-test.c static int test_instance; · line 1053

Quoted 2 lines · Leif Lindholm

arch/arm/kernel/kprobes-test.c +1 -1

-static unsigned long test_check_cc(int cc, unsigned long cpsr)
+inline unsigned long test_check_cc(int cc, unsigned long cpsr)

Not sure why you make this change, surely you can just leave it as static?

Quoted 3 lines

As the function is now only a wrapper on arm_check_condition (with an inline shift), it made more sense to me. This was also suggested by Tixy, and he's since ACKed this patch.

Hmm, I still don't see why you should change the linkage. Make it static inline if you really want the inline, but that seems weird outside of a header file stub.

Quoted 7 lines

Quoted 4 lines

Maybe it's best just to change all of the callers to call arm_check_condition directly, like you have done below for the ARM case. For the Thumb cases will it work if you make sure that you put the condition code in the top bits?

Yes, that is functionally equivalent, and what I did in the RFC version, but it ended up looking messy at the calling point.

Ok, then could you route the ARM variant through the wrapper too?

Quoted 3 lines

Any sane compiler should generate pretty identical code for these two alternative solutions (unless it neglected to auto-inline the static variant).

The problem is when somebody else decides to call test_check_cc from another compilation unit, rather than go through arm_check_condition.

Quoted 4 lines

Quoted 2 lines

Yes, that is functionally equivalent, and what I did in the RFC version, but it ended up looking messy at the calling point.

Ok, then could you route the ARM variant through the wrapper too?

The function Leif added is for checking the condition code in an ARM instruction, so it doesn't need a wrapper when used for this.

The other locations in the kprobes tests get the condition nibble from the ITSATE or from thumb conditional branch instructions. In these cases the code looks cleaner if the condition is passed as a value between 0x0 and 0xf, rather than being shift up to bit position 28 where the conditional ARM instructions have it encoded.

Quoted 4 lines

Quoted 2 lines

Yes, that is functionally equivalent, and what I did in the RFC version, but it ended up looking messy at the calling point.

Ok, then could you route the ARM variant through the wrapper too?

The function Leif added is for checking the condition code in an ARM instruction, so it doesn't need a wrapper when used for this.

The other locations in the kprobes tests get the condition nibble from the ITSATE or from thumb conditional branch instructions. In these cases the code looks cleaner if the condition is passed as a value between 0x0 and 0xf, rather than being shift up to bit position 28 where the conditional ARM instructions have it encoded.

There are several locations in the kernel where software needs to inspect the condition codes of trapped instructions. The original bitmask implementation in the nwfpe code does this in an efficient manner. This series breaks this code out of nwfpe/fpopcode.{ch} into a standalone file for opcode operations, and contains additional patches to kprobes and SWP eumlation to use this interface.

Hi Leif,

Quoted 6 lines · Leif Lindholm

There are several locations in the kernel where software needs to inspect the condition codes of trapped instructions. The original bitmask implementation in the nwfpe code does this in an efficient manner. This series breaks this code out of nwfpe/fpopcode.{ch} into a standalone file for opcode operations, and contains additional patches to kprobes and SWP eumlation to use this interface.

Dave's been posting an opcodes.h which seems to fit in with some of this, so it's definitely worth you guys syncing up if you haven't already.

Dave - any comments?