[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PULL 04/26] target-*: Introduce and use cpu_breakpoint
From: |
Sergey Fedorov |
Subject: |
Re: [Qemu-devel] [PULL 04/26] target-*: Introduce and use cpu_breakpoint_test |
Date: |
Tue, 13 Oct 2015 16:40:28 +0300 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 |
On 13.10.2015 03:13, Richard Henderson wrote:
> On 10/10/2015 12:34 AM, Sergey Fedorov wrote:
>>> @@ -2936,6 +2927,10 @@ static inline void
>>> gen_intermediate_code_internal(AlphaCPU *cpu,
>>> tcg_gen_insn_start(ctx.pc);
>>> num_insns++;
>>>
>>> + if (unlikely(cpu_breakpoint_test(cs, ctx.pc, BP_ANY))) {
>>> + gen_excp(&ctx, EXCP_DEBUG, 0);
>>> + break;
>>> + }
>>
>> Actually, control logic has changed here. The old code used a break
>> statement to exit from QTAILQ_FOREACH loop and continue with instruction
>> translation thus translating at least one instruction. The break
>> statement in the new code makes exit from the translation loop itself,
>> effectively producing zero-length TB which won't get invalidated when
>> clearing the breakpoint. Seems like we should remove the break statement
>> here and in similar cases below, right?
>
> Why do you believe that a zero-length TB won't be cleared?
> The TB still has a start address, which is contained within
> a given page, which is invalidated.
>
> Some target-*/translate.c takes care to advance the PC, but I believe
> that this is only required when the breakpoint instruction *itself*
> could span a page boundary. I.e. the TB needs to be marked to span
> two pages. This situation can never be true for many RISC targets.
>
> We did discuss this exact situation during review of the patch set,
> though it's probably true that there are outstanding errors in some
> translators.
I see your point, but what I am actually concerned about is the
following scenario.
Lets suppose we are going to remove a breakpoint. Eventually we can get
the following function call stack:
#0 tb_invalidate_phys_page_range()
#1 tb_invalidate_phys_addr()
#2 breakpoint_invalidate()
#3 cpu_breakpoint_remove_by_ref()
...
Then, if we come across a zero-sized TB then 'tb_start' would be equal
to 'tb_end'. That is not a big deal unless 'start' is not equal to
'tb_start'. Otherwise, "!(tb_end <= start || tb_start >= end)" condition
check will fail and that TB won't get invalidated as it should be. As I
can see this is a case when we try to remove a breakpoint which is
happened to be set at the very first instruction of a TB. So we either
need to change the condition in tb_invalidate_phys_page_range() or do
the PC advancement trick during translation, no matter can instructions
cross a page boundary or not.
Best regards,
Sergey
[Qemu-devel] [PULL 03/26] target-*: Increment num_insns immediately after tcg_gen_insn_start, Richard Henderson, 2015/10/08
[Qemu-devel] [PULL 06/26] target-arm: Add condexec state to insn_start, Richard Henderson, 2015/10/08
[Qemu-devel] [PULL 05/26] tcg: Allow extra data to be attached to insn_start, Richard Henderson, 2015/10/08
[Qemu-devel] [PULL 13/26] target-sparc: Split out gen_branch_n, Richard Henderson, 2015/10/08
[Qemu-devel] [PULL 16/26] tcg: Merge cpu_gen_code into tb_gen_code, Richard Henderson, 2015/10/08
[Qemu-devel] [PULL 10/26] target-sh4: Add flags state to insn_start, Richard Henderson, 2015/10/08
[Qemu-devel] [PULL 22/26] tcg: Remove tcg_gen_code_search_pc, Richard Henderson, 2015/10/08
[Qemu-devel] [PULL 18/26] tcg: Add TCG_MAX_INSNS, Richard Henderson, 2015/10/08
[Qemu-devel] [PULL 23/26] tcg: Emit prologue to the beginning of code_gen_buffer, Richard Henderson, 2015/10/08