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

[Claudio Fontana]

On 9/1/20 12:20 PM, Alex Bennée wrote:
> 
> 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?

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

Thanks!

Claudio