Re: [PATCH v15 17/23] accel: replace struct CpusAccel with AccelOpsClass

[Alex Bennée]

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