qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/3] QEMU cpus.c refactoring part1
@ 2020-06-29  9:35 Claudio Fontana
  2020-06-29  9:35 ` [PATCH 1/3] softmmu: move softmmu only files from root Claudio Fontana
                   ` (3 more replies)
  0 siblings, 4 replies; 37+ messages in thread
From: Claudio Fontana @ 2020-06-29  9:35 UTC (permalink / raw)
  To: Paolo Bonzini, Thomas Huth, Laurent Vivier, Alex Bennée,
	Peter Maydell, Philippe Mathieu-Daudé,
	Roman Bolshakov, Markus Armbruster
  Cc: Eduardo Habkost, Marcelo Tosatti, qemu-devel, haxm-team,
	Wenchao Wang, Sunil Muthuswamy, Richard Henderson,
	Claudio Fontana, Colin Xu

Motivation and higher level steps:

https://lists.gnu.org/archive/html/qemu-devel/2020-05/msg04628.html

Previous series: [RFC RESEND v7 0/4] QEMU cpus.c refactoring

This series is already reviewed, and is a split of the first three patches
from the previous series (RFC). The forth and last of the previous series
will then be posted separately.

PREVIOUS DISCUSSIONS:

* should we reorder patches or moves inside patches to avoid code going
  from cpus.c to softmmu/cpus.c and then again to softmmu/somethingelse.c ?
  (Philippe)

* some questions about headers in include/softmmu (Philippe)


[SPLIT into TWO series, changed from RFC to PATCH]
----

v6 -> v7:

* rebased changes on top of Pavel Dovgalyuk changes to dma-helpers.c
  "icount: make dma reads deterministic"

----

v5 -> v6:

* rebased changes on top of Emilio G. Cota changes to cpus.c
  "cpu: convert queued work to a QSIMPLEQ"

* keep a pointer in cpus.c instead of a copy of CpusAccel
  (Alex)

----


v4 -> v5: rebase on latest master

* rebased changes on top of roman series to remove one of the extra states for hvf.
  (Is the result now functional for HVF?)

* rebased changes on top of icount changes and fixes to icount_configure and
  the new shift vmstate. (Markus)

v3 -> v4:

* overall: added copyright headers to all files that were missing them
  (used copyright and license of the module the stuff was extracted from).
  For the new interface files, added SUSE LLC.

* 1/4 (move softmmu only files from root):

  MAINTAINERS: moved softmmu/cpus.c to its final location (from patch 2)

* 2/4 (cpu-throttle):

  MAINTAINERS (to patch 1),
  copyright Fabrice Bellard and license from cpus.c

* 3/4 (cpu-timers, icount):

  - MAINTAINERS: add cpu-timers.c and icount.c to Paolo

  - break very long lines (patchew)

  - add copyright SUSE LLC, GPLv2 to cpu-timers.h

  - add copyright Fabrice Bellard and license from cpus.c to timers-state.h
    as it is lifted from cpus.c

  - vl.c: in configure_accelerators bail out if icount_enabled()
    and !tcg_enabled() as qtest does not enable icount anymore.

* 4/4 (accel stuff to accel):

  - add copyright SUSE LLC to files that mostly only consist of the
    new interface. Add whatever copyright was in the accelerator code
    if instead they mostly consist of accelerator code.

  - change a comment to mention the result of the AccelClass experiment

  - moved qtest accelerator into accel/qtest/ , make it like the others.

  - rename xxx-cpus-interface to xxx-cpus (remove "interface" from names)

  - rename accel_int to cpus_accel

  - rename CpusAccel functions from cpu_synchronize_* to synchronize_*


--------

v2 -> v3:

* turned into a 4 patch series, adding a first patch moving
  softmmu code currently in top_srcdir to softmmu/

* cpu-throttle: moved to softmmu/

* cpu-timers, icount:

  - moved to softmmu/

  - fixed assumption of qtest_enabled() => icount_enabled()
  causing the failure of check-qtest-arm goal, in test-arm-mptimer.c

  Fix is in hw/core/ptimer.c,

  where the artificial timeout rate limit should not be applied
  under qtest_enabled(), in a similar way to how it is not applied
  for icount_enabled().

* CpuAccelInterface: no change.


--------


v1 -> v2:

* 1/3 (cpu-throttle): provide a description in the commit message

* 2/3 (cpu-timers, icount): in this v2 separate icount from cpu-timers,
  as icount is actually TCG-specific. Only build it under CONFIG_TCG.

  To do this, qtest had to be detached from icount. To this end, a
  trivial global counter for qtest has been introduced.

* 3/3 (CpuAccelInterface): provided a description.

This is point 8) in that plan. The idea is to extract the unrelated parts
in cpus, and register interfaces from each single accelerator to the main
cpus module (cpus.c).

While doing this RFC, I noticed some assumptions about Windows being
either TCG or HAX (not considering WHPX) that might need to be revisited.
I added a comment there.

The thing builds successfully based on Linux cross-compilations for
windows/hax, windows/whpx, and I got a good build on Darwin/hvf.

Tests run successully for tcg and kvm configurations, but did not test on
windows or darwin.

Welcome your feedback and help on this,

Claudio

Claudio Fontana (3):
  softmmu: move softmmu only files from root
  cpu-throttle: new module, extracted from cpus.c
  cpu-timers, icount: new modules

 MAINTAINERS                                  |  15 +-
 Makefile.target                              |   7 +-
 accel/qtest.c                                |   6 +-
 accel/tcg/cpu-exec.c                         |  43 +-
 accel/tcg/tcg-all.c                          |   7 +-
 accel/tcg/translate-all.c                    |   3 +-
 dma-helpers.c                                |   4 +-
 docs/replay.txt                              |   6 +-
 exec.c                                       |   4 -
 hw/core/ptimer.c                             |   8 +-
 hw/i386/x86.c                                |   1 +
 include/exec/cpu-all.h                       |   4 +
 include/exec/exec-all.h                      |   4 +-
 include/hw/core/cpu.h                        |  37 --
 include/qemu/main-loop.h                     |   5 +
 include/qemu/timer.h                         |  22 +-
 include/sysemu/cpu-throttle.h                |  68 +++
 include/sysemu/cpu-timers.h                  |  81 +++
 include/sysemu/cpus.h                        |  12 +-
 include/sysemu/qtest.h                       |   2 +
 include/sysemu/replay.h                      |   4 +-
 migration/migration.c                        |   1 +
 migration/ram.c                              |   1 +
 replay/replay.c                              |   6 +-
 softmmu/Makefile.objs                        |  13 +
 arch_init.c => softmmu/arch_init.c           |   0
 balloon.c => softmmu/balloon.c               |   0
 softmmu/cpu-throttle.c                       | 122 ++++
 softmmu/cpu-timers.c                         | 284 +++++++++
 cpus.c => softmmu/cpus.c                     | 839 +--------------------------
 softmmu/icount.c                             | 499 ++++++++++++++++
 ioport.c => softmmu/ioport.c                 |   0
 memory.c => softmmu/memory.c                 |   0
 memory_mapping.c => softmmu/memory_mapping.c |   0
 qtest.c => softmmu/qtest.c                   |  34 +-
 softmmu/timers-state.h                       |  69 +++
 softmmu/vl.c                                 |   8 +-
 stubs/Makefile.objs                          |   3 +-
 stubs/clock-warp.c                           |   4 +-
 stubs/cpu-get-clock.c                        |   3 +-
 stubs/cpu-get-icount.c                       |  21 -
 stubs/icount.c                               |  22 +
 stubs/qemu-timer-notify-cb.c                 |   8 +
 stubs/qtest.c                                |   5 +
 target/alpha/translate.c                     |   3 +-
 target/arm/helper.c                          |   7 +-
 target/riscv/csr.c                           |   8 +-
 tests/ptimer-test-stubs.c                    |   7 +-
 tests/test-timed-average.c                   |   2 +-
 util/main-loop.c                             |   4 +-
 util/qemu-timer.c                            |  12 +-
 51 files changed, 1343 insertions(+), 985 deletions(-)
 create mode 100644 include/sysemu/cpu-throttle.h
 create mode 100644 include/sysemu/cpu-timers.h
 rename arch_init.c => softmmu/arch_init.c (100%)
 rename balloon.c => softmmu/balloon.c (100%)
 create mode 100644 softmmu/cpu-throttle.c
 create mode 100644 softmmu/cpu-timers.c
 rename cpus.c => softmmu/cpus.c (59%)
 create mode 100644 softmmu/icount.c
 rename ioport.c => softmmu/ioport.c (100%)
 rename memory.c => softmmu/memory.c (100%)
 rename memory_mapping.c => softmmu/memory_mapping.c (100%)
 rename qtest.c => softmmu/qtest.c (95%)
 create mode 100644 softmmu/timers-state.h
 delete mode 100644 stubs/cpu-get-icount.c
 create mode 100644 stubs/icount.c
 create mode 100644 stubs/qemu-timer-notify-cb.c

-- 
2.16.4


^ permalink raw reply	[flat|nested] 37+ messages in thread

* [PATCH 1/3] softmmu: move softmmu only files from root
  2020-06-29  9:35 [PATCH 0/3] QEMU cpus.c refactoring part1 Claudio Fontana
@ 2020-06-29  9:35 ` Claudio Fontana
  2020-07-03 17:21   ` Paolo Bonzini
  2020-06-29  9:35 ` [PATCH 2/3] cpu-throttle: new module, extracted from cpus.c Claudio Fontana
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 37+ messages in thread
From: Claudio Fontana @ 2020-06-29  9:35 UTC (permalink / raw)
  To: Paolo Bonzini, Thomas Huth, Laurent Vivier, Alex Bennée,
	Peter Maydell, Philippe Mathieu-Daudé,
	Roman Bolshakov, Markus Armbruster
  Cc: Eduardo Habkost, Marcelo Tosatti, qemu-devel, haxm-team,
	Wenchao Wang, Sunil Muthuswamy, Richard Henderson,
	Claudio Fontana, Colin Xu

move arch_init, balloon, cpus, ioport, memory, memory_mapping, qtest.

They are all specific to CONFIG_SOFTMMU.

Signed-off-by: Claudio Fontana <cfontana@suse.de>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Laurent Vivier <lvivier@redhat.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
 MAINTAINERS                                  | 12 ++++++------
 Makefile.target                              |  7 ++-----
 softmmu/Makefile.objs                        | 10 ++++++++++
 arch_init.c => softmmu/arch_init.c           |  0
 balloon.c => softmmu/balloon.c               |  0
 cpus.c => softmmu/cpus.c                     |  0
 ioport.c => softmmu/ioport.c                 |  0
 memory.c => softmmu/memory.c                 |  0
 memory_mapping.c => softmmu/memory_mapping.c |  0
 qtest.c => softmmu/qtest.c                   |  0
 10 files changed, 18 insertions(+), 11 deletions(-)
 rename arch_init.c => softmmu/arch_init.c (100%)
 rename balloon.c => softmmu/balloon.c (100%)
 rename cpus.c => softmmu/cpus.c (100%)
 rename ioport.c => softmmu/ioport.c (100%)
 rename memory.c => softmmu/memory.c (100%)
 rename memory_mapping.c => softmmu/memory_mapping.c (100%)
 rename qtest.c => softmmu/qtest.c (100%)

diff --git a/MAINTAINERS b/MAINTAINERS
index dec252f38b..7214f59383 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -115,7 +115,7 @@ Overall TCG CPUs
 M: Richard Henderson <rth@twiddle.net>
 R: Paolo Bonzini <pbonzini@redhat.com>
 S: Maintained
-F: cpus.c
+F: softmmu/cpus.c
 F: cpus-common.c
 F: exec.c
 F: accel/tcg/
@@ -1708,7 +1708,7 @@ M: David Hildenbrand <david@redhat.com>
 S: Maintained
 F: hw/virtio/virtio-balloon*.c
 F: include/hw/virtio/virtio-balloon.h
-F: balloon.c
+F: softmmu/balloon.c
 F: include/sysemu/balloon.h
 
 virtio-9p
@@ -2177,12 +2177,12 @@ Memory API
 M: Paolo Bonzini <pbonzini@redhat.com>
 S: Supported
 F: include/exec/ioport.h
-F: ioport.c
 F: include/exec/memop.h
 F: include/exec/memory.h
 F: include/exec/ram_addr.h
 F: include/exec/ramblock.h
-F: memory.c
+F: softmmu/ioport.c
+F: softmmu/memory.c
 F: include/exec/memory-internal.h
 F: exec.c
 F: scripts/coccinelle/memory-region-housekeeping.cocci
@@ -2214,13 +2214,13 @@ F: ui/cocoa.m
 Main loop
 M: Paolo Bonzini <pbonzini@redhat.com>
 S: Maintained
-F: cpus.c
 F: include/qemu/main-loop.h
 F: include/sysemu/runstate.h
 F: util/main-loop.c
 F: util/qemu-timer.c
 F: softmmu/vl.c
 F: softmmu/main.c
+F: softmmu/cpus.c
 F: qapi/run-state.json
 
 Human Monitor (HMP)
@@ -2375,7 +2375,7 @@ M: Thomas Huth <thuth@redhat.com>
 M: Laurent Vivier <lvivier@redhat.com>
 R: Paolo Bonzini <pbonzini@redhat.com>
 S: Maintained
-F: qtest.c
+F: softmmu/qtest.c
 F: accel/qtest.c
 F: tests/qtest/
 X: tests/qtest/bios-tables-test-allowed-diff.h
diff --git a/Makefile.target b/Makefile.target
index 8ed1eba95b..7fbf5d8b92 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -152,16 +152,13 @@ endif #CONFIG_BSD_USER
 #########################################################
 # System emulator target
 ifdef CONFIG_SOFTMMU
-obj-y += arch_init.o cpus.o gdbstub.o balloon.o ioport.o
-obj-y += qtest.o
+obj-y += softmmu/
+obj-y += gdbstub.o
 obj-y += dump/
 obj-y += hw/
 obj-y += monitor/
 obj-y += qapi/
-obj-y += memory.o
-obj-y += memory_mapping.o
 obj-y += migration/ram.o
-obj-y += softmmu/
 LIBS := $(libs_softmmu) $(LIBS)
 
 # Hardware support
diff --git a/softmmu/Makefile.objs b/softmmu/Makefile.objs
index dd15c24346..a4bd9f2f52 100644
--- a/softmmu/Makefile.objs
+++ b/softmmu/Makefile.objs
@@ -1,3 +1,13 @@
 softmmu-main-y = softmmu/main.o
+
+obj-y += arch_init.o
+obj-y += cpus.o
+obj-y += balloon.o
+obj-y += ioport.o
+obj-y += memory.o
+obj-y += memory_mapping.o
+
+obj-y += qtest.o
+
 obj-y += vl.o
 vl.o-cflags := $(GPROF_CFLAGS) $(SDL_CFLAGS)
diff --git a/arch_init.c b/softmmu/arch_init.c
similarity index 100%
rename from arch_init.c
rename to softmmu/arch_init.c
diff --git a/balloon.c b/softmmu/balloon.c
similarity index 100%
rename from balloon.c
rename to softmmu/balloon.c
diff --git a/cpus.c b/softmmu/cpus.c
similarity index 100%
rename from cpus.c
rename to softmmu/cpus.c
diff --git a/ioport.c b/softmmu/ioport.c
similarity index 100%
rename from ioport.c
rename to softmmu/ioport.c
diff --git a/memory.c b/softmmu/memory.c
similarity index 100%
rename from memory.c
rename to softmmu/memory.c
diff --git a/memory_mapping.c b/softmmu/memory_mapping.c
similarity index 100%
rename from memory_mapping.c
rename to softmmu/memory_mapping.c
diff --git a/qtest.c b/softmmu/qtest.c
similarity index 100%
rename from qtest.c
rename to softmmu/qtest.c
-- 
2.16.4



^ permalink raw reply related	[flat|nested] 37+ messages in thread

* [PATCH 2/3] cpu-throttle: new module, extracted from cpus.c
  2020-06-29  9:35 [PATCH 0/3] QEMU cpus.c refactoring part1 Claudio Fontana
  2020-06-29  9:35 ` [PATCH 1/3] softmmu: move softmmu only files from root Claudio Fontana
@ 2020-06-29  9:35 ` Claudio Fontana
  2020-06-29  9:35 ` [PATCH 3/3] cpu-timers, icount: new modules Claudio Fontana
  2020-07-02  6:27 ` [PATCH 0/3] QEMU cpus.c refactoring part1 Claudio Fontana
  3 siblings, 0 replies; 37+ messages in thread
From: Claudio Fontana @ 2020-06-29  9:35 UTC (permalink / raw)
  To: Paolo Bonzini, Thomas Huth, Laurent Vivier, Alex Bennée,
	Peter Maydell, Philippe Mathieu-Daudé,
	Roman Bolshakov, Markus Armbruster
  Cc: Eduardo Habkost, Marcelo Tosatti, qemu-devel, haxm-team,
	Wenchao Wang, Sunil Muthuswamy, Richard Henderson,
	Claudio Fontana, Colin Xu

move the vcpu throttling functionality into its own module.

This functionality is not specific to any accelerator,
and it is used currently by migration to slow down guests to try to
have migrations converge, and by the cocoa MacOS UI to throttle speed.

cpu-throttle contains the controls to adjust and inspect throttle
settings, start (set) and stop vcpu throttling, and the throttling
function itself that is run periodically on vcpus to make them take a nap.

Execution of the throttling function on all vcpus is triggered by a timer,
registered at module initialization.

No functionality change.

Signed-off-by: Claudio Fontana <cfontana@suse.de>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Laurent Vivier <lvivier@redhat.com>
---
 MAINTAINERS                   |   1 +
 include/hw/core/cpu.h         |  37 -------------
 include/qemu/main-loop.h      |   5 ++
 include/sysemu/cpu-throttle.h |  68 +++++++++++++++++++++++
 migration/migration.c         |   1 +
 migration/ram.c               |   1 +
 softmmu/Makefile.objs         |   1 +
 softmmu/cpu-throttle.c        | 122 ++++++++++++++++++++++++++++++++++++++++++
 softmmu/cpus.c                |  95 +++-----------------------------
 9 files changed, 207 insertions(+), 124 deletions(-)
 create mode 100644 include/sysemu/cpu-throttle.h
 create mode 100644 softmmu/cpu-throttle.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 7214f59383..827668fa8a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2221,6 +2221,7 @@ F: util/qemu-timer.c
 F: softmmu/vl.c
 F: softmmu/main.c
 F: softmmu/cpus.c
+F: softmmu/cpu-throttle.c
 F: qapi/run-state.json
 
 Human Monitor (HMP)
diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index b3f4b79318..5542577d2b 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -822,43 +822,6 @@ bool cpu_exists(int64_t id);
  */
 CPUState *cpu_by_arch_id(int64_t id);
 
-/**
- * cpu_throttle_set:
- * @new_throttle_pct: Percent of sleep time. Valid range is 1 to 99.
- *
- * Throttles all vcpus by forcing them to sleep for the given percentage of
- * time. A throttle_percentage of 25 corresponds to a 75% duty cycle roughly.
- * (example: 10ms sleep for every 30ms awake).
- *
- * cpu_throttle_set can be called as needed to adjust new_throttle_pct.
- * Once the throttling starts, it will remain in effect until cpu_throttle_stop
- * is called.
- */
-void cpu_throttle_set(int new_throttle_pct);
-
-/**
- * cpu_throttle_stop:
- *
- * Stops the vcpu throttling started by cpu_throttle_set.
- */
-void cpu_throttle_stop(void);
-
-/**
- * cpu_throttle_active:
- *
- * Returns: %true if the vcpus are currently being throttled, %false otherwise.
- */
-bool cpu_throttle_active(void);
-
-/**
- * cpu_throttle_get_percentage:
- *
- * Returns the vcpu throttle percentage. See cpu_throttle_set for details.
- *
- * Returns: The throttle percentage in range 1 to 99.
- */
-int cpu_throttle_get_percentage(void);
-
 #ifndef CONFIG_USER_ONLY
 
 typedef void (*CPUInterruptHandler)(CPUState *, int);
diff --git a/include/qemu/main-loop.h b/include/qemu/main-loop.h
index a6d20b0719..8e98613656 100644
--- a/include/qemu/main-loop.h
+++ b/include/qemu/main-loop.h
@@ -303,6 +303,11 @@ void qemu_mutex_unlock_iothread(void);
  */
 void qemu_cond_wait_iothread(QemuCond *cond);
 
+/*
+ * qemu_cond_timedwait_iothread: like the previous, but with timeout
+ */
+void qemu_cond_timedwait_iothread(QemuCond *cond, int ms);
+
 /* internal interfaces */
 
 void qemu_fd_register(int fd);
diff --git a/include/sysemu/cpu-throttle.h b/include/sysemu/cpu-throttle.h
new file mode 100644
index 0000000000..d65bdef6d0
--- /dev/null
+++ b/include/sysemu/cpu-throttle.h
@@ -0,0 +1,68 @@
+/*
+ * Copyright (c) 2012 SUSE LINUX Products GmbH
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see
+ * <http://www.gnu.org/licenses/gpl-2.0.html>
+ */
+
+#ifndef SYSEMU_CPU_THROTTLE_H
+#define SYSEMU_CPU_THROTTLE_H
+
+#include "qemu/timer.h"
+
+/**
+ * cpu_throttle_init:
+ *
+ * Initialize the CPU throttling API.
+ */
+void cpu_throttle_init(void);
+
+/**
+ * cpu_throttle_set:
+ * @new_throttle_pct: Percent of sleep time. Valid range is 1 to 99.
+ *
+ * Throttles all vcpus by forcing them to sleep for the given percentage of
+ * time. A throttle_percentage of 25 corresponds to a 75% duty cycle roughly.
+ * (example: 10ms sleep for every 30ms awake).
+ *
+ * cpu_throttle_set can be called as needed to adjust new_throttle_pct.
+ * Once the throttling starts, it will remain in effect until cpu_throttle_stop
+ * is called.
+ */
+void cpu_throttle_set(int new_throttle_pct);
+
+/**
+ * cpu_throttle_stop:
+ *
+ * Stops the vcpu throttling started by cpu_throttle_set.
+ */
+void cpu_throttle_stop(void);
+
+/**
+ * cpu_throttle_active:
+ *
+ * Returns: %true if the vcpus are currently being throttled, %false otherwise.
+ */
+bool cpu_throttle_active(void);
+
+/**
+ * cpu_throttle_get_percentage:
+ *
+ * Returns the vcpu throttle percentage. See cpu_throttle_set for details.
+ *
+ * Returns: The throttle percentage in range 1 to 99.
+ */
+int cpu_throttle_get_percentage(void);
+
+#endif /* SYSEMU_CPU_THROTTLE_H */
diff --git a/migration/migration.c b/migration/migration.c
index 481a590f72..676fb6084e 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -23,6 +23,7 @@
 #include "socket.h"
 #include "sysemu/runstate.h"
 #include "sysemu/sysemu.h"
+#include "sysemu/cpu-throttle.h"
 #include "rdma.h"
 #include "ram.h"
 #include "migration/global_state.h"
diff --git a/migration/ram.c b/migration/ram.c
index 069b6e30bc..9ef1e63cb6 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -52,6 +52,7 @@
 #include "migration/colo.h"
 #include "block.h"
 #include "sysemu/sysemu.h"
+#include "sysemu/cpu-throttle.h"
 #include "savevm.h"
 #include "qemu/iov.h"
 #include "multifd.h"
diff --git a/softmmu/Makefile.objs b/softmmu/Makefile.objs
index a4bd9f2f52..a414a74c50 100644
--- a/softmmu/Makefile.objs
+++ b/softmmu/Makefile.objs
@@ -2,6 +2,7 @@ softmmu-main-y = softmmu/main.o
 
 obj-y += arch_init.o
 obj-y += cpus.o
+obj-y += cpu-throttle.o
 obj-y += balloon.o
 obj-y += ioport.o
 obj-y += memory.o
diff --git a/softmmu/cpu-throttle.c b/softmmu/cpu-throttle.c
new file mode 100644
index 0000000000..4e6b2818ca
--- /dev/null
+++ b/softmmu/cpu-throttle.c
@@ -0,0 +1,122 @@
+/*
+ * QEMU System Emulator
+ *
+ * Copyright (c) 2003-2008 Fabrice Bellard
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu-common.h"
+#include "qemu/thread.h"
+#include "hw/core/cpu.h"
+#include "qemu/main-loop.h"
+#include "sysemu/cpus.h"
+#include "sysemu/cpu-throttle.h"
+
+/* vcpu throttling controls */
+static QEMUTimer *throttle_timer;
+static unsigned int throttle_percentage;
+
+#define CPU_THROTTLE_PCT_MIN 1
+#define CPU_THROTTLE_PCT_MAX 99
+#define CPU_THROTTLE_TIMESLICE_NS 10000000
+
+static void cpu_throttle_thread(CPUState *cpu, run_on_cpu_data opaque)
+{
+    double pct;
+    double throttle_ratio;
+    int64_t sleeptime_ns, endtime_ns;
+
+    if (!cpu_throttle_get_percentage()) {
+        return;
+    }
+
+    pct = (double)cpu_throttle_get_percentage() / 100;
+    throttle_ratio = pct / (1 - pct);
+    /* Add 1ns to fix double's rounding error (like 0.9999999...) */
+    sleeptime_ns = (int64_t)(throttle_ratio * CPU_THROTTLE_TIMESLICE_NS + 1);
+    endtime_ns = qemu_clock_get_ns(QEMU_CLOCK_REALTIME) + sleeptime_ns;
+    while (sleeptime_ns > 0 && !cpu->stop) {
+        if (sleeptime_ns > SCALE_MS) {
+            qemu_cond_timedwait_iothread(cpu->halt_cond,
+                                         sleeptime_ns / SCALE_MS);
+        } else {
+            qemu_mutex_unlock_iothread();
+            g_usleep(sleeptime_ns / SCALE_US);
+            qemu_mutex_lock_iothread();
+        }
+        sleeptime_ns = endtime_ns - qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
+    }
+    atomic_set(&cpu->throttle_thread_scheduled, 0);
+}
+
+static void cpu_throttle_timer_tick(void *opaque)
+{
+    CPUState *cpu;
+    double pct;
+
+    /* Stop the timer if needed */
+    if (!cpu_throttle_get_percentage()) {
+        return;
+    }
+    CPU_FOREACH(cpu) {
+        if (!atomic_xchg(&cpu->throttle_thread_scheduled, 1)) {
+            async_run_on_cpu(cpu, cpu_throttle_thread,
+                             RUN_ON_CPU_NULL);
+        }
+    }
+
+    pct = (double)cpu_throttle_get_percentage() / 100;
+    timer_mod(throttle_timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL_RT) +
+                                   CPU_THROTTLE_TIMESLICE_NS / (1 - pct));
+}
+
+void cpu_throttle_set(int new_throttle_pct)
+{
+    /* Ensure throttle percentage is within valid range */
+    new_throttle_pct = MIN(new_throttle_pct, CPU_THROTTLE_PCT_MAX);
+    new_throttle_pct = MAX(new_throttle_pct, CPU_THROTTLE_PCT_MIN);
+
+    atomic_set(&throttle_percentage, new_throttle_pct);
+
+    timer_mod(throttle_timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL_RT) +
+                                       CPU_THROTTLE_TIMESLICE_NS);
+}
+
+void cpu_throttle_stop(void)
+{
+    atomic_set(&throttle_percentage, 0);
+}
+
+bool cpu_throttle_active(void)
+{
+    return (cpu_throttle_get_percentage() != 0);
+}
+
+int cpu_throttle_get_percentage(void)
+{
+    return atomic_read(&throttle_percentage);
+}
+
+void cpu_throttle_init(void)
+{
+    throttle_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL_RT,
+                                  cpu_throttle_timer_tick, NULL);
+}
diff --git a/softmmu/cpus.c b/softmmu/cpus.c
index 41d1c5099f..f446536b88 100644
--- a/softmmu/cpus.c
+++ b/softmmu/cpus.c
@@ -61,6 +61,8 @@
 #include "hw/boards.h"
 #include "hw/hw.h"
 
+#include "sysemu/cpu-throttle.h"
+
 #ifdef CONFIG_LINUX
 
 #include <sys/prctl.h>
@@ -84,14 +86,6 @@ static QemuMutex qemu_global_mutex;
 int64_t max_delay;
 int64_t max_advance;
 
-/* vcpu throttling controls */
-static QEMUTimer *throttle_timer;
-static unsigned int throttle_percentage;
-
-#define CPU_THROTTLE_PCT_MIN 1
-#define CPU_THROTTLE_PCT_MAX 99
-#define CPU_THROTTLE_TIMESLICE_NS 10000000
-
 bool cpu_is_stopped(CPUState *cpu)
 {
     return cpu->stopped || !runstate_is_running();
@@ -738,90 +732,12 @@ static const VMStateDescription vmstate_timers = {
     }
 };
 
-static void cpu_throttle_thread(CPUState *cpu, run_on_cpu_data opaque)
-{
-    double pct;
-    double throttle_ratio;
-    int64_t sleeptime_ns, endtime_ns;
-
-    if (!cpu_throttle_get_percentage()) {
-        return;
-    }
-
-    pct = (double)cpu_throttle_get_percentage()/100;
-    throttle_ratio = pct / (1 - pct);
-    /* Add 1ns to fix double's rounding error (like 0.9999999...) */
-    sleeptime_ns = (int64_t)(throttle_ratio * CPU_THROTTLE_TIMESLICE_NS + 1);
-    endtime_ns = qemu_clock_get_ns(QEMU_CLOCK_REALTIME) + sleeptime_ns;
-    while (sleeptime_ns > 0 && !cpu->stop) {
-        if (sleeptime_ns > SCALE_MS) {
-            qemu_cond_timedwait(cpu->halt_cond, &qemu_global_mutex,
-                                sleeptime_ns / SCALE_MS);
-        } else {
-            qemu_mutex_unlock_iothread();
-            g_usleep(sleeptime_ns / SCALE_US);
-            qemu_mutex_lock_iothread();
-        }
-        sleeptime_ns = endtime_ns - qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
-    }
-    atomic_set(&cpu->throttle_thread_scheduled, 0);
-}
-
-static void cpu_throttle_timer_tick(void *opaque)
-{
-    CPUState *cpu;
-    double pct;
-
-    /* Stop the timer if needed */
-    if (!cpu_throttle_get_percentage()) {
-        return;
-    }
-    CPU_FOREACH(cpu) {
-        if (!atomic_xchg(&cpu->throttle_thread_scheduled, 1)) {
-            async_run_on_cpu(cpu, cpu_throttle_thread,
-                             RUN_ON_CPU_NULL);
-        }
-    }
-
-    pct = (double)cpu_throttle_get_percentage()/100;
-    timer_mod(throttle_timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL_RT) +
-                                   CPU_THROTTLE_TIMESLICE_NS / (1-pct));
-}
-
-void cpu_throttle_set(int new_throttle_pct)
-{
-    /* Ensure throttle percentage is within valid range */
-    new_throttle_pct = MIN(new_throttle_pct, CPU_THROTTLE_PCT_MAX);
-    new_throttle_pct = MAX(new_throttle_pct, CPU_THROTTLE_PCT_MIN);
-
-    atomic_set(&throttle_percentage, new_throttle_pct);
-
-    timer_mod(throttle_timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL_RT) +
-                                       CPU_THROTTLE_TIMESLICE_NS);
-}
-
-void cpu_throttle_stop(void)
-{
-    atomic_set(&throttle_percentage, 0);
-}
-
-bool cpu_throttle_active(void)
-{
-    return (cpu_throttle_get_percentage() != 0);
-}
-
-int cpu_throttle_get_percentage(void)
-{
-    return atomic_read(&throttle_percentage);
-}
-
 void cpu_ticks_init(void)
 {
     seqlock_init(&timers_state.vm_clock_seqlock);
     qemu_spin_init(&timers_state.vm_clock_lock);
     vmstate_register(NULL, 0, &vmstate_timers, &timers_state);
-    throttle_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL_RT,
-                                           cpu_throttle_timer_tick, NULL);
+    cpu_throttle_init();
 }
 
 void configure_icount(QemuOpts *opts, Error **errp)
@@ -1891,6 +1807,11 @@ void qemu_cond_wait_iothread(QemuCond *cond)
     qemu_cond_wait(cond, &qemu_global_mutex);
 }
 
+void qemu_cond_timedwait_iothread(QemuCond *cond, int ms)
+{
+    qemu_cond_timedwait(cond, &qemu_global_mutex, ms);
+}
+
 static bool all_vcpus_paused(void)
 {
     CPUState *cpu;
-- 
2.16.4



^ permalink raw reply related	[flat|nested] 37+ messages in thread

* [PATCH 3/3] cpu-timers, icount: new modules
  2020-06-29  9:35 [PATCH 0/3] QEMU cpus.c refactoring part1 Claudio Fontana
  2020-06-29  9:35 ` [PATCH 1/3] softmmu: move softmmu only files from root Claudio Fontana
  2020-06-29  9:35 ` [PATCH 2/3] cpu-throttle: new module, extracted from cpus.c Claudio Fontana
@ 2020-06-29  9:35 ` Claudio Fontana
  2020-07-08 14:34   ` Paolo Bonzini
  2020-07-02  6:27 ` [PATCH 0/3] QEMU cpus.c refactoring part1 Claudio Fontana
  3 siblings, 1 reply; 37+ messages in thread
