* [PATCH v2] exec: flush the whole TLB if a watchpoint crosses a page boundary
@ 2020-06-03 11:24 Alex Bennée
2020-06-03 12:46 ` Philippe Mathieu-Daudé
2020-06-03 16:12 ` Richard Henderson
0 siblings, 2 replies; 5+ messages in thread
From: Alex Bennée @ 2020-06-03 11:24 UTC (permalink / raw)
To: qemu-devel
Cc: Alexander Bulekov, Richard Henderson, Alex Bennée, Paolo Bonzini
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(-)
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;
--
2.20.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2] exec: flush the whole TLB if a watchpoint crosses a page boundary
2020-06-03 11:24 [PATCH v2] exec: flush the whole TLB if a watchpoint crosses a page boundary Alex Bennée
@ 2020-06-03 12:46 ` Philippe Mathieu-Daudé
2020-06-05 16:11 ` Philippe Mathieu-Daudé
2020-06-03 16:12 ` Richard Henderson
1 sibling, 1 reply; 5+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-06-03 12:46 UTC (permalink / raw)
To: Alex Bennée, qemu-devel
Cc: Alexander Bulekov, Paolo Bonzini, Richard Henderson
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;
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] exec: flush the whole TLB if a watchpoint crosses a page boundary
2020-06-03 11:24 [PATCH v2] exec: flush the whole TLB if a watchpoint crosses a page boundary Alex Bennée
2020-06-03 12:46 ` Philippe Mathieu-Daudé
@ 2020-06-03 16:12 ` Richard Henderson
1 sibling, 0 replies; 5+ messages in thread
From: Richard Henderson @ 2020-06-03 16:12 UTC (permalink / raw)
To: Alex Bennée, qemu-devel; +Cc: Alexander Bulekov, Paolo Bonzini
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~
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] exec: flush the whole TLB if a watchpoint crosses a page boundary
2020-06-03 12:46 ` Philippe Mathieu-Daudé
@ 2020-06-05 16:11 ` Philippe Mathieu-Daudé
2020-06-05 16:27 ` Alex Bennée
0 siblings, 1 reply; 5+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-06-05 16:11 UTC (permalink / raw)
To: Alex Bennée, qemu-devel
Cc: Alexander Bulekov, Paolo Bonzini, Richard Henderson
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;
>>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] exec: flush the whole TLB if a watchpoint crosses a page boundary
2020-06-05 16:11 ` Philippe Mathieu-Daudé
@ 2020-06-05 16:27 ` Alex Bennée
0 siblings, 0 replies; 5+ messages in thread
From: Alex Bennée @ 2020-06-05 16:27 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Alexander Bulekov, Paolo Bonzini, qemu-devel, Richard Henderson
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-06-05 16:29 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-03 11:24 [PATCH v2] exec: flush the whole TLB if a watchpoint crosses a page boundary Alex Bennée
2020-06-03 12:46 ` Philippe Mathieu-Daudé
2020-06-05 16:11 ` Philippe Mathieu-Daudé
2020-06-05 16:27 ` Alex Bennée
2020-06-03 16:12 ` Richard Henderson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).