(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(-)
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;
--
2.11.0
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~
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
© 2016 - 2024 Red Hat, Inc.