From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1Kpo22-0003AI-RL for qemu-devel@nongnu.org; Tue, 14 Oct 2008 13:50:06 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1Kpo22-00039j-0H for qemu-devel@nongnu.org; Tue, 14 Oct 2008 13:50:06 -0400 Received: from [199.232.76.173] (port=59637 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Kpo21-00039W-Mx for qemu-devel@nongnu.org; Tue, 14 Oct 2008 13:50:05 -0400 Received: from yx-out-1718.google.com ([74.125.44.157]:62925) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1Kpo21-0008Cf-5X for qemu-devel@nongnu.org; Tue, 14 Oct 2008 13:50:05 -0400 Received: by yx-out-1718.google.com with SMTP id 3so484976yxi.82 for ; Tue, 14 Oct 2008 10:50:04 -0700 (PDT) Message-ID: <5d6222a80810141050p1de281e1r7738434b7127d638@mail.gmail.com> Date: Tue, 14 Oct 2008 15:50:03 -0200 From: "Glauber Costa" Subject: Re: [Qemu-devel] [PATCH 04/13] Respect length of watchpoints In-Reply-To: <20081014091225.115644848@mchn012c.ww002.siemens.net> MIME-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Content-Disposition: inline References: <20081014091223.932366369@mchn012c.ww002.siemens.net> <20081014091225.115644848@mchn012c.ww002.siemens.net> Reply-To: qemu-devel@nongnu.org List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org Cc: Jan Kiszka On Tue, Oct 14, 2008 at 7:12 AM, Jan Kiszka wrote: > This adds length support for watchpoints. To keep things simple, only > aligned watchpoints are accepted. why? It does not seem that much complicated to handle unaligned watchpoints. Unless I'm totally wrong, we should just store the value as-is, and then check for it. As a matter of fact, because we're masking and testing for the mask, it seems even more complicated to require that. I agree a full aligned world would be a happier world, but unfortunately, unaligned accesses are quite common in x86. > > Signed-off-by: Jan Kiszka > --- > cpu-defs.h | 2 +- > exec.c | 30 ++++++++++++++++++++---------- > 2 files changed, 21 insertions(+), 11 deletions(-) > > Index: b/exec.c > =================================================================== > --- a/exec.c > +++ b/exec.c > @@ -1313,14 +1313,21 @@ static void breakpoint_invalidate(CPUSta > int cpu_watchpoint_insert(CPUState *env, target_ulong addr, target_ulong len, > int flags, CPUWatchpoint **watchpoint) > { > + target_ulong len_mask = ~(len - 1); > CPUWatchpoint *wp; > > + /* sanity checks: allow power-of-2 lengths, deny unaligned watchpoints */ > + if ((len != 1 && len != 2 && len != 4 && len != 8) || (addr & ~len_mask)) { > + fprintf(stderr, "qemu: tried to set invalid watchpoint at " > + TARGET_FMT_lx ", len=" TARGET_FMT_lu "\n", addr, len); > + return -EINVAL; > + } > wp = qemu_malloc(sizeof(*wp)); > if (!wp) > return -ENOBUFS; > > wp->vaddr = addr; > - wp->len = len; > + wp->len_mask = len_mask; > wp->flags = flags; > > wp->next = env->watchpoints; > @@ -1344,10 +1351,12 @@ int cpu_watchpoint_insert(CPUState *env, > int cpu_watchpoint_remove(CPUState *env, target_ulong addr, target_ulong len, > int flags) > { > + target_ulong len_mask = ~(len - 1); > CPUWatchpoint *wp; > > for (wp = env->watchpoints; wp != NULL; wp = wp->next) { > - if (addr == wp->vaddr && len == wp->len && flags == wp->flags) { > + if (addr == wp->vaddr && len_mask == wp->len_mask > + && flags == wp->flags) { > cpu_watchpoint_remove_by_ref(env, wp); > return 0; > } > @@ -2502,7 +2511,7 @@ static CPUWriteMemoryFunc *notdirty_mem_ > }; > > /* Generate a debug exception if a watchpoint has been hit. */ > -static void check_watchpoint(int offset, int flags) > +static void check_watchpoint(int offset, int len_mask, int flags) > { > CPUState *env = cpu_single_env; > target_ulong vaddr; > @@ -2510,7 +2519,8 @@ static void check_watchpoint(int offset, > > vaddr = (env->mem_io_vaddr & TARGET_PAGE_MASK) + offset; > for (wp = env->watchpoints; wp != NULL; wp = wp->next) { > - if (vaddr == wp->vaddr && (wp->flags & flags)) { > + if ((vaddr == (wp->vaddr & len_mask) || > + (vaddr & wp->len_mask) == wp->vaddr) && (wp->flags & flags)) { > env->watchpoint_hit = wp; > cpu_interrupt(env, CPU_INTERRUPT_DEBUG); > break; > @@ -2523,40 +2533,40 @@ static void check_watchpoint(int offset, > phys routines. */ > static uint32_t watch_mem_readb(void *opaque, target_phys_addr_t addr) > { > - check_watchpoint(addr & ~TARGET_PAGE_MASK, BP_MEM_READ); > + check_watchpoint(addr & ~TARGET_PAGE_MASK, ~0x0, BP_MEM_READ); > return ldub_phys(addr); > } > > static uint32_t watch_mem_readw(void *opaque, target_phys_addr_t addr) > { > - check_watchpoint(addr & ~TARGET_PAGE_MASK, BP_MEM_READ); > + check_watchpoint(addr & ~TARGET_PAGE_MASK, ~0x1, BP_MEM_READ); > return lduw_phys(addr); > } > > static uint32_t watch_mem_readl(void *opaque, target_phys_addr_t addr) > { > - check_watchpoint(addr & ~TARGET_PAGE_MASK, BP_MEM_READ); > + check_watchpoint(addr & ~TARGET_PAGE_MASK, ~0x3, BP_MEM_READ); > return ldl_phys(addr); > } > > static void watch_mem_writeb(void *opaque, target_phys_addr_t addr, > uint32_t val) > { > - check_watchpoint(addr & ~TARGET_PAGE_MASK, BP_MEM_WRITE); > + check_watchpoint(addr & ~TARGET_PAGE_MASK, ~0x0, BP_MEM_WRITE); > stb_phys(addr, val); > } > > static void watch_mem_writew(void *opaque, target_phys_addr_t addr, > uint32_t val) > { > - check_watchpoint(addr & ~TARGET_PAGE_MASK, BP_MEM_WRITE); > + check_watchpoint(addr & ~TARGET_PAGE_MASK, ~0x1, BP_MEM_WRITE); > stw_phys(addr, val); > } > > static void watch_mem_writel(void *opaque, target_phys_addr_t addr, > uint32_t val) > { > - check_watchpoint(addr & ~TARGET_PAGE_MASK, BP_MEM_WRITE); > + check_watchpoint(addr & ~TARGET_PAGE_MASK, ~0x3, BP_MEM_WRITE); > stl_phys(addr, val); > } > > Index: b/cpu-defs.h > =================================================================== > --- a/cpu-defs.h > +++ b/cpu-defs.h > @@ -148,7 +148,7 @@ typedef struct CPUBreakpoint { > > typedef struct CPUWatchpoint { > target_ulong vaddr; > - target_ulong len; > + target_ulong len_mask; > int flags; /* BP_* */ > struct CPUWatchpoint *prev, *next; > } CPUWatchpoint; It's less confusing if you call it len_mask from the beginning, instead of changing your own patch for that purpose. > > > -- Glauber Costa. "Free as in Freedom" http://glommer.net "The less confident you are, the more serious you have to act."