From: David Hildenbrand <david@redhat.com>
To: Richard Henderson <richard.henderson@linaro.org>, qemu-devel@nongnu.org
Cc: Thomas Huth <thuth@redhat.com>, Cornelia Huck <cohuck@redhat.com>,
Eduardo Habkost <ehabkost@redhat.com>,
Aleksandar Rikalo <arikalo@wavecomp.com>,
Riku Voipio <riku.voipio@iki.fi>,
qemu-s390x@nongnu.org,
Aleksandar Markovic <amarkovic@wavecomp.com>,
Paolo Bonzini <pbonzini@redhat.com>,
Aurelien Jarno <aurelien@aurel32.net>,
Richard Henderson <rth@twiddle.net>
Subject: Re: [Qemu-devel] [PATCH v1 8/9] exec.c: Factor out core logic of check_watchpoint()
Date: Fri, 23 Aug 2019 20:25:06 +0200 [thread overview]
Message-ID: <f3ede409-1f8d-13c5-f54e-c9b82104e724@redhat.com> (raw)
In-Reply-To: <f08aa5d3-636f-7a48-659a-bd433e7a60b6@linaro.org>
On 23.08.19 18:09, Richard Henderson wrote:
> On 8/23/19 4:27 AM, David Hildenbrand wrote:
>> On 23.08.19 12:07, David Hildenbrand wrote:
>>> We want to perform the same checks in probe_write() to trigger a cpu
>>> exit before doing any modifications. We'll have to pass a PC.
>>>
>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>> ---
>>> exec.c | 23 +++++++++++++++++------
>>> include/hw/core/cpu.h | 2 ++
>>> 2 files changed, 19 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/exec.c b/exec.c
>>> index 1df966d17a..d233a4250b 100644
>>> --- a/exec.c
>>> +++ b/exec.c
>>> @@ -2810,12 +2810,10 @@ static const MemoryRegionOps notdirty_mem_ops = {
>>> },
>>> };
>>>
>>> -/* Generate a debug exception if a watchpoint has been hit. */
>>> -static void check_watchpoint(int offset, int len, MemTxAttrs attrs, int flags)
>>> +void cpu_check_watchpoint(CPUState *cpu, vaddr vaddr, int len,
>>> + MemTxAttrs attrs, int flags, uintptr_t ra)
>>> {
>>> - CPUState *cpu = current_cpu;
>>> CPUClass *cc = CPU_GET_CLASS(cpu);
>>> - target_ulong vaddr;
>>> CPUWatchpoint *wp;
>>>
>>> assert(tcg_enabled());
>>> @@ -2826,7 +2824,7 @@ static void check_watchpoint(int offset, int len, MemTxAttrs attrs, int flags)
>>> cpu_interrupt(cpu, CPU_INTERRUPT_DEBUG);
>>> return;
>>> }
>>> - vaddr = (cpu->mem_io_vaddr & TARGET_PAGE_MASK) + offset;
>>> +
>>> vaddr = cc->adjust_watchpoint_address(cpu, vaddr, len);
>>> QTAILQ_FOREACH(wp, &cpu->watchpoints, entry) {
>>> if (cpu_watchpoint_address_matches(wp, vaddr, len)
>>> @@ -2851,11 +2849,14 @@ static void check_watchpoint(int offset, int len, MemTxAttrs attrs, int flags)
>>> if (wp->flags & BP_STOP_BEFORE_ACCESS) {
>>> cpu->exception_index = EXCP_DEBUG;
>>> mmap_unlock();
>>> - cpu_loop_exit(cpu);
>>> + cpu_loop_exit_restore(cpu, ra);
>>> } else {
>>> /* Force execution of one insn next time. */
>>> cpu->cflags_next_tb = 1 | curr_cflags();
>>> mmap_unlock();
>>> + if (ra) {
>>> + cpu_restore_state(cpu, ra, true);
>>> + }
>>> cpu_loop_exit_noexc(cpu);
>>> }
>>> }
>>> @@ -2865,6 +2866,16 @@ static void check_watchpoint(int offset, int len, MemTxAttrs attrs, int flags)
>>> }
>>> }
>>>
>>> +/* Generate a debug exception if a watchpoint has been hit. */
>>> +static void check_watchpoint(int offset, int len, MemTxAttrs attrs, int flags)
>>> +{
>>> + CPUState *cpu = current_cpu;
>>> + vaddr vaddr;
>>> +
>>> + vaddr = (cpu->mem_io_vaddr & TARGET_PAGE_MASK) + offset;
>>> + cpu_check_watchpoint(cpu, vaddr, len, attrs, flags, 0);
>>> +}
>>> +
>>> /* Watchpoint access routines. Watchpoints are inserted using TLB tricks,
>>> so these check for a hit then pass through to the normal out-of-line
>>> phys routines. */
>>> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
>>> index 77fca95a40..3a2d76b32c 100644
>>> --- a/include/hw/core/cpu.h
>>> +++ b/include/hw/core/cpu.h
>>> @@ -1070,6 +1070,8 @@ static inline bool cpu_breakpoint_test(CPUState *cpu, vaddr pc, int mask)
>>> return false;
>>> }
>>>
>>> +void cpu_check_watchpoint(CPUState *cpu, vaddr vaddr, int len,
>>> + MemTxAttrs attrs, int flags, uintptr_t ra);
>>> int cpu_watchpoint_insert(CPUState *cpu, vaddr addr, vaddr len,
>>> int flags, CPUWatchpoint **watchpoint);
>>> int cpu_watchpoint_remove(CPUState *cpu, vaddr addr,
>>>
>>
>> As we will have bigger accesses with probe_write(), we should do
>>
>> diff --git a/exec.c b/exec.c
>> index d233a4250b..4f8cc62a5f 100644
>> --- a/exec.c
>> +++ b/exec.c
>> @@ -2834,7 +2834,7 @@ void cpu_check_watchpoint(CPUState *cpu, vaddr
>> vaddr, int len,
>> } else {
>> wp->flags |= BP_WATCHPOINT_HIT_WRITE;
>> }
>> - wp->hitaddr = vaddr;
>> + wp->hitaddr = MAX(vaddr, wp->vaddr);
>> wp->hitattrs = attrs;
>> if (!cpu->watchpoint_hit) {
>> if (wp->flags & BP_CPU &&
>>
>> I guess, to make sure we actually indicate the watchpoint.
>
> Yes, that looks right.
>
> As for your changes to use cpu_loop_exit_restore... Those are so right that I
> didn't even recognize how wrong this code is when I was looking through it the
> other day. Watchpoints must not actually be working at all at the moment, really.
We keep finding surprises ... guess we payed the wrong area of QEMU src
code a visit :)
>
> I suspect that we need to use a page flag for this and not use I/O memory at
> all. Or convert to read/write_with_attrs and use magic MemTxResult values, but
> that seems sketchier than page flags. Either way is the only way that we can
> get access to the host return address so that we can unwind and return to the
> main loop.
I wonder if we can pass through the RA from the callers somehow. But I
have to admit, how the whole MemOps (+MemTX) stuff is fits together is
not yet 100% clear to me.
>
> But this is a good step, in the right direction. We'll fix the rest later.
>
> With the MAX,
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>
Thanks!
I'll fixup the smaller things and resend next week. I already have
another set of patches (e.g., probe_read() and return void* from
probe_write() lying around). The s390x part is still in the works.
--
Thanks,
David / dhildenb
next prev parent reply other threads:[~2019-08-23 18:26 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-08-23 10:07 [Qemu-devel] [PATCH v1 0/9] tcg: probe_write() refactorings and watchpoints David Hildenbrand
2019-08-23 10:07 ` [Qemu-devel] [PATCH v1 1/9] s390x/tcg: Use guest_addr_valid() instead of h2g_valid() in probe_write_access() David Hildenbrand
2019-08-23 14:49 ` Richard Henderson
2019-08-23 10:07 ` [Qemu-devel] [PATCH v1 2/9] s390x/tcg: Fix length calculation " David Hildenbrand
2019-08-23 14:50 ` Richard Henderson
2019-08-23 10:07 ` [Qemu-devel] [PATCH v1 3/9] tcg: Factor out CONFIG_USER_ONLY probe_write() from s390x code David Hildenbrand
2019-08-23 15:17 ` Richard Henderson
2019-08-23 15:22 ` Richard Henderson
2019-08-23 15:49 ` David Hildenbrand
2019-08-23 10:07 ` [Qemu-devel] [PATCH v1 4/9] tcg: Enforce single page access in probe_write() for !CONFIG_USER_ONLY David Hildenbrand
2019-08-23 15:28 ` Richard Henderson
2019-08-23 15:29 ` Richard Henderson
2019-08-23 10:07 ` [Qemu-devel] [PATCH v1 5/9] mips/tcg: Call probe_write() for CONFIG_USER_ONLY as well David Hildenbrand
2019-08-23 10:07 ` [Qemu-devel] [PATCH v1 6/9] hppa/tcg: Call probe_write() also for CONFIG_USER_ONLY David Hildenbrand
2019-08-23 15:31 ` Richard Henderson
2019-08-23 10:07 ` [Qemu-devel] [PATCH v1 7/9] s390x/tcg: Pass a size to probe_write() in do_csst() David Hildenbrand
2019-08-23 15:32 ` Richard Henderson
2019-08-23 10:07 ` [Qemu-devel] [PATCH v1 8/9] exec.c: Factor out core logic of check_watchpoint() David Hildenbrand
2019-08-23 11:27 ` David Hildenbrand
2019-08-23 16:09 ` Richard Henderson
2019-08-23 18:25 ` David Hildenbrand [this message]
2019-08-24 15:27 ` Richard Henderson
2019-08-23 10:07 ` [Qemu-devel] [PATCH v1 9/9] tcg: Check for watchpoints in probe_write() David Hildenbrand
2019-08-23 16:15 ` Richard Henderson
2019-08-24 19:45 ` Richard Henderson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=f3ede409-1f8d-13c5-f54e-c9b82104e724@redhat.com \
--to=david@redhat.com \
--cc=amarkovic@wavecomp.com \
--cc=arikalo@wavecomp.com \
--cc=aurelien@aurel32.net \
--cc=cohuck@redhat.com \
--cc=ehabkost@redhat.com \
--cc=pbonzini@redhat.com \
--cc=qemu-devel@nongnu.org \
--cc=qemu-s390x@nongnu.org \
--cc=richard.henderson@linaro.org \
--cc=riku.voipio@iki.fi \
--cc=rth@twiddle.net \
--cc=thuth@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).