From: Claudio Fontana @ 2020-06-29  9:35 UTC (permalink / raw)
  To: Paolo Bonzini, Thomas Huth, Laurent Vivier, Alex Bennée,
	Peter Maydell, Philippe Mathieu-Daudé,
	Roman Bolshakov, Markus Armbruster
  Cc: Eduardo Habkost, Marcelo Tosatti, qemu-devel, haxm-team,
	Wenchao Wang, Sunil Muthuswamy, Richard Henderson,
	Claudio Fontana, Colin Xu

refactoring of cpus.c continues with cpu timer state extraction.

cpu-timers: responsible for the cpu timers state, and for access to
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 misuses icount to warp time
(qtest_clock_warp). In order to solve this problem, detach instead qtest
from icount, and use a trivial separate counter for it.

This requires fixing assumptions scattered in the code that
qtest_enabled() implies icount_enabled().

No functionality change.

Signed-off-by: Claudio Fontana <cfontana@suse.de>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
---
 MAINTAINERS                  |   2 +
 accel/qtest.c                |   6 +-
 accel/tcg/cpu-exec.c         |  43 ++-
 accel/tcg/tcg-all.c          |   7 +-
 accel/tcg/translate-all.c    |   3 +-
 dma-helpers.c                |   4 +-
 docs/replay.txt              |   6 +-
 exec.c                       |   4 -
 hw/core/ptimer.c             |   8 +-
 hw/i386/x86.c                |   1 +
 include/exec/cpu-all.h       |   4 +
 include/exec/exec-all.h      |   4 +-
 include/qemu/timer.h         |  22 +-
 include/sysemu/cpu-timers.h  |  81 +++++
 include/sysemu/cpus.h        |  12 +-
 include/sysemu/qtest.h       |   2 +
 include/sysemu/replay.h      |   4 +-
 replay/replay.c              |   6 +-
 softmmu/Makefile.objs        |   2 +
 softmmu/cpu-timers.c         | 284 ++++++++++++++++
 softmmu/cpus.c               | 750 +------------------------------------------
 softmmu/icount.c             | 499 ++++++++++++++++++++++++++++
 softmmu/qtest.c              |  34 +-
 softmmu/timers-state.h       |  69 ++++
 softmmu/vl.c                 |   8 +-
 stubs/Makefile.objs          |   3 +-
 stubs/clock-warp.c           |   4 +-
 stubs/cpu-get-clock.c        |   3 +-
 stubs/cpu-get-icount.c       |  21 --
 stubs/icount.c               |  22 ++
 stubs/qemu-timer-notify-cb.c |   8 +
 stubs/qtest.c                |   5 +
 target/alpha/translate.c     |   3 +-
 target/arm/helper.c          |   7 +-
 target/riscv/csr.c           |   8 +-
 tests/ptimer-test-stubs.c    |   7 +-
 tests/test-timed-average.c   |   2 +-
 util/main-loop.c             |   4 +-
 util/qemu-timer.c            |  12 +-
 39 files changed, 1121 insertions(+), 853 deletions(-)
 create mode 100644 include/sysemu/cpu-timers.h
 create mode 100644 softmmu/cpu-timers.c
 create mode 100644 softmmu/icount.c
 create mode 100644 softmmu/timers-state.h
 delete mode 100644 stubs/cpu-get-icount.c
 create mode 100644 stubs/icount.c
 create mode 100644 stubs/qemu-timer-notify-cb.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 827668fa8a..d2a5f20963 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2222,6 +2222,8 @@ F: softmmu/vl.c
 F: softmmu/main.c
 F: softmmu/cpus.c
 F: softmmu/cpu-throttle.c
+F: softmmu/cpu-timers.c
+F: softmmu/icount.c
 F: qapi/run-state.json
 
 Human Monitor (HMP)
diff --git a/accel/qtest.c b/accel/qtest.c
index 5b88f55921..119d0f16a4 100644
--- a/accel/qtest.c
+++ b/accel/qtest.c
@@ -19,14 +19,10 @@
 #include "sysemu/accel.h"
 #include "sysemu/qtest.h"
 #include "sysemu/cpus.h"
+#include "sysemu/cpu-timers.h"
 
 static int qtest_init_accel(MachineState *ms)
 {
-    QemuOpts *opts = qemu_opts_create(qemu_find_opts("icount"), NULL, 0,
-                                      &error_abort);
-    qemu_opt_set(opts, "shift", "0", &error_abort);
-    configure_icount(opts, &error_abort);
-    qemu_opts_del(opts);
     return 0;
 }
 
diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
index d95c4848a4..82155c1db3 100644
--- a/accel/tcg/cpu-exec.c
+++ b/accel/tcg/cpu-exec.c
@@ -19,6 +19,7 @@
 
 #include "qemu/osdep.h"
 #include "qemu-common.h"
+#include "qemu/qemu-print.h"
 #include "cpu.h"
 #include "trace.h"
 #include "disas/disas.h"
@@ -36,6 +37,8 @@
 #include "hw/i386/apic.h"
 #endif
 #include "sysemu/cpus.h"
+#include "exec/cpu-all.h"
+#include "sysemu/cpu-timers.h"
 #include "sysemu/replay.h"
 
 /* -icount align implementation. */
