Re: [RFC v3 1/8] cpu-timers, icount: new modules

[Claudio Fontana <cfontana@suse.de>]

Hi Alex, Paolo and all,

thank you for your feedback, could you help me answer the question below?

On 8/3/20 11:05 AM, Claudio Fontana wrote:
> ...

> diff --git a/dma-helpers.c b/dma-helpers.c
> index 2a77b5a9cb..240ef4d5b8 100644
> --- a/dma-helpers.c
> +++ b/dma-helpers.c
> @@ -13,7 +13,7 @@
>  #include "trace-root.h"
>  #include "qemu/thread.h"
>  #include "qemu/main-loop.h"
> -#include "sysemu/cpus.h"
> +#include "sysemu/cpu-timers.h"
>  #include "qemu/range.h"
>  
>  /* #define DEBUG_IOMMU */
> @@ -151,7 +151,7 @@ static void dma_blk_cb(void *opaque, int ret)
>           * from several sectors. This code splits all SGs into several
>           * groups. SGs in every group do not overlap.
>           */
> -        if (mem && use_icount && dbs->dir == DMA_DIRECTION_FROM_DEVICE) {
> +        if (mem && icount_enabled() && dbs->dir == DMA_DIRECTION_FROM_DEVICE) {




In this specific case, where dma_blk_cb() changes its behaviour to be more deterministic
if icount_enabled(),

do you think that if qtest_enabled() we should also follow the more deterministic path,
or should we go through the "normal" path instead, as this patch does?

Tests pass in any case, but I wonder what would be the best behavior for qtest accel in this case.
(Maybe Pavel?)



>              int i;
>              for (i = 0 ; i < dbs->iov.niov ; ++i) {
>                  if (ranges_overlap((intptr_t)dbs->iov.iov[i].iov_base,
> diff --git a/docs/replay.txt b/docs/replay.txt
> index 70c27edb36..8952e6d852 100644
> --- a/docs/replay.txt
> +++ b/docs/replay.txt
> @@ -184,11 +184,11 @@ is then incremented (which is called "warping" the virtual clock) as
>  soon as the timer fires or the CPUs need to go out of the idle state.
>  Two functions are used for this purpose; because these actions change
>  virtual machine state and must be deterministic, each of them creates a
> -checkpoint.  qemu_start_warp_timer checks if the CPUs are idle and if so
> -starts accounting real time to virtual clock.  qemu_account_warp_timer
> +checkpoint.  icount_start_warp_timer checks if the CPUs are idle and if so
> +starts accounting real time to virtual clock.  icount_account_warp_timer
>  is called when the CPUs get an interrupt or when the warp timer fires,
>  and it warps the virtual clock by the amount of real time that has passed
> -since qemu_start_warp_timer.
> +since icount_start_warp_timer.
>  
>  Bottom halves
>  -------------
> diff --git a/exec.c b/exec.c
> index 6f381f98e2..a89ffa93c1 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -102,10 +102,6 @@ uintptr_t qemu_host_page_size;
>  intptr_t qemu_host_page_mask;
>  
>  #if !defined(CONFIG_USER_ONLY)
> -/* 0 = Do not count executed instructions.
> -   1 = Precise instruction counting.
> -   2 = Adaptive rate instruction counting.  */
> -int use_icount;
>  
>  typedef struct PhysPageEntry PhysPageEntry;
>  
> diff --git a/hw/core/ptimer.c b/hw/core/ptimer.c
> index b5a54e2536..c6d2beb1da 100644
> --- a/hw/core/ptimer.c
> +++ b/hw/core/ptimer.c
> @@ -7,11 +7,11 @@
>   */
>  
>  #include "qemu/osdep.h"
> -#include "qemu/timer.h"
>  #include "hw/ptimer.h"
>  #include "migration/vmstate.h"
>  #include "qemu/host-utils.h"
>  #include "sysemu/replay.h"
> +#include "sysemu/cpu-timers.h"
>  #include "sysemu/qtest.h"
>  #include "block/aio.h"
>  #include "sysemu/cpus.h"
> @@ -134,7 +134,8 @@ static void ptimer_reload(ptimer_state *s, int delta_adjust)
>       * on the current generation of host machines.
>       */
>  
> -    if (s->enabled == 1 && (delta * period < 10000) && !use_icount) {
> +    if (s->enabled == 1 && (delta * period < 10000) &&
> +        !icount_enabled() && !qtest_enabled()) {


In this case, it is necessary to also make qtest more deterministic in order to make existing tests pass,
as the results of the timer are affecting the ptimer test results (IIRC tests/ptimer-test.c)


>          period = 10000 / delta;
>          period_frac = 0;
>      }
> @@ -217,7 +218,8 @@ uint64_t ptimer_get_count(ptimer_state *s)
>              uint32_t period_frac = s->period_frac;
>              uint64_t period = s->period;
>  
> -            if (!oneshot && (s->delta * period < 10000) && !use_icount) {
> +            if (!oneshot && (s->delta * period < 10000) &&
> +                !icount_enabled() && !qtest_enabled()) {

...same here.

>                  period = 10000 / s->delta;
>                  period_frac = 0;
>              }


Thanks for your feedback,

Claudio