Re: [Qemu-devel] [PATCH 13/14] target-arm: Use attribute info to handle user-only watchpoints

[Edgar E. Iglesias]

On Tue, Apr 07, 2015 at 09:09:59PM +0100, Peter Maydell wrote:
> Now that we have memory access attribute information in the watchpoint
> checking code, we can correctly implement handling of watchpoints
> which should match only on userspace accesses, where LDRT/STRT/LDT/STT
> from EL1 are treated as userspace accesses.
> 
> Signed-off-by: Peter Maydell <address@hidden>

Reviewed-by: Edgar E. Iglesias <address@hidden>


> ---
>  target-arm/op_helper.c | 23 ++++++++++++-----------
>  1 file changed, 12 insertions(+), 11 deletions(-)
> 
> diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
> index 7713022..ce09ab3 100644
> --- a/target-arm/op_helper.c
> +++ b/target-arm/op_helper.c
> @@ -602,13 +602,22 @@ static bool bp_wp_matches(ARMCPU *cpu, int n, bool 
> is_wp)
>      int pac, hmc, ssc, wt, lbn;
>      /* TODO: check against CPU security state when we implement TrustZone */
>      bool is_secure = false;
> +    int access_el = arm_current_el(env);
>  
>      if (is_wp) {
> -        if (!env->cpu_watchpoint[n]
> -            || !(env->cpu_watchpoint[n]->flags & BP_WATCHPOINT_HIT)) {
> +        CPUWatchpoint *wp = env->cpu_watchpoint[n];
> +
> +        if (!wp || !(wp->flags & BP_WATCHPOINT_HIT)) {
>              return false;
>          }
>          cr = env->cp15.dbgwcr[n];
> +        if (wp->hitattrs & MEMTXATTRS_USER) {
> +            /* The LDRT/STRT/LDT/STT "unprivileged access" instructions 
> should
> +             * match watchpoints as if they were accesses done at EL0, even 
> if
> +             * the CPU is at EL1 or higher.
> +             */
> +            access_el = 0;
> +        }
>      } else {
>          uint64_t pc = is_a64(env) ? env->pc : env->regs[15];
>  
> @@ -649,15 +658,7 @@ static bool bp_wp_matches(ARMCPU *cpu, int n, bool is_wp)
>          break;
>      }
>  
> -    /* TODO: this is not strictly correct because the LDRT/STRT/LDT/STT
> -     * "unprivileged access" instructions should match watchpoints as if
> -     * they were accesses done at EL0, even if the CPU is at EL1 or higher.
> -     * Implementing this would require reworking the core watchpoint code
> -     * to plumb the mmu_idx through to this point. Luckily Linux does not
> -     * rely on this behaviour currently.
> -     * For breakpoints we do want to use the current CPU state.
> -     */
> -    switch (arm_current_el(env)) {
> +    switch (access_el) {
>      case 3:
>      case 2:
>          if (!hmc) {
> -- 
> 1.9.1
>