@@ -56,6 +59,9 @@ typedef struct SyncClocks {
 #define MAX_DELAY_PRINT_RATE 2000000000LL
 #define MAX_NB_PRINTS 100
 
+static int64_t max_delay;
+static int64_t max_advance;
+
 static void align_clocks(SyncClocks *sc, CPUState *cpu)
 {
     int64_t cpu_icount;
@@ -65,7 +71,7 @@ static void align_clocks(SyncClocks *sc, CPUState *cpu)
     }
 
     cpu_icount = cpu->icount_extra + cpu_neg(cpu)->icount_decr.u16.low;
-    sc->diff_clk += cpu_icount_to_ns(sc->last_cpu_icount - cpu_icount);
+    sc->diff_clk += icount_to_ns(sc->last_cpu_icount - cpu_icount);
     sc->last_cpu_icount = cpu_icount;
 
     if (sc->diff_clk > VM_CLOCK_ADVANCE) {
@@ -98,9 +104,9 @@ static void print_delay(const SyncClocks *sc)
             (-sc->diff_clk / (float)1000000000LL <
              (threshold_delay - THRESHOLD_REDUCE))) {
             threshold_delay = (-sc->diff_clk / 1000000000LL) + 1;
-            printf("Warning: The guest is now late by %.1f to %.1f seconds\n",
-                   threshold_delay - 1,
-                   threshold_delay);
+            qemu_printf("Warning: The guest is now late by %.1f to %.1f seconds\n",
+                        threshold_delay - 1,
+                        threshold_delay);
             nb_prints++;
             last_realtime_clock = sc->realtime_clock;
         }
@@ -597,7 +603,7 @@ static inline bool cpu_handle_interrupt(CPUState *cpu,
 
     /* Finally, check if we need to exit to the main loop.  */
     if (unlikely(atomic_read(&cpu->exit_request))
-        || (use_icount
+        || (icount_enabled()
             && cpu_neg(cpu)->icount_decr.u16.low + cpu->icount_extra == 0)) {
         atomic_set(&cpu->exit_request, 0);
         if (cpu->exception_index == -1) {
@@ -638,10 +644,10 @@ static inline void cpu_loop_exec_tb(CPUState *cpu, TranslationBlock *tb,
     }
 
     /* Instruction counter expired.  */
-    assert(use_icount);
+    assert(icount_enabled());
 #ifndef CONFIG_USER_ONLY
     /* Ensure global icount has gone forward */
-    cpu_update_icount(cpu);
+    icount_update(cpu);
     /* Refill decrementer and continue execution.  */
     insns_left = MIN(0xffff, cpu->icount_budget);
     cpu_neg(cpu)->icount_decr.u16.low = insns_left;
@@ -741,3 +747,26 @@ int cpu_exec(CPUState *cpu)
 
     return ret;
 }
+
+#ifndef CONFIG_USER_ONLY
+
+void dump_drift_info(void)
+{
+    if (!icount_enabled()) {
+        return;
+    }
+
+    qemu_printf("Host - Guest clock  %"PRIi64" ms\n",
+                (cpu_get_clock() - icount_get()) / SCALE_MS);
+    if (icount_align_option) {
+        qemu_printf("Max guest delay     %"PRIi64" ms\n",
+                    -max_delay / SCALE_MS);
+        qemu_printf("Max guest advance   %"PRIi64" ms\n",
+                    max_advance / SCALE_MS);
+    } else {
+        qemu_printf("Max guest delay     NA\n");
+        qemu_printf("Max guest advance   NA\n");
+    }
+}
+
+#endif /* !CONFIG_USER_ONLY */
diff --git a/accel/tcg/tcg-all.c b/accel/tcg/tcg-all.c
index 3b4fda5640..e27385d051 100644
--- a/accel/tcg/tcg-all.c
+++ b/accel/tcg/tcg-all.c
@@ -29,6 +29,7 @@
 #include "qom/object.h"
 #include "cpu.h"
 #include "sysemu/cpus.h"
+#include "sysemu/cpu-timers.h"
 #include "qemu/main-loop.h"
 #include "tcg/tcg.h"
 #include "qapi/error.h"
@@ -65,7 +66,7 @@ static void tcg_handle_interrupt(CPUState *cpu, int mask)
         qemu_cpu_kick(cpu);
     } else {
         atomic_set(&cpu_neg(cpu)->icount_decr.u16.high, -1);
-        if (use_icount &&
+        if (icount_enabled() &&
             !cpu->can_do_io
             && (mask & ~old_mask) != 0) {
             cpu_abort(cpu, "Raised interrupt while not in I/O function");
@@ -104,7 +105,7 @@ static bool check_tcg_memory_orders_compatible(void)
 
 static bool default_mttcg_enabled(void)
 {
-    if (use_icount || TCG_OVERSIZED_GUEST) {
+    if (icount_enabled() || TCG_OVERSIZED_GUEST) {
         return false;
     } else {
 #ifdef TARGET_SUPPORTS_MTTCG
@@ -146,7 +147,7 @@ static void tcg_set_thread(Object *obj, const char *value, Error **errp)
     if (strcmp(value, "multi") == 0) {
         if (TCG_OVERSIZED_GUEST) {
             error_setg(errp, "No MTTCG when guest word size > hosts");
-        } else if (use_icount) {
+        } else if (icount_enabled()) {
             error_setg(errp, "No MTTCG when icount is enabled");
         } else {
 #ifndef TARGET_SUPPORTS_MTTCG
diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
index 2afa46bd2b..abd22d8d12 100644
--- a/accel/tcg/translate-all.c
+++ b/accel/tcg/translate-all.c
@@ -57,6 +57,7 @@
 #include "qemu/main-loop.h"
 #include "exec/log.h"
 #include "sysemu/cpus.h"
+#include "sysemu/cpu-timers.h"
 #include "sysemu/tcg.h"
 
 /* #define DEBUG_TB_INVALIDATE */
@@ -369,7 +370,7 @@ static int cpu_restore_state_from_tb(CPUState *cpu, TranslationBlock *tb,
 
  found:
     if (reset_icount && (tb_cflags(tb) & CF_USE_ICOUNT)) {
-        assert(use_icount);
+        assert(icount_enabled());
         /* Reset the cycle counter to the start of the block
            and shift if to the number of actually executed instructions */
         cpu_neg(cpu)->icount_decr.u16.low += num_insns - i;
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) {
             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 21926dc9c7..690446d66f 100644
--- a/exec.c
+++ b/exec.c
@@ -108,10 +108,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()) {
         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()) {
                 period = 10000 / s->delta;
                 period_frac = 0;
             }
diff --git a/hw/i386/x86.c b/hw/i386/x86.c
index 34229b45c7..a6dc5900fe 100644
--- a/hw/i386/x86.c
+++ b/hw/i386/x86.c
@@ -34,6 +34,7 @@
 #include "sysemu/numa.h"
 #include "sysemu/replay.h"
 #include "sysemu/sysemu.h"
+#include "sysemu/cpu-timers.h"
 #include "trace.h"
 
 #include "hw/i386/x86.h"
diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
index fc403d456b..25b6005a91 100644
--- a/include/exec/cpu-all.h
+++ b/include/exec/cpu-all.h
@@ -407,8 +407,12 @@ static inline bool tlb_hit(target_ulong tlb_addr, target_ulong addr)
     return tlb_hit_page(tlb_addr, addr & TARGET_PAGE_MASK);
 }
 
+#ifdef CONFIG_TCG
+void dump_drift_info(void);
 void dump_exec_info(void);
 void dump_opcount_info(void);
+#endif /* CONFIG_TCG */
+
 #endif /* !CONFIG_USER_ONLY */
 
 /* Returns: 0 on success, -1 on error */
diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index 3cf88272df..e019b505a5 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -25,7 +25,7 @@
 #ifdef CONFIG_TCG
 #include "exec/cpu_ldst.h"
 #endif
-#include "sysemu/cpus.h"
+#include "sysemu/cpu-timers.h"
 
 /* allow to see translation results - the slowdown should be negligible, so we leave it */
 #define DEBUG_DISAS
@@ -497,7 +497,7 @@ static inline uint32_t tb_cflags(const TranslationBlock *tb)
 static inline uint32_t curr_cflags(void)
 {
     return (parallel_cpus ? CF_PARALLEL : 0)
-         | (use_icount ? CF_USE_ICOUNT : 0);
+         | (icount_enabled() ? CF_USE_ICOUNT : 0);
 }
 
 /* TranslationBlock invalidate API */
diff --git a/include/qemu/timer.h b/include/qemu/timer.h
index 6a8b48b5a9..5336c756d5 100644
--- a/include/qemu/timer.h
+++ b/include/qemu/timer.h
@@ -166,7 +166,7 @@ bool qemu_clock_expired(QEMUClockType type);
  *
  * Determine whether a clock should be used for deadline
  * calculations. Some clocks, for instance vm_clock with
- * use_icount set, do not count in nanoseconds. Such clocks
+ * icount_enabled() set, do not count in nanoseconds. Such clocks
  * are not used for deadline calculations, and are presumed
  * to interrupt any poll using qemu_notify/aio_notify
  * etc.
@@ -224,13 +224,6 @@ void qemu_clock_notify(QEMUClockType type);
  */
 void qemu_clock_enable(QEMUClockType type, bool enabled);
 
-/**
- * qemu_start_warp_timer:
- *
- * Starts a timer for virtual clock update
- */
-void qemu_start_warp_timer(void);
-
 /**
  * qemu_clock_run_timers:
  * @type: clock on which to operate
@@ -791,12 +784,6 @@ static inline int64_t qemu_soonest_timeout(int64_t timeout1, int64_t timeout2)
  */
 void init_clocks(QEMUTimerListNotifyCB *notify_cb);
 
-int64_t cpu_get_ticks(void);
-/* Caller must hold BQL */
-void cpu_enable_ticks(void);
-/* Caller must hold BQL */
-void cpu_disable_ticks(void);
-
 static inline int64_t get_max_clock_jump(void)
 {
     /* This should be small enough to prevent excessive interrupts from being
@@ -850,13 +837,6 @@ static inline int64_t get_clock(void)
 }
 #endif
 
-/* icount */
-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) */
 
diff --git a/include/sysemu/cpu-timers.h b/include/sysemu/cpu-timers.h
new file mode 100644
index 0000000000..07d724672f
--- /dev/null
+++ b/include/sysemu/cpu-timers.h
@@ -0,0 +1,81 @@
+/*
+ * CPU timers state API
+ *
+ * Copyright 2020 SUSE LLC
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ */
+#ifndef SYSEMU_CPU_TIMERS_H
+#define SYSEMU_CPU_TIMERS_H
+
+#include "qemu/timer.h"
+
+/* init the whole cpu timers API, including icount, ticks, and cpu_throttle */
+void cpu_timers_init(void);
+
+/* icount - Instruction Counter API */
+
+/*
+ * Return the icount enablement state:
+ *
+ * 0 = Disabled - Do not count executed instructions.
+ * 1 = Enabled - Fixed conversion of insn to ns via "shift" option
+ * 2 = Enabled - Runtime adaptive algorithm to compute shift
+ */
+int icount_enabled(void);
+/*
+ * Update the icount with the executed instructions. Called by
+ * cpus-tcg vCPU thread so the main-loop can see time has moved forward.
+ */
+void icount_update(CPUState *cpu);
+
+/* get raw icount value */
+int64_t icount_get_raw(void);
+
+/* return the virtual CPU time in ns, based on the instruction counter. */
+int64_t icount_get(void);
+/*
+ * convert an instruction counter value to ns, based on the icount shift.
+ * This shift is set as a fixed value with the icount "shift" option
+ * (precise mode), or it is constantly approximated and corrected at
+ * runtime in adaptive mode.
+ */
+int64_t icount_to_ns(int64_t icount);
+
+/* configure the icount options, including "shift" */
+void icount_configure(QemuOpts *opts, Error **errp);
+
+/* used by tcg vcpu thread to calc icount budget */
+int64_t icount_round(int64_t count);
+
+/* if the CPUs are idle, start accounting real time to virtual clock. */
+void icount_start_warp_timer(void);
+void icount_account_warp_timer(void);
+
+/*
+ * CPU Ticks and Clock
+ */
+
+/* Caller must hold BQL */
+void cpu_enable_ticks(void);
+/* Caller must hold BQL */
+void cpu_disable_ticks(void);
+
+/*
+ * return the time elapsed in VM between vm_start and vm_stop.  Unless
+ * icount is active, cpu_get_ticks() uses units of the host CPU cycle
+ * counter.
+ */
+int64_t cpu_get_ticks(void);
+
+/*
+ * Returns the monotonic time elapsed in VM, i.e.,
+ * the time between vm_start and vm_stop
+ */
+int64_t cpu_get_clock(void);
+
+void qemu_timer_notify_cb(void *opaque, QEMUClockType type);
+
+#endif /* SYSEMU_CPU_TIMERS_H */
diff --git a/include/sysemu/cpus.h b/include/sysemu/cpus.h
index 3c1da6a018..149de000a0 100644
--- a/include/sysemu/cpus.h
+++ b/include/sysemu/cpus.h
@@ -4,33 +4,23 @@
 #include "qemu/timer.h"
 
 /* cpus.c */
+bool all_cpu_threads_idle(void);
 bool qemu_in_vcpu_thread(void);
 void qemu_init_cpu_loop(void);
 void resume_all_vcpus(void);
 void pause_all_vcpus(void);
 void cpu_stop_current(void);
-void cpu_ticks_init(void);
 
-void configure_icount(QemuOpts *opts, Error **errp);
-extern int use_icount;
 extern int icount_align_option;
 
-/* drift information for info jit command */
-extern int64_t max_delay;
-extern int64_t max_advance;
-void dump_drift_info(void);
-
 /* Unblock cpu */
 void qemu_cpu_kick_self(void);
-void qemu_timer_notify_cb(void *opaque, QEMUClockType type);
 
 void cpu_synchronize_all_states(void);
 void cpu_synchronize_all_post_reset(void);
 void cpu_synchronize_all_post_init(void);
 void cpu_synchronize_all_pre_loadvm(void);
 
-void qtest_clock_warp(int64_t dest);
-
 #ifndef CONFIG_USER_ONLY
 /* vl.c */
 /* *-user doesn't have configurable SMP topology */
diff --git a/include/sysemu/qtest.h b/include/sysemu/qtest.h
index eedd3664f0..91acc98ac4 100644
--- a/include/sysemu/qtest.h
+++ b/include/sysemu/qtest.h
@@ -30,4 +30,6 @@ void qtest_server_set_send_handler(void (*send)(void *, const char *),
                                  void *opaque);
 void qtest_server_inproc_recv(void *opaque, const char *buf);
 
+int64_t qtest_get_clock(void);
+
 #endif
diff --git a/include/sysemu/replay.h b/include/sysemu/replay.h
index 5471bb514d..a140d69a73 100644
--- a/include/sysemu/replay.h
+++ b/include/sysemu/replay.h
@@ -109,12 +109,12 @@ int64_t replay_read_clock(ReplayClockKind kind);
 #define REPLAY_CLOCK(clock, value)                                      \
     (replay_mode == REPLAY_MODE_PLAY ? replay_read_clock((clock))       \
         : replay_mode == REPLAY_MODE_RECORD                             \
-            ? replay_save_clock((clock), (value), cpu_get_icount_raw()) \
+            ? replay_save_clock((clock), (value), icount_get_raw()) \
         : (value))
 #define REPLAY_CLOCK_LOCKED(clock, value)                               \
     (replay_mode == REPLAY_MODE_PLAY ? replay_read_clock((clock))       \
         : replay_mode == REPLAY_MODE_RECORD                             \
-            ? replay_save_clock((clock), (value), cpu_get_icount_raw_locked()) \
+            ? replay_save_clock((clock), (value), icount_get_raw_locked()) \
         : (value))
 
 /* Processing data from random generators */
diff --git a/replay/replay.c b/replay/replay.c
index 83ed9e0e24..4c1457b07e 100644
--- a/replay/replay.c
+++ b/replay/replay.c
@@ -11,10 +11,10 @@
 
 #include "qemu/osdep.h"
 #include "qapi/error.h"
+#include "sysemu/cpu-timers.h"
 #include "sysemu/replay.h"
 #include "sysemu/runstate.h"
 #include "replay-internal.h"
-#include "qemu/timer.h"
 #include "qemu/main-loop.h"
 #include "qemu/option.h"
 #include "sysemu/cpus.h"
@@ -64,7 +64,7 @@ bool replay_next_event_is(int event)
 
 uint64_t replay_get_current_icount(void)
 {
-    return cpu_get_icount_raw();
+    return icount_get_raw();
 }
 
 int replay_get_instructions(void)
@@ -345,7 +345,7 @@ void replay_start(void)
         error_reportf_err(replay_blockers->data, "Record/replay: ");
         exit(1);
     }
-    if (!use_icount) {
+    if (!icount_enabled()) {
         error_report("Please enable icount to use record/replay");
         exit(1);
     }
diff --git a/softmmu/Makefile.objs b/softmmu/Makefile.objs
index a414a74c50..9c0125f37b 100644
--- a/softmmu/Makefile.objs
+++ b/softmmu/Makefile.objs
@@ -7,6 +7,8 @@ obj-y += balloon.o
 obj-y += ioport.o
 obj-y += memory.o
 obj-y += memory_mapping.o
+obj-y += cpu-timers.o
+obj-$(CONFIG_TCG) += icount.o
 
 obj-y += qtest.o
 
diff --git a/softmmu/cpu-timers.c b/softmmu/cpu-timers.c
new file mode 100644
index 0000000000..64addb315d
--- /dev/null
+++ b/softmmu/cpu-timers.c
@@ -0,0 +1,284 @@
+/*
+ * QEMU System Emulator
+ *
+ * Copyright (c) 2003-2008 Fabrice Bellard
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu-common.h"
+#include "qemu/cutils.h"
+#include "migration/vmstate.h"
+#include "qapi/error.h"
+#include "qemu/error-report.h"
+#include "exec/exec-all.h"
+#include "sysemu/cpus.h"
+#include "sysemu/qtest.h"
+#include "qemu/main-loop.h"
+#include "qemu/option.h"
+#include "qemu/seqlock.h"
+#include "sysemu/replay.h"
+#include "sysemu/runstate.h"
+#include "hw/core/cpu.h"
+#include "sysemu/cpu-timers.h"
+#include "sysemu/cpu-throttle.h"
+#include "timers-state.h"
+
+/* clock and ticks */
+
+static int64_t cpu_get_ticks_locked(void)
+{
+    int64_t ticks = timers_state.cpu_ticks_offset;
+    if (timers_state.cpu_ticks_enabled) {
+        ticks += cpu_get_host_ticks();
+    }
+
+    if (timers_state.cpu_ticks_prev > ticks) {
+        /* Non increasing ticks may happen if the host uses software suspend. */
+        timers_state.cpu_ticks_offset += timers_state.cpu_ticks_prev - ticks;
+        ticks = timers_state.cpu_ticks_prev;
+    }
+
+    timers_state.cpu_ticks_prev = ticks;
+    return ticks;
+}
+
+/*
+ * return the time elapsed in VM between vm_start and vm_stop.  Unless
+ * icount is active, cpu_get_ticks() uses units of the host CPU cycle
+ * counter.
+ */
+int64_t cpu_get_ticks(void)
+{
+    int64_t ticks;
+
+    if (icount_enabled()) {
+        return icount_get();
+    }
+
+    qemu_spin_lock(&timers_state.vm_clock_lock);
+    ticks = cpu_get_ticks_locked();
+    qemu_spin_unlock(&timers_state.vm_clock_lock);
+    return ticks;
+}
+
+int64_t cpu_get_clock_locked(void)
+{
+    int64_t time;
+
+    time = timers_state.cpu_clock_offset;
+    if (timers_state.cpu_ticks_enabled) {
+        time += get_clock();
+    }
+
+    return time;
+}
+
+/*
+ * Return the monotonic time elapsed in VM, i.e.,
+ * the time between vm_start and vm_stop
+ */
+int64_t cpu_get_clock(void)
+{
+    int64_t ti;
+    unsigned start;
+
+    do {
+        start = seqlock_read_begin(&timers_state.vm_clock_seqlock);
+        ti = cpu_get_clock_locked();
+    } while (seqlock_read_retry(&timers_state.vm_clock_seqlock, start));
+
+    return ti;
+}
+
+/*
+ * enable cpu_get_ticks()
+ * Caller must hold BQL which serves as mutex for vm_clock_seqlock.
+ */
+void cpu_enable_ticks(void)
+{
+    seqlock_write_lock(&timers_state.vm_clock_seqlock,
+                       &timers_state.vm_clock_lock);
+    if (!timers_state.cpu_ticks_enabled) {
+        timers_state.cpu_ticks_offset -= cpu_get_host_ticks();
+        timers_state.cpu_clock_offset -= get_clock();
+        timers_state.cpu_ticks_enabled = 1;
+    }
+    seqlock_write_unlock(&timers_state.vm_clock_seqlock,
+                       &timers_state.vm_clock_lock);
+}
+
+/*
+ * disable cpu_get_ticks() : the clock is stopped. You must not call
+ * cpu_get_ticks() after that.
+ * Caller must hold BQL which serves as mutex for vm_clock_seqlock.
+ */
+void cpu_disable_ticks(void)
+{
+    seqlock_write_lock(&timers_state.vm_clock_seqlock,
+                       &timers_state.vm_clock_lock);
+    if (timers_state.cpu_ticks_enabled) {
+        timers_state.cpu_ticks_offset += cpu_get_host_ticks();
+        timers_state.cpu_clock_offset = cpu_get_clock_locked();
+        timers_state.cpu_ticks_enabled = 0;
+    }
+    seqlock_write_unlock(&timers_state.vm_clock_seqlock,
+                         &timers_state.vm_clock_lock);
+}
+
+static bool icount_state_needed(void *opaque)
+{
+    return icount_enabled();
+}
+
+static bool icount_shift_state_needed(void *opaque)
+{
+    return icount_enabled() == 2;
+}
+
+static bool warp_timer_state_needed(void *opaque)
+{
+    TimersState *s = opaque;
+    return s->icount_warp_timer != NULL;
+}
+
+static bool adjust_timers_state_needed(void *opaque)
+{
+    TimersState *s = opaque;
+    return s->icount_rt_timer != NULL;
+}
+
+/*
+ * Subsection for warp timer migration is optional, because may not be created
+ */
+static const VMStateDescription icount_vmstate_warp_timer = {
+    .name = "timer/icount/warp_timer",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = warp_timer_state_needed,
+    .fields = (VMStateField[]) {
+        VMSTATE_INT64(vm_clock_warp_start, TimersState),
+        VMSTATE_TIMER_PTR(icount_warp_timer, TimersState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static const VMStateDescription icount_vmstate_adjust_timers = {
+    .name = "timer/icount/timers",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = adjust_timers_state_needed,
+    .fields = (VMStateField[]) {
+        VMSTATE_TIMER_PTR(icount_rt_timer, TimersState),
+        VMSTATE_TIMER_PTR(icount_vm_timer, TimersState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static const VMStateDescription icount_vmstate_shift = {
+    .name = "timer/icount/shift",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = icount_shift_state_needed,
+    .fields = (VMStateField[]) {
+        VMSTATE_INT16(icount_time_shift, TimersState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+/*
+ * This is a subsection for icount migration.
+ */
+static const VMStateDescription icount_vmstate_timers = {
+    .name = "timer/icount",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = icount_state_needed,
+    .fields = (VMStateField[]) {
+        VMSTATE_INT64(qemu_icount_bias, TimersState),
+        VMSTATE_INT64(qemu_icount, TimersState),
+        VMSTATE_END_OF_LIST()
+    },
+    .subsections = (const VMStateDescription * []) {
+        &icount_vmstate_warp_timer,
+        &icount_vmstate_adjust_timers,
+        &icount_vmstate_shift,
+        NULL
+    }
+};
+
+static const VMStateDescription vmstate_timers = {
+    .name = "timer",
+    .version_id = 2,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_INT64(cpu_ticks_offset, TimersState),
+        VMSTATE_UNUSED(8),
+        VMSTATE_INT64_V(cpu_clock_offset, TimersState, 2),
+        VMSTATE_END_OF_LIST()
+    },
+    .subsections = (const VMStateDescription * []) {
+        &icount_vmstate_timers,
+        NULL
+    }
+};
+
+static void do_nothing(CPUState *cpu, run_on_cpu_data unused)
+{
+}
+
+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);
+    }
+}
+
+TimersState timers_state;
+
+/* initialize timers state and the cpu throttle for convenience */
+void cpu_timers_init(void)
+{
+    seqlock_init(&timers_state.vm_clock_seqlock);
+    qemu_spin_init(&timers_state.vm_clock_lock);
+    vmstate_register(NULL, 0, &vmstate_timers, &timers_state);
+
+    cpu_throttle_init();
+}
diff --git a/softmmu/cpus.c b/softmmu/cpus.c
index f446536b88..a44960cd70 100644
--- a/softmmu/cpus.c
+++ b/softmmu/cpus.c
@@ -58,11 +58,10 @@
 #include "hw/nmi.h"
 #include "sysemu/replay.h"
 #include "sysemu/runstate.h"
+#include "sysemu/cpu-timers.h"
 #include "hw/boards.h"
 #include "hw/hw.h"
 
-#include "sysemu/cpu-throttle.h"
-
 #ifdef CONFIG_LINUX
 
 #include <sys/prctl.h>
@@ -83,9 +82,6 @@
 
 static QemuMutex qemu_global_mutex;
 
-int64_t max_delay;
-int64_t max_advance;
-
 bool cpu_is_stopped(CPUState *cpu)
 {
     return cpu->stopped || !runstate_is_running();
@@ -116,7 +112,7 @@ static bool cpu_thread_is_idle(CPUState *cpu)
     return true;
 }
 
-static bool all_cpu_threads_idle(void)
+bool all_cpu_threads_idle(void)
 {
     CPUState *cpu;
 
@@ -128,688 +124,9 @@ static bool all_cpu_threads_idle(void)
     return true;
 }
 
-/***********************************************************/
-/* guest cycle counter */
-
-/* Protected by TimersState seqlock */
-
-static bool icount_sleep = true;
-/* Arbitrarily pick 1MIPS as the minimum allowable speed.  */
-#define MAX_ICOUNT_SHIFT 10
-
-typedef struct TimersState {
-    /* Protected by BQL.  */
-    int64_t cpu_ticks_prev;
-    int64_t cpu_ticks_offset;
-
-    /* Protect fields that can be respectively read outside the
-     * BQL, and written from multiple threads.
-     */
-    QemuSeqLock vm_clock_seqlock;
-    QemuSpin vm_clock_lock;
-
-    int16_t cpu_ticks_enabled;
-
-    /* Conversion factor from emulated instructions to virtual clock ticks.  */
-    int16_t icount_time_shift;
-
-    /* Compensate for varying guest execution speed.  */
-    int64_t qemu_icount_bias;
-
-    int64_t vm_clock_warp_start;
-    int64_t cpu_clock_offset;
-
-    /* Only written by TCG thread */
-    int64_t qemu_icount;
-
-    /* for adjusting icount */
-    QEMUTimer *icount_rt_timer;
-    QEMUTimer *icount_vm_timer;
-    QEMUTimer *icount_warp_timer;
-} TimersState;
-
-static TimersState timers_state;
 bool mttcg_enabled;
 
 
-/* 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_neg(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.
- */
-static void cpu_update_icount_locked(CPUState *cpu)
-{
-    int64_t executed = cpu_get_icount_executed(cpu);
-    cpu->icount_budget -= executed;
-
-    atomic_set_i64(&timers_state.qemu_icount,
-                   timers_state.qemu_icount + executed);
-}
-
-/*
- * 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)
-{
-    seqlock_write_lock(&timers_state.vm_clock_seqlock,
-                       &timers_state.vm_clock_lock);
-    cpu_update_icount_locked(cpu);
-    seqlock_write_unlock(&timers_state.vm_clock_seqlock,
-                         &timers_state.vm_clock_lock);
-}
-
-static int64_t cpu_get_icount_raw_locked(void)
-{
-    CPUState *cpu = current_cpu;
-
-    if (cpu && cpu->running) {
-        if (!cpu->can_do_io) {
-            error_report("Bad icount read");
-            exit(1);
-        }
-        /* Take into account what has run */
-        cpu_update_icount_locked(cpu);
-    }
-    /* The read is protected by the seqlock, but needs atomic64 to avoid UB */
-    return atomic_read_i64(&timers_state.qemu_icount);
-}
-
-static int64_t cpu_get_icount_locked(void)
-{
-    int64_t icount = cpu_get_icount_raw_locked();
-    return atomic_read_i64(&timers_state.qemu_icount_bias) +
-        cpu_icount_to_ns(icount);
-}
-
-int64_t cpu_get_icount_raw(void)
-{
-    int64_t icount;
-    unsigned start;
-
-    do {
-        start = seqlock_read_begin(&timers_state.vm_clock_seqlock);
-        icount = cpu_get_icount_raw_locked();
-    } while (seqlock_read_retry(&timers_state.vm_clock_seqlock, start));
-
-    return icount;
-}
-
-/* Return the virtual CPU time, based on the instruction counter.  */
-int64_t cpu_get_icount(void)
-{
-    int64_t icount;
-    unsigned start;
-
-    do {
-        start = seqlock_read_begin(&timers_state.vm_clock_seqlock);
-        icount = cpu_get_icount_locked();
-    } while (seqlock_read_retry(&timers_state.vm_clock_seqlock, start));
-
-    return icount;
-}
-
-int64_t cpu_icount_to_ns(int64_t icount)
-{
-    return icount << atomic_read(&timers_state.icount_time_shift);
-}
-
-static int64_t cpu_get_ticks_locked(void)
-{
-    int64_t ticks = timers_state.cpu_ticks_offset;
-    if (timers_state.cpu_ticks_enabled) {
-        ticks += cpu_get_host_ticks();
-    }
-
-    if (timers_state.cpu_ticks_prev > ticks) {
-        /* Non increasing ticks may happen if the host uses software suspend.  */
-        timers_state.cpu_ticks_offset += timers_state.cpu_ticks_prev - ticks;
-        ticks = timers_state.cpu_ticks_prev;
-    }
-
-    timers_state.cpu_ticks_prev = ticks;
-    return ticks;
-}
-
-/* return the time elapsed in VM between vm_start and vm_stop.  Unless
- * icount is active, cpu_get_ticks() uses units of the host CPU cycle
- * counter.
- */
-int64_t cpu_get_ticks(void)
-{
-    int64_t ticks;
-
-    if (use_icount) {
-        return cpu_get_icount();
-    }
-
-    qemu_spin_lock(&timers_state.vm_clock_lock);
-    ticks = cpu_get_ticks_locked();
-    qemu_spin_unlock(&timers_state.vm_clock_lock);
-    return ticks;
-}
-
-static int64_t cpu_get_clock_locked(void)
-{
-    int64_t time;
-
-    time = timers_state.cpu_clock_offset;
-    if (timers_state.cpu_ticks_enabled) {
-        time += get_clock();
-    }
-
-    return time;
-}
-
-/* Return the monotonic time elapsed in VM, i.e.,
- * the time between vm_start and vm_stop
- */
-int64_t cpu_get_clock(void)
-{
-    int64_t ti;
-    unsigned start;
-
-    do {
-        start = seqlock_read_begin(&timers_state.vm_clock_seqlock);
-        ti = cpu_get_clock_locked();
-    } while (seqlock_read_retry(&timers_state.vm_clock_seqlock, start));
-
-    return ti;
-}
-
-/* enable cpu_get_ticks()
- * Caller must hold BQL which serves as mutex for vm_clock_seqlock.
- */
-void cpu_enable_ticks(void)
-{
-    seqlock_write_lock(&timers_state.vm_clock_seqlock,
-                       &timers_state.vm_clock_lock);
-    if (!timers_state.cpu_ticks_enabled) {
-        timers_state.cpu_ticks_offset -= cpu_get_host_ticks();
-        timers_state.cpu_clock_offset -= get_clock();
-        timers_state.cpu_ticks_enabled = 1;
-    }
-    seqlock_write_unlock(&timers_state.vm_clock_seqlock,
-                       &timers_state.vm_clock_lock);
-}
-
-/* disable cpu_get_ticks() : the clock is stopped. You must not call
- * cpu_get_ticks() after that.
- * Caller must hold BQL which serves as mutex for vm_clock_seqlock.
- */
-void cpu_disable_ticks(void)
-{
-    seqlock_write_lock(&timers_state.vm_clock_seqlock,
-                       &timers_state.vm_clock_lock);
-    if (timers_state.cpu_ticks_enabled) {
-        timers_state.cpu_ticks_offset += cpu_get_host_ticks();
-        timers_state.cpu_clock_offset = cpu_get_clock_locked();
-        timers_state.cpu_ticks_enabled = 0;
-    }
-    seqlock_write_unlock(&timers_state.vm_clock_seqlock,
-                         &timers_state.vm_clock_lock);
-}
-
-/* Correlation between real and virtual time is always going to be
-   fairly approximate, so ignore small variation.
-   When the guest is idle real and virtual time will be aligned in
-   the IO wait loop.  */
-#define ICOUNT_WOBBLE (NANOSECONDS_PER_SECOND / 10)
-
-static void icount_adjust(void)
-{
-    int64_t cur_time;
-    int64_t cur_icount;
-    int64_t delta;
-
-    /* Protected by TimersState mutex.  */
-    static int64_t last_delta;
-
-    /* If the VM is not running, then do nothing.  */
-    if (!runstate_is_running()) {
-        return;
-    }
-
-    seqlock_write_lock(&timers_state.vm_clock_seqlock,
-                       &timers_state.vm_clock_lock);
-    cur_time = REPLAY_CLOCK_LOCKED(REPLAY_CLOCK_VIRTUAL_RT,
-                                   cpu_get_clock_locked());
-    cur_icount = cpu_get_icount_locked();
-
-    delta = cur_icount - cur_time;
-    /* FIXME: This is a very crude algorithm, somewhat prone to oscillation.  */
-    if (delta > 0
-        && last_delta + ICOUNT_WOBBLE < delta * 2
-        && timers_state.icount_time_shift > 0) {
-        /* The guest is getting too far ahead.  Slow time down.  */
-        atomic_set(&timers_state.icount_time_shift,
-                   timers_state.icount_time_shift - 1);
-    }
-    if (delta < 0
-        && last_delta - ICOUNT_WOBBLE > delta * 2
-        && timers_state.icount_time_shift < MAX_ICOUNT_SHIFT) {
-        /* The guest is getting too far behind.  Speed time up.  */
-        atomic_set(&timers_state.icount_time_shift,
-                   timers_state.icount_time_shift + 1);
-    }
-    last_delta = delta;
-    atomic_set_i64(&timers_state.qemu_icount_bias,
-                   cur_icount - (timers_state.qemu_icount
-                                 << timers_state.icount_time_shift));
-    seqlock_write_unlock(&timers_state.vm_clock_seqlock,
-                         &timers_state.vm_clock_lock);
-}
-
-static void icount_adjust_rt(void *opaque)
-{
-    timer_mod(timers_state.icount_rt_timer,
-              qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL_RT) + 1000);
-    icount_adjust();
-}
-
-static void icount_adjust_vm(void *opaque)
-{
-    timer_mod(timers_state.icount_vm_timer,
-                   qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
-                   NANOSECONDS_PER_SECOND / 10);
-    icount_adjust();
-}
-
-static int64_t qemu_icount_round(int64_t count)
-{
-    int shift = atomic_read(&timers_state.icount_time_shift);
-    return (count + (1 << shift) - 1) >> shift;
-}
-
-static void icount_warp_rt(void)
-{
-    unsigned seq;
-    int64_t warp_start;
-
-    /* The icount_warp_timer is rescheduled soon after vm_clock_warp_start
-     * changes from -1 to another value, so the race here is okay.
-     */
-    do {
-        seq = seqlock_read_begin(&timers_state.vm_clock_seqlock);
-        warp_start = timers_state.vm_clock_warp_start;
-    } while (seqlock_read_retry(&timers_state.vm_clock_seqlock, seq));
-
-    if (warp_start == -1) {
-        return;
-    }
-
-    seqlock_write_lock(&timers_state.vm_clock_seqlock,
-                       &timers_state.vm_clock_lock);
-    if (runstate_is_running()) {
-        int64_t clock = REPLAY_CLOCK_LOCKED(REPLAY_CLOCK_VIRTUAL_RT,
-                                            cpu_get_clock_locked());
-        int64_t warp_delta;
-
-        warp_delta = clock - timers_state.vm_clock_warp_start;
-        if (use_icount == 2) {
-            /*
-             * In adaptive mode, do not let QEMU_CLOCK_VIRTUAL run too
-             * far ahead of real time.
-             */
-            int64_t cur_icount = cpu_get_icount_locked();
-            int64_t delta = clock - cur_icount;
-            warp_delta = MIN(warp_delta, delta);
-        }
-        atomic_set_i64(&timers_state.qemu_icount_bias,
-                       timers_state.qemu_icount_bias + warp_delta);
-    }
-    timers_state.vm_clock_warp_start = -1;
-    seqlock_write_unlock(&timers_state.vm_clock_seqlock,
-                       &timers_state.vm_clock_lock);
-
-    if (qemu_clock_expired(QEMU_CLOCK_VIRTUAL)) {
-        qemu_clock_notify(QEMU_CLOCK_VIRTUAL);
-    }
-}
-
-static void icount_timer_cb(void *opaque)
-{
-    /* No need for a checkpoint because the timer already synchronizes
-     * with CHECKPOINT_CLOCK_VIRTUAL_RT.
-     */
-    icount_warp_rt();
-}
-
-void qtest_clock_warp(int64_t dest)
-{
-    int64_t clock = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
-    AioContext *aio_context;
-    assert(qtest_enabled());
-    aio_context = qemu_get_aio_context();
-    while (clock < dest) {
-        int64_t deadline = qemu_clock_deadline_ns_all(QEMU_CLOCK_VIRTUAL,
-                                                      QEMU_TIMER_ATTR_ALL);
-        int64_t warp = qemu_soonest_timeout(dest - clock, deadline);
-
-        seqlock_write_lock(&timers_state.vm_clock_seqlock,
-                           &timers_state.vm_clock_lock);
-        atomic_set_i64(&timers_state.qemu_icount_bias,
-                       timers_state.qemu_icount_bias + warp);
-        seqlock_write_unlock(&timers_state.vm_clock_seqlock,
-                             &timers_state.vm_clock_lock);
-
-        qemu_clock_run_timers(QEMU_CLOCK_VIRTUAL);
-        timerlist_run_timers(aio_context->tlg.tl[QEMU_CLOCK_VIRTUAL]);
-        clock = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
-    }
-    qemu_clock_notify(QEMU_CLOCK_VIRTUAL);
-}
-
-void qemu_start_warp_timer(void)
-{
-    int64_t clock;
-    int64_t deadline;
-
-    if (!use_icount) {
-        return;
-    }
-
-    /* Nothing to do if the VM is stopped: QEMU_CLOCK_VIRTUAL timers
-     * do not fire, so computing the deadline does not make sense.
-     */
-    if (!runstate_is_running()) {
-        return;
-    }
-
-    if (replay_mode != REPLAY_MODE_PLAY) {
-        if (!all_cpu_threads_idle()) {
-            return;
-        }
-
-        if (qtest_enabled()) {
-            /* When testing, qtest commands advance icount.  */
-            return;
-        }
-
-        replay_checkpoint(CHECKPOINT_CLOCK_WARP_START);
-    } else {
-        /* warp clock deterministically in record/replay mode */
-        if (!replay_checkpoint(CHECKPOINT_CLOCK_WARP_START)) {
-            /* vCPU is sleeping and warp can't be started.
-               It is probably a race condition: notification sent
-               to vCPU was processed in advance and vCPU went to sleep.
-               Therefore we have to wake it up for doing someting. */
-            if (replay_has_checkpoint()) {
-                qemu_clock_notify(QEMU_CLOCK_VIRTUAL);
-            }
-            return;
-        }
-    }
-
-    /* We want to use the earliest deadline from ALL vm_clocks */
-    clock = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL_RT);
-    deadline = qemu_clock_deadline_ns_all(QEMU_CLOCK_VIRTUAL,
-                                          ~QEMU_TIMER_ATTR_EXTERNAL);
-    if (deadline < 0) {
-        static bool notified;
-        if (!icount_sleep && !notified) {
-            warn_report("icount sleep disabled and no active timers");
-            notified = true;
-        }
-        return;
-    }
-
-    if (deadline > 0) {
-        /*
-         * Ensure QEMU_CLOCK_VIRTUAL proceeds even when the virtual CPU goes to
-         * sleep.  Otherwise, the CPU might be waiting for a future timer
-         * interrupt to wake it up, but the interrupt never comes because
-         * the vCPU isn't running any insns and thus doesn't advance the
-         * QEMU_CLOCK_VIRTUAL.
-         */
-        if (!icount_sleep) {
-            /*
-             * We never let VCPUs sleep in no sleep icount mode.
-             * If there is a pending QEMU_CLOCK_VIRTUAL timer we just advance
-             * to the next QEMU_CLOCK_VIRTUAL event and notify it.
-             * It is useful when we want a deterministic execution time,
-             * isolated from host latencies.
-             */
-            seqlock_write_lock(&timers_state.vm_clock_seqlock,
-                               &timers_state.vm_clock_lock);
-            atomic_set_i64(&timers_state.qemu_icount_bias,
-                           timers_state.qemu_icount_bias + deadline);
-            seqlock_write_unlock(&timers_state.vm_clock_seqlock,
-                                 &timers_state.vm_clock_lock);
-            qemu_clock_notify(QEMU_CLOCK_VIRTUAL);
-        } else {
-            /*
-             * We do stop VCPUs and only advance QEMU_CLOCK_VIRTUAL after some
-             * "real" time, (related to the time left until the next event) has
-             * passed. The QEMU_CLOCK_VIRTUAL_RT clock will do this.
-             * This avoids that the warps are visible externally; for example,
-             * you will not be sending network packets continuously instead of
-             * every 100ms.
-             */
-            seqlock_write_lock(&timers_state.vm_clock_seqlock,
-                               &timers_state.vm_clock_lock);
-            if (timers_state.vm_clock_warp_start == -1
-                || timers_state.vm_clock_warp_start > clock) {
-                timers_state.vm_clock_warp_start = clock;
-            }
-            seqlock_write_unlock(&timers_state.vm_clock_seqlock,
-                                 &timers_state.vm_clock_lock);
-            timer_mod_anticipate(timers_state.icount_warp_timer,
-                                 clock + deadline);
-        }
-    } else if (deadline == 0) {
-        qemu_clock_notify(QEMU_CLOCK_VIRTUAL);
-    }
-}
-
-static void qemu_account_warp_timer(void)
-{
-    if (!use_icount || !icount_sleep) {
-        return;
-    }
-
-    /* Nothing to do if the VM is stopped: QEMU_CLOCK_VIRTUAL timers
-     * do not fire, so computing the deadline does not make sense.
-     */
-    if (!runstate_is_running()) {
-        return;
-    }
-
-    /* warp clock deterministically in record/replay mode */
-    if (!replay_checkpoint(CHECKPOINT_CLOCK_WARP_ACCOUNT)) {
-        return;
-    }
-
-    timer_del(timers_state.icount_warp_timer);
-    icount_warp_rt();
-}
-
-static bool icount_state_needed(void *opaque)
-{
-    return use_icount;
-}
-
-static bool warp_timer_state_needed(void *opaque)
-{
-    TimersState *s = opaque;
-    return s->icount_warp_timer != NULL;
-}
-
-static bool adjust_timers_state_needed(void *opaque)
-{
-    TimersState *s = opaque;
-    return s->icount_rt_timer != NULL;
-}
-
-static bool shift_state_needed(void *opaque)
-{
-    return use_icount == 2;
-}
-
-/*
- * Subsection for warp timer migration is optional, because may not be created
- */
-static const VMStateDescription icount_vmstate_warp_timer = {
-    .name = "timer/icount/warp_timer",
-    .version_id = 1,
-    .minimum_version_id = 1,
-    .needed = warp_timer_state_needed,
-    .fields = (VMStateField[]) {
-        VMSTATE_INT64(vm_clock_warp_start, TimersState),
-        VMSTATE_TIMER_PTR(icount_warp_timer, TimersState),
-        VMSTATE_END_OF_LIST()
-    }
-};
-
-static const VMStateDescription icount_vmstate_adjust_timers = {
-    .name = "timer/icount/timers",
-    .version_id = 1,
-    .minimum_version_id = 1,
-    .needed = adjust_timers_state_needed,
-    .fields = (VMStateField[]) {
-        VMSTATE_TIMER_PTR(icount_rt_timer, TimersState),
-        VMSTATE_TIMER_PTR(icount_vm_timer, TimersState),
-        VMSTATE_END_OF_LIST()
-    }
-};
-
-static const VMStateDescription icount_vmstate_shift = {
-    .name = "timer/icount/shift",
-    .version_id = 1,
-    .minimum_version_id = 1,
-    .needed = shift_state_needed,
-    .fields = (VMStateField[]) {
-        VMSTATE_INT16(icount_time_shift, TimersState),
-        VMSTATE_END_OF_LIST()
-    }
-};
-
-/*
- * This is a subsection for icount migration.
- */
-static const VMStateDescription icount_vmstate_timers = {
-    .name = "timer/icount",
-    .version_id = 1,
-    .minimum_version_id = 1,
-    .needed = icount_state_needed,
-    .fields = (VMStateField[]) {
-        VMSTATE_INT64(qemu_icount_bias, TimersState),
-        VMSTATE_INT64(qemu_icount, TimersState),
-        VMSTATE_END_OF_LIST()
-    },
-    .subsections = (const VMStateDescription*[]) {
-        &icount_vmstate_warp_timer,
-        &icount_vmstate_adjust_timers,
-        &icount_vmstate_shift,
-        NULL
-    }
-};
-
-static const VMStateDescription vmstate_timers = {
-    .name = "timer",
-    .version_id = 2,
-    .minimum_version_id = 1,
-    .fields = (VMStateField[]) {
-        VMSTATE_INT64(cpu_ticks_offset, TimersState),
-        VMSTATE_UNUSED(8),
-        VMSTATE_INT64_V(cpu_clock_offset, TimersState, 2),
-        VMSTATE_END_OF_LIST()
-    },
-    .subsections = (const VMStateDescription*[]) {
-        &icount_vmstate_timers,
-        NULL
-    }
-};
-
-void cpu_ticks_init(void)
-{
-    seqlock_init(&timers_state.vm_clock_seqlock);
-    qemu_spin_init(&timers_state.vm_clock_lock);
-    vmstate_register(NULL, 0, &vmstate_timers, &timers_state);
-    cpu_throttle_init();
-}
-
-void configure_icount(QemuOpts *opts, Error **errp)
-{
-    const char *option = qemu_opt_get(opts, "shift");
-    bool sleep = qemu_opt_get_bool(opts, "sleep", true);
-    bool align = qemu_opt_get_bool(opts, "align", false);
-    long time_shift = -1;
-
-    if (!option) {
-        if (qemu_opt_get(opts, "align") != NULL) {
-            error_setg(errp, "Please specify shift option when using align");
-        }
-        return;
-    }
-
-    if (align && !sleep) {
-        error_setg(errp, "align=on and sleep=off are incompatible");
-        return;
-    }
-
-    if (strcmp(option, "auto") != 0) {
-        if (qemu_strtol(option, NULL, 0, &time_shift) < 0
-            || time_shift < 0 || time_shift > MAX_ICOUNT_SHIFT) {
-            error_setg(errp, "icount: Invalid shift value");
-            return;
-        }
-    } else if (icount_align_option) {
-        error_setg(errp, "shift=auto and align=on are incompatible");
-        return;
-    } else if (!icount_sleep) {
-        error_setg(errp, "shift=auto and sleep=off are incompatible");
-        return;
-    }
-
-    icount_sleep = sleep;
-    if (icount_sleep) {
-        timers_state.icount_warp_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL_RT,
-                                         icount_timer_cb, NULL);
-    }
-
-    icount_align_option = align;
-
-    if (time_shift >= 0) {
-        timers_state.icount_time_shift = time_shift;
-        use_icount = 1;
-        return;
-    }
-
-    use_icount = 2;
-
-    /* 125MIPS seems a reasonable initial guess at the guest speed.
-       It will be corrected fairly quickly anyway.  */
-    timers_state.icount_time_shift = 3;
-
-    /* Have both realtime and virtual time triggers for speed adjustment.
-       The realtime trigger catches emulated time passing too slowly,
-       the virtual time trigger catches emulated time passing too fast.
-       Realtime triggers occur even when idle, so use them less frequently
-       than VM triggers.  */
-    timers_state.vm_clock_warp_start = -1;
-    timers_state.icount_rt_timer = timer_new_ms(QEMU_CLOCK_VIRTUAL_RT,
-                                   icount_adjust_rt, NULL);
-    timer_mod(timers_state.icount_rt_timer,
-                   qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL_RT) + 1000);
-    timers_state.icount_vm_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
-                                        icount_adjust_vm, NULL);
-    timer_mod(timers_state.icount_vm_timer,
-                   qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
-                   NANOSECONDS_PER_SECOND / 10);
-}
-
 /***********************************************************/
 /* TCG vCPU kick timer
  *
@@ -854,35 +171,6 @@ static void qemu_cpu_kick_rr_cpus(void)
     };
 }
 
-static void do_nothing(CPUState *cpu, run_on_cpu_data unused)
-{
-}
-
-void qemu_timer_notify_cb(void *opaque, QEMUClockType type)
-{
-    if (!use_icount || 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);
-    }
-}
-
 static void kick_tcg_thread(void *opaque)
 {
     timer_mod(tcg_kick_vcpu_timer, qemu_tcg_next_kick());
@@ -1284,7 +572,7 @@ static int64_t tcg_get_icount_limit(void)
             deadline = INT32_MAX;
         }
 
-        return qemu_icount_round(deadline);
+        return icount_round(deadline);
     } else {
         return replay_get_instructions();
     }
@@ -1300,7 +588,7 @@ static void notify_aio_contexts(void)
 static void handle_icount_deadline(void)
 {
     assert(qemu_in_vcpu_thread());
-    if (use_icount) {
+    if (icount_enabled()) {
         int64_t deadline = qemu_clock_deadline_ns_all(QEMU_CLOCK_VIRTUAL,
                                                       QEMU_TIMER_ATTR_ALL);
 
@@ -1312,7 +600,7 @@ static void handle_icount_deadline(void)
 
 static void prepare_icount_for_run(CPUState *cpu)
 {
-    if (use_icount) {
+    if (icount_enabled()) {
         int insns_left;
 
         /* These should always be cleared by process_icount_data after
@@ -1337,9 +625,9 @@ static void prepare_icount_for_run(CPUState *cpu)
 
 static void process_icount_data(CPUState *cpu)
 {
-    if (use_icount) {
+    if (icount_enabled()) {
         /* Account for executed instructions */
-        cpu_update_icount(cpu);
+        icount_update(cpu);
 
         /* Reset the counters */
         cpu_neg(cpu)->icount_decr.u16.low = 0;
@@ -1440,7 +728,7 @@ static void *qemu_tcg_rr_cpu_thread_fn(void *arg)
         replay_mutex_lock();
         qemu_mutex_lock_iothread();
         /* Account partial waits to QEMU_CLOCK_VIRTUAL.  */
-        qemu_account_warp_timer();
+        icount_account_warp_timer();
 
         /* Run the timers here.  This is much more efficient than
          * waking up the I/O thread and waiting for completion.
@@ -1498,7 +786,7 @@ static void *qemu_tcg_rr_cpu_thread_fn(void *arg)
             atomic_mb_set(&cpu->exit_request, 0);
         }
 
-        if (use_icount && all_cpu_threads_idle()) {
+        if (icount_enabled() && all_cpu_threads_idle()) {
             /*
              * When all cpus are sleeping (e.g in WFI), to avoid a deadlock
              * in the main_loop, wake it up in order to start the warp timer.
@@ -1651,7 +939,7 @@ static void *qemu_tcg_cpu_thread_fn(void *arg)
     CPUState *cpu = arg;
 
     assert(tcg_enabled());
-    g_assert(!use_icount);
+    g_assert(!icount_enabled());
 
     rcu_register_thread();
     tcg_register_thread();
@@ -2230,21 +1518,3 @@ void qmp_inject_nmi(Error **errp)
     nmi_monitor_handle(monitor_get_cpu_index(), errp);
 }
 
-void dump_drift_info(void)
-{
-    if (!use_icount) {
-        return;
-    }
-
-    qemu_printf("Host - Guest clock  %"PRIi64" ms\n",
-                (cpu_get_clock() - cpu_get_icount())/SCALE_MS);
-    if (icount_align_option) {
-        qemu_printf("Max guest delay     %"PRIi64" ms\n",
-                    -max_delay / SCALE_MS);
-        qemu_printf("Max guest advance   %"PRIi64" ms\n",
-                    max_advance / SCALE_MS);
-    } else {
-        qemu_printf("Max guest delay     NA\n");
-        qemu_printf("Max guest advance   NA\n");
-    }
-}
diff --git a/softmmu/icount.c b/softmmu/icount.c
new file mode 100644
index 0000000000..aeede3601c
--- /dev/null
+++ b/softmmu/icount.c
@@ -0,0 +1,499 @@
+/*
+ * QEMU System Emulator
+ *
+ * Copyright (c) 2003-2008 Fabrice Bellard
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu-common.h"
+#include "qemu/cutils.h"
+#include "migration/vmstate.h"
+#include "qapi/error.h"
+#include "qemu/error-report.h"
+#include "exec/exec-all.h"
+#include "sysemu/cpus.h"
+#include "sysemu/qtest.h"
+#include "qemu/main-loop.h"
+#include "qemu/option.h"
+#include "qemu/seqlock.h"
+#include "sysemu/replay.h"
+#include "sysemu/runstate.h"
+#include "hw/core/cpu.h"
+#include "sysemu/cpu-timers.h"
+#include "sysemu/cpu-throttle.h"
+#include "timers-state.h"
+
+/*
+ * ICOUNT: Instruction Counter
+ *
+ * this module is split off from cpu-timers because the icount part
+ * is TCG-specific, and does not need to be built for other accels.
+ */
+static bool icount_sleep = true;
+/* Arbitrarily pick 1MIPS as the minimum allowable speed.  */
+#define MAX_ICOUNT_SHIFT 10
+
+/*
+ * 0 = Do not count executed instructions.
+ * 1 = Fixed conversion of insn to ns via "shift" option
+ * 2 = Runtime adaptive algorithm to compute shift
+ */
+static int use_icount;
+
+int icount_enabled(void)
+{
+    return use_icount;
+}
+
+static void icount_enable_precise(void)
+{
+    use_icount = 1;
+}
+
+static void icount_enable_adaptive(void)
+{
+    use_icount = 2;
+}
+
+/*
+ * 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 icount_get_executed(CPUState *cpu)
+{
+    return (cpu->icount_budget -
+            (cpu_neg(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.
+ */
+static void icount_update_locked(CPUState *cpu)
+{
+    int64_t executed = icount_get_executed(cpu);
+    cpu->icount_budget -= executed;
+
+    atomic_set_i64(&timers_state.qemu_icount,
+                   timers_state.qemu_icount + executed);
+}
+
+/*
+ * 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 icount_update(CPUState *cpu)
+{
+    seqlock_write_lock(&timers_state.vm_clock_seqlock,
+                       &timers_state.vm_clock_lock);
+    icount_update_locked(cpu);
+    seqlock_write_unlock(&timers_state.vm_clock_seqlock,
+                         &timers_state.vm_clock_lock);
+}
+
+static int64_t icount_get_raw_locked(void)
+{
+    CPUState *cpu = current_cpu;
+
+    if (cpu && cpu->running) {
+        if (!cpu->can_do_io) {
+            error_report("Bad icount read");
+            exit(1);
+        }
+        /* Take into account what has run */
+        icount_update_locked(cpu);
+    }
+    /* The read is protected by the seqlock, but needs atomic64 to avoid UB */
+    return atomic_read_i64(&timers_state.qemu_icount);
+}
+
+static int64_t icount_get_locked(void)
+{
+    int64_t icount = icount_get_raw_locked();
+    return atomic_read_i64(&timers_state.qemu_icount_bias) +
+        icount_to_ns(icount);
+}
+
+int64_t icount_get_raw(void)
+{
+    int64_t icount;
+    unsigned start;
+
+    do {
+        start = seqlock_read_begin(&timers_state.vm_clock_seqlock);
+        icount = icount_get_raw_locked();
+    } while (seqlock_read_retry(&timers_state.vm_clock_seqlock, start));
+
+    return icount;
+}
+
+/* Return the virtual CPU time, based on the instruction counter.  */
+int64_t icount_get(void)
+{
+    int64_t icount;
+    unsigned start;
+
+    do {
+        start = seqlock_read_begin(&timers_state.vm_clock_seqlock);
+        icount = icount_get_locked();
+    } while (seqlock_read_retry(&timers_state.vm_clock_seqlock, start));
+
+    return icount;
+}
+
+int64_t icount_to_ns(int64_t icount)
+{
+    return icount << atomic_read(&timers_state.icount_time_shift);
+}
+
+/*
+ * Correlation between real and virtual time is always going to be
+ * fairly approximate, so ignore small variation.
+ * When the guest is idle real and virtual time will be aligned in
+ * the IO wait loop.
+ */
+#define ICOUNT_WOBBLE (NANOSECONDS_PER_SECOND / 10)
+
+static void icount_adjust(void)
+{
+    int64_t cur_time;
+    int64_t cur_icount;
+    int64_t delta;
+
+    /* Protected by TimersState mutex.  */
+    static int64_t last_delta;
+
+    /* If the VM is not running, then do nothing.  */
+    if (!runstate_is_running()) {
+        return;
+    }
+
+    seqlock_write_lock(&timers_state.vm_clock_seqlock,
+                       &timers_state.vm_clock_lock);
+    cur_time = REPLAY_CLOCK_LOCKED(REPLAY_CLOCK_VIRTUAL_RT,
+                                   cpu_get_clock_locked());
+    cur_icount = icount_get_locked();
+
+    delta = cur_icount - cur_time;
+    /* FIXME: This is a very crude algorithm, somewhat prone to oscillation.  */
+    if (delta > 0
+        && last_delta + ICOUNT_WOBBLE < delta * 2
+        && timers_state.icount_time_shift > 0) {
+        /* The guest is getting too far ahead.  Slow time down.  */
+        atomic_set(&timers_state.icount_time_shift,
+                   timers_state.icount_time_shift - 1);
+    }
+    if (delta < 0
+        && last_delta - ICOUNT_WOBBLE > delta * 2
+        && timers_state.icount_time_shift < MAX_ICOUNT_SHIFT) {
+        /* The guest is getting too far behind.  Speed time up.  */
+        atomic_set(&timers_state.icount_time_shift,
+                   timers_state.icount_time_shift + 1);
+    }
+    last_delta = delta;
+    atomic_set_i64(&timers_state.qemu_icount_bias,
+                   cur_icount - (timers_state.qemu_icount
+                                 << timers_state.icount_time_shift));
+    seqlock_write_unlock(&timers_state.vm_clock_seqlock,
+                         &timers_state.vm_clock_lock);
+}
+
+static void icount_adjust_rt(void *opaque)
+{
+    timer_mod(timers_state.icount_rt_timer,
+              qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL_RT) + 1000);
+    icount_adjust();
+}
+
+static void icount_adjust_vm(void *opaque)
+{
+    timer_mod(timers_state.icount_vm_timer,
+                   qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
+                   NANOSECONDS_PER_SECOND / 10);
+    icount_adjust();
+}
+
+int64_t icount_round(int64_t count)
+{
+    int shift = atomic_read(&timers_state.icount_time_shift);
+    return (count + (1 << shift) - 1) >> shift;
+}
+
+static void icount_warp_rt(void)
+{
+    unsigned seq;
+    int64_t warp_start;
+
+    /*
+     * The icount_warp_timer is rescheduled soon after vm_clock_warp_start
+     * changes from -1 to another value, so the race here is okay.
+     */
+    do {
+        seq = seqlock_read_begin(&timers_state.vm_clock_seqlock);
+        warp_start = timers_state.vm_clock_warp_start;
+    } while (seqlock_read_retry(&timers_state.vm_clock_seqlock, seq));
+
+    if (warp_start == -1) {
+        return;
+    }
+
+    seqlock_write_lock(&timers_state.vm_clock_seqlock,
+                       &timers_state.vm_clock_lock);
+    if (runstate_is_running()) {
+        int64_t clock = REPLAY_CLOCK_LOCKED(REPLAY_CLOCK_VIRTUAL_RT,
+                                            cpu_get_clock_locked());
+        int64_t warp_delta;
+
+        warp_delta = clock - timers_state.vm_clock_warp_start;
+        if (icount_enabled() == 2) {
+            /*
+             * In adaptive mode, do not let QEMU_CLOCK_VIRTUAL run too
+             * far ahead of real time.
+             */
+            int64_t cur_icount = icount_get_locked();
+            int64_t delta = clock - cur_icount;
+            warp_delta = MIN(warp_delta, delta);
+        }
+        atomic_set_i64(&timers_state.qemu_icount_bias,
+                       timers_state.qemu_icount_bias + warp_delta);
+    }
+    timers_state.vm_clock_warp_start = -1;
+    seqlock_write_unlock(&timers_state.vm_clock_seqlock,
+                       &timers_state.vm_clock_lock);
+
+    if (qemu_clock_expired(QEMU_CLOCK_VIRTUAL)) {
+        qemu_clock_notify(QEMU_CLOCK_VIRTUAL);
+    }
+}
+
+static void icount_timer_cb(void *opaque)
+{
+    /*
+     * No need for a checkpoint because the timer already synchronizes
+     * with CHECKPOINT_CLOCK_VIRTUAL_RT.
+     */
+    icount_warp_rt();
+}
+
+void icount_start_warp_timer(void)
+{
+    int64_t clock;
+    int64_t deadline;
+
+    if (!icount_enabled()) {
+        return;
+    }
+
+    /*
+     * Nothing to do if the VM is stopped: QEMU_CLOCK_VIRTUAL timers
+     * do not fire, so computing the deadline does not make sense.
+     */
+    if (!runstate_is_running()) {
+        return;
+    }
+
+    if (replay_mode != REPLAY_MODE_PLAY) {
+        if (!all_cpu_threads_idle()) {
+            return;
+        }
+
+        if (qtest_enabled()) {
+            /* When testing, qtest commands advance icount.  */
+            return;
+        }
+
+        replay_checkpoint(CHECKPOINT_CLOCK_WARP_START);
+    } else {
+        /* warp clock deterministically in record/replay mode */
+        if (!replay_checkpoint(CHECKPOINT_CLOCK_WARP_START)) {
+            /*
+             * vCPU is sleeping and warp can't be started.
+             * It is probably a race condition: notification sent
+             * to vCPU was processed in advance and vCPU went to sleep.
+             * Therefore we have to wake it up for doing someting.
+             */
+            if (replay_has_checkpoint()) {
+                qemu_clock_notify(QEMU_CLOCK_VIRTUAL);
+            }
+            return;
+        }
+    }
+
+    /* We want to use the earliest deadline from ALL vm_clocks */
+    clock = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL_RT);
+    deadline = qemu_clock_deadline_ns_all(QEMU_CLOCK_VIRTUAL,
+                                          ~QEMU_TIMER_ATTR_EXTERNAL);
+    if (deadline < 0) {
+        static bool notified;
+        if (!icount_sleep && !notified) {
+            warn_report("icount sleep disabled and no active timers");
+            notified = true;
+        }
+        return;
+    }
+
+    if (deadline > 0) {
+        /*
+         * Ensure QEMU_CLOCK_VIRTUAL proceeds even when the virtual CPU goes to
+         * sleep.  Otherwise, the CPU might be waiting for a future timer
+         * interrupt to wake it up, but the interrupt never comes because
+         * the vCPU isn't running any insns and thus doesn't advance the
+         * QEMU_CLOCK_VIRTUAL.
+         */
+        if (!icount_sleep) {
+            /*
+             * We never let VCPUs sleep in no sleep icount mode.
+             * If there is a pending QEMU_CLOCK_VIRTUAL timer we just advance
+             * to the next QEMU_CLOCK_VIRTUAL event and notify it.
+             * It is useful when we want a deterministic execution time,
+             * isolated from host latencies.
+             */
+            seqlock_write_lock(&timers_state.vm_clock_seqlock,
+                               &timers_state.vm_clock_lock);
+            atomic_set_i64(&timers_state.qemu_icount_bias,
+                           timers_state.qemu_icount_bias + deadline);
+            seqlock_write_unlock(&timers_state.vm_clock_seqlock,
+                                 &timers_state.vm_clock_lock);
+            qemu_clock_notify(QEMU_CLOCK_VIRTUAL);
+        } else {
+            /*
+             * We do stop VCPUs and only advance QEMU_CLOCK_VIRTUAL after some
+             * "real" time, (related to the time left until the next event) has
+             * passed. The QEMU_CLOCK_VIRTUAL_RT clock will do this.
+             * This avoids that the warps are visible externally; for example,
+             * you will not be sending network packets continuously instead of
+             * every 100ms.
+             */
+            seqlock_write_lock(&timers_state.vm_clock_seqlock,
+                               &timers_state.vm_clock_lock);
+            if (timers_state.vm_clock_warp_start == -1
+                || timers_state.vm_clock_warp_start > clock) {
+                timers_state.vm_clock_warp_start = clock;
+            }
+            seqlock_write_unlock(&timers_state.vm_clock_seqlock,
+                                 &timers_state.vm_clock_lock);
+            timer_mod_anticipate(timers_state.icount_warp_timer,
+                                 clock + deadline);
+        }
+    } else if (deadline == 0) {
+        qemu_clock_notify(QEMU_CLOCK_VIRTUAL);
+    }
+}
+
+void icount_account_warp_timer(void)
+{
+    if (!use_icount || !icount_sleep) {
+        return;
+    }
+
+    /*
+     * Nothing to do if the VM is stopped: QEMU_CLOCK_VIRTUAL timers
+     * do not fire, so computing the deadline does not make sense.
+     */
+    if (!runstate_is_running()) {
+        return;
+    }
+
+    /* warp clock deterministically in record/replay mode */
+    if (!replay_checkpoint(CHECKPOINT_CLOCK_WARP_ACCOUNT)) {
+        return;
+    }
+
+    timer_del(timers_state.icount_warp_timer);
+    icount_warp_rt();
+}
+
+void icount_configure(QemuOpts *opts, Error **errp)
+{
+    const char *option = qemu_opt_get(opts, "shift");
+    bool sleep = qemu_opt_get_bool(opts, "sleep", true);
+    bool align = qemu_opt_get_bool(opts, "align", false);
+    long time_shift = -1;
+
+    if (!option) {
+        if (qemu_opt_get(opts, "align") != NULL) {
+            error_setg(errp, "Please specify shift option when using align");
+        }
+        return;
+    }
+
+    if (align && !sleep) {
+        error_setg(errp, "align=on and sleep=off are incompatible");
+        return;
+    }
+
+    if (strcmp(option, "auto") != 0) {
+        if (qemu_strtol(option, NULL, 0, &time_shift) < 0
+            || time_shift < 0 || time_shift > MAX_ICOUNT_SHIFT) {
+            error_setg(errp, "icount: Invalid shift value");
+            return;
+        }
+    } else if (icount_align_option) {
+        error_setg(errp, "shift=auto and align=on are incompatible");
+        return;
+    } else if (!icount_sleep) {
+        error_setg(errp, "shift=auto and sleep=off are incompatible");
+        return;
+    }
+
+    icount_sleep = sleep;
+    if (icount_sleep) {
+        timers_state.icount_warp_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL_RT,
+                                         icount_timer_cb, NULL);
+    }
+
+    icount_align_option = align;
+
+    if (time_shift >= 0) {
+        timers_state.icount_time_shift = time_shift;
+        icount_enable_precise();
+        return;
+    }
+
+    icount_enable_adaptive();
+
+    /*
+     * 125MIPS seems a reasonable initial guess at the guest speed.
+     * It will be corrected fairly quickly anyway.
+     */
+    timers_state.icount_time_shift = 3;
+
+    /*
+     * Have both realtime and virtual time triggers for speed adjustment.
+     * The realtime trigger catches emulated time passing too slowly,
+     * the virtual time trigger catches emulated time passing too fast.
+     * Realtime triggers occur even when idle, so use them less frequently
+     * than VM triggers.
+     */
+    timers_state.vm_clock_warp_start = -1;
+    timers_state.icount_rt_timer = timer_new_ms(QEMU_CLOCK_VIRTUAL_RT,
+                                   icount_adjust_rt, NULL);
+    timer_mod(timers_state.icount_rt_timer,
+                   qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL_RT) + 1000);
+    timers_state.icount_vm_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
+                                        icount_adjust_vm, NULL);
+    timer_mod(timers_state.icount_vm_timer,
+                   qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
+                   NANOSECONDS_PER_SECOND / 10);
+}
diff --git a/softmmu/qtest.c b/softmmu/qtest.c
index 5672b75c35..d79b3f3db1 100644
--- a/softmmu/qtest.c
+++ b/softmmu/qtest.c
@@ -21,7 +21,7 @@
 #include "exec/memory.h"
 #include "hw/irq.h"
 #include "sysemu/accel.h"
-#include "sysemu/cpus.h"
+#include "sysemu/cpu-timers.h"
 #include "qemu/config-file.h"
 #include "qemu/option.h"
 #include "qemu/error-report.h"
@@ -273,6 +273,38 @@ static void qtest_irq_handler(void *opaque, int n, int level)
     }
 }
 
