QEMU-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: Richard Henderson <richard.henderson@linaro.org>
To: David Hildenbrand <david@redhat.com>, 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 09:09:51 -0700
Message-ID: <f08aa5d3-636f-7a48-659a-bd433e7a60b6@linaro.org> (raw)
In-Reply-To: <8df7d599-bc35-621e-c5d9-ac8554cba512@redhat.com>

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.

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.

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>


r~


  reply index

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 [this message]
2019-08-23 18:25       ` David Hildenbrand
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 publically 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=f08aa5d3-636f-7a48-659a-bd433e7a60b6@linaro.org \
    --to=richard.henderson@linaro.org \
    --cc=amarkovic@wavecomp.com \
    --cc=arikalo@wavecomp.com \
    --cc=aurelien@aurel32.net \
    --cc=cohuck@redhat.com \
    --cc=david@redhat.com \
    --cc=ehabkost@redhat.com \
    --cc=pbonzini@redhat.com \
    --cc=qemu-devel@nongnu.org \
    --cc=qemu-s390x@nongnu.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

QEMU-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/qemu-devel/0 qemu-devel/git/0.git
	git clone --mirror https://lore.kernel.org/qemu-devel/1 qemu-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 qemu-devel qemu-devel/ https://lore.kernel.org/qemu-devel \
		qemu-devel@nongnu.org
	public-inbox-index qemu-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.nongnu.qemu-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git