[v13,18/26] target/arm: [tcg] Port to breakpoint_check

Commit Message

Lluís Vilanova July 14, 2017, 9:26 a.m. UTC

Incrementally paves the way towards using the generic instruction translation
loop.

Signed-off-by: Lluís Vilanova <[e-mail]>
Reviewed-by: Richard Henderson <[e-mail]>
---
 target/arm/translate.c |   53 +++++++++++++++++++++++++++++++-----------------
 1 file changed, 34 insertions(+), 19 deletions(-)

Comments

Richard Henderson July 14, 2017, 5:26 p.m. UTC | #1

On 07/13/2017 11:26 PM, Lluís Vilanova wrote:
> Incrementally paves the way towards using the generic instruction translation
> loop.
> 
> Signed-off-by: Lluís Vilanova <[e-mail]>
> Reviewed-by: Richard Henderson <[e-mail]>
> ---
>   target/arm/translate.c |   53 +++++++++++++++++++++++++++++++-----------------
>   1 file changed, 34 insertions(+), 19 deletions(-)
> 
> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index b9183fc511..55bef09739 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -11917,6 +11917,33 @@ static void arm_tr_insn_start(DisasContextBase *dcbase, CPUState *cpu)
>   #endif
>   }
>   
> +static bool arm_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cpu,
> +                                    const CPUBreakpoint *bp)
> +{
> +    DisasContext *dc = container_of(dcbase, DisasContext, base);
> +
> +    if (bp->flags & BP_CPU) {
> +        gen_set_condexec(dc);
> +        gen_set_pc_im(dc, dc->pc);
> +        gen_helper_check_breakpoints(cpu_env);
> +        /* End the TB early; it's likely not going to be executed */
> +        dc->base.is_jmp = DISAS_UPDATE;
> +    } else {
> +        gen_exception_internal_insn(dc, 0, EXCP_DEBUG);
> +        /* The address covered by the breakpoint must be
> +           included in [tb->pc, tb->pc + tb->size) in order
> +           to for it to be properly cleared -- thus we
> +           increment the PC here so that the logic setting
> +           tb->size below does the right thing.  */
> +        /* TODO: Advance PC by correct instruction length to
> +         * avoid disassembler error messages */
> +        dc->pc += 2;
> +        dc->base.is_jmp = DISAS_NORETURN;
> +    }
> +
> +    return true;
> +}
> +
>   /* generate intermediate code for basic block 'tb'.  */
>   void gen_intermediate_code(CPUState *cs, TranslationBlock *tb)
>   {
> @@ -11965,28 +11992,16 @@ void gen_intermediate_code(CPUState *cs, TranslationBlock *tb)
>           if (unlikely(!QTAILQ_EMPTY(&cs->breakpoints))) {
>               CPUBreakpoint *bp;
>               QTAILQ_FOREACH(bp, &cs->breakpoints, entry) {
> -                if (bp->pc == dc->pc) {
> -                    if (bp->flags & BP_CPU) {
> -                        gen_set_condexec(dc);
> -                        gen_set_pc_im(dc, dc->pc);
> -                        gen_helper_check_breakpoints(cpu_env);
> -                        /* End the TB early; it's likely not going to be executed */
> -                        dc->base.is_jmp = DISAS_UPDATE;

Oh I see what you're doing there in the main loop.
And I see that you're copying existing behaviour.

That said, I do wonder if there's a better way.

Looking back at the original patch (5d98bf8f), there do not
seem to have been any other side effects intended; simply
"single step" any insn for which this bp condition is met.

Another way to handle this would be if we could adjust max_insns = num_insns.
That would cause the loop to exit after the current insn, with DISAS_TOO_MANY 
if nothing else.


r~

Richard Henderson July 14, 2017, 5:42 p.m. UTC | #2

On 07/14/2017 07:26 AM, Richard Henderson wrote:
> On 07/13/2017 11:26 PM, Lluís Vilanova wrote:
>> Incrementally paves the way towards using the generic instruction translation
>> loop.
>>
>> Signed-off-by: Lluís Vilanova <[e-mail]>
>> Reviewed-by: Richard Henderson <[e-mail]>
>> ---
>>   target/arm/translate.c |   53 +++++++++++++++++++++++++++++++-----------------
>>   1 file changed, 34 insertions(+), 19 deletions(-)
>>
>> diff --git a/target/arm/translate.c b/target/arm/translate.c
>> index b9183fc511..55bef09739 100644
>> --- a/target/arm/translate.c
>> +++ b/target/arm/translate.c
>> @@ -11917,6 +11917,33 @@ static void arm_tr_insn_start(DisasContextBase 
>> *dcbase, CPUState *cpu)
>>   #endif
>>   }
>> +static bool arm_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cpu,
>> +                                    const CPUBreakpoint *bp)
>> +{
>> +    DisasContext *dc = container_of(dcbase, DisasContext, base);
>> +
>> +    if (bp->flags & BP_CPU) {
>> +        gen_set_condexec(dc);
>> +        gen_set_pc_im(dc, dc->pc);
>> +        gen_helper_check_breakpoints(cpu_env);
>> +        /* End the TB early; it's likely not going to be executed */
>> +        dc->base.is_jmp = DISAS_UPDATE;
>> +    } else {
>> +        gen_exception_internal_insn(dc, 0, EXCP_DEBUG);
>> +        /* The address covered by the breakpoint must be
>> +           included in [tb->pc, tb->pc + tb->size) in order
>> +           to for it to be properly cleared -- thus we
>> +           increment the PC here so that the logic setting
>> +           tb->size below does the right thing.  */
>> +        /* TODO: Advance PC by correct instruction length to
>> +         * avoid disassembler error messages */
>> +        dc->pc += 2;
>> +        dc->base.is_jmp = DISAS_NORETURN;
>> +    }
>> +
>> +    return true;
>> +}
>> +
>>   /* generate intermediate code for basic block 'tb'.  */
>>   void gen_intermediate_code(CPUState *cs, TranslationBlock *tb)
>>   {
>> @@ -11965,28 +11992,16 @@ void gen_intermediate_code(CPUState *cs, 
>> TranslationBlock *tb)
>>           if (unlikely(!QTAILQ_EMPTY(&cs->breakpoints))) {
>>               CPUBreakpoint *bp;
>>               QTAILQ_FOREACH(bp, &cs->breakpoints, entry) {
>> -                if (bp->pc == dc->pc) {
>> -                    if (bp->flags & BP_CPU) {
>> -                        gen_set_condexec(dc);
>> -                        gen_set_pc_im(dc, dc->pc);
>> -                        gen_helper_check_breakpoints(cpu_env);
>> -                        /* End the TB early; it's likely not going to be 
>> executed */
>> -                        dc->base.is_jmp = DISAS_UPDATE;
> 
> Oh I see what you're doing there in the main loop.
> And I see that you're copying existing behaviour.
> 
> That said, I do wonder if there's a better way.
> 
> Looking back at the original patch (5d98bf8f), there do not
> seem to have been any other side effects intended; simply
> "single step" any insn for which this bp condition is met.
> 
> Another way to handle this would be if we could adjust max_insns = num_insns.
> That would cause the loop to exit after the current insn, with DISAS_TOO_MANY 
> if nothing else.

Another possibility is is_jmp = DISAS_TOO_MANY, and exit the translation loop 
after the breakpoint check only for is_jmp > DISAS_TOO_MANY.  That allows all 
of the DISAS_TARGET_N values to exit as well.


r~

Lluís Vilanova July 15, 2017, 7:46 a.m. UTC | #3

Richard Henderson writes:

> On 07/13/2017 11:26 PM, Lluís Vilanova wrote:
>> Incrementally paves the way towards using the generic instruction translation
>> loop.
>> 
>> Signed-off-by: Lluís Vilanova <[e-mail]>
>> Reviewed-by: Richard Henderson <[e-mail]>
>> ---
>> target/arm/translate.c |   53 +++++++++++++++++++++++++++++++-----------------
>> 1 file changed, 34 insertions(+), 19 deletions(-)
>> 
>> diff --git a/target/arm/translate.c b/target/arm/translate.c
>> index b9183fc511..55bef09739 100644
>> --- a/target/arm/translate.c
>> +++ b/target/arm/translate.c
>> @@ -11917,6 +11917,33 @@ static void arm_tr_insn_start(DisasContextBase *dcbase, CPUState *cpu)
>> #endif
>> }
>> +static bool arm_tr_breakpoint_check(DisasContextBase *dcbase, CPUState
>> *cpu,
>> +                                    const CPUBreakpoint *bp)
>> +{
>> +    DisasContext *dc = container_of(dcbase, DisasContext, base);
>> +
>> +    if (bp->flags & BP_CPU) {
>> +        gen_set_condexec(dc);
>> +        gen_set_pc_im(dc, dc->pc);
>> +        gen_helper_check_breakpoints(cpu_env);
>> +        /* End the TB early; it's likely not going to be executed */
>> +        dc->base.is_jmp = DISAS_UPDATE;
>> +    } else {
>> +        gen_exception_internal_insn(dc, 0, EXCP_DEBUG);
>> +        /* The address covered by the breakpoint must be
>> +           included in [tb->pc, tb->pc + tb->size) in order
>> +           to for it to be properly cleared -- thus we
>> +           increment the PC here so that the logic setting
>> +           tb->size below does the right thing.  */
>> +        /* TODO: Advance PC by correct instruction length to
>> +         * avoid disassembler error messages */
>> +        dc->pc += 2;
>> +        dc->base.is_jmp = DISAS_NORETURN;
>> +    }
>> +
>> +    return true;
>> +}
>> +
>> /* generate intermediate code for basic block 'tb'.  */
>> void gen_intermediate_code(CPUState *cs, TranslationBlock *tb)
>> {
>> @@ -11965,28 +11992,16 @@ void gen_intermediate_code(CPUState *cs, TranslationBlock *tb)
>> if (unlikely(!QTAILQ_EMPTY(&cs->breakpoints))) {
>> CPUBreakpoint *bp;
>> QTAILQ_FOREACH(bp, &cs->breakpoints, entry) {
>> -                if (bp->pc == dc->pc) {
>> -                    if (bp->flags & BP_CPU) {
>> -                        gen_set_condexec(dc);
>> -                        gen_set_pc_im(dc, dc->pc);
>> -                        gen_helper_check_breakpoints(cpu_env);
>> -                        /* End the TB early; it's likely not going to be executed */
>> -                        dc->base.is_jmp = DISAS_UPDATE;

> Oh I see what you're doing there in the main loop.
> And I see that you're copying existing behaviour.

> That said, I do wonder if there's a better way.

> Looking back at the original patch (5d98bf8f), there do not
> seem to have been any other side effects intended; simply
> "single step" any insn for which this bp condition is met.

> Another way to handle this would be if we could adjust max_insns = num_insns.
> That would cause the loop to exit after the current insn, with DISAS_TOO_MANY if
> nothing else.

But adjusting num_insns (I guess passed as a pointer to breakpoint_check()) will
stop *after* translating the instructions, instead of before as some targets do
currently.

I'd really prefer to keep the current behaviour, and if in the end we see that
the use of DISAS_NORETURN in all targets is safe to switch into DISAS_TOO_MANY,
we can do so in a future patch.

Cheers,
  Lluis

Lluís Vilanova July 15, 2017, 7:56 a.m. UTC | #4

Richard Henderson writes:

> On 07/14/2017 07:26 AM, Richard Henderson wrote:
>> On 07/13/2017 11:26 PM, Lluís Vilanova wrote:
>>> Incrementally paves the way towards using the generic instruction translation
>>> loop.
>>> 
>>> Signed-off-by: Lluís Vilanova <[e-mail]>
>>> Reviewed-by: Richard Henderson <[e-mail]>
>>> ---
>>> target/arm/translate.c |   53 +++++++++++++++++++++++++++++++-----------------
>>> 1 file changed, 34 insertions(+), 19 deletions(-)
>>> 
>>> diff --git a/target/arm/translate.c b/target/arm/translate.c
>>> index b9183fc511..55bef09739 100644
>>> --- a/target/arm/translate.c
>>> +++ b/target/arm/translate.c
>>> @@ -11917,6 +11917,33 @@ static void arm_tr_insn_start(DisasContextBase
>>> *dcbase, CPUState *cpu)
>>> #endif
>>> }
>>> +static bool arm_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cpu,
>>> +                                    const CPUBreakpoint *bp)
>>> +{
>>> +    DisasContext *dc = container_of(dcbase, DisasContext, base);
>>> +
>>> +    if (bp->flags & BP_CPU) {
>>> +        gen_set_condexec(dc);
>>> +        gen_set_pc_im(dc, dc->pc);
>>> +        gen_helper_check_breakpoints(cpu_env);
>>> +        /* End the TB early; it's likely not going to be executed */
>>> +        dc->base.is_jmp = DISAS_UPDATE;
>>> +    } else {
>>> +        gen_exception_internal_insn(dc, 0, EXCP_DEBUG);
>>> +        /* The address covered by the breakpoint must be
>>> +           included in [tb->pc, tb->pc + tb->size) in order
>>> +           to for it to be properly cleared -- thus we
>>> +           increment the PC here so that the logic setting
>>> +           tb->size below does the right thing.  */
>>> +        /* TODO: Advance PC by correct instruction length to
>>> +         * avoid disassembler error messages */
>>> +        dc->pc += 2;
>>> +        dc->base.is_jmp = DISAS_NORETURN;
>>> +    }
>>> +
>>> +    return true;
>>> +}
>>> +
>>> /* generate intermediate code for basic block 'tb'.  */
>>> void gen_intermediate_code(CPUState *cs, TranslationBlock *tb)
>>> {
>>> @@ -11965,28 +11992,16 @@ void gen_intermediate_code(CPUState *cs,
>>> TranslationBlock *tb)
>>> if (unlikely(!QTAILQ_EMPTY(&cs->breakpoints))) {
>>> CPUBreakpoint *bp;
>>> QTAILQ_FOREACH(bp, &cs->breakpoints, entry) {
>>> -                if (bp->pc == dc->pc) {
>>> -                    if (bp->flags & BP_CPU) {
>>> -                        gen_set_condexec(dc);
>>> -                        gen_set_pc_im(dc, dc->pc);
>>> -                        gen_helper_check_breakpoints(cpu_env);
>>> -                        /* End the TB early; it's likely not going to be
>>> executed */
>>> -                        dc->base.is_jmp = DISAS_UPDATE;
>> 
>> Oh I see what you're doing there in the main loop.
>> And I see that you're copying existing behaviour.
>> 
>> That said, I do wonder if there's a better way.
>> 
>> Looking back at the original patch (5d98bf8f), there do not
>> seem to have been any other side effects intended; simply
>> "single step" any insn for which this bp condition is met.
>> 
>> Another way to handle this would be if we could adjust max_insns = num_insns.
>> That would cause the loop to exit after the current insn, with DISAS_TOO_MANY
>> if nothing else.

> Another possibility is is_jmp = DISAS_TOO_MANY, and exit the translation loop
> after the breakpoint check only for is_jmp > DISAS_TOO_MANY.  That allows all of
> the DISAS_TARGET_N values to exit as well.

After a quick check, I see that arm uses both (DISAS_NORETURN and
DISAS_TARGET_N) to exit in different points after the breakpoint. Moxie, mips
and unicore32 use use DISAS_NORETURN, and the rest use DISAS_TARGET_N.

I really don't know if it's safe to unify into a single behaviour. I'm not sure
if some targets will need to differentiate between DISAS_NORETURN and
DISAS_TOO_MANY (e.g., in the tb_stop() hook). As I said, I'd prefer to keep the
current approach that can accommodate all cases, and trim it down afterwards if
we see it's possible. That is, unless you're sure a new proposal can correctly
cover the cases of all targets.

Thanks,
  Lluis

Richard Henderson July 15, 2017, 5:52 p.m. UTC | #5

On 07/14/2017 09:56 PM, Lluís Vilanova wrote:
>> Another possibility is is_jmp = DISAS_TOO_MANY, and exit the translation loop
>> after the breakpoint check only for is_jmp > DISAS_TOO_MANY.  That allows all of
>> the DISAS_TARGET_N values to exit as well.
> 
> After a quick check, I see that arm uses both (DISAS_NORETURN and
> DISAS_TARGET_N) to exit in different points after the breakpoint. Moxie, mips
> and unicore32 use use DISAS_NORETURN, and the rest use DISAS_TARGET_N.

Of course, because those targets have not yet been ported to this generic 
infrastructure.  I'm saying that we *define* the semantics of the hook to use 
DISAS_TOO_MANY to translate one more insn.  That said, ARM is the *only* target 
that will use this feature.  All others will only use NORETURN.

This isn't really any different than the enumerator you were using previously.

Anyway, you can see my thoughts on this in the v14 series I posted last night.


r~

Patch

diff --git a/target/arm/translate.c b/target/arm/translate.c
index b9183fc511..55bef09739 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -11917,6 +11917,33 @@  static void arm_tr_insn_start(DisasContextBase *dcbase, CPUState *cpu)
 #endif
 }
 
+static bool arm_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cpu,
+                                    const CPUBreakpoint *bp)
+{
+    DisasContext *dc = container_of(dcbase, DisasContext, base);
+
+    if (bp->flags & BP_CPU) {
+        gen_set_condexec(dc);
+        gen_set_pc_im(dc, dc->pc);
+        gen_helper_check_breakpoints(cpu_env);
+        /* End the TB early; it's likely not going to be executed */
+        dc->base.is_jmp = DISAS_UPDATE;
+    } else {
+        gen_exception_internal_insn(dc, 0, EXCP_DEBUG);
+        /* The address covered by the breakpoint must be
+           included in [tb->pc, tb->pc + tb->size) in order
+           to for it to be properly cleared -- thus we
+           increment the PC here so that the logic setting
+           tb->size below does the right thing.  */
+        /* TODO: Advance PC by correct instruction length to
+         * avoid disassembler error messages */
+        dc->pc += 2;
+        dc->base.is_jmp = DISAS_NORETURN;
+    }
+
+    return true;
+}
+
 /* generate intermediate code for basic block 'tb'.  */
 void gen_intermediate_code(CPUState *cs, TranslationBlock *tb)
 {
@@ -11965,28 +11992,16 @@  void gen_intermediate_code(CPUState *cs, TranslationBlock *tb)
         if (unlikely(!QTAILQ_EMPTY(&cs->breakpoints))) {
             CPUBreakpoint *bp;
             QTAILQ_FOREACH(bp, &cs->breakpoints, entry) {
-                if (bp->pc == dc->pc) {
-                    if (bp->flags & BP_CPU) {
-                        gen_set_condexec(dc);
-                        gen_set_pc_im(dc, dc->pc);
-                        gen_helper_check_breakpoints(cpu_env);
-                        /* End the TB early; it's likely not going to be executed */
-                        dc->base.is_jmp = DISAS_UPDATE;
-                    } else {
-                        gen_exception_internal_insn(dc, 0, EXCP_DEBUG);
-                        /* The address covered by the breakpoint must be
-                           included in [tb->pc, tb->pc + tb->size) in order
-                           to for it to be properly cleared -- thus we
-                           increment the PC here so that the logic setting
-                           tb->size below does the right thing.  */
-                        /* TODO: Advance PC by correct instruction length to
-                         * avoid disassembler error messages */
-                        dc->pc += 2;
-                        goto done_generating;
+                if (bp->pc == dc->base.pc_next) {
+                    if (arm_tr_breakpoint_check(&dc->base, cs, bp)) {
+                        break;
                     }
-                    break;
                 }
             }
+
+            if (dc->base.is_jmp == DISAS_NORETURN) {
+                break;
+            }
         }
 
         if (dc->base.num_insns == max_insns && (tb->cflags & CF_LAST_IO)) {