diff mbox series

[v12,12/24] tcg: handle EXCP_ATOMIC exception for system emulation

Message ID 20170213121017.12907-13-alex.bennee@linaro.org
State New
Headers show
Series MTTCG Base enabling patches with ARM enablement | expand

Commit Message

Alex Bennée Feb. 13, 2017, 12:10 p.m. UTC 
From: Pranith Kumar <bobby.prani@gmail.com>


The patch enables handling atomic code in the guest. This should be
preferably done in cpu_handle_exception(), but the current assumptions
regarding when we can execute atomic sections cause a deadlock.

The current mechanism discards the flags which were set in atomic
execution. We ensure they are properly saved by calling the
cc->cpu_exec_enter/leave() functions around the loop.

As we are running cpu_exec_step_atomic() from the outermost loop we
need to avoid an abort() when single stepping over atomic code since
debug exception longjmp will point to the the setlongjmp in
cpu_exec(). We do this by setting a new jmp_env so that it jumps back
here on an exception.

Signed-off-by: Pranith Kumar <bobby.prani@gmail.com>

[AJB: tweak title, merge with new patches]
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

[PENDING, CHANGES] Reviewed-by: Richard Henderson <rth@twiddle.net>
CC: Richard Henderson <rth@twiddle.net>
CC: Paolo Bonzini <pbonzini@redhat.com>
---
 cpu-exec.c | 14 +++++++++++---
 cpus.c     |  9 +++++++++
 2 files changed, 20 insertions(+), 3 deletions(-)

-- 
2.11.0

Comments

Richard Henderson Feb. 13, 2017, 7:19 p.m. UTC | #1 
On 02/13/2017 11:10 PM, Alex Bennée wrote:
> @@ -239,9 +240,16 @@ static void cpu_exec_step(CPUState *cpu)

>                       1 | CF_NOCACHE | CF_IGNORE_ICOUNT);

>      tb->orig_tb = NULL;

>      tb_unlock();

> -    /* execute the generated code */

> -    trace_exec_tb_nocache(tb, pc);

> -    cpu_tb_exec(cpu, tb);

> +

> +    cc->cpu_exec_enter(cpu);

> +

> +    if (sigsetjmp(cpu->jmp_env, 0) == 0) {

> +        /* execute the generated code */

> +        trace_exec_tb_nocache(tb, pc);

> +        cpu_tb_exec(cpu, tb);

> +    }


I don't understand this, since cpu_tb_exec has its own sigsetjmp.  Where is the 
exception supposed to come from that escapes?

> +                } else if (r == EXCP_ATOMIC) {

> +                    qemu_mutex_unlock_iothread();

> +                    cpu_exec_step_atomic(cpu);

> +                    qemu_mutex_lock_iothread();

...
> +            case EXCP_ATOMIC:

> +                qemu_mutex_unlock_iothread();

> +                cpu_exec_step_atomic(cpu);

> +                qemu_mutex_lock_iothread();



I just noticed this, but if you have to do a v13, it might be best to move 
these locks inside cpu_exec_step_atomic, as with tcg_cpu_exec.  Otherwise leave 
it for later.


r~
Pranith Kumar Feb. 13, 2017, 7:33 p.m. UTC | #2 
On Mon, Feb 13, 2017 at 2:19 PM, Richard Henderson <rth@twiddle.net> wrote:
> On 02/13/2017 11:10 PM, Alex Bennée wrote:

>>

>> @@ -239,9 +240,16 @@ static void cpu_exec_step(CPUState *cpu)

>>                       1 | CF_NOCACHE | CF_IGNORE_ICOUNT);

>>      tb->orig_tb = NULL;

>>      tb_unlock();

>> -    /* execute the generated code */

>> -    trace_exec_tb_nocache(tb, pc);

>> -    cpu_tb_exec(cpu, tb);

>> +

>> +    cc->cpu_exec_enter(cpu);

>> +

>> +    if (sigsetjmp(cpu->jmp_env, 0) == 0) {

>> +        /* execute the generated code */

>> +        trace_exec_tb_nocache(tb, pc);

>> +        cpu_tb_exec(cpu, tb);

>> +    }

>

>

> I don't understand this, since cpu_tb_exec has its own sigsetjmp.  Where is

> the exception supposed to come from that escapes?


cpu_exec() has its own sigsetjmp, not cpu_tb_exec(). The exception is
the debug exception from the generated code. Without this new
sigsetjmp, it'll jump to cpu_exec() instead of coming back here.

Thanks,
-- 
Pranith
Richard Henderson Feb. 13, 2017, 7:57 p.m. UTC | #3 
On 02/14/2017 06:33 AM, Pranith Kumar wrote:
> On Mon, Feb 13, 2017 at 2:19 PM, Richard Henderson <rth@twiddle.net> wrote:

>> On 02/13/2017 11:10 PM, Alex Bennée wrote:

>>>

>>> @@ -239,9 +240,16 @@ static void cpu_exec_step(CPUState *cpu)

>>>                       1 | CF_NOCACHE | CF_IGNORE_ICOUNT);

>>>      tb->orig_tb = NULL;

>>>      tb_unlock();

>>> -    /* execute the generated code */

>>> -    trace_exec_tb_nocache(tb, pc);

>>> -    cpu_tb_exec(cpu, tb);

>>> +

>>> +    cc->cpu_exec_enter(cpu);

>>> +

