From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58955) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZcwJp-0000PB-Bi for qemu-devel@nongnu.org; Fri, 18 Sep 2015 10:07:18 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZcwJl-0006JJ-Vv for qemu-devel@nongnu.org; Fri, 18 Sep 2015 10:07:17 -0400 Received: from mail-lb0-x22b.google.com ([2a00:1450:4010:c04::22b]:35843) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZcwJl-0006IC-C5 for qemu-devel@nongnu.org; Fri, 18 Sep 2015 10:07:13 -0400 Received: by lbcao8 with SMTP id ao8so25676724lbc.3 for ; Fri, 18 Sep 2015 07:07:12 -0700 (PDT) References: <1442227888-11467-1-git-send-email-serge.fdrv@gmail.com> From: Sergey Fedorov Message-ID: <55FC1A8E.1070401@gmail.com> Date: Fri, 18 Sep 2015 17:07:10 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] target-arm: fix CPU breakpoint handling List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: QEMU Developers On 18.09.2015 16:50, Peter Maydell wrote: > On 14 September 2015 at 11:51, Sergey Fedorov wrote: >> A QEMU breakpoint match is not definitely an architectural breakpoint >> match. If an exception is generated unconditionally during translation, >> it is hardly possible to ignore it in the debug exceptoin hanlder. > "exception". Will fix it, thanks. > >> Generate a call to helper to check CPU breakpoints and raise an >> exception only if any breakpoint architecturally matches. >> >> Signed-off-by: Sergey Fedorov >> --- >> target-arm/helper.h | 2 ++ >> target-arm/op_helper.c | 20 +++++++++++++++++++- >> target-arm/translate-a64.c | 12 +++++++----- >> target-arm/translate.c | 12 +++++++----- >> 4 files changed, 35 insertions(+), 11 deletions(-) >> >> diff --git a/target-arm/helper.h b/target-arm/helper.h >> index 827b33d..c2a85c7 100644 >> --- a/target-arm/helper.h >> +++ b/target-arm/helper.h >> @@ -54,6 +54,8 @@ DEF_HELPER_1(yield, void, env) >> DEF_HELPER_1(pre_hvc, void, env) >> DEF_HELPER_2(pre_smc, void, env, i32) >> >> +DEF_HELPER_1(check_breakpoints, void, env) >> + >> DEF_HELPER_3(cpsr_write, void, env, i32, i32) >> DEF_HELPER_1(cpsr_read, i32, env) >> >> diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c >> index b298e57..24dcefd 100644 >> --- a/target-arm/op_helper.c >> +++ b/target-arm/op_helper.c >> @@ -877,6 +877,15 @@ bool arm_debug_check_watchpoint(CPUState *cs) >> return check_watchpoints(cpu); >> } >> >> +void HELPER(check_breakpoints)(CPUARMState *env) >> +{ >> + ARMCPU *cpu = arm_env_get_cpu(env); >> + >> + if (check_breakpoints(cpu)) { >> + HELPER(exception_internal(env, EXCP_DEBUG)); >> + } >> +} >> + >> void arm_debug_excp_handler(CPUState *cs) >> { >> /* Called by core code when a watchpoint or breakpoint fires; >> @@ -904,7 +913,16 @@ void arm_debug_excp_handler(CPUState *cs) >> arm_debug_target_el(env)); >> } >> } else { >> - if (check_breakpoints(cpu)) { >> + CPUBreakpoint *bp; >> + uint64_t pc = is_a64(env) ? env->pc : env->regs[15]; >> + >> + QTAILQ_FOREACH(bp, &cs->breakpoints, entry) { >> + if (bp->pc == pc && !(bp->flags & BP_CPU)) { >> + return; >> + } >> + } > This extra code looks right, but isn't it fixing a different bug? You are right, it would better come to separate patch. > >> + >> + { > Rather than adding the extra block I would just de-indent the code > that used to live inside the if() and move the variable declaration > to the top of the new block. Okay, I will adjust the code. > >> bool same_el = (arm_debug_target_el(env) == arm_current_el(env)); >> if (extended_addresses_enabled(env)) { >> env->exception.fsr = (1 << 9) | 0x22; >> diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c >> index 5c13e15..a5927fd 100644 >> --- a/target-arm/translate-a64.c >> +++ b/target-arm/translate-a64.c >> @@ -11000,11 +11000,13 @@ void gen_intermediate_code_internal_a64(ARMCPU *cpu, >> if (unlikely(!QTAILQ_EMPTY(&cs->breakpoints))) { >> QTAILQ_FOREACH(bp, &cs->breakpoints, entry) { >> if (bp->pc == dc->pc) { >> - gen_exception_internal_insn(dc, 0, EXCP_DEBUG); >> - /* Advance PC so that clearing the breakpoint will >> - invalidate this TB. */ >> - dc->pc += 2; >> - goto done_generating; >> + if (bp->flags & BP_CPU) { >> + gen_helper_check_breakpoints(cpu_env); >> + break; >> + } else { >> + gen_exception_internal_insn(dc, 0, EXCP_DEBUG); >> + goto done_generating; >> + } > You seem to have dropped the "advance the PC" code -- why is that ok? > I also dropped the immediately following goto statement. With these changes PC is advanced in the same way as it happens during normal translation. That is because we actually have to do the instruction translation process here to support the case when a breakpoint with matching PC is architecturally mismatched. As I understand, that "advance the PC" code was necessary to produce a TB with non-zero size so that it can be invalidated later when we clear the breakpoint. Best, Sergey