+static int64_t qtest_clock_counter;
+
+int64_t qtest_get_clock(void)
+{
+    return atomic_read_i64(&qtest_clock_counter);
+}
+
+static void qtest_set_clock(int64_t count)
+{
+    atomic_set_i64(&qtest_clock_counter, count);
+}
+
+static void qtest_clock_warp(int64_t dest)
+{
+    int64_t clock = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+    AioContext *aio_context;
+    assert(qtest_enabled());
+    aio_context = qemu_get_aio_context();
+    while (clock < dest) {
+        int64_t deadline = qemu_clock_deadline_ns_all(QEMU_CLOCK_VIRTUAL,
+                                                      QEMU_TIMER_ATTR_ALL);
+        int64_t warp = qemu_soonest_timeout(dest - clock, deadline);
+
+        qtest_set_clock(qtest_get_clock() + warp);
+
+        qemu_clock_run_timers(QEMU_CLOCK_VIRTUAL);
+        timerlist_run_timers(aio_context->tlg.tl[QEMU_CLOCK_VIRTUAL]);
+        clock = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+    }
+    qemu_clock_notify(QEMU_CLOCK_VIRTUAL);
+}
+
 static void qtest_process_command(CharBackend *chr, gchar **words)
 {
     const gchar *command;
diff --git a/softmmu/timers-state.h b/softmmu/timers-state.h
new file mode 100644
index 0000000000..db4e60f18f
--- /dev/null
+++ b/softmmu/timers-state.h
@@ -0,0 +1,69 @@
+/*
+ * QEMU System Emulator
+ *
+ * Copyright (c) 2003-2008 Fabrice Bellard
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#ifndef TIMERS_STATE_H
+#define TIMERS_STATE_H
+
+/* timers state, for sharing between icount and cpu-timers */
+
+typedef struct TimersState {
+    /* Protected by BQL.  */
+    int64_t cpu_ticks_prev;
+    int64_t cpu_ticks_offset;
+
+    /*
+     * Protect fields that can be respectively read outside the
+     * BQL, and written from multiple threads.
+     */
+    QemuSeqLock vm_clock_seqlock;
+    QemuSpin vm_clock_lock;
+
+    int16_t cpu_ticks_enabled;
+
+    /* Conversion factor from emulated instructions to virtual clock ticks.  */
+    int16_t icount_time_shift;
+
+    /* Compensate for varying guest execution speed.  */
+    int64_t qemu_icount_bias;
+
+    int64_t vm_clock_warp_start;
+    int64_t cpu_clock_offset;
+
+    /* Only written by TCG thread */
+    int64_t qemu_icount;
+
+    /* for adjusting icount */
+    QEMUTimer *icount_rt_timer;
+    QEMUTimer *icount_vm_timer;
+    QEMUTimer *icount_warp_timer;
+} TimersState;
+
+extern TimersState timers_state;
+
+/*
+ * icount needs this internal from cpu-timers when adjusting the icount shift.
+ */
+int64_t cpu_get_clock_locked(void);
+
+#endif /* TIMERS_STATE_H */
diff --git a/softmmu/vl.c b/softmmu/vl.c
index 3e15ee2435..4b3558f85c 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"
@@ -2675,7 +2676,7 @@ static void user_register_global_props(void)
 
 static int do_configure_icount(void *opaque, QemuOpts *opts, Error **errp)
 {
-    configure_icount(opts, errp);
+    icount_configure(opts, errp);
     return 0;
 }
 
@@ -2785,7 +2786,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);
     }
@@ -4233,7 +4234,8 @@ void qemu_init(int argc, char **argv, char **envp)
     /* spice needs the timers to be initialized by this point */
     qemu_spice_init();
 
-    cpu_ticks_init();
+    /* initialize cpu timers and VCPU throttle modules */
+    cpu_timers_init();
 
     if (default_net) {
         QemuOptsList *net = qemu_find_opts("net");
diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
index f32b9e47a3..42d4f459e6 100644
--- a/stubs/Makefile.objs
+++ b/stubs/Makefile.objs
@@ -1,7 +1,8 @@
 stub-obj-y += blk-commit-all.o
 stub-obj-y += cmos.o
 stub-obj-y += cpu-get-clock.o
-stub-obj-y += cpu-get-icount.o
+stub-obj-y += qemu-timer-notify-cb.o
+stub-obj-y += icount.o
 stub-obj-y += dump.o
 stub-obj-y += error-printf.o
 stub-obj-y += fdset.o
diff --git a/stubs/clock-warp.c b/stubs/clock-warp.c
index b53e5dd94c..304da5091c 100644
--- a/stubs/clock-warp.c
+++ b/stubs/clock-warp.c
@@ -1,7 +1,7 @@
 #include "qemu/osdep.h"
-#include "qemu/timer.h"
+#include "sysemu/cpu-timers.h"
 
-void qemu_start_warp_timer(void)
+void icount_start_warp_timer(void)
 {
 }
 
diff --git a/stubs/cpu-get-clock.c b/stubs/cpu-get-clock.c
index 5a92810e87..9e92404816 100644
--- a/stubs/cpu-get-clock.c
+++ b/stubs/cpu-get-clock.c
@@ -1,5 +1,6 @@
 #include "qemu/osdep.h"
-#include "qemu/timer.h"
+#include "sysemu/cpu-timers.h"
+#include "qemu/main-loop.h"
 
 int64_t cpu_get_clock(void)
 {
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();
-}
diff --git a/stubs/icount.c b/stubs/icount.c
new file mode 100644
index 0000000000..9054724e92
--- /dev/null
+++ b/stubs/icount.c
@@ -0,0 +1,22 @@
+#include "qemu/osdep.h"
+#include "sysemu/cpu-timers.h"
+
+int64_t icount_get(void)
+{
+    abort();
+}
+
+int64_t icount_get_raw(void)
+{
+    abort();
+}
+
+void icount_configure(QemuOpts *opts, Error **errp)
+{
+    abort();
+}
+
+int icount_enabled(void)
+{
+    return 0;
+}
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..fe775a622c 100644
--- a/stubs/qtest.c
+++ b/stubs/qtest.c
@@ -18,3 +18,8 @@ bool qtest_driver(void)
 {
     return false;
 }
+
+int64_t qtest_get_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 dc9c29f998..06b920fab8 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,17 +1207,17 @@ 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)
 {
-    return (uint64_t)cpu_get_icount_raw();
+    return (uint64_t)icount_get_raw();
 }
 
 static int64_t instructions_ns_per(uint64_t icount)
 {
-    return cpu_icount_to_ns((int64_t)icount);
+    return icount_to_ns((int64_t)icount);
 }
 #endif
 
diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 383be0a955..412ca36393 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -174,8 +174,8 @@ static int write_fcsr(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) {
-        *val = cpu_get_icount();
+    if (icount_enabled()) {
+        *val = icount_get();
     } else {
         *val = cpu_get_host_ticks();
     }
@@ -189,8 +189,8 @@ 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) {
-        *val = cpu_get_icount() >> 32;
+    if (icount_enabled()) {
+        *val = icount_get() >> 32;
     } else {
         *val = cpu_get_host_ticks() >> 32;
     }
diff --git a/tests/ptimer-test-stubs.c b/tests/ptimer-test-stubs.c
index ed393d9082..b4447a3e44 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,10 @@ QEMUTimerListGroup main_loop_tlg;
 
 int64_t ptimer_test_time_ns;
 
-/* Do not artificially limit period - see hw/core/ptimer.c.  */
-int use_icount = 1;
+int icount_enabled(void)
+{
+    return 0;
+}
 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 eda63fe4e0..f1af697572 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"
@@ -521,7 +521,7 @@ void main_loop_wait(int nonblocking)
 
     /* CPU thread can infinitely wait for event after
        missing the warp */
-    qemu_start_warp_timer();
+    icount_start_warp_timer();
     qemu_clock_run_all_timers();
 }
 
diff --git a/util/qemu-timer.c b/util/qemu-timer.c
index f62b4feecd..8b8c5cb762 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)
@@ -417,7 +419,7 @@ static void timerlist_rearm(QEMUTimerList *timer_list)
 {
     /* Interrupt execution to force deadline recalculation.  */
     if (timer_list->clock->type == QEMU_CLOCK_VIRTUAL) {
-        qemu_start_warp_timer();
+        icount_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) {
-            return cpu_get_icount();
+        if (icount_enabled()) {
+            return icount_get();
+        } else if (qtest_enabled()) { /* for qtest_clock_warp */
+            return qtest_get_clock();
         } else {
             return cpu_get_clock();
         }
-- 
2.16.4



^ permalink raw reply related	[flat|nested] 37+ messages in thread

* Re: [PATCH 0/3] QEMU cpus.c refactoring part1
  2020-06-29  9:35 [PATCH 0/3] QEMU cpus.c refactoring part1 Claudio Fontana
                   ` (2 preceding siblings ...)
  2020-06-29  9:35 ` [PATCH 3/3] cpu-timers, icount: new modules Claudio Fontana
@ 2020-07-02  6:27 ` Claudio Fontana
  2020-07-03 17:21   ` Paolo Bonzini
  3 siblings, 1 reply; 37+ messages in thread
From: Claudio Fontana @ 2020-07-02  6:27 UTC (permalink / raw)
  To: Paolo Bonzini, Alex Bennee, Markus Armbruster; +Cc: qemu-devel

Hi Alex, Markus, Paolo, 

maybe this could be queued in one of your queues?

Thanks a lot,

Claudio

On 6/29/20 11:35 AM, Claudio Fontana wrote:
> Motivation and higher level steps:
> 
> https://lists.gnu.org/archive/html/qemu-devel/2020-05/msg04628.html
> 
> Previous series: [RFC RESEND v7 0/4] QEMU cpus.c refactoring
> 
> This series is already reviewed, and is a split of the first three patches
> from the previous series (RFC). The forth and last of the previous series
> will then be posted separately.
> 
> PREVIOUS DISCUSSIONS:
> 
> * should we reorder patches or moves inside patches to avoid code going
>   from cpus.c to softmmu/cpus.c and then again to softmmu/somethingelse.c ?
>   (Philippe)
> 
> * some questions about headers in include/softmmu (Philippe)
> 
> 
> [SPLIT into TWO series, changed from RFC to PATCH]
> ----
> 
> v6 -> v7:
> 
> * rebased changes on top of Pavel Dovgalyuk changes to dma-helpers.c
>   "icount: make dma reads deterministic"
> 
> ----
> 
> v5 -> v6:
> 
> * rebased changes on top of Emilio G. Cota changes to cpus.c
>   "cpu: convert queued work to a QSIMPLEQ"
> 
> * keep a pointer in cpus.c instead of a copy of CpusAccel
>   (Alex)
> 
> ----
> 
> 
> v4 -> v5: rebase on latest master
> 
> * rebased changes on top of roman series to remove one of the extra states for hvf.
>   (Is the result now functional for HVF?)
> 
> * rebased changes on top of icount changes and fixes to icount_configure and
>   the new shift vmstate. (Markus)
> 
> v3 -> v4:
> 
> * overall: added copyright headers to all files that were missing them
>   (used copyright and license of the module the stuff was extracted from).
>   For the new interface files, added SUSE LLC.
> 
> * 1/4 (move softmmu only files from root):
> 
>   MAINTAINERS: moved softmmu/cpus.c to its final location (from patch 2)
> 
> * 2/4 (cpu-throttle):
> 
>   MAINTAINERS (to patch 1),
>   copyright Fabrice Bellard and license from cpus.c
> 
> * 3/4 (cpu-timers, icount):
> 
>   - MAINTAINERS: add cpu-timers.c and icount.c to Paolo
> 
>   - break very long lines (patchew)
> 
>   - add copyright SUSE LLC, GPLv2 to cpu-timers.h
> 
>   - add copyright Fabrice Bellard and license from cpus.c to timers-state.h
>     as it is lifted from cpus.c
> 
>   - vl.c: in configure_accelerators bail out if icount_enabled()
>     and !tcg_enabled() as qtest does not enable icount anymore.
> 
> * 4/4 (accel stuff to accel):
> 
>   - add copyright SUSE LLC to files that mostly only consist of the
>     new interface. Add whatever copyright was in the accelerator code
>     if instead they mostly consist of accelerator code.
> 
>   - change a comment to mention the result of the AccelClass experiment
> 
>   - moved qtest accelerator into accel/qtest/ , make it like the others.
> 
>   - rename xxx-cpus-interface to xxx-cpus (remove "interface" from names)
> 
>   - rename accel_int to cpus_accel
> 
>   - rename CpusAccel functions from cpu_synchronize_* to synchronize_*
> 
> 
> --------
> 
> v2 -> v3:
> 
> * turned into a 4 patch series, adding a first patch moving
>   softmmu code currently in top_srcdir to softmmu/
> 
> * cpu-throttle: moved to softmmu/
> 
> * cpu-timers, icount:
> 
>   - moved to softmmu/
> 
>   - fixed assumption of qtest_enabled() => icount_enabled()
>   causing the failure of check-qtest-arm goal, in test-arm-mptimer.c
> 
>   Fix is in hw/core/ptimer.c,
> 
>   where the artificial timeout rate limit should not be applied
>   under qtest_enabled(), in a similar way to how it is not applied
>   for icount_enabled().
> 
> * CpuAccelInterface: no change.
> 
> 
> --------
> 
> 
> v1 -> v2:
> 
> * 1/3 (cpu-throttle): provide a description in the commit message
> 
> * 2/3 (cpu-timers, icount): in this v2 separate icount from cpu-timers,
>   as icount is actually TCG-specific. Only build it under CONFIG_TCG.
> 
>   To do this, qtest had to be detached from icount. To this end, a
>   trivial global counter for qtest has been introduced.
> 
> * 3/3 (CpuAccelInterface): provided a description.
> 
> This is point 8) in that plan. The idea is to extract the unrelated parts
> in cpus, and register interfaces from each single accelerator to the main
> cpus module (cpus.c).
> 
> While doing this RFC, I noticed some assumptions about Windows being
> either TCG or HAX (not considering WHPX) that might need to be revisited.
> I added a comment there.
> 
> The thing builds successfully based on Linux cross-compilations for
> windows/hax, windows/whpx, and I got a good build on Darwin/hvf.
> 
> Tests run successully for tcg and kvm configurations, but did not test on
> windows or darwin.
> 
> Welcome your feedback and help on this,
> 
> Claudio
> 
> Claudio Fontana (3):
>   softmmu: move softmmu only files from root
>   cpu-throttle: new module, extracted from cpus.c
>   cpu-timers, icount: new modules
> 
>  MAINTAINERS                                  |  15 +-
>  Makefile.target                              |   7 +-
>  accel/qtest.c                                |   6 +-
>  accel/tcg/cpu-exec.c                         |  43 +-
>  accel/tcg/tcg-all.c                          |   7 +-
>  accel/tcg/translate-all.c                    |   3 +-
>  dma-helpers.c                                |   4 +-
>  docs/replay.txt                              |   6 +-
>  exec.c                                       |   4 -
>  hw/core/ptimer.c                             |   8 +-
>  hw/i386/x86.c                                |   1 +
>  include/exec/cpu-all.h                       |   4 +
>  include/exec/exec-all.h                      |   4 +-
>  include/hw/core/cpu.h                        |  37 --
>  include/qemu/main-loop.h                     |   5 +
>  include/qemu/timer.h                         |  22 +-
>  include/sysemu/cpu-throttle.h                |  68 +++
>  include/sysemu/cpu-timers.h                  |  81 +++
>  include/sysemu/cpus.h                        |  12 +-
>  include/sysemu/qtest.h                       |   2 +
>  include/sysemu/replay.h                      |   4 +-
>  migration/migration.c                        |   1 +
>  migration/ram.c                              |   1 +
>  replay/replay.c                              |   6 +-
>  softmmu/Makefile.objs                        |  13 +
>  arch_init.c => softmmu/arch_init.c           |   0
>  balloon.c => softmmu/balloon.c               |   0
>  softmmu/cpu-throttle.c                       | 122 ++++
>  softmmu/cpu-timers.c                         | 284 +++++++++
>  cpus.c => softmmu/cpus.c                     | 839 +--------------------------
>  softmmu/icount.c                             | 499 ++++++++++++++++
>  ioport.c => softmmu/ioport.c                 |   0
>  memory.c => softmmu/memory.c                 |   0
>  memory_mapping.c => softmmu/memory_mapping.c |   0
>  qtest.c => softmmu/qtest.c                   |  34 +-
>  softmmu/timers-state.h                       |  69 +++
>  softmmu/vl.c                                 |   8 +-
>  stubs/Makefile.objs                          |   3 +-
>  stubs/clock-warp.c                           |   4 +-
>  stubs/cpu-get-clock.c                        |   3 +-
>  stubs/cpu-get-icount.c                       |  21 -
>  stubs/icount.c                               |  22 +
>  stubs/qemu-timer-notify-cb.c                 |   8 +
>  stubs/qtest.c                                |   5 +
>  target/alpha/translate.c                     |   3 +-
>  target/arm/helper.c                          |   7 +-
>  target/riscv/csr.c                           |   8 +-
>  tests/ptimer-test-stubs.c                    |   7 +-
>  tests/test-timed-average.c                   |   2 +-
>  util/main-loop.c                             |   4 +-
>  util/qemu-timer.c                            |  12 +-
>  51 files changed, 1343 insertions(+), 985 deletions(-)
>  create mode 100644 include/sysemu/cpu-throttle.h
>  create mode 100644 include/sysemu/cpu-timers.h
>  rename arch_init.c => softmmu/arch_init.c (100%)
>  rename balloon.c => softmmu/balloon.c (100%)
>  create mode 100644 softmmu/cpu-throttle.c
>  create mode 100644 softmmu/cpu-timers.c
>  rename cpus.c => softmmu/cpus.c (59%)
>  create mode 100644 softmmu/icount.c
>  rename ioport.c => softmmu/ioport.c (100%)
>  rename memory.c => softmmu/memory.c (100%)
>  rename memory_mapping.c => softmmu/memory_mapping.c (100%)
>  rename qtest.c => softmmu/qtest.c (95%)
>  create mode 100644 softmmu/timers-state.h
>  delete mode 100644 stubs/cpu-get-icount.c
>  create mode 100644 stubs/icount.c
>  create mode 100644 stubs/qemu-timer-notify-cb.c
> 



^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH 1/3] softmmu: move softmmu only files from root
  2020-06-29  9:35 ` [PATCH 1/3] softmmu: move softmmu only files from root Claudio Fontana
@ 2020-07-03 17:21   ` Paolo Bonzini
  0 siblings, 0 replies; 37+ messages in thread
From: Paolo Bonzini @ 2020-07-03 17:21 UTC (permalink / raw)
  To: Claudio Fontana, Thomas Huth, Laurent Vivier, Alex Bennée,
	Peter Maydell, Philippe Mathieu-Daudé,
	Roman Bolshakov, Markus Armbruster
  Cc: Eduardo Habkost, Marcelo Tosatti, qemu-devel, haxm-team,
	Wenchao Wang, Sunil Muthuswamy, Richard Henderson, Colin Xu

On 29/06/20 11:35, Claudio Fontana wrote:
> move arch_init, balloon, cpus, ioport, memory, memory_mapping, qtest.
> 
> They are all specific to CONFIG_SOFTMMU.
> 
> Signed-off-by: Claudio Fontana <cfontana@suse.de>
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> Reviewed-by: Laurent Vivier <lvivier@redhat.com>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> ---
>  MAINTAINERS                                  | 12 ++++++------
>  Makefile.target                              |  7 ++-----
>  softmmu/Makefile.objs                        | 10 ++++++++++
>  arch_init.c => softmmu/arch_init.c           |  0
>  balloon.c => softmmu/balloon.c               |  0
>  cpus.c => softmmu/cpus.c                     |  0
>  ioport.c => softmmu/ioport.c                 |  0
>  memory.c => softmmu/memory.c                 |  0
>  memory_mapping.c => softmmu/memory_mapping.c |  0
>  qtest.c => softmmu/qtest.c                   |  0
>  10 files changed, 18 insertions(+), 11 deletions(-)
>  rename arch_init.c => softmmu/arch_init.c (100%)
>  rename balloon.c => softmmu/balloon.c (100%)
>  rename cpus.c => softmmu/cpus.c (100%)
>  rename ioport.c => softmmu/ioport.c (100%)
>  rename memory.c => softmmu/memory.c (100%)
>  rename memory_mapping.c => softmmu/memory_mapping.c (100%)
>  rename qtest.c => softmmu/qtest.c (100%)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index dec252f38b..7214f59383 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -115,7 +115,7 @@ Overall TCG CPUs
>  M: Richard Henderson <rth@twiddle.net>
>  R: Paolo Bonzini <pbonzini@redhat.com>
>  S: Maintained
> -F: cpus.c
> +F: softmmu/cpus.c
>  F: cpus-common.c
>  F: exec.c
>  F: accel/tcg/
> @@ -1708,7 +1708,7 @@ M: David Hildenbrand <david@redhat.com>
>  S: Maintained
>  F: hw/virtio/virtio-balloon*.c
>  F: include/hw/virtio/virtio-balloon.h
> -F: balloon.c
> +F: softmmu/balloon.c
>  F: include/sysemu/balloon.h
>  
>  virtio-9p
> @@ -2177,12 +2177,12 @@ Memory API
>  M: Paolo Bonzini <pbonzini@redhat.com>
>  S: Supported
>  F: include/exec/ioport.h
> -F: ioport.c
>  F: include/exec/memop.h
>  F: include/exec/memory.h
>  F: include/exec/ram_addr.h
>  F: include/exec/ramblock.h
> -F: memory.c
> +F: softmmu/ioport.c
> +F: softmmu/memory.c
>  F: include/exec/memory-internal.h
>  F: exec.c
>  F: scripts/coccinelle/memory-region-housekeeping.cocci
> @@ -2214,13 +2214,13 @@ F: ui/cocoa.m
>  Main loop
>  M: Paolo Bonzini <pbonzini@redhat.com>
>  S: Maintained
> -F: cpus.c
>  F: include/qemu/main-loop.h
>  F: include/sysemu/runstate.h
>  F: util/main-loop.c
>  F: util/qemu-timer.c
>  F: softmmu/vl.c
>  F: softmmu/main.c
> +F: softmmu/cpus.c
>  F: qapi/run-state.json
>  
>  Human Monitor (HMP)
> @@ -2375,7 +2375,7 @@ M: Thomas Huth <thuth@redhat.com>
>  M: Laurent Vivier <lvivier@redhat.com>
>  R: Paolo Bonzini <pbonzini@redhat.com>
>  S: Maintained
> -F: qtest.c
> +F: softmmu/qtest.c
>  F: accel/qtest.c
>  F: tests/qtest/
>  X: tests/qtest/bios-tables-test-allowed-diff.h
> diff --git a/Makefile.target b/Makefile.target
> index 8ed1eba95b..7fbf5d8b92 100644
> --- a/Makefile.target
> +++ b/Makefile.target
> @@ -152,16 +152,13 @@ endif #CONFIG_BSD_USER
>  #########################################################
>  # System emulator target
>  ifdef CONFIG_SOFTMMU
> -obj-y += arch_init.o cpus.o gdbstub.o balloon.o ioport.o
> -obj-y += qtest.o
> +obj-y += softmmu/
> +obj-y += gdbstub.o
>  obj-y += dump/
>  obj-y += hw/
>  obj-y += monitor/
>  obj-y += qapi/
> -obj-y += memory.o
> -obj-y += memory_mapping.o
>  obj-y += migration/ram.o
> -obj-y += softmmu/
>  LIBS := $(libs_softmmu) $(LIBS)
>  
>  # Hardware support
> diff --git a/softmmu/Makefile.objs b/softmmu/Makefile.objs
> index dd15c24346..a4bd9f2f52 100644
> --- a/softmmu/Makefile.objs
> +++ b/softmmu/Makefile.objs
> @@ -1,3 +1,13 @@
>  softmmu-main-y = softmmu/main.o
> +
> +obj-y += arch_init.o
> +obj-y += cpus.o
> +obj-y += balloon.o
> +obj-y += ioport.o
> +obj-y += memory.o
> +obj-y += memory_mapping.o
> +
> +obj-y += qtest.o
> +
>  obj-y += vl.o
>  vl.o-cflags := $(GPROF_CFLAGS) $(SDL_CFLAGS)
> diff --git a/arch_init.c b/softmmu/arch_init.c
> similarity index 100%
> rename from arch_init.c
> rename to softmmu/arch_init.c
> diff --git a/balloon.c b/softmmu/balloon.c
> similarity index 100%
> rename from balloon.c
> rename to softmmu/balloon.c
> diff --git a/cpus.c b/softmmu/cpus.c
> similarity index 100%
> rename from cpus.c
> rename to softmmu/cpus.c
> diff --git a/ioport.c b/softmmu/ioport.c
> similarity index 100%
> rename from ioport.c
> rename to softmmu/ioport.c
> diff --git a/memory.c b/softmmu/memory.c
> similarity index 100%
> rename from memory.c
> rename to softmmu/memory.c
> diff --git a/memory_mapping.c b/softmmu/memory_mapping.c
> similarity index 100%
> rename from memory_mapping.c
> rename to softmmu/memory_mapping.c
> diff --git a/qtest.c b/softmmu/qtest.c
> similarity index 100%
> rename from qtest.c
> rename to softmmu/qtest.c
> 

