[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 04/22] target-*: Introduce and use cpu_breakp
From: |
Richard Henderson |
Subject: |
Re: [Qemu-devel] [PATCH v2 04/22] target-*: Introduce and use cpu_breakpoint_test |
Date: |
Fri, 18 Sep 2015 08:40:16 -0700 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.2.0 |
On 09/18/2015 03:32 AM, Peter Maydell wrote:
>> +/* Return true if PC matches an installed breakpoint. */
>> +static inline bool cpu_breakpoint_test(CPUState *cpu, vaddr pc, int mask)
>> +{
>> + CPUBreakpoint *bp;
>> +
>> + if (unlikely(!QTAILQ_EMPTY(&cpu->breakpoints))) {
>> + QTAILQ_FOREACH(bp, &cpu->breakpoints, entry) {
>> + if (bp->pc == pc && (bp->flags & mask)) {
>> + return true;
>> + }
>> + }
>> + }
>> + return false;
>> +}
>
> This won't work with the fix for ARM breakpoints Sergey currently has
> on list: http://patchwork.ozlabs.org/patch/517359/
> where we need to behave differently for "there's a GDB breakpoint
> here" and "there's a CPU breakpoint here" (because the complex
> conditions on the latter require us to call a helper function to
> see if we need to actually generate an EXCP_DEBUG exception).
Hmm. Ok, well, I suppose it might do for all but one target then...
>> + if (unlikely(cpu_breakpoint_test(cs, dc->pc, BP_ANY))) {
>> + 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 (num_insns == max_insns && (tb->cflags & CF_LAST_IO)) {
>> gen_io_start();
>> }
>
> Do you know why some but not all targets do this "advance PC"
> thing if there's a breakpoint?
No. I don't believe I've seen it before yesterday.
My suspicion is that if we have a TB that would span two pages, and the
breakpoint is exactly at the page boundary, then we must advance the pc like
this so that it's clear that the TB utilizes the second page.
If so, it means that there are some targets that are broken based on this (e.g.
i386), and there are a few for which this situation is impossible, and this
fixup is cargo culting (e.g. aarch64).
r~
- Re: [Qemu-devel] [PATCH v2 01/22] tcg: Rename debug_insn_start to insn_start, (continued)
- [Qemu-devel] [PATCH v2 09/22] target-s390x: Add cc_op state to insn_start, Richard Henderson, 2015/09/18
- [Qemu-devel] [PATCH v2 10/22] target-sh4: Add flags state to insn_start, Richard Henderson, 2015/09/18
- [Qemu-devel] [PATCH v2 11/22] target-cris: Mirror gen_opc_pc into insn_start, Richard Henderson, 2015/09/18
- [Qemu-devel] [PATCH v2 12/22] target-sparc: Tidy gen_branch_a interface, Richard Henderson, 2015/09/18
- [Qemu-devel] [PATCH v2 15/22] target-sparc: Add npc state to insn_start, Richard Henderson, 2015/09/18
- [Qemu-devel] [PATCH v2 14/22] target-sparc: Remove gen_opc_jump_pc, Richard Henderson, 2015/09/18