[PATCH 0/4] Add generic ARM ISA condition code checks

[PATCH 0/4] Add generic ARM ISA condition code checks

[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.

---

Reworked based on feedback on RFC set submitted on 2011-11-21.

Leif Lindholm (4):
      Add generic ARM instruction set condition code checks.
      Use generic ARM instruction set condition code checks for nwfpe.
      Add condition code checking to SWP emulation handler.
      Use generic ARM instruction set condition code checks for kprobes.


 arch/arm/include/asm/opcodes.h |   20 ++++++++++++
 arch/arm/kernel/Makefile       |    2 +
 arch/arm/kernel/kprobes-test.c |   66 ++++-----------------------------------
 arch/arm/kernel/opcodes.c      |   68 ++++++++++++++++++++++++++++++++++++++++
 arch/arm/kernel/swp_emulate.c  |   14 ++++++++
 arch/arm/nwfpe/entry.S         |    8 +++--
 arch/arm/nwfpe/fpopcode.c      |   26 ---------------
 arch/arm/nwfpe/fpopcode.h      |    3 --
 8 files changed, 114 insertions(+), 93 deletions(-)
 create mode 100644 arch/arm/include/asm/opcodes.h
 create mode 100644 arch/arm/kernel/opcodes.c

-- 
Signature

[PATCH 1/4] Add generic ARM instruction set condition code checks.

[Leif Lindholm]

This patch breaks the ARM condition checking code out of nwfpe/fpopcode.{ch}
into a standalone file for opcode operations. It also modifies the code
somewhat for coding style adherence, and adds some temporary variables for
increased readability.

Signed-off-by: Leif Lindholm <leif.lindholm@arm.com>
---
 arch/arm/include/asm/opcodes.h |   20 ++++++++++++
 arch/arm/kernel/Makefile       |    2 +
 arch/arm/kernel/opcodes.c      |   68 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 89 insertions(+), 1 deletions(-)
 create mode 100644 arch/arm/include/asm/opcodes.h
 create mode 100644 arch/arm/kernel/opcodes.c

diff --git a/arch/arm/include/asm/opcodes.h b/arch/arm/include/asm/opcodes.h
new file mode 100644
index 0000000..aea97bf
--- /dev/null
+++ b/arch/arm/include/asm/opcodes.h
@@ -0,0 +1,20 @@
+/*
+ *  arch/arm/include/asm/opcodes.h
+ *
+ * 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.
+ */
+
+#ifndef __ASM_ARM_OPCODES_H
+#define __ASM_ARM_OPCODES_H
+
+#ifndef __ASSEMBLY__
+extern unsigned int arm_check_condition(unsigned int opcode, unsigned int psr);
+#endif
+
+#define ARM_OPCODE_CONDTEST_FAIL   0
+#define ARM_OPCODE_CONDTEST_PASS   1
+#define ARM_OPCODE_CONDTEST_UNCOND 2
+
+#endif /* __ASM_ARM_OPCODES_H */
diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile
index 16eed6a..43b740d 100644
--- a/arch/arm/kernel/Makefile
+++ b/arch/arm/kernel/Makefile
@@ -13,7 +13,7 @@ CFLAGS_REMOVE_return_address.o = -pg
 
 # Object file lists.
 
-obj-y		:= elf.o entry-armv.o entry-common.o irq.o \
+obj-y		:= elf.o entry-armv.o entry-common.o irq.o opcodes.o \
 		   process.o ptrace.o return_address.o setup.o signal.o \
 		   sys_arm.o stacktrace.o time.o traps.o
 
diff --git a/arch/arm/kernel/opcodes.c b/arch/arm/kernel/opcodes.c
new file mode 100644
index 0000000..c3171cc
--- /dev/null
+++ b/arch/arm/kernel/opcodes.c
@@ -0,0 +1,68 @@
+/*
+ *  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
+ *                              opcode space from v5 onwards
+ *
+ * Code that needs to check whether a condition has explicitly passed,
+ * should compare the return value to 1.
+ * Code that wants to check if a condition means that the instruction
+ * should be executed, should compare the return value to !0.
+ */
+unsigned int arm_check_condition(unsigned int opcode, unsigned int psr)
+{
+	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
+			return ARM_OPCODE_CONDTEST_FAIL;
+	 } else {
+		return ARM_OPCODE_CONDTEST_UNCOND;
+	}
+}
+
+EXPORT_SYMBOL(arm_check_condition);

[PATCH 2/4] Use generic ARM instruction set condition code checks for nwfpe.

[Leif Lindholm]

This patch changes the nwfpe implementation to use the new generic
ARM instruction set condition code checks, rather than a local
implementation. It also removes the existing condition code checking,
which has been used for the generic support (in kernel/opcodes.{ch}).

Signed-off-by: Leif Lindholm <leif.lindholm@arm.com>
---
This code has not been tested beyond building, linking and booting.

 arch/arm/nwfpe/entry.S    |    8 +++++---
 arch/arm/nwfpe/fpopcode.c |   26 --------------------------
 arch/arm/nwfpe/fpopcode.h |    3 ---
 3 files changed, 5 insertions(+), 32 deletions(-)

diff --git a/arch/arm/nwfpe/entry.S b/arch/arm/nwfpe/entry.S
index cafa183..d18dde9 100644
--- a/arch/arm/nwfpe/entry.S
+++ b/arch/arm/nwfpe/entry.S
@@ -20,6 +20,8 @@
     Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
 */
 
