Claudio Fontana <cfont...@suse.de> writes:
I'm confused as to the benefit of this classification because (see
bellow).
> also centralize the registration of the cpus.c module
> accelerator operations in accel/accel-softmmu.c
>
> Consequently, rename all tcg-cpus.c, kvm-cpus.c etc to
> tcg-accel-ops.c, kvm-accel-ops.c etc, also matching the
> object type names.
>
> Signed-off-by: Claudio Fontana <cfont...@suse.de>
> ---
> accel/accel-softmmu.h | 15 ++++++
> accel/kvm/kvm-cpus.h | 2 -
> ...g-cpus-icount.h => tcg-accel-ops-icount.h} | 2 +
> accel/tcg/tcg-accel-ops-mttcg.h | 19 ++++++++
> .../tcg/{tcg-cpus-rr.h => tcg-accel-ops-rr.h} | 0
> accel/tcg/{tcg-cpus.h => tcg-accel-ops.h} | 6 +--
> include/qemu/accel.h | 2 +
> include/sysemu/accel-ops.h | 45 ++++++++++++++++++
> include/sysemu/cpus.h | 26 ++--------
> .../i386/hax/{hax-cpus.h => hax-accel-ops.h} | 2 -
> target/i386/hax/hax-windows.h | 2 +-
> .../i386/hvf/{hvf-cpus.h => hvf-accel-ops.h} | 2 -
> .../whpx/{whpx-cpus.h => whpx-accel-ops.h} | 2 -
> accel/accel-common.c | 11 +++++
> accel/accel-softmmu.c | 43 +++++++++++++++--
> accel/kvm/{kvm-cpus.c => kvm-accel-ops.c} | 26 +++++++---
> accel/kvm/kvm-all.c | 2 -
> accel/qtest/qtest.c | 23 ++++++---
> ...g-cpus-icount.c => tcg-accel-ops-icount.c} | 21 +++------
> ...tcg-cpus-mttcg.c => tcg-accel-ops-mttcg.c} | 14 ++----
> .../tcg/{tcg-cpus-rr.c => tcg-accel-ops-rr.c} | 13 ++---
> accel/tcg/{tcg-cpus.c => tcg-accel-ops.c} | 47 ++++++++++++++++++-
> accel/tcg/tcg-all.c | 12 -----
> accel/xen/xen-all.c | 22 ++++++---
> bsd-user/main.c | 3 +-
> linux-user/main.c | 1 +
> softmmu/cpus.c | 12 ++---
> softmmu/vl.c | 7 ++-
> .../i386/hax/{hax-cpus.c => hax-accel-ops.c} | 31 ++++++++----
> target/i386/hax/hax-all.c | 5 +-
> target/i386/hax/hax-mem.c | 2 +-
> target/i386/hax/hax-posix.c | 2 +-
> target/i386/hax/hax-windows.c | 2 +-
> .../i386/hvf/{hvf-cpus.c => hvf-accel-ops.c} | 29 +++++++++---
> target/i386/hvf/hvf.c | 3 +-
> target/i386/hvf/x86hvf.c | 2 +-
> .../whpx/{whpx-cpus.c => whpx-accel-ops.c} | 31 ++++++++----
> target/i386/whpx/whpx-all.c | 7 +--
> MAINTAINERS | 3 +-
> accel/kvm/meson.build | 2 +-
> accel/tcg/meson.build | 8 ++--
> target/i386/hax/meson.build | 2 +-
> target/i386/hvf/meson.build | 2 +-
> target/i386/whpx/meson.build | 2 +-
> 44 files changed, 353 insertions(+), 162 deletions(-)
> create mode 100644 accel/accel-softmmu.h
> rename accel/tcg/{tcg-cpus-icount.h => tcg-accel-ops-icount.h} (88%)
> create mode 100644 accel/tcg/tcg-accel-ops-mttcg.h
> rename accel/tcg/{tcg-cpus-rr.h => tcg-accel-ops-rr.h} (100%)
> rename accel/tcg/{tcg-cpus.h => tcg-accel-ops.h} (72%)
> create mode 100644 include/sysemu/accel-ops.h
> rename target/i386/hax/{hax-cpus.h => hax-accel-ops.h} (95%)
> rename target/i386/hvf/{hvf-cpus.h => hvf-accel-ops.h} (94%)
> rename target/i386/whpx/{whpx-cpus.h => whpx-accel-ops.h} (96%)
> rename accel/kvm/{kvm-cpus.c => kvm-accel-ops.c} (72%)
> rename accel/tcg/{tcg-cpus-icount.c => tcg-accel-ops-icount.c} (89%)
> rename accel/tcg/{tcg-cpus-mttcg.c => tcg-accel-ops-mttcg.c} (92%)
> rename accel/tcg/{tcg-cpus-rr.c => tcg-accel-ops-rr.c} (97%)
> rename accel/tcg/{tcg-cpus.c => tcg-accel-ops.c} (63%)
> rename target/i386/hax/{hax-cpus.c => hax-accel-ops.c} (69%)
> rename target/i386/hvf/{hvf-cpus.c => hvf-accel-ops.c} (84%)
> rename target/i386/whpx/{whpx-cpus.c => whpx-accel-ops.c} (71%)
>
> diff --git a/accel/accel-softmmu.h b/accel/accel-softmmu.h
> new file mode 100644
> index 0000000000..5e192f1882
> --- /dev/null
> +++ b/accel/accel-softmmu.h
> @@ -0,0 +1,15 @@
> +/*
> + * QEMU System Emulation accel internal functions
> + *
> + * Copyright 2021 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 ACCEL_SOFTMMU_H
> +#define ACCEL_SOFTMMU_H
> +
> +void accel_init_ops_interfaces(AccelClass *ac);
> +
> +#endif /* ACCEL_SOFTMMU_H */
> diff --git a/accel/kvm/kvm-cpus.h b/accel/kvm/kvm-cpus.h
> index 3df732b816..bf0bd1bee4 100644
> --- a/accel/kvm/kvm-cpus.h
> +++ b/accel/kvm/kvm-cpus.h
> @@ -12,8 +12,6 @@
>
> #include "sysemu/cpus.h"
>
> -extern const CpusAccel kvm_cpus;
> -
> int kvm_init_vcpu(CPUState *cpu, Error **errp);
> int kvm_cpu_exec(CPUState *cpu);
> void kvm_destroy_vcpu(CPUState *cpu);
> diff --git a/accel/tcg/tcg-cpus-icount.h b/accel/tcg/tcg-accel-ops-icount.h
> similarity index 88%
> rename from accel/tcg/tcg-cpus-icount.h
> rename to accel/tcg/tcg-accel-ops-icount.h
> index b695939dfa..d884aa2aaa 100644
> --- a/accel/tcg/tcg-cpus-icount.h
> +++ b/accel/tcg/tcg-accel-ops-icount.h
> @@ -14,4 +14,6 @@ void icount_handle_deadline(void);
> void icount_prepare_for_run(CPUState *cpu);
> void icount_process_data(CPUState *cpu);
>
> +void icount_handle_interrupt(CPUState *cpu, int mask);
> +
> #endif /* TCG_CPUS_ICOUNT_H */
> diff --git a/accel/tcg/tcg-accel-ops-mttcg.h b/accel/tcg/tcg-accel-ops-mttcg.h
> new file mode 100644
> index 0000000000..9fdc5a2ab5
> --- /dev/null
> +++ b/accel/tcg/tcg-accel-ops-mttcg.h
> @@ -0,0 +1,19 @@
> +/*
> + * QEMU TCG Multi Threaded vCPUs implementation
> + *
> + * Copyright 2021 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 TCG_CPUS_MTTCG_H
> +#define TCG_CPUS_MTTCG_H
> +
> +/* kick MTTCG vCPU thread */
> +void mttcg_kick_vcpu_thread(CPUState *cpu);
> +
> +/* start an mttcg vCPU thread */
> +void mttcg_start_vcpu_thread(CPUState *cpu);
> +
> +#endif /* TCG_CPUS_MTTCG_H */
> diff --git a/accel/tcg/tcg-cpus-rr.h b/accel/tcg/tcg-accel-ops-rr.h
> similarity index 100%
> rename from accel/tcg/tcg-cpus-rr.h
> rename to accel/tcg/tcg-accel-ops-rr.h
> diff --git a/accel/tcg/tcg-cpus.h b/accel/tcg/tcg-accel-ops.h
> similarity index 72%
> rename from accel/tcg/tcg-cpus.h
> rename to accel/tcg/tcg-accel-ops.h
> index d6893a32f8..48130006de 100644
> --- a/accel/tcg/tcg-cpus.h
> +++ b/accel/tcg/tcg-accel-ops.h
> @@ -14,12 +14,8 @@
>
> #include "sysemu/cpus.h"
>
> -extern const CpusAccel tcg_cpus_mttcg;
> -extern const CpusAccel tcg_cpus_icount;
> -extern const CpusAccel tcg_cpus_rr;
> -
> void tcg_cpus_destroy(CPUState *cpu);
> int tcg_cpus_exec(CPUState *cpu);
> -void tcg_cpus_handle_interrupt(CPUState *cpu, int mask);
> +void tcg_handle_interrupt(CPUState *cpu, int mask);
>
> #endif /* TCG_CPUS_H */
> diff --git a/include/qemu/accel.h b/include/qemu/accel.h
> index fac4a18703..b9d6d69eb8 100644
> --- a/include/qemu/accel.h
> +++ b/include/qemu/accel.h
> @@ -69,6 +69,8 @@ typedef struct AccelClass {
> AccelClass *accel_find(const char *opt_name);
> AccelState *current_accel(void);
>
> +void accel_init_interfaces(AccelClass *ac);
> +
> #ifndef CONFIG_USER_ONLY
> int accel_init_machine(AccelState *accel, MachineState *ms);
>
> diff --git a/include/sysemu/accel-ops.h b/include/sysemu/accel-ops.h
> new file mode 100644
> index 0000000000..032f6979d7
> --- /dev/null
> +++ b/include/sysemu/accel-ops.h
> @@ -0,0 +1,45 @@
> +/*
> + * Accelerator OPS, used for cpus.c module
> + *
> + * Copyright 2021 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.
> + */
AhH I think the comment from the earlier patch was intended to be added
in this one ;-)
> +/* initialize the arch-independent accel operation interfaces */
> +void accel_init_ops_interfaces(AccelClass *ac)
> +{
> + const char *ac_name;
> + char *ops_name;
> + AccelOpsClass *ops;
> +
> + ac_name = object_class_get_name(OBJECT_CLASS(ac));
> + g_assert(ac_name != NULL);
> +
> + ops_name = g_strdup_printf("%s" ACCEL_OPS_SUFFIX, ac_name);
> + ops = ACCEL_OPS_CLASS(object_class_by_name(ops_name));
> + g_free(ops_name);
> +
> + /*
> + * all accelerators need to define ops, providing at least a mandatory
> + * non-NULL create_vcpu_thread operation.
> + */
> + g_assert(ops != NULL);
If create_vcpu_thread is mandatory then surely:
g_assert(ops && ops->create_vcpu_thread);
> + if (ops->ops_init) {
> + ops->ops_init(ops);
> + }
> + cpus_register_accel(ops);
> +}
> +
<snip>
>
> -const CpusAccel kvm_cpus = {
> - .create_vcpu_thread = kvm_start_vcpu_thread,
> +static void kvm_accel_ops_class_init(ObjectClass *oc, void *data)
> +{
> + AccelOpsClass *ops = ACCEL_OPS_CLASS(oc);
>
> - .synchronize_post_reset = kvm_cpu_synchronize_post_reset,
> - .synchronize_post_init = kvm_cpu_synchronize_post_init,
> - .synchronize_state = kvm_cpu_synchronize_state,
> - .synchronize_pre_loadvm = kvm_cpu_synchronize_pre_loadvm,
> + ops->create_vcpu_thread = kvm_start_vcpu_thread;
> + ops->synchronize_post_reset = kvm_cpu_synchronize_post_reset;
> + ops->synchronize_post_init = kvm_cpu_synchronize_post_init;
> + ops->synchronize_state = kvm_cpu_synchronize_state;
> + ops->synchronize_pre_loadvm = kvm_cpu_synchronize_pre_loadvm;
> };
(continuing)
comparing the above with...
> +
> +static void tcg_accel_ops_init(AccelOpsClass *ops)
> +{
> + if (qemu_tcg_mttcg_enabled()) {
> + ops->create_vcpu_thread = mttcg_start_vcpu_thread;
> + ops->kick_vcpu_thread = mttcg_kick_vcpu_thread;
> + ops->handle_interrupt = tcg_handle_interrupt;
> +
> + } else if (icount_enabled()) {
> + ops->create_vcpu_thread = rr_start_vcpu_thread;
> + ops->kick_vcpu_thread = rr_kick_vcpu_thread;
> + ops->handle_interrupt = icount_handle_interrupt;
> + ops->get_virtual_clock = icount_get;
> + ops->get_elapsed_ticks = icount_get;
> +
> + } else {
> + ops->create_vcpu_thread = rr_start_vcpu_thread;
> + ops->kick_vcpu_thread = rr_kick_vcpu_thread;
> + ops->handle_interrupt = tcg_handle_interrupt;
> + }
> +}
> +
..I wonder if loosing the const structures are worth it. Why not keep
them const and have the initial assignment:
if(qemu_tcg_mttcg_enabled()) {
ops = &mttcg_ops;
} else {
...
is this an unavoidable result of the classification process? In which
case I want a better argument for it in the commit log.
--
Alex Bennée
