Re: [RFC v5 4/4] cpus: extract out accel-specific code to each accel

["Alex Bennée" <alex.bennee@linaro.org>]

Claudio Fontana <cfontana@suse.de> writes:

> each accelerator registers a new "CpusAccel" interface
> implementation on initialization, providing functions for
> starting a vcpu, kicking a vcpu, and sychronizing state.
>
> This way the code in cpus.c is now all general softmmu code,
> nothing accelerator-specific anymore.
>
> There is still some ifdeffery for WIN32 though.
>
> Signed-off-by: Claudio Fontana <cfontana@suse.de>
> ---
>  MAINTAINERS                   |   1 +
>  accel/Makefile.objs           |   2 +-
>  accel/kvm/Makefile.objs       |   2 +
>  accel/kvm/kvm-all.c           |  15 +-
>  accel/kvm/kvm-cpus.c          |  94 +++++
>  accel/kvm/kvm-cpus.h          |  17 +
>  accel/qtest/Makefile.objs     |   2 +
>  accel/qtest/qtest-cpus.c      | 105 +++++
>  accel/qtest/qtest-cpus.h      |  17 +
>  accel/{ => qtest}/qtest.c     |   7 +
>  accel/stubs/kvm-stub.c        |   3 +-
>  accel/tcg/Makefile.objs       |   1 +
>  accel/tcg/tcg-all.c           |  12 +-
>  accel/tcg/tcg-cpus.c          | 523 ++++++++++++++++++++++++
>  accel/tcg/tcg-cpus.h          |  17 +
>  hw/core/cpu.c                 |   1 +
>  include/sysemu/cpus.h         |  32 ++
>  include/sysemu/hw_accel.h     |  57 +--
>  include/sysemu/kvm.h          |   2 +-
>  softmmu/cpus.c                | 911 ++++--------------------------------------
>  stubs/Makefile.objs           |   1 +
>  stubs/cpu-synchronize-state.c |  15 +
>  target/i386/Makefile.objs     |   7 +-
>  target/i386/hax-all.c         |   6 +-
>  target/i386/hax-cpus.c        |  85 ++++
>  target/i386/hax-cpus.h        |  17 +
>  target/i386/hax-i386.h        |   2 +
>  target/i386/hax-posix.c       |  12 +
>  target/i386/hax-windows.c     |  20 +
>  target/i386/hvf/Makefile.objs |   2 +-
>  target/i386/hvf/hvf-cpus.c    | 141 +++++++
>  target/i386/hvf/hvf-cpus.h    |  17 +
>  target/i386/hvf/hvf.c         |   3 +
>  target/i386/whpx-all.c        |   3 +
>  target/i386/whpx-cpus.c       |  96 +++++
>  target/i386/whpx-cpus.h       |  17 +
>  36 files changed, 1362 insertions(+), 903 deletions(-)
>  create mode 100644 accel/kvm/kvm-cpus.c
>  create mode 100644 accel/kvm/kvm-cpus.h
>  create mode 100644 accel/qtest/Makefile.objs
>  create mode 100644 accel/qtest/qtest-cpus.c
>  create mode 100644 accel/qtest/qtest-cpus.h
>  rename accel/{ => qtest}/qtest.c (86%)
>  create mode 100644 accel/tcg/tcg-cpus.c
>  create mode 100644 accel/tcg/tcg-cpus.h
>  create mode 100644 stubs/cpu-synchronize-state.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

Predictably for such a spider patch I got a bunch of conflicts
attempting to merge on my testing branch so only a few comments.

>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index f308537d42..ef8cbb2680 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -427,6 +427,7 @@ WHPX CPUs
>  M: Sunil Muthuswamy <sunilmut@microsoft.com>
>  S: Supported
>  F: target/i386/whpx-all.c
> +F: target/i386/whpx-cpus.c
>  F: target/i386/whp-dispatch.h
>  F: accel/stubs/whpx-stub.c
>  F: include/sysemu/whpx.h
> diff --git a/accel/Makefile.objs b/accel/Makefile.objs
> index ff72f0d030..c5e58eb53d 100644
> --- a/accel/Makefile.objs
> +++ b/accel/Makefile.objs
> @@ -1,5 +1,5 @@
>  common-obj-$(CONFIG_SOFTMMU) += accel.o
> -obj-$(call land,$(CONFIG_SOFTMMU),$(CONFIG_POSIX)) += qtest.o
> +obj-$(call land,$(CONFIG_SOFTMMU),$(CONFIG_POSIX)) += qtest/

This does raise the question if qtest is "just another" accelerator then
should we not be creating a CONFIG_QTEST symbol for explicitness?

>  obj-$(CONFIG_KVM) += kvm/
>  obj-$(CONFIG_TCG) += tcg/
>  obj-$(CONFIG_XEN) += xen/
<snip>
> +static void *qtest_cpu_thread_fn(void *arg)
> +{
> +#ifdef _WIN32
> +    error_report("qtest is not supported under Windows");
> +    exit(1);
> +#else

This is literally impossible to build isn't it?
>  
>  static int qtest_init_accel(MachineState *ms)
>  {
> +    cpus_register_accel(&qtest_cpus);
>      return 0;
>  }

I wonder if these register functions could be moved to initfns like we
use for our hardware models?

<snip>
>  
> +/*
> + * every accelerator is supposed to register this.
> + * Could be in the AccelClass instead, but ends up being too complicated
> + * to access in practice, and inefficient for each call of each method.
> + */
> +static CpusAccel cpus_accel;
> +

wait what? Does an indirection cause that much trouble? I'm surprised
given how often we use it elsewhere in the code. I guess others might
argue for a full QOM-ification of the accelerator but I think we can at
least have an indirection rather than a copy of the structure.


-- 
Alex Bennée