From: "Alex Bennée" <alex.bennee@linaro.org>
To: Claudio Fontana <cfontana@suse.de>
Cc: "Laurent Vivier" <lvivier@redhat.com>,
"Peter Maydell" <peter.maydell@linaro.org>,
"Thomas Huth" <thuth@redhat.com>,
"Eduardo Habkost" <ehabkost@redhat.com>,
"Richard Henderson" <richard.henderson@linaro.org>,
qemu-devel@nongnu.org, "Roman Bolshakov" <r.bolshakov@yadro.com>,
"Alistair Francis" <alistair.francis@wdc.com>,
"Paolo Bonzini" <pbonzini@redhat.com>,
"Philippe Mathieu-Daudé" <philmd@redhat.com>
Subject: Re: [PATCH v15 17/23] accel: replace struct CpusAccel with AccelOpsClass
Date: Wed, 03 Feb 2021 14:43:27 +0000 [thread overview]
Message-ID: <877dnprtj0.fsf@linaro.org> (raw)
In-Reply-To: <20210201100903.17309-18-cfontana@suse.de>
Claudio Fontana <cfontana@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 <cfontana@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
next prev parent reply other threads:[~2021-02-03 16:47 UTC|newest]
Thread overview: 59+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-02-01 10:08 [PATCH v15 00/23] i386 cleanup PART 2 Claudio Fontana
2021-02-01 10:08 ` [PATCH v15 01/23] cpu: Introduce TCGCpuOperations struct Claudio Fontana
2021-02-02 13:46 ` Alex Bennée
2021-02-01 10:08 ` [PATCH v15 02/23] target/riscv: remove CONFIG_TCG, as it is always TCG Claudio Fontana
2021-02-01 10:08 ` [PATCH v15 03/23] accel/tcg: split TCG-only code from cpu_exec_realizefn Claudio Fontana
2021-02-01 10:08 ` [PATCH v15 04/23] cpu: Move synchronize_from_tb() to tcg_ops Claudio Fontana
2021-02-03 10:11 ` Alex Bennée
2021-02-03 12:31 ` Claudio Fontana
2021-02-04 11:18 ` Claudio Fontana
2021-02-04 11:44 ` Alex Bennée
2021-02-01 10:08 ` [PATCH v15 05/23] cpu: Move cpu_exec_* " Claudio Fontana
2021-02-01 10:08 ` [PATCH v15 06/23] cpu: Move tlb_fill " Claudio Fontana
2021-02-01 10:08 ` [PATCH v15 07/23] cpu: Move debug_excp_handler " Claudio Fontana
2021-02-01 10:08 ` [PATCH v15 08/23] target/arm: do not use cc->do_interrupt for KVM directly Claudio Fontana
2021-02-01 10:08 ` [PATCH v15 09/23] cpu: move cc->do_interrupt to tcg_ops Claudio Fontana
2021-02-01 10:08 ` [PATCH v15 10/23] cpu: move cc->transaction_failed " Claudio Fontana
2021-02-01 10:08 ` [PATCH v15 11/23] cpu: move do_unaligned_access " Claudio Fontana
2021-02-01 10:08 ` [PATCH v15 12/23] physmem: make watchpoint checking code TCG-only Claudio Fontana
2021-02-01 10:08 ` [PATCH v15 13/23] cpu: move adjust_watchpoint_address to tcg_ops Claudio Fontana
2021-02-03 10:15 ` Alex Bennée
2021-02-01 10:08 ` [PATCH v15 14/23] cpu: move debug_check_watchpoint " Claudio Fontana
2021-02-03 13:11 ` Alex Bennée
2021-02-01 10:08 ` [PATCH v15 15/23] cpu: tcg_ops: move to tcg-cpu-ops.h, keep a pointer in CPUClass Claudio Fontana
2021-02-03 13:23 ` Alex Bennée
2021-02-03 14:41 ` Claudio Fontana
2021-02-03 14:48 ` Philippe Mathieu-Daudé
2021-02-03 16:51 ` Claudio Fontana
2021-02-01 10:08 ` [PATCH v15 16/23] accel: extend AccelState and AccelClass to user-mode Claudio Fontana
2021-02-01 10:08 ` [PATCH v15 17/23] accel: replace struct CpusAccel with AccelOpsClass Claudio Fontana
2021-02-03 14:43 ` Alex Bennée [this message]
2021-02-01 10:08 ` [PATCH v15 18/23] accel: introduce AccelCPUClass extending CPUClass Claudio Fontana
2021-02-03 14:27 ` Philippe Mathieu-Daudé
2021-02-03 14:49 ` Claudio Fontana
2021-02-03 14:51 ` Philippe Mathieu-Daudé
2021-02-03 14:56 ` Claudio Fontana
2021-02-01 10:08 ` [PATCH v15 19/23] i386: split cpu accelerators from cpu.c, using AccelCPUClass Claudio Fontana
2021-02-03 16:47 ` Alex Bennée
2021-02-01 10:09 ` [PATCH v15 20/23] cpu: call AccelCPUClass::cpu_realizefn in cpu_exec_realizefn Claudio Fontana
2021-02-01 10:09 ` [PATCH v15 21/23] hw/core/cpu: call qemu_init_vcpu in cpu_common_realizefn Claudio Fontana
2021-02-03 16:51 ` Alex Bennée
2021-02-04 10:23 ` Claudio Fontana
2021-02-04 13:41 ` Philippe Mathieu-Daudé
2021-02-04 14:18 ` Claudio Fontana
2021-02-04 14:24 ` Peter Maydell
2021-02-04 14:57 ` Claudio Fontana
2021-02-01 10:09 ` [PATCH v15 22/23] accel: introduce new accessor functions Claudio Fontana
2021-02-03 14:23 ` Philippe Mathieu-Daudé
2021-02-03 14:24 ` Claudio Fontana
2021-02-01 10:09 ` [PATCH v15 23/23] accel-cpu: make cpu_realizefn return a bool Claudio Fontana
2021-02-03 13:34 ` Philippe Mathieu-Daudé
2021-02-03 16:56 ` Alex Bennée
2021-02-03 14:22 ` [PATCH v15 00/23] i386 cleanup PART 2 Alex Bennée
2021-02-03 14:43 ` Claudio Fontana
2021-02-03 16:15 ` Alex Bennée
2021-02-03 14:59 ` Eduardo Habkost
2021-02-03 16:57 ` Alex Bennée
2021-02-03 17:10 ` Claudio Fontana
2021-02-03 22:07 ` Alex Bennée
2021-02-04 9:46 ` Claudio Fontana
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=877dnprtj0.fsf@linaro.org \
--to=alex.bennee@linaro.org \
--cc=alistair.francis@wdc.com \
--cc=cfontana@suse.de \
--cc=ehabkost@redhat.com \
--cc=lvivier@redhat.com \
--cc=pbonzini@redhat.com \
--cc=peter.maydell@linaro.org \
--cc=philmd@redhat.com \
--cc=qemu-devel@nongnu.org \
--cc=r.bolshakov@yadro.com \
--cc=richard.henderson@linaro.org \
--cc=thuth@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).