Queued, thanks.  But I only got patch 1/3.

Paolo



^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH 0/3] QEMU cpus.c refactoring part1
  2020-07-02  6:27 ` [PATCH 0/3] QEMU cpus.c refactoring part1 Claudio Fontana
@ 2020-07-03 17:21   ` Paolo Bonzini
  0 siblings, 0 replies; 37+ messages in thread
From: Paolo Bonzini @ 2020-07-03 17:21 UTC (permalink / raw)
  To: Claudio Fontana, Alex Bennee, Markus Armbruster; +Cc: qemu-devel

On 02/07/20 08:27, Claudio Fontana wrote:
> Hi Alex, Markus, Paolo, 
> 
> maybe this could be queued in one of your queues?

Nevermind, Thunderbird was acting funny.  Queued all now.

Paolo

> Thanks a lot,
> 
> Claudio
> 
> On 6/29/20 11:35 AM, Claudio Fontana wrote:
>> Motivation and higher level steps:
>>
>> https://lists.gnu.org/archive/html/qemu-devel/2020-05/msg04628.html
>>
>> Previous series: [RFC RESEND v7 0/4] QEMU cpus.c refactoring
>>
>> This series is already reviewed, and is a split of the first three patches
>> from the previous series (RFC). The forth and last of the previous series
>> will then be posted separately.
>>
>> PREVIOUS DISCUSSIONS:
>>
>> * should we reorder patches or moves inside patches to avoid code going
>>   from cpus.c to softmmu/cpus.c and then again to softmmu/somethingelse.c ?
>>   (Philippe)
>>
>> * some questions about headers in include/softmmu (Philippe)
>>
>>
>> [SPLIT into TWO series, changed from RFC to PATCH]
>> ----
>>
>> v6 -> v7:
>>
>> * rebased changes on top of Pavel Dovgalyuk changes to dma-helpers.c
>>   "icount: make dma reads deterministic"
>>
>> ----
>>
>> v5 -> v6:
>>
>> * rebased changes on top of Emilio G. Cota changes to cpus.c
>>   "cpu: convert queued work to a QSIMPLEQ"
>>
>> * keep a pointer in cpus.c instead of a copy of CpusAccel
>>   (Alex)
>>
>> ----
>>
>>
>> v4 -> v5: rebase on latest master
>>
>> * rebased changes on top of roman series to remove one of the extra states for hvf.
>>   (Is the result now functional for HVF?)
>>
>> * rebased changes on top of icount changes and fixes to icount_configure and
>>   the new shift vmstate. (Markus)
>>
>> v3 -> v4:
>>
>> * overall: added copyright headers to all files that were missing them
>>   (used copyright and license of the module the stuff was extracted from).
>>   For the new interface files, added SUSE LLC.
>>
>> * 1/4 (move softmmu only files from root):
>>
>>   MAINTAINERS: moved softmmu/cpus.c to its final location (from patch 2)
>>
>> * 2/4 (cpu-throttle):
>>
>>   MAINTAINERS (to patch 1),
>>   copyright Fabrice Bellard and license from cpus.c
>>
>> * 3/4 (cpu-timers, icount):
>>
>>   - MAINTAINERS: add cpu-timers.c and icount.c to Paolo
>>
>>   - break very long lines (patchew)
>>
>>   - add copyright SUSE LLC, GPLv2 to cpu-timers.h
>>
>>   - add copyright Fabrice Bellard and license from cpus.c to timers-state.h
>>     as it is lifted from cpus.c
>>
>>   - vl.c: in configure_accelerators bail out if icount_enabled()
>>     and !tcg_enabled() as qtest does not enable icount anymore.
>>
>> * 4/4 (accel stuff to accel):
>>
>>   - add copyright SUSE LLC to files that mostly only consist of the
>>     new interface. Add whatever copyright was in the accelerator code
>>     if instead they mostly consist of accelerator code.
>>
>>   - change a comment to mention the result of the AccelClass experiment
>>
>>   - moved qtest accelerator into accel/qtest/ , make it like the others.
>>
>>   - rename xxx-cpus-interface to xxx-cpus (remove "interface" from names)
>>
>>   - rename accel_int to cpus_accel
>>
>>   - rename CpusAccel functions from cpu_synchronize_* to synchronize_*
>>
>>
>> --------
>>
>> v2 -> v3:
>>
>> * turned into a 4 patch series, adding a first patch moving
>>   softmmu code currently in top_srcdir to softmmu/
>>
>> * cpu-throttle: moved to softmmu/
>>
>> * cpu-timers, icount:
>>
>>   - moved to softmmu/
>>
>>   - fixed assumption of qtest_enabled() => icount_enabled()
>>   causing the failure of check-qtest-arm goal, in test-arm-mptimer.c
>>
>>   Fix is in hw/core/ptimer.c,
>>
>>   where the artificial timeout rate limit should not be applied
>>   under qtest_enabled(), in a similar way to how it is not applied
>>   for icount_enabled().
>>
>> * CpuAccelInterface: no change.
>>
>>
>> --------
>>
>>
>> v1 -> v2:
>>
>> * 1/3 (cpu-throttle): provide a description in the commit message
>>
>> * 2/3 (cpu-timers, icount): in this v2 separate icount from cpu-timers,
>>   as icount is actually TCG-specific. Only build it under CONFIG_TCG.
>>
>>   To do this, qtest had to be detached from icount. To this end, a
>>   trivial global counter for qtest has been introduced.
>>
>> * 3/3 (CpuAccelInterface): provided a description.
>>
>> This is point 8) in that plan. The idea is to extract the unrelated parts
>> in cpus, and register interfaces from each single accelerator to the main
>> cpus module (cpus.c).
>>
>> While doing this RFC, I noticed some assumptions about Windows being
>> either TCG or HAX (not considering WHPX) that might need to be revisited.
>> I added a comment there.
>>
>> The thing builds successfully based on Linux cross-compilations for
>> windows/hax, windows/whpx, and I got a good build on Darwin/hvf.
>>
>> Tests run successully for tcg and kvm configurations, but did not test on
>> windows or darwin.
>>
>> Welcome your feedback and help on this,
>>
>> Claudio
>>
>> Claudio Fontana (3):
>>   softmmu: move softmmu only files from root
>>   cpu-throttle: new module, extracted from cpus.c
>>   cpu-timers, icount: new modules
>>
>>  MAINTAINERS                                  |  15 +-
>>  Makefile.target                              |   7 +-
>>  accel/qtest.c                                |   6 +-
>>  accel/tcg/cpu-exec.c                         |  43 +-
>>  accel/tcg/tcg-all.c                          |   7 +-
>>  accel/tcg/translate-all.c                    |   3 +-
>>  dma-helpers.c                                |   4 +-
>>  docs/replay.txt                              |   6 +-
>>  exec.c                                       |   4 -
>>  hw/core/ptimer.c                             |   8 +-
>>  hw/i386/x86.c                                |   1 +
>>  include/exec/cpu-all.h                       |   4 +
>>  include/exec/exec-all.h                      |   4 +-
>>  include/hw/core/cpu.h                        |  37 --
>>  include/qemu/main-loop.h                     |   5 +
>>  include/qemu/timer.h                         |  22 +-
>>  include/sysemu/cpu-throttle.h                |  68 +++
>>  include/sysemu/cpu-timers.h                  |  81 +++
>>  include/sysemu/cpus.h                        |  12 +-
>>  include/sysemu/qtest.h                       |   2 +
>>  include/sysemu/replay.h                      |   4 +-
>>  migration/migration.c                        |   1 +
>>  migration/ram.c                              |   1 +
>>  replay/replay.c                              |   6 +-
>>  softmmu/Makefile.objs                        |  13 +
>>  arch_init.c => softmmu/arch_init.c           |   0
>>  balloon.c => softmmu/balloon.c               |   0
>>  softmmu/cpu-throttle.c                       | 122 ++++
>>  softmmu/cpu-timers.c                         | 284 +++++++++
>>  cpus.c => softmmu/cpus.c                     | 839 +--------------------------
>>  softmmu/icount.c                             | 499 ++++++++++++++++
>>  ioport.c => softmmu/ioport.c                 |   0
>>  memory.c => softmmu/memory.c                 |   0
>>  memory_mapping.c => softmmu/memory_mapping.c |   0
>>  qtest.c => softmmu/qtest.c                   |  34 +-
>>  softmmu/timers-state.h                       |  69 +++
>>  softmmu/vl.c                                 |   8 +-
>>  stubs/Makefile.objs                          |   3 +-
>>  stubs/clock-warp.c                           |   4 +-
>>  stubs/cpu-get-clock.c                        |   3 +-
>>  stubs/cpu-get-icount.c                       |  21 -
>>  stubs/icount.c                               |  22 +
>>  stubs/qemu-timer-notify-cb.c                 |   8 +
>>  stubs/qtest.c                                |   5 +
>>  target/alpha/translate.c                     |   3 +-
>>  target/arm/helper.c                          |   7 +-
>>  target/riscv/csr.c                           |   8 +-
>>  tests/ptimer-test-stubs.c                    |   7 +-
>>  tests/test-timed-average.c                   |   2 +-
>>  util/main-loop.c                             |   4 +-
>>  util/qemu-timer.c                            |  12 +-
>>  51 files changed, 1343 insertions(+), 985 deletions(-)
>>  create mode 100644 include/sysemu/cpu-throttle.h
>>  create mode 100644 include/sysemu/cpu-timers.h
>>  rename arch_init.c => softmmu/arch_init.c (100%)
>>  rename balloon.c => softmmu/balloon.c (100%)
>>  create mode 100644 softmmu/cpu-throttle.c
>>  create mode 100644 softmmu/cpu-timers.c
>>  rename cpus.c => softmmu/cpus.c (59%)
>>  create mode 100644 softmmu/icount.c
>>  rename ioport.c => softmmu/ioport.c (100%)
>>  rename memory.c => softmmu/memory.c (100%)
>>  rename memory_mapping.c => softmmu/memory_mapping.c (100%)
>>  rename qtest.c => softmmu/qtest.c (95%)
>>  create mode 100644 softmmu/timers-state.h
>>  delete mode 100644 stubs/cpu-get-icount.c
>>  create mode 100644 stubs/icount.c
>>  create mode 100644 stubs/qemu-timer-notify-cb.c
>>
> 



^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH 3/3] cpu-timers, icount: new modules
  2020-06-29  9:35 ` [PATCH 3/3] cpu-timers, icount: new modules Claudio Fontana
@ 2020-07-08 14:34   ` Paolo Bonzini
  2020-07-08 15:00     ` Claudio Fontana
  0 siblings, 1 reply; 37+ messages in thread
From: Paolo Bonzini @ 2020-07-08 14:34 UTC (permalink / raw)
  To: Claudio Fontana, Thomas Huth, Laurent Vivier, Alex Bennée,
	Peter Maydell, Philippe Mathieu-Daudé,
	Roman Bolshakov, Markus Armbruster
  Cc: Eduardo Habkost, Colin Xu, Marcelo Tosatti, qemu-devel,
	haxm-team, Wenchao Wang, Sunil Muthuswamy, Richard Henderson

On 29/06/20 11:35, Claudio Fontana wrote:
> refactoring of cpus.c continues with cpu timer state extraction.
> 
> cpu-timers: responsible for the cpu timers state, and for access to
> 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 misuses icount to warp time
> (qtest_clock_warp). In order to solve this problem, detach instead qtest
> from icount, and use a trivial separate counter for it.
> 
> This requires fixing assumptions scattered in the code that
> qtest_enabled() implies icount_enabled().
> 
> No functionality change.
> 
> Signed-off-by: Claudio Fontana <cfontana@suse.de>
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

Claudio,

this weirdly enough causes iotest 267 (i.e. basically vmstate
save/restore) to break on s390:

+Unexpected storage key flag data: 0
+error while loading state for instance 0x0 of device 's390-skeys'
+Error: Error -22 while loading VM state

Bisectable, 100% failure rate, etc. :(  Can you split the patch in
multiple parts, specifically separating any rename or introducing of
includes from the final file move?

Also, the patch breaks --disable-tcg, which is easily fixed by changing
the prototype for icount_enabled() to

	#if defined CONFIG_TCG || !defined NEED_CPU_H
	extern bool icount_enabled(void);
	#else
	#define icount_enabled() 0
	#endif

(This way, more TCG-only code in cpus.c gets elided).  You can integrate
this change in the next version.

Paolo



^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH 3/3] cpu-timers, icount: new modules
  2020-07-08 14:34   ` Paolo Bonzini
@ 2020-07-08 15:00     ` Claudio Fontana
  2020-07-08 15:05       ` Paolo Bonzini
  0 siblings, 1 reply; 37+ messages in thread
From: Claudio Fontana @ 2020-07-08 15:00 UTC (permalink / raw)
  To: Paolo Bonzini, Thomas Huth, Laurent Vivier, Alex Bennée,
	Peter Maydell, Philippe Mathieu-Daudé,
	Roman Bolshakov, Markus Armbruster
  Cc: Eduardo Habkost, Colin Xu, Marcelo Tosatti, qemu-devel,
	haxm-team, Wenchao Wang, Sunil Muthuswamy, Richard Henderson

On 7/8/20 4:34 PM, Paolo Bonzini wrote:
> On 29/06/20 11:35, Claudio Fontana wrote:
>> refactoring of cpus.c continues with cpu timer state extraction.
>>
>> cpu-timers: responsible for the cpu timers state, and for access to
>> 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 misuses icount to warp time
>> (qtest_clock_warp). In order to solve this problem, detach instead qtest
>> from icount, and use a trivial separate counter for it.
>>
>> This requires fixing assumptions scattered in the code that
>> qtest_enabled() implies icount_enabled().
>>
>> No functionality change.
>>
>> Signed-off-by: Claudio Fontana <cfontana@suse.de>
>> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> 
> Claudio,
> 
> this weirdly enough causes iotest 267 (i.e. basically vmstate
> save/restore) to break on s390:
> 
> +Unexpected storage key flag data: 0
> +error while loading state for instance 0x0 of device 's390-skeys'
> +Error: Error -22 while loading VM state
> 
> Bisectable, 100% failure rate, etc. :(  Can you split the patch in
> multiple parts, specifically separating any rename or introducing of
> includes from the final file move?

Hi Paolo,

will take a look!

Is this captured by some travis / cirrus-ci / anything I can easily see the result of?


> 
> Also, the patch breaks --disable-tcg, which is easily fixed by changing
> the prototype for icount_enabled() to
> 
> 	#if defined CONFIG_TCG || !defined NEED_CPU_H
> 	extern bool icount_enabled(void);
> 	#else
> 	#define icount_enabled() 0
> 	#endif
> 
> (This way, more TCG-only code in cpus.c gets elided).  You can integrate
> this change in the next version.
> 
> Paolo
> 

Weird, I tested with --disable-tcg explicitly (but may be some time ago now, as I constantly rebased).

Will take a look at the introduction of this #defines in place of variables,
as this mechanisms will not work in the future for target-specific modules.

Ciao, will let you know what I find,

Claudio




^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH 3/3] cpu-timers, icount: new modules
  2020-07-08 15:00     ` Claudio Fontana
@ 2020-07-08 15:05       ` Paolo Bonzini
  2020-07-08 15:07         ` Thomas Huth
                           ` (2 more replies)
  0 siblings, 3 replies; 37+ messages in thread
From: Paolo Bonzini @ 2020-07-08 15:05 UTC (permalink / raw)
  To: Claudio Fontana, Thomas Huth, Laurent Vivier, Alex Bennée,
	Peter Maydell, Philippe Mathieu-Daudé,
	Roman Bolshakov, Markus Armbruster
  Cc: Eduardo Habkost, Colin Xu, Marcelo Tosatti, qemu-devel,
	haxm-team, Wenchao Wang, Sunil Muthuswamy, Richard Henderson

On 08/07/20 17:00, Claudio Fontana wrote:
>> Bisectable, 100% failure rate, etc. :(  Can you split the patch in
>> multiple parts, specifically separating any rename or introducing of
>> includes from the final file move?
> Hi Paolo,
> 
> will take a look!
> 
> Is this captured by some travis / cirrus-ci / anything I can easily see the result of?
> 
> 

Nope, unfortunately we don't have an s390 CI.  But if you can get your
hands on one, just "./configure --target-list=s390x-softmmu && make &&
make check-block" will show it.

>>
>> 	#if defined CONFIG_TCG || !defined NEED_CPU_H
>> 	extern bool icount_enabled(void);
>> 	#else
>> 	#define icount_enabled() 0
>> 	#endif
>>
>> (This way, more TCG-only code in cpus.c gets elided).  You can integrate
>> this change in the next version.
>>
>> Paolo
>>
> 
> Weird, I tested with --disable-tcg explicitly (but may be some time ago now, as I constantly rebased).
> 
> Will take a look at the introduction of this #defines in place of variables,
> as this mechanisms will not work in the future for target-specific modules.

This is only done for per-target files so it should not be a problem.

Paolo



^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH 3/3] cpu-timers, icount: new modules
  2020-07-08 15:05       ` Paolo Bonzini
@ 2020-07-08 15:07         ` Thomas Huth
  2020-07-08 15:12           ` Paolo Bonzini
  2020-07-08 15:17         ` Claudio Fontana
  2020-07-09 18:38         ` Claudio Fontana
  2 siblings, 1 reply; 37+ messages in thread
From: Thomas Huth @ 2020-07-08 15:07 UTC (permalink / raw)
  To: Paolo Bonzini, Claudio Fontana, Laurent Vivier, Alex Bennée,
	Peter Maydell, Philippe Mathieu-Daudé,
	Roman Bolshakov, Markus Armbruster
  Cc: Eduardo Habkost, Colin Xu, Marcelo Tosatti, qemu-devel,
	haxm-team, Wenchao Wang, Sunil Muthuswamy, Richard Henderson

On 08/07/2020 17.05, Paolo Bonzini wrote:
> On 08/07/20 17:00, Claudio Fontana wrote:
>>> Bisectable, 100% failure rate, etc. :(  Can you split the patch in
>>> multiple parts, specifically separating any rename or introducing of
>>> includes from the final file move?
>> Hi Paolo,
>>
>> will take a look!
>>
>> Is this captured by some travis / cirrus-ci / anything I can easily see the result of?
>>
>>
> 
> Nope, unfortunately we don't have an s390 CI.  But if you can get your
> hands on one, just "./configure --target-list=s390x-softmmu && make &&
> make check-block" will show it.

We've got a s390x builder on Travis ... or is this only about the s390x
target?

 Thomas



^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH 3/3] cpu-timers, icount: new modules
  2020-07-08 15:07         ` Thomas Huth
@ 2020-07-08 15:12           ` Paolo Bonzini
  2020-07-08 15:15             ` Claudio Fontana
  2020-07-08 15:15             ` Thomas Huth
  0 siblings, 2 replies; 37+ messages in thread
From: Paolo Bonzini @ 2020-07-08 15:12 UTC (permalink / raw)
  To: Thomas Huth, Claudio Fontana, Laurent Vivier, Alex Bennée,
	Peter Maydell, Philippe Mathieu-Daudé,
	Roman Bolshakov, Markus Armbruster
  Cc: Eduardo Habkost, Colin Xu, Marcelo Tosatti, qemu-devel,
	haxm-team, Wenchao Wang, Sunil Muthuswamy, Richard Henderson

On 08/07/20 17:07, Thomas Huth wrote:
>> Nope, unfortunately we don't have an s390 CI.  But if you can get your
>> hands on one, just "./configure --target-list=s390x-softmmu && make &&
>> make check-block" will show it.
>
> We've got a s390x builder on Travis ... or is this only about the s390x
> target?

It is about s390 host.  But Travis is not covering all submitted
patches, is it?

Paolo



^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH 3/3] cpu-timers, icount: new modules
  2020-07-08 15:12           ` Paolo Bonzini
@ 2020-07-08 15:15             ` Claudio Fontana
  2020-07-08 15:15             ` Thomas Huth
  1 sibling, 0 replies; 37+ messages in thread
From: Claudio Fontana @ 2020-07-08 15:15 UTC (permalink / raw)
  To: Paolo Bonzini, Thomas Huth, Laurent Vivier, Alex Bennée,
	Peter Maydell, Philippe Mathieu-Daudé,
	Roman Bolshakov, Markus Armbruster
  Cc: Eduardo Habkost, Colin Xu, Marcelo Tosatti, qemu-devel,
	haxm-team, Wenchao Wang, Sunil Muthuswamy, Richard Henderson

On 7/8/20 5:12 PM, Paolo Bonzini wrote:
> On 08/07/20 17:07, Thomas Huth wrote:
>>> Nope, unfortunately we don't have an s390 CI.  But if you can get your
>>> hands on one, just "./configure --target-list=s390x-softmmu && make &&
>>> make check-block" will show it.
>>
>> We've got a s390x builder on Travis ... or is this only about the s390x
>> target?
> 
> It is about s390 host.  But Travis is not covering all submitted
> patches, is it?
> 
> Paolo
> 

Probably something in-flight (in the queues), because for me it was all green for [s390] on travis,
supposing that the tests run as part of travis CI include check-block.

Thanks,

CLaudio 


^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH 3/3] cpu-timers, icount: new modules
  2020-07-08 15:12           ` Paolo Bonzini
  2020-07-08 15:15             ` Claudio Fontana
@ 2020-07-08 15:15             ` Thomas Huth
  1 sibling, 0 replies; 37+ messages in thread
From: Thomas Huth @ 2020-07-08 15:15 UTC (permalink / raw)
  To: Paolo Bonzini, Claudio Fontana, Laurent Vivier, Alex Bennée,
	Peter Maydell, Philippe Mathieu-Daudé,
	Roman Bolshakov, Markus Armbruster
  Cc: Eduardo Habkost, Colin Xu, Marcelo Tosatti, qemu-devel,
	haxm-team, Wenchao Wang, Sunil Muthuswamy, Richard Henderson

On 08/07/2020 17.12, Paolo Bonzini wrote:
> On 08/07/20 17:07, Thomas Huth wrote:
>>> Nope, unfortunately we don't have an s390 CI.  But if you can get your
>>> hands on one, just "./configure --target-list=s390x-softmmu && make &&
>>> make check-block" will show it.
>>
>> We've got a s390x builder on Travis ... or is this only about the s390x
>> target?
> 
> It is about s390 host.  But Travis is not covering all submitted
> patches, is it?

No, but everybody can enable it for their github repo if they like.

 Thomas



^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH 3/3] cpu-timers, icount: new modules
  2020-07-08 15:05       ` Paolo Bonzini
  2020-07-08 15:07         ` Thomas Huth
@ 2020-07-08 15:17         ` Claudio Fontana
  2020-07-08 15:23           ` Paolo Bonzini
  2020-07-09 18:38         ` Claudio Fontana
  2 siblings, 1 reply; 37+ messages in thread
From: Claudio Fontana @ 2020-07-08 15:17 UTC (permalink / raw)
  To: Paolo Bonzini, Thomas Huth, Laurent Vivier, Alex Bennée,
	Peter Maydell, Philippe Mathieu-Daudé,
	Roman Bolshakov, Markus Armbruster
  Cc: Eduardo Habkost, Colin Xu, Marcelo Tosatti, qemu-devel,
	haxm-team, Wenchao Wang, Sunil Muthuswamy, Richard Henderson

On 7/8/20 5:05 PM, Paolo Bonzini wrote:
> On 08/07/20 17:00, Claudio Fontana wrote:
>>> Bisectable, 100% failure rate, etc. :(  Can you split the patch in
>>> multiple parts, specifically separating any rename or introducing of
>>> includes from the final file move?
>> Hi Paolo,
>>
>> will take a look!
>>
>> Is this captured by some travis / cirrus-ci / anything I can easily see the result of?
>>
>>
> 
> Nope, unfortunately we don't have an s390 CI.  But if you can get your
> hands on one, just "./configure --target-list=s390x-softmmu && make &&
> make check-block" will show it.
> 
>>>
>>> 	#if defined CONFIG_TCG || !defined NEED_CPU_H
>>> 	extern bool icount_enabled(void);
>>> 	#else
>>> 	#define icount_enabled() 0
>>> 	#endif
>>>
>>> (This way, more TCG-only code in cpus.c gets elided).  You can integrate
>>> this change in the next version.
>>>
>>> Paolo
>>>
>>
>> Weird, I tested with --disable-tcg explicitly (but may be some time ago now, as I constantly rebased).
>>
>> Will take a look at the introduction of this #defines in place of variables,
>> as this mechanisms will not work in the future for target-specific modules.
> 
> This is only done for per-target files so it should not be a problem.
> 
> Paolo
> 

K, I tried with latest master, disable-tcg build still works for me on x86, so it is something in queue I guess?

Thanks,

C




^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH 3/3] cpu-timers, icount: new modules
  2020-07-08 15:17         ` Claudio Fontana
@ 2020-07-08 15:23           ` Paolo Bonzini
  2020-07-08 15:30             ` Claudio Fontana
  0 siblings, 1 reply; 37+ messages in thread
From: Paolo Bonzini @ 2020-07-08 15:23 UTC (permalink / raw)
  To: Claudio Fontana, Thomas Huth, Laurent Vivier, Alex Bennée,
	Peter Maydell, Philippe Mathieu-Daudé,
	Roman Bolshakov, Markus Armbruster
  Cc: Eduardo Habkost, Colin Xu, Marcelo Tosatti, qemu-devel,
	haxm-team, Wenchao Wang, Sunil Muthuswamy, Richard Henderson

On 08/07/20 17:17, Claudio Fontana wrote:
> On 7/8/20 5:05 PM, Paolo Bonzini wrote:
>> On 08/07/20 17:00, Claudio Fontana wrote:
>>>> Bisectable, 100% failure rate, etc. :(  Can you split the patch in
>>>> multiple parts, specifically separating any rename or introducing of
>>>> includes from the final file move?
>>> Hi Paolo,
>>>
>>> will take a look!
>>>
>>> Is this captured by some travis / cirrus-ci / anything I can easily see the result of?
>>>
>>>
>>
>> Nope, unfortunately we don't have an s390 CI.  But if you can get your
>> hands on one, just "./configure --target-list=s390x-softmmu && make &&
>> make check-block" will show it.
>>
>>>>
>>>> 	#if defined CONFIG_TCG || !defined NEED_CPU_H
>>>> 	extern bool icount_enabled(void);
>>>> 	#else
>>>> 	#define icount_enabled() 0
>>>> 	#endif
>>>>
>>>> (This way, more TCG-only code in cpus.c gets elided).  You can integrate
>>>> this change in the next version.
>>>>
>>>> Paolo
>>>>
>>>
>>> Weird, I tested with --disable-tcg explicitly (but may be some time ago now, as I constantly rebased).
>>>
>>> Will take a look at the introduction of this #defines in place of variables,
>>> as this mechanisms will not work in the future for target-specific modules.
>>
>> This is only done for per-target files so it should not be a problem.
>>
>> Paolo
>>
> 
> K, I tried with latest master, disable-tcg build still works for me on x86, so it is something in queue I guess?

Peter reported the issue in the v1 pull request; there were two
different breakages but the cpus.c one was yours.

Paolo



^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH 3/3] cpu-timers, icount: new modules
  2020-07-08 15:23           ` Paolo Bonzini
@ 2020-07-08 15:30             ` Claudio Fontana
  0 siblings, 0 replies; 37+ messages in thread
From: Claudio Fontana @ 2020-07-08 15:30 UTC (permalink / raw)
  To: Paolo Bonzini, Thomas Huth, Laurent Vivier, Alex Bennée,
	Peter Maydell, Philippe Mathieu-Daudé,
	Roman Bolshakov, Markus Armbruster
  Cc: Eduardo Habkost, Colin Xu, Marcelo Tosatti, qemu-devel,
	haxm-team, Wenchao Wang, Sunil Muthuswamy, Richard Henderson

On 7/8/20 5:23 PM, Paolo Bonzini wrote:
> On 08/07/20 17:17, Claudio Fontana wrote:
>> On 7/8/20 5:05 PM, Paolo Bonzini wrote:
>>> On 08/07/20 17:00, Claudio Fontana wrote:
>>>>> Bisectable, 100% failure rate, etc. :(  Can you split the patch in
>>>>> multiple parts, specifically separating any rename or introducing of
>>>>> includes from the final file move?
>>>> Hi Paolo,
>>>>
>>>> will take a look!
>>>>
>>>> Is this captured by some travis / cirrus-ci / anything I can easily see the result of?
>>>>
>>>>
>>>
>>> Nope, unfortunately we don't have an s390 CI.  But if you can get your
>>> hands on one, just "./configure --target-list=s390x-softmmu && make &&
>>> make check-block" will show it.
>>>
>>>>>
>>>>> 	#if defined CONFIG_TCG || !defined NEED_CPU_H
>>>>> 	extern bool icount_enabled(void);
>>>>> 	#else
>>>>> 	#define icount_enabled() 0
>>>>> 	#endif
>>>>>
>>>>> (This way, more TCG-only code in cpus.c gets elided).  You can integrate
>>>>> this change in the next version.
>>>>>
>>>>> Paolo
>>>>>
>>>>
>>>> Weird, I tested with --disable-tcg explicitly (but may be some time ago now, as I constantly rebased).
>>>>
>>>> Will take a look at the introduction of this #defines in place of variables,
>>>> as this mechanisms will not work in the future for target-specific modules.
>>>
>>> This is only done for per-target files so it should not be a problem.
>>>
>>> Paolo
>>>
>>
>> K, I tried with latest master, disable-tcg build still works for me on x86, so it is something in queue I guess?
> 
> Peter reported the issue in the v1 pull request; there were two
> different breakages but the cpus.c one was yours.
> 
> Paolo
> 

I see, since I don't manage to reproduce the problem at the moment, I will wait for master to be updated, after which I expect the issue to become apparent.

As of now, I do

configure --disable-tcg
make -j120

which works for me, based on latest master.

Ciao,

C


^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH 3/3] cpu-timers, icount: new modules
  2020-07-08 15:05       ` Paolo Bonzini
  2020-07-08 15:07         ` Thomas Huth
  2020-07-08 15:17         ` Claudio Fontana
