lkml.org 
[lkml]   [2011]   [Jan]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH] ARM: vfp: Fix up exception location in Thumb mode
    From
    On Fri, Jan 14, 2011 at 11:51 AM, Colin Cross <ccross@android.com> wrote:
    > On Fri, Jan 14, 2011 at 11:23 AM, Colin Cross <ccross@android.com> wrote:
    >> On Fri, Jan 14, 2011 at 10:47 AM, Russell King - ARM Linux
    >> <linux@arm.linux.org.uk> 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@android.com>
    >

    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@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2011-01-26 00:35    [W:3.558 / U:0.192 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site