From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37758) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zlzoh-0003uh-PC for qemu-devel@nongnu.org; Tue, 13 Oct 2015 09:40:36 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zlzod-0002RA-QG for qemu-devel@nongnu.org; Tue, 13 Oct 2015 09:40:35 -0400 Received: from mail-lb0-x22d.google.com ([2a00:1450:4010:c04::22d]:34336) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zlzod-0002R0-IY for qemu-devel@nongnu.org; Tue, 13 Oct 2015 09:40:31 -0400 Received: by lbbck17 with SMTP id ck17so20515671lbb.1 for ; Tue, 13 Oct 2015 06:40:30 -0700 (PDT) References: <1444211031-11624-1-git-send-email-rth@twiddle.net> <1444211031-11624-5-git-send-email-rth@twiddle.net> <5617C252.6070104@gmail.com> <561C4CA4.9010506@twiddle.net> From: Sergey Fedorov Message-ID: <561D09CC.9010903@gmail.com> Date: Tue, 13 Oct 2015 16:40:28 +0300 MIME-Version: 1.0 In-Reply-To: <561C4CA4.9010506@twiddle.net> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PULL 04/26] target-*: Introduce and use cpu_breakpoint_test List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Richard Henderson , qemu-devel@nongnu.org Cc: peter.maydell@linaro.org On 13.10.2015 03:13, Richard Henderson wrote: > On 10/10/2015 12:34 AM, Sergey Fedorov wrote: >>> @@ -2936,6 +2927,10 @@ static inline void >>> gen_intermediate_code_internal(AlphaCPU *cpu, >>> tcg_gen_insn_start(ctx.pc); >>> num_insns++; >>> >>> + if (unlikely(cpu_breakpoint_test(cs, ctx.pc, BP_ANY))) { >>> + gen_excp(&ctx, EXCP_DEBUG, 0); >>> + break; >>> + } >> >> Actually, control logic has changed here. The old code used a break >> statement to exit from QTAILQ_FOREACH loop and continue with instruction >> translation thus translating at least one instruction. The break >> statement in the new code makes exit from the translation loop itself, >> effectively producing zero-length TB which won't get invalidated when >> clearing the breakpoint. Seems like we should remove the break statement >> here and in similar cases below, right? > > Why do you believe that a zero-length TB won't be cleared? > The TB still has a start address, which is contained within > a given page, which is invalidated. > > Some target-*/translate.c takes care to advance the PC, but I believe > that this is only required when the breakpoint instruction *itself* > could span a page boundary. I.e. the TB needs to be marked to span > two pages. This situation can never be true for many RISC targets. > > We did discuss this exact situation during review of the patch set, > though it's probably true that there are outstanding errors in some > translators. I see your point, but what I am actually concerned about is the following scenario. Lets suppose we are going to remove a breakpoint. Eventually we can get the following function call stack: #0 tb_invalidate_phys_page_range() #1 tb_invalidate_phys_addr() #2 breakpoint_invalidate() #3 cpu_breakpoint_remove_by_ref() ... Then, if we come across a zero-sized TB then 'tb_start' would be equal to 'tb_end'. That is not a big deal unless 'start' is not equal to 'tb_start'. Otherwise, "!(tb_end <= start || tb_start >= end)" condition check will fail and that TB won't get invalidated as it should be. As I can see this is a case when we try to remove a breakpoint which is happened to be set at the very first instruction of a TB. So we either need to change the condition in tb_invalidate_phys_page_range() or do the PC advancement trick during translation, no matter can instructions cross a page boundary or not. Best regards, Sergey