+#include <asm/opcodes.h>
+
 /* This is the kernel's entry point into the floating point emulator.
 It is called from the kernel with code similar to this:
 
@@ -81,11 +83,11 @@ nwfpe_enter:
 	mov	r6, r0			@ save the opcode
 emulate:
 	ldr	r1, [sp, #S_PSR]	@ fetch the PSR
-	bl	checkCondition		@ check the condition
-	cmp	r0, #0			@ r0 = 0 ==> condition failed
+	bl	arm_check_condition	@ check the condition
+	cmp	r0, #ARM_OPCODE_CONDTEST_PASS	@ condition passed?
 
 	@ if condition code failed to match, next insn
-	beq	next			@ get the next instruction;
+	bne	next			@ get the next instruction;
 
 	mov	r0, r6			@ prepare for EmulateAll()
 	bl	EmulateAll		@ emulate the instruction
diff --git a/arch/arm/nwfpe/fpopcode.c b/arch/arm/nwfpe/fpopcode.c
index 922b811..ff98346 100644
--- a/arch/arm/nwfpe/fpopcode.c
+++ b/arch/arm/nwfpe/fpopcode.c
@@ -61,29 +61,3 @@ const float32 float32Constant[] = {
 	0x41200000		/* single 10.0 */
 };
 
-/* 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 aCC[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
-};
-
-unsigned int checkCondition(const unsigned int opcode, const unsigned int ccodes)
-{
-	return (aCC[opcode >> 28] >> (ccodes >> 28)) & 1;
-}
diff --git a/arch/arm/nwfpe/fpopcode.h b/arch/arm/nwfpe/fpopcode.h
index 786e4c9..78f02db 100644
--- a/arch/arm/nwfpe/fpopcode.h
+++ b/arch/arm/nwfpe/fpopcode.h
@@ -475,9 +475,6 @@ static inline unsigned int getDestinationSize(const unsigned int opcode)
 	return (nRc);
 }
 
-extern unsigned int checkCondition(const unsigned int opcode,
-				   const unsigned int ccodes);
-
 extern const float64 float64Constant[];
 extern const float32 float32Constant[];
 

[PATCH 3/4] Add condition code checking to SWP emulation handler.

[Leif Lindholm]

This patch fixes two separate issues with the SWP emulation handler:
1: Certain processors implementing ARMv7-A can (legally) take an
   undef exception even when the condition code would have meant that
   the instruction should not have been executed.
2: Opcodes with all flags set (condition code = 0xf) have been reused
   in recent, and not-so-recent, versions of the ARM architecture to
   implement unconditional extensions to the instruction set. The
   existing code would still have processed any undefs triggered by
   executing an opcode with such a value.

This patch uses the new generic ARM instruction set condition code
checks to implement proper handling of these situations.

Signed-off-by: Leif Lindholm <leif.lindholm@arm.com>
---
 arch/arm/kernel/swp_emulate.c |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/arch/arm/kernel/swp_emulate.c b/arch/arm/kernel/swp_emulate.c
index 5f452f8..8629bf7 100644
--- a/arch/arm/kernel/swp_emulate.c
+++ b/arch/arm/kernel/swp_emulate.c
@@ -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,19 @@ 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);
+	switch (res) {
+	case ARM_OPCODE_CONDTEST_FAIL: {
+		/* Condition failed - return to next instruction */
+		regs->ARM_pc += 4;
+		return 0;
+	} break;
+	case ARM_OPCODE_CONDTEST_UNCOND: {
+		/* If unconditional encoding - not a SWP, undef */
+		return -EFAULT;
+	} break;
+	}
+
 	if (current->pid != previous_pid) {
 		pr_debug("\"%s\" (%ld) uses deprecated SWP{B} instruction\n",
 			 current->comm, (unsigned long)current->pid);

[PATCH 4/4] Use generic ARM instruction set condition code checks for kprobes.

[Leif Lindholm]

This patch changes the kprobes implementation to use the generic ARM
instruction set condition code checks, rather than a dedicated
implementation.

Cc: Tixy <tixy@yxit.co.uk>
Signed-off-by: Leif Lindholm <leif.lindholm@arm.com>
---
 arch/arm/kernel/kprobes-test.c |   66 ++++------------------------------------
 1 files changed, 6 insertions(+), 60 deletions(-)

diff --git a/arch/arm/kernel/kprobes-test.c b/arch/arm/kernel/kprobes-test.c
index e17cdd6..278c275 100644
--- a/arch/arm/kernel/kprobes-test.c
+++ b/arch/arm/kernel/kprobes-test.c
@@ -202,6 +202,8 @@
 #include <linux/slab.h>
 #include <linux/kprobes.h>
 
+#include <asm/opcodes.h>
+
 #include "kprobes.h"
 #include "kprobes-test.h"
 
@@ -1048,67 +1050,11 @@ static int test_instance;
  */
 #define PSR_IGNORE_BITS (PSR_A_BIT | PSR_F_BIT)
 
-static unsigned long test_check_cc(int cc, unsigned long cpsr)
+inline unsigned long test_check_cc(int cc, unsigned long cpsr)
 {
-	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;
-
-	case 0x5: /* pl */
-		return (~cpsr) & PSR_N_BIT;
-
-	case 0x6: /* vs */
-		return cpsr & PSR_V_BIT;
-
-	case 0x7: /* vc */
-		return (~cpsr) & PSR_V_BIT;
+	int ret = arm_check_condition(cc << 28, cpsr);
 
-	case 0x8: /* hi */
-		cpsr &= ~(cpsr >> 1); /* PSR_C_BIT &= ~PSR_Z_BIT */
-		return cpsr & PSR_C_BIT;
-
-	case 0x9: /* ls */
-		cpsr &= ~(cpsr >> 1); /* PSR_C_BIT &= ~PSR_Z_BIT */
-		return (~cpsr) & PSR_C_BIT;
-
-	case 0xa: /* ge */
-		cpsr ^= (cpsr << 3); /* PSR_N_BIT ^= PSR_V_BIT */
-		return (~cpsr) & PSR_N_BIT;
-
-	case 0xb: /* lt */
-		cpsr ^= (cpsr << 3); /* PSR_N_BIT ^= PSR_V_BIT */
-		return cpsr & PSR_N_BIT;
-
-	case 0xc: /* gt */
-		temp = cpsr ^ (cpsr << 3); /* PSR_N_BIT ^= PSR_V_BIT */
-		temp |= (cpsr << 1);	   /* PSR_N_BIT |= PSR_Z_BIT */
-		return (~temp) & PSR_N_BIT;
-
-	case 0xd: /* le */
-		temp = cpsr ^ (cpsr << 3); /* PSR_N_BIT ^= PSR_V_BIT */
-		temp |= (cpsr << 1);	   /* PSR_N_BIT |= PSR_Z_BIT */
-		return temp & PSR_N_BIT;
-
-	case 0xe: /* al */
-	case 0xf: /* unconditional */
-		return true;
-	}
-	BUG();
-	return false;
+	return (ret != ARM_OPCODE_CONDTEST_FAIL);
 }
 
 static int is_last_scenario;
