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

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

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 927 bytes --]

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.

Reported-by: TeLeMan <geleman@gmail.com>
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 exec.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/exec.c b/exec.c
index a0494c7..efa1345 100644
--- a/exec.c
+++ b/exec.c
@@ -1492,7 +1492,8 @@ void tb_invalidate_phys_addr(target_phys_addr_t addr)
 
 static void breakpoint_invalidate(CPUArchState *env, target_ulong pc)
 {
-    tb_invalidate_phys_addr(cpu_get_phys_page_debug(env, pc));
+    tb_invalidate_phys_addr(cpu_get_phys_page_debug(env, pc) +
+                            (pc & ~TARGET_PAGE_MASK));
 }
 #endif
 #endif /* TARGET_HAS_ICE */
-- 
1.7.3.4


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

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

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1077 bytes --]

On 2012-05-23 23:34, Jan Kiszka 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, forgot the tag: this should go in before 1.1 of course.

> 
> Reported-by: TeLeMan <geleman@gmail.com>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  exec.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index a0494c7..efa1345 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1492,7 +1492,8 @@ void tb_invalidate_phys_addr(target_phys_addr_t addr)
>  
>  static void breakpoint_invalidate(CPUArchState *env, target_ulong pc)
>  {
> -    tb_invalidate_phys_addr(cpu_get_phys_page_debug(env, pc));
> +    tb_invalidate_phys_addr(cpu_get_phys_page_debug(env, pc) +
> +                            (pc & ~TARGET_PAGE_MASK));
>  }
>  #endif
>  #endif /* TARGET_HAS_ICE */


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

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

[Max Filippov]

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.

> Reported-by: TeLeMan <geleman@gmail.com>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  exec.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/exec.c b/exec.c
> index a0494c7..efa1345 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1492,7 +1492,8 @@ void tb_invalidate_phys_addr(target_phys_addr_t addr)
>
>  static void breakpoint_invalidate(CPUArchState *env, target_ulong pc)
>  {
> -    tb_invalidate_phys_addr(cpu_get_phys_page_debug(env, pc));
> +    tb_invalidate_phys_addr(cpu_get_phys_page_debug(env, pc) +
> +                            (pc & ~TARGET_PAGE_MASK));
>  }
>  #endif
>  #endif /* TARGET_HAS_ICE */
> --
> 1.7.3.4

-- 
Thanks.
-- Max

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

[Jan Kiszka]

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. So the regression was caused by this
refactoring.

Jan

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

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