@ 2020-07-09 18:38         ` Claudio Fontana
  2020-07-09 18:46           ` Claudio Fontana
  2020-07-10  4:36           ` Thomas Huth
  2 siblings, 2 replies; 37+ messages in thread
From: Claudio Fontana @ 2020-07-09 18:38 UTC (permalink / raw)
  To: Paolo Bonzini, Thomas Huth, Laurent Vivier, Alex Bennée,
	Peter Maydell, Philippe Mathieu-Daudé,
	Roman Bolshakov, Markus Armbruster
  Cc: Eduardo Habkost, Marcelo Tosatti, qemu-devel, haxm-team,
	Wenchao Wang, Sunil Muthuswamy, Richard Henderson, Colin Xu

On 7/8/20 5:05 PM, Paolo Bonzini wrote:
> On 08/07/20 17:00, Claudio Fontana wrote:
>>> Bisectable, 100% failure rate, etc. :(  Can you split the patch in
>>> multiple parts, specifically separating any rename or introducing of
>>> includes from the final file move?
>> Hi Paolo,
>>
>> will take a look!
>>
>> Is this captured by some travis / cirrus-ci / anything I can easily see the result of?
>>
>>
> 
> Nope, unfortunately we don't have an s390 CI.  But if you can get your
> hands on one, just "./configure --target-list=s390x-softmmu && make &&
> make check-block" will show it.

So this is tricky, but I am making some progress after getting my hands on one.
Maybe if someone understands s390 keys better, I could be clued in.

In short this goes away if I again set icount to enabled for qtest,
basically ensuring that --enable-tcg is there and then reenabling icount.

qtest was forcing icount and shift=0 by creating qemu options, in order to misuse its counter feature,
instead of using a separate counter.

Removing that ugliness we end up with different behavior of save/load, because vmstate will now suddenly not contain icount-related values anymore.
What I do not understand is why this causes a problem because save should just not store the icount state and load should just not load the icount state,
and why we die on the load of s390 keys state (it works just fine for other architectures).

Here is a diff that makes the problem disappear, but needs --enable-tcg:


----------------------------------------------------------------------------------------------------
diff --git a/accel/qtest.c b/accel/qtest.c
index 119d0f16a4..4cb16abc2c 100644
--- a/accel/qtest.c
+++ b/accel/qtest.c
@@ -23,6 +23,12 @@
 
 static int qtest_init_accel(MachineState *ms)
 {
+    QemuOpts *opts = qemu_opts_create(qemu_find_opts("icount"), NULL, 0,
+                                      &error_abort);
+    qemu_opt_set(opts, "shift", "0", &error_abort);
+    icount_configure(opts, &error_abort);
+    qemu_opts_del(opts);
+
     return 0;
 }
 
diff --git a/softmmu/vl.c b/softmmu/vl.c
index f39fd5270b..a5e788c86a 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -2786,10 +2786,12 @@ static void configure_accelerators(const char *progname)
         error_report("falling back to %s", ac->name);
     }
 
+    /*
     if (icount_enabled() && !tcg_enabled()) {
         error_report("-icount is not allowed with hardware virtualization");
         exit(1);
    }
+    */
 }
 
 static void create_default_memdev(MachineState *ms, const char *path)
----------------------------------------------------------------------------------------------------

Without this patch, here is the full failure, maybe someone has a good hint, otherwise I'll keep digging from here inside the s390-specific code.

QA output created by 267

=== No block devices at all ===

Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
Testing:
QEMU X.Y.Z monitor - type 'help' for more information
(qemu) savevm snap0
Error: No block device can accept snapshots
(qemu) info snapshots
No available block device supports snapshots
(qemu) loadvm snap0
Error: No block device supports snapshots
(qemu) quit


=== -drive if=none ===

Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
Testing: -drive driver=file,file=TEST_DIR/t.IMGFMT,if=none
QEMU X.Y.Z monitor - type 'help' for more information
(qemu) savevm snap0
Error: Device 'none0' is writable but does not support snapshots
(qemu) info snapshots
No available block device supports snapshots
(qemu) loadvm snap0
Error: Device 'none0' is writable but does not support snapshots
(qemu) quit

Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
Testing: -drive driver=IMGFMT,file=TEST_DIR/t.IMGFMT,if=none
QEMU X.Y.Z monitor - type 'help' for more information
(qemu) savevm snap0
(qemu) info snapshots
List of snapshots present on all disks:
ID        TAG                     VM SIZE                DATE       VM CLOCK
--        snap0                      SIZE yyyy-mm-dd hh:mm:ss   00:00:00.000
(qemu) loadvm snap0
(qemu) quit

Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
Testing: -drive driver=IMGFMT,file=TEST_DIR/t.IMGFMT,if=none -device virtio-blk,drive=none0
QEMU X.Y.Z monitor - type 'help' for more information
(qemu) savevm snap0
(qemu) info snapshots
List of snapshots present on all disks:
ID        TAG                     VM SIZE                DATE       VM CLOCK
--        snap0                      SIZE yyyy-mm-dd hh:mm:ss   00:00:00.000
(qemu) loadvm snap0
(qemu) quit


=== -drive if=virtio ===

Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
Testing: -drive driver=file,file=TEST_DIR/t.IMGFMT,if=virtio
QEMU X.Y.Z monitor - type 'help' for more information
(qemu) savevm snap0
Error: Device 'virtio0' is writable but does not support snapshots
(qemu) info snapshots
No available block device supports snapshots
(qemu) loadvm snap0
Error: Device 'virtio0' is writable but does not support snapshots
(qemu) quit

Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
Testing: -drive driver=IMGFMT,file=TEST_DIR/t.IMGFMT,if=virtio
QEMU X.Y.Z monitor - type 'help' for more information
(qemu) savevm snap0
(qemu) info snapshots
List of snapshots present on all disks:
ID        TAG                     VM SIZE                DATE       VM CLOCK
--        snap0                      SIZE yyyy-mm-dd hh:mm:ss   00:00:00.000
(qemu) loadvm snap0
(qemu) quit


=== Simple -blockdev ===

Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
Testing: -blockdev driver=file,filename=TEST_DIR/t.IMGFMT,node-name=file
QEMU X.Y.Z monitor - type 'help' for more information
(qemu) savevm snap0
Error: Device '' is writable but does not support snapshots
(qemu) info snapshots
No available block device supports snapshots
(qemu) loadvm snap0
Error: Device '' is writable but does not support snapshots
(qemu) quit

Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
Testing: -blockdev driver=file,filename=TEST_DIR/t.IMGFMT,node-name=file -blockdev driver=IMGFMT,file=file,node-name=fmt
QEMU X.Y.Z monitor - type 'help' for more information
(qemu) savevm snap0
(qemu) info snapshots
List of snapshots present on all disks:
ID        TAG                     VM SIZE                DATE       VM CLOCK
--        snap0                      SIZE yyyy-mm-dd hh:mm:ss   00:00:00.000
(qemu) loadvm snap0
(qemu) quit

Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
Testing: -blockdev driver=file,filename=TEST_DIR/t.IMGFMT,node-name=file -blockdev driver=raw,file=file,node-name=raw -blockdev driver=IMGFMT,file=raw,node-name=fmt
QEMU X.Y.Z monitor - type 'help' for more information
(qemu) savevm snap0
(qemu) info snapshots
List of snapshots present on all disks:
ID        TAG                     VM SIZE                DATE       VM CLOCK
--        snap0                      SIZE yyyy-mm-dd hh:mm:ss   00:00:00.000
(qemu) loadvm snap0
(qemu) quit


=== -blockdev with a filter on top ===

Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
Testing: -blockdev driver=file,filename=TEST_DIR/t.IMGFMT,node-name=file -blockdev driver=IMGFMT,file=file,node-name=fmt -blockdev driver=copy-on-read,file=fmt,node-name=filter
QEMU X.Y.Z monitor - type 'help' for more information
(qemu) savevm snap0
(qemu) info snapshots
List of snapshots present on all disks:
ID        TAG                     VM SIZE                DATE       VM CLOCK
--        snap0                      SIZE yyyy-mm-dd hh:mm:ss   00:00:00.000
(qemu) loadvm snap0
(qemu) quit


=== -blockdev with a backing file ===

Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=134217728
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 backing_file=TEST_DIR/t.IMGFMT.base
Testing: -blockdev driver=file,filename=TEST_DIR/t.IMGFMT.base,node-name=backing-file -blockdev driver=file,filename=TEST_DIR/t.IMGFMT,node-name=file -blockdev driver=IMGFMT,file=file,backing=backing-file,node-name=fmt
QEMU X.Y.Z monitor - type 'help' for more information
(qemu) savevm snap0
(qemu) info snapshots
List of snapshots present on all disks:
ID        TAG                     VM SIZE                DATE       VM CLOCK
--        snap0                      SIZE yyyy-mm-dd hh:mm:ss   00:00:00.000
(qemu) loadvm snap0
Unexpected storage key flag data: 0
error while loading state for instance 0x0 of device 's390-skeys'
Error: Error -22 while loading VM state




> 
>>>
>>> 	#if defined CONFIG_TCG || !defined NEED_CPU_H
>>> 	extern bool icount_enabled(void);
>>> 	#else
>>> 	#define icount_enabled() 0
>>> 	#endif
>>>
>>> (This way, more TCG-only code in cpus.c gets elided).  You can integrate
>>> this change in the next version.
>>>
>>> Paolo
>>>
>>
>> Weird, I tested with --disable-tcg explicitly (but may be some time ago now, as I constantly rebased).
>>
>> Will take a look at the introduction of this #defines in place of variables,
>> as this mechanisms will not work in the future for target-specific modules.
> 
> This is only done for per-target files so it should not be a problem.
> 
> Paolo
> 
> 



^ permalink raw reply related	[flat|nested] 37+ messages in thread

* Re: [PATCH 3/3] cpu-timers, icount: new modules
  2020-07-09 18:38         ` Claudio Fontana
@ 2020-07-09 18:46           ` Claudio Fontana
  2020-07-10  6:33             ` Cornelia Huck
  2020-07-10  4:36           ` Thomas Huth
  1 sibling, 1 reply; 37+ messages in thread
From: Claudio Fontana @ 2020-07-09 18:46 UTC (permalink / raw)
  To: Paolo Bonzini, Thomas Huth, Laurent Vivier, Alex Bennée,
	Peter Maydell, Philippe Mathieu-Daudé,
	Roman Bolshakov, Markus Armbruster
  Cc: Cornelia Huck, Eduardo Habkost, Colin Xu, Marcelo Tosatti,
	qemu-devel, haxm-team, Wenchao Wang, Sunil Muthuswamy,
	Richard Henderson

On 7/9/20 8:38 PM, Claudio Fontana wrote:
> On 7/8/20 5:05 PM, Paolo Bonzini wrote:
>> On 08/07/20 17:00, Claudio Fontana wrote:
>>>> Bisectable, 100% failure rate, etc. :(  Can you split the patch in
>>>> multiple parts, specifically separating any rename or introducing of
>>>> includes from the final file move?
>>> Hi Paolo,
>>>
>>> will take a look!
>>>
>>> Is this captured by some travis / cirrus-ci / anything I can easily see the result of?
>>>
>>>
>>
>> Nope, unfortunately we don't have an s390 CI.  But if you can get your
>> hands on one, just "./configure --target-list=s390x-softmmu && make &&
>> make check-block" will show it.
> 
> So this is tricky, but I am making some progress after getting my hands on one.
> Maybe if someone understands s390 keys better, I could be clued in.


Also adding Cornelia to Cc:.

Maybe the savevm_s390_storage_keys SaveVMHandlers etc assume that the icount state part of the vmstate is there?


> 
> In short this goes away if I again set icount to enabled for qtest,
> basically ensuring that --enable-tcg is there and then reenabling icount.
> 
> qtest was forcing icount and shift=0 by creating qemu options, in order to misuse its counter feature,
> instead of using a separate counter.
> 
> Removing that ugliness we end up with different behavior of save/load, because vmstate will now suddenly not contain icount-related values anymore.
> What I do not understand is why this causes a problem because save should just not store the icount state and load should just not load the icount state,
> and why we die on the load of s390 keys state (it works just fine for other architectures).
> 
> Here is a diff that makes the problem disappear, but needs --enable-tcg:
> 
> 
> ----------------------------------------------------------------------------------------------------
> diff --git a/accel/qtest.c b/accel/qtest.c
> index 119d0f16a4..4cb16abc2c 100644
> --- a/accel/qtest.c
> +++ b/accel/qtest.c
> @@ -23,6 +23,12 @@
>  
>  static int qtest_init_accel(MachineState *ms)
>  {
> +    QemuOpts *opts = qemu_opts_create(qemu_find_opts("icount"), NULL, 0,
> +                                      &error_abort);
> +    qemu_opt_set(opts, "shift", "0", &error_abort);
> +    icount_configure(opts, &error_abort);
> +    qemu_opts_del(opts);
> +
>      return 0;
>  }
>  
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index f39fd5270b..a5e788c86a 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -2786,10 +2786,12 @@ static void configure_accelerators(const char *progname)
>          error_report("falling back to %s", ac->name);
>      }
>  
> +    /*
>      if (icount_enabled() && !tcg_enabled()) {
>          error_report("-icount is not allowed with hardware virtualization");
>          exit(1);
>     }
> +    */
>  }
>  
>  static void create_default_memdev(MachineState *ms, const char *path)
> ----------------------------------------------------------------------------------------------------
> 
> Without this patch, here is the full failure, maybe someone has a good hint, otherwise I'll keep digging from here inside the s390-specific code.
> 
> QA output created by 267
> 
> === No block devices at all ===
> 
> Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
> Testing:
> QEMU X.Y.Z monitor - type 'help' for more information
> (qemu) savevm snap0
> Error: No block device can accept snapshots
> (qemu) info snapshots
> No available block device supports snapshots
> (qemu) loadvm snap0
> Error: No block device supports snapshots
> (qemu) quit
> 
> 
> === -drive if=none ===
> 
> Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
> Testing: -drive driver=file,file=TEST_DIR/t.IMGFMT,if=none
> QEMU X.Y.Z monitor - type 'help' for more information
> (qemu) savevm snap0
> Error: Device 'none0' is writable but does not support snapshots
> (qemu) info snapshots
> No available block device supports snapshots
> (qemu) loadvm snap0
> Error: Device 'none0' is writable but does not support snapshots
> (qemu) quit
> 
> Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
> Testing: -drive driver=IMGFMT,file=TEST_DIR/t.IMGFMT,if=none
> QEMU X.Y.Z monitor - type 'help' for more information
> (qemu) savevm snap0
> (qemu) info snapshots
> List of snapshots present on all disks:
> ID        TAG                     VM SIZE                DATE       VM CLOCK
> --        snap0                      SIZE yyyy-mm-dd hh:mm:ss   00:00:00.000
> (qemu) loadvm snap0
> (qemu) quit
> 
> Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
> Testing: -drive driver=IMGFMT,file=TEST_DIR/t.IMGFMT,if=none -device virtio-blk,drive=none0
> QEMU X.Y.Z monitor - type 'help' for more information
> (qemu) savevm snap0
> (qemu) info snapshots
> List of snapshots present on all disks:
> ID        TAG                     VM SIZE                DATE       VM CLOCK
> --        snap0                      SIZE yyyy-mm-dd hh:mm:ss   00:00:00.000
> (qemu) loadvm snap0
> (qemu) quit
> 
> 
> === -drive if=virtio ===
> 
> Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
> Testing: -drive driver=file,file=TEST_DIR/t.IMGFMT,if=virtio
> QEMU X.Y.Z monitor - type 'help' for more information
> (qemu) savevm snap0
> Error: Device 'virtio0' is writable but does not support snapshots
> (qemu) info snapshots
> No available block device supports snapshots
> (qemu) loadvm snap0
> Error: Device 'virtio0' is writable but does not support snapshots
> (qemu) quit
> 
> Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
> Testing: -drive driver=IMGFMT,file=TEST_DIR/t.IMGFMT,if=virtio
> QEMU X.Y.Z monitor - type 'help' for more information
> (qemu) savevm snap0
> (qemu) info snapshots
> List of snapshots present on all disks:
> ID        TAG                     VM SIZE                DATE       VM CLOCK
> --        snap0                      SIZE yyyy-mm-dd hh:mm:ss   00:00:00.000
> (qemu) loadvm snap0
> (qemu) quit
> 
> 
> === Simple -blockdev ===
> 
> Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
> Testing: -blockdev driver=file,filename=TEST_DIR/t.IMGFMT,node-name=file
> QEMU X.Y.Z monitor - type 'help' for more information
> (qemu) savevm snap0
> Error: Device '' is writable but does not support snapshots
> (qemu) info snapshots
> No available block device supports snapshots
> (qemu) loadvm snap0
> Error: Device '' is writable but does not support snapshots
> (qemu) quit
> 
> Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
> Testing: -blockdev driver=file,filename=TEST_DIR/t.IMGFMT,node-name=file -blockdev driver=IMGFMT,file=file,node-name=fmt
> QEMU X.Y.Z monitor - type 'help' for more information
> (qemu) savevm snap0
> (qemu) info snapshots
> List of snapshots present on all disks:
> ID        TAG                     VM SIZE                DATE       VM CLOCK
> --        snap0                      SIZE yyyy-mm-dd hh:mm:ss   00:00:00.000
> (qemu) loadvm snap0
> (qemu) quit
> 
> Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
> Testing: -blockdev driver=file,filename=TEST_DIR/t.IMGFMT,node-name=file -blockdev driver=raw,file=file,node-name=raw -blockdev driver=IMGFMT,file=raw,node-name=fmt
> QEMU X.Y.Z monitor - type 'help' for more information
> (qemu) savevm snap0
> (qemu) info snapshots
> List of snapshots present on all disks:
> ID        TAG                     VM SIZE                DATE       VM CLOCK
> --        snap0                      SIZE yyyy-mm-dd hh:mm:ss   00:00:00.000
> (qemu) loadvm snap0
> (qemu) quit
> 
> 
> === -blockdev with a filter on top ===
> 
> Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
> Testing: -blockdev driver=file,filename=TEST_DIR/t.IMGFMT,node-name=file -blockdev driver=IMGFMT,file=file,node-name=fmt -blockdev driver=copy-on-read,file=fmt,node-name=filter
> QEMU X.Y.Z monitor - type 'help' for more information
> (qemu) savevm snap0
> (qemu) info snapshots
> List of snapshots present on all disks:
> ID        TAG                     VM SIZE                DATE       VM CLOCK
> --        snap0                      SIZE yyyy-mm-dd hh:mm:ss   00:00:00.000
> (qemu) loadvm snap0
> (qemu) quit
> 
> 
> === -blockdev with a backing file ===
> 
> Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=134217728
> Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 backing_file=TEST_DIR/t.IMGFMT.base
> Testing: -blockdev driver=file,filename=TEST_DIR/t.IMGFMT.base,node-name=backing-file -blockdev driver=file,filename=TEST_DIR/t.IMGFMT,node-name=file -blockdev driver=IMGFMT,file=file,backing=backing-file,node-name=fmt
> QEMU X.Y.Z monitor - type 'help' for more information
> (qemu) savevm snap0
> (qemu) info snapshots
> List of snapshots present on all disks:
> ID        TAG                     VM SIZE                DATE       VM CLOCK
> --        snap0                      SIZE yyyy-mm-dd hh:mm:ss   00:00:00.000
> (qemu) loadvm snap0
> Unexpected storage key flag data: 0
> error while loading state for instance 0x0 of device 's390-skeys'
> Error: Error -22 while loading VM state
> 
> 
> 
> 
>>
>>>>
>>>> 	#if defined CONFIG_TCG || !defined NEED_CPU_H
>>>> 	extern bool icount_enabled(void);
>>>> 	#else
>>>> 	#define icount_enabled() 0
>>>> 	#endif
>>>>
>>>> (This way, more TCG-only code in cpus.c gets elided).  You can integrate
>>>> this change in the next version.
>>>>
>>>> Paolo
>>>>
>>>
>>> Weird, I tested with --disable-tcg explicitly (but may be some time ago now, as I constantly rebased).
>>>
>>> Will take a look at the introduction of this #defines in place of variables,
>>> as this mechanisms will not work in the future for target-specific modules.
>>
>> This is only done for per-target files so it should not be a problem.
>>
>> Paolo
>>
>>
> 
> 



^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH 3/3] cpu-timers, icount: new modules
  2020-07-09 18:38         ` Claudio Fontana
  2020-07-09 18:46           ` Claudio Fontana
@ 2020-07-10  4:36           ` Thomas Huth
  2020-07-10 22:45             ` Paolo Bonzini
  1 sibling, 1 reply; 37+ messages in thread
From: Thomas Huth @ 2020-07-10  4:36 UTC (permalink / raw)
  To: Claudio Fontana, Paolo Bonzini, Laurent Vivier, Alex Bennée,
	Peter Maydell, Philippe Mathieu-Daudé,
	Roman Bolshakov, Markus Armbruster
  Cc: Eduardo Habkost, Colin Xu, Marcelo Tosatti, qemu-devel,
	haxm-team, Wenchao Wang, Sunil Muthuswamy, Richard Henderson

On 09/07/2020 20.38, Claudio Fontana wrote:
> On 7/8/20 5:05 PM, Paolo Bonzini wrote:
>> On 08/07/20 17:00, Claudio Fontana wrote:
>>>> Bisectable, 100% failure rate, etc. :(  Can you split the patch in
>>>> multiple parts, specifically separating any rename or introducing of
>>>> includes from the final file move?
>>> Hi Paolo,
>>>
>>> will take a look!
>>>
>>> Is this captured by some travis / cirrus-ci / anything I can easily see the result of?
>>>
>>>
>>
>> Nope, unfortunately we don't have an s390 CI.  But if you can get your
>> hands on one, just "./configure --target-list=s390x-softmmu && make &&
>> make check-block" will show it.
> 
> So this is tricky, but I am making some progress after getting my hands on one.
> Maybe if someone understands s390 keys better, I could be clued in.
> 
> In short this goes away if I again set icount to enabled for qtest,
> basically ensuring that --enable-tcg is there and then reenabling icount.
> 
> qtest was forcing icount and shift=0 by creating qemu options, in order to misuse its counter feature,
> instead of using a separate counter.
> 
> Removing that ugliness we end up with different behavior of save/load, because vmstate will now suddenly not contain icount-related values anymore.
> What I do not understand is why this causes a problem because save should just not store the icount state and load should just not load the icount state,
> and why we die on the load of s390 keys state (it works just fine for other architectures).

FYI, the s390 storage keys are just part of the RAM of the machine,
AFAIK they are in no way related to icount. To me it sounds like your
problem is that the migration stream got corrupted elsewhere and is now
just failing in the skey handler by accident.
Is there maybe an endianness bug around somewhere? s390x is one of the
last big endian hosts that is still around...

 Thomas



^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH 3/3] cpu-timers, icount: new modules
  2020-07-09 18:46           ` Claudio Fontana
@ 2020-07-10  6:33             ` Cornelia Huck
  2020-07-10 19:20               ` Claudio Fontana
  2020-07-11 11:40               ` Claudio Fontana
  0 siblings, 2 replies; 37+ messages in thread
From: Cornelia Huck @ 2020-07-10  6:33 UTC (permalink / raw)
  To: Claudio Fontana
  Cc: Laurent Vivier, Peter Maydell, Thomas Huth, Eduardo Habkost,
	Colin Xu, Jason J. Herne, Alex Bennée, Marcelo Tosatti,
	Markus Armbruster, qemu-devel, Roman Bolshakov, haxm-team,
	Wenchao Wang, Paolo Bonzini, Sunil Muthuswamy,
	Philippe Mathieu-Daudé,
	Richard Henderson

On Thu, 9 Jul 2020 20:46:56 +0200
Claudio Fontana <cfontana@suse.de> wrote:

> On 7/9/20 8:38 PM, Claudio Fontana wrote:
> > On 7/8/20 5:05 PM, Paolo Bonzini wrote:  
> >> On 08/07/20 17:00, Claudio Fontana wrote:  
> >>>> Bisectable, 100% failure rate, etc. :(  Can you split the patch in
> >>>> multiple parts, specifically separating any rename or introducing of
> >>>> includes from the final file move?  
> >>> Hi Paolo,
> >>>
> >>> will take a look!
> >>>
> >>> Is this captured by some travis / cirrus-ci / anything I can easily see the result of?
> >>>
> >>>  
> >>
> >> Nope, unfortunately we don't have an s390 CI.  But if you can get your
> >> hands on one, just "./configure --target-list=s390x-softmmu && make &&
> >> make check-block" will show it.  
> > 
> > So this is tricky, but I am making some progress after getting my hands on one.
> > Maybe if someone understands s390 keys better, I could be clued in.  
> 
> 
> Also adding Cornelia to Cc:.
> 
> Maybe the savevm_s390_storage_keys SaveVMHandlers etc assume that the icount state part of the vmstate is there?

I don't see anything that would deal with icount here. Adding Jason to
cc: in case he has an idea. (I assume it would behave the same under
KVM, as the only thing different are the internal callbacks.)

> 
> 
> > 
> > In short this goes away if I again set icount to enabled for qtest,
> > basically ensuring that --enable-tcg is there and then reenabling icount.
> > 
> > qtest was forcing icount and shift=0 by creating qemu options, in order to misuse its counter feature,
> > instead of using a separate counter.
> > 
> > Removing that ugliness we end up with different behavior of save/load, because vmstate will now suddenly not contain icount-related values anymore.
> > What I do not understand is why this causes a problem because save should just not store the icount state and load should just not load the icount state,
> > and why we die on the load of s390 keys state (it works just fine for other architectures).

Yes, I don't really see why skeys is so special. No endianness stuff, I
assume?

> > 
> > Here is a diff that makes the problem disappear, but needs --enable-tcg:
> > 
> > 
> > ----------------------------------------------------------------------------------------------------
> > diff --git a/accel/qtest.c b/accel/qtest.c
> > index 119d0f16a4..4cb16abc2c 100644
> > --- a/accel/qtest.c
> > +++ b/accel/qtest.c
> > @@ -23,6 +23,12 @@
> >  
> >  static int qtest_init_accel(MachineState *ms)
> >  {
> > +    QemuOpts *opts = qemu_opts_create(qemu_find_opts("icount"), NULL, 0,
> > +                                      &error_abort);
> > +    qemu_opt_set(opts, "shift", "0", &error_abort);
> > +    icount_configure(opts, &error_abort);
> > +    qemu_opts_del(opts);
> > +
> >      return 0;
> >  }
> >  
> > diff --git a/softmmu/vl.c b/softmmu/vl.c
> > index f39fd5270b..a5e788c86a 100644
> > --- a/softmmu/vl.c
> > +++ b/softmmu/vl.c
> > @@ -2786,10 +2786,12 @@ static void configure_accelerators(const char *progname)
> >          error_report("falling back to %s", ac->name);
> >      }
> >  
> > +    /*
> >      if (icount_enabled() && !tcg_enabled()) {
> >          error_report("-icount is not allowed with hardware virtualization");
> >          exit(1);
> >     }
> > +    */
> >  }
> >  
> >  static void create_default_memdev(MachineState *ms, const char *path)
> > ----------------------------------------------------------------------------------------------------
> > 
> > Without this patch, here is the full failure, maybe someone has a good hint, otherwise I'll keep digging from here inside the s390-specific code.
> > 
> > QA output created by 267
> > 
> > === No block devices at all ===
> > 
> > Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
> > Testing:
> > QEMU X.Y.Z monitor - type 'help' for more information
> > (qemu) savevm snap0
> > Error: No block device can accept snapshots
> > (qemu) info snapshots
> > No available block device supports snapshots
> > (qemu) loadvm snap0
> > Error: No block device supports snapshots
> > (qemu) quit
> > 
> > 
> > === -drive if=none ===
> > 
> > Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
> > Testing: -drive driver=file,file=TEST_DIR/t.IMGFMT,if=none
> > QEMU X.Y.Z monitor - type 'help' for more information
> > (qemu) savevm snap0
> > Error: Device 'none0' is writable but does not support snapshots
> > (qemu) info snapshots
> > No available block device supports snapshots
> > (qemu) loadvm snap0
> > Error: Device 'none0' is writable but does not support snapshots
> > (qemu) quit
> > 
> > Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
> > Testing: -drive driver=IMGFMT,file=TEST_DIR/t.IMGFMT,if=none
> > QEMU X.Y.Z monitor - type 'help' for more information
> > (qemu) savevm snap0
> > (qemu) info snapshots
> > List of snapshots present on all disks:
> > ID        TAG                     VM SIZE                DATE       VM CLOCK
> > --        snap0                      SIZE yyyy-mm-dd hh:mm:ss   00:00:00.000
> > (qemu) loadvm snap0
> > (qemu) quit
> > 
> > Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
> > Testing: -drive driver=IMGFMT,file=TEST_DIR/t.IMGFMT,if=none -device virtio-blk,drive=none0
> > QEMU X.Y.Z monitor - type 'help' for more information
> > (qemu) savevm snap0
> > (qemu) info snapshots
> > List of snapshots present on all disks:
> > ID        TAG                     VM SIZE                DATE       VM CLOCK
> > --        snap0                      SIZE yyyy-mm-dd hh:mm:ss   00:00:00.000
> > (qemu) loadvm snap0
> > (qemu) quit
> > 
> > 
> > === -drive if=virtio ===
> > 
> > Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
> > Testing: -drive driver=file,file=TEST_DIR/t.IMGFMT,if=virtio
> > QEMU X.Y.Z monitor - type 'help' for more information
> > (qemu) savevm snap0
> > Error: Device 'virtio0' is writable but does not support snapshots
> > (qemu) info snapshots
> > No available block device supports snapshots
> > (qemu) loadvm snap0
> > Error: Device 'virtio0' is writable but does not support snapshots
> > (qemu) quit
> > 
> > Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
> > Testing: -drive driver=IMGFMT,file=TEST_DIR/t.IMGFMT,if=virtio
> > QEMU X.Y.Z monitor - type 'help' for more information
> > (qemu) savevm snap0
> > (qemu) info snapshots
> > List of snapshots present on all disks:
> > ID        TAG                     VM SIZE                DATE       VM CLOCK
> > --        snap0                      SIZE yyyy-mm-dd hh:mm:ss   00:00:00.000
> > (qemu) loadvm snap0
> > (qemu) quit
> > 
> > 
> > === Simple -blockdev ===
> > 
> > Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
> > Testing: -blockdev driver=file,filename=TEST_DIR/t.IMGFMT,node-name=file
> > QEMU X.Y.Z monitor - type 'help' for more information
> > (qemu) savevm snap0
> > Error: Device '' is writable but does not support snapshots
> > (qemu) info snapshots
> > No available block device supports snapshots
> > (qemu) loadvm snap0
> > Error: Device '' is writable but does not support snapshots
> > (qemu) quit
> > 
> > Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
> > Testing: -blockdev driver=file,filename=TEST_DIR/t.IMGFMT,node-name=file -blockdev driver=IMGFMT,file=file,node-name=fmt
> > QEMU X.Y.Z monitor - type 'help' for more information
> > (qemu) savevm snap0
> > (qemu) info snapshots
> > List of snapshots present on all disks:
> > ID        TAG                     VM SIZE                DATE       VM CLOCK
> > --        snap0                      SIZE yyyy-mm-dd hh:mm:ss   00:00:00.000
> > (qemu) loadvm snap0
> > (qemu) quit
> > 
> > Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
> > Testing: -blockdev driver=file,filename=TEST_DIR/t.IMGFMT,node-name=file -blockdev driver=raw,file=file,node-name=raw -blockdev driver=IMGFMT,file=raw,node-name=fmt
> > QEMU X.Y.Z monitor - type 'help' for more information
> > (qemu) savevm snap0
> > (qemu) info snapshots
> > List of snapshots present on all disks:
> > ID        TAG                     VM SIZE                DATE       VM CLOCK
> > --        snap0                      SIZE yyyy-mm-dd hh:mm:ss   00:00:00.000
> > (qemu) loadvm snap0
> > (qemu) quit
> > 
> > 
> > === -blockdev with a filter on top ===
> > 
> > Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
> > Testing: -blockdev driver=file,filename=TEST_DIR/t.IMGFMT,node-name=file -blockdev driver=IMGFMT,file=file,node-name=fmt -blockdev driver=copy-on-read,file=fmt,node-name=filter
> > QEMU X.Y.Z monitor - type 'help' for more information
> > (qemu) savevm snap0
> > (qemu) info snapshots
> > List of snapshots present on all disks:
> > ID        TAG                     VM SIZE                DATE       VM CLOCK
> > --        snap0                      SIZE yyyy-mm-dd hh:mm:ss   00:00:00.000
> > (qemu) loadvm snap0
> > (qemu) quit
> > 
> > 
> > === -blockdev with a backing file ===
> > 
> > Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=134217728
> > Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 backing_file=TEST_DIR/t.IMGFMT.base
> > Testing: -blockdev driver=file,filename=TEST_DIR/t.IMGFMT.base,node-name=backing-file -blockdev driver=file,filename=TEST_DIR/t.IMGFMT,node-name=file -blockdev driver=IMGFMT,file=file,backing=backing-file,node-name=fmt
> > QEMU X.Y.Z monitor - type 'help' for more information
> > (qemu) savevm snap0
> > (qemu) info snapshots
> > List of snapshots present on all disks:
> > ID        TAG                     VM SIZE                DATE       VM CLOCK
> > --        snap0                      SIZE yyyy-mm-dd hh:mm:ss   00:00:00.000
> > (qemu) loadvm snap0
> > Unexpected storage key flag data: 0
> > error while loading state for instance 0x0 of device 's390-skeys'
> > Error: Error -22 while loading VM state
> > 
> > 
> > 
> >   
> >>  
> >>>>
> >>>> 	#if defined CONFIG_TCG || !defined NEED_CPU_H
> >>>> 	extern bool icount_enabled(void);
> >>>> 	#else
> >>>> 	#define icount_enabled() 0
> >>>> 	#endif
> >>>>
> >>>> (This way, more TCG-only code in cpus.c gets elided).  You can integrate
> >>>> this change in the next version.
> >>>>
> >>>> Paolo
> >>>>  
> >>>
> >>> Weird, I tested with --disable-tcg explicitly (but may be some time ago now, as I constantly rebased).
> >>>
> >>> Will take a look at the introduction of this #defines in place of variables,
> >>> as this mechanisms will not work in the future for target-specific modules.  
> >>
> >> This is only done for per-target files so it should not be a problem.
> >>
> >> Paolo
> >>
> >>  
> > 
> >   
> 



^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH 3/3] cpu-timers, icount: new modules
  2020-07-10  6:33             ` Cornelia Huck
