TCG: Fix TB invalidation after breakpoint insertion/deletion

PATCH In review Jan Kiszka 4 participants 17 messages May 24, 2012 – May 28, 2012

lore

Share & tools

lore

Jan Kiszka

author

2012-05-24 02:34 UTC #0 lore raw

PATCH 1/1 TCG: Fix TB invalidation after breakpoint insertion/deletion

From: Jan Kiszka <[email protected]>

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 <[email protected]>

Signed-off-by Jan Kiszka <[email protected]>

---
 exec.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

exec.c +2 -1

index a0494c7..efa1345 100644
@@ -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 */

Signature · 1 line

1.7.3.4

Jan Kiszka

author

2012-05-24 02:44 UTC #1 lore raw

Quoted 7 lines · Jan Kiszka

From: Jan Kiszka <[email protected]>
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.

Quoted 21 lines

Reported-by TeLeMan <[email protected]>

Signed-off-by Jan Kiszka <[email protected]>

---
 exec.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

exec.c +2 -1

index a0494c7..efa1345 100644
@@ -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 */

Max Filippov

2012-05-24 10:51 UTC #2 lore raw

In reply to Jan Kiszka

Quoted 7 lines · Jan Kiszka

From: Jan Kiszka <[email protected]>
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.

Quoted 22 lines

Reported-by TeLeMan <[email protected]>

Signed-off-by Jan Kiszka <[email protected]>

---
exec.c | 3 ++-

 1 files changed, 2 insertions(+), 1 deletions(-)

exec.c +2 -2

index a0494c7..efa1345 100644
@@ -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

Signature · 2 lines

Thanks.
-- Max

Jan Kiszka

2012-05-24 11:25 UTC #3 lore raw

Quoted 7 lines · Max Filippov

Quoted 6 lines · Jan Kiszka
From: Jan Kiszka <[email protected]>
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.

Quoted 7 lines

Quoted 1 line
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

Signature · 2 lines

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

Max Filippov

2012-05-24 12:08 UTC #4 lore raw

Quoted 10 lines · Jan Kiszka

Quoted 9 lines · Max Filippov
Quoted 7 lines · Jan Kiszka
From: Jan Kiszka <[email protected]>
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.

Quoted 8 lines

Quoted 5 lines
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 (:

Quoted 2 lines

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?

Signature · 2 lines

Thanks.
-- Max

Jan Kiszka

2012-05-24 12:17 UTC #5 lore raw

Quoted 23 lines · Max Filippov

Quoted 10 lines · Jan Kiszka
Quoted 9 lines · Max Filippov
Quoted 7 lines · Jan Kiszka
From: Jan Kiszka <[email protected]>
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.
Quoted 8 lines
Quoted 5 lines
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

Quoted 6 lines

    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

Signature · 2 lines

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

Max Filippov

2012-05-24 12:42 UTC #6 lore raw

Quoted 28 lines · Jan Kiszka

Quoted 27 lines · Max Filippov
Quoted 10 lines · Jan Kiszka
Quoted 9 lines · Max Filippov
Quoted 7 lines · Jan Kiszka
From: Jan Kiszka <[email protected]>
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.
Quoted 8 lines
Quoted 5 lines
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;

Quoted 8 lines

Quoted 2 lines
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.

Signature · 2 lines

Thanks.
-- Max

Jan Kiszka

2012-05-24 13:27 UTC #7 lore raw

Quoted 36 lines · Max Filippov

Quoted 35 lines · Jan Kiszka
Quoted 23 lines · Max Filippov
Quoted 10 lines · Jan Kiszka
Quoted 9 lines · Max Filippov
Quoted 7 lines · Jan Kiszka
From: Jan Kiszka <[email protected]>
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.
Quoted 8 lines
Quoted 5 lines
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
Quoted 6 lines
    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.

Quoted 6 lines

Quoted 1 line
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

Signature · 2 lines

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

Max Filippov

2012-05-24 14:11 UTC #8 lore raw

Quoted 39 lines · Jan Kiszka

Quoted 38 lines · Max Filippov
Quoted 36 lines · Jan Kiszka
Quoted 23 lines · Max Filippov
Quoted 10 lines · Jan Kiszka
Quoted 9 lines · Max Filippov
Quoted 7 lines · Jan Kiszka
From: Jan Kiszka <[email protected]>
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.
Quoted 8 lines
Quoted 5 lines
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
Quoted 6 lines
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.

Quoted 7 lines

Quoted 4 lines
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

Quoted 2 lines

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.

Signature · 2 lines

Thanks.
-- Max

Jan Kiszka

2012-05-24 14:21 UTC #9 lore raw

Quoted 48 lines · Max Filippov

Quoted 39 lines · Jan Kiszka
Quoted 38 lines · Max Filippov
Quoted 36 lines · Jan Kiszka
Quoted 23 lines · Max Filippov
Quoted 10 lines · Jan Kiszka
Quoted 9 lines · Max Filippov
Quoted 7 lines · Jan Kiszka
From: Jan Kiszka <[email protected]>
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.
Quoted 8 lines
Quoted 5 lines
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
Quoted 6 lines
    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.
Quoted 7 lines
Quoted 4 lines
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).