@@ -1128,7 +1074,7 @@ static unsigned long test_context_cpsr(int scenario)
 
 	if (!test_case_is_thumb) {
 		/* Testing ARM code */
-		probe_should_run = test_check_cc(current_instruction >> 28, cpsr) != 0;
+		probe_should_run = arm_check_condition(current_instruction, cpsr) != 0;
 		if (scenario == 15)
 			is_last_scenario = true;
 

[PATCH 1/4] Add generic ARM instruction set condition code checks.

[Dave Martin]

On Fri, Nov 25, 2011 at 05:19:23PM +0000, Leif Lindholm wrote:
> This patch breaks the ARM condition checking code out of nwfpe/fpopcode.{ch}
> into a standalone file for opcode operations. It also modifies the code
> somewhat for coding style adherence, and adds some temporary variables for
> increased readability.
> 
> Signed-off-by: Leif Lindholm <leif.lindholm@arm.com>

Heh, looks like I also invented the same header file in the meantime

Would you be able to comment on this?

http://comments.gmane.org/gmane.linux.ports.arm.kernel/141568

I think the content is complementary to what you're proposing.

Cheers
---Dave

> ---
>  arch/arm/include/asm/opcodes.h |   20 ++++++++++++
>  arch/arm/kernel/Makefile       |    2 +
>  arch/arm/kernel/opcodes.c      |   68 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 89 insertions(+), 1 deletions(-)
>  create mode 100644 arch/arm/include/asm/opcodes.h
>  create mode 100644 arch/arm/kernel/opcodes.c
> 
> diff --git a/arch/arm/include/asm/opcodes.h b/arch/arm/include/asm/opcodes.h
> new file mode 100644
> index 0000000..aea97bf
> --- /dev/null
> +++ b/arch/arm/include/asm/opcodes.h
> @@ -0,0 +1,20 @@
> +/*
> + *  arch/arm/include/asm/opcodes.h
> + *
> + * 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.
> + */
> +
> +#ifndef __ASM_ARM_OPCODES_H
> +#define __ASM_ARM_OPCODES_H
> +
> +#ifndef __ASSEMBLY__
> +extern unsigned int arm_check_condition(unsigned int opcode, unsigned int psr);
> +#endif
> +
> +#define ARM_OPCODE_CONDTEST_FAIL   0
> +#define ARM_OPCODE_CONDTEST_PASS   1
> +#define ARM_OPCODE_CONDTEST_UNCOND 2
> +
> +#endif /* __ASM_ARM_OPCODES_H */
> diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile
> index 16eed6a..43b740d 100644
> --- a/arch/arm/kernel/Makefile
> +++ b/arch/arm/kernel/Makefile
> @@ -13,7 +13,7 @@ CFLAGS_REMOVE_return_address.o = -pg
>  
>  # Object file lists.
>  
> -obj-y		:= elf.o entry-armv.o entry-common.o irq.o \
> +obj-y		:= elf.o entry-armv.o entry-common.o irq.o opcodes.o \
>  		   process.o ptrace.o return_address.o setup.o signal.o \
>  		   sys_arm.o stacktrace.o time.o traps.o
>  
> diff --git a/arch/arm/kernel/opcodes.c b/arch/arm/kernel/opcodes.c
> new file mode 100644
> index 0000000..c3171cc
> --- /dev/null
> +++ b/arch/arm/kernel/opcodes.c
> @@ -0,0 +1,68 @@
> +/*
> + *  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
> + *                              opcode space from v5 onwards
> + *
> + * Code that needs to check whether a condition has explicitly passed,
> + * should compare the return value to 1.
> + * Code that wants to check if a condition means that the instruction
> + * should be executed, should compare the return value to !0.
> + */
> +unsigned int arm_check_condition(unsigned int opcode, unsigned int psr)
> +{
> +	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
> +			return ARM_OPCODE_CONDTEST_FAIL;
> +	 } else {
> +		return ARM_OPCODE_CONDTEST_UNCOND;
> +	}
> +}
> +
> +EXPORT_SYMBOL(arm_check_condition);
> 
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 4/4] Use generic ARM instruction set condition code checks for kprobes.

[Tixy]

On Fri, 2011-11-25 at 17:20 +0000, Leif Lindholm wrote:
> This patch changes the kprobes implementation to use the generic ARM
> instruction set condition code checks, rather than a dedicated
> implementation.
> 
> Cc: Tixy <tixy@yxit.co.uk>
> Signed-off-by: Leif Lindholm <leif.lindholm@arm.com>

Acked-by: Jon Medhurst <tixy@yxit.co.uk>

