Re: [Qemu-devel] [PATCH 04/13] Respect length of watchpoints

["Glauber Costa" <glommer@gmail.com>]

On Tue, Oct 14, 2008 at 7:12 AM, Jan Kiszka <jan.kiszka@siemens.com> 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 <jan.kiszka@siemens.com>
> ---
>  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."