From 8037fa55ace19e1f328d03e2cefa78d5f3d81310 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alex=20Benn=C3=A9e?= Date: Tue, 21 Mar 2017 16:26:27 +0000 Subject: [PATCH 01/11] scripts/qemugdb/mtree.py: fix up mtree dump MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since QEMU has been able to build with native Int128 support this was broken as it attempts to fish values out of the non-existent structure. Also the alias print was trying to make a %x out of gdb.ValueType directly which didn't seem to work. Signed-off-by: Alex Bennée --- scripts/qemugdb/mtree.py | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/scripts/qemugdb/mtree.py b/scripts/qemugdb/mtree.py index cc8131c2e7..e6791b7885 100644 --- a/scripts/qemugdb/mtree.py +++ b/scripts/qemugdb/mtree.py @@ -21,7 +21,15 @@ def isnull(ptr): return ptr == gdb.Value(0).cast(ptr.type) def int128(p): - return int(p['lo']) + (int(p['hi']) << 64) + '''Read an Int128 type to a python integer. + + QEMU can be built with native Int128 support so we need to detect + if the value is a structure or the native type. + ''' + if p.type.code == gdb.TYPE_CODE_STRUCT: + return int(p['lo']) + (int(p['hi']) << 64) + else: + return int(("%s" % p), 16) class MtreeCommand(gdb.Command): '''Display the memory tree hierarchy''' @@ -69,7 +77,7 @@ def print_item(self, ptr, offset = gdb.Value(0), level = 0): gdb.write('%s alias: %s@%016x (@ %s)\n' % (' ' * level, alias['name'].string(), - ptr['alias_offset'], + int(ptr['alias_offset']), alias, ), gdb.STDOUT) From 8695350357abc38a4f915ba6cb62e796bbbaf111 Mon Sep 17 00:00:00 2001 From: Nikunj A Dadhania Date: Mon, 10 Apr 2017 11:36:55 +0530 Subject: [PATCH 02/11] cpus: fix wrong define name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit While the configure script generates TARGET_SUPPORTS_MTTCG define, one of the define is cpus.c is checking wrong name: TARGET_SUPPORT_MTTCG Signed-off-by: Nikunj A Dadhania Signed-off-by: Alex Bennée --- cpus.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpus.c b/cpus.c index 68fdbc40b9..58d90aa2b9 100644 --- a/cpus.c +++ b/cpus.c @@ -202,7 +202,7 @@ void qemu_tcg_configure(QemuOpts *opts, Error **errp) } else if (use_icount) { error_setg(errp, "No MTTCG when icount is enabled"); } else { -#ifndef TARGET_SUPPORT_MTTCG +#ifndef TARGET_SUPPORTS_MTTCG error_report("Guest not yet converted to MTTCG - " "you may get unexpected results"); #endif From b4e79a502ff2ac06a21c45f598870de807eb5d20 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alex=20Benn=C3=A9e?= Date: Thu, 30 Mar 2017 16:04:09 +0100 Subject: [PATCH 03/11] target/i386/misc_helper: wrap BQL around another IRQ generator MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Anything that calls into HW emulation must be protected by the BQL. Signed-off-by: Alex Bennée Reviewed-by: Richard Henderson Acked-by: Eduardo Habkost --- target/i386/misc_helper.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/target/i386/misc_helper.c b/target/i386/misc_helper.c index ca2ea09f54..628f64aad5 100644 --- a/target/i386/misc_helper.c +++ b/target/i386/misc_helper.c @@ -18,6 +18,7 @@ */ #include "qemu/osdep.h" +#include "qemu/main-loop.h" #include "cpu.h" #include "exec/helper-proto.h" #include "exec/exec-all.h" @@ -156,7 +157,9 @@ void helper_write_crN(CPUX86State *env, int reg, target_ulong t0) break; case 8: if (!(env->hflags2 & HF2_VINTR_MASK)) { + qemu_mutex_lock_iothread(); cpu_set_apic_tpr(x86_env_get_cpu(env)->apic_state, t0); + qemu_mutex_unlock_iothread(); } env->v_tpr = t0 & 0x0f; break; From bf51c7206facff628df24c5499ace9c97c503962 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alex=20Benn=C3=A9e?= Date: Thu, 30 Mar 2017 18:32:29 +0100 Subject: [PATCH 04/11] cpus: remove icount handling from qemu_tcg_cpu_thread_fn MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We should never be running in multi-threaded mode with icount enabled. There is no point calling handle_icount_deadline here so remove it and assert !use_icount. Signed-off-by: Alex Bennée Reviewed-by: Richard Henderson --- cpus.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cpus.c b/cpus.c index 58d90aa2b9..fc0ddc8793 100644 --- a/cpus.c +++ b/cpus.c @@ -1392,6 +1392,8 @@ static void *qemu_tcg_cpu_thread_fn(void *arg) { CPUState *cpu = arg; + g_assert(!use_icount); + rcu_register_thread(); qemu_mutex_lock_iothread(); @@ -1434,8 +1436,6 @@ static void *qemu_tcg_cpu_thread_fn(void *arg) } } - handle_icount_deadline(); - atomic_mb_set(&cpu->exit_request, 0); qemu_tcg_wait_io_event(cpu); } From 243c5f77f6734776a45d50612b0f3ca2f2f6448e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alex=20Benn=C3=A9e?= Date: Thu, 30 Mar 2017 18:49:22 +0100 Subject: [PATCH 05/11] cpus: check cpu->running in cpu_get_icount_raw() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The lifetime of current_cpu is now the lifetime of the vCPU thread. However get_icount_raw() can apply a fudge factor if called while code is running to take into account the current executed instruction count. To ensure this is always the case we also check cpu->running. Signed-off-by: Alex Bennée Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Richard Henderson --- cpus.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpus.c b/cpus.c index fc0ddc8793..7ec6473c02 100644 --- a/cpus.c +++ b/cpus.c @@ -229,7 +229,7 @@ int64_t cpu_get_icount_raw(void) CPUState *cpu = current_cpu; icount = timers_state.qemu_icount; - if (cpu) { + if (cpu && cpu->running) { if (!cpu->can_do_io) { fprintf(stderr, "Bad icount read\n"); exit(1); From 05248382251a58a14da60a640d29f570524174d3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alex=20Benn=C3=A9e?= Date: Wed, 29 Mar 2017 16:46:59 +0100 Subject: [PATCH 06/11] cpus: move icount preparation out of tcg_exec_cpu MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit As icount is only supported for single-threaded execution due to the requirement for determinism let's remove it from the common tcg_exec_cpu path. Also remove the additional fiddling which shouldn't be required as the icount counters should all be rectified as you enter the loop. Signed-off-by: Alex Bennée --- cpus.c | 69 ++++++++++++++++++++++++++++++++++++++-------------------- 1 file changed, 46 insertions(+), 23 deletions(-) diff --git a/cpus.c b/cpus.c index 7ec6473c02..6034b104c3 100644 --- a/cpus.c +++ b/cpus.c @@ -1179,6 +1179,46 @@ static void handle_icount_deadline(void) } } +static void prepare_icount_for_run(CPUState *cpu) +{ + if (use_icount) { + int64_t count; + int decr; + + /* These should always be cleared by process_icount_data after + * each vCPU execution. However u16.high can be raised + * asynchronously by cpu_exit/cpu_interrupt/tcg_handle_interrupt + */ + g_assert(cpu->icount_decr.u16.low == 0); + g_assert(cpu->icount_extra == 0); + + + count = tcg_get_icount_limit(); + + timers_state.qemu_icount += count; + decr = (count > 0xffff) ? 0xffff : count; + count -= decr; + cpu->icount_decr.u16.low = decr; + cpu->icount_extra = count; + } +} + +static void process_icount_data(CPUState *cpu) +{ + if (use_icount) { + /* Fold pending instructions back into the + instruction counter, and clear the interrupt flag. */ + timers_state.qemu_icount -= (cpu->icount_decr.u16.low + + cpu->icount_extra); + + /* Reset the counters */ + cpu->icount_decr.u16.low = 0; + cpu->icount_extra = 0; + replay_account_executed_instructions(); + } +} + + static int tcg_cpu_exec(CPUState *cpu) { int ret; @@ -1189,20 +1229,6 @@ static int tcg_cpu_exec(CPUState *cpu) #ifdef CONFIG_PROFILER ti = profile_getclock(); #endif - if (use_icount) { - int64_t count; - int decr; - timers_state.qemu_icount -= (cpu->icount_decr.u16.low - + cpu->icount_extra); - cpu->icount_decr.u16.low = 0; - cpu->icount_extra = 0; - count = tcg_get_icount_limit(); - timers_state.qemu_icount += count; - decr = (count > 0xffff) ? 0xffff : count; - count -= decr; - cpu->icount_decr.u16.low = decr; - cpu->icount_extra = count; - } qemu_mutex_unlock_iothread(); cpu_exec_start(cpu); ret = cpu_exec(cpu); @@ -1211,15 +1237,6 @@ static int tcg_cpu_exec(CPUState *cpu) #ifdef CONFIG_PROFILER tcg_time += profile_getclock() - ti; #endif - if (use_icount) { - /* Fold pending instructions back into the - instruction counter, and clear the interrupt flag. */ - timers_state.qemu_icount -= (cpu->icount_decr.u16.low - + cpu->icount_extra); - cpu->icount_decr.u32 = 0; - cpu->icount_extra = 0; - replay_account_executed_instructions(); - } return ret; } @@ -1306,7 +1323,13 @@ static void *qemu_tcg_rr_cpu_thread_fn(void *arg) if (cpu_can_run(cpu)) { int r; + + prepare_icount_for_run(cpu); + r = tcg_cpu_exec(cpu); + + process_icount_data(cpu); + if (r == EXCP_DEBUG) { cpu_handle_guest_debug(cpu); break; From e4cd96571f00e290e93dcc65a6d2b616b159dea6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alex=20Benn=C3=A9e?= Date: Fri, 31 Mar 2017 16:09:42 +0100 Subject: [PATCH 07/11] cpus: don't credit executed instructions before they have run MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Outside of the vCPU thread icount time will only be tracked against timers_state.qemu_icount. We no longer credit cycles until they have completed the run. Inside the vCPU thread we adjust for passage of time by looking at how many have run so far. This is only valid inside the vCPU thread while it is running. Signed-off-by: Alex Bennée --- cpus.c | 25 +++++++++++++++++++------ include/qom/cpu.h | 1 + 2 files changed, 20 insertions(+), 6 deletions(-) diff --git a/cpus.c b/cpus.c index 6034b104c3..0ecb0b87f0 100644 --- a/cpus.c +++ b/cpus.c @@ -223,6 +223,15 @@ void qemu_tcg_configure(QemuOpts *opts, Error **errp) } } +/* The current number of executed instructions is based on what we + * originally budgeted minus the current state of the decrementing + * icount counters in extra/u16.low. + */ +static int64_t cpu_get_icount_executed(CPUState *cpu) +{ + return cpu->icount_budget - (cpu->icount_decr.u16.low + cpu->icount_extra); +} + int64_t cpu_get_icount_raw(void) { int64_t icount; @@ -234,7 +243,8 @@ int64_t cpu_get_icount_raw(void) fprintf(stderr, "Bad icount read\n"); exit(1); } - icount -= (cpu->icount_decr.u16.low + cpu->icount_extra); + /* Take into account what has run */ + icount += cpu_get_icount_executed(cpu); } return icount; } @@ -1195,7 +1205,10 @@ static void prepare_icount_for_run(CPUState *cpu) count = tcg_get_icount_limit(); - timers_state.qemu_icount += count; + /* To calculate what we have executed so far we need to know + * what we originally budgeted to run this cycle */ + cpu->icount_budget = count; + decr = (count > 0xffff) ? 0xffff : count; count -= decr; cpu->icount_decr.u16.low = decr; @@ -1206,14 +1219,14 @@ static void prepare_icount_for_run(CPUState *cpu) static void process_icount_data(CPUState *cpu) { if (use_icount) { - /* Fold pending instructions back into the - instruction counter, and clear the interrupt flag. */ - timers_state.qemu_icount -= (cpu->icount_decr.u16.low - + cpu->icount_extra); + /* Account for executed instructions */ + timers_state.qemu_icount += cpu_get_icount_executed(cpu); /* Reset the counters */ cpu->icount_decr.u16.low = 0; cpu->icount_extra = 0; + cpu->icount_budget = 0; + replay_account_executed_instructions(); } } diff --git a/include/qom/cpu.h b/include/qom/cpu.h index c3292efe1c..5d10359c8f 100644 --- a/include/qom/cpu.h +++ b/include/qom/cpu.h @@ -332,6 +332,7 @@ struct CPUState { /* updates protected by BQL */ uint32_t interrupt_request; int singlestep_enabled; + int64_t icount_budget; int64_t icount_extra; sigjmp_buf jmp_env; From 512d3c807177b5cfff6b5a4925d71ae1b5521093 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alex=20Benn=C3=A9e?= Date: Wed, 5 Apr 2017 12:32:37 +0100 Subject: [PATCH 08/11] cpus: introduce cpu_update_icount helper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit By holding off updates to timer_state.qemu_icount we can run into trouble when the non-vCPU thread needs to know the time. This helper ensures we atomically update timers_state.qemu_icount based on what has been currently executed. Signed-off-by: Alex Bennée --- cpus.c | 23 +++++++++++++++++++++-- include/qemu/timer.h | 1 + 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/cpus.c b/cpus.c index 0ecb0b87f0..a5125d7167 100644 --- a/cpus.c +++ b/cpus.c @@ -232,12 +232,31 @@ static int64_t cpu_get_icount_executed(CPUState *cpu) return cpu->icount_budget - (cpu->icount_decr.u16.low + cpu->icount_extra); } +/* + * Update the global shared timer_state.qemu_icount to take into + * account executed instructions. This is done by the TCG vCPU + * thread so the main-loop can see time has moved forward. + */ +void cpu_update_icount(CPUState *cpu) +{ + int64_t executed = cpu_get_icount_executed(cpu); + cpu->icount_budget -= executed; + +#ifdef CONFIG_ATOMIC64 + atomic_set__nocheck(&timers_state.qemu_icount, + atomic_read__nocheck(&timers_state.qemu_icount) + + executed); +#else /* FIXME: we need 64bit atomics to do this safely */ + timers_state.qemu_icount += executed; +#endif +} + int64_t cpu_get_icount_raw(void) { int64_t icount; CPUState *cpu = current_cpu; - icount = timers_state.qemu_icount; + icount = atomic_read(&timers_state.qemu_icount); if (cpu && cpu->running) { if (!cpu->can_do_io) { fprintf(stderr, "Bad icount read\n"); @@ -1220,7 +1239,7 @@ static void process_icount_data(CPUState *cpu) { if (use_icount) { /* Account for executed instructions */ - timers_state.qemu_icount += cpu_get_icount_executed(cpu); + cpu_update_icount(cpu); /* Reset the counters */ cpu->icount_decr.u16.low = 0; diff --git a/include/qemu/timer.h b/include/qemu/timer.h index e1742f2f3d..8a1eb74839 100644 --- a/include/qemu/timer.h +++ b/include/qemu/timer.h @@ -869,6 +869,7 @@ int64_t cpu_get_icount_raw(void); int64_t cpu_get_icount(void); int64_t cpu_get_clock(void); int64_t cpu_icount_to_ns(int64_t icount); +void cpu_update_icount(CPUState *cpu); /*******************************************/ /* host CPU ticks (if available) */ From eda5f7c6a147c8eb927a6198ec48fe677cb079b3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alex=20Benn=C3=A9e?= Date: Wed, 5 Apr 2017 12:35:48 +0100 Subject: [PATCH 09/11] cpu-exec: update icount after each TB_EXIT MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit There is no particular reason we shouldn't update the global system icount time as we exit each TranslationBlock run. This ensures the main-loop doesn't have to wait until we exit to the outer loop for executed instructions to be credited to timer_state. The prepare_icount_for_run function is slightly tweaked to match the logic we run in cpu_loop_exec_tb. Based on Paolo's original suggestion. Signed-off-by: Alex Bennée --- cpu-exec.c | 14 +++++++------- cpus.c | 18 +++++------------- 2 files changed, 12 insertions(+), 20 deletions(-) diff --git a/cpu-exec.c b/cpu-exec.c index 748cb66bca..63a56d0407 100644 --- a/cpu-exec.c +++ b/cpu-exec.c @@ -600,13 +600,13 @@ static inline void cpu_loop_exec_tb(CPUState *cpu, TranslationBlock *tb, /* Instruction counter expired. */ assert(use_icount); #ifndef CONFIG_USER_ONLY - if (cpu->icount_extra) { - /* Refill decrementer and continue execution. */ - cpu->icount_extra += insns_left; - insns_left = MIN(0xffff, cpu->icount_extra); - cpu->icount_extra -= insns_left; - cpu->icount_decr.u16.low = insns_left; - } else { + /* Ensure global icount has gone forward */ + cpu_update_icount(cpu); + /* Refill decrementer and continue execution. */ + insns_left = MIN(0xffff, cpu->icount_budget); + cpu->icount_decr.u16.low = insns_left; + cpu->icount_extra = cpu->icount_budget - insns_left; + if (!cpu->icount_extra) { /* Execute any remaining instructions, then let the main loop * handle the next event. */ diff --git a/cpus.c b/cpus.c index a5125d7167..9c8bd2c991 100644 --- a/cpus.c +++ b/cpus.c @@ -1211,8 +1211,7 @@ static void handle_icount_deadline(void) static void prepare_icount_for_run(CPUState *cpu) { if (use_icount) { - int64_t count; - int decr; + int insns_left; /* These should always be cleared by process_icount_data after * each vCPU execution. However u16.high can be raised @@ -1221,17 +1220,10 @@ static void prepare_icount_for_run(CPUState *cpu) g_assert(cpu->icount_decr.u16.low == 0); g_assert(cpu->icount_extra == 0); - - count = tcg_get_icount_limit(); - - /* To calculate what we have executed so far we need to know - * what we originally budgeted to run this cycle */ - cpu->icount_budget = count; - - decr = (count > 0xffff) ? 0xffff : count; - count -= decr; - cpu->icount_decr.u16.low = decr; - cpu->icount_extra = count; + cpu->icount_budget = tcg_get_icount_limit(); + insns_left = MIN(0xffff, cpu->icount_budget); + cpu->icount_decr.u16.low = insns_left; + cpu->icount_extra = cpu->icount_budget - insns_left; } } From 1d05906b95e7f2a35be2392506f9af8f89ff39a5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alex=20Benn=C3=A9e?= Date: Wed, 5 Apr 2017 10:53:47 +0100 Subject: [PATCH 10/11] cpus: call cpu_update_icount on read MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This ensures each time the vCPU thread reads the icount we update the master timer_state.qemu_icount field. This way as long as updates are in BQL protected sections (which they should be) the main-loop can never come to update the log and find time has gone backwards. Signed-off-by: Alex Bennée --- cpus.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/cpus.c b/cpus.c index 9c8bd2c991..740b8dc3f8 100644 --- a/cpus.c +++ b/cpus.c @@ -253,19 +253,21 @@ void cpu_update_icount(CPUState *cpu) int64_t cpu_get_icount_raw(void) { - int64_t icount; CPUState *cpu = current_cpu; - icount = atomic_read(&timers_state.qemu_icount); if (cpu && cpu->running) { if (!cpu->can_do_io) { fprintf(stderr, "Bad icount read\n"); exit(1); } /* Take into account what has run */ - icount += cpu_get_icount_executed(cpu); + cpu_update_icount(cpu); } - return icount; +#ifdef CONFIG_ATOMIC64 + return atomic_read__nocheck(&timers_state.qemu_icount); +#else /* FIXME: we need 64bit atomics to do this safely */ + return timers_state.qemu_icount; +#endif } /* Return the virtual CPU time, based on the instruction counter. */ From 982263ce714ffcc4c7c41a7b255bd29e093912fe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alex=20Benn=C3=A9e?= Date: Wed, 5 Apr 2017 11:05:28 +0100 Subject: [PATCH 11/11] replay: assert time only goes forward MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If we find ourselves trying to add an event to the log where time has gone backwards it is because a vCPU event has occurred and the main-loop is not yet aware of time moving forward. This should not happen and if it does its better to fail early than generate a log that will have weird behaviour. Signed-off-by: Alex Bennée --- replay/replay-internal.c | 4 ++++ replay/replay.c | 4 ++++ 2 files changed, 8 insertions(+) diff --git a/replay/replay-internal.c b/replay/replay-internal.c index bea7b4aa6b..fca8514012 100644 --- a/replay/replay-internal.c +++ b/replay/replay-internal.c @@ -195,6 +195,10 @@ void replay_save_instructions(void) if (replay_file && replay_mode == REPLAY_MODE_RECORD) { replay_mutex_lock(); int diff = (int)(replay_get_current_step() - replay_state.current_step); + + /* Time can only go forward */ + assert(diff >= 0); + if (diff > 0) { replay_put_event(EVENT_INSTRUCTION); replay_put_dword(diff); diff --git a/replay/replay.c b/replay/replay.c index 9e0724e756..f810628cac 100644 --- a/replay/replay.c +++ b/replay/replay.c @@ -84,6 +84,10 @@ void replay_account_executed_instructions(void) if (replay_state.instructions_count > 0) { int count = (int)(replay_get_current_step() - replay_state.current_step); + + /* Time can only go forward */ + assert(count >= 0); + replay_state.instructions_count -= count; replay_state.current_step += count; if (replay_state.instructions_count == 0) {