diff mbox

target/i386: move nested exception check to x86_cpu_exec_interrupt

Message ID 20170306155722.31315-1-alex.bennee@linaro.org
State New
Headers show

Commit Message

Alex Bennée March 6, 2017, 3:57 p.m. UTC 
(DUMB CODE MOTION COMMIT WITHOUT FULL UNDERSTANDING OF X86 NEEDS REVIEW)

During code generation cpu_svm_check_intercept_param can through a
sequence of events generated a tlb_fill which fails due to a double
tb_lock(). Moving the checking to x86_cpu_exec_interrupt where it can
take the lock at will.

Reported-by: Alexander Boettcher <alexander.boettcher@genode-labs.com>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

CC: Richard Henderson <rth@twiddle.net>
---
 target/i386/cpu.h         |  1 +
 target/i386/excp_helper.c | 54 +------------------------------------------
 target/i386/seg_helper.c  | 59 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 61 insertions(+), 53 deletions(-)

-- 
2.11.0

Comments

Richard Henderson March 6, 2017, 7:24 p.m. UTC | #1 
On 03/07/2017 02:57 AM, Alex Bennée wrote:
>      cs->exception_index = intno;

>      env->error_code = error_code;

>      env->exception_is_int = is_int;

>      env->exception_next_eip = env->eip + next_eip_addend;

> +    env->exception_retaddr = retaddr;

>      cpu_loop_exit_restore(cs, retaddr);


Since you've restored state here, I don't see that you need to save the retaddr 
at all ...

> +    if (!env->exception_is_int) {

> +        cpu_svm_check_intercept_param(env,

> +                                      SVM_EXIT_EXCP_BASE + cs->exception_index,

> +                                      env->error_code,

> +                                      env->exception_retaddr);

> +        cs->exception_index = check_exception(env, cs->exception_index,

> +                                              &env->error_code,

> +                                              env->exception_retaddr);

> +    } else {

> +        cpu_svm_check_intercept_param(env, SVM_EXIT_SWINT, 0,

> +                                      env->exception_retaddr);

> +    }


... simply pass zero here instead.

But do I see correctly from the thread that we don't have a reproducer?


r~
Alex Bennée March 7, 2017, 3:03 p.m. UTC | #2 
Richard Henderson <rth@twiddle.net> writes:

> On 03/07/2017 02:57 AM, Alex Bennée wrote:

>>      cs->exception_index = intno;

>>      env->error_code = error_code;

>>      env->exception_is_int = is_int;

>>      env->exception_next_eip = env->eip + next_eip_addend;

>> +    env->exception_retaddr = retaddr;

>>      cpu_loop_exit_restore(cs, retaddr);

>

> Since you've restored state here, I don't see that you need to save

> the retaddr at all ...

>

>> +    if (!env->exception_is_int) {

>> +        cpu_svm_check_intercept_param(env,

>> +                                      SVM_EXIT_EXCP_BASE + cs->exception_index,

>> +                                      env->error_code,

>> +                                      env->exception_retaddr);

>> +        cs->exception_index = check_exception(env, cs->exception_index,

>> +                                              &env->error_code,

>> +                                              env->exception_retaddr);

>> +    } else {

>> +        cpu_svm_check_intercept_param(env, SVM_EXIT_SWINT, 0,

>> +                                      env->exception_retaddr);

>> +    }

>

> ... simply pass zero here instead.

>

> But do I see correctly from the thread that we don't have a reproducer?


Yeah - I've nabbed Paolo's much cleaner and more informed fix and that
seems fine.

--
Alex Bennée
diff mbox

Patch

diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index fb09aee7f8..07c591672c 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -1105,6 +1105,7 @@  typedef struct CPUX86State {
     /* exception/interrupt handling */
     int error_code;
     int exception_is_int;
+    uintptr_t exception_retaddr;
     target_ulong exception_next_eip;
     target_ulong dr[8]; /* debug registers; note dr4 and dr5 are unused */
     union {
diff --git a/target/i386/excp_helper.c b/target/i386/excp_helper.c
index ee596c6082..0a1a02cdd4 100644
--- a/target/i386/excp_helper.c
+++ b/target/i386/excp_helper.c
@@ -35,51 +35,6 @@  void helper_raise_exception(CPUX86State *env, int exception_index)
 }
 
 /*
- * Check nested exceptions and change to double or triple fault if
- * needed. It should only be called, if this is not an interrupt.
- * Returns the new exception number.
- */
-static int check_exception(CPUX86State *env, int intno, int *error_code,
-                           uintptr_t retaddr)
-{
-    int first_contributory = env->old_exception == 0 ||
-                              (env->old_exception >= 10 &&
-                               env->old_exception <= 13);
-    int second_contributory = intno == 0 ||
-                               (intno >= 10 && intno <= 13);
-
-    qemu_log_mask(CPU_LOG_INT, "check_exception old: 0x%x new 0x%x\n",
-                env->old_exception, intno);
-
-#if !defined(CONFIG_USER_ONLY)
-    if (env->old_exception == EXCP08_DBLE) {
-        if (env->hflags & HF_SVMI_MASK) {
-            cpu_vmexit(env, SVM_EXIT_SHUTDOWN, 0, retaddr); /* does not return */
-        }
-
-        qemu_log_mask(CPU_LOG_RESET, "Triple fault\n");
-
-        qemu_system_reset_request();
-        return EXCP_HLT;
-    }
-#endif
-
-    if ((first_contributory && second_contributory)
-        || (env->old_exception == EXCP0E_PAGE &&
-            (second_contributory || (intno == EXCP0E_PAGE)))) {
-        intno = EXCP08_DBLE;
-        *error_code = 0;
-    }
-
-    if (second_contributory || (intno == EXCP0E_PAGE) ||
-        (intno == EXCP08_DBLE)) {
-        env->old_exception = intno;
-    }
-
-    return intno;
-}
-
-/*
  * Signal an interruption. It is executed in the main CPU loop.
  * is_int is TRUE if coming from the int instruction. next_eip is the
  * env->eip value AFTER the interrupt instruction. It is only relevant if
@@ -92,18 +47,11 @@  static void QEMU_NORETURN raise_interrupt2(CPUX86State *env, int intno,
 {
     CPUState *cs = CPU(x86_env_get_cpu(env));
 
-    if (!is_int) {
-        cpu_svm_check_intercept_param(env, SVM_EXIT_EXCP_BASE + intno,
-                                      error_code, retaddr);
-        intno = check_exception(env, intno, &error_code, retaddr);
-    } else {
-        cpu_svm_check_intercept_param(env, SVM_EXIT_SWINT, 0, retaddr);
-    }
-
     cs->exception_index = intno;
     env->error_code = error_code;
     env->exception_is_int = is_int;
     env->exception_next_eip = env->eip + next_eip_addend;
+    env->exception_retaddr = retaddr;
     cpu_loop_exit_restore(cs, retaddr);
 }
 
diff --git a/target/i386/seg_helper.c b/target/i386/seg_helper.c
index 5c845dc25c..055b6d4025 100644
--- a/target/i386/seg_helper.c
+++ b/target/i386/seg_helper.c
@@ -25,6 +25,7 @@ 
 #include "exec/exec-all.h"
 #include "exec/cpu_ldst.h"
 #include "exec/log.h"
+#include "sysemu/sysemu.h"
 
 //#define DEBUG_PCALL
 
@@ -1314,12 +1315,70 @@  void do_interrupt_x86_hardirq(CPUX86State *env, int intno, int is_hw)
     do_interrupt_all(x86_env_get_cpu(env), intno, 0, 0, 0, is_hw);
 }
 
+/*
+ * Check nested exceptions and change to double or triple fault if
+ * needed. It should only be called, if this is not an interrupt.
+ * Returns the new exception number.
+ */
+static int check_exception(CPUX86State *env, int intno, int *error_code,
+                           uintptr_t retaddr)
+{
+    int first_contributory = env->old_exception == 0 ||
+                              (env->old_exception >= 10 &&
+                               env->old_exception <= 13);
+    int second_contributory = intno == 0 ||
+                               (intno >= 10 && intno <= 13);
+
+    qemu_log_mask(CPU_LOG_INT, "check_exception old: 0x%x new 0x%x\n",
+                env->old_exception, intno);
+
+#if !defined(CONFIG_USER_ONLY)
+    if (env->old_exception == EXCP08_DBLE) {
+        if (env->hflags & HF_SVMI_MASK) {
+            cpu_vmexit(env, SVM_EXIT_SHUTDOWN, 0, retaddr); /* does not return */
+        }
+
+        qemu_log_mask(CPU_LOG_RESET, "Triple fault\n");
+
+        qemu_system_reset_request();
+        return EXCP_HLT;
+    }
+#endif
+
+    if ((first_contributory && second_contributory)
+        || (env->old_exception == EXCP0E_PAGE &&
+            (second_contributory || (intno == EXCP0E_PAGE)))) {
+        intno = EXCP08_DBLE;
+        *error_code = 0;
+    }
+
+    if (second_contributory || (intno == EXCP0E_PAGE) ||
+        (intno == EXCP08_DBLE)) {
+        env->old_exception = intno;
+    }
+
+    return intno;
+}
+
 bool x86_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
 {
     X86CPU *cpu = X86_CPU(cs);
     CPUX86State *env = &cpu->env;
     bool ret = false;
 
+    if (!env->exception_is_int) {
+        cpu_svm_check_intercept_param(env,
+                                      SVM_EXIT_EXCP_BASE + cs->exception_index,
+                                      env->error_code,
+                                      env->exception_retaddr);
+        cs->exception_index = check_exception(env, cs->exception_index,
+                                              &env->error_code,
+                                              env->exception_retaddr);
+    } else {
+        cpu_svm_check_intercept_param(env, SVM_EXIT_SWINT, 0,
+                                      env->exception_retaddr);
+    }
+
 #if !defined(CONFIG_USER_ONLY)
     if (interrupt_request & CPU_INTERRUPT_POLL) {
         cs->interrupt_request &= ~CPU_INTERRUPT_POLL;