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