From patchwork Thu Apr 7 11:34:37 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Li Bin X-Patchwork-Id: 8771281 Return-Path: X-Original-To: patchwork-linux-arm@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork1.web.kernel.org (Postfix) with ESMTP id 520FD9F36E for ; Thu, 7 Apr 2016 11:44:33 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 2D972201CD for ; Thu, 7 Apr 2016 11:44:32 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.9]) (using TLSv1.2 with cipher AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 08381201BB for ; Thu, 7 Apr 2016 11:44:31 +0000 (UTC) Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.80.1 #2 (Red Hat Linux)) id 1ao8Kj-0002aQ-Qe; Thu, 07 Apr 2016 11:42:45 +0000 Received: from szxga01-in.huawei.com ([58.251.152.64]) by bombadil.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1ao8Kf-0002Vo-6n for linux-arm-kernel@lists.infradead.org; Thu, 07 Apr 2016 11:42:43 +0000 Received: from 172.24.1.48 (EHLO szxeml432-hub.china.huawei.com) ([172.24.1.48]) by szxrg01-dlp.huawei.com (MOS 4.3.7-GA FastPath queued) with ESMTP id DIH26705; Thu, 07 Apr 2016 19:35:00 +0800 (CST) Received: from [127.0.0.1] (10.177.23.78) by szxeml432-hub.china.huawei.com (10.82.67.209) with Microsoft SMTP Server id 14.3.235.1; Thu, 7 Apr 2016 19:34:48 +0800 Subject: Re: [PATCH 2/2] arm64: Fix watchpoint recursion when single-step is wrongly triggered in irq To: Pratyush Anand References: <1458549470-124791-1-git-send-email-hekuang@huawei.com> <1458549470-124791-2-git-send-email-hekuang@huawei.com> <20160321102423.GB15150@dhcppc6.redhat.com> <56FD1BD1.7070101@huawei.com> <20160404051714.GH28435@dhcppc0.redhat.com> From: Li Bin Message-ID: <570645CD.6030400@huawei.com> Date: Thu, 7 Apr 2016 19:34:37 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Thunderbird/38.5.1 MIME-Version: 1.0 In-Reply-To: <20160404051714.GH28435@dhcppc0.redhat.com> X-Originating-IP: [10.177.23.78] X-CFilter-Loop: Reflected X-Mirapoint-Virus-RAPID-Raw: score=unknown(0), refid=str=0001.0A020204.570645E7.004B, ss=1, re=0.000, recu=0.000, reip=0.000, cl=1, cld=1, fgs=0, ip=0.0.0.0, so=2013-06-18 04:22:30, dmn=2013-03-21 17:37:32 X-Mirapoint-Loop-Id: 072bba0174c02b63e03ed9cbcfb93434 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20160407_044242_199005_C0C20087 X-CRM114-Status: GOOD ( 21.62 ) X-Spam-Score: -5.2 (-----) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: mark.rutland@arm.com, yang.shi@linaro.org, wangnan0@huawei.com, marc.zyngier@arm.com, catalin.marinas@arm.com, Hanjun Guo , will.deacon@arm.com, He Kuang , linux-kernel@vger.kernel.org, gregkh@linuxfoundation.org, james.morse@arm.com, hanjun.guo@linaro.org, richard@nod.at, Ding Tianhong , Dave.Martin@arm.com, linux-arm-kernel@lists.infradead.org Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org X-Spam-Status: No, score=-5.2 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_MED, RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP Hi Pratyush, on 2016/4/4 13:17, Pratyush Anand wrote: > Hi Li, > > On 31/03/2016:08:45:05 PM, Li Bin wrote: >> Hi Pratyush, >> >> on 2016/3/21 18:24, Pratyush Anand wrote: >>> On 21/03/2016:08:37:50 AM, He Kuang wrote: >>>> On arm64, watchpoint handler enables single-step to bypass the next >>>> instruction for not recursive enter. If an irq is triggered right >>>> after the watchpoint, a single-step will be wrongly triggered in irq >>>> handler, which causes the watchpoint address not stepped over and >>>> system hang. >>> >>> Does patch [1] resolves this issue as well? I hope it should. Patch[1] has still >>> not been sent for review. Your test result will be helpful. >>> >>> ~Pratyush >>> >>> [1] https://github.com/pratyushanand/linux/commit/7623c8099ac22eaa00e7e0f52430f7a4bd154652 >> >> This patch did not consider that, when excetpion return, the singlestep flag >> should be restored, otherwise the right singlestep will not triggered. >> Right? > > Yes, you are right, and there are other problems as well. Will Deacon pointed > out [1] that kernel debugging is per-cpu rather than per-task. So, I did thought > of a per-cpu implementation by introducing a new element "flags" in struct > pt_regs. But even with that I see issues. For example: > - While executing single step instruction, we get a data abort > - In the kernel_entry of data abort we disable single stepping based on "flags" > bit field > - While handling data abort we receive anther interrupt, so we are again in > kernel_entry (for el1_irq). Single stepping will be disabled again (although > it does not matter). > > Now the issue is that, what condition should be verified in kernel_exit for > enabling single step again? In the above scenario, kernel_exit for el1_irq > should not enable single stepping, but how to prevent that elegantly? The condition for kernel_entry to disable the single step is that MDSCR_EL1.SS has been set. And only when the corresponding kernel_entry has disabled the single step, the kernel_exit should enable it, but the kernel_exit of single-step exception should be handled specially, that when disable single step in single-step exception handler, flag of pt_regs stored in stack should be cleard to prevent to be re-enabled by kernel_exit. Thanks, Li Bin --- arch/arm64/include/asm/assembler.h | 11 +++++++++++ arch/arm64/include/asm/debug-monitors.h | 2 +- arch/arm64/include/asm/ptrace.h | 2 ++ arch/arm64/kernel/asm-offsets.c | 1 + arch/arm64/kernel/debug-monitors.c | 3 ++- arch/arm64/kernel/entry.S | 18 ++++++++++++++++++ arch/arm64/kernel/hw_breakpoint.c | 2 +- arch/arm64/kernel/kgdb.c | 2 +- 8 files changed, 37 insertions(+), 4 deletions(-) diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h index 70f7b9e..56a4335 100644 --- a/arch/arm64/include/asm/assembler.h +++ b/arch/arm64/include/asm/assembler.h @@ -60,6 +60,17 @@ msr daifclr, #8 .endm + .macro disable_step, orig + bic \orig, \orig, #1 + msr mdscr_el1, \orig + .endm + + .macro enable_step, orig + disable_dbg + orr \orig, \orig, #1 + msr mdscr_el1, \orig + .endm + .macro disable_step_tsk, flgs, tmp tbz \flgs, #TIF_SINGLESTEP, 9990f mrs \tmp, mdscr_el1 diff --git a/arch/arm64/include/asm/debug-monitors.h b/arch/arm64/include/asm/debug-monitors.h index 2fcb9b7..50f114c 100644 --- a/arch/arm64/include/asm/debug-monitors.h +++ b/arch/arm64/include/asm/debug-monitors.h @@ -115,7 +115,7 @@ void user_rewind_single_step(struct task_struct *task); void user_fastforward_single_step(struct task_struct *task); void kernel_enable_single_step(struct pt_regs *regs); -void kernel_disable_single_step(void); +void kernel_disable_single_step(struct pt_regs *regs); int kernel_active_single_step(void); #ifdef CONFIG_HAVE_HW_BREAKPOINT diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h index a307eb6..da00cd8 100644 --- a/arch/arm64/include/asm/ptrace.h +++ b/arch/arm64/include/asm/ptrace.h @@ -113,6 +113,8 @@ struct pt_regs { u64 sp; u64 pc; u64 pstate; + u64 flags; + u64 padding; }; }; u64 orig_x0; diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c index 3ae6b31..7936ba9 100644 --- a/arch/arm64/kernel/asm-offsets.c +++ b/arch/arm64/kernel/asm-offsets.c @@ -56,6 +56,7 @@ int main(void) DEFINE(S_COMPAT_SP, offsetof(struct pt_regs, compat_sp)); #endif DEFINE(S_PSTATE, offsetof(struct pt_regs, pstate)); + DEFINE(S_FLAGS, offsetof(struct pt_regs, flags)); DEFINE(S_PC, offsetof(struct pt_regs, pc)); DEFINE(S_ORIG_X0, offsetof(struct pt_regs, orig_x0)); DEFINE(S_SYSCALLNO, offsetof(struct pt_regs, syscallno)); diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c index c45f296..9d2bdbf 100644 --- a/arch/arm64/kernel/debug-monitors.c +++ b/arch/arm64/kernel/debug-monitors.c @@ -403,9 +403,10 @@ void kernel_enable_single_step(struct pt_regs *regs) enable_debug_monitors(DBG_ACTIVE_EL1); } -void kernel_disable_single_step(void) +void kernel_disable_single_step(struct pt_regs *regs) { WARN_ON(!irqs_disabled()); + regs->flags = 0; mdscr_write(mdscr_read() & ~DBG_MDSCR_SS); disable_debug_monitors(DBG_ACTIVE_EL1); } diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S index 12e8d2b..753bef2 100644 --- a/arch/arm64/kernel/entry.S +++ b/arch/arm64/kernel/entry.S @@ -87,6 +87,15 @@ stp x26, x27, [sp, #16 * 13] stp x28, x29, [sp, #16 * 14] + .if \el == 1 + mrs x19, mdscr_el1 + and x20, x19, #1 + str x20, [sp, #S_FLAGS] + tbz x20, #0, 1f + disable_step x19 +1: + .endif + .if \el == 0 mrs x21, sp_el0 mov tsk, sp @@ -154,6 +163,15 @@ alternative_endif .endif msr elr_el1, x21 // set up the return data msr spsr_el1, x22 + + .if \el == 1 + ldr x19, [sp, #S_FLAGS] + tbz x19, #0, 1f + mrs x19, mdscr_el1 + enable_step x19 +1: + .endif + ldp x0, x1, [sp, #16 * 0] ldp x2, x3, [sp, #16 * 1] ldp x4, x5, [sp, #16 * 2] diff --git a/arch/arm64/kernel/hw_breakpoint.c b/arch/arm64/kernel/hw_breakpoint.c index b45c95d..6afbb80 100644 --- a/arch/arm64/kernel/hw_breakpoint.c +++ b/arch/arm64/kernel/hw_breakpoint.c @@ -802,7 +802,7 @@ int reinstall_suspended_bps(struct pt_regs *regs) toggle_bp_registers(AARCH64_DBG_REG_WCR, DBG_ACTIVE_EL0, 1); if (*kernel_step != ARM_KERNEL_STEP_SUSPEND) { - kernel_disable_single_step(); + kernel_disable_single_step(regs); handled_exception = 1; } else { handled_exception = 0; diff --git a/arch/arm64/kernel/kgdb.c b/arch/arm64/kernel/kgdb.c index b67531a..f92c5fb 100644 --- a/arch/arm64/kernel/kgdb.c +++ b/arch/arm64/kernel/kgdb.c @@ -183,7 +183,7 @@ int kgdb_arch_handle_exception(int exception_vector, int signo, * Received continue command, disable single step */ if (kernel_active_single_step()) - kernel_disable_single_step(); + kernel_disable_single_step(linux_regs); err = 0; break;