Re: [PATCH v6 01/16] cpu-timers, icount: new modules

[Alex Bennée]

Claudio Fontana <cfontana@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 <cfontana@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?

<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 :-/

> 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.bennee@linaro.org>

-- 
Alex Bennée