| Message ID | 1348160260-19486-1-git-send-email-sboyd@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Stephen, On Thu, Sep 20, 2012 at 05:57:40PM +0100, Stephen Boyd wrote: > The reset value of the BCR, BVR, WCR, and WVR registers are all > UNKNOWN on ARMv7. Unfortunately, reset_ctrl_regs() clears these > registers *after* enabling monitor mode, not before, and so some > implementations may experience UNPREDICTABLE behavior if the > reset values of these registers are non-zero. Clear the > breakpoints before enabling monitor mode so that we don't > experience boot hangs/loops due to breakpoints being enabled > out of reset. > > Signed-off-by: Stephen Boyd <sboyd@codeaurora.org> Aha, thanks for the patch. We should definitely zero these registers before enabling monitor mode. However... > +/* Determine if halting mode is enabled */ > +static int halting_mode_enabled(void) > +{ > + u32 dscr; > + > + ARM_DBG_READ(c1, 0, dscr); > + > + if (WARN_ONCE(dscr & ARM_DSCR_HDBGEN, > + "halting debug mode enabled. Unable to access hardware resources.\n")) { > + return -EPERM; > + } > + return 0; > +} ...it looks like debug arch 7.1 defines this bit as UNKNOWN when the OS lock is clear, so we probably shouldn't be reading it at all. I'll pour myself a stiff drink and start reading the debug arch docs to work out what on Earth we should do. Stay tuned. Will
On Thu, Sep 20, 2012 at 06:35:56PM +0100, Will Deacon wrote: > On Thu, Sep 20, 2012 at 05:57:40PM +0100, Stephen Boyd wrote: > > +/* Determine if halting mode is enabled */ > > +static int halting_mode_enabled(void) > > +{ > > + u32 dscr; > > + > > + ARM_DBG_READ(c1, 0, dscr); > > + > > + if (WARN_ONCE(dscr & ARM_DSCR_HDBGEN, > > + "halting debug mode enabled. Unable to access hardware resources.\n")) { > > + return -EPERM; > > + } > > + return 0; > > +} > > ...it looks like debug arch 7.1 defines this bit as UNKNOWN when the OS lock > is clear, so we probably shouldn't be reading it at all. I'll pour myself a > stiff drink and start reading the debug arch docs to work out what on Earth > we should do. > > Stay tuned. Ok, I've pushed a bunch of patches to my hw-breakpoint branch (head commit 55cb726797c7). I'll post them to the list after the merge window, but please do take them for a spin if you get a chance. Will
On 09/24/12 10:19, Will Deacon wrote: > On Thu, Sep 20, 2012 at 06:35:56PM +0100, Will Deacon wrote: >> On Thu, Sep 20, 2012 at 05:57:40PM +0100, Stephen Boyd wrote: >>> +/* Determine if halting mode is enabled */ >>> +static int halting_mode_enabled(void) >>> +{ >>> + u32 dscr; >>> + >>> + ARM_DBG_READ(c1, 0, dscr); >>> + >>> + if (WARN_ONCE(dscr & ARM_DSCR_HDBGEN, >>> + "halting debug mode enabled. Unable to access hardware resources.\n")) { >>> + return -EPERM; >>> + } >>> + return 0; >>> +} >> ...it looks like debug arch 7.1 defines this bit as UNKNOWN when the OS lock >> is clear, so we probably shouldn't be reading it at all. I'll pour myself a >> stiff drink and start reading the debug arch docs to work out what on Earth >> we should do. >> >> Stay tuned. > Ok, I've pushed a bunch of patches to my hw-breakpoint branch (head commit > 55cb726797c7). I'll post them to the list after the merge window, but please > do take them for a spin if you get a chance. > Sure, I'll try them later today. I would say just send them out so people can add tested and reviewed tags. Otherwise we should all start planning week long vacations every 7 to 8 weeks to coincide with the merge window. Also, it would be nice if we could fix this in 3.7 or even 3.6. Booting on an MSM8660 is very fragile right now and this patch fixes it for me.
On Mon, Sep 24, 2012 at 07:04:56PM +0100, Stephen Boyd wrote: > On 09/24/12 10:19, Will Deacon wrote: > > Ok, I've pushed a bunch of patches to my hw-breakpoint branch (head commit > > 55cb726797c7). I'll post them to the list after the merge window, but please > > do take them for a spin if you get a chance. > > > > Sure, I'll try them later today. I would say just send them out so > people can add tested and reviewed tags. Otherwise we should all start > planning week long vacations every 7 to 8 weeks to coincide with the > merge window. I like the idea of that! However, it's more that most people are worrying about getting their trees into shape rather than reviewing new code at the moment, so I suspect that posting it now will go largely un-noticed and I don't think this is a critical fix (see below). > Also, it would be nice if we could fix this in 3.7 or even 3.6. Booting > on an MSM8660 is very fragile right now and this patch fixes it for me. Whilst I appreciate that it solves your problem, I'm *really* wary about changing this stuff without giving it a thorough airing beforehand. The debug reset path is extremely error-prone and its behaviour is tangled up with the state of various external signals into the core, meaning that different platforms with the same CPU can take different paths at runtime. I can definitely CC stable once we're happy with this, but I'd rather avoid rushing the patches in before 3.8. Cheers, Will
On 09/24/12 10:19, Will Deacon wrote: > Ok, I've pushed a bunch of patches to my hw-breakpoint branch (head commit > 55cb726797c7). I'll post them to the list after the merge window, but please > do take them for a spin if you get a chance. > Forgot to reply here. Took them for a spin last week with that commit. No more boot hangs but I can't say much else beyond that. When you post to the list I'll add my tested-by.
On Tue, Oct 02, 2012 at 01:34:28AM +0100, Stephen Boyd wrote: > On 09/24/12 10:19, Will Deacon wrote: > > Ok, I've pushed a bunch of patches to my hw-breakpoint branch (head commit > > 55cb726797c7). I'll post them to the list after the merge window, but please > > do take them for a spin if you get a chance. > > > > Forgot to reply here. Took them for a spin last week with that commit. > No more boot hangs but I can't say much else beyond that. When you post > to the list I'll add my tested-by. Thanks Stephen. I've also got some patches for OS save/restore of the debug registers using the various hardware locking and readout mechanisms, so that debug state can be persisted across low-power states. Do any of the Qualcomm chips (v7 as opposed to v7.1) implement the save/restore registers? Will
On 10/02/12 02:13, Will Deacon wrote: > > Thanks Stephen. I've also got some patches for OS save/restore of the debug > registers using the various hardware locking and readout mechanisms, so that > debug state can be persisted across low-power states. > > Do any of the Qualcomm chips (v7 as opposed to v7.1) implement the save/restore > registers? Yes, we have save and restore registers.
diff --git a/arch/arm/kernel/hw_breakpoint.c b/arch/arm/kernel/hw_breakpoint.c index ba386bd..9eac571 100644 --- a/arch/arm/kernel/hw_breakpoint.c +++ b/arch/arm/kernel/hw_breakpoint.c @@ -216,6 +216,20 @@ static int get_num_brps(void) return core_has_mismatch_brps() ? brps - 1 : brps; } +/* Determine if halting mode is enabled */ +static int halting_mode_enabled(void) +{ + u32 dscr; + + ARM_DBG_READ(c1, 0, dscr); + + if (WARN_ONCE(dscr & ARM_DSCR_HDBGEN, + "halting debug mode enabled. Unable to access hardware resources.\n")) { + return -EPERM; + } + return 0; +} + /* * In order to access the breakpoint/watchpoint control registers, * we must be running in debug monitor mode. Unfortunately, we can @@ -225,16 +239,13 @@ static int get_num_brps(void) static int enable_monitor_mode(void) { u32 dscr; - int ret = 0; + int ret; ARM_DBG_READ(c1, 0, dscr); /* Ensure that halting mode is disabled. */ - if (WARN_ONCE(dscr & ARM_DSCR_HDBGEN, - "halting debug mode enabled. Unable to access hardware resources.\n")) { - ret = -EPERM; + if ((ret = halting_mode_enabled())) goto out; - } /* If monitor mode is already enabled, just return. */ if (dscr & ARM_DSCR_MDBGEN) @@ -935,7 +946,7 @@ static void reset_ctrl_regs(void *unused) isb(); reset_regs: - if (enable_monitor_mode()) + if (halting_mode_enabled()) return; /* We must also reset any reserved registers. */ @@ -949,6 +960,7 @@ reset_regs: write_wb_reg(ARM_BASE_WCR + i, 0UL); write_wb_reg(ARM_BASE_WVR + i, 0UL); } + enable_monitor_mode(); } static int __cpuinit dbg_reset_notify(struct notifier_block *self,
The reset value of the BCR, BVR, WCR, and WVR registers are all UNKNOWN on ARMv7. Unfortunately, reset_ctrl_regs() clears these registers *after* enabling monitor mode, not before, and so some implementations may experience UNPREDICTABLE behavior if the reset values of these registers are non-zero. Clear the breakpoints before enabling monitor mode so that we don't experience boot hangs/loops due to breakpoints being enabled out of reset. Signed-off-by: Stephen Boyd <sboyd@codeaurora.org> --- arch/arm/kernel/hw_breakpoint.c | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-)