Re: [Qemu-devel] [PATCH] TCG: Fix TB invalidation after breakpoint insertion/deletion

[Jan Kiszka <jan.kiszka@siemens.com>]

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

> 
>> But it looks like cpu_get_phys_page_debug was broken for quite a while.
>> Let's fix those archs to return more than page-aligned addresses.
> 
> You mean make them all return full physical address?
> I'd propose to rename the function then as well.

No, to return the page base address like x86 etc. do and like most if
not all users expect it to. So fix ARM & Co.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux