Re: [PATCH] ARM: vfp: Fix up exception location in Thumb mode

[Catalin Marinas]

On 14 January 2011 17:30, Russell King - ARM Linux
<linux@xxxxxxxxxxxxxxxx> wrote:
> On Fri, Jan 14, 2011 at 04:58:47PM +0000, Catalin Marinas wrote:
>> I agree, this code needs some clean-up. Maybe for Undef we could unify
>> the ARM and Thumb-2 offsets so that they are both 4 (it may confuse the
>> breakpoint code, I haven't checked).
>>
>> Otherwise just let the code handling the undef deal with the ARM/Thumb
>> difference. For SVC, it makes sense to have different offsets as we
>> always return to the next instruction.
[...]
> When the VFP support code tests the state of the VFP hardware during boot,
> it sets the VFP handler to point at vfp_testing_entry, bypassing the normal
> VFP handling code, and executes a VFP instruction.
>
> If this VFP instruction faults (eg, because there is no VFP hardware
> present or we're not permitted to use it), it could end up resuming
> execution in the middle of the 16-bit paired instruction because
> regs->ARM_pc points in the middle of it.

Yes, that's possible. We probably never tried a Thumb-2 kernel where
VFP isn't present.

> Or maybe we should just make it unconditional that whenever we have an
> undefined instruction exception, the regs->ARM_pc value will always be
> set for resuming execution after the faulted instruction. ÂThat makes
> it consistent with r2 throughout the code in every case.

I have some comments below.

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

The itttt above should just be itt. The reveq is conditionally
compiled and beq doesn't necessarily need one.

> Â#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)

Is r2 here always the PC value following instruction? If the Thumb
instruction was 32-bit, it just points in the middle of the faulting
instruction.

> + Â Â Â @ 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)

That's correct.

> + Â Â Â @ r4 = PC value for the first 16-bit Thumb instruction

I think r4 here points in the middle of tha faulting instruction for
32-bit Thumb.

> + Â Â Â @
> Â#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,14 @@
> Â#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
> +@
> Â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

That's right.

> +@ Â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
>
> @@ -138,9 +138,12 @@ check_for_exception:
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â@ exception before retrying branch
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â@ out before setting an FPEXC that
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â@ stops us reading stuff
> - Â Â Â VFPFMXR FPEXC, r1 Â Â Â Â Â Â Â @ restore FPEXC last
> -    sub   r2, r2, #4
> -    str   r2, [sp, #S_PC]     @ retry the instruction
> + Â Â Â VFPFMXR FPEXC, r1 Â Â Â Â Â Â Â @ Restore FPEXC last
> +    sub   r2, r2, #4       Â@ Retry current instruction - if Thumb
> +    str   r2, [sp, #S_PC]     @ mode it's two 16-bit instructions,
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â @ else it's one 32-bit instruction, so
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â @ always subtract 4 from the following
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â @ instruction address.

I would say it's always a 32-bit instruction but made up of two 16-bit
values to allow half-word alignment.

-- 
Catalin
--
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/