Re: [Qemu-devel] [PATCH 1/2] cpu: Add callback to check architectural watchpoint match

[Peter Maydell]

On 14 September 2015 at 11:50, Sergey Fedorov <address@hidden> wrote:
> When QEMU watchpoint matches, that is not definitely an architectural
> watchpoint match yet. If it is a stop-before-access watchpoint then that
> is hardly possible to ignore it after throwing a TCG exception.
>
> A special callback is introduced to check for architectural watchpoint
> match before raising a TCG exception.
>
> Signed-off-by: Sergey Fedorov <address@hidden>
> ---
>  exec.c            | 5 +++++
>  include/qom/cpu.h | 3 +++
>  qom/cpu.c         | 9 +++++++++
>  3 files changed, 17 insertions(+)
>
> diff --git a/exec.c b/exec.c
> index 54cd70a..64ed543 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1921,6 +1921,7 @@ static const MemoryRegionOps notdirty_mem_ops = {
>  static void check_watchpoint(int offset, int len, MemTxAttrs attrs, int 
> flags)
>  {
>      CPUState *cpu = current_cpu;
> +    CPUClass *cc = CPU_GET_CLASS(cpu);
>      CPUArchState *env = cpu->env_ptr;
>      target_ulong pc, cs_base;
>      target_ulong vaddr;
> @@ -1947,6 +1948,10 @@ static void check_watchpoint(int offset, int len, 
> MemTxAttrs attrs, int flags)
>              wp->hitattrs = attrs;
>              if (!cpu->watchpoint_hit) {
>                  cpu->watchpoint_hit = wp;
> +                if (wp->flags & BP_CPU && !cc->debug_check_watchpoint(cpu)) {
> +                    cpu->watchpoint_hit = NULL;
> +                    continue;
> +                }

Rather than setting cpu->watchpoint_hit, and then undoing it if the
"is this a non-matching CPU wp" check failed, it would be better
to just do the test before we assign to cpu->watchpoint_hit.
We can just pass the wp pointer through to the debug_check_watchpoint
hook in case the target needs it. (I think the ARM one at the
moment doesn't, though it would be slightly more efficient to check
only the wp in question, rather than all of them.)

>                  tb_check_watchpoint(cpu);
>                  if (wp->flags & BP_STOP_BEFORE_ACCESS) {
>                      cpu->exception_index = EXCP_DEBUG;
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index 39712ab..4e0a1b9 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -101,6 +101,8 @@ struct TranslationBlock;
>   * @get_phys_page_debug: Callback for obtaining a physical address.
>   * @gdb_read_register: Callback for letting GDB read a register.
>   * @gdb_write_register: Callback for letting GDB write a register.
> + * @debug_check_watchpoint: Callback for checking an architectural watchpoint
> + * match.

I think we could make this slightly more informative:
 * @debug_check_watchpoint: Callback: return true if the architectural
    watchpoint whose address has matched should really fire.

>   * @debug_excp_handler: Callback for handling debug exceptions.
>   * @write_elf64_note: Callback for writing a CPU-specific ELF note to a
>   * 64-bit VM coredump.
> @@ -155,6 +157,7 @@ typedef struct CPUClass {
>      hwaddr (*get_phys_page_debug)(CPUState *cpu, vaddr addr);
>      int (*gdb_read_register)(CPUState *cpu, uint8_t *buf, int reg);
>      int (*gdb_write_register)(CPUState *cpu, uint8_t *buf, int reg);
> +    bool (*debug_check_watchpoint)(CPUState *cpu);
>      void (*debug_excp_handler)(CPUState *cpu);

Otherwise looks good.

thanks
-- PMM