From: David Hildenbrand <david@redhat.com> To: Richard Henderson <richard.henderson@linaro.org>, qemu-devel@nongnu.org Cc: peter.maydell@linaro.org Subject: Re: [Qemu-devel] [PATCH 4/6] exec: Factor out cpu_watchpoint_address_matches Date: Mon, 26 Aug 2019 10:41:43 +0200 Message-ID: <77c9af3e-4c18-a20f-137e-468d04583241@redhat.com> (raw) In-Reply-To: <20190824213451.31118-5-richard.henderson@linaro.org> On 24.08.19 23:34, Richard Henderson wrote: > We want to move the check for watchpoints from > memory_region_section_get_iotlb to tlb_set_page_with_attrs. > Isolate the loop over watchpoints to an exported function. > > Rename the existing cpu_watchpoint_address_matches to > watchpoint_address_matches, since it doesn't actually > have a cpu argument. > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > include/hw/core/cpu.h | 7 +++++++ > exec.c | 45 ++++++++++++++++++++++++++++--------------- > 2 files changed, 36 insertions(+), 16 deletions(-) > > diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h > index 7bd8bed5b2..c7cda65c66 100644 > --- a/include/hw/core/cpu.h > +++ b/include/hw/core/cpu.h > @@ -1096,6 +1096,12 @@ static inline void cpu_check_watchpoint(CPUState *cpu, vaddr addr, vaddr len, > MemTxAttrs atr, int fl, uintptr_t ra) > { > } > + > +static inline int cpu_watchpoint_address_matches(CPUState *cpu, > + vaddr addr, vaddr len) > +{ > + return 0; > +} > #else > int cpu_watchpoint_insert(CPUState *cpu, vaddr addr, vaddr len, > int flags, CPUWatchpoint **watchpoint); > @@ -1105,6 +1111,7 @@ void cpu_watchpoint_remove_by_ref(CPUState *cpu, CPUWatchpoint *watchpoint); > void cpu_watchpoint_remove_all(CPUState *cpu, int mask); > void cpu_check_watchpoint(CPUState *cpu, vaddr addr, vaddr len, > MemTxAttrs attrs, int flags, uintptr_t ra); > +int cpu_watchpoint_address_matches(CPUState *cpu, vaddr addr, vaddr len); > #endif > > /** > diff --git a/exec.c b/exec.c > index cb6f5763dc..8575ce51ad 100644 > --- a/exec.c > +++ b/exec.c > @@ -1138,9 +1138,8 @@ void cpu_watchpoint_remove_all(CPUState *cpu, int mask) > * partially or completely with the address range covered by the > * access). > */ > -static inline bool cpu_watchpoint_address_matches(CPUWatchpoint *wp, > - vaddr addr, > - vaddr len) > +static inline bool watchpoint_address_matches(CPUWatchpoint *wp, > + vaddr addr, vaddr len) > { > /* We know the lengths are non-zero, but a little caution is > * required to avoid errors in the case where the range ends > @@ -1152,6 +1151,20 @@ static inline bool cpu_watchpoint_address_matches(CPUWatchpoint *wp, > > return !(addr > wpend || wp->vaddr > addrend); > } > + > +/* Return flags for watchpoints that match addr + prot. */ > +int cpu_watchpoint_address_matches(CPUState *cpu, vaddr addr, vaddr len) > +{ > + CPUWatchpoint *wp; > + int ret = 0; > + > + QTAILQ_FOREACH(wp, &cpu->watchpoints, entry) { > + if (watchpoint_address_matches(wp, addr, TARGET_PAGE_SIZE)) { > + ret |= wp->flags; > + } > + } > + return ret; > +} > #endif /* !CONFIG_USER_ONLY */ > > /* Add a breakpoint. */ > @@ -1459,7 +1472,7 @@ hwaddr memory_region_section_get_iotlb(CPUState *cpu, > target_ulong *address) > { > hwaddr iotlb; > - CPUWatchpoint *wp; > + int flags, match; > > if (memory_region_is_ram(section->mr)) { > /* Normal RAM. */ > @@ -1477,17 +1490,17 @@ hwaddr memory_region_section_get_iotlb(CPUState *cpu, > iotlb += xlat; > } > > - /* Make accesses to pages with watchpoints go via the > - watchpoint trap routines. */ > - QTAILQ_FOREACH(wp, &cpu->watchpoints, entry) { > - if (cpu_watchpoint_address_matches(wp, vaddr, TARGET_PAGE_SIZE)) { > - /* Avoid trapping reads of pages with a write breakpoint. */ > - if ((prot & PAGE_WRITE) || (wp->flags & BP_MEM_READ)) { > - iotlb = PHYS_SECTION_WATCH + paddr; > - *address |= TLB_MMIO; > - break; In the old code, we were able to break once we found a hit ... > - } > - } > + /* Avoid trapping reads of pages with a write breakpoint. */ > + match = (prot & PAGE_READ ? BP_MEM_READ : 0) > + | (prot & PAGE_WRITE ? BP_MEM_WRITE : 0); > + flags = cpu_watchpoint_address_matches(cpu, vaddr, TARGET_PAGE_SIZE); > + if (flags & match) { ... now you cannot break early anymore. Maybe pass in the match to cpu_watchpoint_address_matches() ? Anyhow, code seems to be correct, so Reviewed-by: David Hildenbrand <david@redhat.com> -- Thanks, David / dhildenb
next prev parent reply index Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-08-24 21:34 [Qemu-devel] [PATCH 0/6] exec: Cleanup watchpoints Richard Henderson 2019-08-24 21:34 ` [Qemu-devel] [PATCH 1/6] exec: Move user-only watchpoint stubs inline Richard Henderson 2019-08-26 7:45 ` David Hildenbrand 2019-08-24 21:34 ` [Qemu-devel] [PATCH 2/6] exec: Factor out core logic of check_watchpoint() Richard Henderson 2019-08-24 21:34 ` [Qemu-devel] [PATCH 3/6] cputlb: Fold TLB_RECHECK into TLB_INVALID_MASK Richard Henderson 2019-08-26 8:36 ` David Hildenbrand 2019-08-24 21:34 ` [Qemu-devel] [PATCH 4/6] exec: Factor out cpu_watchpoint_address_matches Richard Henderson 2019-08-26 8:41 ` David Hildenbrand [this message] 2019-08-28 21:28 ` Richard Henderson 2019-08-28 21:54 ` David Hildenbrand 2019-08-24 21:34 ` [Qemu-devel] [PATCH 5/6] cputlb: Handle watchpoints via TLB_WATCHPOINT Richard Henderson 2019-08-28 22:00 ` David Hildenbrand 2019-08-24 21:34 ` [Qemu-devel] [PATCH 6/6] tcg: Check for watchpoints in probe_write() Richard Henderson 2019-08-28 21:47 ` [Qemu-devel] [PATCH 0/6] exec: Cleanup watchpoints 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=77c9af3e-4c18-a20f-137e-468d04583241@redhat.com \ --to=david@redhat.com \ --cc=peter.maydell@linaro.org \ --cc=qemu-devel@nongnu.org \ --cc=richard.henderson@linaro.org \ /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 git clone --mirror https://lore.kernel.org/qemu-devel/2 qemu-devel/git/2.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