On 9/1/20 12:20 PM, Alex Bennée wrote:
>
> Claudio Fontana <cfont...@suse.de> writes:
>
>> refactoring of cpus.c continues with cpu timer state extraction.
>>
>> cpu-timers: responsible for the softmmu cpu timers state,
>> including cpu clocks and ticks.
>>
>> icount: counts the TCG instructions executed. As such it is specific to
>> the TCG accelerator. Therefore, it is built only under CONFIG_TCG.
>>
>> One complication is due to qtest, which uses an icount field to warp time
>> as part of qtest (qtest_clock_warp).
>>
>> In order to solve this problem, provide a separate counter for qtest.
>>
>> This requires fixing assumptions scattered in the code that
>> qtest_enabled() implies icount_enabled(), checking each specific case.
>>
>> Signed-off-by: Claudio Fontana <cfont...@suse.de>
>> ---
> <snip>
>> +
>> +void qemu_timer_notify_cb(void *opaque, QEMUClockType type)
>> +{
>> + if (!icount_enabled() || type != QEMU_CLOCK_VIRTUAL) {
>> + qemu_notify_event();
>> + return;
>> + }
>> +
>> + if (qemu_in_vcpu_thread()) {
>> + /*
>> + * A CPU is currently running; kick it back out to the
>> + * tcg_cpu_exec() loop so it will recalculate its
>> + * icount deadline immediately.
>> + */
>> + qemu_cpu_kick(current_cpu);
>> + } else if (first_cpu) {
>> + /*
>> + * qemu_cpu_kick is not enough to kick a halted CPU out of
>> + * qemu_tcg_wait_io_event. async_run_on_cpu, instead,
>> + * causes cpu_thread_is_idle to return false. This way,
>> + * handle_icount_deadline can run.
>> + * If we have no CPUs at all for some reason, we don't
>> + * need to do anything.
>> + */
>> + async_run_on_cpu(first_cpu, do_nothing, RUN_ON_CPU_NULL);
>> + }
>> +}
>> +
>
> See bellow for comments on stub.
>
>> diff --git a/softmmu/vl.c b/softmmu/vl.c
>> index 0cc86b0766..ff94cf4983 100644
>> --- a/softmmu/vl.c
>> +++ b/softmmu/vl.c
>> @@ -74,6 +74,7 @@
>> #include "hw/audio/soundhw.h"
>> #include "audio/audio.h"
>> #include "sysemu/cpus.h"
>> +#include "sysemu/cpu-timers.h"
>> #include "migration/colo.h"
>> #include "migration/postcopy-ram.h"
>> #include "sysemu/kvm.h"
>> @@ -2802,7 +2803,7 @@ static void configure_accelerators(const char
>> *progname)
>> error_report("falling back to %s", ac->name);
>> }
>>
>> - if (use_icount && !(tcg_enabled() || qtest_enabled())) {
>> + if (icount_enabled() && !tcg_enabled()) {
>> error_report("-icount is not allowed with hardware virtualization");
>> exit(1);
>> }
>> @@ -4237,7 +4238,11 @@ void qemu_init(int argc, char **argv, char **envp)
>> semihosting_arg_fallback(kernel_filename, kernel_cmdline);
>> }
>>
>> - cpu_ticks_init();
>> + /* initialize cpu timers and VCPU throttle modules */
>> + cpu_timers_init();
>> +
>> + /* spice needs the timers to be initialized by this point */
>> + qemu_spice_init();
>
> This seems to be an additional fix?
This seems to be a mistake on my part, some rebase leftover, the initialization
already happened before.
Will remove/fix, thanks for the catch!
>
> <snip>
>> diff --git a/stubs/cpu-get-icount.c b/stubs/cpu-get-icount.c
>> deleted file mode 100644
>> index b35f844638..0000000000
>> --- a/stubs/cpu-get-icount.c
>> +++ /dev/null
>> @@ -1,21 +0,0 @@
>> -#include "qemu/osdep.h"
>> -#include "qemu/timer.h"
>> -#include "sysemu/cpus.h"
>> -#include "qemu/main-loop.h"
>> -
>> -int use_icount;
>> -
>> -int64_t cpu_get_icount(void)
>> -{
>> - abort();
>> -}
>> -
>> -int64_t cpu_get_icount_raw(void)
>> -{
>> - abort();
>> -}
>> -
>> -void qemu_timer_notify_cb(void *opaque, QEMUClockType type)
>> -{
>> - qemu_notify_event();
>> -}
>
> Hmm so this was existing behaviour for stubs - I find it slightly weird
> that a stub function actually does something - although of course that
> might be stubbed as well :-/
I am puzzled as well to see this.
I tried removing the qemu_notify_event() call, and it does not seem to break
anything for me,
but I'd keep it unchanged for now, maybe it was added for some reason?
>
>> diff --git a/stubs/icount.c b/stubs/icount.c
>> new file mode 100644
>> index 0000000000..61e28cbaf9
>> --- /dev/null
>> +++ b/stubs/icount.c
>> @@ -0,0 +1,45 @@
>> +#include "qemu/osdep.h"
>> +#include "qapi/error.h"
>> +#include "sysemu/cpu-timers.h"
>> +
>> +/* icount - Instruction Counter API */
>> +
>> +int use_icount;
>> +
>> +void cpu_update_icount(CPUState *cpu)
>> +{
>> + abort();
>> +}
>> +void configure_icount(QemuOpts *opts, Error **errp)
>> +{
>> + /* signal error */
>> + error_setg(errp, "cannot configure icount, TCG support not available");
>> +}
>> +int64_t cpu_get_icount_raw(void)
>> +{
>> + abort();
>> + return 0;
>> +}
>> +int64_t cpu_get_icount(void)
>> +{
>> + abort();
>> + return 0;
>> +}
>> +int64_t cpu_icount_to_ns(int64_t icount)
>> +{
>> + abort();
>> + return 0;
>> +}
>> +int64_t qemu_icount_round(int64_t count)
>> +{
>> + abort();
>> + return 0;
>> +}
>> +void qemu_start_warp_timer(void)
>> +{
>> + abort();
>> +}
>> +void qemu_account_warp_timer(void)
>> +{
>> + abort();
>> +}
>> diff --git a/stubs/meson.build b/stubs/meson.build
>> index 019bd79c7a..57fec4f8c8 100644
>> --- a/stubs/meson.build
>> +++ b/stubs/meson.build
>> @@ -3,10 +3,10 @@ stub_ss.add(files('bdrv-next-monitor-owned.c'))
>> stub_ss.add(files('blk-commit-all.c'))
>> stub_ss.add(files('blockdev-close-all-bdrv-states.c'))
>> stub_ss.add(files('change-state-handler.c'))
>> -stub_ss.add(files('clock-warp.c'))
>> stub_ss.add(files('cmos.c'))
>> stub_ss.add(files('cpu-get-clock.c'))
>> -stub_ss.add(files('cpu-get-icount.c'))
>> +stub_ss.add(files('qemu-timer-notify-cb.c'))
>> +stub_ss.add(files('icount.c'))
>> stub_ss.add(files('dump.c'))
>> stub_ss.add(files('error-printf.c'))
>> stub_ss.add(files('fd-register.c'))
>> diff --git a/stubs/qemu-timer-notify-cb.c b/stubs/qemu-timer-notify-cb.c
>> new file mode 100644
>> index 0000000000..845e46f8e0
>> --- /dev/null
>> +++ b/stubs/qemu-timer-notify-cb.c
>> @@ -0,0 +1,8 @@
>> +#include "qemu/osdep.h"
>> +#include "sysemu/cpu-timers.h"
>> +#include "qemu/main-loop.h"
>> +
>> +void qemu_timer_notify_cb(void *opaque, QEMUClockType type)
>> +{
>> + qemu_notify_event();
>> +}
>> diff --git a/stubs/qtest.c b/stubs/qtest.c
>> index 891eb954fb..4666a49d7d 100644
>> --- a/stubs/qtest.c
>> +++ b/stubs/qtest.c
>> @@ -18,3 +18,8 @@ bool qtest_driver(void)
>> {
>> return false;
>> }
>> +
>> +int64_t qtest_get_virtual_clock(void)
>> +{
>> + return 0;
>> +}
>> diff --git a/target/alpha/translate.c b/target/alpha/translate.c
>> index 8870284f57..36be602179 100644
>> --- a/target/alpha/translate.c
>> +++ b/target/alpha/translate.c
>> @@ -20,6 +20,7 @@
>> #include "qemu/osdep.h"
>> #include "cpu.h"
>> #include "sysemu/cpus.h"
>> +#include "sysemu/cpu-timers.h"
>> #include "disas/disas.h"
>> #include "qemu/host-utils.h"
>> #include "exec/exec-all.h"
>> @@ -1329,7 +1330,7 @@ static DisasJumpType gen_mfpr(DisasContext *ctx, TCGv
>> va, int regno)
>> case 249: /* VMTIME */
>> helper = gen_helper_get_vmtime;
>> do_helper:
>> - if (use_icount) {
>> + if (icount_enabled()) {
>> gen_io_start();
>> helper(va);
>> return DISAS_PC_STALE;
>> diff --git a/target/arm/helper.c b/target/arm/helper.c
>> index 44d666627a..dc2b91084c 100644
>> --- a/target/arm/helper.c
>> +++ b/target/arm/helper.c
>> @@ -24,6 +24,7 @@
>> #include "hw/irq.h"
>> #include "hw/semihosting/semihost.h"
>> #include "sysemu/cpus.h"
>> +#include "sysemu/cpu-timers.h"
>> #include "sysemu/kvm.h"
>> #include "sysemu/tcg.h"
>> #include "qemu/range.h"
>> @@ -1206,7 +1207,7 @@ static int64_t cycles_ns_per(uint64_t cycles)
>>
>> static bool instructions_supported(CPUARMState *env)
>> {
>> - return use_icount == 1 /* Precise instruction counting */;
>> + return icount_enabled() == 1; /* Precise instruction counting */
>> }
>>
>> static uint64_t instructions_get_count(CPUARMState *env)
>> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
>> index 200001de74..5231404a70 100644
>> --- a/target/riscv/csr.c
>> +++ b/target/riscv/csr.c
>> @@ -299,7 +299,7 @@ static int write_vstart(CPURISCVState *env, int csrno,
>> target_ulong val)
>> static int read_instret(CPURISCVState *env, int csrno, target_ulong *val)
>> {
>> #if !defined(CONFIG_USER_ONLY)
>> - if (use_icount) {
>> + if (icount_enabled()) {
>> *val = cpu_get_icount();
>> } else {
>> *val = cpu_get_host_ticks();
>> @@ -314,7 +314,7 @@ static int read_instret(CPURISCVState *env, int csrno,
>> target_ulong *val)
>> static int read_instreth(CPURISCVState *env, int csrno, target_ulong *val)
>> {
>> #if !defined(CONFIG_USER_ONLY)
>> - if (use_icount) {
>> + if (icount_enabled()) {
>> *val = cpu_get_icount() >> 32;
>> } else {
>> *val = cpu_get_host_ticks() >> 32;
>> diff --git a/tests/ptimer-test-stubs.c b/tests/ptimer-test-stubs.c
>> index ed393d9082..e935a1395e 100644
>> --- a/tests/ptimer-test-stubs.c
>> +++ b/tests/ptimer-test-stubs.c
>> @@ -12,6 +12,7 @@
>> #include "qemu/main-loop.h"
>> #include "sysemu/replay.h"
>> #include "migration/vmstate.h"
>> +#include "sysemu/cpu-timers.h"
>>
>> #include "ptimer-test.h"
>>
>> @@ -30,8 +31,8 @@ QEMUTimerListGroup main_loop_tlg;
>>
>> int64_t ptimer_test_time_ns;
>>
>> -/* Do not artificially limit period - see hw/core/ptimer.c. */
>> -int use_icount = 1;
>> +/* under qtest_enabled(), will not artificially limit period - see
>> hw/core/ptimer.c. */
>> +int use_icount;
>> bool qtest_allowed;
>>
>> void timer_init_full(QEMUTimer *ts,
>> diff --git a/tests/test-timed-average.c b/tests/test-timed-average.c
>> index e2bcf5fe13..82c92500df 100644
>> --- a/tests/test-timed-average.c
>> +++ b/tests/test-timed-average.c
>> @@ -11,7 +11,7 @@
>> */
>>
>> #include "qemu/osdep.h"
>> -
>> +#include "sysemu/cpu-timers.h"
>> #include "qemu/timed-average.h"
>>
>> /* This is the clock for QEMU_CLOCK_VIRTUAL */
>> diff --git a/util/main-loop.c b/util/main-loop.c
>> index f69f055013..4015d58967 100644
>> --- a/util/main-loop.c
>> +++ b/util/main-loop.c
>> @@ -27,7 +27,7 @@
>> #include "qemu/cutils.h"
>> #include "qemu/timer.h"
>> #include "sysemu/qtest.h"
>> -#include "sysemu/cpus.h"
>> +#include "sysemu/cpu-timers.h"
>> #include "sysemu/replay.h"
>> #include "qemu/main-loop.h"
>> #include "block/aio.h"
>> @@ -517,9 +517,13 @@ void main_loop_wait(int nonblocking)
>> mlpoll.state = ret < 0 ? MAIN_LOOP_POLL_ERR : MAIN_LOOP_POLL_OK;
>> notifier_list_notify(&main_loop_poll_notifiers, &mlpoll);
>>
>> - /* CPU thread can infinitely wait for event after
>> - missing the warp */
>> - qemu_start_warp_timer();
>> + if (icount_enabled()) {
>> + /*
>> + * CPU thread can infinitely wait for event after
>> + * missing the warp
>> + */
>> + qemu_start_warp_timer();
>> + }
>> qemu_clock_run_all_timers();
>> }
>>
>> diff --git a/util/qemu-timer.c b/util/qemu-timer.c
>> index f62b4feecd..100a4eba22 100644
>> --- a/util/qemu-timer.c
>> +++ b/util/qemu-timer.c
>> @@ -26,8 +26,10 @@
>> #include "qemu/main-loop.h"
>> #include "qemu/timer.h"
>> #include "qemu/lockable.h"
>> +#include "sysemu/cpu-timers.h"
>> #include "sysemu/replay.h"
>> #include "sysemu/cpus.h"
>> +#include "sysemu/qtest.h"
>>
>> #ifdef CONFIG_POSIX
>> #include <pthread.h>
>> @@ -134,7 +136,7 @@ static void qemu_clock_init(QEMUClockType type,
>> QEMUTimerListNotifyCB *notify_cb
>>
>> bool qemu_clock_use_for_deadline(QEMUClockType type)
>> {
>> - return !(use_icount && (type == QEMU_CLOCK_VIRTUAL));
>> + return !(icount_enabled() && (type == QEMU_CLOCK_VIRTUAL));
>> }
>>
>> void qemu_clock_notify(QEMUClockType type)
>> @@ -416,7 +418,7 @@ static bool timer_mod_ns_locked(QEMUTimerList
>> *timer_list,
>> static void timerlist_rearm(QEMUTimerList *timer_list)
>> {
>> /* Interrupt execution to force deadline recalculation. */
>> - if (timer_list->clock->type == QEMU_CLOCK_VIRTUAL) {
>> + if (icount_enabled() && timer_list->clock->type == QEMU_CLOCK_VIRTUAL) {
>> qemu_start_warp_timer();
>> }
>> timerlist_notify(timer_list);
>> @@ -633,8 +635,10 @@ int64_t qemu_clock_get_ns(QEMUClockType type)
>> return get_clock();
>> default:
>> case QEMU_CLOCK_VIRTUAL:
>> - if (use_icount) {
>> + if (icount_enabled()) {
>> return cpu_get_icount();
>> + } else if (qtest_enabled()) { /* for qtest_clock_warp */
>> + return qtest_get_virtual_clock();
>> } else {
>> return cpu_get_clock();
>> }
>
> Otherwise:
>
> Reviewed-by: Alex Bennée <alex.ben...@linaro.org>
>
Thanks!
Claudio