> ---
>  arch/arm/kernel/kprobes-test.c |   66 ++++------------------------------------
>  1 files changed, 6 insertions(+), 60 deletions(-)
> 
> diff --git a/arch/arm/kernel/kprobes-test.c b/arch/arm/kernel/kprobes-test.c
> index e17cdd6..278c275 100644
> --- a/arch/arm/kernel/kprobes-test.c
> +++ b/arch/arm/kernel/kprobes-test.c
> @@ -202,6 +202,8 @@
>  #include <linux/slab.h>
>  #include <linux/kprobes.h>
>  
> +#include <asm/opcodes.h>
> +
>  #include "kprobes.h"
>  #include "kprobes-test.h"
>  
> @@ -1048,67 +1050,11 @@ static int test_instance;
>   */
>  #define PSR_IGNORE_BITS (PSR_A_BIT | PSR_F_BIT)
>  
> -static unsigned long test_check_cc(int cc, unsigned long cpsr)
> +inline unsigned long test_check_cc(int cc, unsigned long cpsr)
>  {
> -	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;
> -
> -	case 0x5: /* pl */
> -		return (~cpsr) & PSR_N_BIT;
> -
> -	case 0x6: /* vs */
> -		return cpsr & PSR_V_BIT;
> -
> -	case 0x7: /* vc */
> -		return (~cpsr) & PSR_V_BIT;
> +	int ret = arm_check_condition(cc << 28, cpsr);
>  
> -	case 0x8: /* hi */
> -		cpsr &= ~(cpsr >> 1); /* PSR_C_BIT &= ~PSR_Z_BIT */
> -		return cpsr & PSR_C_BIT;
> -
> -	case 0x9: /* ls */
> -		cpsr &= ~(cpsr >> 1); /* PSR_C_BIT &= ~PSR_Z_BIT */
> -		return (~cpsr) & PSR_C_BIT;
> -
> -	case 0xa: /* ge */
> -		cpsr ^= (cpsr << 3); /* PSR_N_BIT ^= PSR_V_BIT */
> -		return (~cpsr) & PSR_N_BIT;
> -
> -	case 0xb: /* lt */
> -		cpsr ^= (cpsr << 3); /* PSR_N_BIT ^= PSR_V_BIT */
> -		return cpsr & PSR_N_BIT;
> -
> -	case 0xc: /* gt */
> -		temp = cpsr ^ (cpsr << 3); /* PSR_N_BIT ^= PSR_V_BIT */
> -		temp |= (cpsr << 1);	   /* PSR_N_BIT |= PSR_Z_BIT */
> -		return (~temp) & PSR_N_BIT;
> -
> -	case 0xd: /* le */
> -		temp = cpsr ^ (cpsr << 3); /* PSR_N_BIT ^= PSR_V_BIT */
> -		temp |= (cpsr << 1);	   /* PSR_N_BIT |= PSR_Z_BIT */
> -		return temp & PSR_N_BIT;
> -
> -	case 0xe: /* al */
> -	case 0xf: /* unconditional */
> -		return true;
> -	}
> -	BUG();
> -	return false;
> +	return (ret != ARM_OPCODE_CONDTEST_FAIL);
>  }
>  
>  static int is_last_scenario;
> @@ -1128,7 +1074,7 @@ static unsigned long test_context_cpsr(int scenario)
>  
>  	if (!test_case_is_thumb) {
>  		/* Testing ARM code */
> -		probe_should_run = test_check_cc(current_instruction >> 28, cpsr) != 0;
> +		probe_should_run = arm_check_condition(current_instruction, cpsr) != 0;
>  		if (scenario == 15)
>  			is_last_scenario = true;
>  
> 
> 

[PATCH 1/4] Add generic ARM instruction set condition code checks.

[Dave Martin]

On Fri, Nov 25, 2011 at 05:19:23PM +0000, Leif Lindholm wrote:
> This patch breaks the ARM condition checking code out of nwfpe/fpopcode.{ch}
> into a standalone file for opcode operations. It also modifies the code
> somewhat for coding style adherence, and adds some temporary variables for
> increased readability.
> 
> Signed-off-by: Leif Lindholm <leif.lindholm@arm.com>
> ---
>  arch/arm/include/asm/opcodes.h |   20 ++++++++++++
>  arch/arm/kernel/Makefile       |    2 +
>  arch/arm/kernel/opcodes.c      |   68 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 89 insertions(+), 1 deletions(-)
>  create mode 100644 arch/arm/include/asm/opcodes.h
>  create mode 100644 arch/arm/kernel/opcodes.c
> 
> diff --git a/arch/arm/include/asm/opcodes.h b/arch/arm/include/asm/opcodes.h
> new file mode 100644
> index 0000000..aea97bf
> --- /dev/null
> +++ b/arch/arm/include/asm/opcodes.h
> @@ -0,0 +1,20 @@
> +/*
> + *  arch/arm/include/asm/opcodes.h
> + *
> + * 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.
> + */
> +
> +#ifndef __ASM_ARM_OPCODES_H
> +#define __ASM_ARM_OPCODES_H
> +
> +#ifndef __ASSEMBLY__
> +extern unsigned int arm_check_condition(unsigned int opcode, unsigned int psr);
> +#endif
> +
> +#define ARM_OPCODE_CONDTEST_FAIL   0
> +#define ARM_OPCODE_CONDTEST_PASS   1
> +#define ARM_OPCODE_CONDTEST_UNCOND 2
> +
> +#endif /* __ASM_ARM_OPCODES_H */
> diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile
> index 16eed6a..43b740d 100644
> --- a/arch/arm/kernel/Makefile
> +++ b/arch/arm/kernel/Makefile
> @@ -13,7 +13,7 @@ CFLAGS_REMOVE_return_address.o = -pg
>  
>  # Object file lists.
>  
> -obj-y		:= elf.o entry-armv.o entry-common.o irq.o \
> +obj-y		:= elf.o entry-armv.o entry-common.o irq.o opcodes.o \
>  		   process.o ptrace.o return_address.o setup.o signal.o \
>  		   sys_arm.o stacktrace.o time.o traps.o
>  
> diff --git a/arch/arm/kernel/opcodes.c b/arch/arm/kernel/opcodes.c
> new file mode 100644
> index 0000000..c3171cc
> --- /dev/null
> +++ b/arch/arm/kernel/opcodes.c
> @@ -0,0 +1,68 @@
> +/*
> + *  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
> + *                              opcode space from v5 onwards
> + *
> + * Code that needs to check whether a condition has explicitly passed,
> + * should compare the return value to 1.

This paragraph should refer to the symbolic constants -- there's not so
much point having them if people hard-code integer comparisons into all
the calling code.

The wording feels a bit confusing too.  Maybe something like:

"Code that needs to check when an instruction is conditional and would
pass its condition check should check whether the return value ==
ARM_OPCODE_CONDTEST_PASS."


> + * Code that wants to check if a condition means that the instruction
> + * should be executed, should compare the return value to !0.

!0 == 1.  I guess this is not what you mean.

Maybe:

"Code that needs to check when an instruction would architecturally
execute (including some instructions on ARMv5 and later which cannot be
conditional) should check whether the return value !=
ARM_OPCODE_CONDTEST_FAIL."

This documentation should perhaps move to opcodes.h, but I don't have a
strong opinion.

> + */
> +unsigned int arm_check_condition(unsigned int opcode, unsigned int psr)
> +{
> +	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
> +			return ARM_OPCODE_CONDTEST_FAIL;
> +	 } else {

Remove the space before } (checkpatch.pl ought to pick this up)

> +		return ARM_OPCODE_CONDTEST_UNCOND;
> +	}
> +}
> +
> +EXPORT_SYMBOL(arm_check_condition);

