[Qemu-devel] [PATCH] target-arm: Fix and improve AA32 singlestep translation completion code

[Qemu-devel] [PATCH] target-arm: Fix and improve AA32 singlestep translation completion code

[Sergey Fedorov]

The AArch32 translation completion code for singlestep enabled/active
case was a way more confusing and too repetitive then it needs to be.
Probably that was the cause for a bug to be introduced into it at some
point. The bug was that SWI/HVC/SMC exception would be generated in
condition-failed instruction code path whereas it shouldn't.

This patch rewrites the code in a way similar to the non-singlestep
case.

In the condition-passed/unconditional instruction code path we need to:
 - Write the condexec bits back to the CPU state
 - Advance the singlestep state machine and generate a corresponding
   exception in case of SWI/HVC/SMC
 - Write the PC back to the CPU state if it hasn't already been written
   and generate an appropriate singlestep exception otherwise

In the condition-failed instruction code path we need to:
 - Set a TCG label to jump to it if the condition is failed
 - Write the condexec bits back to the CPU state
 - Write the PC back to the CPU state since it hasn't been written in
   this case
 - Generate an appropriate singlestep exception

Signed-off-by: Sergey Fedorov <serge.fdrv@gmail.com>
---
 target-arm/translate.c | 65 ++++++++++++++++++++++++--------------------------
 1 file changed, 31 insertions(+), 34 deletions(-)

