diff mbox series

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

Message ID 20210818191920.390759-51-richard.henderson@linaro.org
State New
Headers show
Series Unaligned access for user-only | expand

Commit Message

Richard Henderson Aug. 18, 2021, 7:19 p.m. UTC 
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>

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

-- 
2.25.1

Comments

Peter Maydell Aug. 19, 2021, 3:26 p.m. UTC | #1 
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'

> +))

> --
Eduardo Habkost Aug. 19, 2021, 4:52 p.m. UTC | #2 
On Thu, Aug 19, 2021 at 04:26:10PM +0100, Peter Maydell wrote:
> 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 !


Ouch.  Sorry about that.  :(

> 

> Is there a reason to prefer this patch over just reverting

> 1b36e4f5a5de585 ?


I agree with reverting the commit.

-- 
Eduardo
diff mbox series

Patch

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'
+))