Cheers
---Dave

[PATCH 3/4] Add condition code checking to SWP emulation handler.

[Dave Martin]

On Fri, Nov 25, 2011 at 05:19:53PM +0000, Leif Lindholm wrote:
> This patch fixes two separate issues with the SWP emulation handler:
> 1: Certain processors implementing ARMv7-A can (legally) take an
>    undef exception even when the condition code would have meant that
>    the instruction should not have been executed.
> 2: Opcodes with all flags set (condition code = 0xf) have been reused
>    in recent, and not-so-recent, versions of the ARM architecture to
>    implement unconditional extensions to the instruction set. The
>    existing code would still have processed any undefs triggered by
>    executing an opcode with such a value.
> 
> This patch uses the new generic ARM instruction set condition code
> checks to implement proper handling of these situations.
> 
> Signed-off-by: Leif Lindholm <leif.lindholm@arm.com>
> ---
>  arch/arm/kernel/swp_emulate.c |   14 ++++++++++++++
>  1 files changed, 14 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/kernel/swp_emulate.c b/arch/arm/kernel/swp_emulate.c
> index 5f452f8..8629bf7 100644
> --- a/arch/arm/kernel/swp_emulate.c
> +++ b/arch/arm/kernel/swp_emulate.c
> @@ -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,19 @@ 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);
> +	switch (res) {
> +	case ARM_OPCODE_CONDTEST_FAIL: {
> +		/* Condition failed - return to next instruction */
> +		regs->ARM_pc += 4;
> +		return 0;
> +	} break;
> +	case ARM_OPCODE_CONDTEST_UNCOND: {
> +		/* If unconditional encoding - not a SWP, undef */
> +		return -EFAULT;
> +	} break;
> +	}
> +

Can we lose the extra { } inside the switch here?

Those cases contain no declarations, so there's no need for a nested
block in either case.  This also solves the indentation problem.

Documentation/CodingStyle appears to prefer an unconditional break; to
be indented flush with the contents of the case block that it ends.

Cheers
---Dave

[PATCH 4/4] Use generic ARM instruction set condition code checks for kprobes.

[Dave Martin]

On Fri, Nov 25, 2011 at 05:20:05PM +0000, Leif Lindholm wrote:
> This patch changes the kprobes implementation to use the generic ARM
> instruction set condition code checks, rather than a dedicated
> implementation.
> 
> Cc: Tixy <tixy@yxit.co.uk>
> Signed-off-by: Leif Lindholm <leif.lindholm@arm.com>
> ---
>  arch/arm/kernel/kprobes-test.c |   66 ++++------------------------------------
>  1 files changed, 6 insertions(+), 60 deletions(-)
> 
> diff --git a/arch/arm/kernel/kprobes-test.c b/arch/arm/kernel/kprobes-test.c
> index e17cdd6..278c275 100644
> --- a/arch/arm/kernel/kprobes-test.c
> +++ b/arch/arm/kernel/kprobes-test.c
> @@ -202,6 +202,8 @@
>  #include <linux/slab.h>
>  #include <linux/kprobes.h>
>  
> +#include <asm/opcodes.h>
> +
>  #include "kprobes.h"
>  #include "kprobes-test.h"
>  
> @@ -1048,67 +1050,11 @@ static int test_instance;
>   */
>  #define PSR_IGNORE_BITS (PSR_A_BIT | PSR_F_BIT)
>  
> -static unsigned long test_check_cc(int cc, unsigned long cpsr)
> +inline unsigned long test_check_cc(int cc, unsigned long cpsr)
>  {
> -	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;
> -
> -	case 0x5: /* pl */
> -		return (~cpsr) & PSR_N_BIT;
> -
> -	case 0x6: /* vs */
> -		return cpsr & PSR_V_BIT;
> -
> -	case 0x7: /* vc */
> -		return (~cpsr) & PSR_V_BIT;
> +	int ret = arm_check_condition(cc << 28, cpsr);
>  
> -	case 0x8: /* hi */
> -		cpsr &= ~(cpsr >> 1); /* PSR_C_BIT &= ~PSR_Z_BIT */
> -		return cpsr & PSR_C_BIT;
> -
> -	case 0x9: /* ls */
> -		cpsr &= ~(cpsr >> 1); /* PSR_C_BIT &= ~PSR_Z_BIT */
> -		return (~cpsr) & PSR_C_BIT;
> -
> -	case 0xa: /* ge */
> -		cpsr ^= (cpsr << 3); /* PSR_N_BIT ^= PSR_V_BIT */
> -		return (~cpsr) & PSR_N_BIT;
> -
> -	case 0xb: /* lt */
> -		cpsr ^= (cpsr << 3); /* PSR_N_BIT ^= PSR_V_BIT */
> -		return cpsr & PSR_N_BIT;
> -
> -	case 0xc: /* gt */
> -		temp = cpsr ^ (cpsr << 3); /* PSR_N_BIT ^= PSR_V_BIT */
> -		temp |= (cpsr << 1);	   /* PSR_N_BIT |= PSR_Z_BIT */
> -		return (~temp) & PSR_N_BIT;
> -
> -	case 0xd: /* le */
> -		temp = cpsr ^ (cpsr << 3); /* PSR_N_BIT ^= PSR_V_BIT */
> -		temp |= (cpsr << 1);	   /* PSR_N_BIT |= PSR_Z_BIT */
> -		return temp & PSR_N_BIT;
> -
> -	case 0xe: /* al */
> -	case 0xf: /* unconditional */
> -		return true;
> -	}
> -	BUG();
> -	return false;
> +	return (ret != ARM_OPCODE_CONDTEST_FAIL);

