From 7e9a7c50d9a400ef51242d661a261123c2cc9485 Mon Sep 17 00:00:00 2001 From: Richard Henderson Date: Fri, 8 Jul 2016 12:19:32 -0700 Subject: [PATCH 1/4] cputlb: Move VICTIM_TLB_HIT out of line There are currently 22 invocations of this function, and we're about to increase that number. Signed-off-by: Richard Henderson --- cputlb.c | 29 +++++++++++++++++++++++++++++ softmmu_template.h | 25 ------------------------- 2 files changed, 29 insertions(+), 25 deletions(-) diff --git a/cputlb.c b/cputlb.c index 079e4979ca..7518544e12 100644 --- a/cputlb.c +++ b/cputlb.c @@ -498,6 +498,35 @@ tb_page_addr_t get_page_addr_code(CPUArchState *env1, target_ulong addr) return qemu_ram_addr_from_host_nofail(p); } +/* Return true if ADDR is present in the victim tlb, and has been copied + back to the main tlb. */ +static bool victim_tlb_hit(CPUArchState *env, size_t mmu_idx, size_t index, + size_t elt_ofs, target_ulong page) +{ + size_t vidx; + for (vidx = 0; vidx < CPU_VTLB_SIZE; ++vidx) { + CPUTLBEntry *vtlb = &env->tlb_v_table[mmu_idx][vidx]; + target_ulong cmp = *(target_ulong *)((uintptr_t)vtlb + elt_ofs); + + if (cmp == page) { + /* Found entry in victim tlb, swap tlb and iotlb. */ + CPUTLBEntry tmptlb, *tlb = &env->tlb_table[mmu_idx][index]; + CPUIOTLBEntry tmpio, *io = &env->iotlb[mmu_idx][index]; + CPUIOTLBEntry *vio = &env->iotlb_v[mmu_idx][vidx]; + + tmptlb = *tlb; *tlb = *vtlb; *vtlb = tmptlb; + tmpio = *io; *io = *vio; *vio = tmpio; + return true; + } + } + return false; +} + +/* Macro to call the above, with local variables from the use context. */ +#define VICTIM_TLB_HIT(TY) \ + victim_tlb_hit(env, mmu_idx, index, offsetof(CPUTLBEntry, TY), \ + addr & TARGET_PAGE_MASK) + #define MMUSUFFIX _mmu #define SHIFT 0 diff --git a/softmmu_template.h b/softmmu_template.h index 4d378ca630..405ba3590e 100644 --- a/softmmu_template.h +++ b/softmmu_template.h @@ -116,31 +116,6 @@ # define helper_te_st_name helper_le_st_name #endif -/* macro to check the victim tlb */ -#define VICTIM_TLB_HIT(ty) \ -({ \ - /* we are about to do a page table walk. our last hope is the \ - * victim tlb. try to refill from the victim tlb before walking the \ - * page table. */ \ - int vidx; \ - CPUIOTLBEntry tmpiotlb; \ - CPUTLBEntry tmptlb; \ - for (vidx = CPU_VTLB_SIZE-1; vidx >= 0; --vidx) { \ - if (env->tlb_v_table[mmu_idx][vidx].ty == (addr & TARGET_PAGE_MASK)) {\ - /* found entry in victim tlb, swap tlb and iotlb */ \ - tmptlb = env->tlb_table[mmu_idx][index]; \ - env->tlb_table[mmu_idx][index] = env->tlb_v_table[mmu_idx][vidx]; \ - env->tlb_v_table[mmu_idx][vidx] = tmptlb; \ - tmpiotlb = env->iotlb[mmu_idx][index]; \ - env->iotlb[mmu_idx][index] = env->iotlb_v[mmu_idx][vidx]; \ - env->iotlb_v[mmu_idx][vidx] = tmpiotlb; \ - break; \ - } \ - } \ - /* return true when there is a vtlb hit, i.e. vidx >=0 */ \ - vidx >= 0; \ -}) - #ifndef SOFTMMU_CODE_ACCESS static inline DATA_TYPE glue(io_read, SUFFIX)(CPUArchState *env, CPUIOTLBEntry *iotlbentry, From a390284b80d2b6581143cdb40666674e60e635ae Mon Sep 17 00:00:00 2001 From: Samuel Damashek Date: Wed, 6 Jul 2016 14:26:52 -0400 Subject: [PATCH 2/4] cputlb: Add address parameter to VICTIM_TLB_HIT [rth: Split out from the original patch.] Signed-off-by: Samuel Damashek Message-Id: <20160706182652.16190-1-samuel.damashek@invincea.com> Signed-off-by: Richard Henderson --- cputlb.c | 4 ++-- softmmu_template.h | 10 +++++----- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/cputlb.c b/cputlb.c index 7518544e12..d068ee597e 100644 --- a/cputlb.c +++ b/cputlb.c @@ -523,9 +523,9 @@ static bool victim_tlb_hit(CPUArchState *env, size_t mmu_idx, size_t index, } /* Macro to call the above, with local variables from the use context. */ -#define VICTIM_TLB_HIT(TY) \ +#define VICTIM_TLB_HIT(TY, ADDR) \ victim_tlb_hit(env, mmu_idx, index, offsetof(CPUTLBEntry, TY), \ - addr & TARGET_PAGE_MASK) + (ADDR) & TARGET_PAGE_MASK) #define MMUSUFFIX _mmu diff --git a/softmmu_template.h b/softmmu_template.h index 405ba3590e..aeab0169f5 100644 --- a/softmmu_template.h +++ b/softmmu_template.h @@ -161,7 +161,7 @@ WORD_TYPE helper_le_ld_name(CPUArchState *env, target_ulong addr, /* If the TLB entry is for a different page, reload and try again. */ if ((addr & TARGET_PAGE_MASK) != (tlb_addr & (TARGET_PAGE_MASK | TLB_INVALID_MASK))) { - if (!VICTIM_TLB_HIT(ADDR_READ)) { + if (!VICTIM_TLB_HIT(ADDR_READ, addr)) { tlb_fill(ENV_GET_CPU(env), addr, READ_ACCESS_TYPE, mmu_idx, retaddr); } @@ -235,7 +235,7 @@ WORD_TYPE helper_be_ld_name(CPUArchState *env, target_ulong addr, /* If the TLB entry is for a different page, reload and try again. */ if ((addr & TARGET_PAGE_MASK) != (tlb_addr & (TARGET_PAGE_MASK | TLB_INVALID_MASK))) { - if (!VICTIM_TLB_HIT(ADDR_READ)) { + if (!VICTIM_TLB_HIT(ADDR_READ, addr)) { tlb_fill(ENV_GET_CPU(env), addr, READ_ACCESS_TYPE, mmu_idx, retaddr); } @@ -345,7 +345,7 @@ void helper_le_st_name(CPUArchState *env, target_ulong addr, DATA_TYPE val, /* If the TLB entry is for a different page, reload and try again. */ if ((addr & TARGET_PAGE_MASK) != (tlb_addr & (TARGET_PAGE_MASK | TLB_INVALID_MASK))) { - if (!VICTIM_TLB_HIT(addr_write)) { + if (!VICTIM_TLB_HIT(addr_write, addr)) { tlb_fill(ENV_GET_CPU(env), addr, MMU_DATA_STORE, mmu_idx, retaddr); } tlb_addr = env->tlb_table[mmu_idx][index].addr_write; @@ -415,7 +415,7 @@ void helper_be_st_name(CPUArchState *env, target_ulong addr, DATA_TYPE val, /* If the TLB entry is for a different page, reload and try again. */ if ((addr & TARGET_PAGE_MASK) != (tlb_addr & (TARGET_PAGE_MASK | TLB_INVALID_MASK))) { - if (!VICTIM_TLB_HIT(addr_write)) { + if (!VICTIM_TLB_HIT(addr_write, addr)) { tlb_fill(ENV_GET_CPU(env), addr, MMU_DATA_STORE, mmu_idx, retaddr); } tlb_addr = env->tlb_table[mmu_idx][index].addr_write; @@ -477,7 +477,7 @@ void probe_write(CPUArchState *env, target_ulong addr, int mmu_idx, if ((addr & TARGET_PAGE_MASK) != (tlb_addr & (TARGET_PAGE_MASK | TLB_INVALID_MASK))) { /* TLB entry is for a different page */ - if (!VICTIM_TLB_HIT(addr_write)) { + if (!VICTIM_TLB_HIT(addr_write, addr)) { tlb_fill(ENV_GET_CPU(env), addr, MMU_DATA_STORE, mmu_idx, retaddr); } } From 81daabaf7a572f138a8b88ba6eea556bdb0cce46 Mon Sep 17 00:00:00 2001 From: Samuel Damashek Date: Fri, 8 Jul 2016 12:54:34 -0700 Subject: [PATCH 3/4] cputlb: Fix for self-modifying writes across page boundaries As it currently stands, QEMU does not properly handle self-modifying code when the write is unaligned and crosses a page boundary. The procedure for handling a write to the current translation block is to write-protect the current translation block, catch the write, split up the translation block into the current instruction (which remains write-protected so that the current instruction is not modified) and the remaining instructions in the translation block, and then restore the CPU state to before the write occurred so the write will be retried and successfully executed. However, since unaligned writes across pages are split into one-byte writes for simplicity, writes to the second page (which is not the current TB) may succeed before a write to the current TB is attempted, and since these writes are not invalidated before resuming state after splitting the TB, these writes will be performed a second time, thus corrupting the second page. Credit goes to Patrick Hulin for discovering this. In recent 64-bit versions of Windows running in emulated mode, this results in either being very unstable (a BSOD after a couple minutes of uptime), or being entirely unable to boot. Windows performs one or more 8-byte unaligned self-modifying writes (xors) which intersect the end of the current TB and the beginning of the next TB, which runs into the aforementioned issue. This commit fixes that issue by making the unaligned write loop perform the writes in forwards order, instead of reverse order. This way, QEMU immediately tries to write to the current TB, and splits the TB before any write to the second page is executed. The write then proceeds as intended. With this patch applied, I am able to boot and use Windows 7 64-bit and Windows 10 64-bit in QEMU without KVM. Per Richard Henderson's input, this patch also ensures the second page is in the TLB before executing the write loop, to ensure the second page is mapped. The original discussion of the issue is located at http://lists.nongnu.org/archive/html/qemu-devel/2014-08/msg02161.html. Signed-off-by: Samuel Damashek Message-Id: <20160706182652.16190-1-samuel.damashek@invincea.com> Signed-off-by: Richard Henderson --- softmmu_template.h | 44 +++++++++++++++++++++++++++++++++++--------- 1 file changed, 35 insertions(+), 9 deletions(-) diff --git a/softmmu_template.h b/softmmu_template.h index aeab0169f5..284ab2c7b2 100644 --- a/softmmu_template.h +++ b/softmmu_template.h @@ -370,12 +370,25 @@ void helper_le_st_name(CPUArchState *env, target_ulong addr, DATA_TYPE val, if (DATA_SIZE > 1 && unlikely((addr & ~TARGET_PAGE_MASK) + DATA_SIZE - 1 >= TARGET_PAGE_SIZE)) { - int i; + int i, index2; + target_ulong page2, tlb_addr2; do_unaligned_access: - /* XXX: not efficient, but simple */ - /* Note: relies on the fact that tlb_fill() does not remove the - * previous page from the TLB cache. */ - for (i = DATA_SIZE - 1; i >= 0; i--) { + /* Ensure the second page is in the TLB. Note that the first page + is already guaranteed to be filled, and that the second page + cannot evict the first. */ + page2 = (addr + DATA_SIZE) & TARGET_PAGE_MASK; + index2 = (page2 >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1); + tlb_addr2 = env->tlb_table[mmu_idx][index2].addr_write; + if (page2 != (tlb_addr2 & (TARGET_PAGE_MASK | TLB_INVALID_MASK)) + && !VICTIM_TLB_HIT(addr_write, page2)) { + tlb_fill(ENV_GET_CPU(env), page2, MMU_DATA_STORE, + mmu_idx, retaddr); + } + + /* XXX: not efficient, but simple. */ + /* This loop must go in the forward direction to avoid issues + with self-modifying code in Windows 64-bit. */ + for (i = 0; i < DATA_SIZE; ++i) { /* Little-endian extract. */ uint8_t val8 = val >> (i * 8); /* Note the adjustment at the beginning of the function. @@ -440,12 +453,25 @@ void helper_be_st_name(CPUArchState *env, target_ulong addr, DATA_TYPE val, if (DATA_SIZE > 1 && unlikely((addr & ~TARGET_PAGE_MASK) + DATA_SIZE - 1 >= TARGET_PAGE_SIZE)) { - int i; + int i, index2; + target_ulong page2, tlb_addr2; do_unaligned_access: + /* Ensure the second page is in the TLB. Note that the first page + is already guaranteed to be filled, and that the second page + cannot evict the first. */ + page2 = (addr + DATA_SIZE) & TARGET_PAGE_MASK; + index2 = (page2 >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1); + tlb_addr2 = env->tlb_table[mmu_idx][index2].addr_write; + if (page2 != (tlb_addr2 & (TARGET_PAGE_MASK | TLB_INVALID_MASK)) + && !VICTIM_TLB_HIT(addr_write, page2)) { + tlb_fill(ENV_GET_CPU(env), page2, MMU_DATA_STORE, + mmu_idx, retaddr); + } + /* XXX: not efficient, but simple */ - /* Note: relies on the fact that tlb_fill() does not remove the - * previous page from the TLB cache. */ - for (i = DATA_SIZE - 1; i >= 0; i--) { + /* This loop must go in the forward direction to avoid issues + with self-modifying code. */ + for (i = 0; i < DATA_SIZE; ++i) { /* Big-endian extract. */ uint8_t val8 = val >> (((DATA_SIZE - 1) * 8) - (i * 8)); /* Note the adjustment at the beginning of the function. From 7399a337e4126f7c8c8af3336726f001378c4798 Mon Sep 17 00:00:00 2001 From: Stanislav Shmarov Date: Thu, 7 Jul 2016 11:33:12 +0300 Subject: [PATCH 4/4] translate-all: Fix user-mode self-modifying code in 2 page long TB In user-mode emulation Translation Block can consist of 2 guest pages. In that case QEMU also mprotects 2 host pages that are dedicated for guest memory, containing instructions. QEMU detects self-modifying code with SEGFAULT signal processing. In case if instruction in 1st page is modifying memory of 2nd page (or vice versa) QEMU will mark 2nd page with PAGE_WRITE, invalidate TB, generate new TB contatining 1 guest instruction and exit to CPU loop. QEMU won't call mprotect, and new TB will cause same SEGFAULT. Page will have both PAGE_WRITE_ORG and PAGE_WRITE flags, so QEMU will handle the signal as guest binary problem, and exit with guest SEGFAULT. Solution is to do following: In case if current TB was invalidated continue to invalidate TBs from remaining guest pages and mark pages as PAGE_WRITE. After that disable host page protection with mprotect. If current tb was invalidated longjmp to main loop. That is more efficient, since we won't get SEGFAULT when executing new TB. Reviewed-by: Sergey Fedorov Signed-off-by: Stanislav Shmarov Message-Id: <1467880392-1043630-1-git-send-email-snarpix@gmail.com> Signed-off-by: Richard Henderson --- translate-all.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/translate-all.c b/translate-all.c index eaa95e4cd7..0d47c1c0cf 100644 --- a/translate-all.c +++ b/translate-all.c @@ -2000,6 +2000,7 @@ int page_check_range(target_ulong start, target_ulong len, int flags) int page_unprotect(target_ulong address, uintptr_t pc) { unsigned int prot; + bool current_tb_invalidated; PageDesc *p; target_ulong host_start, host_end, addr; @@ -2021,6 +2022,7 @@ int page_unprotect(target_ulong address, uintptr_t pc) host_end = host_start + qemu_host_page_size; prot = 0; + current_tb_invalidated = false; for (addr = host_start ; addr < host_end ; addr += TARGET_PAGE_SIZE) { p = page_find(addr >> TARGET_PAGE_BITS); p->flags |= PAGE_WRITE; @@ -2028,10 +2030,7 @@ int page_unprotect(target_ulong address, uintptr_t pc) /* and since the content will be modified, we must invalidate the corresponding translated code. */ - if (tb_invalidate_phys_page(addr, pc)) { - mmap_unlock(); - return 2; - } + current_tb_invalidated |= tb_invalidate_phys_page(addr, pc); #ifdef DEBUG_TB_CHECK tb_invalidate_check(addr); #endif @@ -2040,7 +2039,8 @@ int page_unprotect(target_ulong address, uintptr_t pc) prot & PAGE_BITS); mmap_unlock(); - return 1; + /* If current TB was invalidated return to main loop */ + return current_tb_invalidated ? 2 : 1; } mmap_unlock(); return 0;