Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle pathological cases more consistently #1651

Closed
wants to merge 6 commits into from
Closed

Handle pathological cases more consistently #1651

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7890383
Do not run tbs that request an exit
rhelmot Jul 11, 2022
a2a5db9
Do not only lift blocks with only 1 insn just because there is no phy…
rhelmot Jul 11, 2022
4a8af17
Do not call write callback twice for same address in same write
rhelmot Jul 13, 2022
33c488b
Add test case for unaligned write callbacks
rhelmot Jul 14, 2022
d9d91df
Add testcase for blocks being lifted on jump into unmapped mem
rhelmot Aug 2, 2022
b459b42
Make unaligned reads not trigger recursive callbacks
rhelmot Aug 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions qemu/accel/tcg/cpu-exec.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -595,6 +595,9 @@ int cpu_exec(struct uc_struct *uc, CPUState *cpu)
}

tb = tb_find(cpu, last_tb, tb_exit, cflags);
if (unlikely(cpu->exit_request)) {
continue;
}
cpu_loop_exec_tb(cpu, tb, &last_tb, &tb_exit);
/* Try to align the host and virtual clocks
if the guest is in advance */
Expand Down
12 changes: 8 additions & 4 deletions qemu/accel/tcg/cputlb.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1451,7 +1451,7 @@ load_helper(CPUArchState *env, target_ulong addr, TCGMemOpIdx oi,
continue;
if (!HOOK_BOUND_CHECK(hook, addr))
continue;
if ((handled = ((uc_cb_eventmem_t)hook->callback)(uc, UC_MEM_FETCH_UNMAPPED, addr, size - uc->size_recur_mem, 0, hook->user_data)))
if ((handled = ((uc_cb_eventmem_t)hook->callback)(uc, UC_MEM_FETCH_UNMAPPED, addr, size, 0, hook->user_data)))
break;

// the last callback may already asked to stop emulation
Expand All @@ -1466,7 +1466,7 @@ load_helper(CPUArchState *env, target_ulong addr, TCGMemOpIdx oi,
continue;
if (!HOOK_BOUND_CHECK(hook, addr))
continue;
if ((handled = ((uc_cb_eventmem_t)hook->callback)(uc, UC_MEM_READ_UNMAPPED, addr, size - uc->size_recur_mem, 0, hook->user_data)))
if ((handled = ((uc_cb_eventmem_t)hook->callback)(uc, UC_MEM_READ_UNMAPPED, addr, size, 0, hook->user_data)))
break;

// the last callback may already asked to stop emulation
Expand Down Expand Up @@ -1518,7 +1518,7 @@ load_helper(CPUArchState *env, target_ulong addr, TCGMemOpIdx oi,
continue;
if (!HOOK_BOUND_CHECK(hook, addr))
continue;
if ((handled = ((uc_cb_eventmem_t)hook->callback)(uc, UC_MEM_READ_PROT, addr, size - uc->size_recur_mem, 0, hook->user_data)))
if ((handled = ((uc_cb_eventmem_t)hook->callback)(uc, UC_MEM_READ_PROT, addr, size, 0, hook->user_data)))
break;

// the last callback may already asked to stop emulation
Expand Down Expand Up @@ -1546,7 +1546,7 @@ load_helper(CPUArchState *env, target_ulong addr, TCGMemOpIdx oi,
continue;
if (!HOOK_BOUND_CHECK(hook, addr))
continue;
if ((handled = ((uc_cb_eventmem_t)hook->callback)(uc, UC_MEM_FETCH_PROT, addr, size - uc->size_recur_mem, 0, hook->user_data)))
if ((handled = ((uc_cb_eventmem_t)hook->callback)(uc, UC_MEM_FETCH_PROT, addr, size, 0, hook->user_data)))
break;

// the last callback may already asked to stop emulation
Expand Down Expand Up @@ -2136,6 +2136,7 @@ store_helper(CPUArchState *env, target_ulong addr, uint64_t val,
CPUTLBEntry *entry2;
target_ulong page2, tlb_addr2;
size_t size2;
int old_size;

do_unaligned_access:
/*
Expand Down Expand Up @@ -2178,6 +2179,8 @@ store_helper(CPUArchState *env, target_ulong addr, uint64_t val,
* This loop must go in the forward direction to avoid issues
* with self-modifying code in Windows 64-bit.
*/
old_size = uc->size_recur_mem;
uc->size_recur_mem = size;
for (i = 0; i < size; ++i) {
uint8_t val8;
if (memop_big_endian(op)) {
Expand All @@ -2189,6 +2192,7 @@ store_helper(CPUArchState *env, target_ulong addr, uint64_t val,
}
helper_ret_stb_mmu(env, addr + i, val8, oi, retaddr);
}
uc->size_recur_mem = old_size;
return;
}

Expand Down
5 changes: 2 additions & 3 deletions qemu/accel/tcg/translate-all.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1584,9 +1584,8 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
phys_pc = get_page_addr_code(env, pc);

if (phys_pc == -1) {
/* Generate a temporary TB with 1 insn in it */
cflags &= ~CF_COUNT_MASK;
cflags |= CF_NOCACHE | 1;
/* Generate a temporary TB; do not cache */
cflags |= CF_NOCACHE;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried your patch but I failed to understand the change here. Any reproduction code?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you asking for a testcase?

The meaning of the original code is to set the count bitfield of the cflags to 1 (only lift 1 instruction) and set the CF_NOCACHE bit (do not cache the tb). The new code performs only the latter change.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you asking for a testcase?

The meaning of the original code is to set the count bitfield of the cflags to 1 (only lift 1 instruction) and set the CF_NOCACHE bit (do not cache the tb). The new code performs only the latter change.

It's better if a testcase could be provided so that I can understand it better. My question is that, why setting the count to 1 solves the problem?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're misunderstanding the order of the diff - the old code set the count to 1, and this was undesired behavior because then it would only lift one instruction. Presumably the old code optimizes guests using exceptional control flow, but it produces inaccurate metrics (block counts).

}

cflags &= ~CF_CLUSTER_MASK;
Expand Down
55 changes: 55 additions & 0 deletions tests/unit/test_x86.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1106,6 +1106,60 @@ static void test_x86_correct_address_in_long_jump_hook(void)
OK(uc_close(uc));
}

struct writelog_t {
uint32_t addr, size;
};

static void test_x86_unaligned_write_callback(uc_engine *uc, uc_mem_type type,
uint64_t address, int size, int64_t value, void *user_data)
{
TEST_CHECK(size != 0);
struct writelog_t *write_log = (struct writelog_t *)user_data;

for (int i = 0; i < 10; i++) {
if (write_log[i].size == 0) {
write_log[i].addr = (uint32_t) address;
write_log[i].size = (uint32_t) size;
return;
}
}
TEST_ASSERT(false);
}

static void test_x86_unaligned_write(void)
{
uc_engine *uc;
uc_hook hook;
char code[] = "\xa3\x01\x00\x20\x00"; // mov dword ptr [0x200001], eax
uint32_t r_eax = 0x41424344;
struct writelog_t write_log[10];
memset(write_log, 0, sizeof(write_log));

uc_common_setup(&uc, UC_ARCH_X86, UC_MODE_32, code, sizeof(code) - 1);
OK(uc_mem_map(uc, 0x200000, 0x1000, UC_PROT_ALL));
OK(uc_hook_add(uc, &hook, UC_HOOK_MEM_WRITE, test_x86_unaligned_write_callback,
write_log, 1, 0));

OK(uc_reg_write(uc, UC_X86_REG_EAX, &r_eax));
OK(uc_emu_start(uc, code_start, code_start + sizeof(code) - 1, 0, 0));

TEST_CHECK(write_log[0].addr == 0x200001);
TEST_CHECK(write_log[0].size == 4);
TEST_CHECK(write_log[1].size == 0);

char b;
OK(uc_mem_read(uc, 0x200001, &b, 1));
TEST_CHECK(b == 0x44);
OK(uc_mem_read(uc, 0x200002, &b, 1));
TEST_CHECK(b == 0x43);
OK(uc_mem_read(uc, 0x200003, &b, 1));
TEST_CHECK(b == 0x42);
OK(uc_mem_read(uc, 0x200004, &b, 1));
TEST_CHECK(b == 0x41);

OK(uc_close(uc));
}

TEST_LIST = {
{"test_x86_in", test_x86_in},
{"test_x86_out", test_x86_out},
Expand Down Expand Up @@ -1143,4 +1197,5 @@ TEST_LIST = {
test_x86_correct_address_in_small_jump_hook},
{"test_x86_correct_address_in_long_jump_hook",
test_x86_correct_address_in_long_jump_hook},
{"test_x86_unaligned_write", test_x86_unaligned_write},
{NULL, NULL}};