Re: [Qemu-devel] [PATCH] TCG: Fix TB invalidation after breakpoint insertion/deletion
On 05/24/2012 10:58 PM, Max Filippov wrote: On Thu, May 24, 2012 at 6:26 PM, Avi Kivity a...@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
On Mon, May 28, 2012 at 1:34 PM, Avi Kivity a...@redhat.com wrote: On 05/24/2012 10:58 PM, Max Filippov wrote: On Thu, May 24, 2012 at 6:26 PM, Avi Kivity a...@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
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
Re: [Qemu-devel] [PATCH] TCG: Fix TB invalidation after breakpoint insertion/deletion
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.
Reported-by: TeLeMan gele...@gmail.com
Signed-off-by: Jan Kiszka jan.kis...@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
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. 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
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 (:
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
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. 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
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. -- Thanks. -- Max
Re: [Qemu-devel] [PATCH] TCG: Fix TB invalidation after breakpoint insertion/deletion
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). 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
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 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
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. 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
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
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. 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
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
Re: [Qemu-devel] [PATCH] TCG: Fix TB invalidation after breakpoint insertion/deletion
On Thu, May 24, 2012 at 6:26 PM, Avi Kivity a...@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
On 2012-05-23 23:34, Jan Kiszka 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, forgot the tag: this should go in before 1.1 of course.
Reported-by: TeLeMan gele...@gmail.com
Signed-off-by: Jan Kiszka jan.kis...@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 */
signature.asc
Description: OpenPGP digital signature