diff --git a/target-arm/translate.c b/target-arm/translate.c
index 5d22879..c9eeb09 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -11480,48 +11480,45 @@ void gen_intermediate_code(CPUARMState *env, TranslationBlock *tb)
        instruction was a conditional branch or trap, and the PC has
        already been written.  */
     if (unlikely(cs->singlestep_enabled || dc->ss_active)) {
-        /* Make sure the pc is updated, and raise a debug exception.  */
-        if (dc->condjmp) {
-            gen_set_condexec(dc);
-            if (dc->is_jmp == DISAS_SWI) {
-                gen_ss_advance(dc);
-                gen_exception(EXCP_SWI, syn_aa32_svc(dc->svc_imm, dc->thumb),
-                              default_exception_el(dc));
-            } else if (dc->is_jmp == DISAS_HVC) {
-                gen_ss_advance(dc);
-                gen_exception(EXCP_HVC, syn_aa32_hvc(dc->svc_imm), 2);
-            } else if (dc->is_jmp == DISAS_SMC) {
-                gen_ss_advance(dc);
-                gen_exception(EXCP_SMC, syn_aa32_smc(), 3);
-            } else if (dc->ss_active) {
-                gen_step_complete_exception(dc);
-            } else {
-                gen_exception_internal(EXCP_DEBUG);
-            }
-            gen_set_label(dc->condlabel);
-        }
-        if (dc->condjmp || dc->is_jmp == DISAS_NEXT ||
-            dc->is_jmp == DISAS_UPDATE) {
-            gen_set_pc_im(dc, dc->pc);
-            dc->condjmp = 0;
-        }
+        /* Unconditional and "condition passed" instruction codepath. */
         gen_set_condexec(dc);
-        if (dc->is_jmp == DISAS_SWI && !dc->condjmp) {
+        switch (dc->is_jmp) {
+        case DISAS_SWI:
             gen_ss_advance(dc);
             gen_exception(EXCP_SWI, syn_aa32_svc(dc->svc_imm, dc->thumb),
                           default_exception_el(dc));
-        } else if (dc->is_jmp == DISAS_HVC && !dc->condjmp) {
+            break;
+        case DISAS_HVC:
             gen_ss_advance(dc);
             gen_exception(EXCP_HVC, syn_aa32_hvc(dc->svc_imm), 2);
-        } else if (dc->is_jmp == DISAS_SMC && !dc->condjmp) {
+            break;
+        case DISAS_SMC:
             gen_ss_advance(dc);
             gen_exception(EXCP_SMC, syn_aa32_smc(), 3);
-        } else if (dc->ss_active) {
-            gen_step_complete_exception(dc);
-        } else {
-            /* FIXME: Single stepping a WFI insn will not halt
-               the CPU.  */
-            gen_exception_internal(EXCP_DEBUG);
+            break;
+        case DISAS_NEXT:
+        case DISAS_UPDATE:
+            gen_set_pc_im(dc, dc->pc);
+            /* fall through */
+        default:
+            if (dc->ss_active) {
+                gen_step_complete_exception(dc);
+            } else {
+                /* FIXME: Single stepping a WFI insn will not halt
+                   the CPU.  */
+                gen_exception_internal(EXCP_DEBUG);
+            }
+        }
+        if (dc->condjmp) {
+            /* "Condition failed" instruction codepath. */
+            gen_set_label(dc->condlabel);
+            gen_set_condexec(dc);
+            gen_set_pc_im(dc, dc->pc);
+            if (dc->ss_active) {
+                gen_step_complete_exception(dc);
+            } else {
+                gen_exception_internal(EXCP_DEBUG);
+            }
         }
     } else {
         /* While branches must always occur at the end of an IT block,
-- 
1.9.1

Re: [Qemu-devel] [PATCH] target-arm: Fix and improve AA32 singlestep translation completion code

[Peter Maydell]

On 25 November 2015 at 18:02, Sergey Fedorov <serge.fdrv@gmail.com> wrote:
> The AArch32 translation completion code for singlestep enabled/active
> case was a way more confusing and too repetitive then it needs to be.
> Probably that was the cause for a bug to be introduced into it at some
> point. The bug was that SWI/HVC/SMC exception would be generated in
> condition-failed instruction code path whereas it shouldn't.

So I did some testing, and I think this is a bug that's not actually
really visible to Linux guests. For both QEMU's gdbstub and for gdb
running within a system emulation, gdb for 32-bit ARM will prefer to
do singlestep via setting breakpoints rather than trying to use the
gdbstub's singlestep command. So while we should definitely fix it
(and the code cleanup is nice) I think we don't need to do this for 2.5,
and I'm going to put this on my review-for-2.6 list. Do you agree?

thanks
-- PMM

Re: [Qemu-devel] [PATCH] target-arm: Fix and improve AA32 singlestep translation completion code

[Sergey Fedorov]

On 26.11.2015 15:33, Peter Maydell wrote:
> On 25 November 2015 at 18:02, Sergey Fedorov <serge.fdrv@gmail.com> wrote:
>> The AArch32 translation completion code for singlestep enabled/active
>> case was a way more confusing and too repetitive then it needs to be.
>> Probably that was the cause for a bug to be introduced into it at some
>> point. The bug was that SWI/HVC/SMC exception would be generated in
>> condition-failed instruction code path whereas it shouldn't.
> So I did some testing, and I think this is a bug that's not actually
> really visible to Linux guests. For both QEMU's gdbstub and for gdb
> running within a system emulation, gdb for 32-bit ARM will prefer to
> do singlestep via setting breakpoints rather than trying to use the
> gdbstub's singlestep command. So while we should definitely fix it
> (and the code cleanup is nice) I think we don't need to do this for 2.5,
> and I'm going to put this on my review-for-2.6 list. Do you agree?

Sure, that's okay. I just wanted to finish this before I move on to
something else.

BTW, I used the following quick-and-dirty Perl script to do testing (it
was helpful to detect some bugs in my first attempts):

#!/usr/bin/perl

use strict;
use warnings;

use IO::Socket::INET;

our $addr = 'localhost:1234';

sub recv_pack {
    my $sock = shift;
    my $c = $sock->getc() || die();
    if ($c eq '+') {
        return $c;
    }
    if ($c eq '-') {
        die;
    }
    if ($c eq '$') {
        my $packet = $c;
        while (($c = $sock->getc()) ne '#') {
            defined($c) || die();
            $packet .= $c;
        }
        $sock->getc();
        $sock->getc();
        $sock->print('+') || die();
        return $packet;
    }
    return "";
}

sub wait_ack {
    my $sock = shift;
    my $pack = recv_pack($sock);
    while ($pack ne "+") {
        $pack = recv_pack($sock);
    }
}

sub send_pack {
    my $sock = shift;
    my $packet = shift;
    my $sum = unpack("%8C*", $packet);
    $packet = '$' . $packet . '#' . sprintf("%hhx", $sum);
    $sock->print($packet) || die();
    wait_ack($sock);
}

our $sock = IO::Socket::INET->new($addr) || die();

our $quit = 0;

$SIG{INT} = sub { $quit = 1; };

my $nr_packets = 0;

while (!$quit) {
    send_pack($sock, 's');
    recv_pack($sock);
    printf("\r%d packets sent", ++$nr_packets);
    STDOUT->flush();
}

print("\n");

send_pack($sock, 'c');

Best regards,
Sergey

Re: [Qemu-devel] [PATCH] target-arm: Fix and improve AA32 singlestep translation completion code

[Peter Maydell]

On 25 November 2015 at 18:02, Sergey Fedorov <serge.fdrv@gmail.com> wrote:
> The AArch32 translation completion code for singlestep enabled/active
> case was a way more confusing and too repetitive then it needs to be.
> Probably that was the cause for a bug to be introduced into it at some
> point. The bug was that SWI/HVC/SMC exception would be generated in
> condition-failed instruction code path whereas it shouldn't.
>
> This patch rewrites the code in a way similar to the non-singlestep
> case.
>
> In the condition-passed/unconditional instruction code path we need to:
>  - Write the condexec bits back to the CPU state
>  - Advance the singlestep state machine and generate a corresponding
>    exception in case of SWI/HVC/SMC
>  - Write the PC back to the CPU state if it hasn't already been written
>    and generate an appropriate singlestep exception otherwise
>
> In the condition-failed instruction code path we need to:
>  - Set a TCG label to jump to it if the condition is failed
>  - Write the condexec bits back to the CPU state
>  - Write the PC back to the CPU state since it hasn't been written in
>    this case
>  - Generate an appropriate singlestep exception
>
> Signed-off-by: Sergey Fedorov <serge.fdrv@gmail.com>

This looks much clearer than the code we had, and the parallel
between the singlestep code and the non-singlestep code is nice.

Applied to target-arm.next, thanks.

-- PMM