@ 2020-07-10 19:20               ` Claudio Fontana
  2020-07-13 10:46                 ` Cornelia Huck
  2020-07-11 11:40               ` Claudio Fontana
  1 sibling, 1 reply; 37+ messages in thread
From: Claudio Fontana @ 2020-07-10 19:20 UTC (permalink / raw)
  To: Cornelia Huck
  Cc: Laurent Vivier, Peter Maydell, Thomas Huth, Jason J. Herne,
	Philippe Mathieu-Daudé,
	Marcelo Tosatti, Markus Armbruster, qemu-devel, Roman Bolshakov,
	Wenchao Wang, haxm-team, Colin Xu, Paolo Bonzini,
	Sunil Muthuswamy, Richard Henderson, Alex Bennée,
	Eduardo Habkost

On 7/10/20 8:33 AM, Cornelia Huck wrote:
> On Thu, 9 Jul 2020 20:46:56 +0200
> Claudio Fontana <cfontana@suse.de> wrote:
> 
>> On 7/9/20 8:38 PM, Claudio Fontana wrote:
>>> On 7/8/20 5:05 PM, Paolo Bonzini wrote:  
>>>> On 08/07/20 17:00, Claudio Fontana wrote:  
>>>>>> Bisectable, 100% failure rate, etc. :(  Can you split the patch in
>>>>>> multiple parts, specifically separating any rename or introducing of
>>>>>> includes from the final file move?  
>>>>> Hi Paolo,
>>>>>
>>>>> will take a look!
>>>>>
>>>>> Is this captured by some travis / cirrus-ci / anything I can easily see the result of?
>>>>>
>>>>>  
>>>>
>>>> Nope, unfortunately we don't have an s390 CI.  But if you can get your
>>>> hands on one, just "./configure --target-list=s390x-softmmu && make &&
>>>> make check-block" will show it.  
>>>
>>> So this is tricky, but I am making some progress after getting my hands on one.
>>> Maybe if someone understands s390 keys better, I could be clued in.  
>>
>>
>> Also adding Cornelia to Cc:.
>>
>> Maybe the savevm_s390_storage_keys SaveVMHandlers etc assume that the icount state part of the vmstate is there?
> 
> I don't see anything that would deal with icount here. Adding Jason to
> cc: in case he has an idea. (I assume it would behave the same under
> KVM, as the only thing different are the internal callbacks.)

yes, same between tcg and kvm.

> 
>>
>>
>>>
>>> In short this goes away if I again set icount to enabled for qtest,
>>> basically ensuring that --enable-tcg is there and then reenabling icount.
>>>
>>> qtest was forcing icount and shift=0 by creating qemu options, in order to misuse its counter feature,
>>> instead of using a separate counter.
>>>
>>> Removing that ugliness we end up with different behavior of save/load, because vmstate will now suddenly not contain icount-related values anymore.
>>> What I do not understand is why this causes a problem because save should just not store the icount state and load should just not load the icount state,
>>> and why we die on the load of s390 keys state (it works just fine for other architectures).
> 
> Yes, I don't really see why skeys is so special. No endianness stuff, I
> assume?


No, does not seem to be the issue.

I discovered a way simpler way to "fix" it: 

static bool icount_state_needed(void *opaque)
{
    return 1;
}

Ie, making sure that the state is always saved/restored, even when unused.

Really weird.

I logged/debugged the vmstate code, and I can see that things seem symmetric between save and load when it comes to timers.

something puts 0s into the key somehow...







> 
>>>
>>> Here is a diff that makes the problem disappear, but needs --enable-tcg:
>>>
>>>
>>> ----------------------------------------------------------------------------------------------------
>>> diff --git a/accel/qtest.c b/accel/qtest.c
>>> index 119d0f16a4..4cb16abc2c 100644
>>> --- a/accel/qtest.c
>>> +++ b/accel/qtest.c
>>> @@ -23,6 +23,12 @@
>>>  
>>>  static int qtest_init_accel(MachineState *ms)
>>>  {
>>> +    QemuOpts *opts = qemu_opts_create(qemu_find_opts("icount"), NULL, 0,
>>> +                                      &error_abort);
>>> +    qemu_opt_set(opts, "shift", "0", &error_abort);
>>> +    icount_configure(opts, &error_abort);
>>> +    qemu_opts_del(opts);
>>> +
>>>      return 0;
>>>  }
>>>  
>>> diff --git a/softmmu/vl.c b/softmmu/vl.c
>>> index f39fd5270b..a5e788c86a 100644
>>> --- a/softmmu/vl.c
>>> +++ b/softmmu/vl.c
>>> @@ -2786,10 +2786,12 @@ static void configure_accelerators(const char *progname)
>>>          error_report("falling back to %s", ac->name);
>>>      }
>>>  
>>> +    /*
>>>      if (icount_enabled() && !tcg_enabled()) {
>>>          error_report("-icount is not allowed with hardware virtualization");
>>>          exit(1);
>>>     }
>>> +    */
>>>  }
>>>  
>>>  static void create_default_memdev(MachineState *ms, const char *path)
>>> ----------------------------------------------------------------------------------------------------
>>>
>>> Without this patch, here is the full failure, maybe someone has a good hint, otherwise I'll keep digging from here inside the s390-specific code.
>>>
>>> QA output created by 267
>>>
>>> === No block devices at all ===
>>>
>>> Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
>>> Testing:
>>> QEMU X.Y.Z monitor - type 'help' for more information
>>> (qemu) savevm snap0
>>> Error: No block device can accept snapshots
>>> (qemu) info snapshots
>>> No available block device supports snapshots
>>> (qemu) loadvm snap0
>>> Error: No block device supports snapshots
>>> (qemu) quit
>>>
>>>
>>> === -drive if=none ===
>>>
>>> Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
>>> Testing: -drive driver=file,file=TEST_DIR/t.IMGFMT,if=none
>>> QEMU X.Y.Z monitor - type 'help' for more information
>>> (qemu) savevm snap0
>>> Error: Device 'none0' is writable but does not support snapshots
>>> (qemu) info snapshots
>>> No available block device supports snapshots
>>> (qemu) loadvm snap0
>>> Error: Device 'none0' is writable but does not support snapshots
>>> (qemu) quit
>>>
>>> Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
>>> Testing: -drive driver=IMGFMT,file=TEST_DIR/t.IMGFMT,if=none
>>> QEMU X.Y.Z monitor - type 'help' for more information
>>> (qemu) savevm snap0
>>> (qemu) info snapshots
>>> List of snapshots present on all disks:
>>> ID        TAG                     VM SIZE                DATE       VM CLOCK
>>> --        snap0                      SIZE yyyy-mm-dd hh:mm:ss   00:00:00.000
>>> (qemu) loadvm snap0
>>> (qemu) quit
>>>
>>> Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
>>> Testing: -drive driver=IMGFMT,file=TEST_DIR/t.IMGFMT,if=none -device virtio-blk,drive=none0
>>> QEMU X.Y.Z monitor - type 'help' for more information
>>> (qemu) savevm snap0
>>> (qemu) info snapshots
>>> List of snapshots present on all disks:
>>> ID        TAG                     VM SIZE                DATE       VM CLOCK
>>> --        snap0                      SIZE yyyy-mm-dd hh:mm:ss   00:00:00.000
>>> (qemu) loadvm snap0
>>> (qemu) quit
>>>
>>>
>>> === -drive if=virtio ===
>>>
>>> Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
>>> Testing: -drive driver=file,file=TEST_DIR/t.IMGFMT,if=virtio
>>> QEMU X.Y.Z monitor - type 'help' for more information
>>> (qemu) savevm snap0
>>> Error: Device 'virtio0' is writable but does not support snapshots
>>> (qemu) info snapshots
>>> No available block device supports snapshots
>>> (qemu) loadvm snap0
>>> Error: Device 'virtio0' is writable but does not support snapshots
>>> (qemu) quit
>>>
>>> Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
>>> Testing: -drive driver=IMGFMT,file=TEST_DIR/t.IMGFMT,if=virtio
>>> QEMU X.Y.Z monitor - type 'help' for more information
>>> (qemu) savevm snap0
>>> (qemu) info snapshots
>>> List of snapshots present on all disks:
>>> ID        TAG                     VM SIZE                DATE       VM CLOCK
>>> --        snap0                      SIZE yyyy-mm-dd hh:mm:ss   00:00:00.000
>>> (qemu) loadvm snap0
>>> (qemu) quit
>>>
>>>
>>> === Simple -blockdev ===
>>>
>>> Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
>>> Testing: -blockdev driver=file,filename=TEST_DIR/t.IMGFMT,node-name=file
>>> QEMU X.Y.Z monitor - type 'help' for more information
>>> (qemu) savevm snap0
>>> Error: Device '' is writable but does not support snapshots
>>> (qemu) info snapshots
>>> No available block device supports snapshots
>>> (qemu) loadvm snap0
>>> Error: Device '' is writable but does not support snapshots
>>> (qemu) quit
>>>
>>> Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
>>> Testing: -blockdev driver=file,filename=TEST_DIR/t.IMGFMT,node-name=file -blockdev driver=IMGFMT,file=file,node-name=fmt
>>> QEMU X.Y.Z monitor - type 'help' for more information
>>> (qemu) savevm snap0
>>> (qemu) info snapshots
>>> List of snapshots present on all disks:
>>> ID        TAG                     VM SIZE                DATE       VM CLOCK
>>> --        snap0                      SIZE yyyy-mm-dd hh:mm:ss   00:00:00.000
>>> (qemu) loadvm snap0
>>> (qemu) quit
>>>
>>> Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
>>> Testing: -blockdev driver=file,filename=TEST_DIR/t.IMGFMT,node-name=file -blockdev driver=raw,file=file,node-name=raw -blockdev driver=IMGFMT,file=raw,node-name=fmt
>>> QEMU X.Y.Z monitor - type 'help' for more information
>>> (qemu) savevm snap0
>>> (qemu) info snapshots
>>> List of snapshots present on all disks:
>>> ID        TAG                     VM SIZE                DATE       VM CLOCK
>>> --        snap0                      SIZE yyyy-mm-dd hh:mm:ss   00:00:00.000
>>> (qemu) loadvm snap0
>>> (qemu) quit
>>>
>>>
>>> === -blockdev with a filter on top ===
>>>
>>> Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
>>> Testing: -blockdev driver=file,filename=TEST_DIR/t.IMGFMT,node-name=file -blockdev driver=IMGFMT,file=file,node-name=fmt -blockdev driver=copy-on-read,file=fmt,node-name=filter
>>> QEMU X.Y.Z monitor - type 'help' for more information
>>> (qemu) savevm snap0
>>> (qemu) info snapshots
>>> List of snapshots present on all disks:
>>> ID        TAG                     VM SIZE                DATE       VM CLOCK
>>> --        snap0                      SIZE yyyy-mm-dd hh:mm:ss   00:00:00.000
>>> (qemu) loadvm snap0
>>> (qemu) quit
>>>
>>>
>>> === -blockdev with a backing file ===
>>>
>>> Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=134217728
>>> Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 backing_file=TEST_DIR/t.IMGFMT.base
>>> Testing: -blockdev driver=file,filename=TEST_DIR/t.IMGFMT.base,node-name=backing-file -blockdev driver=file,filename=TEST_DIR/t.IMGFMT,node-name=file -blockdev driver=IMGFMT,file=file,backing=backing-file,node-name=fmt
>>> QEMU X.Y.Z monitor - type 'help' for more information
>>> (qemu) savevm snap0
>>> (qemu) info snapshots
>>> List of snapshots present on all disks:
>>> ID        TAG                     VM SIZE                DATE       VM CLOCK
>>> --        snap0                      SIZE yyyy-mm-dd hh:mm:ss   00:00:00.000
>>> (qemu) loadvm snap0
>>> Unexpected storage key flag data: 0
>>> error while loading state for instance 0x0 of device 's390-skeys'
>>> Error: Error -22 while loading VM state
>>>
>>>
>>>
>>>   
>>>>  
>>>>>>
>>>>>> 	#if defined CONFIG_TCG || !defined NEED_CPU_H
>>>>>> 	extern bool icount_enabled(void);
>>>>>> 	#else
>>>>>> 	#define icount_enabled() 0
>>>>>> 	#endif
>>>>>>
>>>>>> (This way, more TCG-only code in cpus.c gets elided).  You can integrate
>>>>>> this change in the next version.
>>>>>>
>>>>>> Paolo
>>>>>>  
>>>>>
>>>>> Weird, I tested with --disable-tcg explicitly (but may be some time ago now, as I constantly rebased).
>>>>>
>>>>> Will take a look at the introduction of this #defines in place of variables,
>>>>> as this mechanisms will not work in the future for target-specific modules.  
>>>>
>>>> This is only done for per-target files so it should not be a problem.
>>>>
>>>> Paolo
>>>>
>>>>  
>>>
>>>   
>>
> 
> 



^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH 3/3] cpu-timers, icount: new modules
  2020-07-10  4:36           ` Thomas Huth
@ 2020-07-10 22:45             ` Paolo Bonzini
  2020-07-11  9:14               ` Claudio Fontana
  0 siblings, 1 reply; 37+ messages in thread
From: Paolo Bonzini @ 2020-07-10 22:45 UTC (permalink / raw)
  To: Thomas Huth, Claudio Fontana, Laurent Vivier, Alex Bennée,
	Peter Maydell, Philippe Mathieu-Daudé,
	Roman Bolshakov, Markus Armbruster
  Cc: Eduardo Habkost, Colin Xu, Marcelo Tosatti, qemu-devel,
	haxm-team, Wenchao Wang, Sunil Muthuswamy, Richard Henderson

On 10/07/20 06:36, Thomas Huth wrote:
> 
> In short this goes away if I again set icount to enabled for qtest,
> basically ensuring that --enable-tcg is there and then reenabling icount.
> 
> qtest was forcing icount and shift=0 by creating qemu options, in order to misuse its counter feature,
> instead of using a separate counter.

Why would it need a separate counter?  In both cases it's a
manually-updated counter that is used for QEMU_CLOCK_VIRTUAL.  The only
difference is that shift > 0 doesn't make sense for qtest.

Paolo



^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH 3/3] cpu-timers, icount: new modules
  2020-07-10 22:45             ` Paolo Bonzini
@ 2020-07-11  9:14               ` Claudio Fontana
  2020-07-11  9:39                 ` Paolo Bonzini
  0 siblings, 1 reply; 37+ messages in thread
From: Claudio Fontana @ 2020-07-11  9:14 UTC (permalink / raw)
  To: Paolo Bonzini, Thomas Huth, Laurent Vivier, Alex Bennée,
	Peter Maydell, Philippe Mathieu-Daudé,
	Roman Bolshakov, Markus Armbruster
  Cc: Eduardo Habkost, Colin Xu, Marcelo Tosatti, qemu-devel,
	haxm-team, Wenchao Wang, Sunil Muthuswamy, Richard Henderson

On 7/11/20 12:45 AM, Paolo Bonzini wrote:
> On 10/07/20 06:36, Thomas Huth wrote:
>>
>> In short this goes away if I again set icount to enabled for qtest,
>> basically ensuring that --enable-tcg is there and then reenabling icount.
>>
>> qtest was forcing icount and shift=0 by creating qemu options, in order to misuse its counter feature,
>> instead of using a separate counter.
> 
> Why would it need a separate counter?  In both cases it's a
> manually-updated counter that is used for QEMU_CLOCK_VIRTUAL.  The only
> difference is that shift > 0 doesn't make sense for qtest.
> 
> Paolo
> 

I think I would reverse the question. Why reuse for qtest a counter that has absolutely nothing to do with it?

qtest has nothing to do with instruction counting.

Keeping the relationship between qtest and the icount module means that
functionality of icount cannot be carved out into a separate module that is only included with tcg.

Ciao,

CLaudio


^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH 3/3] cpu-timers, icount: new modules
  2020-07-11  9:14               ` Claudio Fontana
@ 2020-07-11  9:39                 ` Paolo Bonzini
  2020-07-11 11:49                   ` Claudio Fontana
  0 siblings, 1 reply; 37+ messages in thread
From: Paolo Bonzini @ 2020-07-11  9:39 UTC (permalink / raw)
  To: Claudio Fontana, Thomas Huth, Laurent Vivier, Alex Bennée,
	Peter Maydell, Philippe Mathieu-Daudé,
	Roman Bolshakov, Markus Armbruster
  Cc: Eduardo Habkost, Colin Xu, Marcelo Tosatti, qemu-devel,
	haxm-team, Wenchao Wang, Sunil Muthuswamy, Richard Henderson

On 11/07/20 11:14, Claudio Fontana wrote:
> On 7/11/20 12:45 AM, Paolo Bonzini wrote:
>> On 10/07/20 06:36, Thomas Huth wrote:
>>>
>>> In short this goes away if I again set icount to enabled for qtest,
>>> basically ensuring that --enable-tcg is there and then reenabling icount.
>>>
>>> qtest was forcing icount and shift=0 by creating qemu options, in order to misuse its counter feature,
>>> instead of using a separate counter.
>>
>> Why would it need a separate counter?  In both cases it's a
>> manually-updated counter that is used for QEMU_CLOCK_VIRTUAL.  The only
>> difference is that shift > 0 doesn't make sense for qtest.
> 
> I think I would reverse the question. Why reuse for qtest a counter that has absolutely nothing to do with it?
> 
> qtest has nothing to do with instruction counting.

Apart from the name, icount is more like deterministic execution than
instruction counting (it's not a coincidence that record/replay is
fundamentally based on icount).  qtests need to be deterministic and
describe which qtest instructions run before a given timer fires and
which run after.

And in both cases, determinism is achieved by controlling the
advancement of QEMU_CLOCK_VIRTUAL.  It's only this central component of
icount that is shared by qtest and TCG, and I think the problem is that
this patch conflates all of them together:

- the basic "is QEMU_CLOCK_VIRTUAL software-driven" part is embedded in
qemu-timer and should not be carved out into a separate module.  This
includes the use_icount variable, which should be kept in core QEMU code.

- the fact qtest uses -icount instead of configuring the variables
directly is definitely a hack and can be removed.

- the adaptive frequency adjustment is definitely TCG specific, and so
are the particular functions in cpus.c that test icount_enabled() and
broke with this patch.  All this code should be included in the TCG
module only or, before that, should be made conditional on $(CONFIG_TCG).

So I think this patch should have been the last, not the first. :)  Once
you move all the accelerator runtime code from cpus.c to separate files,
it will be possible to move the frequency adjustment and deadline
management code into accel/tcg.  And then it will be obvious which code
is not TCG-specific and can be extracted for convenience into a
cpu-timers.c file.

Thanks,

Paolo



^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH 3/3] cpu-timers, icount: new modules
  2020-07-10  6:33             ` Cornelia Huck
  2020-07-10 19:20               ` Claudio Fontana
@ 2020-07-11 11:40               ` Claudio Fontana
  2020-07-13 10:51                 ` Cornelia Huck
  1 sibling, 1 reply; 37+ messages in thread
From: Claudio Fontana @ 2020-07-11 11:40 UTC (permalink / raw)
  To: Cornelia Huck, Jason J. Herne
  Cc: Laurent Vivier, Peter Maydell, Thomas Huth, Eduardo Habkost,
	Colin Xu, Alex Bennée, Marcelo Tosatti, Markus Armbruster,
	qemu-devel, Roman Bolshakov, haxm-team, Wenchao Wang,
	Paolo Bonzini, Sunil Muthuswamy, Philippe Mathieu-Daudé,
	Richard Henderson

On 7/10/20 8:33 AM, Cornelia Huck wrote:
> On Thu, 9 Jul 2020 20:46:56 +0200
> Claudio Fontana <cfontana@suse.de> wrote:
> 
>> On 7/9/20 8:38 PM, Claudio Fontana wrote:
>>> On 7/8/20 5:05 PM, Paolo Bonzini wrote:  
>>>> On 08/07/20 17:00, Claudio Fontana wrote:  
>>>>>> Bisectable, 100% failure rate, etc. :(  Can you split the patch in
>>>>>> multiple parts, specifically separating any rename or introducing of
>>>>>> includes from the final file move?  
>>>>> Hi Paolo,
>>>>>
>>>>> will take a look!
>>>>>
>>>>> Is this captured by some travis / cirrus-ci / anything I can easily see the result of?
>>>>>
>>>>>  
>>>>
>>>> Nope, unfortunately we don't have an s390 CI.  But if you can get your
>>>> hands on one, just "./configure --target-list=s390x-softmmu && make &&
>>>> make check-block" will show it.  
>>>
>>> So this is tricky, but I am making some progress after getting my hands on one.
>>> Maybe if someone understands s390 keys better, I could be clued in.  
>>
>>
>> Also adding Cornelia to Cc:.
>>
>> Maybe the savevm_s390_storage_keys SaveVMHandlers etc assume that the icount state part of the vmstate is there?
> 
> I don't see anything that would deal with icount here. Adding Jason to
> cc: in case he has an idea. (I assume it would behave the same under
> KVM, as the only thing different are the internal callbacks.)
> 


I found out something that for me shows that more investigation here is warranted.


Here is my latest workaround for the problem:



diff --git a/hw/s390x/s390-skeys.c b/hw/s390x/s390-skeys.c
index 1e036cc602..47c9a015af 100644
--- a/hw/s390x/s390-skeys.c
+++ b/hw/s390x/s390-skeys.c
@@ -252,6 +252,8 @@ static const TypeInfo qemu_s390_skeys_info = {
     .class_size    = sizeof(S390SKeysClass),
 };
 
+extern void qemu_fflush(QEMUFile *f);
+
 static void s390_storage_keys_save(QEMUFile *f, void *opaque)
 {
     S390SKeysState *ss = S390_SKEYS(opaque);
@@ -302,6 +304,7 @@ static void s390_storage_keys_save(QEMUFile *f, void *opaque)
     g_free(buf);
 end_stream:
     qemu_put_be64(f, eos);
+    qemu_fflush(f);
 }
 
 static int s390_storage_keys_load(QEMUFile *f, void *opaque, int version_id)
------------------------------------------------------------------------------------


I think that this might imply that my patch changing the migration stream has only triggered an existing problem.

The sympthom is: the load keys code does not see the EOS (byte value 1).
It does see the keys (which are all empty in the test, ie 32678 times the byte value 0). 

The workaround for the sympthom: flush the qemu file after putting the EOS in there.


Any ideas on where to investigate next?

Thanks,

Claudio



>>
>>
>>>
>>> In short this goes away if I again set icount to enabled for qtest,
>>> basically ensuring that --enable-tcg is there and then reenabling icount.
>>>
>>> qtest was forcing icount and shift=0 by creating qemu options, in order to misuse its counter feature,
>>> instead of using a separate counter.
>>>
>>> Removing that ugliness we end up with different behavior of save/load, because vmstate will now suddenly not contain icount-related values anymore.
>>> What I do not understand is why this causes a problem because save should just not store the icount state and load should just not load the icount state,
>>> and why we die on the load of s390 keys state (it works just fine for other architectures).
> 
> Yes, I don't really see why skeys is so special. No endianness stuff, I
> assume?
> 
>>>
>>> Here is a diff that makes the problem disappear, but needs --enable-tcg:
>>>
>>>
>>> ----------------------------------------------------------------------------------------------------
>>> diff --git a/accel/qtest.c b/accel/qtest.c
>>> index 119d0f16a4..4cb16abc2c 100644
>>> --- a/accel/qtest.c
>>> +++ b/accel/qtest.c
>>> @@ -23,6 +23,12 @@
>>>  
>>>  static int qtest_init_accel(MachineState *ms)
>>>  {
>>> +    QemuOpts *opts = qemu_opts_create(qemu_find_opts("icount"), NULL, 0,
>>> +                                      &error_abort);
>>> +    qemu_opt_set(opts, "shift", "0", &error_abort);
>>> +    icount_configure(opts, &error_abort);
>>> +    qemu_opts_del(opts);
>>> +
>>>      return 0;
>>>  }
>>>  
>>> diff --git a/softmmu/vl.c b/softmmu/vl.c
>>> index f39fd5270b..a5e788c86a 100644
>>> --- a/softmmu/vl.c
>>> +++ b/softmmu/vl.c
>>> @@ -2786,10 +2786,12 @@ static void configure_accelerators(const char *progname)
>>>          error_report("falling back to %s", ac->name);
>>>      }
>>>  
>>> +    /*
>>>      if (icount_enabled() && !tcg_enabled()) {
>>>          error_report("-icount is not allowed with hardware virtualization");
>>>          exit(1);
>>>     }
>>> +    */
>>>  }
>>>  
>>>  static void create_default_memdev(MachineState *ms, const char *path)
>>> ----------------------------------------------------------------------------------------------------
>>>
>>> Without this patch, here is the full failure, maybe someone has a good hint, otherwise I'll keep digging from here inside the s390-specific code.
>>>
>>> QA output created by 267
>>>
>>> === No block devices at all ===
>>>
>>> Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
>>> Testing:
>>> QEMU X.Y.Z monitor - type 'help' for more information
>>> (qemu) savevm snap0
>>> Error: No block device can accept snapshots
>>> (qemu) info snapshots
>>> No available block device supports snapshots
>>> (qemu) loadvm snap0
>>> Error: No block device supports snapshots
>>> (qemu) quit
>>>
>>>
>>> === -drive if=none ===
>>>
>>> Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
>>> Testing: -drive driver=file,file=TEST_DIR/t.IMGFMT,if=none
>>> QEMU X.Y.Z monitor - type 'help' for more information
>>> (qemu) savevm snap0
>>> Error: Device 'none0' is writable but does not support snapshots
>>> (qemu) info snapshots
>>> No available block device supports snapshots
>>> (qemu) loadvm snap0
>>> Error: Device 'none0' is writable but does not support snapshots
>>> (qemu) quit
>>>
>>> Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
>>> Testing: -drive driver=IMGFMT,file=TEST_DIR/t.IMGFMT,if=none
>>> QEMU X.Y.Z monitor - type 'help' for more information
>>> (qemu) savevm snap0
>>> (qemu) info snapshots
>>> List of snapshots present on all disks:
>>> ID        TAG                     VM SIZE                DATE       VM CLOCK
>>> --        snap0                      SIZE yyyy-mm-dd hh:mm:ss   00:00:00.000
>>> (qemu) loadvm snap0
>>> (qemu) quit
>>>
>>> Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
>>> Testing: -drive driver=IMGFMT,file=TEST_DIR/t.IMGFMT,if=none -device virtio-blk,drive=none0
>>> QEMU X.Y.Z monitor - type 'help' for more information
>>> (qemu) savevm snap0
>>> (qemu) info snapshots
>>> List of snapshots present on all disks:
>>> ID        TAG                     VM SIZE                DATE       VM CLOCK
>>> --        snap0                      SIZE yyyy-mm-dd hh:mm:ss   00:00:00.000
>>> (qemu) loadvm snap0
>>> (qemu) quit
>>>
>>>
>>> === -drive if=virtio ===
>>>
>>> Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
>>> Testing: -drive driver=file,file=TEST_DIR/t.IMGFMT,if=virtio
>>> QEMU X.Y.Z monitor - type 'help' for more information
>>> (qemu) savevm snap0
>>> Error: Device 'virtio0' is writable but does not support snapshots
>>> (qemu) info snapshots
>>> No available block device supports snapshots
>>> (qemu) loadvm snap0
>>> Error: Device 'virtio0' is writable but does not support snapshots
>>> (qemu) quit
>>>
>>> Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
>>> Testing: -drive driver=IMGFMT,file=TEST_DIR/t.IMGFMT,if=virtio
>>> QEMU X.Y.Z monitor - type 'help' for more information
>>> (qemu) savevm snap0
>>> (qemu) info snapshots
>>> List of snapshots present on all disks:
>>> ID        TAG                     VM SIZE                DATE       VM CLOCK
>>> --        snap0                      SIZE yyyy-mm-dd hh:mm:ss   00:00:00.000
>>> (qemu) loadvm snap0
>>> (qemu) quit
>>>
>>>
>>> === Simple -blockdev ===
>>>
>>> Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
>>> Testing: -blockdev driver=file,filename=TEST_DIR/t.IMGFMT,node-name=file
>>> QEMU X.Y.Z monitor - type 'help' for more information
>>> (qemu) savevm snap0
>>> Error: Device '' is writable but does not support snapshots
>>> (qemu) info snapshots
>>> No available block device supports snapshots
>>> (qemu) loadvm snap0
>>> Error: Device '' is writable but does not support snapshots
>>> (qemu) quit
>>>
>>> Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
>>> Testing: -blockdev driver=file,filename=TEST_DIR/t.IMGFMT,node-name=file -blockdev driver=IMGFMT,file=file,node-name=fmt
>>> QEMU X.Y.Z monitor - type 'help' for more information
>>> (qemu) savevm snap0
>>> (qemu) info snapshots
>>> List of snapshots present on all disks:
>>> ID        TAG                     VM SIZE                DATE       VM CLOCK
>>> --        snap0                      SIZE yyyy-mm-dd hh:mm:ss   00:00:00.000
>>> (qemu) loadvm snap0
>>> (qemu) quit
>>>
>>> Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
>>> Testing: -blockdev driver=file,filename=TEST_DIR/t.IMGFMT,node-name=file -blockdev driver=raw,file=file,node-name=raw -blockdev driver=IMGFMT,file=raw,node-name=fmt
>>> QEMU X.Y.Z monitor - type 'help' for more information
>>> (qemu) savevm snap0
>>> (qemu) info snapshots
>>> List of snapshots present on all disks:
>>> ID        TAG                     VM SIZE                DATE       VM CLOCK
>>> --        snap0                      SIZE yyyy-mm-dd hh:mm:ss   00:00:00.000
>>> (qemu) loadvm snap0
>>> (qemu) quit
>>>
>>>
>>> === -blockdev with a filter on top ===
>>>
>>> Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
>>> Testing: -blockdev driver=file,filename=TEST_DIR/t.IMGFMT,node-name=file -blockdev driver=IMGFMT,file=file,node-name=fmt -blockdev driver=copy-on-read,file=fmt,node-name=filter
>>> QEMU X.Y.Z monitor - type 'help' for more information
>>> (qemu) savevm snap0
>>> (qemu) info snapshots
>>> List of snapshots present on all disks:
>>> ID        TAG                     VM SIZE                DATE       VM CLOCK
>>> --        snap0                      SIZE yyyy-mm-dd hh:mm:ss   00:00:00.000
>>> (qemu) loadvm snap0
>>> (qemu) quit
>>>
>>>
>>> === -blockdev with a backing file ===
>>>
>>> Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=134217728
>>> Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 backing_file=TEST_DIR/t.IMGFMT.base
>>> Testing: -blockdev driver=file,filename=TEST_DIR/t.IMGFMT.base,node-name=backing-file -blockdev driver=file,filename=TEST_DIR/t.IMGFMT,node-name=file -blockdev driver=IMGFMT,file=file,backing=backing-file,node-name=fmt
>>> QEMU X.Y.Z monitor - type 'help' for more information
>>> (qemu) savevm snap0
>>> (qemu) info snapshots
>>> List of snapshots present on all disks:
>>> ID        TAG                     VM SIZE                DATE       VM CLOCK
>>> --        snap0                      SIZE yyyy-mm-dd hh:mm:ss   00:00:00.000
>>> (qemu) loadvm snap0
>>> Unexpected storage key flag data: 0
>>> error while loading state for instance 0x0 of device 's390-skeys'
>>> Error: Error -22 while loading VM state
>>>
>>>
>>>
>>>   
>>>>  
>>>>>>
>>>>>> 	#if defined CONFIG_TCG || !defined NEED_CPU_H
>>>>>> 	extern bool icount_enabled(void);
>>>>>> 	#else
>>>>>> 	#define icount_enabled() 0
>>>>>> 	#endif
>>>>>>
>>>>>> (This way, more TCG-only code in cpus.c gets elided).  You can integrate
>>>>>> this change in the next version.
>>>>>>
>>>>>> Paolo
>>>>>>  
>>>>>
>>>>> Weird, I tested with --disable-tcg explicitly (but may be some time ago now, as I constantly rebased).
>>>>>
>>>>> Will take a look at the introduction of this #defines in place of variables,
>>>>> as this mechanisms will not work in the future for target-specific modules.  
>>>>
>>>> This is only done for per-target files so it should not be a problem.
>>>>
>>>> Paolo
>>>>
>>>>  
>>>
>>>   
>>
> 



^ permalink raw reply related	[flat|nested] 37+ messages in thread

* Re: [PATCH 3/3] cpu-timers, icount: new modules
  2020-07-11  9:39                 ` Paolo Bonzini
@ 2020-07-11 11:49                   ` Claudio Fontana
  2020-07-11 12:19                     ` Paolo Bonzini
  0 siblings, 1 reply; 37+ messages in thread
From: Claudio Fontana @ 2020-07-11 11:49 UTC (permalink / raw)
  To: Paolo Bonzini, Thomas Huth, Laurent Vivier, Alex Bennée,
	Peter Maydell, Philippe Mathieu-Daudé,
	Roman Bolshakov, Markus Armbruster
  Cc: Eduardo Habkost, Colin Xu, Marcelo Tosatti, qemu-devel,
	haxm-team, Wenchao Wang, Sunil Muthuswamy, Richard Henderson

On 7/11/20 11:39 AM, Paolo Bonzini wrote:
> On 11/07/20 11:14, Claudio Fontana wrote:
>> On 7/11/20 12:45 AM, Paolo Bonzini wrote:
>>> On 10/07/20 06:36, Thomas Huth wrote:
>>>>
>>>> In short this goes away if I again set icount to enabled for qtest,
>>>> basically ensuring that --enable-tcg is there and then reenabling icount.
>>>>
>>>> qtest was forcing icount and shift=0 by creating qemu options, in order to misuse its counter feature,
>>>> instead of using a separate counter.
>>>
>>> Why would it need a separate counter?  In both cases it's a
>>> manually-updated counter that is used for QEMU_CLOCK_VIRTUAL.  The only
>>> difference is that shift > 0 doesn't make sense for qtest.
>>
>> I think I would reverse the question. Why reuse for qtest a counter that has absolutely nothing to do with it?
>>
>> qtest has nothing to do with instruction counting.
> 
> Apart from the name, icount is more like deterministic execution than

Maybe we should start choosing names more carefully in a way to express what we mean?

> instruction counting (it's not a coincidence that record/replay is
> fundamentally based on icount).

record/replay is tcg-only.

>  qtests need to be deterministic and
> describe which qtest instructions run before a given timer fires and
> which run after.
> 
> And in both cases, determinism is achieved by controlling the
> advancement of QEMU_CLOCK_VIRTUAL.  It's only this central component of
> icount that is shared by qtest and TCG, and I think the problem is that
> this patch conflates all of them together:

I think that the existing code in master conflates them together actually.
Qtest can have its own counter, it does not need to be the icount instruction counter.


> 
> - the basic "is QEMU_CLOCK_VIRTUAL software-driven" part is embedded in
> qemu-timer and should not be carved out into a separate module.  This
> includes the use_icount variable, which should be kept in core QEMU code.

I don't see how this follows, how is using a global use_icount variable better than having this checked using icount_enabled()?

> 
> - the fact qtest uses -icount instead of configuring the variables
> directly is definitely a hack and can be removed.
> 
> - the adaptive frequency adjustment is definitely TCG specific, and so
> are the particular functions in cpus.c that test icount_enabled() and
> broke with this patch.  All this code should be included in the TCG
> module only or, before that, should be made conditional on $(CONFIG_TCG).
> 
> So I think this patch should have been the last, not the first. :)  Once
> you move all the accelerator runtime code from cpus.c to separate files,
> it will be possible to move the frequency adjustment and deadline
> management code into accel/tcg.  And then it will be obvious which code
> is not TCG-specific and can be extracted for convenience into a
> cpu-timers.c file.
> 
> Thanks,
> 
> Paolo
> 

I will come back to this later on, this patch seems to have uncovered an underlying issue, which shows on s390.

I'd rather now continue investigating that, choosing to try to actually understand the issue, rather than hiding it under the carpet.

Thanks,

Claudio


^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH 3/3] cpu-timers, icount: new modules
  2020-07-11 11:49                   ` Claudio Fontana
