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

[PATCH 1/4] [RFC] 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
somwhat 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 |   14 +++++++++
 arch/arm/kernel/Makefile       |    2 +
 arch/arm/kernel/opcodes.c      |   61 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 76 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..4c18fcb
--- /dev/null
+++ b/arch/arm/include/asm/opcodes.h
@@ -0,0 +1,14 @@
+/*
+ *  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
+
+extern unsigned int arm_check_condition(unsigned int opcode, unsigned int psr);
+
+#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..8adc20c
--- /dev/null
+++ b/arch/arm/kernel/opcodes.c
@@ -0,0 +1,61 @@
+/*
+ *  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 <asm/opcodes.h>
+
+#define ARM_OPCODE_UNCONDITIONAL 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:
+ * 0 - if condition fails
+ * 1 - if condition passes (including AL)
+ * 2 - if unconditional encoding (or before ARMv3, NV condition)
+ *
+ * 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;
+
+	/* NV condition or unconditional */
+	if (cc_bits == ARM_OPCODE_UNCONDITIONAL)
+		return 2;
+
+	return (cc_map[cc_bits] >> (psr_cond)) & 1;
+}

[PATCH 2/4] [RFC] 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}).

This code has not been tested beyond building, linking and booting.

Signed-off-by: Leif Lindholm <leif.lindholm@arm.com>
---
 arch/arm/nwfpe/entry.S    |    6 +++---
 arch/arm/nwfpe/fpopcode.c |   26 --------------------------
 arch/arm/nwfpe/fpopcode.h |    3 ---
 3 files changed, 3 insertions(+), 32 deletions(-)

diff --git a/arch/arm/nwfpe/entry.S b/arch/arm/nwfpe/entry.S
index cafa183..749de0a 100644
--- a/arch/arm/nwfpe/entry.S
+++ b/arch/arm/nwfpe/entry.S
@@ -81,11 +81,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, #1			@ r0 = 1 ==> 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] [RFC] 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 the 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 |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/arch/arm/kernel/swp_emulate.c b/arch/arm/kernel/swp_emulate.c
index 5f452f8..2e379d5 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>
 
@@ -183,6 +184,16 @@ static int swp_handler(struct pt_regs *regs, unsigned int instr)
 	unsigned int address, destreg, data, type;
 	unsigned int res = 0;
 
+	res = arm_check_condition(instr, regs->ARM_cpsr);
+	if (!res) {
+		/* Condition failed - return to next instruction */
+		regs->ARM_pc += 4;
+		return 0;
+	} else if (res != 1) {
+		/* If unconditional encoding - not a SWP, undef */
+		return -EFAULT;
+	}
+
 	perf_sw_event(PERF_COUNT_SW_EMULATION_FAULTS, 1, regs, regs->ARM_pc);
 
 	if (current->pid != previous_pid) {

[PATCH 4/4] [RFC] 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.

Note, this is a direct interface change, and the resulting code is not
the prettiest, but this is an RFC only.

This code builds and links, but current 3.2-rc2 does not boot on my
Versatile Express board with CONFIG_ARM_KPROBES_TEST enabled either
with or without this patch.

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

diff --git a/arch/arm/kernel/kprobes-test.c b/arch/arm/kernel/kprobes-test.c
index e17cdd6..6f5328a 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,69 +1050,6 @@ 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)
-{
-	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;
-
-	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;
-}
-
 static int is_last_scenario;
 static int probe_should_run; /* 0 = no, 1 = yes, -1 = unknown */
 static int memory_needs_checking;
@@ -1128,7 +1067,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;
 
@@ -1136,7 +1075,7 @@ static unsigned long test_context_cpsr(int scenario)
 		/* Testing Thumb code without setting ITSTATE */
 		if (kprobe_test_cc_position) {
 			int cc = (current_instruction >> kprobe_test_cc_position) & 0xf;
-			probe_should_run = test_check_cc(cc, cpsr) != 0;
+			probe_should_run = arm_check_condition(cc << 28, cpsr) != 0;
 		}
 
 		if (scenario == 15)
@@ -1163,7 +1102,7 @@ static unsigned long test_context_cpsr(int scenario)
 		cpsr |= (mask & 0x8) << 23;	/* ITSTATE<1> */
 		cpsr |= (mask & 0x10) << 21;	/* ITSTATE<0> */
 
