[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PULL 13/13] target-arm: Fix CPU breakpoint handling
From: |
Sergey Fedorov |
Subject: |
Re: [Qemu-devel] [PULL 13/13] target-arm: Fix CPU breakpoint handling |
Date: |
Wed, 21 Oct 2015 21:15:26 +0300 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 |
On 16.10.2015 16:58, Peter Maydell wrote:
> From: Sergey Fedorov <address@hidden>
>
> A QEMU breakpoint match is not definitely an architectural breakpoint
> match. If an exception is generated unconditionally during translation,
> it is hardly possible to ignore it in the debug exception handler.
>
> Generate a call to a helper to check CPU breakpoints and raise an
> exception only if any breakpoint matches architecturally.
>
> Signed-off-by: Sergey Fedorov <address@hidden>
> Reviewed-by: Peter Maydell <address@hidden>
> Signed-off-by: Peter Maydell <address@hidden>
> ---
> target-arm/helper.h | 2 ++
> target-arm/op_helper.c | 29 ++++++++++++++++++-----------
> target-arm/translate-a64.c | 17 ++++++++++++-----
> target-arm/translate.c | 19 ++++++++++++++-----
> 4 files changed, 46 insertions(+), 21 deletions(-)
>
(snip)
> diff --git a/target-arm/translate.c b/target-arm/translate.c
> index 1273000..9f1d740 100644
> --- a/target-arm/translate.c
> +++ b/target-arm/translate.c
> @@ -11342,11 +11342,20 @@ void gen_intermediate_code(CPUARMState *env,
> TranslationBlock *tb)
> CPUBreakpoint *bp;
> QTAILQ_FOREACH(bp, &cs->breakpoints, entry) {
> if (bp->pc == dc->pc) {
> - gen_exception_internal_insn(dc, 0, EXCP_DEBUG);
> - /* Advance PC so that clearing the breakpoint will
> - invalidate this TB. */
> - dc->pc += 2;
> - goto done_generating;
> + if (bp->flags & BP_CPU) {
> + gen_helper_check_breakpoints(cpu_env);
> + /* End the TB early; it's likely not going to be
> executed */
> + dc->is_jmp = DISAS_UPDATE;
> + } else {
> + gen_exception_internal_insn(dc, 0, EXCP_DEBUG);
> + /* Advance PC so that clearing the breakpoint will
> + invalidate this TB. */
> + /* TODO: Advance PC by correct instruction length to
> + * avoid disassembler error messages */
> + dc->pc += 2;
> + goto done_generating;
> + }
> + break;
> }
> }
> }
It turns out that this change introduced an issue which can be
illustrated by the following test:
cat >test.s <<EOF
.text
.global _start
_start:
adr r0, bp
mcr p14, 0, r0, c0, c0, 4 // DBGBVR0
mov r0, #1
orr r0, r0, #(0xf << 5)
mcr p14, 0, r0, c0, c0, 5 // DBGBCR0
bp:
nop
wfi
b .
EOF
arm-linux-gnueabi-as -o test.o test.s
arm-linux-gnueabi-ld -Ttext=0x40000000 -o test.elf test.o
./qemu-system-arm -nographic -machine virt -cpu cortex-a15 -kernel \
test.elf -D qemu.log -d in_asm,exec -singlestep
Actually, that is the same test as in
https://lists.gnu.org/archive/html/qemu-devel/2015-08/msg02715.html but
for AArch32.
Running this test QEMU hangs executing code at the address where
breakpoint is set:
----------------
IN:
0x40000000: e28f000c add r0, pc, #12 ; 0xc
Trace 0x7f7c8bdc0028 [40000000]
----------------
IN:
0x40000004: ee000e90 mcr 14, 0, r0, cr0, cr0, {4}
Trace 0x7f7c8bdc0070 [40000004]
----------------
IN:
0x40000008: e3a00001 mov r0, #1 ; 0x1
Trace 0x7f7c8bdc00b0 [40000008]
----------------
IN:
0x4000000c: e3800e1e orr r0, r0, #480 ; 0x1e0
Trace 0x7f7c8bdc00f0 [4000000c]
----------------
IN:
0x40000010: ee000eb0 mcr 14, 0, r0, cr0, cr0, {5}
Trace 0x7f7c8bdc0140 [40000010]
----------------
IN:
0x40000014: e1a00000 nop (mov r0,r0)
Trace 0x7f7c8bdc0180 [40000014]
Trace 0x7f7c8bdc0180 [40000014]
Trace 0x7f7c8bdc0180 [40000014]
Trace 0x7f7c8bdc0180 [40000014]
...
I can conclude that it is due to 'dc->is_jmp = DISAS_UPDATE'. With the
following patch everything is okay:
diff --git a/target-arm/translate.c b/target-arm/translate.c
index 9f1d740..b55c5c2 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -11345,7 +11345,6 @@ void gen_intermediate_code(CPUARMState *env,
TranslationBlock *tb)
if (bp->flags & BP_CPU) {
gen_helper_check_breakpoints(cpu_env);
/* End the TB early; it's likely not going to
be executed */
- dc->is_jmp = DISAS_UPDATE;
} else {
gen_exception_internal_insn(dc, 0, EXCP_DEBUG);
/* Advance PC so that clearing the breakpoint will
As far as I understand, we can't do this in target-arm/translate.c
before dc->pc is advanced properly because CPU state's PC doesn't get
updated as in target-arm/translate-a64.c. Compare:
target-arm/translate.c:
case
DISAS_JUMP:
case
DISAS_UPDATE:
/* indicate that the hash table must be used to find the
next TB */
tcg_gen_exit_tb(0);
break;
target-arm/translate-a64.c:
case DISAS_UPDATE:
gen_a64_set_pc_im(dc->pc);
/* fall through */
case DISAS_JUMP:
/* indicate that the hash table must be used to find the
next TB */
tcg_gen_exit_tb(0);
break;
I think we could fix this problem by cleaning up DISAS_UPDATE usage in
target-arm/translate.c and implementing PC update as in
target-arm/translate-a64.c. I could prepare a patch for that.
Another problem, I think, is that we should somehow restore the CPU
state before raising an exception from check_breakpoints() helper. But
so far I have no idea how to fix this...
Any suggestions are highly appreciated :)
Best regards,
Sergey
- [Qemu-devel] [PULL 00/13] target-arm queue, Peter Maydell, 2015/10/16
- [Qemu-devel] [PULL 03/13] target-arm: Avoid calling arm_el_is_aa64() function for unimplemented EL, Peter Maydell, 2015/10/16
- [Qemu-devel] [PULL 02/13] target-arm: Break the TB after ISB to execute self-modified code correctly, Peter Maydell, 2015/10/16
- [Qemu-devel] [PULL 12/13] target-arm: Fix GDB breakpoint handling, Peter Maydell, 2015/10/16
- [Qemu-devel] [PULL 10/13] hw/arm/virt: Allow zero address for PCI IO space, Peter Maydell, 2015/10/16
- [Qemu-devel] [PULL 09/13] target-arm: Add MDCR_EL2, Peter Maydell, 2015/10/16
- [Qemu-devel] [PULL 07/13] arm: imx25-pdk: Fix machine name, Peter Maydell, 2015/10/16
- [Qemu-devel] [PULL 13/13] target-arm: Fix CPU breakpoint handling, Peter Maydell, 2015/10/16
- Re: [Qemu-devel] [PULL 13/13] target-arm: Fix CPU breakpoint handling,
Sergey Fedorov <=
- [Qemu-devel] [PULL 11/13] target-arm: implement arm_debug_target_el(), Peter Maydell, 2015/10/16
- [Qemu-devel] [PULL 08/13] misc: zynq_slcr: Fix MMIO writes, Peter Maydell, 2015/10/16
- [Qemu-devel] [PULL 01/13] target-arm: Add missing 'static' attribute, Peter Maydell, 2015/10/16
- [Qemu-devel] [PULL 06/13] target-arm: Provide model numbers for Sharp PDAs, Peter Maydell, 2015/10/16
- [Qemu-devel] [PULL 04/13] hw/arm/virt: smbios: inform guest of kvm, Peter Maydell, 2015/10/16
- [Qemu-devel] [PULL 05/13] target-arm: Implement AArch64 OSLAR/OSLSR_EL1 sysregs, Peter Maydell, 2015/10/16
- Re: [Qemu-devel] [PULL 00/13] target-arm queue, Peter Maydell, 2015/10/17