qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
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


  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).