diff mbox series

[5/5] tcg: Release mmap_lock on translation fault

Message ID 20190709092049.13771-6-richard.henderson@linaro.org
State Superseded
Headers show
Series tcg: Fix mmap_lock assertion failure, take 2 | expand

Commit Message

Richard Henderson July 9, 2019, 9:20 a.m. UTC
Turn helper_retaddr into a multi-state flag that may now also
indicate when we're performing a read on behalf of the translator.
In this case, release the mmap_lock before the longjmp back to
the main cpu loop, and thereby avoid a failing assert therein.

Fixes: https://bugs.launchpad.net/qemu/+bug/1832353
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

---
 include/exec/cpu_ldst_useronly_template.h | 20 +++++--
 accel/tcg/user-exec.c                     | 65 ++++++++++++++++-------
 2 files changed, 62 insertions(+), 23 deletions(-)

-- 
2.17.1

Comments

Alex Bennée July 9, 2019, 10:37 a.m. UTC | #1
Richard Henderson <richard.henderson@linaro.org> writes:

> Turn helper_retaddr into a multi-state flag that may now also

> indicate when we're performing a read on behalf of the translator.

> In this case, release the mmap_lock before the longjmp back to

> the main cpu loop, and thereby avoid a failing assert therein.

>

> Fixes: https://bugs.launchpad.net/qemu/+bug/1832353

> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>


Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

Tested-by: Alex Bennée <alex.bennee@linaro.org>


> ---

>  include/exec/cpu_ldst_useronly_template.h | 20 +++++--

>  accel/tcg/user-exec.c                     | 65 ++++++++++++++++-------

>  2 files changed, 62 insertions(+), 23 deletions(-)

>

> diff --git a/include/exec/cpu_ldst_useronly_template.h b/include/exec/cpu_ldst_useronly_template.h

> index d663826ac2..35caae8ca6 100644

> --- a/include/exec/cpu_ldst_useronly_template.h

> +++ b/include/exec/cpu_ldst_useronly_template.h

> @@ -64,12 +64,18 @@

>  static inline RES_TYPE

>  glue(glue(cpu_ld, USUFFIX), MEMSUFFIX)(CPUArchState *env, abi_ptr ptr)

>  {

> -#if !defined(CODE_ACCESS)

> +#ifdef CODE_ACCESS

> +    RES_TYPE ret;

> +    set_helper_retaddr(1);

> +    ret = glue(glue(ld, USUFFIX), _p)(g2h(ptr));

> +    clear_helper_retaddr();

> +    return ret;

> +#else

>      trace_guest_mem_before_exec(

>          env_cpu(env), ptr,

>          trace_mem_build_info(SHIFT, false, MO_TE, false));

> -#endif

>      return glue(glue(ld, USUFFIX), _p)(g2h(ptr));

> +#endif

>  }

>

>  #ifndef CODE_ACCESS

