* [Qemu-devel] [PATCH v3 0/2] Architectural watchpoint check
@ 2016-01-31 16:15 Sergey Fedorov
2016-01-31 16:15 ` [Qemu-devel] [PATCH v3 1/2] cpu: Add callback to check architectural watchpoint match Sergey Fedorov
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Sergey Fedorov @ 2016-01-31 16:15 UTC (permalink / raw)
To: qemu-devel, qemu-arm
Cc: Peter Maydell, Peter Crosthwaite, Paolo Bonzini, Sergey Fedorov,
Andreas Färber, Richard Henderson
This series is intended to fix ARM watchpoint emulation misbehavior.
QEMU hangs when QEMU watchpoint fires but it does not pass additional
architectural checks in ARM CPU debug exception handler. For details,
please see individual patches. The most relevant parts of the original
discussion about ARM breakpoint and watchpoint emulation misbehavior can be
found at:
https://lists.gnu.org/archive/html/qemu-devel/2015-08/msg02715.html
https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg00527.html
Changes in v2:
* Check moved before setting cpu->watchpoint_hit
* Pointer to watchpoint being checked passed to debug_check_watchpoint()
callback
* BP_WATCHPOINT_HIT flag cleared from flags from wp->flags in no-fire case
* Comment for debug_check_watchpoint() callback improved
Sergey Fedorov (2):
cpu: Add callback to check architectural watchpoint match
target-arm: Implement checking of fired watchpoint
exec.c | 6 ++++++
include/qom/cpu.h | 4 ++++
qom/cpu.c | 9 +++++++++
target-arm/cpu.c | 1 +
target-arm/internals.h | 3 +++
target-arm/op_helper.c | 35 +++++++++++++++++++++--------------
6 files changed, 44 insertions(+), 14 deletions(-)
--
1.9.1
^ permalink raw reply [flat|nested] 7+ messages in thread* [Qemu-devel] [PATCH v3 1/2] cpu: Add callback to check architectural watchpoint match 2016-01-31 16:15 [Qemu-devel] [PATCH v3 0/2] Architectural watchpoint check Sergey Fedorov @ 2016-01-31 16:15 ` Sergey Fedorov 2016-02-02 12:23 ` Peter Maydell 2016-01-31 16:15 ` [Qemu-devel] [PATCH v3 2/2] target-arm: Implement checking of fired watchpoint Sergey Fedorov 2016-02-08 15:57 ` [Qemu-devel] [PATCH v3 0/2] Architectural watchpoint check Peter Maydell 2 siblings, 1 reply; 7+ messages in thread From: Sergey Fedorov @ 2016-01-31 16:15 UTC (permalink / raw) To: qemu-devel, qemu-arm Cc: Peter Maydell, Peter Crosthwaite, Paolo Bonzini, Sergey Fedorov, Andreas Färber, Richard Henderson 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 <serge.fdrv@gmail.com> --- exec.c | 6 ++++++ include/qom/cpu.h | 4 ++++ qom/cpu.c | 9 +++++++++ 3 files changed, 19 insertions(+) diff --git a/exec.c b/exec.c index 9e076bc..14e7c76 100644 --- a/exec.c +++ b/exec.c @@ -2024,6 +2024,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; @@ -2049,6 +2050,11 @@ static void check_watchpoint(int offset, int len, MemTxAttrs attrs, int flags) wp->hitaddr = vaddr; wp->hitattrs = attrs; if (!cpu->watchpoint_hit) { + if (wp->flags & BP_CPU && + !cc->debug_check_watchpoint(cpu, wp)) { + wp->flags &= ~BP_WATCHPOINT_HIT; + continue; + } cpu->watchpoint_hit = wp; tb_check_watchpoint(cpu); if (wp->flags & BP_STOP_BEFORE_ACCESS) { diff --git a/include/qom/cpu.h b/include/qom/cpu.h index 035179c..984bc8d 100644 --- a/include/qom/cpu.h +++ b/include/qom/cpu.h @@ -64,6 +64,7 @@ typedef uint64_t vaddr; #define CPU_GET_CLASS(obj) OBJECT_GET_CLASS(CPUClass, (obj), TYPE_CPU) typedef struct CPUState CPUState; +typedef struct CPUWatchpoint CPUWatchpoint; typedef void (*CPUUnassignedAccess)(CPUState *cpu, hwaddr addr, bool is_write, bool is_exec, int opaque, @@ -106,6 +107,8 @@ struct TranslationBlock; * a memory access with the specified memory transaction attributes. * @gdb_read_register: Callback for letting GDB read a register. * @gdb_write_register: Callback for letting GDB write a register. + * @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. @@ -165,6 +168,7 @@ typedef struct CPUClass { int (*asidx_from_attrs)(CPUState *cpu, MemTxAttrs attrs); 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, CPUWatchpoint *wp); void (*debug_excp_handler)(CPUState *cpu); int (*write_elf64_note)(WriteCoreDumpFunction f, CPUState *cpu, diff --git a/qom/cpu.c b/qom/cpu.c index 8f537a4..5a6a47e 100644 --- a/qom/cpu.c +++ b/qom/cpu.c @@ -188,6 +188,14 @@ static int cpu_common_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg) return 0; } +static bool cpu_common_debug_check_watchpoint(CPUState *cpu, CPUWatchpoint *wp) +{ + /* If no extra check is required, QEMU watchpoint match can be considered + * as an architectural match. + */ + return true; +} + bool target_words_bigendian(void); static bool cpu_common_virtio_is_big_endian(CPUState *cpu) { @@ -352,6 +360,7 @@ static void cpu_class_init(ObjectClass *klass, void *data) k->gdb_write_register = cpu_common_gdb_write_register; k->virtio_is_big_endian = cpu_common_virtio_is_big_endian; k->debug_excp_handler = cpu_common_noop; + k->debug_check_watchpoint = cpu_common_debug_check_watchpoint; k->cpu_exec_enter = cpu_common_noop; k->cpu_exec_exit = cpu_common_noop; k->cpu_exec_interrupt = cpu_common_exec_interrupt; -- 1.9.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/2] cpu: Add callback to check architectural watchpoint match 2016-01-31 16:15 ` [Qemu-devel] [PATCH v3 1/2] cpu: Add callback to check architectural watchpoint match Sergey Fedorov @ 2016-02-02 12:23 ` Peter Maydell 2016-02-08 15:42 ` Peter Maydell 0 siblings, 1 reply; 7+ messages in thread From: Peter Maydell @ 2016-02-02 12:23 UTC (permalink / raw) To: Sergey Fedorov Cc: Peter Crosthwaite, QEMU Developers, qemu-arm, Paolo Bonzini, Andreas Färber, Richard Henderson On 31 January 2016 at 16:15, Sergey Fedorov <serge.fdrv@gmail.com> 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 <serge.fdrv@gmail.com> This looks OK to me, but there's one QOM CPU style question I'd like Andreas's view on. > @@ -2024,6 +2024,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; > @@ -2049,6 +2050,11 @@ static void check_watchpoint(int offset, int len, MemTxAttrs attrs, int flags) > wp->hitaddr = vaddr; > wp->hitattrs = attrs; > if (!cpu->watchpoint_hit) { > + if (wp->flags & BP_CPU && > + !cc->debug_check_watchpoint(cpu, wp)) { At least some of the QOM CPU methods have wrapper functions (eg cpu_set_pc(), cpu_unaligned_access()) that just bundle up the CPU_GET_CLASS and method invocation). Should new methods like the debug_check_watchpoint() introduced by this patch have that kind of wrapper function, or is it optional? (There also seems to be a mix of "implement default/common behaviour in a common method implementation" vs "implement it in the wrapper function if the method pointer is NULL". I think the former, as done in this patch, is probably nicer but again would defer to Andreas.) thanks -- PMM ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/2] cpu: Add callback to check architectural watchpoint match 2016-02-02 12:23 ` Peter Maydell @ 2016-02-08 15:42 ` Peter Maydell 2016-02-08 15:57 ` Peter Maydell 0 siblings, 1 reply; 7+ messages in thread From: Peter Maydell @ 2016-02-08 15:42 UTC (permalink / raw) To: Sergey Fedorov Cc: Peter Crosthwaite, QEMU Developers, qemu-arm, Paolo Bonzini, Andreas Färber, Richard Henderson Andreas -- can I nudge you to say your preferences on whether QOM CPU methods should have wrapper functions, please? I think this patchset is otherwise ready to apply. thanks -- PMM On 2 February 2016 at 12:23, Peter Maydell <peter.maydell@linaro.org> wrote: > On 31 January 2016 at 16:15, Sergey Fedorov <serge.fdrv@gmail.com> 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 <serge.fdrv@gmail.com> > > This looks OK to me, but there's one QOM CPU style question > I'd like Andreas's view on. > >> @@ -2024,6 +2024,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; >> @@ -2049,6 +2050,11 @@ static void check_watchpoint(int offset, int len, MemTxAttrs attrs, int flags) >> wp->hitaddr = vaddr; >> wp->hitattrs = attrs; >> if (!cpu->watchpoint_hit) { >> + if (wp->flags & BP_CPU && >> + !cc->debug_check_watchpoint(cpu, wp)) { > > At least some of the QOM CPU methods have wrapper functions > (eg cpu_set_pc(), cpu_unaligned_access()) that just bundle up > the CPU_GET_CLASS and method invocation). Should new methods > like the debug_check_watchpoint() introduced by this patch have > that kind of wrapper function, or is it optional? > > (There also seems to be a mix of "implement default/common > behaviour in a common method implementation" vs "implement > it in the wrapper function if the method pointer is NULL". > I think the former, as done in this patch, is probably nicer > but again would defer to Andreas.) > > thanks > -- PMM ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/2] cpu: Add callback to check architectural watchpoint match 2016-02-08 15:42 ` Peter Maydell @ 2016-02-08 15:57 ` Peter Maydell 0 siblings, 0 replies; 7+ messages in thread From: Peter Maydell @ 2016-02-08 15:57 UTC (permalink / raw) To: Sergey Fedorov Cc: Peter Crosthwaite, QEMU Developers, qemu-arm, Paolo Bonzini, Andreas Färber, Richard Henderson On 8 February 2016 at 15:42, Peter Maydell <peter.maydell@linaro.org> wrote: > Andreas -- can I nudge you to say your preferences on whether > QOM CPU methods should have wrapper functions, please? > I think this patchset is otherwise ready to apply. Andreas tells me on IRC that he doesn't insist on wrapper functions, so I think this patch is fine as is. thanks -- PMM ^ permalink raw reply [flat|nested] 7+ messages in thread
* [Qemu-devel] [PATCH v3 2/2] target-arm: Implement checking of fired watchpoint 2016-01-31 16:15 [Qemu-devel] [PATCH v3 0/2] Architectural watchpoint check Sergey Fedorov 2016-01-31 16:15 ` [Qemu-devel] [PATCH v3 1/2] cpu: Add callback to check architectural watchpoint match Sergey Fedorov @ 2016-01-31 16:15 ` Sergey Fedorov 2016-02-08 15:57 ` [Qemu-devel] [PATCH v3 0/2] Architectural watchpoint check Peter Maydell 2 siblings, 0 replies; 7+ messages in thread From: Sergey Fedorov @ 2016-01-31 16:15 UTC (permalink / raw) To: qemu-devel, qemu-arm Cc: Peter Maydell, Peter Crosthwaite, Paolo Bonzini, Sergey Fedorov, Andreas Färber, Richard Henderson ARM stops before access to a location covered by watchpoint. Also, QEMU watchpoint fire is not necessarily an architectural watchpoint match. Unfortunately, that is hardly possible to ignore a fired watchpoint in debug exception handler. So move watchpoint check from debug exception handler to the dedicated watchpoint checking callback. Signed-off-by: Sergey Fedorov <serge.fdrv@gmail.com> Reviewed-by: Peter Maydell <peter.maydell@linaro.org> --- target-arm/cpu.c | 1 + target-arm/internals.h | 3 +++ target-arm/op_helper.c | 35 +++++++++++++++++++++-------------- 3 files changed, 25 insertions(+), 14 deletions(-) diff --git a/target-arm/cpu.c b/target-arm/cpu.c index 0e582c4..21ec18e 100644 --- a/target-arm/cpu.c +++ b/target-arm/cpu.c @@ -1474,6 +1474,7 @@ static void arm_cpu_class_init(ObjectClass *oc, void *data) cc->gdb_arch_name = arm_gdb_arch_name; cc->gdb_stop_before_watchpoint = true; cc->debug_excp_handler = arm_debug_excp_handler; + cc->debug_check_watchpoint = arm_debug_check_watchpoint; cc->disas_set_info = arm_disas_set_info; diff --git a/target-arm/internals.h b/target-arm/internals.h index d226bbe..16d9487 100644 --- a/target-arm/internals.h +++ b/target-arm/internals.h @@ -409,6 +409,9 @@ void hw_breakpoint_update(ARMCPU *cpu, int n); */ void hw_breakpoint_update_all(ARMCPU *cpu); +/* Callback function for checking if a watchpoint should trigger. */ +bool arm_debug_check_watchpoint(CPUState *cs, CPUWatchpoint *wp); + /* Callback function for when a watchpoint or breakpoint triggers. */ void arm_debug_excp_handler(CPUState *cs); diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c index a5ee65f..4adf9cc 100644 --- a/target-arm/op_helper.c +++ b/target-arm/op_helper.c @@ -975,6 +975,16 @@ void HELPER(check_breakpoints)(CPUARMState *env) } } +bool arm_debug_check_watchpoint(CPUState *cs, CPUWatchpoint *wp) +{ + /* Called by core code when a CPU watchpoint fires; need to check if this + * is also an architectural watchpoint match. + */ + ARMCPU *cpu = ARM_CPU(cs); + + return check_watchpoints(cpu); +} + void arm_debug_excp_handler(CPUState *cs) { /* Called by core code when a watchpoint or breakpoint fires; @@ -986,23 +996,20 @@ void arm_debug_excp_handler(CPUState *cs) if (wp_hit) { if (wp_hit->flags & BP_CPU) { + bool wnr = (wp_hit->flags & BP_WATCHPOINT_HIT_WRITE) != 0; + bool same_el = arm_debug_target_el(env) == arm_current_el(env); + cs->watchpoint_hit = NULL; - if (check_watchpoints(cpu)) { - bool wnr = (wp_hit->flags & BP_WATCHPOINT_HIT_WRITE) != 0; - bool same_el = arm_debug_target_el(env) == arm_current_el(env); - - if (extended_addresses_enabled(env)) { - env->exception.fsr = (1 << 9) | 0x22; - } else { - env->exception.fsr = 0x2; - } - env->exception.vaddress = wp_hit->hitaddr; - raise_exception(env, EXCP_DATA_ABORT, - syn_watchpoint(same_el, 0, wnr), - arm_debug_target_el(env)); + + if (extended_addresses_enabled(env)) { + env->exception.fsr = (1 << 9) | 0x22; } else { - cpu_resume_from_signal(cs, NULL); + env->exception.fsr = 0x2; } + env->exception.vaddress = wp_hit->hitaddr; + raise_exception(env, EXCP_DATA_ABORT, + syn_watchpoint(same_el, 0, wnr), + arm_debug_target_el(env)); } } else { uint64_t pc = is_a64(env) ? env->pc : env->regs[15]; -- 1.9.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH v3 0/2] Architectural watchpoint check 2016-01-31 16:15 [Qemu-devel] [PATCH v3 0/2] Architectural watchpoint check Sergey Fedorov 2016-01-31 16:15 ` [Qemu-devel] [PATCH v3 1/2] cpu: Add callback to check architectural watchpoint match Sergey Fedorov 2016-01-31 16:15 ` [Qemu-devel] [PATCH v3 2/2] target-arm: Implement checking of fired watchpoint Sergey Fedorov @ 2016-02-08 15:57 ` Peter Maydell 2 siblings, 0 replies; 7+ messages in thread From: Peter Maydell @ 2016-02-08 15:57 UTC (permalink / raw) To: Sergey Fedorov Cc: Peter Crosthwaite, QEMU Developers, qemu-arm, Paolo Bonzini, Andreas Färber, Richard Henderson On 31 January 2016 at 16:15, Sergey Fedorov <serge.fdrv@gmail.com> wrote: > This series is intended to fix ARM watchpoint emulation misbehavior. > QEMU hangs when QEMU watchpoint fires but it does not pass additional > architectural checks in ARM CPU debug exception handler. For details, > please see individual patches. The most relevant parts of the original > discussion about ARM breakpoint and watchpoint emulation misbehavior can be > found at: > https://lists.gnu.org/archive/html/qemu-devel/2015-08/msg02715.html > https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg00527.html > > Changes in v2: > * Check moved before setting cpu->watchpoint_hit > * Pointer to watchpoint being checked passed to debug_check_watchpoint() > callback > * BP_WATCHPOINT_HIT flag cleared from flags from wp->flags in no-fire case > * Comment for debug_check_watchpoint() callback improved Applied to target-arm.next, thanks. -- PMM ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2016-02-08 15:58 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-01-31 16:15 [Qemu-devel] [PATCH v3 0/2] Architectural watchpoint check Sergey Fedorov 2016-01-31 16:15 ` [Qemu-devel] [PATCH v3 1/2] cpu: Add callback to check architectural watchpoint match Sergey Fedorov 2016-02-02 12:23 ` Peter Maydell 2016-02-08 15:42 ` Peter Maydell 2016-02-08 15:57 ` Peter Maydell 2016-01-31 16:15 ` [Qemu-devel] [PATCH v3 2/2] target-arm: Implement checking of fired watchpoint Sergey Fedorov 2016-02-08 15:57 ` [Qemu-devel] [PATCH v3 0/2] Architectural watchpoint check Peter Maydell
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).