Re: [Qemu-devel] [PATCH v2 08/14] exec.c: Capture the memory attributes for a watchpoint hit

[Alex Bennée]

Peter Maydell <address@hidden> writes:

> Capture the memory attributes for the transaction which triggered
> a watchpoint; this allows CPU specific code to implement features
> like ARM's "user-mode only WPs also hit for LDRT/STRT accesses
> made from privileged code". This change also correctly passes
> through the memory attributes to the underlying device when
> a watchpoint access doesn't hit.
>
> Signed-off-by: Peter Maydell <address@hidden>
Reviewed-by: Alex Bennée <address@hidden>

> ---
>  exec.c            | 46 +++++++++++++++++++++++++++++++---------------
>  include/qom/cpu.h |  2 ++
>  2 files changed, 33 insertions(+), 15 deletions(-)
>
> diff --git a/exec.c b/exec.c
> index 399543e..53d59bb 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1858,7 +1858,7 @@ static const MemoryRegionOps notdirty_mem_ops = {
>  };
>  
>  /* Generate a debug exception if a watchpoint has been hit.  */
> -static void check_watchpoint(int offset, int len, int flags)
> +static void check_watchpoint(int offset, int len, MemTxAttrs attrs, int 
> flags)
>  {
>      CPUState *cpu = current_cpu;
>      CPUArchState *env = cpu->env_ptr;
> @@ -1884,6 +1884,7 @@ static void check_watchpoint(int offset, int len, int 
> flags)
>                  wp->flags |= BP_WATCHPOINT_HIT_WRITE;
>              }
>              wp->hitaddr = vaddr;
> +            wp->hitattrs = attrs;
>              if (!cpu->watchpoint_hit) {
>                  cpu->watchpoint_hit = wp;
>                  tb_check_watchpoint(cpu);
> @@ -1905,39 +1906,54 @@ static void check_watchpoint(int offset, int len, int 
> flags)
>  /* 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.  */
> -static uint64_t watch_mem_read(void *opaque, hwaddr addr,
> -                               unsigned size)
> +static MemTxResult watch_mem_read(void *opaque, hwaddr addr, uint64_t *pdata,
> +                                  unsigned size, MemTxAttrs attrs)
>  {
> -    check_watchpoint(addr & ~TARGET_PAGE_MASK, size, BP_MEM_READ);
> +    MemTxResult res;
> +    uint64_t data;
> +
> +    check_watchpoint(addr & ~TARGET_PAGE_MASK, size, attrs, BP_MEM_READ);
>      switch (size) {
> -    case 1: return ldub_phys(&address_space_memory, addr);
> -    case 2: return lduw_phys(&address_space_memory, addr);
> -    case 4: return ldl_phys(&address_space_memory, addr);
> +    case 1:
> +        data = address_space_ldub(&address_space_memory, addr, attrs, &res);
> +        break;
> +    case 2:
> +        data = address_space_lduw(&address_space_memory, addr, attrs, &res);
> +        break;
> +    case 4:
> +        data = address_space_ldl(&address_space_memory, addr, attrs, &res);
> +        break;
>      default: abort();
>      }
> +    *pdata = data;
> +    return res;
>  }
>  
> -static void watch_mem_write(void *opaque, hwaddr addr,
> -                            uint64_t val, unsigned size)
> +static MemTxResult watch_mem_write(void *opaque, hwaddr addr,
> +                                   uint64_t val, unsigned size,
> +                                   MemTxAttrs attrs)
>  {
> -    check_watchpoint(addr & ~TARGET_PAGE_MASK, size, BP_MEM_WRITE);
> +    MemTxResult res;
> +
> +    check_watchpoint(addr & ~TARGET_PAGE_MASK, size, attrs, BP_MEM_WRITE);
>      switch (size) {
>      case 1:
> -        stb_phys(&address_space_memory, addr, val);
> +        address_space_stb(&address_space_memory, addr, val, attrs, &res);
>          break;
>      case 2:
> -        stw_phys(&address_space_memory, addr, val);
> +        address_space_stw(&address_space_memory, addr, val, attrs, &res);
>          break;
>      case 4:
> -        stl_phys(&address_space_memory, addr, val);
> +        address_space_stl(&address_space_memory, addr, val, attrs, &res);
>          break;
>      default: abort();
>      }
> +    return res;
>  }
>  
>  static const MemoryRegionOps watch_mem_ops = {
> -    .read = watch_mem_read,
> -    .write = watch_mem_write,
> +    .read_with_attrs = watch_mem_read,
> +    .write_with_attrs = watch_mem_write,
>      .endianness = DEVICE_NATIVE_ENDIAN,
>  };
>  
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index 9dafb48..39f0f19 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -24,6 +24,7 @@
>  #include <setjmp.h>
>  #include "hw/qdev-core.h"
>  #include "exec/hwaddr.h"
> +#include "exec/memattrs.h"
>  #include "qemu/queue.h"
>  #include "qemu/thread.h"
>  #include "qemu/tls.h"
> @@ -195,6 +196,7 @@ typedef struct CPUWatchpoint {
>      vaddr vaddr;
>      vaddr len;
>      vaddr hitaddr;
> +    MemTxAttrs hitattrs;
>      int flags; /* BP_* */
>      QTAILQ_ENTRY(CPUWatchpoint) entry;
>  } CPUWatchpoint;

-- 
Alex Bennée