Re: [PATCH v7 00/16] QEMU cpus.c refactoring part2

[Claudio Fontana]

On 9/4/20 7:29 PM, Paolo Bonzini wrote:
> 
> 
> Il ven 4 set 2020, 18:08 Claudio Fontana <cfontana@suse.de 
> <mailto:cfontana@suse.de>> ha scritto:
> 
>     btw not sure whose queues should these go through?
> 
> 
> I can take this; the dust from the meson merge should settle soon.
> 
> Paolo

Thanks Paolo,

if it can help I try to keep a rebased functioning version at

https://github.com/hw-claudio/qemu.git "cpus-refactoring"

some of the CI complains about:

check: qemu-img not found
Makefile.mtest:1229: recipe for target 'check-block' failed

Ciao,

Claudio

> 
> 
>     Thanks,
> 
>     Claudio
> 
>     On 9/3/20 12:55 PM, Claudio Fontana wrote:
>     > Motivation and higher level steps:
>     >
>     > https://lists.gnu.org/archive/html/qemu-devel/2020-05/msg04628.html
>     >
>     > Current state is all reviewed, I think this is a good base to
>     > include at this point.
>     > I had to fix one build issue in patch 1 that went undetected previously 
> due to
>     > a meson thing (parallel build).
>     >
>     > Further work for future patches include:
>     >
>     > * additional improvements to naming consistency in QEMU files. 
> (Claudio, Alex, Roman)
>     >
>     > * tcg vcpu start refactoring, providing then multiple structs for mttg, 
> icount, normal.
>     >   (Claudio, Alex)
>     >
>     > * possible removal of NULL check for virtual clock if the qcow2 early 
> call is solved.
>     >   (Claudio, Paolo, Berto)
>     >
>     > PATCH v6 -> PATCH v7:
>     >
>     > * in patch "cpu-timers, icount: new modules":
>     >   - remove redundant double initialization of spice
>     >   - remove lingering calls to icount_get
>     >
>     > PATCH v5 -> PATCH v6:
>     >
>     > * moved to mason build system
>     >
>     > * patch (new): cpus: remove checks for non-NULL cpus_accel (Richard)
>     >
>     >   This has however a big caveat: in some cases the virtual clock is
>     >   queried before an accelerator is set or ticks are enabled; this is
>     >   currently special cased (keeping the NULL check in 
> cpus_get_virtual_clock),
>     >   but maybe this should not happen at all? (Paolo, Berto)
>     >
>     > * in patch "cpu-timers, icount: new modules"
>     >   do not change (yet) icount_enabled() to a function.
>     >   Mimic instead what is done with tcg_enabled(). (Richard)
>     >
>     > * split the changes into two separate patches, with name-only changes
>     >   extracted out into a separate patch (Richard).
>     >   Removed existing Reviewed-by because of these changes (Alex)-
>     >   Alex are you ok with them?
>     >
>     > * in patch "cpus: prepare new CpusAccel cpu accelerator interface"
>     >   remove some unneeded stubs from stubs/cpu-synchronize-state.c
>     >   Use const for the CpusAccel interface. (Richard)
>     >
>     > * in patch "cpus: extract out TCG-specific code to accel/tcg"
>     >   use const for the CpusAccel interface. (Richard)
>     >
>     > * in patch "cpus: extract out qtest-specific code to accel/qtest"
>     >   use const for the CpusAccel interface;
>     >   use g_assert_not_reached (Richard)
>     >
>     > * in patch "cpus: extract out kvm-specific code to accel/kvm"
>     >   use const for the CpusAccel interface. (Richard)
>     >
>     > * in patch "cpus: extract out hax-specific code to target/i386/"
>     >   use const for the CpusAccel interface. (Richard)
>     >
>     > * in patch "cpus: extract out whpx-specific code to target/i386/"
>     >   use const for the CpusAccel interface. (Richard)
>     >
>     > * in patch "cpus: extract out hvf-specific code to target/i386/hvf/"
>     >   use const for the CpusAccel interface. (Richard)
>     >
>     >
>     > RFC v4 -> PATCH v5:
>     >
>     > * in patch 2, move comment about cpus_get_elapsed_ticks from patch 3
>     >   (Philippe)
>     >
>     > * in patch 11-14, do not create separate xxx-int.h files,
>     >   instead use the xxx-cpus.h files (Philippe)
>     >
>     > RFC v3 -> v4:
>     >
>     > * added patch 9: cleanup unneeded includes
>     >
>     > * added patch 10: add handle_interrupt to the interface (Roman)
>     >
>     > * added patch 11-14: remove accelerator specific internal functions
>     >   from global includes (Roman)
>     >
>     > * in patch 2, removed leftover "if hvf_enabled" hunk
>     >
>     > * in patch 2, convert if (!tcg_enabled) with more punctual if 
> (hax_enabled)
>     >   when eating dummy APC
>     >
>     > ----
>     >
>     > RFC v2 -> v3:
>     >
>     > * provided defaults for all methods.
>     >   Only create_vcpu_thread is now a mandatory field. (Paolo)
>     >
>     > * separated new CpusAccel patch from its first user, new patch nr. 2:
>     >   "cpus: prepare new CpusAccel cpu accelerator interface"
>     >
>     > * new CpusAccel methods: get_virtual_clock and get_elapsed_ticks.
>     >   (Paolo)
>     >
>     >   In this series, get_virtual_clock has a separate implementation
>     >   between TCG/icount and qtest,
>     >   while get_elapsed_ticks only returns a virtual counter for icount.
>     >
>     >   Looking for more comments in this area.
>     >
>     > ----
>     >
>     > RFC v1 -> v2:
>     >
>     > * split the cpus.c accelerator refactoring into 6 patches.
>     >
>     > * other minor changes to be able to proceed step by step.
>     >
>     > ----
>     >
>     > * Rebased on commit 255ae6e2158c743717bed76c9a2365ee4bcd326e,
>     > "replay: notify the main loop when there are no instructions"
>     >
>     > [SPLIT into part1 and part2]
>     >
>     > ----
>     >
>     > 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 (16):
>     >   cpu-timers, icount: new modules
>     >   icount: rename functions to be consistent with the module name
>     >   cpus: prepare new CpusAccel cpu accelerator interface
>     >   cpus: extract out TCG-specific code to accel/tcg
>     >   cpus: extract out qtest-specific code to accel/qtest
>     >   cpus: extract out kvm-specific code to accel/kvm
>     >   cpus: extract out hax-specific code to target/i386/
>     >   cpus: extract out whpx-specific code to target/i386/
>     >   cpus: extract out hvf-specific code to target/i386/hvf/
>     >   cpus: cleanup now unneeded includes
>     >   cpus: remove checks for non-NULL cpus_accel
>     >   cpus: add handle_interrupt to the CpusAccel interface
>     >   hvf: remove hvf specific functions from global includes
>     >   whpx: remove whpx specific functions from global includes
>     >   hax: remove hax specific functions from global includes
>     >   kvm: remove kvm specific functions from global includes
>     >
>     >  MAINTAINERS                    |    5 +-
>     >  accel/kvm/kvm-all.c            |   14 +-
>     >  accel/kvm/kvm-cpus.c           |   88 ++
>     >  accel/kvm/kvm-cpus.h           |   24 +
>     >  accel/kvm/meson.build          |    5 +-
>     >  accel/meson.build              |    2 +-
>     >  accel/qtest/meson.build        |    7 +
>     >  accel/qtest/qtest-cpus.c       |   91 ++
>     >  accel/qtest/qtest-cpus.h       |   17 +
>     >  accel/{ => qtest}/qtest.c      |   13 +-
>     >  accel/stubs/hax-stub.c         |   10 -
>     >  accel/stubs/hvf-stub.c         |   30 -
>     >  accel/stubs/kvm-stub.c         |   23 -
>     >  accel/stubs/meson.build        |    2 -
>     >  accel/stubs/whpx-stub.c        |   47 -
>     >  accel/tcg/cpu-exec.c           |   43 +-
>     >  accel/tcg/meson.build          |    2 +-
>     >  accel/tcg/tcg-all.c            |   43 +-
>     >  accel/tcg/tcg-cpus.c           |  569 +++++++++++
>     >  accel/tcg/tcg-cpus.h           |   17 +
>     >  accel/tcg/translate-all.c      |    3 +-
>     >  dma-helpers.c                  |    4 +-
>     >  docs/replay.txt                |    6 +-
>     >  exec.c                         |    4 -
>     >  hw/core/cpu.c                  |   14 +-
>     >  hw/core/ptimer.c               |    8 +-
>     >  hw/i386/x86.c                  |    3 +-
>     >  include/exec/cpu-all.h         |    4 +
>     >  include/exec/exec-all.h        |    4 +-
>     >  include/hw/core/cpu.h          |   14 -
>     >  include/qemu/timer.h           |   24 +-
>     >  include/sysemu/cpu-timers.h    |   90 ++
>     >  include/sysemu/cpus.h          |   50 +-
>     >  include/sysemu/hax.h           |   17 -
>     >  include/sysemu/hvf.h           |    8 -
>     >  include/sysemu/hw_accel.h      |   69 +-
>     >  include/sysemu/kvm.h           |    7 -
>     >  include/sysemu/qtest.h         |    2 +
>     >  include/sysemu/replay.h        |    4 +-
>     >  include/sysemu/whpx.h          |   19 -
>     >  replay/replay.c                |    6 +-
>     >  softmmu/cpu-timers.c           |  279 ++++++
>     >  softmmu/cpus.c                 | 1697 +++-----------------------------
>     >  softmmu/icount.c               |  492 +++++++++
>     >  softmmu/meson.build            |   10 +-
>     >  softmmu/qtest.c                |   34 +-
>     >  softmmu/timers-state.h         |   69 ++
>     >  softmmu/vl.c                   |    8 +-
>     >  stubs/clock-warp.c             |    7 -
>     >  stubs/cpu-get-clock.c          |    3 +-
>     >  stubs/cpu-get-icount.c         |   21 -
>     >  stubs/cpu-synchronize-state.c  |    9 +
>     >  stubs/cpus-get-virtual-clock.c |    8 +
>     >  stubs/icount.c                 |   45 +
>     >  stubs/meson.build              |    6 +-
>     >  stubs/qemu-timer-notify-cb.c   |    8 +
>     >  stubs/qtest.c                  |    5 +
>     >  target/alpha/translate.c       |    3 +-
>     >  target/arm/helper.c            |    7 +-
>     >  target/i386/hax-all.c          |   17 +-
>     >  target/i386/hax-cpus.c         |   84 ++
>     >  target/i386/hax-cpus.h         |   33 +
>     >  target/i386/hax-i386.h         |    2 +
>     >  target/i386/hax-mem.c          |    2 +-
>     >  target/i386/hax-posix.c        |   13 +-
>     >  target/i386/hax-windows.c      |   22 +-
>     >  target/i386/hax-windows.h      |    2 +
>     >  target/i386/hvf/hvf-cpus.c     |  131 +++
>     >  target/i386/hvf/hvf-cpus.h     |   25 +
>     >  target/i386/hvf/hvf.c          |   12 +-
>     >  target/i386/hvf/meson.build    |    1 +
>     >  target/i386/hvf/x86hvf.c       |    2 +
>     >  target/i386/hvf/x86hvf.h       |    1 -
>     >  target/i386/meson.build        |   14 +-
>     >  target/i386/whpx-all.c         |   13 +-
>     >  target/i386/whpx-cpus.c        |   96 ++
>     >  target/i386/whpx-cpus.h        |   34 +
>     >  target/riscv/csr.c             |    8 +-
>     >  tests/ptimer-test-stubs.c      |    5 +-
>     >  tests/test-timed-average.c     |    2 +-
>     >  util/main-loop.c               |   12 +-
>     >  util/qemu-timer.c              |   14 +-
>     >  82 files changed, 2637 insertions(+), 2031 deletions(-)
>     >  create mode 100644 accel/kvm/kvm-cpus.c
>     >  create mode 100644 accel/kvm/kvm-cpus.h
>     >  create mode 100644 accel/qtest/meson.build
>     >  create mode 100644 accel/qtest/qtest-cpus.c
>     >  create mode 100644 accel/qtest/qtest-cpus.h
>     >  rename accel/{ => qtest}/qtest.c (81%)
>     >  delete mode 100644 accel/stubs/hvf-stub.c
>     >  delete mode 100644 accel/stubs/whpx-stub.c
>     >  create mode 100644 accel/tcg/tcg-cpus.c
>     >  create mode 100644 accel/tcg/tcg-cpus.h
>     >  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/clock-warp.c
>     >  delete mode 100644 stubs/cpu-get-icount.c
>     >  create mode 100644 stubs/cpu-synchronize-state.c
>     >  create mode 100644 stubs/cpus-get-virtual-clock.c
>     >  create mode 100644 stubs/icount.c
>     >  create mode 100644 stubs/qemu-timer-notify-cb.c
>     >  create mode 100644 target/i386/hax-cpus.c
>     >  create mode 100644 target/i386/hax-cpus.h
>     >  create mode 100644 target/i386/hvf/hvf-cpus.c
>     >  create mode 100644 target/i386/hvf/hvf-cpus.h
>     >  create mode 100644 target/i386/whpx-cpus.c
>     >  create mode 100644 target/i386/whpx-cpus.h
>     >
>