* [Qemu-devel] [PATCH] target-arm: fix CPU breakpoint handling
@ 2015-09-14 10:51 Sergey Fedorov
2015-09-18 13:50 ` Peter Maydell
0 siblings, 1 reply; 9+ messages in thread
From: Sergey Fedorov @ 2015-09-14 10:51 UTC (permalink / raw)
To: qemu-devel; +Cc: Sergey Fedorov, Peter Maydell
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 exceptoin hanlder.
Generate a call to helper to check CPU breakpoints and raise an
exception only if any breakpoint architecturally matches.
Signed-off-by: Sergey Fedorov <serge.fdrv@gmail.com>
---
target-arm/helper.h | 2 ++
target-arm/op_helper.c | 20 +++++++++++++++++++-
target-arm/translate-a64.c | 12 +++++++-----
target-arm/translate.c | 12 +++++++-----
4 files changed, 35 insertions(+), 11 deletions(-)
diff --git a/target-arm/helper.h b/target-arm/helper.h
index 827b33d..c2a85c7 100644
--- a/target-arm/helper.h
+++ b/target-arm/helper.h
@@ -54,6 +54,8 @@ DEF_HELPER_1(yield, void, env)
DEF_HELPER_1(pre_hvc, void, env)
DEF_HELPER_2(pre_smc, void, env, i32)
+DEF_HELPER_1(check_breakpoints, void, env)
+
DEF_HELPER_3(cpsr_write, void, env, i32, i32)
DEF_HELPER_1(cpsr_read, i32, env)
diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
index b298e57..24dcefd 100644
--- a/target-arm/op_helper.c
+++ b/target-arm/op_helper.c
@@ -877,6 +877,15 @@ bool arm_debug_check_watchpoint(CPUState *cs)
return check_watchpoints(cpu);
}
+void HELPER(check_breakpoints)(CPUARMState *env)
+{
+ ARMCPU *cpu = arm_env_get_cpu(env);
+
+ if (check_breakpoints(cpu)) {
+ HELPER(exception_internal(env, EXCP_DEBUG));
+ }
+}
+
void arm_debug_excp_handler(CPUState *cs)
{
/* Called by core code when a watchpoint or breakpoint fires;
@@ -904,7 +913,16 @@ void arm_debug_excp_handler(CPUState *cs)
arm_debug_target_el(env));
}
} else {
- if (check_breakpoints(cpu)) {
+ CPUBreakpoint *bp;
+ uint64_t pc = is_a64(env) ? env->pc : env->regs[15];
+
+ QTAILQ_FOREACH(bp, &cs->breakpoints, entry) {
+ if (bp->pc == pc && !(bp->flags & BP_CPU)) {
+ return;
+ }
+ }
+
+ {
bool same_el = (arm_debug_target_el(env) == arm_current_el(env));
if (extended_addresses_enabled(env)) {
env->exception.fsr = (1 << 9) | 0x22;
diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
index 5c13e15..a5927fd 100644
--- a/target-arm/translate-a64.c
+++ b/target-arm/translate-a64.c
@@ -11000,11 +11000,13 @@ void gen_intermediate_code_internal_a64(ARMCPU *cpu,
if (unlikely(!QTAILQ_EMPTY(&cs->breakpoints))) {
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);
+ break;
+ } else {
+ gen_exception_internal_insn(dc, 0, EXCP_DEBUG);
+ goto done_generating;
+ }
}
}
}
diff --git a/target-arm/translate.c b/target-arm/translate.c
index e27634f..7e2d214 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -11292,11 +11292,13 @@ static inline void gen_intermediate_code_internal(ARMCPU *cpu,
if (unlikely(!QTAILQ_EMPTY(&cs->breakpoints))) {
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);
+ break;
+ } else {
+ gen_exception_internal_insn(dc, 0, EXCP_DEBUG);
+ goto done_generating;
+ }
}
}
}
--
1.9.1
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [Qemu-devel] [PATCH] target-arm: fix CPU breakpoint handling
2015-09-14 10:51 [Qemu-devel] [PATCH] target-arm: fix CPU breakpoint handling Sergey Fedorov
@ 2015-09-18 13:50 ` Peter Maydell
2015-09-18 14:07 ` Sergey Fedorov
0 siblings, 1 reply; 9+ messages in thread
From: Peter Maydell @ 2015-09-18 13:50 UTC (permalink / raw)
To: Sergey Fedorov; +Cc: QEMU Developers
On 14 September 2015 at 11:51, Sergey Fedorov <serge.fdrv@gmail.com> wrote:
> 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 exceptoin hanlder.
"exception".
>
> Generate a call to helper to check CPU breakpoints and raise an
> exception only if any breakpoint architecturally matches.
>
> Signed-off-by: Sergey Fedorov <serge.fdrv@gmail.com>
> ---
> target-arm/helper.h | 2 ++
> target-arm/op_helper.c | 20 +++++++++++++++++++-
> target-arm/translate-a64.c | 12 +++++++-----
> target-arm/translate.c | 12 +++++++-----
> 4 files changed, 35 insertions(+), 11 deletions(-)
>
> diff --git a/target-arm/helper.h b/target-arm/helper.h
> index 827b33d..c2a85c7 100644
> --- a/target-arm/helper.h
> +++ b/target-arm/helper.h
> @@ -54,6 +54,8 @@ DEF_HELPER_1(yield, void, env)
> DEF_HELPER_1(pre_hvc, void, env)
> DEF_HELPER_2(pre_smc, void, env, i32)
>
> +DEF_HELPER_1(check_breakpoints, void, env)
> +
> DEF_HELPER_3(cpsr_write, void, env, i32, i32)
> DEF_HELPER_1(cpsr_read, i32, env)
>
> diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
> index b298e57..24dcefd 100644
> --- a/target-arm/op_helper.c
> +++ b/target-arm/op_helper.c
> @@ -877,6 +877,15 @@ bool arm_debug_check_watchpoint(CPUState *cs)
> return check_watchpoints(cpu);
> }
>
> +void HELPER(check_breakpoints)(CPUARMState *env)
> +{
> + ARMCPU *cpu = arm_env_get_cpu(env);
> +
> + if (check_breakpoints(cpu)) {
> + HELPER(exception_internal(env, EXCP_DEBUG));
> + }
> +}
> +
> void arm_debug_excp_handler(CPUState *cs)
> {
> /* Called by core code when a watchpoint or breakpoint fires;
> @@ -904,7 +913,16 @@ void arm_debug_excp_handler(CPUState *cs)
> arm_debug_target_el(env));
> }
> } else {
> - if (check_breakpoints(cpu)) {
> + CPUBreakpoint *bp;
> + uint64_t pc = is_a64(env) ? env->pc : env->regs[15];
> +
> + QTAILQ_FOREACH(bp, &cs->breakpoints, entry) {
> + if (bp->pc == pc && !(bp->flags & BP_CPU)) {
> + return;
> + }
> + }
This extra code looks right, but isn't it fixing a different bug?
> +
> + {
Rather than adding the extra block I would just de-indent the code
that used to live inside the if() and move the variable declaration
to the top of the new block.
> bool same_el = (arm_debug_target_el(env) == arm_current_el(env));
> if (extended_addresses_enabled(env)) {
> env->exception.fsr = (1 << 9) | 0x22;
> diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
> index 5c13e15..a5927fd 100644
> --- a/target-arm/translate-a64.c
> +++ b/target-arm/translate-a64.c
> @@ -11000,11 +11000,13 @@ void gen_intermediate_code_internal_a64(ARMCPU *cpu,
> if (unlikely(!QTAILQ_EMPTY(&cs->breakpoints))) {
> 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);
> + break;
> + } else {
> + gen_exception_internal_insn(dc, 0, EXCP_DEBUG);
> + goto done_generating;
> + }
You seem to have dropped the "advance the PC" code -- why is that ok?
thanks
-- PMM
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [Qemu-devel] [PATCH] target-arm: fix CPU breakpoint handling
2015-09-18 13:50 ` Peter Maydell
@ 2015-09-18 14:07 ` Sergey Fedorov
2015-09-18 14:14 ` Peter Maydell
2015-09-25 11:34 ` Sergey Fedorov
0 siblings, 2 replies; 9+ messages in thread
From: Sergey Fedorov @ 2015-09-18 14:07 UTC (permalink / raw)
To: Peter Maydell; +Cc: QEMU Developers
On 18.09.2015 16:50, Peter Maydell wrote:
> On 14 September 2015 at 11:51, Sergey Fedorov <serge.fdrv@gmail.com> wrote:
>> 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 exceptoin hanlder.
> "exception".
Will fix it, thanks.
>
>> Generate a call to helper to check CPU breakpoints and raise an
>> exception only if any breakpoint architecturally matches.
>>
>> Signed-off-by: Sergey Fedorov <serge.fdrv@gmail.com>
>> ---
>> target-arm/helper.h | 2 ++
>> target-arm/op_helper.c | 20 +++++++++++++++++++-
>> target-arm/translate-a64.c | 12 +++++++-----
>> target-arm/translate.c | 12 +++++++-----
>> 4 files changed, 35 insertions(+), 11 deletions(-)
>>
>> diff --git a/target-arm/helper.h b/target-arm/helper.h
>> index 827b33d..c2a85c7 100644
>> --- a/target-arm/helper.h
>> +++ b/target-arm/helper.h
>> @@ -54,6 +54,8 @@ DEF_HELPER_1(yield, void, env)
>> DEF_HELPER_1(pre_hvc, void, env)
>> DEF_HELPER_2(pre_smc, void, env, i32)
>>
>> +DEF_HELPER_1(check_breakpoints, void, env)
>> +
>> DEF_HELPER_3(cpsr_write, void, env, i32, i32)
>> DEF_HELPER_1(cpsr_read, i32, env)
>>
>> diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
>> index b298e57..24dcefd 100644
>> --- a/target-arm/op_helper.c
>> +++ b/target-arm/op_helper.c
>> @@ -877,6 +877,15 @@ bool arm_debug_check_watchpoint(CPUState *cs)
>> return check_watchpoints(cpu);
>> }
>>
>> +void HELPER(check_breakpoints)(CPUARMState *env)
>> +{
>> + ARMCPU *cpu = arm_env_get_cpu(env);
>> +
>> + if (check_breakpoints(cpu)) {
>> + HELPER(exception_internal(env, EXCP_DEBUG));
>> + }
>> +}
>> +
>> void arm_debug_excp_handler(CPUState *cs)
>> {
>> /* Called by core code when a watchpoint or breakpoint fires;
>> @@ -904,7 +913,16 @@ void arm_debug_excp_handler(CPUState *cs)
>> arm_debug_target_el(env));
>> }
>> } else {
>> - if (check_breakpoints(cpu)) {
>> + CPUBreakpoint *bp;
>> + uint64_t pc = is_a64(env) ? env->pc : env->regs[15];
>> +
>> + QTAILQ_FOREACH(bp, &cs->breakpoints, entry) {
>> + if (bp->pc == pc && !(bp->flags & BP_CPU)) {
>> + return;
>> + }
>> + }
> This extra code looks right, but isn't it fixing a different bug?
You are right, it would better come to separate patch.
>
>> +
>> + {
> Rather than adding the extra block I would just de-indent the code
> that used to live inside the if() and move the variable declaration
> to the top of the new block.
Okay, I will adjust the code.
>
>> bool same_el = (arm_debug_target_el(env) == arm_current_el(env));
>> if (extended_addresses_enabled(env)) {
>> env->exception.fsr = (1 << 9) | 0x22;
>> diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
>> index 5c13e15..a5927fd 100644
>> --- a/target-arm/translate-a64.c
>> +++ b/target-arm/translate-a64.c
>> @@ -11000,11 +11000,13 @@ void gen_intermediate_code_internal_a64(ARMCPU *cpu,
>> if (unlikely(!QTAILQ_EMPTY(&cs->breakpoints))) {
>> 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);
>> + break;
>> + } else {
>> + gen_exception_internal_insn(dc, 0, EXCP_DEBUG);
>> + goto done_generating;
>> + }
> You seem to have dropped the "advance the PC" code -- why is that ok?
>
I also dropped the immediately following goto statement. With these
changes PC is advanced in the same way as it happens during normal
translation. That is because we actually have to do the instruction
translation process here to support the case when a breakpoint with
matching PC is architecturally mismatched. As I understand, that
"advance the PC" code was necessary to produce a TB with non-zero size
so that it can be invalidated later when we clear the breakpoint.
Best,
Sergey
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [Qemu-devel] [PATCH] target-arm: fix CPU breakpoint handling
2015-09-18 14:07 ` Sergey Fedorov
@ 2015-09-18 14:14 ` Peter Maydell
2015-09-18 16:33 ` Sergey Fedorov
2015-09-25 11:34 ` Sergey Fedorov
1 sibling, 1 reply; 9+ messages in thread
From: Peter Maydell @ 2015-09-18 14:14 UTC (permalink / raw)
To: Sergey Fedorov; +Cc: QEMU Developers
On 18 September 2015 at 15:07, Sergey Fedorov <serge.fdrv@gmail.com> wrote:
> On 18.09.2015 16:50, Peter Maydell wrote:
>> On 14 September 2015 at 11:51, Sergey Fedorov <serge.fdrv@gmail.com> wrote:
>>> --- a/target-arm/translate-a64.c
>>> +++ b/target-arm/translate-a64.c
>>> @@ -11000,11 +11000,13 @@ void gen_intermediate_code_internal_a64(ARMCPU *cpu,
>>> if (unlikely(!QTAILQ_EMPTY(&cs->breakpoints))) {
>>> 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);
>>> + break;
>>> + } else {
>>> + gen_exception_internal_insn(dc, 0, EXCP_DEBUG);
>>> + goto done_generating;
>>> + }
>> You seem to have dropped the "advance the PC" code -- why is that ok?
>>
>
> I also dropped the immediately following goto statement. With these
> changes PC is advanced in the same way as it happens during normal
> translation. That is because we actually have to do the instruction
> translation process here to support the case when a breakpoint with
> matching PC is architecturally mismatched. As I understand, that
> "advance the PC" code was necessary to produce a TB with non-zero size
> so that it can be invalidated later when we clear the breakpoint.
OK, that makes sense for the BP_CPU case but you still have the
"goto done_generating;" in the else clause...
Also, should we maybe make this TB be only one insn long even for
the BP_CPU case? It seems like in the common case we will only
execute one insn.
thanks
-- PMM
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [Qemu-devel] [PATCH] target-arm: fix CPU breakpoint handling
2015-09-18 14:14 ` Peter Maydell
@ 2015-09-18 16:33 ` Sergey Fedorov
2015-09-18 16:36 ` Peter Maydell
0 siblings, 1 reply; 9+ messages in thread
From: Sergey Fedorov @ 2015-09-18 16:33 UTC (permalink / raw)
To: Peter Maydell; +Cc: QEMU Developers
On 18.09.2015 17:14, Peter Maydell wrote:
> On 18 September 2015 at 15:07, Sergey Fedorov <serge.fdrv@gmail.com> wrote:
>> On 18.09.2015 16:50, Peter Maydell wrote:
>>> On 14 September 2015 at 11:51, Sergey Fedorov <serge.fdrv@gmail.com> wrote:
>>>> --- a/target-arm/translate-a64.c
>>>> +++ b/target-arm/translate-a64.c
>>>> @@ -11000,11 +11000,13 @@ void gen_intermediate_code_internal_a64(ARMCPU *cpu,
>>>> if (unlikely(!QTAILQ_EMPTY(&cs->breakpoints))) {
>>>> 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);
>>>> + break;
>>>> + } else {
>>>> + gen_exception_internal_insn(dc, 0, EXCP_DEBUG);
>>>> + goto done_generating;
>>>> + }
>>> You seem to have dropped the "advance the PC" code -- why is that ok?
>>>
>> I also dropped the immediately following goto statement. With these
>> changes PC is advanced in the same way as it happens during normal
>> translation. That is because we actually have to do the instruction
>> translation process here to support the case when a breakpoint with
>> matching PC is architecturally mismatched. As I understand, that
>> "advance the PC" code was necessary to produce a TB with non-zero size
>> so that it can be invalidated later when we clear the breakpoint.
> OK, that makes sense for the BP_CPU case but you still have the
> "goto done_generating;" in the else clause...
>
> Also, should we maybe make this TB be only one insn long even for
> the BP_CPU case? It seems like in the common case we will only
> execute one insn.
>
Right, I have to fix this PC advancement. But I can't think of why we
will only execute one insn...
Best,
Sergey
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [Qemu-devel] [PATCH] target-arm: fix CPU breakpoint handling
2015-09-18 16:33 ` Sergey Fedorov
@ 2015-09-18 16:36 ` Peter Maydell
2015-09-18 17:14 ` Sergey Fedorov
0 siblings, 1 reply; 9+ messages in thread
From: Peter Maydell @ 2015-09-18 16:36 UTC (permalink / raw)
To: Sergey Fedorov; +Cc: QEMU Developers
On 18 September 2015 at 17:33, Sergey Fedorov <serge.fdrv@gmail.com> wrote:
> On 18.09.2015 17:14, Peter Maydell wrote:
>> On 18 September 2015 at 15:07, Sergey Fedorov <serge.fdrv@gmail.com> wrote:
>>> On 18.09.2015 16:50, Peter Maydell wrote:
>>>> On 14 September 2015 at 11:51, Sergey Fedorov <serge.fdrv@gmail.com> wrote:
>>>>> --- a/target-arm/translate-a64.c
>>>>> +++ b/target-arm/translate-a64.c
>>>>> @@ -11000,11 +11000,13 @@ void gen_intermediate_code_internal_a64(ARMCPU *cpu,
>>>>> if (unlikely(!QTAILQ_EMPTY(&cs->breakpoints))) {
>>>>> 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);
>>>>> + break;
>>>>> + } else {
>>>>> + gen_exception_internal_insn(dc, 0, EXCP_DEBUG);
>>>>> + goto done_generating;
>>>>> + }
>>>> You seem to have dropped the "advance the PC" code -- why is that ok?
>>>>
>>> I also dropped the immediately following goto statement. With these
>>> changes PC is advanced in the same way as it happens during normal
>>> translation. That is because we actually have to do the instruction
>>> translation process here to support the case when a breakpoint with
>>> matching PC is architecturally mismatched. As I understand, that
>>> "advance the PC" code was necessary to produce a TB with non-zero size
>>> so that it can be invalidated later when we clear the breakpoint.
>> OK, that makes sense for the BP_CPU case but you still have the
>> "goto done_generating;" in the else clause...
>>
>> Also, should we maybe make this TB be only one insn long even for
>> the BP_CPU case? It seems like in the common case we will only
>> execute one insn.
>>
>
> Right, I have to fix this PC advancement. But I can't think of why we
> will only execute one insn...
Well, typically we'll take the BP exception in the helper function.
There's nothing inherently wrong with translating further code
after this insn, but it's usually not going to be executed, so
we might as well end the TB early.
thanks
-- PMM
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [Qemu-devel] [PATCH] target-arm: fix CPU breakpoint handling
2015-09-18 16:36 ` Peter Maydell
@ 2015-09-18 17:14 ` Sergey Fedorov
0 siblings, 0 replies; 9+ messages in thread
From: Sergey Fedorov @ 2015-09-18 17:14 UTC (permalink / raw)
To: Peter Maydell; +Cc: QEMU Developers
On 18.09.2015 19:36, Peter Maydell wrote:
> On 18 September 2015 at 17:33, Sergey Fedorov <serge.fdrv@gmail.com> wrote:
>> On 18.09.2015 17:14, Peter Maydell wrote:
>>> On 18 September 2015 at 15:07, Sergey Fedorov <serge.fdrv@gmail.com> wrote:
>>>> On 18.09.2015 16:50, Peter Maydell wrote:
>>>>> On 14 September 2015 at 11:51, Sergey Fedorov <serge.fdrv@gmail.com> wrote:
>>>>>> --- a/target-arm/translate-a64.c
>>>>>> +++ b/target-arm/translate-a64.c
>>>>>> @@ -11000,11 +11000,13 @@ void gen_intermediate_code_internal_a64(ARMCPU *cpu,
>>>>>> if (unlikely(!QTAILQ_EMPTY(&cs->breakpoints))) {
>>>>>> 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);
>>>>>> + break;
>>>>>> + } else {
>>>>>> + gen_exception_internal_insn(dc, 0, EXCP_DEBUG);
>>>>>> + goto done_generating;
>>>>>> + }
>>>>> You seem to have dropped the "advance the PC" code -- why is that ok?
>>>>>
>>>> I also dropped the immediately following goto statement. With these
>>>> changes PC is advanced in the same way as it happens during normal
>>>> translation. That is because we actually have to do the instruction
>>>> translation process here to support the case when a breakpoint with
>>>> matching PC is architecturally mismatched. As I understand, that
>>>> "advance the PC" code was necessary to produce a TB with non-zero size
>>>> so that it can be invalidated later when we clear the breakpoint.
>>> OK, that makes sense for the BP_CPU case but you still have the
>>> "goto done_generating;" in the else clause...
>>>
>>> Also, should we maybe make this TB be only one insn long even for
>>> the BP_CPU case? It seems like in the common case we will only
>>> execute one insn.
>>>
>> Right, I have to fix this PC advancement. But I can't think of why we
>> will only execute one insn...
> Well, typically we'll take the BP exception in the helper function.
> There's nothing inherently wrong with translating further code
> after this insn, but it's usually not going to be executed, so
> we might as well end the TB early.
>
Thank, it is clear now. What about getting rid of "goto done generating"
and always translate one insn to update PC accordingly? Sometimes in_asm
log complains about that when disassembler and translator disagree about
the instruction size.
Best,
Sergey
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] target-arm: fix CPU breakpoint handling
2015-09-18 14:07 ` Sergey Fedorov
2015-09-18 14:14 ` Peter Maydell
@ 2015-09-25 11:34 ` Sergey Fedorov
2015-09-25 11:42 ` Sergey Fedorov
1 sibling, 1 reply; 9+ messages in thread
From: Sergey Fedorov @ 2015-09-25 11:34 UTC (permalink / raw)
To: Peter Maydell; +Cc: QEMU Developers
On 18.09.2015 17:07, Sergey Fedorov wrote:
> On 18.09.2015 16:50, Peter Maydell wrote:
>> On 14 September 2015 at 11:51, Sergey Fedorov <serge.fdrv@gmail.com> wrote:
>> @@ -904,7 +913,16 @@ void arm_debug_excp_handler(CPUState *cs)
>> arm_debug_target_el(env));
>> }
>> } else {
>> - if (check_breakpoints(cpu)) {
>> + CPUBreakpoint *bp;
>> + uint64_t pc = is_a64(env) ? env->pc : env->regs[15];
>> +
>> + QTAILQ_FOREACH(bp, &cs->breakpoints, entry) {
>> + if (bp->pc == pc && !(bp->flags & BP_CPU)) {
>> + return;
>> + }
>> + }
>> This extra code looks right, but isn't it fixing a different bug?
> You are right, it would better come to separate patch.
Actually, I can't think of it as a separate patch. This change is really
required only if we remove check_breakpoints() here. Otherwise
check_breakpoints() calls bp_wp_matches() which do the necessary check.
Best regards,
Sergey
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [Qemu-devel] [PATCH] target-arm: fix CPU breakpoint handling
2015-09-25 11:34 ` Sergey Fedorov
@ 2015-09-25 11:42 ` Sergey Fedorov
0 siblings, 0 replies; 9+ messages in thread
From: Sergey Fedorov @ 2015-09-25 11:42 UTC (permalink / raw)
To: Peter Maydell; +Cc: QEMU Developers
On 25.09.2015 14:34, Sergey Fedorov wrote:
> On 18.09.2015 17:07, Sergey Fedorov wrote:
>> On 18.09.2015 16:50, Peter Maydell wrote:
>>> On 14 September 2015 at 11:51, Sergey Fedorov <serge.fdrv@gmail.com> wrote:
>>> @@ -904,7 +913,16 @@ void arm_debug_excp_handler(CPUState *cs)
>>> arm_debug_target_el(env));
>>> }
>>> } else {
>>> - if (check_breakpoints(cpu)) {
>>> + CPUBreakpoint *bp;
>>> + uint64_t pc = is_a64(env) ? env->pc : env->regs[15];
>>> +
>>> + QTAILQ_FOREACH(bp, &cs->breakpoints, entry) {
>>> + if (bp->pc == pc && !(bp->flags & BP_CPU)) {
>>> + return;
>>> + }
>>> + }
>>> This extra code looks right, but isn't it fixing a different bug?
>> You are right, it would better come to separate patch.
> Actually, I can't think of it as a separate patch. This change is really
> required only if we remove check_breakpoints() here. Otherwise
> check_breakpoints() calls bp_wp_matches() which do the necessary check.
>
...but considering the order of breakpoint enumeration it is not so
simple. The difference is when we have GDB and CPU breakpoint to the
same address. In this case check_breakpoints() returns true, but we
should handle GDB breakpoints first. Sorry for my misunderstanding, I
will split this patch as you suggested.
Best regards,
Sergey
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2015-09-25 11:43 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-09-14 10:51 [Qemu-devel] [PATCH] target-arm: fix CPU breakpoint handling Sergey Fedorov
2015-09-18 13:50 ` Peter Maydell
2015-09-18 14:07 ` Sergey Fedorov
2015-09-18 14:14 ` Peter Maydell
2015-09-18 16:33 ` Sergey Fedorov
2015-09-18 16:36 ` Peter Maydell
2015-09-18 17:14 ` Sergey Fedorov
2015-09-25 11:34 ` Sergey Fedorov
2015-09-25 11:42 ` Sergey Fedorov
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).