-		probe_should_run = test_check_cc((cpsr >> 12) & 0xf, cpsr) != 0;
+		probe_should_run = arm_check_condition(((cpsr >> 12) & 0xf) << 28, cpsr) != 0;
 
 	} else {
 		/* Testing Thumb code with several combinations of ITSTATE */

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

[Will Deacon]

Hi Leif,

On Mon, Nov 21, 2011 at 06:30:46PM +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
> somwhat for coding style adherence, and adds some temporary variables for
> increased readability.

s/somwhat/somewhat/

> diff --git a/arch/arm/kernel/opcodes.c b/arch/arm/kernel/opcodes.c
> new file mode 100644
> index 0000000..8adc20c
> --- /dev/null
> +++ b/arch/arm/kernel/opcodes.c
> @@ -0,0 +1,61 @@
> +/*
> + *  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 <asm/opcodes.h>
> +
> +#define ARM_OPCODE_UNCONDITIONAL 0xf

Maybe ARM_OPCODE_CONDITION_UNCOND or something (in case we decide to put
more opcodes in here).

> +/*
> + * 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:
> + * 0 - if condition fails
> + * 1 - if condition passes (including AL)
> + * 2 - if unconditional encoding (or before ARMv3, NV condition)

Can you #define these in the header file please? You'll need to make sure
they're available to assembly code too.

> + * 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)

Why not just return int?

> +{
> +	unsigned int cc_bits  = opcode >> 28;
> +	unsigned int psr_cond = psr >> 28;
> +
> +	/* NV condition or unconditional */
> +	if (cc_bits == ARM_OPCODE_UNCONDITIONAL)
> +		return 2;
> +
> +	return (cc_map[cc_bits] >> (psr_cond)) & 1;
> +}

Then you can use the new defines here.

Will

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

[Will Deacon]

On Mon, Nov 21, 2011 at 06:30:58PM +0000, Leif Lindholm wrote:
> diff --git a/arch/arm/kernel/swp_emulate.c b/arch/arm/kernel/swp_emulate.c
> index 5f452f8..2e379d5 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>
>  
> @@ -183,6 +184,16 @@ static int swp_handler(struct pt_regs *regs, unsigned int instr)
>  	unsigned int address, destreg, data, type;
>  	unsigned int res = 0;
>  
> +	res = arm_check_condition(instr, regs->ARM_cpsr);
> +	if (!res) {
> +		/* Condition failed - return to next instruction */
> +		regs->ARM_pc += 4;
> +		return 0;
> +	} else if (res != 1) {
> +		/* If unconditional encoding - not a SWP, undef */
> +		return -EFAULT;
> +	}
> +
>  	perf_sw_event(PERF_COUNT_SW_EMULATION_FAULTS, 1, regs, regs->ARM_pc);

Although the definition of this perf event is by no means well defined, I
think I'd like it to increment even if the opcode condigion isn't satisfied.
After all, we *have* faulted by this point, we just haven't needed to do
anything much in the handler.

Will

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

[Russell King - ARM Linux]

