From b4bf3c776becf1d106c30eafe450fdb7158565ae Mon Sep 17 00:00:00 2001 From: Pavel Dovgalyuk Date: Wed, 11 Apr 2018 19:57:23 -0400 Subject: [PATCH] icount: fix cpu_restore_state_from_tb for non-tb-exit cases In icount mode, instructions that access io memory spaces in the middle of the translation block invoke TB recompilation. After recompilation, such instructions become last in the TB and are allowed to access io memory spaces. When the code includes instruction like i386 'xchg eax, 0xffffd080' which accesses APIC, QEMU goes into an infinite loop of the recompilation. This instruction includes two memory accesses - one read and one write. After the first access, APIC calls cpu_report_tpr_access, which restores the CPU state to get the current eip. But cpu_restore_state_from_tb resets the cpu->can_do_io flag which makes the second memory access invalid. Therefore the second memory access causes a recompilation of the block. Then these operations repeat again and again. This patch moves resetting cpu->can_do_io flag from cpu_restore_state_from_tb to cpu_loop_exit* functions. It also adds a parameter for cpu_restore_state which controls restoring icount. There is no need to restore icount when we only query CPU state without breaking the TB. Restoring it in such cases leads to the incorrect flow of the virtual time. In most cases new parameter is true (icount should be recalculated). But there are two cases in i386 and openrisc when the CPU state is only queried without the need to break the TB. This patch fixes both of these cases. Backports commit afd46fcad2dceffda35c0586f5723c127b6e09d8 from qemu --- qemu/accel/tcg/cpu-exec-common.c | 10 +++++----- qemu/accel/tcg/cpu-exec.c | 1 - qemu/accel/tcg/translate-all.c | 30 +++++++++++++++--------------- qemu/include/exec/exec-all.h | 5 ++++- qemu/target/arm/op_helper.c | 6 +++--- qemu/target/i386/helper.c | 2 +- qemu/target/i386/svm_helper.c | 2 +- qemu/target/m68k/op_helper.c | 4 ++-- 8 files changed, 31 insertions(+), 29 deletions(-) diff --git a/qemu/accel/tcg/cpu-exec-common.c b/qemu/accel/tcg/cpu-exec-common.c index 852661a5..2143b37a 100644 --- a/qemu/accel/tcg/cpu-exec-common.c +++ b/qemu/accel/tcg/cpu-exec-common.c @@ -26,23 +26,23 @@ /* exit the current TB, but without causing any exception to be raised */ void cpu_loop_exit_noexc(CPUState *cpu) { - /* XXX: restore cpu registers saved in host registers */ - cpu->exception_index = -1; - siglongjmp(cpu->jmp_env, 1); + cpu_loop_exit(cpu); } void cpu_loop_exit(CPUState *cpu) { + /* Undo the setting in cpu_tb_exec. */ + cpu->can_do_io = 1; siglongjmp(cpu->jmp_env, 1); } void cpu_loop_exit_restore(CPUState *cpu, uintptr_t pc) { if (pc) { - cpu_restore_state(cpu, pc); + cpu_restore_state(cpu, pc, true); } - siglongjmp(cpu->jmp_env, 1); + cpu_loop_exit(cpu); } void cpu_loop_exit_atomic(CPUState *cpu, uintptr_t pc) diff --git a/qemu/accel/tcg/cpu-exec.c b/qemu/accel/tcg/cpu-exec.c index 93f5b55a..36855aa5 100644 --- a/qemu/accel/tcg/cpu-exec.c +++ b/qemu/accel/tcg/cpu-exec.c @@ -549,7 +549,6 @@ int cpu_exec(struct uc_struct *uc, CPUState *cpu) g_assert(cpu == uc->current_cpu); g_assert(cc == CPU_GET_CLASS(uc, cpu)); #endif /* buggy compiler */ - cpu->can_do_io = 1; // Unicorn: commented out //tb_lock_reset(); } diff --git a/qemu/accel/tcg/translate-all.c b/qemu/accel/tcg/translate-all.c index 3ee894f9..556e4c50 100644 --- a/qemu/accel/tcg/translate-all.c +++ b/qemu/accel/tcg/translate-all.c @@ -280,9 +280,11 @@ static int encode_search(TCGContext *tcg_ctx, TranslationBlock *tb, uint8_t *blo /* The cpu state corresponding to 'searched_pc' is restored. * Called with tb_lock held. + * When reset_icount is true, current TB will be interrupted and + * icount should be recalculated. */ static int cpu_restore_state_from_tb(CPUState *cpu, TranslationBlock *tb, - uintptr_t searched_pc) + uintptr_t searched_pc, bool reset_icount) { target_ulong data[TARGET_INSN_START_WORDS] = { tb->pc }; uintptr_t host_pc = (uintptr_t)tb->tc.ptr; @@ -314,15 +316,12 @@ static int cpu_restore_state_from_tb(CPUState *cpu, TranslationBlock *tb, found: // UNICORN: Commented out - //if (tb->cflags & CF_USE_ICOUNT) { + //if (reset_icount && (tb->cflags & CF_USE_ICOUNT)) { // assert(use_icount); - // /* Reset the cycle counter to the start of the block. */ - // cpu->icount_decr.u16.low += num_insns; - // /* Clear the IO flag. */ - // cpu->can_do_io = 0; + // /* Reset the cycle counter to the start of the block + // and shift if to the number of actually executed instructions */ + // cpu->icount_decr.u16.low += num_insns - i; //} - - cpu->icount_decr.u16.low -= i; restore_state_to_opc(env, tb, data); #ifdef CONFIG_PROFILER @@ -332,7 +331,7 @@ found: return 0; } -bool cpu_restore_state(CPUState *cpu, uintptr_t host_pc) +bool cpu_restore_state(CPUState *cpu, uintptr_t host_pc, bool will_exit) { TranslationBlock *tb; CPUArchState *env = cpu->env_ptr; @@ -359,7 +358,7 @@ bool cpu_restore_state(CPUState *cpu, uintptr_t host_pc) if (check_offset < tcg_ctx->code_gen_buffer_size) { tb = tb_find_pc(env->uc, host_pc); if (tb) { - cpu_restore_state_from_tb(cpu, tb, host_pc); + cpu_restore_state_from_tb(cpu, tb, host_pc, will_exit); if (tb->cflags & CF_NOCACHE) { /* one-shot translation, invalidate it immediately */ tb_phys_invalidate(cpu->uc, tb, -1); @@ -1601,7 +1600,8 @@ void tb_invalidate_phys_page_range(struct uc_struct *uc, tb_page_addr_t start, t current_tb_modified = 1; // self-modifying code will restore state from TB - cpu_restore_state_from_tb(uc->cpu, current_tb, uc->cpu->mem_io_pc); + cpu_restore_state_from_tb(uc->cpu, current_tb, + uc->cpu->mem_io_pc, true); cpu_get_tb_cpu_state(env, ¤t_pc, ¤t_cs_base, ¤t_flags); } @@ -1719,7 +1719,7 @@ static bool tb_invalidate_phys_page(tb_page_addr_t addr, uintptr_t pc) restore the CPU state */ current_tb_modified = 1; - cpu_restore_state_from_tb(cpu, current_tb, pc); + cpu_restore_state_from_tb(cpu, current_tb, pc, true); cpu_get_tb_cpu_state(env, ¤t_pc, ¤t_cs_base, ¤t_flags); } @@ -1801,7 +1801,7 @@ void tb_check_watchpoint(CPUState *cpu) tb = tb_find_pc(env->uc, cpu->mem_io_pc); if (tb) { /* We can use retranslation to find the PC. */ - cpu_restore_state_from_tb(cpu, tb, cpu->mem_io_pc); + cpu_restore_state_from_tb(cpu, tb, cpu->mem_io_pc, true); tb_phys_invalidate(cpu->uc, tb, -1); } else { /* The exception probably happened in a helper. The CPU state should @@ -1816,7 +1816,7 @@ void tb_check_watchpoint(CPUState *cpu) tb_invalidate_phys_range(cpu->uc, addr, addr + 1); } - cpu_restore_state_from_tb(cpu, tb, cpu->mem_io_pc); + cpu_restore_state_from_tb(cpu, tb, cpu->mem_io_pc, true); tb_phys_invalidate(cpu->uc, tb, -1); } @@ -1837,7 +1837,7 @@ void cpu_io_recompile(CPUState *cpu, uintptr_t retaddr) (void *)retaddr); } n = cpu->icount_decr.u16.low + tb->icount; - cpu_restore_state_from_tb(cpu, tb, retaddr); + cpu_restore_state_from_tb(cpu, tb, retaddr, true); /* Calculate how many instructions had been executed before the fault occurred. */ n = n - cpu->icount_decr.u16.low; diff --git a/qemu/include/exec/exec-all.h b/qemu/include/exec/exec-all.h index 7670a877..086ffd87 100644 --- a/qemu/include/exec/exec-all.h +++ b/qemu/include/exec/exec-all.h @@ -47,13 +47,16 @@ void restore_state_to_opc(CPUArchState *env, struct TranslationBlock *tb, * cpu_restore_state: * @cpu: the vCPU state is to be restore to * @searched_pc: the host PC the fault occurred at + * @will_exit: true if the TB executed will be interrupted after some + * cpu adjustments. Required for maintaining the correct + * icount valus * @return: true if state was restored, false otherwise * * Attempt to restore the state for a fault occurring in translated * code. If the searched_pc is not in translated code no state is * restored and the function returns false. */ -bool cpu_restore_state(CPUState *cpu, uintptr_t searched_pc); +bool cpu_restore_state(CPUState *cpu, uintptr_t searched_pc, bool will_exit); void QEMU_NORETURN cpu_loop_exit_noexc(CPUState *cpu); diff --git a/qemu/target/arm/op_helper.c b/qemu/target/arm/op_helper.c index 170e1b64..9a556aa1 100644 --- a/qemu/target/arm/op_helper.c +++ b/qemu/target/arm/op_helper.c @@ -179,7 +179,7 @@ void tlb_fill(CPUState *cs, target_ulong addr, int size, ARMCPU *cpu = ARM_CPU(cs->uc, cs); /* now we have a real cpu fault */ - cpu_restore_state(cs, retaddr); + cpu_restore_state(cs, retaddr, true); deliver_fault(cpu, addr, access_type, mmu_idx, &fi); } @@ -194,7 +194,7 @@ void arm_cpu_do_unaligned_access(CPUState *cs, vaddr vaddr, ARMMMUFaultInfo fi = {0}; /* now we have a real cpu fault */ - cpu_restore_state(cs, retaddr); + cpu_restore_state(cs, retaddr, true); fi.type = ARMFault_Alignment; deliver_fault(cpu, vaddr, access_type, mmu_idx, &fi); @@ -214,7 +214,7 @@ void arm_cpu_do_transaction_failed(CPUState *cs, hwaddr physaddr, ARMMMUFaultInfo fi = {}; /* now we have a real cpu fault */ - cpu_restore_state(cs, retaddr); + cpu_restore_state(cs, retaddr, true); fi.ea = arm_extabort_type(response); fi.type = ARMFault_SyncExternal; diff --git a/qemu/target/i386/helper.c b/qemu/target/i386/helper.c index 5f7af94c..75b4c149 100644 --- a/qemu/target/i386/helper.c +++ b/qemu/target/i386/helper.c @@ -1014,7 +1014,7 @@ void cpu_report_tpr_access(CPUX86State *env, TPRAccess access) CPUState *cs = CPU(cpu); if (tcg_enabled(env->uc)) { - cpu_restore_state(cs, cs->mem_io_pc); + cpu_restore_state(cs, cs->mem_io_pc, false); apic_handle_tpr_access_report(cpu->apic_state, env->eip, access); } diff --git a/qemu/target/i386/svm_helper.c b/qemu/target/i386/svm_helper.c index f6cd2bbe..e18ba9e3 100644 --- a/qemu/target/i386/svm_helper.c +++ b/qemu/target/i386/svm_helper.c @@ -573,7 +573,7 @@ void cpu_vmexit(CPUX86State *env, uint32_t exit_code, uint64_t exit_info_1, { CPUState *cs = CPU(x86_env_get_cpu(env)); - cpu_restore_state(cs, retaddr); + cpu_restore_state(cs, retaddr, true); qemu_log_mask(CPU_LOG_TB_IN_ASM, "vmexit(%08x, %016" PRIx64 ", %016" PRIx64 ", " TARGET_FMT_lx ")!\n", diff --git a/qemu/target/m68k/op_helper.c b/qemu/target/m68k/op_helper.c index a38659d4..43be77a2 100644 --- a/qemu/target/m68k/op_helper.c +++ b/qemu/target/m68k/op_helper.c @@ -1056,7 +1056,7 @@ void HELPER(chk)(CPUM68KState *env, int32_t val, int32_t ub) CPUState *cs = CPU(m68k_env_get_cpu(env)); /* Recover PC and CC_OP for the beginning of the insn. */ - cpu_restore_state(cs, GETPC()); + cpu_restore_state(cs, GETPC(), true); /* flags have been modified by gen_flush_flags() */ env->cc_op = CC_OP_FLAGS; @@ -1087,7 +1087,7 @@ void HELPER(chk2)(CPUM68KState *env, int32_t val, int32_t lb, int32_t ub) CPUState *cs = CPU(m68k_env_get_cpu(env)); /* Recover PC and CC_OP for the beginning of the insn. */ - cpu_restore_state(cs, GETPC()); + cpu_restore_state(cs, GETPC(), true); /* flags have been modified by gen_flush_flags() */ env->cc_op = CC_OP_FLAGS;