On 2012-05-24 11:29, Max Filippov wrote: > On Thu, May 24, 2012 at 6:21 PM, Jan Kiszka <jan.kis...@siemens.com> wrote: >> On 2012-05-24 11:11, Max Filippov wrote: >>> On Thu, May 24, 2012 at 5:26 PM, Jan Kiszka <jan.kis...@siemens.com> wrote: >>>> On 2012-05-24 09:42, Max Filippov wrote: >>>>> On Thu, May 24, 2012 at 4:16 PM, Jan Kiszka <jan.kis...@siemens.com> >>>>> wrote: >>>>>> On 2012-05-24 09:08, Max Filippov wrote: >>>>>>> On Thu, May 24, 2012 at 3:25 PM, Jan Kiszka <jan.kis...@siemens.com> >>>>>>> wrote: >>>>>>>> On 2012-05-24 07:51, Max Filippov wrote: >>>>>>>>> On Thu, May 24, 2012 at 6:34 AM, Jan Kiszka <jan.kis...@web.de> wrote: >>>>>>>>>> From: Jan Kiszka <jan.kis...@siemens.com> >>>>>>>>>> >>>>>>>>>> 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