> @@ -90,12 +96,18 @@ glue(glue(glue(cpu_ld, USUFFIX), MEMSUFFIX), _ra)(CPUArchState *env,

>  static inline int

>  glue(glue(cpu_lds, SUFFIX), MEMSUFFIX)(CPUArchState *env, abi_ptr ptr)

>  {

> -#if !defined(CODE_ACCESS)

> +#ifdef CODE_ACCESS

> +    int ret;

> +    set_helper_retaddr(1);

> +    ret = glue(glue(ld, USUFFIX), _p)(g2h(ptr));

> +    clear_helper_retaddr();

> +    return ret;

> +#else

>      trace_guest_mem_before_exec(

>          env_cpu(env), ptr,

>          trace_mem_build_info(SHIFT, true, MO_TE, false));

> -#endif

>      return glue(glue(lds, SUFFIX), _p)(g2h(ptr));

> +#endif

>  }

>

>  #ifndef CODE_ACCESS

> diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c

> index 4384b59a4d..5adea629de 100644

> --- a/accel/tcg/user-exec.c

> +++ b/accel/tcg/user-exec.c

> @@ -64,27 +64,55 @@ static inline int handle_cpu_signal(uintptr_t pc, siginfo_t *info,

>      CPUState *cpu = current_cpu;

>      CPUClass *cc;

>      unsigned long address = (unsigned long)info->si_addr;

> -    MMUAccessType access_type;

> +    MMUAccessType access_type = is_write ? MMU_DATA_STORE : MMU_DATA_LOAD;

>

> -    /* We must handle PC addresses from two different sources:

> -     * a call return address and a signal frame address.

> -     *

> -     * Within cpu_restore_state_from_tb we assume the former and adjust

> -     * the address by -GETPC_ADJ so that the address is within the call

> -     * insn so that addr does not accidentally match the beginning of the

> -     * next guest insn.

> -     *

> -     * However, when the PC comes from the signal frame, it points to

> -     * the actual faulting host insn and not a call insn.  Subtracting

> -     * GETPC_ADJ in that case may accidentally match the previous guest insn.

> -     *

> -     * So for the later case, adjust forward to compensate for what

> -     * will be done later by cpu_restore_state_from_tb.

> -     */

> -    if (helper_retaddr) {

> +    switch (helper_retaddr) {

> +    default:

> +        /*

> +         * Fault during host memory operation within a helper function.

> +         * The helper's host return address, saved here, gives us a

> +         * pointer into the generated code that will unwind to the

> +         * correct guest pc.

> +         */

>          pc = helper_retaddr;

> -    } else {

> +        break;

> +

> +    case 0:

> +        /*

> +         * Fault during host memory operation within generated code.

> +         * (Or, a unrelated bug within qemu, but we can't tell from here).

> +         *

> +         * We take the host pc from the signal frame.  However, we cannot

> +         * use that value directly.  Within cpu_restore_state_from_tb, we

> +         * assume PC comes from GETPC(), as used by the helper functions,

> +         * so we adjust the address by -GETPC_ADJ to form an address that

> +         * is within the call insn, so that the address does not accidentially

> +         * match the beginning of the next guest insn.  However, when the

> +         * pc comes fromt he signal frame it points to the actual faulting

> +         * host memory insn and not a call insn.

> +         *

> +         * Therefore, adjust to compensate for what will be done later

> +         * by cpu_restore_state_from_tb.

> +         */

>          pc += GETPC_ADJ;

> +        break;

> +

> +    case 1:

> +        /*

> +         * Fault during host read for translation, or loosely, "execution".

> +         *

> +         * The guest pc is already pointing to the start of the TB for which

> +         * code is being generated.  If the guest translator manages the

> +         * page crossings correctly, this is exactly the correct address

> +         * (and if it doesn't there's little we can do about that here).

> +         * Therefore, do not trigger the unwinder.

> +         *

> +         * Like tb_gen_code, release the memory lock before cpu_loop_exit.

> +         */

> +        pc = 0;

> +        access_type = MMU_INST_FETCH;

> +        mmap_unlock();

> +        break;

>      }

>

>      /* For synchronous signals we expect to be coming from the vCPU

> @@ -155,7 +183,6 @@ static inline int handle_cpu_signal(uintptr_t pc, siginfo_t *info,

>      clear_helper_retaddr();

>

>      cc = CPU_GET_CLASS(cpu);

> -    access_type = is_write ? MMU_DATA_STORE : MMU_DATA_LOAD;

>      cc->tlb_fill(cpu, address, 0, access_type, MMU_USER_IDX, false, pc);

>      g_assert_not_reached();

>  }



--
Alex Bennée
diff mbox series

Patch

diff --git a/include/exec/cpu_ldst_useronly_template.h b/include/exec/cpu_ldst_useronly_template.h
index d663826ac2..35caae8ca6 100644
--- a/include/exec/cpu_ldst_useronly_template.h
+++ b/include/exec/cpu_ldst_useronly_template.h
@@ -64,12 +64,18 @@ 
 static inline RES_TYPE
 glue(glue(cpu_ld, USUFFIX), MEMSUFFIX)(CPUArchState *env, abi_ptr ptr)
 {
-#if !defined(CODE_ACCESS)
+#ifdef CODE_ACCESS
+    RES_TYPE ret;
+    set_helper_retaddr(1);
+    ret = glue(glue(ld, USUFFIX), _p)(g2h(ptr));
+    clear_helper_retaddr();
+    return ret;
+#else
     trace_guest_mem_before_exec(
         env_cpu(env), ptr,
         trace_mem_build_info(SHIFT, false, MO_TE, false));
-#endif
     return glue(glue(ld, USUFFIX), _p)(g2h(ptr));
+#endif
 }
 
 #ifndef CODE_ACCESS
@@ -90,12 +96,18 @@  glue(glue(glue(cpu_ld, USUFFIX), MEMSUFFIX), _ra)(CPUArchState *env,
 static inline int
 glue(glue(cpu_lds, SUFFIX), MEMSUFFIX)(CPUArchState *env, abi_ptr ptr)
 {
-#if !defined(CODE_ACCESS)
+#ifdef CODE_ACCESS
+    int ret;
+    set_helper_retaddr(1);
+    ret = glue(glue(ld, USUFFIX), _p)(g2h(ptr));
+    clear_helper_retaddr();
+    return ret;
+#else
     trace_guest_mem_before_exec(
         env_cpu(env), ptr,
         trace_mem_build_info(SHIFT, true, MO_TE, false));
-#endif
     return glue(glue(lds, SUFFIX), _p)(g2h(ptr));
+#endif
 }
 
 #ifndef CODE_ACCESS
diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c
index 4384b59a4d..5adea629de 100644
--- a/accel/tcg/user-exec.c
+++ b/accel/tcg/user-exec.c
@@ -64,27 +64,55 @@  static inline int handle_cpu_signal(uintptr_t pc, siginfo_t *info,
     CPUState *cpu = current_cpu;
     CPUClass *cc;
     unsigned long address = (unsigned long)info->si_addr;
-    MMUAccessType access_type;
+    MMUAccessType access_type = is_write ? MMU_DATA_STORE : MMU_DATA_LOAD;
 
-    /* We must handle PC addresses from two different sources:
-     * a call return address and a signal frame address.
-     *
-     * Within cpu_restore_state_from_tb we assume the former and adjust
-     * the address by -GETPC_ADJ so that the address is within the call
-     * insn so that addr does not accidentally match the beginning of the
-     * next guest insn.
-     *
-     * However, when the PC comes from the signal frame, it points to
-     * the actual faulting host insn and not a call insn.  Subtracting
-     * GETPC_ADJ in that case may accidentally match the previous guest insn.
-     *
-     * So for the later case, adjust forward to compensate for what
-     * will be done later by cpu_restore_state_from_tb.
-     */
-    if (helper_retaddr) {
+    switch (helper_retaddr) {
+    default:
+        /*
+         * Fault during host memory operation within a helper function.
+         * The helper's host return address, saved here, gives us a
+         * pointer into the generated code that will unwind to the
+         * correct guest pc.
+         */
         pc = helper_retaddr;
-    } else {
+        break;
+
+    case 0:
+        /*
+         * Fault during host memory operation within generated code.
+         * (Or, a unrelated bug within qemu, but we can't tell from here).
+         *
+         * We take the host pc from the signal frame.  However, we cannot
+         * use that value directly.  Within cpu_restore_state_from_tb, we
+         * assume PC comes from GETPC(), as used by the helper functions,
+         * so we adjust the address by -GETPC_ADJ to form an address that
+         * is within the call insn, so that the address does not accidentially
+         * match the beginning of the next guest insn.  However, when the
+         * pc comes fromt he signal frame it points to the actual faulting
+         * host memory insn and not a call insn.
+         *
+         * Therefore, adjust to compensate for what will be done later
+         * by cpu_restore_state_from_tb.
+         */
         pc += GETPC_ADJ;
+        break;
+
+    case 1:
+        /*
+         * Fault during host read for translation, or loosely, "execution".
+         * 
+         * The guest pc is already pointing to the start of the TB for which
+         * code is being generated.  If the guest translator manages the
+         * page crossings correctly, this is exactly the correct address
+         * (and if it doesn't there's little we can do about that here).
+         * Therefore, do not trigger the unwinder.
+         *
+         * Like tb_gen_code, release the memory lock before cpu_loop_exit.
+         */
+        pc = 0;
+        access_type = MMU_INST_FETCH;
+        mmap_unlock();
+        break;
     }
 
     /* For synchronous signals we expect to be coming from the vCPU
@@ -155,7 +183,6 @@  static inline int handle_cpu_signal(uintptr_t pc, siginfo_t *info,
     clear_helper_retaddr();
 
     cc = CPU_GET_CLASS(cpu);
-    access_type = is_write ? MMU_DATA_STORE : MMU_DATA_LOAD;
     cc->tlb_fill(cpu, address, 0, access_type, MMU_USER_IDX, false, pc);
     g_assert_not_reached();
 }