From: Jed Davis <jld@mozilla.com> To: Will Deacon <will.deacon@arm.com> Cc: Russell King <linux@arm.linux.org.uk>, Peter Zijlstra <a.p.zijlstra@chello.nl>, Paul Mackerras <paulus@samba.org>, Ingo Molnar <mingo@redhat.com>, Arnaldo Carvalho de Melo <acme@ghostprotocols.net>, Robert Richter <rric@kernel.org>, "linux-arm-kernel@lists.infradead.org" <linux-arm-kernel@lists.infradead.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "oprofile-list@lists.sf.net" <oprofile-list@lists.sf.net> Subject: Re: [PATCH] ARM: Fix r7/r11 confusion when CONFIG_THUMB2_KERNEL=y Date: Fri, 19 Jul 2013 21:46:55 -0700 [thread overview] Message-ID: <20130720044655.GC9433@mozilla.com> (raw) In-Reply-To: <20130715135420.GG10000@mudshark.cambridge.arm.com> On Mon, Jul 15, 2013 at 02:54:20PM +0100, Will Deacon wrote: > On Sat, Jul 13, 2013 at 04:18:20AM +0100, Jed Davis wrote: [...] > > Effects of this are probably limited to failure of EHABI unwinding when > > starting from a function that uses r7 to restore its stack pointer, but > > the possibility for further breakage (which would be invisible on > > non-Thumb kernels) is worrying. [...] > I'm struggling to understand exactly the problem that this patch is trying > to address. If it's just a code consistency issue, I don't think it's worth > it (I actually find it less confusing the way we currently have things) but > if there is a real bug, perhaps you could provide a testcase? There is a real bug here, but my commit message wasn't very clear. This was breaking PERF_COUNT_SW_CONTEXT_SWITCHES with CONFIG_THUMB2_KERNEL=y (with my other recently posted patch applied), because kernel/sched.c is built with -fno-omit-frame-pointer (which is wrong, but that's a problem for another patch) and so __schedule's EHABI entry uses 0x97 (mov sp, r7), and somewhere along the line the unwinder gets the r11 value instead. This would also apply to any function with a variable-length array, but __schedule happens to have the perf hook I was trying to use. I should add that this bug doesn't affect me directly at the moment, because we're not currently using CONFIG_THUMB2_KERNEL on Firefox OS, because our kernels are assorted older versions with hardware vendors' changes and there are some issues with it. But I felt it was worth tracking this down before trying to send changes upstream. The "right" thing to do here might be to just include all the registers, or at least {r4-pc}, in struct stackframe. The parts that aren't {fp, sp, lr, pc} could be ifdef'ed if we're concerned enough about the overhead in kernels using APCS frame pointer unwinding. I agree that a test case would be good -- I'm more than a little worried of regressions without one -- but I could use some advice on how best to do that. --Jed
WARNING: multiple messages have this Message-ID (diff)
From: jld@mozilla.com (Jed Davis) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH] ARM: Fix r7/r11 confusion when CONFIG_THUMB2_KERNEL=y Date: Fri, 19 Jul 2013 21:46:55 -0700 [thread overview] Message-ID: <20130720044655.GC9433@mozilla.com> (raw) In-Reply-To: <20130715135420.GG10000@mudshark.cambridge.arm.com> On Mon, Jul 15, 2013 at 02:54:20PM +0100, Will Deacon wrote: > On Sat, Jul 13, 2013 at 04:18:20AM +0100, Jed Davis wrote: [...] > > Effects of this are probably limited to failure of EHABI unwinding when > > starting from a function that uses r7 to restore its stack pointer, but > > the possibility for further breakage (which would be invisible on > > non-Thumb kernels) is worrying. [...] > I'm struggling to understand exactly the problem that this patch is trying > to address. If it's just a code consistency issue, I don't think it's worth > it (I actually find it less confusing the way we currently have things) but > if there is a real bug, perhaps you could provide a testcase? There is a real bug here, but my commit message wasn't very clear. This was breaking PERF_COUNT_SW_CONTEXT_SWITCHES with CONFIG_THUMB2_KERNEL=y (with my other recently posted patch applied), because kernel/sched.c is built with -fno-omit-frame-pointer (which is wrong, but that's a problem for another patch) and so __schedule's EHABI entry uses 0x97 (mov sp, r7), and somewhere along the line the unwinder gets the r11 value instead. This would also apply to any function with a variable-length array, but __schedule happens to have the perf hook I was trying to use. I should add that this bug doesn't affect me directly at the moment, because we're not currently using CONFIG_THUMB2_KERNEL on Firefox OS, because our kernels are assorted older versions with hardware vendors' changes and there are some issues with it. But I felt it was worth tracking this down before trying to send changes upstream. The "right" thing to do here might be to just include all the registers, or at least {r4-pc}, in struct stackframe. The parts that aren't {fp, sp, lr, pc} could be ifdef'ed if we're concerned enough about the overhead in kernels using APCS frame pointer unwinding. I agree that a test case would be good -- I'm more than a little worried of regressions without one -- but I could use some advice on how best to do that. --Jed
next prev parent reply other threads:[~2013-07-20 4:46 UTC|newest] Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top 2013-07-13 3:18 [PATCH] ARM: Fix r7/r11 confusion when CONFIG_THUMB2_KERNEL=y Jed Davis 2013-07-13 3:18 ` Jed Davis 2013-07-15 13:54 ` Will Deacon 2013-07-15 13:54 ` Will Deacon 2013-07-20 4:46 ` Jed Davis [this message] 2013-07-20 4:46 ` Jed Davis 2013-07-21 21:37 ` Will Deacon 2013-07-21 21:37 ` Will Deacon 2013-07-22 13:56 ` Robert Richter 2013-07-22 13:56 ` Robert Richter 2013-07-22 18:52 ` Dave Martin 2013-07-22 18:52 ` Dave Martin 2013-07-29 21:21 ` Jed Davis 2013-07-29 21:21 ` Jed Davis 2013-07-30 9:25 ` Dave Martin 2013-07-30 9:25 ` Dave Martin 2013-07-30 9:38 ` [PATCH] ARM: Fix r7/r11 confusion when CONFIG_THUMB2_KERNEL=y [OT] Jean-Francois Moine 2013-07-30 9:38 ` Jean-Francois Moine 2013-07-30 9:44 ` Dave Martin 2013-07-30 9:44 ` Dave Martin 2013-07-30 10:09 ` Jean-Francois Moine 2013-07-30 10:09 ` Jean-Francois Moine 2013-07-30 11:46 ` Dave Martin 2013-07-30 11:46 ` Dave Martin 2013-07-30 17:50 ` Christopher Covington 2013-07-30 17:50 ` Christopher Covington 2013-07-30 9:49 ` Will Deacon 2013-07-30 9:49 ` Will Deacon 2013-07-31 9:03 ` Jean-Francois Moine 2013-07-31 9:03 ` Jean-Francois Moine 2013-07-31 10:38 ` Will Deacon 2013-07-31 10:38 ` Will Deacon 2014-01-06 9:54 ` walimis 2014-01-06 9:54 ` walimis
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20130720044655.GC9433@mozilla.com \ --to=jld@mozilla.com \ --cc=a.p.zijlstra@chello.nl \ --cc=acme@ghostprotocols.net \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux@arm.linux.org.uk \ --cc=mingo@redhat.com \ --cc=oprofile-list@lists.sf.net \ --cc=paulus@samba.org \ --cc=rric@kernel.org \ --cc=will.deacon@arm.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.