| Message ID | 20190709163656.3100-6-richard.henderson@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | tcg: Fix mmap_lock assertion failure, take 2 | expand |
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> with check-tcg this time Tested-by: Alex Bennée <alex.bennee@linaro.org> > --- > include/exec/cpu_ldst_useronly_template.h | 20 +++++-- > accel/tcg/user-exec.c | 66 ++++++++++++++++------- > 2 files changed, 63 insertions(+), 23 deletions(-) > > diff --git a/include/exec/cpu_ldst_useronly_template.h b/include/exec/cpu_ldst_useronly_template.h > index d663826ac2..2378f2958c 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(lds, SUFFIX), _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..897d1571c4 100644 > --- a/accel/tcg/user-exec.c > +++ b/accel/tcg/user-exec.c > @@ -64,27 +64,56 @@ 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 from the signal frame it points to the actual faulting > + * host memory insn and not the return from 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 the translator doesn't handle page boundaries correctly > + * 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 +184,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 --git a/include/exec/cpu_ldst_useronly_template.h b/include/exec/cpu_ldst_useronly_template.h index d663826ac2..2378f2958c 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(lds, SUFFIX), _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..897d1571c4 100644 --- a/accel/tcg/user-exec.c +++ b/accel/tcg/user-exec.c @@ -64,27 +64,56 @@ 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 from the signal frame it points to the actual faulting + * host memory insn and not the return from 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 the translator doesn't handle page boundaries correctly + * 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 +184,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(); }
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 | 66 ++++++++++++++++------- 2 files changed, 63 insertions(+), 23 deletions(-) -- 2.17.1