[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [RFC v5 4/4] cpus: extract out accel-specific code to each accel
From: |
Alex Bennée |
Subject: |
Re: [RFC v5 4/4] cpus: extract out accel-specific code to each accel |
Date: |
Tue, 16 Jun 2020 15:16:10 +0100 |
User-agent: |
mu4e 1.5.3; emacs 28.0.50 |
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
[RFC v5 3/4] cpu-timers, icount: new modules, Claudio Fontana, 2020/06/15