[Max Filippov]

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 (:

> So the regression was caused by this
> refactoring.

The refactoring is this:

-static void breakpoint_invalidate(CPUArchState *env, target_ulong pc)
+void tb_invalidate_phys_addr(target_phys_addr_t addr)
 {
-    target_phys_addr_t addr;
     ram_addr_t ram_addr;
     MemoryRegionSection *section;

-    addr = cpu_get_phys_page_debug(env, pc);
     section = phys_page_find(addr >> TARGET_PAGE_BITS);
     if (!(memory_region_is_ram(section->mr)
           || (section->mr->rom_device && section->mr->readable))) {
@@ -1479,6 +1477,11 @@ static void breakpoint_invalidate(CPUArchState
*env, target_ulong pc)
         + section_addr(section, addr);
     tb_invalidate_phys_page_range(ram_addr, ram_addr + 1, 0);
 }
+
+static void breakpoint_invalidate(CPUArchState *env, target_ulong pc)
+{
+    tb_invalidate_phys_addr(cpu_get_phys_page_debug(env, pc));
+}

so it's literally just code move.

Is there a real bug that is fixed by the patch?

-- 
Thanks.
-- Max

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

[Jan Kiszka]

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.

Jan

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

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

[Max Filippov]

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.

-- 
Thanks.
-- Max

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

[Jan Kiszka]

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).

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.

Jan

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

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

[Max Filippov]

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

> 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.

-- 
Thanks.
-- Max

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

[Jan Kiszka]

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

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

[Max Filippov]

On Thu, May 24, 2012 at 6:21 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> 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.

Maybe, but

addr = cpu_get_phys_page_debug(env, pc);

which should have lost its in-page offset according to you.

>>
>>> 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.

-- 
Thanks.
-- Max

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

[Jan Kiszka]

On 2012-05-24 11:29, Max Filippov wrote:
> On Thu, May 24, 2012 at 6:21 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>> 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.
> 
> 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

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

[Avi Kivity]

On 05/24/2012 05:11 PM, Max Filippov wrote:
>>
>> 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


Indeed.  Note how the |= cleverly accommodates both truncating and
non-truncating cpu_get_phys_page_debug().

> 
>> 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.
> 

Agree to both.  cpu_translate_virtual_address() or similar would be more
explanatory IMO.

btw, how does the thing work for soft-tlb cpus?  It looks like this
thing should trigger on tlb loads, not when the breakpoint is set.  This
is true for hardware page tables as well, as the mapping can change,
though it's less likely.
-- 
error compiling committee.c: too many arguments to function

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

[Max Filippov]

On Thu, May 24, 2012 at 6:26 PM, Avi Kivity <avi@redhat.com> wrote:
> On 05/24/2012 05:11 PM, Max Filippov wrote:
>>>
>>> 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
>
> Indeed.  Note how the |= cleverly accommodates both truncating and
> non-truncating cpu_get_phys_page_debug().

Right. If the fix is going to be checked in then TeLeMan's original version
with '|' is preferable for this reason.

>>> 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.
>
> Agree to both.  cpu_translate_virtual_address() or similar would be more
> explanatory IMO.

Looks like Jan has an opposite opinion.

> btw, how does the thing work for soft-tlb cpus?  It looks like this
> thing should trigger on tlb loads, not when the breakpoint is set.  This
> is true for hardware page tables as well, as the mapping can change,
> though it's less likely.

I guess it works by chance. And chances high that breakpoints are
removed in the same virtual memory context where they got triggered,
so that right TBs are invalidated.

> --
> error compiling committee.c: too many arguments to function

-- 
Thanks.
-- Max

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

[Avi Kivity]

On 05/24/2012 10:58 PM, Max Filippov wrote:
> On Thu, May 24, 2012 at 6:26 PM, Avi Kivity <avi@redhat.com> wrote:
>> On 05/24/2012 05:11 PM, Max Filippov wrote:
>>>>
>>>> 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
>>
>> Indeed.  Note how the |= cleverly accommodates both truncating and
>> non-truncating cpu_get_phys_page_debug().
> 
> Right. If the fix is going to be checked in then TeLeMan's original version
> with '|' is preferable for this reason.

I disagree.  Whatever we call cpu_get_phys_page_debug() has to either
mask out the low bits, or not (I prefer the latter, since it's
unambiguous for large pages), but it has to be consistent.  Once it's
consistent, there's no reason to use clever tricks.


-- 
error compiling committee.c: too many arguments to function

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

[Max Filippov]

On Mon, May 28, 2012 at 1:34 PM, Avi Kivity <avi@redhat.com> wrote:
> On 05/24/2012 10:58 PM, Max Filippov wrote:
>> On Thu, May 24, 2012 at 6:26 PM, Avi Kivity <avi@redhat.com> wrote:
>>> On 05/24/2012 05:11 PM, Max Filippov wrote:
>>>>>
>>>>> 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
>>>
>>> Indeed.  Note how the |= cleverly accommodates both truncating and
>>> non-truncating cpu_get_phys_page_debug().
>>
>> Right. If the fix is going to be checked in then TeLeMan's original version
>> with '|' is preferable for this reason.
>
> I disagree.  Whatever we call cpu_get_phys_page_debug() has to either
> mask out the low bits, or not (I prefer the latter, since it's
> unambiguous for large pages), but it has to be consistent.  Once it's
> consistent, there's no reason to use clever tricks.

I meant a one line fix for the 1.1. I suspect that fixing entire
cpu_get_phys_page_debug thing for 1.1 is too risky.

-- 
Thanks.
-- Max

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

[Avi Kivity]

On 05/28/2012 02:54 PM, Max Filippov wrote:
>>>
>>> Right. If the fix is going to be checked in then TeLeMan's original version
>>> with '|' is preferable for this reason.
>>
>> I disagree.  Whatever we call cpu_get_phys_page_debug() has to either
>> mask out the low bits, or not (I prefer the latter, since it's
>> unambiguous for large pages), but it has to be consistent.  Once it's
>> consistent, there's no reason to use clever tricks.
> 
> I meant a one line fix for the 1.1. I suspect that fixing entire
> cpu_get_phys_page_debug thing for 1.1 is too risky.
> 

Agree for 1.1.


-- 
error compiling committee.c: too many arguments to function