Re: [Qemu-devel] [PATCH 5/5] tcg: Release mmap_lock on translation fault

["Alex Bennée" <alex.bennee@linaro.org>]

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