ARM SWP/SWPB emulation support
Hello
I have two questions regarding the following patch:
commit 64d2dc384e41e2b7acead6804593ddaaf8aad8e1
Author: Leif Lindholm<[email protected]>
Date: Thu Sep 16 18:00:47 2010 +0100
ARM: 6396/1: Add SWP/SWPB emulation for ARMv7 processors
First, the Kconfig text for the patch says the following:
"In some older versions of glibc [<=2.8] SWP is used during futex
trylock() operations with the assumption that the code will not be preempted. This invalid assumption may be more likely to fail with SWP emulation enabled, leading to deadlock of the user application."
I see that the SWP emulation is done using LDREX / STREX instructions, so it looks like the SWP emulation should happen in an atomic manner with respect to loading the previous value at the address being swapped with, and storing the new value. Does this text simply imply that the old glibc implementation of futex trylock() is incorrect in the specific way it uses SWP instructions, or does it mean that there is a specific atomicity problem within the SWP emulation code itself?
Second, I noticed that the SWP emulation code does not look@the condition code for the SWP instruction being emulated. From my understanding of the ARM architecture, the CPU may raise an undefined instruction exception regardless of the condition code that may appear in the relevant bits of the underfined instruction. Thus, with the SWP emulation code as-is, it is possible that a conditional SWP/SWPB instruction may raise an undefined instruction exception (and thus trigger the swp emulation code to run) even though the condition for that instruction may not have been met in CPSR. As a result, conditional SWP instructions may be emulated as if they had not been conditional. I have a patch to add condition code checking to the SWP emulation code and if this seems right to you, I could send it out. What do you think?
Thanks
Steve
Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
Quoted 15 lines · Stepan Moskovchenko
I have two questions regarding the following patch:
commit
64d2dc384e41e2b7acead6804593ddaaf8aad8e1
Author: Leif Lindholm<[email protected]>
Date: ? Thu Sep 16 18:00:47 2010 +0100
ARM: 6396/1: Add SWP/SWPB emulation for ARMv7 processorsFirst, the
Kconfigtext for the patch says the following:?"In some older versions of glibc [<=2.8] SWP is used during futex ?trylock() operations with the assumption that the code will not ?be preempted. This invalid assumption may be more likely to fail ?with SWP emulation enabled, leading to deadlock of the user ?application."
Quoted 11 lines
I see that the SWP emulation is done using LDREX / STREX instructions, so it looks like the SWP emulation should happen in an atomic manner with respect to
loading the previous value at the address being swapped with, and storing the
new value. Does this text simply imply that the old glibc implementation of futextrylock()is incorrect in the specific way it uses SWP instructions, or
does it mean that there is a specific atomicity problem within the SWP emulation
code itself?
The old glibc code assumes that a code sequence that includes SWP is atomic, not that the SWP instruction itself is atomic. It works as long as there is no interrupt during that sequence. With the SWP emulation, the SWP instruction traps into the kernel and the atomicity is automatically broken (Linux may reschedule a different task). The SWP emulation in the kernel provides an atomic implementation of SWP using LDREX/STREX (though only in relation to other CPUs in the inner shareable domain, it doesn't do bus locking to be used with devices).
Quoted 2 lines
Second, I noticed that the SWP emulation code does not look at the condition code for the SWP instruction being emulated.
Leif is just looking at this and should post a patch probably next week (it was already spotted by someone else).
Signature · 1 line
Catalin
PATCH 1/1 arm: Add condition code check to SWP emulator
When emulating a SWP/SWPB instruction, check the condition code of the instruction and compare it against CPSR status bits rather than relying on the architecture to only raise an undefined instruction exception if the condition checks are passing.
---
Hello
I already had a version prepared, so here it is.
Steve
arch/arm/kernel/swp_emulate.c | 57 +++++++++++++++++++++++++++++++++++++++++ 1 files changed, 57 insertions(+), 0 deletions(-)
arch/arm/kernel/swp_emulate.c +57 -0
@@ -173,6 +173,57 @@ static int emulate_swpX(unsigned int address, unsigned int *data,return res; } +static int check_condition (struct pt_regs *regs,unsigned int insn) +{ +unsigned int base_cond, neg, cond =0 ; +unsigned int cpsr_z, cpsr_c, cpsr_n, cpsr_v; + + cpsr_n = (regs->ARM_cpsr &PSR_N_BIT ) ?1 :0 ; + cpsr_z = (regs->ARM_cpsr &PSR_Z_BIT ) ?1 :0 ; + cpsr_c = (regs->ARM_cpsr &PSR_C_BIT ) ?1 :0 ; + cpsr_v = (regs->ARM_cpsr &PSR_V_BIT ) ?1 :0 ; + +/* Upper 3 bits indicate condition, lower bit incicates negation */ + base_cond = insn >>29 ; + neg = insn &BIT (28 ) ?1 :0 ; + +switch (base_cond) { +case 0x0 :/* equal */ + cond = cpsr_z; +break ; + +case 0x1 :/* carry set */ + cond = cpsr_c; +break ; + +case 0x2 :/* minus / negative */ + cond = cpsr_n; +break ; + +case 0x3 :/* overflow */ + cond = cpsr_v; +break ; + +case 0x4 :/* unsigned higher */ + cond = (cpsr_c ==1 ) && (cpsr_z ==0 ); +break ; + +case 0x5 :/* signed greater / equal */ + cond = (cpsr_n == cpsr_v); +break ; + +case 0x6 :/* signed greater */ + cond = (cpsr_z ==0 ) && (cpsr_n == cpsr_v); +break ; + +case 0x7 :/* always */ + cond =1 ; +break ; + }; + +return cond && !neg; +} +/* * swp_handler logs the id of calling process, dissects the instruction, sanity * checks the memory location, calls emulate_swpX for the actual operation and
@@ -191,6 +242,12 @@ static int swp_handler(struct pt_regs *regs, unsigned int instr) previous_pid = current->pid; } +/* Ignore the instruction if it fails its condition code check */ +if (!check_condition (regs, instr)) { + regs->ARM_pc +=4 ; +return 0 ; + } + address = regs->uregs[EXTRACT_REG_NUM (instr,RN_OFFSET )]; data = regs->uregs[EXTRACT_REG_NUM (instr,RT2_OFFSET )]; destreg =EXTRACT_REG_NUM (instr,RT_OFFSET );
Signature · 2 lines
Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
Re: [PATCH] arm: Add condition code check to SWP emulator
Quoted 39 lines · Stepan Moskovchenko
arch/arm/kernel/swp_emulate.c +39
-0+static int check_condition (struct pt_regs *regs,unsigned int insn) +{ +unsigned int base_cond, neg, cond =0 ; +unsigned int cpsr_z, cpsr_c, cpsr_n, cpsr_v; + + cpsr_n = (regs->ARM_cpsr &PSR_N_BIT ) ?1 :0 ; + cpsr_z = (regs->ARM_cpsr &PSR_Z_BIT ) ?1 :0 ; + cpsr_c = (regs->ARM_cpsr &PSR_C_BIT ) ?1 :0 ; + cpsr_v = (regs->ARM_cpsr &PSR_V_BIT ) ?1 :0 ; + +/* Upper 3 bits indicate condition, lower bit incicates negation */ + base_cond = insn >>29 ; + neg = insn &BIT (28 ) ?1 :0 ; + +switch (base_cond) { +case 0x0 :/* equal */ + cond = cpsr_z; +break ; + +case 0x1 :/* carry set */ + cond = cpsr_c; +break ; + +case 0x2 :/* minus / negative */ + cond = cpsr_n; +break ; + +case 0x3 :/* overflow */ + cond = cpsr_v; +break ; + +case 0x4 :/* unsigned higher */ + cond = (cpsr_c ==1 ) && (cpsr_z ==0 ); +break ; + +case 0x5 :/* signed greater / equal */ + cond = (cpsr_n == cpsr_v); +break ; +
arch/arm/kernel/swp_emulate.c static int emulate_swpX(unsigned int address, unsigned int *data, · line 224
Quoted 10 lines
arch/arm/kernel/swp_emulate.c +10
-0+case 0x6 :/* signed greater */ + cond = (cpsr_z ==0 ) && (cpsr_n == cpsr_v); +break ; + +case 0x7 :/* always */ + cond =1 ; +break ; + }; + +return cond && !neg;
There's a much better algorithm to check this. See the bottom of arch/arm/nwfpe/fpopcode.c.
It would probably be best for there to be a common function for doing this kind of check, rather than having several implementations of it scattered around the kernel.
Re: [PATCH] arm: Add condition code check to SWP emulator
Quoted 54 lines · Russell King - ARM Linux
Quoted 39 lines · Stepan Moskovchenko
(quoted diff) +39
-0+static int check_condition(struct pt_regs *regs, unsigned int insn) +{ + ? ? unsigned int base_cond, neg, cond = 0; + ? ? unsigned int cpsr_z, cpsr_c, cpsr_n, cpsr_v; + + ? ? cpsr_n = (regs->ARM_cpsr & PSR_N_BIT) ? 1 : 0; + ? ? cpsr_z = (regs->ARM_cpsr & PSR_Z_BIT) ? 1 : 0; + ? ? cpsr_c = (regs->ARM_cpsr & PSR_C_BIT) ? 1 : 0; + ? ? cpsr_v = (regs->ARM_cpsr & PSR_V_BIT) ? 1 : 0; + + ? ? /* Upper 3 bits indicate condition, lower bit incicates negation */ + ? ? base_cond = insn >> 29; + ? ? neg = insn & BIT(28) ? 1 : 0; + + ? ? switch (base_cond) { + ? ? case 0x0: ? ? ? /* equal */ + ? ? ? ? ? ? cond = cpsr_z; + ? ? ? ? ? ? break; + + ? ? case 0x1: ? ? ? /* carry set */ + ? ? ? ? ? ? cond = cpsr_c; + ? ? ? ? ? ? break; + + ? ? case 0x2: ? ? ? /* minus / negative */ + ? ? ? ? ? ? cond = cpsr_n; + ? ? ? ? ? ? break; + + ? ? case 0x3: ? ? ? /* overflow */ + ? ? ? ? ? ? cond = cpsr_v; + ? ? ? ? ? ? break; + + ? ? case 0x4: ? ? ? /* unsigned higher */ + ? ? ? ? ? ? cond = (cpsr_c == 1) && (cpsr_z == 0); + ? ? ? ? ? ? break; + + ? ? case 0x5: ? ? ? /* signed greater / equal */ + ? ? ? ? ? ? cond = (cpsr_n == cpsr_v); + ? ? ? ? ? ? break; +Review
Quoted 10 lines
(quoted diff) +10
-0+ ? ? case 0x6: ? ? ? /* signed greater */ + ? ? ? ? ? ? cond = (cpsr_z == 0) && (cpsr_n == cpsr_v); + ? ? ? ? ? ? break; + + ? ? case 0x7: ? ? ? /* always */ + ? ? ? ? ? ? cond = 1; + ? ? ? ? ? ? break; + ? ? }; + + ? ? return cond && !neg;There's a much better algorithm to check this. ?See the bottom of
arch/arm/nwfpe/fpopcode.c.
Quoted 3 lines
It would probably be best for there to be a common function for doing this kind of check, rather than having several implementations of it scattered around the kernel.
That's what Leif started doing.
Signature · 1 line
Catalin
Re: [PATCH] arm: Add condition code check to SWP emulator
Stepan, thank you for spotting this.
Quoted 10 lines · Catalin Marinas
Quoted 6 lines · Russell King - ARM Linux
There's a much better algorithm to check this. See the bottom of
arch/arm/nwfpe/fpopcode.c.It would probably be best for there to be a common function for doing this kind of check, rather than having several implementations of it scattered around the kernel.
That's what Leif started doing.
Yes, I put together an RFC patch set based on that.
But rather than duplicating code, I was looking to break it out for generic use. And then it was pointed out to me that we also have an additional implementation in kernel/kprobes-test.c (test_check_cc()).
I forgot to add the cover message on the set I just submitted to the linux-arm-kernel list, but it was meant to say:
---
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.
---
/
Leif