Redundant ().

I also don't see a reason not to do the following:

	return arm_check_condition(cc << 28, cpsr) != ARM_OPCODE_CONDTEST_FAIL;

Up to you, though.  The presence of "ret" is harmless; the compiler
ought to generate the exact same code in any case.

>  }
>  
>  static int is_last_scenario;
> @@ -1128,7 +1074,7 @@ static unsigned long test_context_cpsr(int scenario)
>  
>  	if (!test_case_is_thumb) {
>  		/* Testing ARM code */
> -		probe_should_run = test_check_cc(current_instruction >> 28, cpsr) != 0;
> +		probe_should_run = arm_check_condition(current_instruction, cpsr) != 0;

Use the appropriate symbolic constant for this comparison.

Cheers
---Dave

[PATCH 4/4] Use generic ARM instruction set condition code checks for kprobes.

[Leif Lindholm]

This patch changes the kprobes implementation to use the generic ARM
instruction set condition code checks, rather than a dedicated
implementation.

Signed-off-by: Leif Lindholm <leif.lindholm@arm.com>
Acked-by: Jon Medhurst <tixy@yxit.co.uk>
---
 arch/arm/kernel/kprobes-test.c |   66 ++++------------------------------------
 1 files changed, 6 insertions(+), 60 deletions(-)

diff --git a/arch/arm/kernel/kprobes-test.c b/arch/arm/kernel/kprobes-test.c
index e17cdd6..f81c2b4 100644
--- a/arch/arm/kernel/kprobes-test.c
+++ b/arch/arm/kernel/kprobes-test.c
@@ -202,6 +202,8 @@
 #include <linux/slab.h>
 #include <linux/kprobes.h>
 
+#include <asm/opcodes.h>
+
 #include "kprobes.h"
 #include "kprobes-test.h"
 
@@ -1048,67 +1050,11 @@ static int test_instance;
  */
 #define PSR_IGNORE_BITS (PSR_A_BIT | PSR_F_BIT)
 
-static unsigned long test_check_cc(int cc, unsigned long cpsr)
+inline unsigned long test_check_cc(int cc, unsigned long cpsr)
 {
-	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;
-
-	case 0x5: /* pl */
-		return (~cpsr) & PSR_N_BIT;
-
-	case 0x6: /* vs */
-		return cpsr & PSR_V_BIT;
-
-	case 0x7: /* vc */
-		return (~cpsr) & PSR_V_BIT;
+	int ret = arm_check_condition(cc << 28, cpsr);
 
-	case 0x8: /* hi */
-		cpsr &= ~(cpsr >> 1); /* PSR_C_BIT &= ~PSR_Z_BIT */
-		return cpsr & PSR_C_BIT;
-
-	case 0x9: /* ls */
-		cpsr &= ~(cpsr >> 1); /* PSR_C_BIT &= ~PSR_Z_BIT */
-		return (~cpsr) & PSR_C_BIT;
-
-	case 0xa: /* ge */
-		cpsr ^= (cpsr << 3); /* PSR_N_BIT ^= PSR_V_BIT */
-		return (~cpsr) & PSR_N_BIT;
-
-	case 0xb: /* lt */
-		cpsr ^= (cpsr << 3); /* PSR_N_BIT ^= PSR_V_BIT */
-		return cpsr & PSR_N_BIT;
-
-	case 0xc: /* gt */
-		temp = cpsr ^ (cpsr << 3); /* PSR_N_BIT ^= PSR_V_BIT */
-		temp |= (cpsr << 1);	   /* PSR_N_BIT |= PSR_Z_BIT */
-		return (~temp) & PSR_N_BIT;
-
-	case 0xd: /* le */
-		temp = cpsr ^ (cpsr << 3); /* PSR_N_BIT ^= PSR_V_BIT */
-		temp |= (cpsr << 1);	   /* PSR_N_BIT |= PSR_Z_BIT */
-		return temp & PSR_N_BIT;
-
-	case 0xe: /* al */
-	case 0xf: /* unconditional */
-		return true;
-	}
-	BUG();
-	return false;
+	return (ret != ARM_OPCODE_CONDTEST_FAIL);
 }
 
 static int is_last_scenario;