@ 2020-07-11 12:19                     ` Paolo Bonzini
  2020-07-11 12:48                       ` Claudio Fontana
  2020-07-29  8:48                       ` Claudio Fontana
  0 siblings, 2 replies; 37+ messages in thread
From: Paolo Bonzini @ 2020-07-11 12:19 UTC (permalink / raw)
  To: Claudio Fontana, Thomas Huth, Laurent Vivier, Alex Bennée,
	Peter Maydell, Philippe Mathieu-Daudé,
	Roman Bolshakov, Markus Armbruster
  Cc: Eduardo Habkost, Colin Xu, Marcelo Tosatti, qemu-devel,
	haxm-team, Wenchao Wang, Sunil Muthuswamy, Richard Henderson

On 11/07/20 13:49, Claudio Fontana wrote:
>> Apart from the name, icount is more like deterministic execution than
> 
> Maybe we should start choosing names more carefully in a way to express what we mean?

I don't disagree.  For icount in particular however we're about 12 years
too late.

>>  qtests need to be deterministic and
>> describe which qtest instructions run before a given timer fires and
>> which run after.
>>
>> And in both cases, determinism is achieved by controlling the
>> advancement of QEMU_CLOCK_VIRTUAL.  It's only this central component of
>> icount that is shared by qtest and TCG, and I think the problem is that
>> this patch conflates all of them together:
> 
> I think that the existing code in master conflates them together actually.
> Qtest can have its own counter, it does not need to be the icount
> instruction counter.

If you want you can add to your accelerator ops series one for
qemu_get_clock_ns(QEMU_CLOCK_VIRTUAL), cpu_get_ticks() and
qemu_start_warp_timer(), that would certainly work for me; those three
are the only non-TCG-specific functions that read use_icount, as far as
I can see.  qemu_start_warp_timer() does have an "if (qtest_enabled())"
even, so it's clearly fishy.

It may even be a good idea for TCG to have three sets of accelerator ops
for respectively multi-threaded, round-robin and icount.

My point is that this patch is not the right way to start the
refactoring because *for now* it's wrong to treat icount as a TCG-only
concept.  Having more separation between accelerators, as well as a
clear interface between core and accelerators is certainly a laudable
goal though.

>> - the basic "is QEMU_CLOCK_VIRTUAL software-driven" part is embedded in
>> qemu-timer and should not be carved out into a separate module.  This
>> includes the use_icount variable, which should be kept in core QEMU code.
> 
> I don't see how this follows, how is using a global use_icount variable better than having this checked using icount_enabled()?

If you can get rid of use_icount using a new accelerator ops member, it
would be even better. :)

> I will come back to this later on, this patch seems to have uncovered an underlying issue, which shows on s390.
> 
> I'd rather now continue investigating that, choosing to try to
> actually understand the issue, rather than hiding it under the
> carpet.

Thanks.  But I don't think it's sweeping anything under the carpet; it's
great if we find a currently latent s390 bug, but it is orthogonal to
the design of that core<->accelerator interface.

(And by the way, my suggested patch to icount_enabled() was completely
wrong!).

Paolo



^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH 3/3] cpu-timers, icount: new modules
  2020-07-11 12:19                     ` Paolo Bonzini
@ 2020-07-11 12:48                       ` Claudio Fontana
  2020-07-29  8:48                       ` Claudio Fontana
  1 sibling, 0 replies; 37+ messages in thread
From: Claudio Fontana @ 2020-07-11 12:48 UTC (permalink / raw)
  To: Paolo Bonzini, Thomas Huth, Laurent Vivier, Alex Bennée,
	Peter Maydell, Philippe Mathieu-Daudé,
	Roman Bolshakov, Markus Armbruster
  Cc: Eduardo Habkost, Marcelo Tosatti, qemu-devel, haxm-team,
	Wenchao Wang, Sunil Muthuswamy, Richard Henderson, Colin Xu

On 7/11/20 2:19 PM, Paolo Bonzini wrote:
> On 11/07/20 13:49, Claudio Fontana wrote:
>>> Apart from the name, icount is more like deterministic execution than
>>
>> Maybe we should start choosing names more carefully in a way to express what we mean?
> 
> I don't disagree.  For icount in particular however we're about 12 years
> too late.
> 
>>>  qtests need to be deterministic and
>>> describe which qtest instructions run before a given timer fires and
>>> which run after.
>>>
>>> And in both cases, determinism is achieved by controlling the
>>> advancement of QEMU_CLOCK_VIRTUAL.  It's only this central component of
>>> icount that is shared by qtest and TCG, and I think the problem is that
>>> this patch conflates all of them together:
>>
>> I think that the existing code in master conflates them together actually.
>> Qtest can have its own counter, it does not need to be the icount
>> instruction counter.
> 
> If you want you can add to your accelerator ops series one for
> qemu_get_clock_ns(QEMU_CLOCK_VIRTUAL), cpu_get_ticks() and
> qemu_start_warp_timer(), that would certainly work for me; those three
> are the only non-TCG-specific functions that read use_icount, as far as
> I can see.  qemu_start_warp_timer() does have an "if (qtest_enabled())"
> even, so it's clearly fishy.
> 
> It may even be a good idea for TCG to have three sets of accelerator ops
> for respectively multi-threaded, round-robin and icount.
> 
> My point is that this patch is not the right way to start the
> refactoring because *for now* it's wrong to treat icount as a TCG-only
> concept.  Having more separation between accelerators, as well as a
> clear interface between core and accelerators is certainly a laudable
> goal though.
> 
>>> - the basic "is QEMU_CLOCK_VIRTUAL software-driven" part is embedded in
>>> qemu-timer and should not be carved out into a separate module.  This
>>> includes the use_icount variable, which should be kept in core QEMU code.
>>
>> I don't see how this follows, how is using a global use_icount variable better than having this checked using icount_enabled()?
> 
> If you can get rid of use_icount using a new accelerator ops member, it
> would be even better. :)
> 
>> I will come back to this later on, this patch seems to have uncovered an underlying issue, which shows on s390.
>>
>> I'd rather now continue investigating that, choosing to try to
>> actually understand the issue, rather than hiding it under the
>> carpet.
> 
> Thanks.  But I don't think it's sweeping anything under the carpet; it's
> great if we find a currently latent s390 bug, but it is orthogonal to
> the design of that core<->accelerator interface.

Yes, absolutely this is what I wanted to express.

I would like to find out what the problem is that appears in s390,
I am not sure though that it is actually an s390-specific problem, it could even be a migration qemu-file issue,
as apparently just flushing with qemu_fflush(f) "fixes" it.

My patch made the stream a bit smaller, and changed the layout of the s390-skeys, which have an interesting field length (32768),
I wonder if I got just the right alignment to trigger a bug where the qemu-file buffer is not properly flushed.

> 
> (And by the way, my suggested patch to icount_enabled() was completely
> wrong!).
> 
> Paolo
> 
> 

We will come back to this later, thanks a lot for the exchange!

Ciao,

Claudio


^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH 3/3] cpu-timers, icount: new modules
  2020-07-10 19:20               ` Claudio Fontana
@ 2020-07-13 10:46                 ` Cornelia Huck
  0 siblings, 0 replies; 37+ messages in thread
From: Cornelia Huck @ 2020-07-13 10:46 UTC (permalink / raw)
  To: Claudio Fontana
  Cc: Laurent Vivier, Peter Maydell, Thomas Huth, Jason J. Herne,
	Philippe Mathieu-Daudé,
	Marcelo Tosatti, Markus Armbruster, qemu-devel, Roman Bolshakov,
	Wenchao Wang, haxm-team, Colin Xu, Paolo Bonzini,
	Sunil Muthuswamy, Richard Henderson, Alex Bennée,
	Eduardo Habkost

On Fri, 10 Jul 2020 21:20:08 +0200
Claudio Fontana <cfontana@suse.de> wrote:

> >>> In short this goes away if I again set icount to enabled for qtest,
> >>> basically ensuring that --enable-tcg is there and then reenabling icount.
> >>>
> >>> qtest was forcing icount and shift=0 by creating qemu options, in order to misuse its counter feature,
> >>> instead of using a separate counter.
> >>>
> >>> Removing that ugliness we end up with different behavior of save/load, because vmstate will now suddenly not contain icount-related values anymore.
> >>> What I do not understand is why this causes a problem because save should just not store the icount state and load should just not load the icount state,
> >>> and why we die on the load of s390 keys state (it works just fine for other architectures).  
> > 
> > Yes, I don't really see why skeys is so special. No endianness stuff, I
> > assume?  
> 
> 
> No, does not seem to be the issue.

Hm, had been worth a thought.

> 
> I discovered a way simpler way to "fix" it: 
> 
> static bool icount_state_needed(void *opaque)
> {
>     return 1;
> }
> 
> Ie, making sure that the state is always saved/restored, even when unused.
> 
> Really weird.
> 
> I logged/debugged the vmstate code, and I can see that things seem symmetric between save and load when it comes to timers.
> 
> something puts 0s into the key somehow...

Maybe writing one 0 to many?



^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH 3/3] cpu-timers, icount: new modules
  2020-07-11 11:40               ` Claudio Fontana
@ 2020-07-13 10:51                 ` Cornelia Huck
  2020-07-13 11:27                   ` Claudio Fontana
  0 siblings, 1 reply; 37+ messages in thread
From: Cornelia Huck @ 2020-07-13 10:51 UTC (permalink / raw)
  To: Claudio Fontana
  Cc: Jason J. Herne, Laurent Vivier, Thomas Huth, Eduardo Habkost,
	Colin Xu, Peter Maydell, Philippe Mathieu-Daudé,
	Marcelo Tosatti, Markus Armbruster, qemu-devel, Roman Bolshakov,
	haxm-team, Wenchao Wang, Paolo Bonzini, Sunil Muthuswamy,
	Alex Bennée, Richard Henderson

On Sat, 11 Jul 2020 13:40:50 +0200
Claudio Fontana <cfontana@suse.de> wrote:

> I found out something that for me shows that more investigation here is warranted.
> 
> 
> Here is my latest workaround for the problem:
> 
> 
> 
> diff --git a/hw/s390x/s390-skeys.c b/hw/s390x/s390-skeys.c
> index 1e036cc602..47c9a015af 100644
> --- a/hw/s390x/s390-skeys.c
> +++ b/hw/s390x/s390-skeys.c
> @@ -252,6 +252,8 @@ static const TypeInfo qemu_s390_skeys_info = {
>      .class_size    = sizeof(S390SKeysClass),
>  };
>  
> +extern void qemu_fflush(QEMUFile *f);
> +
>  static void s390_storage_keys_save(QEMUFile *f, void *opaque)
>  {
>      S390SKeysState *ss = S390_SKEYS(opaque);
> @@ -302,6 +304,7 @@ static void s390_storage_keys_save(QEMUFile *f, void *opaque)
>      g_free(buf);
>  end_stream:
>      qemu_put_be64(f, eos);
> +    qemu_fflush(f);
>  }
>  
>  static int s390_storage_keys_load(QEMUFile *f, void *opaque, int version_id)
> ------------------------------------------------------------------------------------
> 
> 
> I think that this might imply that my patch changing the migration stream has only triggered an existing problem.

Looks a bit like it.

> 
> The sympthom is: the load keys code does not see the EOS (byte value 1).
> It does see the keys (which are all empty in the test, ie 32678 times the byte value 0). 

Yes, that (zero keys) is expected.

> 
> The workaround for the sympthom: flush the qemu file after putting the EOS in there.
> 
> 
> Any ideas on where to investigate next?

Do any other users of the SaveVMHandlers interface see errors as well
(or do they do the fflush dance)?



^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH 3/3] cpu-timers, icount: new modules
  2020-07-13 10:51                 ` Cornelia Huck
@ 2020-07-13 11:27                   ` Claudio Fontana
  0 siblings, 0 replies; 37+ messages in thread
From: Claudio Fontana @ 2020-07-13 11:27 UTC (permalink / raw)
  To: Cornelia Huck
  Cc: Jason J. Herne, Laurent Vivier, Thomas Huth, Eduardo Habkost,
	Colin Xu, Peter Maydell, Philippe Mathieu-Daudé,
	Marcelo Tosatti, Markus Armbruster, qemu-devel, Roman Bolshakov,
	haxm-team, Wenchao Wang, Paolo Bonzini, Sunil Muthuswamy,
	Alex Bennée, Richard Henderson

On 7/13/20 12:51 PM, Cornelia Huck wrote:
> On Sat, 11 Jul 2020 13:40:50 +0200
> Claudio Fontana <cfontana@suse.de> wrote:
> 
>> I found out something that for me shows that more investigation here is warranted.
>>
>>
>> Here is my latest workaround for the problem:
>>
>>
>>
>> diff --git a/hw/s390x/s390-skeys.c b/hw/s390x/s390-skeys.c
>> index 1e036cc602..47c9a015af 100644
>> --- a/hw/s390x/s390-skeys.c
>> +++ b/hw/s390x/s390-skeys.c
>> @@ -252,6 +252,8 @@ static const TypeInfo qemu_s390_skeys_info = {
>>      .class_size    = sizeof(S390SKeysClass),
>>  };
>>  
>> +extern void qemu_fflush(QEMUFile *f);
>> +
>>  static void s390_storage_keys_save(QEMUFile *f, void *opaque)
>>  {
>>      S390SKeysState *ss = S390_SKEYS(opaque);
>> @@ -302,6 +304,7 @@ static void s390_storage_keys_save(QEMUFile *f, void *opaque)
>>      g_free(buf);
>>  end_stream:
>>      qemu_put_be64(f, eos);
>> +    qemu_fflush(f);
>>  }
>>  
>>  static int s390_storage_keys_load(QEMUFile *f, void *opaque, int version_id)
>> ------------------------------------------------------------------------------------
>>
>>
>> I think that this might imply that my patch changing the migration stream has only triggered an existing problem.
> 
> Looks a bit like it.
> 
>>
>> The sympthom is: the load keys code does not see the EOS (byte value 1).
>> It does see the keys (which are all empty in the test, ie 32678 times the byte value 0). 
> 
> Yes, that (zero keys) is expected.
> 
>>
>> The workaround for the sympthom: flush the qemu file after putting the EOS in there.
>>
>>
>> Any ideas on where to investigate next?
> 
> Do any other users of the SaveVMHandlers interface see errors as well
> (or do they do the fflush dance)?
> 

Hi Cornelia,

just want to point you also to this discussion that I made outside of the context of this particular patch, with a much simpler reproducer:

https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg03969.html

There do not seem to be many users of the "Old style" vmstate_save as it is called in the code.

I could find only:

fgrep -R -- ".save_state"

hw/s390x/tod.c:    .save_state = s390_tod_save,
hw/s390x/s390-skeys.c:    .save_state = s390_storage_keys_save,
net/slirp.c:    .save_state = net_slirp_state_save,

It is difficult to see what does fflush where, and which methods are supposed to do what,
because we have quite a few methods in SaveVMHandlers and VMStateDescription.

Some of the fflushes are just done in the code just after writing the EOS,
and in some cases there is no fflush after writing the EOS, but there are other methods called at completion time as far as I understand, where the fflush is done.

The issue in the stream seems to appear only just after the s390 keys are written.

Maybe better to continue the discussion at: 

https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg03969.html

?
Otherwise it is fine, I can follow two threads if you prefer so.

Thanks,

Claudio








^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH 3/3] cpu-timers, icount: new modules
  2020-07-11 12:19                     ` Paolo Bonzini
  2020-07-11 12:48                       ` Claudio Fontana
@ 2020-07-29  8:48                       ` Claudio Fontana
  2020-07-29 10:01                         ` Paolo Bonzini
  1 sibling, 1 reply; 37+ messages in thread
From: Claudio Fontana @ 2020-07-29  8:48 UTC (permalink / raw)
  To: Paolo Bonzini, Thomas Huth, Laurent Vivier, Alex Bennée,
	Peter Maydell, Philippe Mathieu-Daudé,
	Roman Bolshakov, Markus Armbruster
  Cc: Eduardo Habkost, Colin Xu, Marcelo Tosatti, qemu-devel,
	haxm-team, Wenchao Wang, Sunil Muthuswamy, Richard Henderson

Hi Paolo,

now that things exposed by my patch are fixed, back on this topic :-)

On 7/11/20 2:19 PM, Paolo Bonzini wrote:
> On 11/07/20 13:49, Claudio Fontana wrote:
>>> Apart from the name, icount is more like deterministic execution than
>>
>> Maybe we should start choosing names more carefully in a way to express what we mean?
> 
> I don't disagree.  For icount in particular however we're about 12 years
> too late.
> 
>>>  qtests need to be deterministic and
>>> describe which qtest instructions run before a given timer fires and
>>> which run after.
>>>
>>> And in both cases, determinism is achieved by controlling the
>>> advancement of QEMU_CLOCK_VIRTUAL.  It's only this central component of
>>> icount that is shared by qtest and TCG, and I think the problem is that
>>> this patch conflates all of them together:
>>
>> I think that the existing code in master conflates them together actually.
>> Qtest can have its own counter, it does not need to be the icount
>> instruction counter.
> 
> If you want you can add to your accelerator ops series one for
> qemu_get_clock_ns(QEMU_CLOCK_VIRTUAL), cpu_get_ticks() and
> qemu_start_warp_timer(), that would certainly work for me;


The problem I see here is, as usual, one of meaning.

Are qemu_get_clock_ns, cpu_get_ticks and qemu_start_warp_timer
accelerator-specific cpu interfaces?

Looking at their implementation, currently I don't think they are, what do you think?

Should these be grouped together with

create_vcpu_thread,
kick_vcpu_thread,
synchronize_cpu_state

in the same interface?



> those three
> are the only non-TCG-specific functions that read use_icount, as far as
> I can see.  


There is some more use of use_icount also in non-TCG code, to either ignore icount VIRTUAL timers or produce more deterministic behavior:

dma-helpers.c checks it to make reads "more deterministic" then icount is enabled,
util/async.c indirectly checks it calling timerlistgroup_deadline_ns to do aio_compute_timeout,
as does main_loop_wait(), to check that each timer can be used for deadlines (is not an icount VIRTUAL timer).
hw/core/ptimer.c checks it to again offer more deterministic behavior for icount,
and there is some use of it in target/riscv/csr.c (curious, as I would expect to rely on cpu_get_host_ticks instead of using icount directly).
and the notify_cb() in timerlist_notify() also checks it.

May be more..?


> qemu_start_warp_timer() does have an "if (qtest_enabled())"
> even, so it's clearly fishy.

[should read "qemu_start_warp_timer() does have an "if (icount_enabled())"']

It looks absolutely fishy to me.

One way it could be better I think would be to have this "if" in the places where qemu_start_warp_timer is actually called, and make it clear that this an icount-specific thing,

ie saying in

main_loop_wait()
{

if (icount_enabled()) {
  icount_start_warp_timer()
}

}

and also in timerlist_rearm saying:

if (icount_enabled()) {
  icount_start_warp_timer()
}


Regarding the actual warp, icount_warp_rt(),
note that qtest has its own "qtest_clock_warp" function,

which in my mind is just _misusing_ the icount bias field to realize its functionality:

atomic_set_i64(&timers_state.qemu_icount_bias,
               timers_state.qemu_icount_bias + warp);

instead of having its own counter.

Speculation: does separating the two even allow qtesting icount and replay functionality?


> 
> It may even be a good idea for TCG to have three sets of accelerator ops
> for respectively multi-threaded, round-robin and icount.
> 
> My point is that this patch is not the right way to start the
> refactoring because *for now* it's wrong to treat icount as a TCG-only
> concept.  


If we create a separate counter for qtest, as we do in the patch, I think is correct to treat them separately,

and check for qtest_enabled() and icount_enabled() separately as necessary in each given situation,
as we actually have now already, but with the opportune adjustments to correct for the now non-shared counter.

Which means that we need to make more explicit the cases where the qtest_enabled() and icount_enabled() cases are actually matching,
which I think would benefit from being explicit.

In my understanding the cases where qtest_enabled() and icount_enabled() actually match are the ones where we want to behave more "deterministically",
either because we are testing (qtest_enabled()) or because we want a stable instruction count (icount_enabled()).


>Having more separation between accelerators, as well as a
> clear interface between core and accelerators is certainly a laudable
> goal though.
> 
>>> - the basic "is QEMU_CLOCK_VIRTUAL software-driven" part is embedded in
>>> qemu-timer and should not be carved out into a separate module.  This
>>> includes the use_icount variable, which should be kept in core QEMU code.
>>
>> I don't see how this follows, how is using a global use_icount variable better than having this checked using icount_enabled()?
> 
> If you can get rid of use_icount using a new accelerator ops member, it
> would be even better. :)


Maybe you could mention in more detail what you propose here?

To me it really seems correct to separate icount and qtest, the fact that their implementation currently ends up using the same counter is what needs to be rectified first,
but if you see a better abstraction to be able to refactor them better let me know, maybe it could be a next step?

I am not sure that the cpu accelerator interface is the right place to do this abstraction semantically, but maybe you can show me otherwise?


> 
>> I will come back to this later on, this patch seems to have uncovered an underlying issue, which shows on s390.
>>
>> I'd rather now continue investigating that, choosing to try to
>> actually understand the issue, rather than hiding it under the
>> carpet.
> 
> Thanks.  But I don't think it's sweeping anything under the carpet; it's
> great if we find a currently latent s390 bug, but it is orthogonal to
> the design of that core<->accelerator interface.



Happy to see the loadvm bug now finally fixed, it has been bothering me for a while :-)


> 
> (And by the way, my suggested patch to icount_enabled() was completely
> wrong!).
> 
> Paolo
> 

Ciao,

Claudio


^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH 3/3] cpu-timers, icount: new modules
  2020-07-29  8:48                       ` Claudio Fontana
@ 2020-07-29 10:01                         ` Paolo Bonzini
  2020-07-30 16:33                           ` Claudio Fontana
  0 siblings, 1 reply; 37+ messages in thread
From: Paolo Bonzini @ 2020-07-29 10:01 UTC (permalink / raw)
  To: Claudio Fontana, Thomas Huth, Laurent Vivier, Alex Bennée,
	Peter Maydell, Philippe Mathieu-Daudé,
	Roman Bolshakov, Markus Armbruster
  Cc: Eduardo Habkost, Colin Xu, Marcelo Tosatti, qemu-devel,
	haxm-team, Wenchao Wang, Sunil Muthuswamy, Richard Henderson

On 29/07/20 10:48, Claudio Fontana wrote:
>> If you want you can add to your accelerator ops series one for
>> qemu_get_clock_ns(QEMU_CLOCK_VIRTUAL), cpu_get_ticks() and
>> qemu_start_warp_timer(), that would certainly work for me;
> 
> The problem I see here is, as usual, one of meaning.
> 
> Are qemu_get_clock_ns, cpu_get_ticks and qemu_start_warp_timer
> accelerator-specific cpu interfaces?

qemu_get_clock_ns(QEMU_CLOCK_VIRTUAL) is because it needs to take icount
into account, likewise for cpu_get_ticks(); icount is TCG and qtest
specific (with subtle differences between TCG makes icount optional and
qtest makes it mandatory, so further separation of the two concepts is
totally fine for me).

qemu_start_warp_timer() also is accelerator-specific because, besides
icount not being applicable to virtualizing accelerators, the warp timer
is not needed for qtest, only for TCG.

> Looking at their implementation, currently I don't think they are, what do you think?
> 
> Should these be grouped together with
> 
> create_vcpu_thread,
> kick_vcpu_thread,
> synchronize_cpu_state
> 
> in the same interface?

I think so.

Paolo



^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH 3/3] cpu-timers, icount: new modules
  2020-07-29 10:01                         ` Paolo Bonzini
@ 2020-07-30 16:33                           ` Claudio Fontana
  2020-07-30 22:09                             ` Paolo Bonzini
  0 siblings, 1 reply; 37+ messages in thread
From: Claudio Fontana @ 2020-07-30 16:33 UTC (permalink / raw)
  To: Paolo Bonzini, Thomas Huth, Laurent Vivier, Alex Bennée,
	Peter Maydell, Philippe Mathieu-Daudé,
	Roman Bolshakov, Markus Armbruster
  Cc: Eduardo Habkost, Colin Xu, Marcelo Tosatti, qemu-devel,
	haxm-team, Wenchao Wang, Sunil Muthuswamy, Richard Henderson

On 7/29/20 12:01 PM, Paolo Bonzini wrote:
> On 29/07/20 10:48, Claudio Fontana wrote:
>>> If you want you can add to your accelerator ops series one for
>>> qemu_get_clock_ns(QEMU_CLOCK_VIRTUAL), cpu_get_ticks() and
>>> qemu_start_warp_timer(), that would certainly work for me;
>>
>> The problem I see here is, as usual, one of meaning.
>>
>> Are qemu_get_clock_ns, cpu_get_ticks and qemu_start_warp_timer
>> accelerator-specific cpu interfaces?
> 
> qemu_get_clock_ns(QEMU_CLOCK_VIRTUAL) is because it needs to take icount
> into account, likewise for cpu_get_ticks(); icount is TCG and qtest
> specific (with subtle differences between TCG makes icount optional and
> qtest makes it mandatory, so further separation of the two concepts is
> totally fine for me).
> 
> qemu_start_warp_timer() also is accelerator-specific because, besides
> icount not being applicable to virtualizing accelerators, the warp timer
> is not needed for qtest, only for TCG.
> 
>> Looking at their implementation, currently I don't think they are, what do you think?
>>
>> Should these be grouped together with
>>
>> create_vcpu_thread,
>> kick_vcpu_thread,
>> synchronize_cpu_state
>>
>> in the same interface?
> 
> I think so.
> 
> Paolo
> 

One problem I noticed is that qemu_clock_get_ns is util/qemu-timer.c, which is tools _and_ softmmu,
while I tried to extract the softmmu-only timer code in softmmu/cpu-timers.c,

and the way I saw it, accelerator cpu interface was softmmu only..

Will pause and keep thinking about this.

Thanks,

Claudio



^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH 3/3] cpu-timers, icount: new modules
  2020-07-30 16:33                           ` Claudio Fontana
@ 2020-07-30 22:09                             ` Paolo Bonzini
  2020-07-31 10:59                               ` Claudio Fontana
  0 siblings, 1 reply; 37+ messages in thread
From: Paolo Bonzini @ 2020-07-30 22:09 UTC (permalink / raw)
  To: Claudio Fontana, Thomas Huth, Laurent Vivier, Alex Bennée,
	Peter Maydell, Philippe Mathieu-Daudé,
	Roman Bolshakov, Markus Armbruster
  Cc: Eduardo Habkost, Colin Xu, Marcelo Tosatti, qemu-devel,
	haxm-team, Wenchao Wang, Sunil Muthuswamy, Richard Henderson

On 30/07/20 18:33, Claudio Fontana wrote:
> One problem I noticed is that qemu_clock_get_ns is util/qemu-timer.c,
> which is tools _and_ softmmu, while I tried to extract the
> softmmu-only timer code in softmmu/cpu-timers.c,

Not all of it, only the VIRTUAL clock which is

        if (use_icount) {
            return cpu_get_icount();
        } else {
            return cpu_get_clock();
        }

and would be changed to something like

	return cpu_get_virtual_clock();

In turn cpu_get_virtual_clock() is

	return (accel_ops->cpu_get_virtual clock ?: cpu_get_clock)();

in the emulators, plus a stub that replaces stubs/cpu-get-icount.c and
is just

	return get_clock_realtime();

as in stubs/cpu-get-clock.c.

Paolo



^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH 3/3] cpu-timers, icount: new modules
  2020-07-30 22:09                             ` Paolo Bonzini
@ 2020-07-31 10:59                               ` Claudio Fontana
  0 siblings, 0 replies; 37+ messages in thread
From: Claudio Fontana @ 2020-07-31 10:59 UTC (permalink / raw)
  To: Paolo Bonzini, Thomas Huth, Laurent Vivier, Alex Bennée,
	Peter Maydell, Philippe Mathieu-Daudé,
	Roman Bolshakov, Markus Armbruster
  Cc: Eduardo Habkost, Marcelo Tosatti, qemu-devel, haxm-team,
	Wenchao Wang, Sunil Muthuswamy, Richard Henderson, Colin Xu

On 7/31/20 12:09 AM, Paolo Bonzini wrote:
> On 30/07/20 18:33, Claudio Fontana wrote:
>> One problem I noticed is that qemu_clock_get_ns is util/qemu-timer.c,
>> which is tools _and_ softmmu, while I tried to extract the
>> softmmu-only timer code in softmmu/cpu-timers.c,
> 
> Not all of it, only the VIRTUAL clock which is
> 
>         if (use_icount) {
>             return cpu_get_icount();
>         } else {
>             return cpu_get_clock();
>         }
> 
> and would be changed to something like
> 
> 	return cpu_get_virtual_clock();
> 
> In turn cpu_get_virtual_clock() is
> 
> 	return (accel_ops->cpu_get_virtual clock ?: cpu_get_clock)();
> 
> in the emulators, plus a stub that replaces stubs/cpu-get-icount.c and
> is just
> 
> 	return get_clock_realtime();
> 
> as in stubs/cpu-get-clock.c.
> 
> Paolo
> 
> 

works, hooking up cpu_get_ticks() also works.

qemu_start_warp_timer seems to make sense only for tcg/icount and not for qtest, which directly sets and warps the clock, is that right?

Would you want a start_warp_timer cpu accel interface that is actually useful only for tcg, to avoid if (icount_enabled()) { qemu_start_warp_timer()? }

Thanks,

Claudio


^ permalink raw reply	[flat|nested] 37+ messages in thread

end of thread, other threads:[~2020-07-31 11:00 UTC | newest]

Thread overview: 37+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-29  9:35 [PATCH 0/3] QEMU cpus.c refactoring part1 Claudio Fontana
2020-06-29  9:35 ` [PATCH 1/3] softmmu: move softmmu only files from root Claudio Fontana
2020-07-03 17:21   ` Paolo Bonzini
2020-06-29  9:35 ` [PATCH 2/3] cpu-throttle: new module, extracted from cpus.c Claudio Fontana
2020-06-29  9:35 ` [PATCH 3/3] cpu-timers, icount: new modules Claudio Fontana
2020-07-08 14:34   ` Paolo Bonzini
2020-07-08 15:00     ` Claudio Fontana
2020-07-08 15:05       ` Paolo Bonzini
2020-07-08 15:07         ` Thomas Huth
2020-07-08 15:12           ` Paolo Bonzini
2020-07-08 15:15             ` Claudio Fontana
2020-07-08 15:15             ` Thomas Huth
2020-07-08 15:17         ` Claudio Fontana
2020-07-08 15:23           ` Paolo Bonzini
2020-07-08 15:30             ` Claudio Fontana
2020-07-09 18:38         ` Claudio Fontana
2020-07-09 18:46           ` Claudio Fontana
2020-07-10  6:33             ` Cornelia Huck
2020-07-10 19:20               ` Claudio Fontana
2020-07-13 10:46                 ` Cornelia Huck
2020-07-11 11:40               ` Claudio Fontana
2020-07-13 10:51                 ` Cornelia Huck
2020-07-13 11:27                   ` Claudio Fontana
2020-07-10  4:36           ` Thomas Huth
2020-07-10 22:45             ` Paolo Bonzini
2020-07-11  9:14               ` Claudio Fontana
2020-07-11  9:39                 ` Paolo Bonzini
2020-07-11 11:49                   ` Claudio Fontana
2020-07-11 12:19                     ` Paolo Bonzini
2020-07-11 12:48                       ` Claudio Fontana
2020-07-29  8:48                       ` Claudio Fontana
2020-07-29 10:01                         ` Paolo Bonzini
2020-07-30 16:33                           ` Claudio Fontana
2020-07-30 22:09                             ` Paolo Bonzini
2020-07-31 10:59                               ` Claudio Fontana
2020-07-02  6:27 ` [PATCH 0/3] QEMU cpus.c refactoring part1 Claudio Fontana
2020-07-03 17:21   ` Paolo Bonzini

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).