On Mon, Nov 21, 2011 at 06:30:46PM +0000, Leif Lindholm wrote:
> +/*
> + * Returns:
> + * 0 - if condition fails
> + * 1 - if condition passes (including AL)
> + * 2 - if unconditional encoding (or before ARMv3, NV condition)

The comment is wrong.  NV always meant 'never execute' including v3,
v4 and v5 architectures.

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

[Tixy]

On Mon, 2011-11-21 at 18:31 +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.
> 
> Note, this is a direct interface change, and the resulting code is not
> the prettiest, but this is an RFC only.

It might be prettier to keep test_check_cc() but defined like:

inline unsigned long test_check_cc(int cc, unsigned long cpsr)
{
	return arm_check_condition(cc << 28, cpsr);
}

But still change to directly calling arm_check_condition() in the case
of checking ARM instructions.

> This code builds and links, but current 3.2-rc2 does not boot on my
> Versatile Express board with CONFIG_ARM_KPROBES_TEST enabled either
> with or without this patch.

What are the symptoms? Have you tried setting VERBOSE to '1' at the top
of kprobes-test.h?

Currently the test code has problems building for Thumb, there's a
patch:
http://www.mail-archive.com/linaro-dev at lists.linaro.org/msg07925.html

Also, when the test module is built-in, the tests fail near the
beginning due to an issue with Thumb symbol handling in the kernel, see
http://www.spinics.net/lists/arm-kernel/msg138283.html

But building it as a module and insmod'ing should work.

-- 
Tixy

> Cc: Tixy <tixy@yxit.co.uk>
> Signed-off-by: Leif Lindholm <leif.lindholm@arm.com>
> ---
>  arch/arm/kernel/kprobes-test.c |   71 +++-------------------------------------
>  1 files changed, 5 insertions(+), 66 deletions(-)
> 
> diff --git a/arch/arm/kernel/kprobes-test.c b/arch/arm/kernel/kprobes-test.c
> index e17cdd6..6f5328a 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,69 +1050,6 @@ 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)
> -{
> -	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;
> -
> -	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;
> -}
> -
>  static int is_last_scenario;
>  static int probe_should_run; /* 0 = no, 1 = yes, -1 = unknown */
>  static int memory_needs_checking;
> @@ -1128,7 +1067,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;
>  
> @@ -1136,7 +1075,7 @@ static unsigned long test_context_cpsr(int scenario)
>  		/* Testing Thumb code without setting ITSTATE */
>  		if (kprobe_test_cc_position) {
>  			int cc = (current_instruction >> kprobe_test_cc_position) & 0xf;
> -			probe_should_run = test_check_cc(cc, cpsr) != 0;
> +			probe_should_run = arm_check_condition(cc << 28, cpsr) != 0;
>  		}
>  
>  		if (scenario == 15)
> @@ -1163,7 +1102,7 @@ static unsigned long test_context_cpsr(int scenario)
>  		cpsr |= (mask & 0x8) << 23;	/* ITSTATE<1> */
>  		cpsr |= (mask & 0x10) << 21;	/* ITSTATE<0> */
>  
> -		probe_should_run = test_check_cc((cpsr >> 12) & 0xf, cpsr) != 0;
> +		probe_should_run = arm_check_condition(((cpsr >> 12) & 0xf) << 28, cpsr) != 0;
>  
>  	} else {
>  		/* Testing Thumb code with several combinations of ITSTATE */
> 
> 

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

[Dave Martin]

On Mon, Nov 21, 2011 at 07:08:48PM +0000, Russell King - ARM Linux wrote:
> On Mon, Nov 21, 2011 at 06:30:46PM +0000, Leif Lindholm wrote:
> > +/*
> > + * Returns:
> > + * 0 - if condition fails
> > + * 1 - if condition passes (including AL)
> > + * 2 - if unconditional encoding (or before ARMv3, NV condition)
> 
> The comment is wrong.  NV always meant 'never execute' including v3,
> v4 and v5 architectures.

Actually, if the v5 ARM ARM is to believed this was only true prior to
ARMv3.

Apparently the behaviour of instructions encoded with the NV condition
code is UNPREDICTABLE in v3 and v4.  With the exception of the ARMv5 BLX
<label> and v5E PLD instructions, I've no idea whether any actual
implementation did anything crazy with these opcodes though, prior to
ARMv6.

Perhaps someone implemented some v4/v3 parts with custom instructions in
this space, or dealt with the NV condition in a unexpected way -- I'm
not aware of any such implementation, though.

Maybe the comment can be reworded more conservatively, such as

"2 - NV condition, or separate unconditional opcode space from v5
onwards"

That wording judiciously doesn't say anything about v3/v4.

Cheers
---Dave

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

[Tixy]

On Thu, 2011-11-24 at 17:02 +0000, Leif Lindholm wrote of kprobes test
failures...
> Actually, it seems it fails explicitly because the SWP emulation is
> enabled (making the Cortex-A9 take an undef on SWP{B}):
> <<<
> .word 0xe0ff1392 @ smlals pc, r1, r2, r3
> .word 0xe0f0f392 @ smlals r0, pc, r2, r3
> .word 0xe0f0139f @ smlals r0, r1, pc, r3
> .word 0xe0f01f92 @ smlals r0, r1, r2, pc
> 
> Synchronization primitives
> ---------------------------------------------------------
> .word 0xe108e097 @ swp  lr, r7, [r8]
> Internal error: Oops - undefined instruction: 0 [#1] SMP
> Modules linked in: test_kprobes(+)
> CPU: 0    Not tainted  (3.2.0-rc3+ #9)
> PC is at kprobe_arm_test_cases+0xe1d0/0x18db9 [test_kprobes]
>  >>>

That would explain things, taking an undef abort in the kprobes handler
(which itself is running in an undef hook) will OOPS. I think that the
test code should not try to test the SWP instruction on ARMv7. It's
deprecated and the GCC compiler won't generate it - hence my need to
encode the instruction as ".word 0xe108e097" rather than get use
assembler syntax. I'll create a patch to update the test code.

> Now, disabling the SWP emulation, it still fails - but at least not as
> an oops this time:
> <<<
> FAIL: test memory differs
> FAIL: Test strd r2, [r3], r4
> FAIL: Scenario 0
> initial_regs:
> r0  21522152 | r1  21522052 | r2  12345678 | r3  b6521d78
> r4  00000030 | r5  21522452 | r6  21522752 | r7  21522652
> r8  21522952 | r9  21522852 | r10 21522b52 | r11 21522a52
> r12 21522d52 | sp  b6521d60 | lr  21522f52 | pc  7f010c34
> cpsr 00000013
> expected_regs:
> r0  21522152 | r1  21522052 | r2  12345678 | r3  b6521da8
> r4  00000030 | r5  21522452 | r6  21522752 | r7  21522652
> r8  21522952 | r9  21522852 | r10 21522b52 | r11 21522a52
> r12 21522d52 | sp  b6521d60 | lr  21522f52 | pc  7f010c38
> cpsr 00000013
> result_regs:
> r0  21522152 | r1  21522052 | r2  12345678 | r3  b6521da8
> r4  00000030 | r5  21522452 | r6  21522752 | r7  21522652
> r8  21522952 | r9  21522852 | r10 21522b52 | r11 21522a52
> 
> r12 21522d52 | sp  b6521d60 | lr  21522f52 | pc  7f010c38
> cpsr 00000013
> current_stack=b6521d60
> expected_memory:
> ba987654 ba987754 ba987854 ba987954
> ba987a54 ba987b54 12345678 b6521da8
> ba987e54 ba987f54 ba988054 ba988154
> ba988254 b6521dd8 ba988454 ba988554
> ba988654 ba988754 ba988854 ba988954
> ba988a54 ba988b54 ba988c54 ba988d54
> ba988e54 ba988f54 ba989054 ba989154
> ba989254 ba989354 ba989454 ba989554
> ba989654 ba989754 ba989854 ba989954
> ba989a54 ba989b54 ba989c54 ba989d54
> ba989e54 ba989f54 ba98a054 ba98a154
> ba98a254 ba98a354 ba98a454 ba98a554
> ba98a654 ba98a754 ba98a854 ba98a954
> ba98aa54 ba98ab54 ba98ac54 ba98ad54
> ba98ae54 ba98af54 ba98b054 ba98b154
> ba98b254 ba98b354 ba98b454 ba98b554
> result_memory:
> ba987654 ba987754 ba987854 ba987954
> ba987a54 ba987b54 12345678 b6521d78
> ba987e54 ba987f54 ba988054 ba988154
> ba988254 b6521dd8 ba988454 ba988554
> ba988654 ba988754 ba988854 ba988954
> ba988a54 ba988b54 ba988c54 ba988d54
> ba988e54 ba988f54 ba989054 ba989154
> ba989254 ba989354 ba989454 ba989554
> ba989654 ba989754 ba989854 ba989954
> ba989a54 ba989b54 ba989c54 ba989d54
> ba989e54 ba989f54 ba98a054 ba98a154
> ba98a254 ba98a354 ba98a454 ba98a554
> ba98a654 ba98a754 ba98a854 ba98a954
> ba98aa54 ba98ab54 ba98ac54 ba98ad54
> ba98ae54 ba98af54 ba98b054 ba98b154
> ba98b254 ba98b354 ba98b454 ba98b554
> kprobe tests failed
> FATAL: Error inserting test_kprobes
> 
The kprobes instruction emulation has stored the original value of r3
into memory, whereas the real instruction has store the updated value of
r3. Either this instruction form is UNPREDICTABLE, in which case the
test code should not be trying to test it, otherwise the instruction
emulation code needs fixing. Either way, a patch is needed. I'll look
into this.

> Should I go back on-list with this?

I've cc'd the list so the problem is archived.
 
-- 
Tixy

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

[Leif Lindholm]

On 11/24/11 19:40, Tixy wrote:
>> Now, disabling the SWP emulation, it still fails - but at least not as
>> an oops this time:
>> <<<
>> FAIL: test memory differs
>> FAIL: Test strd r2, [r3], r4
>> FAIL: Scenario 0
...
>> FATAL: Error inserting test_kprobes
>>
> The kprobes instruction emulation has stored the original value of r3
> into memory, whereas the real instruction has store the updated value of
> r3. Either this instruction form is UNPREDICTABLE, in which case the
> test code should not be trying to test it, otherwise the instruction
> emulation code needs fixing. Either way, a patch is needed. I'll look
> into this.

Yes, this instruction form is UNPREDICTABLE, and some quick experiments 
show that it behaves differently on for example Cortex-A8 and Cortex-A9.

/
     Leif