| Message ID | 4FBD9E3A.6080704@web.de |
|---|---|
| State | New |
| Headers | show |
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 */
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
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
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?
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
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.
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
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.
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
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.
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.
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
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
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.
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.
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.
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 */