Re: [PATCH] ARM: vfp: Fix up exception location in Thumb mode
From: Colin Cross
Date: Tue Jan 25 2011 - 18:33:32 EST
On Fri, Jan 14, 2011 at 11:51 AM, Colin Cross <ccross@xxxxxxxxxxx> wrote:
> On Fri, Jan 14, 2011 at 11:23 AM, Colin Cross <ccross@xxxxxxxxxxx> wrote:
>> On Fri, Jan 14, 2011 at 10:47 AM, Russell King - ARM Linux
>> <linux@xxxxxxxxxxxxxxxx> wrote:
>>> So... this incrementally on top of the previous patch (which I've
>>> reproduced below as there's a subtle comment change in there wrt IRQ
>>> state.)
>>>
>>> This means we have consistent state - both r2 and regs->ARM_pc always
>>> point to the next instruction to be executed in every case, which means
>>> its easy to understand and remember while reading through the code.
>>>
>>> diff -u b/arch/arm/kernel/entry-armv.S b/arch/arm/kernel/entry-armv.S
>>> --- b/arch/arm/kernel/entry-armv.S
>>> +++ b/arch/arm/kernel/entry-armv.S
>>> @@ -499,10 +499,11 @@
>>> blo __und_usr_unknown
>>> 3: ldrht r0, [r4]
>>> add r2, r2, #2 @ r2 is PC + 2, make it PC + 4
>>> - orr r0, r0, r5, lsl #16
>>> + str r2, [sp, #S_PC] @ it's a 2x16bit instr, update
>>> + orr r0, r0, r5, lsl #16 @ regs->ARM_pc
>>> @
>>> @ r0 = the two 16-bit Thumb instructions which caused the exception
>>> - @ r2 = PC value for the following Thumb instruction (:= regs->ARM_pc+2)
>>> + @ r2 = PC value for the following Thumb instruction (:= regs->ARM_pc)
>>> @ r4 = PC value for the first 16-bit Thumb instruction
>>> @
>>> #else
>>>
>>> 8<-x-x-
>>>
>>> diff --git a/arch/arm/kernel/entry-armv.S b/arch/arm/kernel/entry-armv.S
>>> index 2b46fea..5876eec 100644
>>> --- a/arch/arm/kernel/entry-armv.S
>>> +++ b/arch/arm/kernel/entry-armv.S
>>> @@ -461,27 +461,35 @@ ENDPROC(__irq_usr)
>>> .align 5
>>> __und_usr:
>>> usr_entry
>>> -
>>> - @
>>> - @ fall through to the emulation code, which returns using r9 if
>>> - @ it has emulated the instruction, or the more conventional lr
>>> - @ if we are to treat this as a real undefined instruction
>>> @
>>> - @ r0 - instruction
>>> + @ The emulation code returns using r9 if it has emulated the
>>> + @ instruction, or the more conventional lr if we are to treat
>>> + @ this as a real undefined instruction
>>> @
>>> adr r9, BSYM(ret_from_exception)
>>> adr lr, BSYM(__und_usr_unknown)
>>> + @
>>> + @ r2 = regs->ARM_pc, which is either 2 or 4 bytes ahead of the
>>> + @ faulting instruction depending on Thumb mode.
>>> + @ r3 = regs->ARM_cpsr
>>> + @
>>> tst r3, #PSR_T_BIT @ Thumb mode?
>>> - itet eq @ explicit IT needed for the 1f label
>>> + itttt eq @ explicit IT needed for the 1f label
>>> subeq r4, r2, #4 @ ARM instr at LR - 4
>>> - subne r4, r2, #2 @ Thumb instr at LR - 2
>>> 1: ldreqt r0, [r4]
>>> #ifdef CONFIG_CPU_ENDIAN_BE8
>>> reveq r0, r0 @ little endian instruction
>>> #endif
>>> + @
>>> + @ r0 = 32-bit ARM instruction which caused the exception
>>> + @ r2 = PC value for the following instruction (:= regs->ARM_pc)
>>> + @ r4 = PC value for the faulting instruction
>>> + @
>>> beq call_fpe
>>> +
>>> @ Thumb instruction
>>> #if __LINUX_ARM_ARCH__ >= 7
>>> + sub r4, r2, #2 @ Thumb instr at LR - 2
>>> 2:
>>> ARM( ldrht r5, [r4], #2 )
>>> THUMB( ldrht r5, [r4] )
>>> @@ -492,18 +500,19 @@ __und_usr:
>>> 3: ldrht r0, [r4]
>>> add r2, r2, #2 @ r2 is PC + 2, make it PC + 4
>>> orr r0, r0, r5, lsl #16
>>> + @
>>> + @ r0 = the two 16-bit Thumb instructions which caused the exception
>>> + @ r2 = PC value for the following Thumb instruction (:= regs->ARM_pc+2)
>>> + @ r4 = PC value for the first 16-bit Thumb instruction
>>> + @
>>> #else
>>> b __und_usr_unknown
>>> #endif
>>> - UNWIND(.fnend )
>>> + UNWIND(.fnend)
>>> ENDPROC(__und_usr)
>>>
>>> - @
>>> - @ fallthrough to call_fpe
>>> - @
>>> -
>>> /*
>>> - * The out of line fixup for the ldrt above.
>>> + * The out of line fixup for the ldrt instructions above.
>>> */
>>> .pushsection .fixup, "ax"
>>> 4: mov pc, r9
>>> @@ -534,11 +543,12 @@ ENDPROC(__und_usr)
>>> * NEON handler code.
>>> *
>>> * Emulators may wish to make use of the following registers:
>>> - * r0 = instruction opcode.
>>> - * r2 = PC+4
>>> + * r0 = instruction opcode (32-bit ARM or two 16-bit Thumb)
>>> + * r2 = PC value to resume execution after successful emulation
>>> * r9 = normal "successful" return address
>>> - * r10 = this threads thread_info structure.
>>> + * r10 = this threads thread_info structure
>>> * lr = unrecognised instruction return address
>>> + * IRQs disabled, FIQs enabled.
>>> */
>>> @
>>> @ Fall-through from Thumb-2 __und_usr
>>> diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c
>>> index ee57640..eeb9250 100644
>>> --- a/arch/arm/kernel/traps.c
>>> +++ b/arch/arm/kernel/traps.c
>>> @@ -347,9 +347,9 @@ asmlinkage void __exception do_undefinstr(struct pt_regs *regs)
>>> void __user *pc;
>>>
>>> /*
>>> - * According to the ARM ARM, PC is 2 or 4 bytes ahead,
>>> - * depending whether we're in Thumb mode or not.
>>> - * Correct this offset.
>>> + * According to the ARM ARM, the PC is 2 or 4 bytes ahead
>>> + * depending on Thumb mode. Correct this offset so that
>>> + * regs->ARM_pc points at the faulting instruction.
>>> */
>>> regs->ARM_pc -= correction;
>>>
>>> diff --git a/arch/arm/vfp/entry.S b/arch/arm/vfp/entry.S
>>> index 4fa9903..2bf6089 100644
>>> --- a/arch/arm/vfp/entry.S
>>> +++ b/arch/arm/vfp/entry.S
>>> @@ -19,6 +19,15 @@
>>> #include <asm/vfpmacros.h>
>>> #include "../kernel/entry-header.S"
>>>
>>> +@ VFP entry point.
>>> +@
>>> +@ r0 = instruction opcode (32-bit ARM or two 16-bit Thumb)
>>> +@ r2 = PC value to resume execution after successful emulation
>>> +@ r9 = normal "successful" return address
>>> +@ r10 = this threads thread_info structure
>>> +@ lr = unrecognised instruction return address
>>> +@ IRQs disabled.
>>> +@
>>> ENTRY(do_vfp)
>>> #ifdef CONFIG_PREEMPT
>>> ldr r4, [r10, #TI_PREEMPT] @ get preempt count
>>> diff --git a/arch/arm/vfp/vfphw.S b/arch/arm/vfp/vfphw.S
>>> index 9897dcf..7292921 100644
>>> --- a/arch/arm/vfp/vfphw.S
>>> +++ b/arch/arm/vfp/vfphw.S
>>> @@ -61,13 +61,13 @@
>>>
>>> @ VFP hardware support entry point.
>>> @
>>> -@ r0 = faulted instruction
>>> -@ r2 = faulted PC+4
>>> -@ r9 = successful return
>>> +@ r0 = instruction opcode (32-bit ARM or two 16-bit Thumb)
>>> +@ r2 = PC value to resume execution after successful emulation
>>> +@ r9 = normal "successful" return address
>>> @ r10 = vfp_state union
>>> @ r11 = CPU number
>>> -@ lr = failure return
>>> -
>>> +@ lr = unrecognised instruction return address
>>> +@ IRQs enabled.
>>> ENTRY(vfp_support_entry)
>>> DBGSTR3 "instr %08x pc %08x state %p", r0, r2, r10
>>
>> I tested copying r2 to regs->ARM_pc like this patch does, and it fixes
>> my test case. Could this second patch go first so it can be applied
>> to stable?
>>
>
> Also, both patches together Tested-by: Colin Cross <ccross@xxxxxxxxxxx>
>
I think there is an additional change needed to
__und_usr_unknown/do_undefinstr. do_undefinstr, which gets called
directly in __und_usr as well as by mov pc, lr, expects regs->ARM_pc
to be the fault address, and not the next PC, and gets called for 2 or
4 byte instructions. If it gets called after the regs->ARM_pc fixup
added in the second patch, it will return to the middle of the
faulting instruction.
If it do_undefinstr called on a 4 byte thumb instruction, with
regs->ARM_pc pointing to the end of the faulting instruction, there is
no good way to figure out where the PC of the instruction that faulted
is. Would it make more sense to convert all the exception handlers to
get regs->ARM_pc as the PC of the faulting instruction, rather than
getting the PC of the next instruction? That way, each handler can
easily use thumb_mode(regs) and decoding the instruction to determine
how big the instruction is, and decide whether to return to the
beginning or end of the faulting instruction.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/