| Message ID | 20200603112442.22833-1-alex.bennee@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] exec: flush the whole TLB if a watchpoint crosses a page boundary | expand |
On 6/3/20 1:24 PM, Alex Bennée wrote: > There is no particular reason why you can't have a watchpoint in TCG > that covers a large chunk of the address space. We could be clever > about it but these cases are pretty rare and we can assume the user > will expect a little performance degradation. > > NB: In my testing gdb will silently squash a watchpoint like: > > watch (char[0x7fffffffff]) *0x0 > > to a 4 byte watchpoint. Practically it will limit the maximum size > based on max-value-size. However given enough of a tweak the sky is > the limit. > > Reported-by: Alexander Bulekov <alxndr@bu.edu> > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > > --- > v2 > - use cleaner in_page = -(addr | TARGET_PAGE_MASK) logic per rth Can we have a macro for this? Maybe QEMU_IN_PAGE_OFFSET(addr, TARGET_PAGE_MASK)? or QEMU_OFFSET_IN_PAGE()... > --- > exec.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/exec.c b/exec.c > index 5162f0d12f9..65a4376df37 100644 > --- a/exec.c > +++ b/exec.c > @@ -1036,6 +1036,7 @@ int cpu_watchpoint_insert(CPUState *cpu, vaddr addr, vaddr len, > int flags, CPUWatchpoint **watchpoint) > { > CPUWatchpoint *wp; > + vaddr in_page; > > /* forbid ranges which are empty or run off the end of the address space */ > if (len == 0 || (addr + len - 1) < addr) { > @@ -1056,7 +1057,12 @@ int cpu_watchpoint_insert(CPUState *cpu, vaddr addr, vaddr len, > QTAILQ_INSERT_TAIL(&cpu->watchpoints, wp, entry); > } > > - tlb_flush_page(cpu, addr); > + in_page = -(addr | TARGET_PAGE_MASK); > + if (len <= in_page) { > + tlb_flush_page(cpu, addr); > + } else { > + tlb_flush(cpu); > + } > > if (watchpoint) > *watchpoint = wp; >
On 6/3/20 4:24 AM, Alex Bennée wrote: > There is no particular reason why you can't have a watchpoint in TCG > that covers a large chunk of the address space. We could be clever > about it but these cases are pretty rare and we can assume the user > will expect a little performance degradation. > > NB: In my testing gdb will silently squash a watchpoint like: > > watch (char[0x7fffffffff]) *0x0 > > to a 4 byte watchpoint. Practically it will limit the maximum size > based on max-value-size. However given enough of a tweak the sky is > the limit. > > Reported-by: Alexander Bulekov <alxndr@bu.edu> > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > > --- > v2 > - use cleaner in_page = -(addr | TARGET_PAGE_MASK) logic per rth > --- > exec.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) Reviewed-by: Richard Henderson <richard.henderson@linaro.org> Queued to tcg-next. r~
On 6/3/20 2:46 PM, Philippe Mathieu-Daudé wrote: > On 6/3/20 1:24 PM, Alex Bennée wrote: >> There is no particular reason why you can't have a watchpoint in TCG >> that covers a large chunk of the address space. We could be clever >> about it but these cases are pretty rare and we can assume the user >> will expect a little performance degradation. >> >> NB: In my testing gdb will silently squash a watchpoint like: >> >> watch (char[0x7fffffffff]) *0x0 >> >> to a 4 byte watchpoint. Practically it will limit the maximum size >> based on max-value-size. However given enough of a tweak the sky is >> the limit. >> >> Reported-by: Alexander Bulekov <alxndr@bu.edu> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> >> >> --- >> v2 >> - use cleaner in_page = -(addr | TARGET_PAGE_MASK) logic per rth > > Can we have a macro for this? > Maybe QEMU_IN_PAGE_OFFSET(addr, TARGET_PAGE_MASK)? > or QEMU_OFFSET_IN_PAGE()... As this is queued, I suppose the implicit answer is "no." > >> --- >> exec.c | 8 +++++++- >> 1 file changed, 7 insertions(+), 1 deletion(-) >> >> diff --git a/exec.c b/exec.c >> index 5162f0d12f9..65a4376df37 100644 >> --- a/exec.c >> +++ b/exec.c >> @@ -1036,6 +1036,7 @@ int cpu_watchpoint_insert(CPUState *cpu, vaddr addr, vaddr len, >> int flags, CPUWatchpoint **watchpoint) >> { >> CPUWatchpoint *wp; >> + vaddr in_page; >> >> /* forbid ranges which are empty or run off the end of the address space */ >> if (len == 0 || (addr + len - 1) < addr) { >> @@ -1056,7 +1057,12 @@ int cpu_watchpoint_insert(CPUState *cpu, vaddr addr, vaddr len, >> QTAILQ_INSERT_TAIL(&cpu->watchpoints, wp, entry); >> } >> >> - tlb_flush_page(cpu, addr); >> + in_page = -(addr | TARGET_PAGE_MASK); >> + if (len <= in_page) { >> + tlb_flush_page(cpu, addr); >> + } else { >> + tlb_flush(cpu); >> + } >> >> if (watchpoint) >> *watchpoint = wp; >> >
Philippe Mathieu-Daudé <philmd@redhat.com> writes: > On 6/3/20 2:46 PM, Philippe Mathieu-Daudé wrote: >> On 6/3/20 1:24 PM, Alex Bennée wrote: >>> There is no particular reason why you can't have a watchpoint in TCG >>> that covers a large chunk of the address space. We could be clever >>> about it but these cases are pretty rare and we can assume the user >>> will expect a little performance degradation. >>> >>> NB: In my testing gdb will silently squash a watchpoint like: >>> >>> watch (char[0x7fffffffff]) *0x0 >>> >>> to a 4 byte watchpoint. Practically it will limit the maximum size >>> based on max-value-size. However given enough of a tweak the sky is >>> the limit. >>> >>> Reported-by: Alexander Bulekov <alxndr@bu.edu> >>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> >>> >>> --- >>> v2 >>> - use cleaner in_page = -(addr | TARGET_PAGE_MASK) logic per rth >> >> Can we have a macro for this? >> Maybe QEMU_IN_PAGE_OFFSET(addr, TARGET_PAGE_MASK)? >> or QEMU_OFFSET_IN_PAGE()... > > As this is queued, I suppose the implicit answer is "no." Richard took it into tcg/next as is. I think having a macro may well be nice clean-up but I struggled to pick a good include location so left it for a future clean-up series ;-) -- Alex Bennée
diff --git a/exec.c b/exec.c index 5162f0d12f9..65a4376df37 100644 --- a/exec.c +++ b/exec.c @@ -1036,6 +1036,7 @@ int cpu_watchpoint_insert(CPUState *cpu, vaddr addr, vaddr len, int flags, CPUWatchpoint **watchpoint) { CPUWatchpoint *wp; + vaddr in_page; /* forbid ranges which are empty or run off the end of the address space */ if (len == 0 || (addr + len - 1) < addr) { @@ -1056,7 +1057,12 @@ int cpu_watchpoint_insert(CPUState *cpu, vaddr addr, vaddr len, QTAILQ_INSERT_TAIL(&cpu->watchpoints, wp, entry); } - tlb_flush_page(cpu, addr); + in_page = -(addr | TARGET_PAGE_MASK); + if (len <= in_page) { + tlb_flush_page(cpu, addr); + } else { + tlb_flush(cpu); + } if (watchpoint) *watchpoint = wp;
There is no particular reason why you can't have a watchpoint in TCG that covers a large chunk of the address space. We could be clever about it but these cases are pretty rare and we can assume the user will expect a little performance degradation. NB: In my testing gdb will silently squash a watchpoint like: watch (char[0x7fffffffff]) *0x0 to a 4 byte watchpoint. Practically it will limit the maximum size based on max-value-size. However given enough of a tweak the sky is the limit. Reported-by: Alexander Bulekov <alxndr@bu.edu> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> --- v2 - use cleaner in_page = -(addr | TARGET_PAGE_MASK) logic per rth --- exec.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) -- 2.20.1