| Message ID | 53BE8817.10703@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 07/10/2014 02:33 PM, Sebastian Hesselbarth wrote: > On 07/08/2014 05:17 PM, Jason Cooper wrote: >> On Sun, Jul 06, 2014 at 08:08:45AM +0200, Jean-Francois Moine wrote: >>> Since the official 3.15.0 release, the kernel crashes at boot time >>> when compiled with the option CONFIG_PREEMPT. >>> >>> Reverting the commit 431a84b1a4f7d1a0085d5b91330c5053cc8e8b12 >>> >>> ARM: 8034/1: Disable preemption in iwmmxt_task_enable() >>> >>> removes the problem. >>> >>> Linux version 3.16.0-rc3-00062-gd92a333-dirty (jef@armhf) (gcc >>> version 4.8.3 (Debian 4.8.3-4) ) #5 PREEMPT Thu Jul 3 19:46:39 CEST 2014 > [...] >>> PJ4 iWMMXt v2 coprocessor enabled. > [...] >>> Unable to handle kernel paging request at virtual address fffffffe >>> pgd = bb25c000 >>> [fffffffe] *pgd=3bfde821, *pte=00000000, *ppte=00000000 >>> Internal error: Oops: 80000007 [#1] PREEMPT ARM >>> Modules linked in: >>> CPU: 0 PID: 62 Comm: startpar Not tainted >>> 3.16.0-rc3-00062-gd92a333-dirty #5 >>> task: bb230b80 ti: bb256000 task.ti: bb256000 >>> PC is at 0xfffffffe >>> LR is at iwmmxt_task_copy+0x44/0x4c >>> pc : [<fffffffe>] lr : [<800130ac>] psr: 40000033 >>> sp : bb257de8 ip : 00000013 fp : bb257ea4 >>> r10: bb256000 r9 : fffffdfe r8 : 76e898e6 >>> r7 : bb257ec8 r6 : bb256000 r5 : 7ea12760 r4 : 000000a0 >>> r3 : ffffffff r2 : 00000003 r1 : bb257df8 r0 : 00000000 >>> Flags: nZcv IRQs on FIQs on Mode SVC_32 ISA Thumb Segment user >>> Control: 10c5387d Table: 3b25c019 DAC: 00000015 >>> Process startpar (pid: 62, stack limit = 0xbb256248) > > Ok, I have been able to debug this despite my limited knowledge of > iWMMXt and ARM asm. While the patch below fixes the issue, I have > no clue if it is the right approach or if there should be a different > solution. I'd like to leave that to either Russell or Catalin to decide. After thinking a while about it and because I missed it to mention: I did a bisect which ends in commit 1fb333489fb917c704ad43e51b45c12f52215a9c ("Merge branches 'alignment', 'fixes', 'l2c' (early part) and 'misc' into for-next") which clearly isn't the offending commit itself but finally causing iwmmxt code to show the issue. I compared introduced {inc,dec}_preempt_count macros .macro inc_preempt_count, ti, tmp ldr \tmp, [\ti, #TI_PREEMPT] @ get preempt count add \tmp, \tmp, #1 @ increment it str \tmp, [\ti, #TI_PREEMPT] .endm .macro dec_preempt_count, ti, tmp ldr \tmp, [\ti, #TI_PREEMPT] @ get preempt count sub \tmp, \tmp, #1 @ decrement it str \tmp, [\ti, #TI_PREEMPT] .endm with common C defines for preempt_{disable,enable} #define preempt_disable() \ do { \ preempt_count_inc(); \ barrier(); \ } while (0) #define preempt_enable() \ do { \ barrier(); \ preempt_count_dec(); \ } while (0) and wondered about the missing barriers. The thing about iwmmxt.S is that it is assembled with -mcpu=iwmmxt causing the assembler to drop down to xscale instructions (?) which don't allow any atomic operations. Anyway, I may be wrong about it. At least I wanted to mention that bisect ends in above merge commit of l2c related cleanup and not the iwmmxt preempt commit itself. Sebastian > If anything in below explanation is wrong, please correct me > immediately! > > Above mentioned commit basically added {inc,dec}_preempt_count macros > to iwmmxt_task_enable to run it with preemption disabled: > > ENTRY(iwmmxt_task_enable) > + inc_preempt_count r10, r3 > [...] > concan_save: > [...] > concan_dump: > [...] > concan_load: > [...] > +3: > +#ifdef CONFIG_PREEMPT_COUNT > + get_thread_info r10 > +#endif > +4: dec_preempt_count r10, r3 > mov pc, lr > > Unfortunately, other procedures in iwmmxt.S, e.g. iwmmxt_task_copy, > also branch to above concan_{save,dump,load} labels without disabling > preemption first: > > ENTRY(iwmmxt_task_copy) > [...] > 1: @ this task owns Concan regs -- grab a copy from there > mov r0, #0 @ nothing to load > mov r2, #3 @ save all regs > mov r3, lr @ preserve return address > bl concan_dump > msr cpsr_c, ip @ restore interrupt mode > mov pc, r3 > > This causes two issues that finally lead to observed behavior: > (a) introduced {inc,dec}_preempt_count use r3 as temporary register, > while iwmmxt_task_copy uses it to store its return address > (b) branching to concan_foo labels decrements preempt_count without > incrementing it first > > The patch below addresses (a) by using r4 as temporary register for > {inc,dec}_preempt_count macro and (b) by moving concan_foo into > separate code sections and call them from iwmmxt_task_enable like > the other procedures do.
On Thu, Jul 10, 2014 at 10:55:07PM +0200, Sebastian Hesselbarth wrote: > On 07/10/2014 02:33 PM, Sebastian Hesselbarth wrote: > > Ok, I have been able to debug this despite my limited knowledge of > > iWMMXt and ARM asm. While the patch below fixes the issue, I have > > no clue if it is the right approach or if there should be a different > > solution. I'd like to leave that to either Russell or Catalin to decide. > > After thinking a while about it and because I missed it to mention: > > I did a bisect which ends in > > commit 1fb333489fb917c704ad43e51b45c12f52215a9c > ("Merge branches 'alignment', 'fixes', 'l2c' (early part) and 'misc' > into for-next") > > which clearly isn't the offending commit itself but finally causing > iwmmxt code to show the issue. > > I compared introduced {inc,dec}_preempt_count macros > > .macro inc_preempt_count, ti, tmp > ldr \tmp, [\ti, #TI_PREEMPT] @ get preempt count > add \tmp, \tmp, #1 @ increment it > str \tmp, [\ti, #TI_PREEMPT] > .endm > > .macro dec_preempt_count, ti, tmp > ldr \tmp, [\ti, #TI_PREEMPT] @ get preempt count > sub \tmp, \tmp, #1 @ decrement it > str \tmp, [\ti, #TI_PREEMPT] > .endm > > with common C defines for preempt_{disable,enable} > > #define preempt_disable() \ > do { \ > preempt_count_inc(); \ > barrier(); \ > } while (0) > > #define preempt_enable() \ > do { \ > barrier(); \ > preempt_count_dec(); \ > } while (0) > > and wondered about the missing barriers. The barriers there are just compiler barriers - they're there to prevent the compiler moving stores across the change in preempt count. The assembly is safe because we're in control of when and how we insert the explicit data accesses. > Anyway, I may be wrong about it. At least I wanted to mention that > bisect ends in above merge commit of l2c related cleanup and not > the iwmmxt preempt commit itself. The iwmmxt preempt commit is definitely incorrect, and your analysis of the dump based on Jean-Francois's report is spot on, and your solution looks pretty much like a good solution.
diff --git a/arch/arm/kernel/iwmmxt.S b/arch/arm/kernel/iwmmxt.S index a5599cf..9fa671a 100644 --- a/arch/arm/kernel/iwmmxt.S +++ b/arch/arm/kernel/iwmmxt.S @@ -70,7 +70,7 @@ */ ENTRY(iwmmxt_task_enable) - inc_preempt_count r10, r3 + inc_preempt_count r10, r4 XSC(mrc p15, 0, r2, c15, c1, 0) PJ4(mrc p15, 0, r2, c1, c0, 2) @@ -95,10 +95,18 @@ ENTRY(iwmmxt_task_enable) mrc p15, 0, r2, c2, c0, 0 mov r2, r2 @ cpwait - teq r1, #0 @ test for last ownership - mov lr, r9 @ normal exit from exception - beq concan_load @ no owner, skip save - + mov r4, r1 + teq r4, #0 @ test for last ownership + bleq concan_load @ no owner, skip save + teq r4, #0 @ test for last ownership + blne concan_save + +#ifdef CONFIG_PREEMPT_COUNT + get_thread_info r10 +#endif +4: dec_preempt_count r10, r4 + mov pc, r9 @ normal exit from exception + concan_save: tmrc r2, wCon @@ -175,10 +183,6 @@ concan_load: tmcr wCon, r2 3: -#ifdef CONFIG_PREEMPT_COUNT - get_thread_info r10 -#endif -4: dec_preempt_count r10, r3 mov pc, lr /*