From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:58576) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SXZ7Q-0006OL-23 for qemu-devel@nongnu.org; Thu, 24 May 2012 10:34:32 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SXZ7J-0003QA-F3 for qemu-devel@nongnu.org; Thu, 24 May 2012 10:34:23 -0400 Received: from thoth.sbs.de ([192.35.17.2]:29677) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SXZ7J-0003Px-4f for qemu-devel@nongnu.org; Thu, 24 May 2012 10:34:17 -0400 Message-ID: <4FBE46E1.8050201@siemens.com> Date: Thu, 24 May 2012 11:34:09 -0300 From: Jan Kiszka MIME-Version: 1.0 References: <4FBD9E3A.6080704@web.de> <4FBE1A98.3090708@siemens.com> <4FBE26AF.2090103@siemens.com> <4FBE3716.6040205@siemens.com> <4FBE43EB.2090507@siemens.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] TCG: Fix TB invalidation after breakpoint insertion/deletion List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Max Filippov Cc: Blue Swirl , TeLeMan , qemu-devel , Avi Kivity On 2012-05-24 11:29, Max Filippov wrote: > On Thu, May 24, 2012 at 6:21 PM, Jan Kiszka wrote: >> On 2012-05-24 11:11, Max Filippov wrote: >>> On Thu, May 24, 2012 at 5:26 PM, Jan Kiszka wrote: >>>> On 2012-05-24 09:42, Max Filippov wrote: >>>>> On Thu, May 24, 2012 at 4:16 PM, Jan Kiszka wrote: >>>>>> On 2012-05-24 09:08, Max Filippov wrote: >>>>>>> On Thu, May 24, 2012 at 3:25 PM, Jan Kiszka wrote: >>>>>>>> On 2012-05-24 07:51, Max Filippov wrote: >>>>>>>>> On Thu, May 24, 2012 at 6:34 AM, Jan Kiszka wrote: >>>>>>>>>> From: Jan Kiszka >>>>>>>>>> >>>>>>>>>> tb_invalidate_phys_addr has to called with the exact physical address of >>>>>>>>>> the breakpoint we add/remove, not just the page's base address. >>>>>>>>>> Otherwise we easily fail to flush the right TB. >>>>>>>>>> >>>>>>>>>> Regression of 1e7855a558. >>>>>>>>> >>>>>>>>> Sorry, I fail to see how 1e7855a558 could introduce a regression, it >>>>>>>>> just rearranged the code. >>>>>>>>> Even more, AFAIK cpu_get_phys_page_debug returns complete physical >>>>>>>>> address, not just >>>>>>>>> physical page. Probably it has a misleading name. >>>>>>>> >>>>>>>> Unfortunately, cpu_get_phys_page_debug does NOT deliver the sub-page >>>>>>>> offset, only the page base address. >>>>>>> >>>>>>> Ok, i386 has probably the most explicit implementation, >>>>>>> let's look at the target-i386/helper.c:876 >>>>>>> >>>>>>> page_offset = (addr & TARGET_PAGE_MASK) & (page_size - 1); >>>>>>> paddr = (pte & TARGET_PAGE_MASK) + page_offset; >>>>>>> return paddr; >>>>>>> >>>>>>> that's clearly physical page plus in-page offset. >>>>>>> I can provide other samples (: >>>>>> >>>>>> "page_offset" is misleading: addr & TARGET_PAGE_MASK kills all the >>>>>> offset bits. It will only contain the relevant bits between page_size >>>>>> and TARGET_PAGE_SIZE. >>>>>> >>>>>> Check also ppc's cpu_get_phys_page_debug, it's clearer in this regard. >>>>> >>>>> Ok, for i386, ppc, microblaze (and maybe others) you're right. >>>>> What about ARM, CRIS, MIPS, SH4, xtensa (and maybe others)? >>>>> Looks like this is a long-standing discrepancy and consequently >>>>> a long-standing bug in the breakpoint_invalidate. >>>> >>>> Not in breakpoint_invalidate as the missing offset was compensated >>>> before your commit (well, starting with c2f07f81a2 in fact). >>> >>> I'd say that compensation that you mention >>> >>> ram_addr = (memory_region_get_ram_addr(section.mr) >>> + section.offset_within_region) & TARGET_PAGE_MASK; >>> this >>>> ram_addr |= (pc & ~TARGET_PAGE_MASK); >>> tb_invalidate_phys_page_range(ram_addr, ram_addr + 1, 0); >>> >>> was removed by f3705d53296d, not by 1e7855a558 >> >> Unless I misinterpret section_addr, it does return the lower address >> bits unmodified. > > Maybe, but > > addr = cpu_get_phys_page_debug(env, pc); > > which should have lost its in-page offset according to you. Err, right. Jan -- Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux