Re: [PATCH v3 50/66] hw/core/cpu: Move cpu properties to cpu-sysemu.c

[Peter Maydell <peter.maydell@linaro.org>]

On Wed, 18 Aug 2021 at 21:09, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> The comment in cpu-common.c is absolutely correct, we can't
> rely on the ifdef in a file built once.  This was only "working"
> because we used ifndef.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Fixes: 1b36e4f5a5de585

which moved the properties out of cpu.c and into cpu-common.c
with the remark "There's no reason to keep the property list
separate from the CPU class code" despite there being a big
fat warning comment saying why it can't go in a compiled-once
source file !

Is there a reason to prefer this patch over just reverting
1b36e4f5a5de585 ?

thanks
-- PMM

(patch below for context)

> ---
>  hw/core/cpu-common.h | 17 +++++++++++++++++
>  hw/core/cpu-common.c | 18 ++----------------
>  hw/core/cpu-sysemu.c | 24 ++++++++++++++++++++++++
>  hw/core/cpu-user.c   | 22 ++++++++++++++++++++++
>  hw/core/meson.build  |  4 ++++
>  5 files changed, 69 insertions(+), 16 deletions(-)
>  create mode 100644 hw/core/cpu-common.h
>  create mode 100644 hw/core/cpu-user.c
>
> diff --git a/hw/core/cpu-common.h b/hw/core/cpu-common.h
> new file mode 100644
> index 0000000000..9049f74e67
> --- /dev/null
> +++ b/hw/core/cpu-common.h
> @@ -0,0 +1,17 @@
> +/*
> + * QEMU CPU model
> + *
> + * Copyright (c) 2014 SUSE LINUX Products GmbH
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + *
> + * 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 HW_CORE_CPU_COMMON_H
> +#define HW_CORE_CPU_COMMON_H
> +
> +void cpu_class_init_props(DeviceClass *dc);
> +
> +#endif
> diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c
> index e2f5a64604..6406ea79f0 100644
> --- a/hw/core/cpu-common.c
> +++ b/hw/core/cpu-common.c
> @@ -34,6 +34,7 @@
>  #include "hw/qdev-properties.h"
>  #include "trace/trace-root.h"
>  #include "qemu/plugin.h"
> +#include "cpu-common.h"
>
>  CPUState *cpu_by_arch_id(int64_t id)
>  {
> @@ -257,21 +258,6 @@ static int64_t cpu_common_get_arch_id(CPUState *cpu)
>      return cpu->cpu_index;
>  }
>
> -static Property cpu_common_props[] = {
> -#ifndef CONFIG_USER_ONLY
> -    /* Create a memory property for softmmu CPU object,
> -     * so users can wire up its memory. (This can't go in hw/core/cpu.c
> -     * because that file is compiled only once for both user-mode
> -     * and system builds.) The default if no link is set up is to use
> -     * the system address space.
> -     */
> -    DEFINE_PROP_LINK("memory", CPUState, memory, TYPE_MEMORY_REGION,
> -                     MemoryRegion *),
> -#endif
> -    DEFINE_PROP_BOOL("start-powered-off", CPUState, start_powered_off, false),
> -    DEFINE_PROP_END_OF_LIST(),
> -};
> -
>  static void cpu_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
> @@ -286,7 +272,7 @@ static void cpu_class_init(ObjectClass *klass, void *data)
>      dc->realize = cpu_common_realizefn;
>      dc->unrealize = cpu_common_unrealizefn;
>      dc->reset = cpu_common_reset;
> -    device_class_set_props(dc, cpu_common_props);
> +    cpu_class_init_props(dc);
>      /*
>       * Reason: CPUs still need special care by board code: wiring up
>       * IRQs, adding reset handlers, halting non-first CPUs, ...
> diff --git a/hw/core/cpu-sysemu.c b/hw/core/cpu-sysemu.c
> index 00253f8929..5a62960f3c 100644
> --- a/hw/core/cpu-sysemu.c
> +++ b/hw/core/cpu-sysemu.c
> @@ -20,8 +20,11 @@
>
>  #include "qemu/osdep.h"
>  #include "qapi/error.h"
> +#include "exec/memory.h"
>  #include "hw/core/cpu.h"
>  #include "hw/core/sysemu-cpu-ops.h"
> +#include "hw/qdev-properties.h"
> +#include "cpu-common.h"
>
>  bool cpu_paging_enabled(const CPUState *cpu)
>  {
> @@ -143,3 +146,24 @@ GuestPanicInformation *cpu_get_crash_info(CPUState *cpu)
>      }
>      return res;
>  }
> +
> +/*
> + * This can't go in hw/core/cpu-common.c because that file is compiled only
> + * once for both user-mode and system builds.
> + */
> +static Property cpu_sysemu_props[] = {
> +    /*
> +     * Create a memory property for softmmu CPU object, so users can wire
> +     * up its memory. The default if no link is set up is to use the
> +     * system address space.
> +     */
> +    DEFINE_PROP_LINK("memory", CPUState, memory, TYPE_MEMORY_REGION,
> +                     MemoryRegion *),
> +    DEFINE_PROP_BOOL("start-powered-off", CPUState, start_powered_off, false),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +void cpu_class_init_props(DeviceClass *dc)
> +{
> +    device_class_set_props(dc, cpu_sysemu_props);
> +}
> diff --git a/hw/core/cpu-user.c b/hw/core/cpu-user.c
> new file mode 100644
> index 0000000000..62037e8669
> --- /dev/null
> +++ b/hw/core/cpu-user.c
> @@ -0,0 +1,22 @@
> +/*
> + * QEMU CPU model (user-only emulation specific)
> + *
> + * Copyright (c) 2021 Linaro, Ltd.
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "hw/core/cpu.h"
> +#include "cpu-common.h"
> +
> +/*
> + * This can't go in hw/core/cpu-common.c because that file is compiled only
> + * once for both user-mode and system builds.
> + */
> +void cpu_class_init_props(DeviceClass *dc)
> +{
> +}
> diff --git a/hw/core/meson.build b/hw/core/meson.build
> index 18f44fb7c2..4b3de4e247 100644
> --- a/hw/core/meson.build
> +++ b/hw/core/meson.build
> @@ -43,3 +43,7 @@ specific_ss.add(when: 'CONFIG_SOFTMMU', if_true: files(
>    'machine-qmp-cmds.c',
>    'numa.c',
>  ))
> +
> +specific_ss.add(when: 'CONFIG_USER_ONLY', if_true: files(
> +  'cpu-user.c'
> +))
> --