From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39118) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a8VqY-0001Ue-St for qemu-devel@nongnu.org; Mon, 14 Dec 2015 11:19:38 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a8VqX-0000Xm-Qz for qemu-devel@nongnu.org; Mon, 14 Dec 2015 11:19:34 -0500 Sender: Richard Henderson References: <1449773244-17078-1-git-send-email-serge.fdrv@gmail.com> <566B5E9E.8040108@twiddle.net> <566C7D38.4040609@gmail.com> From: Richard Henderson Message-ID: <566EEC05.2080702@twiddle.net> Date: Mon, 14 Dec 2015 08:19:17 -0800 MIME-Version: 1.0 In-Reply-To: <566C7D38.4040609@gmail.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] target-*: Get rid of "PC advancement" trick List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Sergey Fedorov , qemu-devel@nongnu.org Cc: Peter Maydell , Eduardo Habkost , Anthony Green , Alexander Graf , Max Filippov , Michael Walle , qemu-arm@nongnu.org, qemu-ppc@nongnu.org, Paolo Bonzini , "Edgar E. Iglesias" , Guan Xuetao , Leon Alrae , Aurelien Jarno , Jia Liu On 12/12/2015 12:02 PM, Sergey Fedorov wrote: > On 12/12/15 02:39, Richard Henderson wrote: >> On 12/10/2015 10:47 AM, Sergey Fedorov wrote: >>> The "PC advancement" trick was used just after recognizing that a >>> breakpoint exception was going to be generated. This trick has had two >>> points: >>> 1. Guarantee that tb->size isn't zero: there are many places where it's >>> expected to be non-zero. In fact, that is even stated in the comment >>> for this field. >>> 2. Try to satisfy disassembler's check for instruction length. To this >>> end, PC advancement was done for estimated instruction length, but >>> actually, didn't work properly in variable-instruction-length cases. >>> >>> Substitute this trick with checking for TB size at the end of >>> translation. If we get an empty TB then just set tb->size to 1 and skip >>> disassembling. Setting tb->size to 1 is enough to get correct behaviour, >>> whereas an empty TB doesn't obviously need to be disassembled. >> >> This doesn't help when the TB already has instructions, the TB would >> ordinarily cross a page boundary, and the breakpoint is at the page boundary. > > I see your point. But I am wondering why most architectures stop translating on > a page boundary whereas i386 and m86k don't. There are some comments which say > that's to ensure instruction fetch aborts occur at the right place. Isn't it > necessary for all architectures? In order to support targets with variable-sized instructions, like i386, where a single instruction may straddle the page boundary, generic code can handle a TB taking code from two (and only two) pages. It's true that some of the way we treat i386 TBs plays a bit fast and loose, in that it's possible to design a code sequence that will report a page access error at the beginning of the TB, rather than another runtime exception which might have been visible if we'd executed the TB. I believe that someone fixed that for arm thumb, but no one has bothered about this for i386, m68k, or s390. Because it just doesn't happen in practice; one has to go out of one's way to design the errant code sequence. > At least for those architectures which do stop translating on a page boundary, > I think this patch is applicable. Certainly, it would be better to have a > single solution for all architectures. For that, I think it might be interesting to arrange for non-empty TBs to exit prior to recognizing a breakpoint. So that a breakpoint TB is always just the one operation. Except for the fact that "generate an exception" has traditionally been a target-specific helper, we could almost make the entire breakpoint generation be done in common code. I'd think something like a generic "must we end the TB now" predicate would be the proper hook. It would contain all of the usual stuff: tcg_op_buf_full and checks for singlestep, but then add "is there a breakpoint at the next pc". r~