@@ -1128,7 +1074,7 @@ static unsigned long test_context_cpsr(int scenario)
 
 	if (!test_case_is_thumb) {
 		/* Testing ARM code */
-		probe_should_run = test_check_cc(current_instruction >> 28, cpsr) != 0;
+		probe_should_run = arm_check_condition(current_instruction, cpsr) != ARM_OPCODE_CONDTEST_FAIL;
 		if (scenario == 15)
 			is_last_scenario = true;
 

[PATCH 4/4] Use generic ARM instruction set condition code checks for kprobes.

[Will Deacon]

DISCLAIMER: I'm not a kprobes expert!

On Thu, Dec 08, 2011 at 05:32:13PM +0000, Leif Lindholm wrote:
> diff --git a/arch/arm/kernel/kprobes-test.c b/arch/arm/kernel/kprobes-test.c
> index e17cdd6..f81c2b4 100644
> --- a/arch/arm/kernel/kprobes-test.c
> +++ b/arch/arm/kernel/kprobes-test.c
> @@ -202,6 +202,8 @@
>  #include <linux/slab.h>
>  #include <linux/kprobes.h>
>  
> +#include <asm/opcodes.h>
> +

[...]

> -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?

>  {
> -	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;
> -
> -	case 0x5: /* pl */
> -		return (~cpsr) & PSR_N_BIT;
> -
> -	case 0x6: /* vs */
> -		return cpsr & PSR_V_BIT;
> -
> -	case 0x7: /* vc */
> -		return (~cpsr) & PSR_V_BIT;
> +	int ret = arm_check_condition(cc << 28, cpsr);

[...]

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?

> @@ -1128,7 +1074,7 @@ static unsigned long test_context_cpsr(int scenario)
>  
>  	if (!test_case_is_thumb) {
>  		/* Testing ARM code */
> -		probe_should_run = test_check_cc(current_instruction >> 28, cpsr) != 0;
> +		probe_should_run = arm_check_condition(current_instruction, cpsr) != ARM_OPCODE_CONDTEST_FAIL;
>  		if (scenario == 15)
>  			is_last_scenario = true;

Will

[PATCH 4/4] Use generic ARM instruction set condition code checks for kprobes.

[Leif Lindholm]

On 12/09/11 15:54, Will Deacon wrote:
> On Thu, Dec 08, 2011 at 05:32:13PM +0000, Leif Lindholm wrote:
>> -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.

> 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.
Also, since in the other locations test_check_cc is actually used to 
check conditions for Thumb instructions (rather than ARM), it makes 
sense to me to wrap it behind something that doesn't appear to suggest 
otherwise.

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

/
     Leif

[PATCH 4/4] Use generic ARM instruction set condition code checks for kprobes.

[Will Deacon]

On Fri, Dec 09, 2011 at 04:17:44PM +0000, Leif Lindholm wrote:
> On 12/09/11 15:54, Will Deacon wrote:
> > On Thu, Dec 08, 2011 at 05:32:13PM +0000, Leif Lindholm wrote:
> >> -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.

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.

> > 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?

> 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.

Will

[PATCH 4/4] Use generic ARM instruction set condition code checks for kprobes.

[Leif Lindholm]

On 12/09/11 16:40, Will Deacon wrote:
 > 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.

OK

>>> 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?

I would have preferred the opposite, but that would seem to require more
invasive changes to the Thumb handling code (at least for the
'kprobe_test_flags & TEST_FLAG_NO_ITBLOCK' case). Will do.

/
     Leif

-- IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium.  Thank you.

[PATCH 4/4] Use generic ARM instruction set condition code checks for kprobes.

[Jon Medhurst (Tixy)]

On Fri, 2011-12-09 at 16:40 +0000, Will Deacon wrote:
> On Fri, Dec 09, 2011 at 04:17:44PM +0000, Leif Lindholm wrote:
> > On 12/09/11 15:54, Will Deacon wrote:
> > > On Thu, Dec 08, 2011 at 05:32:13PM +0000, Leif Lindholm wrote:
> > >> -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.
> 
> 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.

As you pointed out later, it does need to be static, to avoid potential
linkage issues. (I always incorrectly imagine that 'inline' is a
glorified macro.)

> > > 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?

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.

> > 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.

-- 
Tixy

[PATCH 4/4] Use generic ARM instruction set condition code checks for kprobes.

[Leif Lindholm]

On 12/09/11 18:05, Jon Medhurst (Tixy) wrote:
>>> 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.

It doesn't, but...

> 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.

I did what Will suggested though, and it does make it a bit cleaner. It
also moves the ARM_OPCODE_* testing out of the test_context_cpsr code,
so that all three cases have the probe_should_run setting pretty much
identical.

I'll post the updated set shortly, so you can have a look.

/
        Leif

-- IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium.  Thank you.

[PATCH 4/4] Use generic ARM instruction set condition code checks for kprobes.

[Leif Lindholm]

On 12/09/11 18:05, Jon Medhurst (Tixy) wrote:
>>> 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.

It doesn't, but...

> 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.

I did what Will suggested though, and it does make it a bit cleaner. It 
also moves the ARM_OPCODE_* testing out of the test_context_cpsr code, 
so that all three cases have the probe_should_run setting pretty much 
identical.

I'll post the updated set shortly, so you can have a look.

/
	Leif

[PATCH 4/4] Use generic ARM instruction set condition code checks for kprobes.

[Leif Lindholm]

This patch changes the kprobes implementation to use the generic ARM
instruction set condition code checks, rather than a dedicated
implementation.

Signed-off-by: Leif Lindholm <leif.lindholm@arm.com>
---
Tixy's ACK removed as patch modified.

 arch/arm/kernel/kprobes-test.c |   66 ++++------------------------------------
 1 files changed, 7 insertions(+), 59 deletions(-)

diff --git a/arch/arm/kernel/kprobes-test.c b/arch/arm/kernel/kprobes-test.c
index e17cdd6..1862d8f 100644
--- a/arch/arm/kernel/kprobes-test.c
+++ b/arch/arm/kernel/kprobes-test.c
@@ -202,6 +202,8 @@
 #include <linux/slab.h>
 #include <linux/kprobes.h>
 
+#include <asm/opcodes.h>
+
 #include "kprobes.h"
 #include "kprobes-test.h"
 
@@ -1050,65 +1052,9 @@ static int test_instance;
 
 static unsigned long test_check_cc(int cc, unsigned long cpsr)
 {
-	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;
-
-	case 0x5: /* pl */
-		return (~cpsr) & PSR_N_BIT;
-
-	case 0x6: /* vs */
-		return cpsr & PSR_V_BIT;
-
-	case 0x7: /* vc */
-		return (~cpsr) & PSR_V_BIT;
+	int ret = arm_check_condition(cc << 28, cpsr);
 
-	case 0x8: /* hi */
-		cpsr &= ~(cpsr >> 1); /* PSR_C_BIT &= ~PSR_Z_BIT */
-		return cpsr & PSR_C_BIT;
-
-	case 0x9: /* ls */
-		cpsr &= ~(cpsr >> 1); /* PSR_C_BIT &= ~PSR_Z_BIT */
-		return (~cpsr) & PSR_C_BIT;
-
-	case 0xa: /* ge */
-		cpsr ^= (cpsr << 3); /* PSR_N_BIT ^= PSR_V_BIT */
-		return (~cpsr) & PSR_N_BIT;
-
-	case 0xb: /* lt */
-		cpsr ^= (cpsr << 3); /* PSR_N_BIT ^= PSR_V_BIT */
-		return cpsr & PSR_N_BIT;
-
-	case 0xc: /* gt */
-		temp = cpsr ^ (cpsr << 3); /* PSR_N_BIT ^= PSR_V_BIT */
-		temp |= (cpsr << 1);	   /* PSR_N_BIT |= PSR_Z_BIT */
-		return (~temp) & PSR_N_BIT;
-
-	case 0xd: /* le */
-		temp = cpsr ^ (cpsr << 3); /* PSR_N_BIT ^= PSR_V_BIT */
-		temp |= (cpsr << 1);	   /* PSR_N_BIT |= PSR_Z_BIT */
-		return temp & PSR_N_BIT;
-
-	case 0xe: /* al */
-	case 0xf: /* unconditional */
-		return true;
-	}
-	BUG();
-	return false;
+	return (ret != ARM_OPCODE_CONDTEST_FAIL);
 }
 
 static int is_last_scenario;
@@ -1128,7 +1074,9 @@ static unsigned long test_context_cpsr(int scenario)
 
 	if (!test_case_is_thumb) {
 		/* Testing ARM code */
-		probe_should_run = test_check_cc(current_instruction >> 28, cpsr) != 0;
+		int cc = current_instruction >> 28;
+
+		probe_should_run = test_check_cc(cc, cpsr) != 0;
 		if (scenario == 15)
 			is_last_scenario = true;
 

[PATCH 4/4] Use generic ARM instruction set condition code checks for kprobes.

[Tixy]

On Fri, 2011-12-09 at 18:55 +0000, Leif Lindholm wrote:
> This patch changes the kprobes implementation to use the generic ARM
> instruction set condition code checks, rather than a dedicated
> implementation.
> 
> Signed-off-by: Leif Lindholm <leif.lindholm@arm.com>

Acked-by: Jon Medhurst <tixy@yxit.co.uk>

> ---
> Tixy's ACK removed as patch modified.
> 
>  arch/arm/kernel/kprobes-test.c |   66 ++++------------------------------------
>  1 files changed, 7 insertions(+), 59 deletions(-)
> 
> diff --git a/arch/arm/kernel/kprobes-test.c b/arch/arm/kernel/kprobes-test.c
> index e17cdd6..1862d8f 100644
> --- a/arch/arm/kernel/kprobes-test.c
> +++ b/arch/arm/kernel/kprobes-test.c
> @@ -202,6 +202,8 @@
>  #include <linux/slab.h>
>  #include <linux/kprobes.h>
>  
> +#include <asm/opcodes.h>
> +
>  #include "kprobes.h"
>  #include "kprobes-test.h"
>  
> @@ -1050,65 +1052,9 @@ static int test_instance;
>  
>  static unsigned long test_check_cc(int cc, unsigned long cpsr)
>  {
> -	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;
> -
> -	case 0x5: /* pl */
> -		return (~cpsr) & PSR_N_BIT;
> -
> -	case 0x6: /* vs */
> -		return cpsr & PSR_V_BIT;
> -
> -	case 0x7: /* vc */
> -		return (~cpsr) & PSR_V_BIT;
> +	int ret = arm_check_condition(cc << 28, cpsr);
>  
> -	case 0x8: /* hi */
> -		cpsr &= ~(cpsr >> 1); /* PSR_C_BIT &= ~PSR_Z_BIT */
> -		return cpsr & PSR_C_BIT;
> -
> -	case 0x9: /* ls */
> -		cpsr &= ~(cpsr >> 1); /* PSR_C_BIT &= ~PSR_Z_BIT */
> -		return (~cpsr) & PSR_C_BIT;
> -
> -	case 0xa: /* ge */
> -		cpsr ^= (cpsr << 3); /* PSR_N_BIT ^= PSR_V_BIT */
> -		return (~cpsr) & PSR_N_BIT;
> -
> -	case 0xb: /* lt */
> -		cpsr ^= (cpsr << 3); /* PSR_N_BIT ^= PSR_V_BIT */
> -		return cpsr & PSR_N_BIT;
> -
> -	case 0xc: /* gt */
> -		temp = cpsr ^ (cpsr << 3); /* PSR_N_BIT ^= PSR_V_BIT */
> -		temp |= (cpsr << 1);	   /* PSR_N_BIT |= PSR_Z_BIT */
> -		return (~temp) & PSR_N_BIT;
> -
> -	case 0xd: /* le */
> -		temp = cpsr ^ (cpsr << 3); /* PSR_N_BIT ^= PSR_V_BIT */
> -		temp |= (cpsr << 1);	   /* PSR_N_BIT |= PSR_Z_BIT */
> -		return temp & PSR_N_BIT;
> -
> -	case 0xe: /* al */
> -	case 0xf: /* unconditional */
> -		return true;
> -	}
> -	BUG();
> -	return false;
> +	return (ret != ARM_OPCODE_CONDTEST_FAIL);
>  }
>  
>  static int is_last_scenario;
> @@ -1128,7 +1074,9 @@ static unsigned long test_context_cpsr(int scenario)
>  
>  	if (!test_case_is_thumb) {
>  		/* Testing ARM code */
> -		probe_should_run = test_check_cc(current_instruction >> 28, cpsr) != 0;
> +		int cc = current_instruction >> 28;
> +
> +		probe_should_run = test_check_cc(cc, cpsr) != 0;
>  		if (scenario == 15)
>  			is_last_scenario = true;
>  
> 
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 4/4] Use generic ARM instruction set condition code checks for kprobes.

[Will Deacon]

On Fri, Dec 09, 2011 at 06:55:44PM +0000, Leif Lindholm wrote:
> This patch changes the kprobes implementation to use the generic ARM
> instruction set condition code checks, rather than a dedicated
> implementation.
> 
> Signed-off-by: Leif Lindholm <leif.lindholm@arm.com>
> ---
> Tixy's ACK removed as patch modified.
> 
>  arch/arm/kernel/kprobes-test.c |   66 ++++------------------------------------
>  1 files changed, 7 insertions(+), 59 deletions(-)
> 

This looks good to me. Given that Tixy's re-acked it, my reviewed-by is fairly
worthless, but here it is anyway (take it or leave it!):

Reviewed-by: Will Deacon <will.deacon@arm.com>

Will