>>> +    if (sigsetjmp(cpu->jmp_env, 0) == 0) {

>>> +        /* execute the generated code */

>>> +        trace_exec_tb_nocache(tb, pc);

>>> +        cpu_tb_exec(cpu, tb);

>>> +    }

>>

>>

>> I don't understand this, since cpu_tb_exec has its own sigsetjmp.  Where is

>> the exception supposed to come from that escapes?

>

> cpu_exec() has its own sigsetjmp, not cpu_tb_exec(). The exception is

> the debug exception from the generated code. Without this new

> sigsetjmp, it'll jump to cpu_exec() instead of coming back here.


Bah.  Sorry, ENOCOFFEE.

Reviewed-by: Richard Henderson <rth@twiddle.net>



r~
Alex Bennée Feb. 14, 2017, 10:50 a.m. UTC | #4 
Richard Henderson <rth@twiddle.net> writes:

> On 02/13/2017 11:10 PM, Alex Bennée wrote:

>> @@ -239,9 +240,16 @@ static void cpu_exec_step(CPUState *cpu)

<snip>
>> +                } else if (r == EXCP_ATOMIC) {

>> +                    qemu_mutex_unlock_iothread();

>> +                    cpu_exec_step_atomic(cpu);

>> +                    qemu_mutex_lock_iothread();

> ...

>> +            case EXCP_ATOMIC:

>> +                qemu_mutex_unlock_iothread();

>> +                cpu_exec_step_atomic(cpu);

>> +                qemu_mutex_lock_iothread();

>

>

> I just noticed this, but if you have to do a v13, it might be best to

> move these locks inside cpu_exec_step_atomic, as with tcg_cpu_exec.

> Otherwise leave it for later.


Will that work given cpu_exec_step_atomic() is common between linux-user
and system emulation?

--
Alex Bennée
Richard Henderson Feb. 15, 2017, 9:53 p.m. UTC | #5 
On 02/14/2017 09:50 PM, Alex Bennée wrote:
>

> Richard Henderson <rth@twiddle.net> writes:

>

>> On 02/13/2017 11:10 PM, Alex Bennée wrote:

>>> @@ -239,9 +240,16 @@ static void cpu_exec_step(CPUState *cpu)

> <snip>

>>> +                } else if (r == EXCP_ATOMIC) {

>>> +                    qemu_mutex_unlock_iothread();

>>> +                    cpu_exec_step_atomic(cpu);

>>> +                    qemu_mutex_lock_iothread();

>> ...

>>> +            case EXCP_ATOMIC:

>>> +                qemu_mutex_unlock_iothread();

>>> +                cpu_exec_step_atomic(cpu);

>>> +                qemu_mutex_lock_iothread();

>>

>>

>> I just noticed this, but if you have to do a v13, it might be best to

>> move these locks inside cpu_exec_step_atomic, as with tcg_cpu_exec.

>> Otherwise leave it for later.

>

> Will that work given cpu_exec_step_atomic() is common between linux-user

> and system emulation?


Ug.  No, you're right.


r~
diff mbox series

Patch

diff --git a/cpu-exec.c b/cpu-exec.c
index b0ddada8c1..e61f5747c8 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -228,6 +228,7 @@  static void cpu_exec_nocache(CPUState *cpu, int max_cycles,
 
 static void cpu_exec_step(CPUState *cpu)
 {
+    CPUClass *cc = CPU_GET_CLASS(cpu);
     CPUArchState *env = (CPUArchState *)cpu->env_ptr;
     TranslationBlock *tb;
     target_ulong cs_base, pc;
@@ -239,9 +240,16 @@  static void cpu_exec_step(CPUState *cpu)
                      1 | CF_NOCACHE | CF_IGNORE_ICOUNT);
     tb->orig_tb = NULL;
     tb_unlock();
-    /* execute the generated code */
-    trace_exec_tb_nocache(tb, pc);
-    cpu_tb_exec(cpu, tb);
+
+    cc->cpu_exec_enter(cpu);
+
+    if (sigsetjmp(cpu->jmp_env, 0) == 0) {
+        /* execute the generated code */
+        trace_exec_tb_nocache(tb, pc);
+        cpu_tb_exec(cpu, tb);
+    }
+
+    cc->cpu_exec_exit(cpu);
     tb_lock();
     tb_phys_invalidate(tb, -1);
     tb_free(tb);
diff --git a/cpus.c b/cpus.c
index 25897edbd3..cb44544fcf 100644
--- a/cpus.c
+++ b/cpus.c
@@ -1347,6 +1347,11 @@  static void *qemu_tcg_rr_cpu_thread_fn(void *arg)
                 if (r == EXCP_DEBUG) {
                     cpu_handle_guest_debug(cpu);
                     break;
+                } else if (r == EXCP_ATOMIC) {
+                    qemu_mutex_unlock_iothread();
+                    cpu_exec_step_atomic(cpu);
+                    qemu_mutex_lock_iothread();
+                    break;
                 }
             } else if (cpu->stop) {
                 if (cpu->unplug) {
@@ -1457,6 +1462,10 @@  static void *qemu_tcg_cpu_thread_fn(void *arg)
                  */
                 g_assert(cpu->halted);
                 break;
+            case EXCP_ATOMIC:
+                qemu_mutex_unlock_iothread();
+                cpu_exec_step_atomic(cpu);
+                qemu_mutex_lock_iothread();
             default:
                 /* Ignore everything else? */
                 break;