Quoted 8 lines

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.

Quoted 6 lines

Quoted 3 lines
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

Signature · 2 lines

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

Max Filippov

2012-05-24 14:29 UTC #10 lore raw

Quoted 56 lines · Jan Kiszka

Quoted 55 lines · Max Filippov
Quoted 39 lines · Jan Kiszka
Quoted 38 lines · Max Filippov
Quoted 36 lines · Jan Kiszka
Quoted 23 lines · Max Filippov
Quoted 10 lines · Jan Kiszka
Quoted 9 lines · Max Filippov
Quoted 7 lines · Jan Kiszka
From: Jan Kiszka <[email protected]>
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.
Quoted 8 lines
Quoted 5 lines
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
Quoted 6 lines
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.
Quoted 7 lines
Quoted 4 lines
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);

Quoted 4 lines

Quoted 1 line
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.

Quoted 9 lines

Quoted 6 lines
Quoted 3 lines
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.

Signature · 2 lines

Thanks.
-- Max

Jan Kiszka

2012-05-24 14:34 UTC #11 lore raw

Quoted 59 lines · Max Filippov

Quoted 58 lines · Jan Kiszka
Quoted 56 lines · Max Filippov
Quoted 39 lines · Jan Kiszka
Quoted 38 lines · Max Filippov
Quoted 36 lines · Jan Kiszka
Quoted 23 lines · Max Filippov
Quoted 10 lines · Jan Kiszka
Quoted 9 lines · Max Filippov
Quoted 7 lines · Jan Kiszka
From: Jan Kiszka <[email protected]>
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.
Quoted 8 lines
Quoted 5 lines
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
Quoted 6 lines
    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.
Quoted 7 lines
Quoted 4 lines
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

Quoted 8 lines

Quoted 2 lines
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

Signature · 2 lines

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

Avi Kivity

2012-05-24 14:26 UTC #12 lore raw

In reply to Max Filippov

Quoted 4 lines · Max Filippov

Quoted 3 lines
Not in breakpoint_invalidate as the missing offset was compensated before your commit (well, starting with c2f07f81a2 in fact).

Quoted 8 lines

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

Quoted 7 lines

Quoted 3 lines
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.

Signature · 1 line

error compiling committee.c: too many arguments to function

Max Filippov

2012-05-24 19:59 UTC #13 lore raw

Quoted 12 lines · Avi Kivity

Quoted 11 lines · Max Filippov
Quoted 3 lines
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);

Quoted 4 lines

Quoted 1 line
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.

Quoted 8 lines

Quoted 5 lines
Quoted 2 lines
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.

Quoted 4 lines

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.

Quoted 2 lines

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

Signature · 2 lines

Thanks.
-- Max

Avi Kivity

2012-05-28 09:34 UTC #14 lore raw

Quoted 13 lines · Max Filippov

Quoted 12 lines · Avi Kivity
Quoted 11 lines · Max Filippov
Quoted 3 lines
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);

Quoted 7 lines

Quoted 4 lines
Quoted 1 line
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.

Signature · 1 line

error compiling committee.c: too many arguments to function

Max Filippov

2012-05-28 11:54 UTC #15 lore raw

Quoted 19 lines · Avi Kivity

Quoted 18 lines · Max Filippov
Quoted 16 lines · Avi Kivity
Quoted 4 lines · Max Filippov
Quoted 3 lines
Not in breakpoint_invalidate as the missing offset was compensated before your commit (well, starting with c2f07f81a2 in fact).
Quoted 8 lines
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().

Quoted 7 lines

Quoted 2 lines
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.

Signature · 2 lines

Thanks.
-- Max

Avi Kivity

2012-05-28 12:04 UTC #16 lore raw

Quoted 4 lines · Max Filippov

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

Quoted 8 lines

Quoted 4 lines
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.

Signature · 1 line

error compiling committee.c: too many arguments to function