All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v11 0/7] i386 cleanup PART 2
@ 2020-12-11 10:09 Claudio Fontana
  2020-12-11 10:09 ` [PATCH v11 1/7] accel: extend AccelState and AccelClass to user-mode Claudio Fontana
                   ` (6 more replies)
  0 siblings, 7 replies; 19+ messages in thread
From: Claudio Fontana @ 2020-12-11 10:09 UTC (permalink / raw)
  To: Paolo Bonzini, Thomas Huth, Richard Henderson,
	Stefano Stabellini, Wenchao Wang, Roman Bolshakov,
	Sunil Muthuswamy, Philippe Mathieu-Daudé
  Cc: Laurent Vivier, Peter Maydell, Eduardo Habkost, Paul Durrant,
	Jason Wang, Marcelo Tosatti, qemu-devel, Peter Xu,
	Dario Faggioli, Cameron Esfahani, haxm-team, Claudio Fontana,
	Anthony Perard, Bruce Rogers, Olaf Hering, Emilio G . Cota,
	Colin Xu

Hello, this is version 11 of the cleanup, PART 2.

The series has been split into two separate parts;
to find PART 1, see the series "i386 cleanup PART 1"

v10 -> v11: split off PART 2,

no further changes to PART 2 other than the split.

v9 -> v10: minor tweaks and fixes

* in "i386: split cpu accelerators from cpu.c",

use kvm/kvm-cpu.c, hvf/hvf-cpu.c, tcg/tcg-cpu.c.
Easier to understand compared to editing multiple cpu.c files,
and matches the header files if needed (kvm-cpu.h).

* in "accel: replace struct CpusAccel with AccelOpsClass",

make it a bit more consistent, by naming the files defining
the AccelOpsClass types "...-accel-ops.c" instead of the old
naming "...-cpus.c".

* in "cpu: move cc->transaction_failed to tcg_ops",

protect with CONFIG_TCG the use of tcg_ops for hw/misc/jazz.c,

 #include "exec/memattrs.h" (Philippe, Eduardo)

* in "cpu: Move synchronize_from_tb() to tcg_ops",

 #include "hw/core/cpu.h" (Philippe, Eduardo)

do not remove the comment about struct TcgCpuOperations (Philippe)

* in "accel/tcg: split TCG-only code from cpu_exec_realizefn",

invert tcg_target_initialized set order (Alex)

* in "i386: move TCG cpu class initialization out of helper.c",

extract helper-tcg.h, tcg-cpu.c, and tcg-cpu.h directly into
tcg/, avoiding the extra move later to tcg/ (Alex)



v8 -> v9: move additional methods to CPUClass->tcg_ops

do_unaligned_access, transaction_failed and do_interrupt.

do_interrupt is a bit tricky, as the same code is reused
(albeit not usually directly) for KVM under certain odd conditions.

Change arm, as the only user of do_interrupt callback for KVM,
to instead call the target function directly arm_do_interrupt.

v7 -> v8: add missing CONFIG_TCGs, fix bugs

* add the prerequisite patches for "3 tcg" at the beginning of the
  series for convenience (already reviewed, queued by RH).

* add CONFIG_TCG to TCGCpuOperations and tcg_ops variable use

* reduce the scope of the realizefn refactoring, do not
  introduce a separate cpu_accel_realize, and instead use the
  existing cpu_exec_realizefn, there is not enough benefit
  to introduce a new function.

* fix bugs in user mode due to attempt to move the tcg_region_init()
  early, so it could be done just once in tcg_init() for both
  softmmu and user mode. Unfortunately it needs to remain deferred
  for user mode, as it needs to be done after prologue init and
  after the GUEST_BASE has been set.

v6 -> v7: integrate TCGCpuOperations, refactored cpu_exec_realizefn

* integrate TCGCpuOperations (Eduardo)

Taken some refactoring from Eduardo for Tcg-only operations on
CPUClass.

* refactored cpu_exec_realizefn

The other main change is a refactoring of cpu_exec_realizefn,
directly linked to the effort of making many cpu_exec operations
TCG-only (Eduardo series above):

cpu_exec_realizefn is actually a TCG-only thing, with the
exception of a couple things that can be done in base cpu code.

This changes all targets realizefn, so I guess I have to Cc:
the Multiverse? (Universe was already CCed for all accelerators).


v5 -> v6: remove MODULE_INIT_ACCEL_CPU


instead, use a call to accel_init_interfaces().

* The class lookups are now general and performed in accel/

  new AccelCPUClass for new archs are supported as new
  ones appear in the class hierarchy, no need for stubs.

* Split the code a bit better


v4 -> v5: centralized and simplified initializations

I put in Cc: Emilio G. Cota, specifically because in patch 8
I (re)moved for user-mode the call to tcg_regions_init().

The call happens now inside the tcg AccelClass machine_init,
(so earlier). This seems to work fine, but thought to get the
author opinion on this.

Rebased on "tcg-cpus: split into 3 tcg variants" series
(queued by Richard), to avoid some code churn:


https://lists.gnu.org/archive/html/qemu-devel/2020-10/msg04356.html


* Extended AccelClass to user-mode.

user-mode now does not call tcg_exec_init directly,
instead it uses the tcg accel class, and its init_machine method.

Since user-mode does not define or use a machine state,
the machine is just passed as NULL.

The immediate advantage is that now we can call current_accel()
from both user mode and softmmu, so we can work out the correct
class to use for accelerator initializations.

* QOMification of CpusAccelOps

simple QOMification of CpusAccelOps abstract class.

* Centralized all accel_cpu_init, so only one per cpu-arch,
  plus one for all accels will remain.

  So we can expect accel_cpu_init() to be limited to:
  
  softmmu/cpus.c - initializes the chosen softmmu accel ops for the cpus module.
  target/ARCH/cpu.c - initializes the chosen arch-specific cpu accelerator.
  
These changes are meant to address concerns/issues (Paolo):

1) the use of if (tcg_enabled()) and similar in the module_init call path

2) the excessive number of accel_cpu_init() to hunt down in the codebase.


* Fixed wrong use of host_cpu_class_init (Eduardo)


v3 -> v4: QOMification of X86CPUAccelClass


In this version I basically QOMified X86CPUAccel, taking the
suggestions from Eduardo as the starting point,
but stopping just short of making it an actual QOM interface,
using a plain abstract class, and then subclasses for the
actual objects.

Initialization is still using the existing qemu initialization
framework (module_call_init), which is I still think is better
than the alternatives proposed, in the current state.

Possibly some improvements could be developed in the future here.
In this case, effort should be put in keeping things extendible,
in order not to be blocked once accelerators also become modules.

Motivation and higher level steps:

https://lists.gnu.org/archive/html/qemu-devel/2020-05/msg04628.html

Looking forward to your comments on this proposal,

Ciao,

Claudio


Claudio Fontana (7):
  accel: extend AccelState and AccelClass to user-mode
  accel: replace struct CpusAccel with AccelOpsClass
  accel: introduce AccelCPUClass extending CPUClass
  i386: split cpu accelerators from cpu.c, using AccelCPUClass
  cpu: call AccelCPUClass::cpu_realizefn in cpu_exec_realizefn
  hw/core/cpu: call qemu_init_vcpu in cpu_common_realizefn
  cpu: introduce cpu_accel_instance_init

 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/hw/boards.h                           |   2 +-
 include/hw/core/accel-cpu.h                   |  25 ++
 include/hw/core/cpu.h                         |  19 +
 include/{sysemu => qemu}/accel.h              |  16 +-
 include/sysemu/accel-ops.h                    |  45 ++
 include/sysemu/cpus.h                         |  26 +-
 include/sysemu/hvf.h                          |   2 +-
 include/sysemu/kvm.h                          |   2 +-
 include/sysemu/kvm_int.h                      |   2 +-
 target/i386/cpu.h                             |  20 +-
 .../i386/hax/{hax-cpus.h => hax-accel-ops.h}  |   2 -
 target/i386/hax/hax-windows.h                 |   2 +-
 target/i386/host-cpu.h                        |  19 +
 .../i386/hvf/{hvf-cpus.h => hvf-accel-ops.h}  |   2 -
 target/i386/hvf/hvf-i386.h                    |   2 +-
 target/i386/kvm/kvm-cpu.h                     |  41 ++
 target/i386/tcg/tcg-cpu.h                     |  15 -
 .../whpx/{whpx-cpus.h => whpx-accel-ops.h}    |   2 -
 accel/accel-common.c                          | 105 +++++
 accel/{accel.c => accel-softmmu.c}            |  60 ++-
 accel/accel-user.c                            |  24 ++
 accel/kvm/{kvm-cpus.c => kvm-accel-ops.c}     |  26 +-
 accel/kvm/kvm-all.c                           |   2 -
 accel/qtest/qtest.c                           |  25 +-
 ...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                           |  17 +-
 accel/xen/xen-all.c                           |  24 +-
 bsd-user/main.c                               |  11 +-
 cpu.c                                         |   5 +
 hw/core/cpu.c                                 |  11 +
 hw/i386/pc_piix.c                             |   1 +
 linux-user/main.c                             |   7 +-
 softmmu/cpus.c                                |  12 +-
 softmmu/memory.c                              |   2 +-
 softmmu/qtest.c                               |   2 +-
 softmmu/vl.c                                  |   8 +-
 target/alpha/cpu.c                            |   5 +-
 target/arm/cpu.c                              |   4 +-
 target/avr/cpu.c                              |   3 +-
 target/cris/cpu.c                             |   2 -
 target/hppa/cpu.c                             |   1 -
 target/i386/cpu.c                             | 393 ++----------------
 .../i386/hax/{hax-cpus.c => hax-accel-ops.c}  |  31 +-
 target/i386/hax/hax-all.c                     |   7 +-
 target/i386/hax/hax-mem.c                     |   2 +-
 target/i386/hax/hax-posix.c                   |   2 +-
 target/i386/hax/hax-windows.c                 |   2 +-
 target/i386/host-cpu.c                        | 198 +++++++++
 .../i386/hvf/{hvf-cpus.c => hvf-accel-ops.c}  |  29 +-
 target/i386/hvf/hvf-cpu.c                     |  65 +++
 target/i386/hvf/hvf.c                         |   5 +-
 target/i386/hvf/x86_task.c                    |   2 +-
 target/i386/hvf/x86hvf.c                      |   2 +-
 target/i386/kvm/kvm-cpu.c                     | 148 +++++++
 target/i386/kvm/kvm.c                         |   3 +-
 target/i386/tcg/tcg-cpu.c                     | 116 +++++-
 .../whpx/{whpx-cpus.c => whpx-accel-ops.c}    |  31 +-
 target/i386/whpx/whpx-all.c                   |   6 +-
 target/lm32/cpu.c                             |   3 -
 target/m68k/cpu.c                             |   2 -
 target/microblaze/cpu.c                       |   9 +-
 target/mips/cpu.c                             |   2 -
 target/moxie/cpu.c                            |   4 +-
 target/nios2/cpu.c                            |   4 +-
 target/openrisc/cpu.c                         |   4 +-
 target/riscv/cpu.c                            |   8 +-
 target/rx/cpu.c                               |   8 +-
 target/s390x/cpu.c                            |   3 +-
 target/sh4/cpu.c                              |   2 -
 target/sparc/cpu.c                            |   4 +-
 target/tilegx/cpu.c                           |   2 -
 target/tricore/cpu.c                          |   2 -
 target/unicore32/cpu.c                        |   6 +-
 target/xtensa/cpu.c                           |   2 -
 MAINTAINERS                                   |   8 +-
 accel/kvm/meson.build                         |   2 +-
 accel/meson.build                             |   4 +-
 accel/tcg/meson.build                         |  10 +-
 target/i386/hax/meson.build                   |   2 +-
 target/i386/hvf/meson.build                   |   3 +-
 target/i386/kvm/meson.build                   |   7 +-
 target/i386/meson.build                       |   6 +-
 target/i386/whpx/meson.build                  |   2 +-
 target/ppc/translate_init.c.inc               |   5 +-
 93 files changed, 1230 insertions(+), 666 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/hw/core/accel-cpu.h
 rename include/{sysemu => qemu}/accel.h (94%)
 create mode 100644 include/sysemu/accel-ops.h
 rename target/i386/hax/{hax-cpus.h => hax-accel-ops.h} (95%)
 create mode 100644 target/i386/host-cpu.h
 rename target/i386/hvf/{hvf-cpus.h => hvf-accel-ops.h} (94%)
 create mode 100644 target/i386/kvm/kvm-cpu.h
 delete mode 100644 target/i386/tcg/tcg-cpu.h
 rename target/i386/whpx/{whpx-cpus.h => whpx-accel-ops.h} (96%)
 create mode 100644 accel/accel-common.c
 rename accel/{accel.c => accel-softmmu.c} (64%)
 create mode 100644 accel/accel-user.c
 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%)
 create mode 100644 target/i386/host-cpu.c
 rename target/i386/hvf/{hvf-cpus.c => hvf-accel-ops.c} (84%)
 create mode 100644 target/i386/hvf/hvf-cpu.c
 create mode 100644 target/i386/kvm/kvm-cpu.c
 rename target/i386/whpx/{whpx-cpus.c => whpx-accel-ops.c} (72%)

-- 
2.26.2



^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v11 1/7] accel: extend AccelState and AccelClass to user-mode
  2020-12-11 10:09 [PATCH v11 0/7] i386 cleanup PART 2 Claudio Fontana
@ 2020-12-11 10:09 ` Claudio Fontana
  2020-12-11 10:09 ` [PATCH v11 2/7] accel: replace struct CpusAccel with AccelOpsClass Claudio Fontana
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 19+ messages in thread
From: Claudio Fontana @ 2020-12-11 10:09 UTC (permalink / raw)
  To: Paolo Bonzini, Thomas Huth, Richard Henderson,
	Stefano Stabellini, Wenchao Wang, Roman Bolshakov,
	Sunil Muthuswamy, Philippe Mathieu-Daudé
  Cc: Laurent Vivier, Peter Maydell, Eduardo Habkost, Paul Durrant,
	Alex Bennée, Jason Wang, Marcelo Tosatti, qemu-devel,
	Peter Xu, Dario Faggioli, Cameron Esfahani, haxm-team,
	Claudio Fontana, Anthony Perard, Bruce Rogers, Olaf Hering,
	Emilio G . Cota, Colin Xu

Signed-off-by: Claudio Fontana <cfontana@suse.de>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
---
 include/hw/boards.h                |  2 +-
 include/{sysemu => qemu}/accel.h   | 14 +++++----
 include/sysemu/hvf.h               |  2 +-
 include/sysemu/kvm.h               |  2 +-
 include/sysemu/kvm_int.h           |  2 +-
 target/i386/hvf/hvf-i386.h         |  2 +-
 accel/accel-common.c               | 50 ++++++++++++++++++++++++++++++
 accel/{accel.c => accel-softmmu.c} | 27 ++--------------
 accel/accel-user.c                 | 24 ++++++++++++++
 accel/qtest/qtest.c                |  2 +-
 accel/tcg/tcg-all.c                | 13 ++++++--
 accel/xen/xen-all.c                |  2 +-
 bsd-user/main.c                    |  6 +++-
 linux-user/main.c                  |  6 +++-
 softmmu/memory.c                   |  2 +-
 softmmu/qtest.c                    |  2 +-
 softmmu/vl.c                       |  2 +-
 target/i386/hax/hax-all.c          |  2 +-
 target/i386/hvf/hvf.c              |  2 +-
 target/i386/hvf/x86_task.c         |  2 +-
 target/i386/whpx/whpx-all.c        |  2 +-
 MAINTAINERS                        |  2 +-
 accel/meson.build                  |  4 ++-
 accel/tcg/meson.build              |  2 +-
 24 files changed, 124 insertions(+), 52 deletions(-)
 rename include/{sysemu => qemu}/accel.h (95%)
 create mode 100644 accel/accel-common.c
 rename accel/{accel.c => accel-softmmu.c} (75%)
 create mode 100644 accel/accel-user.c

diff --git a/include/hw/boards.h b/include/hw/boards.h
index f94f4ad5d8..f8ae50c49c 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -6,7 +6,7 @@
 #include "exec/memory.h"
 #include "sysemu/hostmem.h"
 #include "sysemu/blockdev.h"
-#include "sysemu/accel.h"
+#include "qemu/accel.h"
 #include "qapi/qapi-types-machine.h"
 #include "qemu/module.h"
 #include "qom/object.h"
diff --git a/include/sysemu/accel.h b/include/qemu/accel.h
similarity index 95%
rename from include/sysemu/accel.h
rename to include/qemu/accel.h
index e08b8ab8fa..fac4a18703 100644
--- a/include/sysemu/accel.h
+++ b/include/qemu/accel.h
@@ -20,8 +20,8 @@
  * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
  * THE SOFTWARE.
  */
-#ifndef HW_ACCEL_H
-#define HW_ACCEL_H
+#ifndef QEMU_ACCEL_H
+#define QEMU_ACCEL_H
 
 #include "qom/object.h"
 #include "exec/hwaddr.h"
@@ -37,8 +37,8 @@ typedef struct AccelClass {
     /*< public >*/
 
     const char *name;
-#ifndef CONFIG_USER_ONLY
     int (*init_machine)(MachineState *ms);
+#ifndef CONFIG_USER_ONLY
     void (*setup_post)(MachineState *ms, AccelState *accel);
     bool (*has_memory)(MachineState *ms, AddressSpace *as,
                        hwaddr start_addr, hwaddr size);
@@ -67,11 +67,13 @@ typedef struct AccelClass {
     OBJECT_GET_CLASS(AccelClass, (obj), TYPE_ACCEL)
 
 AccelClass *accel_find(const char *opt_name);
+AccelState *current_accel(void);
+
+#ifndef CONFIG_USER_ONLY
 int accel_init_machine(AccelState *accel, MachineState *ms);
 
 /* Called just before os_setup_post (ie just before drop OS privs) */
 void accel_setup_post(MachineState *ms);
+#endif /* !CONFIG_USER_ONLY */
 
-AccelState *current_accel(void);
-
-#endif
+#endif /* QEMU_ACCEL_H */
diff --git a/include/sysemu/hvf.h b/include/sysemu/hvf.h
index f893768df9..c98636bc81 100644
--- a/include/sysemu/hvf.h
+++ b/include/sysemu/hvf.h
@@ -13,7 +13,7 @@
 #ifndef HVF_H
 #define HVF_H
 
-#include "sysemu/accel.h"
+#include "qemu/accel.h"
 #include "qom/object.h"
 
 #ifdef CONFIG_HVF
diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
index bb5d5cf497..739682f3c3 100644
--- a/include/sysemu/kvm.h
+++ b/include/sysemu/kvm.h
@@ -17,7 +17,7 @@
 #include "qemu/queue.h"
 #include "hw/core/cpu.h"
 #include "exec/memattrs.h"
-#include "sysemu/accel.h"
+#include "qemu/accel.h"
 #include "qom/object.h"
 
 #ifdef NEED_CPU_H
diff --git a/include/sysemu/kvm_int.h b/include/sysemu/kvm_int.h
index 65740806da..ccb8869f01 100644
--- a/include/sysemu/kvm_int.h
+++ b/include/sysemu/kvm_int.h
@@ -10,7 +10,7 @@
 #define QEMU_KVM_INT_H
 
 #include "exec/memory.h"
-#include "sysemu/accel.h"
+#include "qemu/accel.h"
 #include "sysemu/kvm.h"
 
 typedef struct KVMSlot
diff --git a/target/i386/hvf/hvf-i386.h b/target/i386/hvf/hvf-i386.h
index e0edffd077..50b914fd67 100644
--- a/target/i386/hvf/hvf-i386.h
+++ b/target/i386/hvf/hvf-i386.h
@@ -16,7 +16,7 @@
 #ifndef HVF_I386_H
 #define HVF_I386_H
 
-#include "sysemu/accel.h"
+#include "qemu/accel.h"
 #include "sysemu/hvf.h"
 #include "cpu.h"
 #include "x86.h"
diff --git a/accel/accel-common.c b/accel/accel-common.c
new file mode 100644
index 0000000000..ddec8cb5ae
--- /dev/null
+++ b/accel/accel-common.c
@@ -0,0 +1,50 @@
+/*
+ * QEMU accel class, components common to system emulation and user mode
+ *
+ * Copyright (c) 2003-2008 Fabrice Bellard
+ * Copyright (c) 2014 Red Hat Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/accel.h"
+
+static const TypeInfo accel_type = {
+    .name = TYPE_ACCEL,
+    .parent = TYPE_OBJECT,
+    .class_size = sizeof(AccelClass),
+    .instance_size = sizeof(AccelState),
+};
+
+/* Lookup AccelClass from opt_name. Returns NULL if not found */
+AccelClass *accel_find(const char *opt_name)
+{
+    char *class_name = g_strdup_printf(ACCEL_CLASS_NAME("%s"), opt_name);
+    AccelClass *ac = ACCEL_CLASS(object_class_by_name(class_name));
+    g_free(class_name);
+    return ac;
+}
+
+static void register_accel_types(void)
+{
+    type_register_static(&accel_type);
+}
+
+type_init(register_accel_types);
diff --git a/accel/accel.c b/accel/accel-softmmu.c
similarity index 75%
rename from accel/accel.c
rename to accel/accel-softmmu.c
index cb555e3b06..f89da8f9d1 100644
--- a/accel/accel.c
+++ b/accel/accel-softmmu.c
@@ -1,5 +1,5 @@
 /*
- * QEMU System Emulator, accelerator interfaces
+ * QEMU accel class, system emulation components
  *
  * Copyright (c) 2003-2008 Fabrice Bellard
  * Copyright (c) 2014 Red Hat Inc.
@@ -24,28 +24,12 @@
  */
 
 #include "qemu/osdep.h"
-#include "sysemu/accel.h"
+#include "qemu/accel.h"
 #include "hw/boards.h"
 #include "sysemu/arch_init.h"
 #include "sysemu/sysemu.h"
 #include "qom/object.h"
 
-static const TypeInfo accel_type = {
-    .name = TYPE_ACCEL,
-    .parent = TYPE_OBJECT,
-    .class_size = sizeof(AccelClass),
-    .instance_size = sizeof(AccelState),
-};
-
-/* Lookup AccelClass from opt_name. Returns NULL if not found */
-AccelClass *accel_find(const char *opt_name)
-{
-    char *class_name = g_strdup_printf(ACCEL_CLASS_NAME("%s"), opt_name);
-    AccelClass *ac = ACCEL_CLASS(object_class_by_name(class_name));
-    g_free(class_name);
-    return ac;
-}
-
 int accel_init_machine(AccelState *accel, MachineState *ms)
 {
     AccelClass *acc = ACCEL_GET_CLASS(accel);
@@ -76,10 +60,3 @@ void accel_setup_post(MachineState *ms)
         acc->setup_post(ms, accel);
     }
 }
-
-static void register_accel_types(void)
-{
-    type_register_static(&accel_type);
-}
-
-type_init(register_accel_types);
diff --git a/accel/accel-user.c b/accel/accel-user.c
new file mode 100644
index 0000000000..26bdda6236
--- /dev/null
+++ b/accel/accel-user.c
@@ -0,0 +1,24 @@
+/*
+ * QEMU accel class, user-mode components
+ *
+ * Copyright 2020 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.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/accel.h"
+
+AccelState *current_accel(void)
+{
+    static AccelState *accel;
+
+    if (!accel) {
+        AccelClass *ac = accel_find("tcg");
+
+        g_assert(ac != NULL);
+        accel = ACCEL(object_new_with_class(OBJECT_CLASS(ac)));
+    }
+    return accel;
+}
diff --git a/accel/qtest/qtest.c b/accel/qtest/qtest.c
index b282cea5cf..b4e731cb2b 100644
--- a/accel/qtest/qtest.c
+++ b/accel/qtest/qtest.c
@@ -17,7 +17,7 @@
 #include "qemu/module.h"
 #include "qemu/option.h"
 #include "qemu/config-file.h"
-#include "sysemu/accel.h"
+#include "qemu/accel.h"
 #include "sysemu/qtest.h"
 #include "sysemu/cpus.h"
 #include "sysemu/cpu-timers.h"
diff --git a/accel/tcg/tcg-all.c b/accel/tcg/tcg-all.c
index 1ac0b76515..7125d0cc29 100644
--- a/accel/tcg/tcg-all.c
+++ b/accel/tcg/tcg-all.c
@@ -30,9 +30,12 @@
 #include "tcg/tcg.h"
 #include "qapi/error.h"
 #include "qemu/error-report.h"
-#include "hw/boards.h"
+#include "qemu/accel.h"
 #include "qapi/qapi-builtin-visit.h"
+
+#ifndef CONFIG_USER_ONLY
 #include "tcg-cpus.h"
+#endif /* CONFIG_USER_ONLY */
 
 struct TCGState {
     AccelState parent_obj;
@@ -106,8 +109,12 @@ static int tcg_init(MachineState *ms)
     mttcg_enabled = s->mttcg_enabled;
 
     /*
-     * Initialize TCG regions
+     * Initialize TCG regions only for softmmu.
+     *
+     * This needs to be done later for user mode, because the prologue
+     * generation needs to be delayed so that GUEST_BASE is already set.
      */
+#ifndef CONFIG_USER_ONLY
     tcg_region_init();
 
     if (mttcg_enabled) {
@@ -117,6 +124,8 @@ static int tcg_init(MachineState *ms)
     } else {
         cpus_register_accel(&tcg_cpus_rr);
     }
+#endif /* !CONFIG_USER_ONLY */
+
     return 0;
 }
 
diff --git a/accel/xen/xen-all.c b/accel/xen/xen-all.c
index 878a4089d9..594aaf6b49 100644
--- a/accel/xen/xen-all.c
+++ b/accel/xen/xen-all.c
@@ -15,7 +15,7 @@
 #include "hw/xen/xen-legacy-backend.h"
 #include "hw/xen/xen_pt.h"
 #include "chardev/char.h"
-#include "sysemu/accel.h"
+#include "qemu/accel.h"
 #include "sysemu/cpus.h"
 #include "sysemu/xen.h"
 #include "sysemu/runstate.h"
diff --git a/bsd-user/main.c b/bsd-user/main.c
index 0a918e8f74..ff295bcb29 100644
--- a/bsd-user/main.c
+++ b/bsd-user/main.c
@@ -20,6 +20,7 @@
 #include "qemu/osdep.h"
 #include "qemu-common.h"
 #include "qemu/units.h"
+#include "qemu/accel.h"
 #include "sysemu/tcg.h"
 #include "qemu-version.h"
 #include <machine/trap.h>
@@ -908,8 +909,11 @@ int main(int argc, char **argv)
     }
 
     /* init tcg before creating CPUs and to get qemu_host_page_size */
-    tcg_exec_init(0);
+    {
+        AccelClass *ac = ACCEL_GET_CLASS(current_accel());
 
+        ac->init_machine(NULL);
+    }
     cpu_type = parse_cpu_option(cpu_model);
     cpu = cpu_create(cpu_type);
     env = cpu->env_ptr;
diff --git a/linux-user/main.c b/linux-user/main.c
index 24d1eb73ad..5c059a8445 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -20,6 +20,7 @@
 #include "qemu/osdep.h"
 #include "qemu-common.h"
 #include "qemu/units.h"
+#include "qemu/accel.h"
 #include "sysemu/tcg.h"
 #include "qemu-version.h"
 #include <sys/syscall.h>
@@ -703,8 +704,11 @@ int main(int argc, char **argv, char **envp)
     cpu_type = parse_cpu_option(cpu_model);
 
     /* init tcg before creating CPUs and to get qemu_host_page_size */
-    tcg_exec_init(0);
+    {
+        AccelClass *ac = ACCEL_GET_CLASS(current_accel());
 
+        ac->init_machine(NULL);
+    }
     cpu = cpu_create(cpu_type);
     env = cpu->env_ptr;
     cpu_reset(cpu);
diff --git a/softmmu/memory.c b/softmmu/memory.c
index 22bacbbc78..585ec6f8dc 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -32,7 +32,7 @@
 #include "sysemu/kvm.h"
 #include "sysemu/runstate.h"
 #include "sysemu/tcg.h"
-#include "sysemu/accel.h"
+#include "qemu/accel.h"
 #include "hw/boards.h"
 #include "migration/vmstate.h"
 
diff --git a/softmmu/qtest.c b/softmmu/qtest.c
index 7965dc9a16..130c366615 100644
--- a/softmmu/qtest.c
+++ b/softmmu/qtest.c
@@ -20,7 +20,7 @@
 #include "exec/ioport.h"
 #include "exec/memory.h"
 #include "hw/irq.h"
-#include "sysemu/accel.h"
+#include "qemu/accel.h"
 #include "sysemu/cpu-timers.h"
 #include "qemu/config-file.h"
 #include "qemu/option.h"
diff --git a/softmmu/vl.c b/softmmu/vl.c
index e6e0ad5a92..bc20c526d2 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -40,7 +40,7 @@
 
 #include "qemu/error-report.h"
 #include "qemu/sockets.h"
-#include "sysemu/accel.h"
+#include "qemu/accel.h"
 #include "hw/usb.h"
 #include "hw/isa/isa.h"
 #include "hw/scsi/scsi.h"
diff --git a/target/i386/hax/hax-all.c b/target/i386/hax/hax-all.c
index fecfe8cd6e..d7f4bb44a7 100644
--- a/target/i386/hax/hax-all.c
+++ b/target/i386/hax/hax-all.c
@@ -28,7 +28,7 @@
 #include "exec/address-spaces.h"
 
 #include "qemu-common.h"
-#include "sysemu/accel.h"
+#include "qemu/accel.h"
 #include "sysemu/reset.h"
 #include "sysemu/runstate.h"
 #include "hw/boards.h"
diff --git a/target/i386/hvf/hvf.c b/target/i386/hvf/hvf.c
index ed9356565c..ffc9efa40f 100644
--- a/target/i386/hvf/hvf.c
+++ b/target/i386/hvf/hvf.c
@@ -69,7 +69,7 @@
 #include "exec/address-spaces.h"
 #include "hw/i386/apic_internal.h"
 #include "qemu/main-loop.h"
-#include "sysemu/accel.h"
+#include "qemu/accel.h"
 #include "target/i386/cpu.h"
 
 #include "hvf-cpus.h"
diff --git a/target/i386/hvf/x86_task.c b/target/i386/hvf/x86_task.c
index 6f04478b3a..d66dfd7669 100644
--- a/target/i386/hvf/x86_task.c
+++ b/target/i386/hvf/x86_task.c
@@ -28,7 +28,7 @@
 
 #include "hw/i386/apic_internal.h"
 #include "qemu/main-loop.h"
-#include "sysemu/accel.h"
+#include "qemu/accel.h"
 #include "target/i386/cpu.h"
 
 // TODO: taskswitch handling
diff --git a/target/i386/whpx/whpx-all.c b/target/i386/whpx/whpx-all.c
index f4f3e33eac..ee6b606194 100644
--- a/target/i386/whpx/whpx-all.c
+++ b/target/i386/whpx/whpx-all.c
@@ -13,7 +13,7 @@
 #include "exec/address-spaces.h"
 #include "exec/ioport.h"
 #include "qemu-common.h"
-#include "sysemu/accel.h"
+#include "qemu/accel.h"
 #include "sysemu/whpx.h"
 #include "sysemu/cpus.h"
 #include "sysemu/runstate.h"
diff --git a/MAINTAINERS b/MAINTAINERS
index d876f504a6..6235dd3a9f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -434,7 +434,7 @@ Overall
 M: Richard Henderson <richard.henderson@linaro.org>
 R: Paolo Bonzini <pbonzini@redhat.com>
 S: Maintained
-F: include/sysemu/accel.h
+F: include/qemu/accel.h
 F: accel/accel.c
 F: accel/Makefile.objs
 F: accel/stubs/Makefile.objs
diff --git a/accel/meson.build b/accel/meson.build
index b26cca227a..b44ba30c86 100644
--- a/accel/meson.build
+++ b/accel/meson.build
@@ -1,4 +1,6 @@
-softmmu_ss.add(files('accel.c'))
+specific_ss.add(files('accel-common.c'))
+softmmu_ss.add(files('accel-softmmu.c'))
+user_ss.add(files('accel-user.c'))
 
 subdir('qtest')
 subdir('kvm')
diff --git a/accel/tcg/meson.build b/accel/tcg/meson.build
index f39aab0a0c..424d9bb1fc 100644
--- a/accel/tcg/meson.build
+++ b/accel/tcg/meson.build
@@ -1,5 +1,6 @@
 tcg_ss = ss.source_set()
 tcg_ss.add(files(
+  'tcg-all.c',
   'cpu-exec-common.c',
   'cpu-exec.c',
   'tcg-runtime-gvec.c',
@@ -13,7 +14,6 @@ tcg_ss.add(when: 'CONFIG_PLUGIN', if_true: [files('plugin-gen.c'), libdl])
 specific_ss.add_all(when: 'CONFIG_TCG', if_true: tcg_ss)
 
 specific_ss.add(when: ['CONFIG_SOFTMMU', 'CONFIG_TCG'], if_true: files(
-  'tcg-all.c',
   'cputlb.c',
   'tcg-cpus.c',
   'tcg-cpus-mttcg.c',
-- 
2.26.2



^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v11 2/7] accel: replace struct CpusAccel with AccelOpsClass
  2020-12-11 10:09 [PATCH v11 0/7] i386 cleanup PART 2 Claudio Fontana
  2020-12-11 10:09 ` [PATCH v11 1/7] accel: extend AccelState and AccelClass to user-mode Claudio Fontana
@ 2020-12-11 10:09 ` Claudio Fontana
  2020-12-11 10:09 ` [PATCH v11 3/7] accel: introduce AccelCPUClass extending CPUClass Claudio Fontana
                   ` (4 subsequent siblings)
  6 siblings, 0 replies; 19+ messages in thread
From: Claudio Fontana @ 2020-12-11 10:09 UTC (permalink / raw)
  To: Paolo Bonzini, Thomas Huth, Richard Henderson,
	Stefano Stabellini, Wenchao Wang, Roman Bolshakov,
	Sunil Muthuswamy, Philippe Mathieu-Daudé
  Cc: Laurent Vivier, Peter Maydell, Eduardo Habkost, Paul Durrant,
	Jason Wang, Marcelo Tosatti, qemu-devel, Peter Xu,
	Dario Faggioli, Cameron Esfahani, haxm-team, Claudio Fontana,
	Anthony Perard, Bruce Rogers, Olaf Hering, Emilio G . Cota,
	Colin Xu

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                                  |  6 ++-
 .../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                   |  4 +-
 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, 349 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} (72%)

diff --git a/accel/accel-softmmu.h b/accel/accel-softmmu.h
new file mode 100644
index 0000000000..2877b5c234
--- /dev/null
+++ b/accel/accel-softmmu.h
@@ -0,0 +1,15 @@
+/*
+ * QEMU System Emulation accel internal functions
+ *
+ * Copyright 2020 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..0af91dd3b3
--- /dev/null
+++ b/accel/tcg/tcg-accel-ops-mttcg.h
@@ -0,0 +1,19 @@
+/*
+ * QEMU TCG Multi Threaded vCPUs implementation
+ *
+ * Copyright 2020 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..6102d2f80d
--- /dev/null
+++ b/include/sysemu/accel-ops.h
@@ -0,0 +1,45 @@
+/*
+ * Accelerator OPS, used for cpus.c module
+ *
+ * Copyright 2020 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_OPS_H
+#define ACCEL_OPS_H
+
+#include "qom/object.h"
+
+#define ACCEL_OPS_SUFFIX "-ops"
+#define TYPE_ACCEL_OPS "accel" ACCEL_OPS_SUFFIX
+#define ACCEL_OPS_NAME(name) (name "-" TYPE_ACCEL_OPS)
+
+typedef struct AccelOpsClass AccelOpsClass;
+DECLARE_CLASS_CHECKERS(AccelOpsClass, ACCEL_OPS, TYPE_ACCEL_OPS)
+
+/* cpus.c operations interface */
+struct AccelOpsClass {
+    /*< private >*/
+    ObjectClass parent_class;
+    /*< public >*/
+
+    /* initialization function called when accel is chosen */
+    void (*ops_init)(AccelOpsClass *ops);
+
+    void (*create_vcpu_thread)(CPUState *cpu); /* MANDATORY NON-NULL */
+    void (*kick_vcpu_thread)(CPUState *cpu);
+
+    void (*synchronize_post_reset)(CPUState *cpu);
+    void (*synchronize_post_init)(CPUState *cpu);
+    void (*synchronize_state)(CPUState *cpu);
+    void (*synchronize_pre_loadvm)(CPUState *cpu);
+
+    void (*handle_interrupt)(CPUState *cpu, int mask);
+
+    int64_t (*get_virtual_clock)(void);
+    int64_t (*get_elapsed_ticks)(void);
+};
+
+#endif /* ACCEL_OPS_H */
diff --git a/include/sysemu/cpus.h b/include/sysemu/cpus.h
index e8156728c6..2cd74392e0 100644
--- a/include/sysemu/cpus.h
+++ b/include/sysemu/cpus.h
@@ -2,30 +2,14 @@
 #define QEMU_CPUS_H
 
 #include "qemu/timer.h"
+#include "sysemu/accel-ops.h"
 
-/* cpus.c */
+/* register accel-specific operations */
+void cpus_register_accel(const AccelOpsClass *i);
 
-/* CPU execution threads */
+/* accel/dummy-cpus.c */
 
-typedef struct CpusAccel {
-    void (*create_vcpu_thread)(CPUState *cpu); /* MANDATORY */
-    void (*kick_vcpu_thread)(CPUState *cpu);
-
-    void (*synchronize_post_reset)(CPUState *cpu);
-    void (*synchronize_post_init)(CPUState *cpu);
-    void (*synchronize_state)(CPUState *cpu);
-    void (*synchronize_pre_loadvm)(CPUState *cpu);
-
-    void (*handle_interrupt)(CPUState *cpu, int mask);
-
-    int64_t (*get_virtual_clock)(void);
-    int64_t (*get_elapsed_ticks)(void);
-} CpusAccel;
-
-/* register accel-specific cpus interface implementation */
-void cpus_register_accel(const CpusAccel *i);
-
-/* Create a dummy vcpu for CpusAccel->create_vcpu_thread */
+/* Create a dummy vcpu for AccelOpsClass->create_vcpu_thread */
 void dummy_start_vcpu_thread(CPUState *);
 
 /* interface available for cpus accelerator threads */
diff --git a/target/i386/hax/hax-cpus.h b/target/i386/hax/hax-accel-ops.h
similarity index 95%
rename from target/i386/hax/hax-cpus.h
rename to target/i386/hax/hax-accel-ops.h
index ee8ab7a631..c7698519cd 100644
--- a/target/i386/hax/hax-cpus.h
+++ b/target/i386/hax/hax-accel-ops.h
@@ -12,8 +12,6 @@
 
 #include "sysemu/cpus.h"
 
-extern const CpusAccel hax_cpus;
-
 #include "hax-interface.h"
 #include "hax-i386.h"
 
diff --git a/target/i386/hax/hax-windows.h b/target/i386/hax/hax-windows.h
index a5ce12d663..b1f5d4f32f 100644
--- a/target/i386/hax/hax-windows.h
+++ b/target/i386/hax/hax-windows.h
@@ -23,7 +23,7 @@
 #include <winioctl.h>
 #include <windef.h>
 
-#include "hax-cpus.h"
+#include "hax-accel-ops.h"
 
 #define HAX_INVALID_FD INVALID_HANDLE_VALUE
 
diff --git a/target/i386/hvf/hvf-cpus.h b/target/i386/hvf/hvf-accel-ops.h
similarity index 94%
rename from target/i386/hvf/hvf-cpus.h
rename to target/i386/hvf/hvf-accel-ops.h
index ced31b82c0..8f992da168 100644
--- a/target/i386/hvf/hvf-cpus.h
+++ b/target/i386/hvf/hvf-accel-ops.h
@@ -12,8 +12,6 @@
 
 #include "sysemu/cpus.h"
 
-extern const CpusAccel hvf_cpus;
-
 int hvf_init_vcpu(CPUState *);
 int hvf_vcpu_exec(CPUState *);
 void hvf_cpu_synchronize_state(CPUState *);
diff --git a/target/i386/whpx/whpx-cpus.h b/target/i386/whpx/whpx-accel-ops.h
similarity index 96%
rename from target/i386/whpx/whpx-cpus.h
rename to target/i386/whpx/whpx-accel-ops.h
index bdb367d1d0..2dee6d61ea 100644
--- a/target/i386/whpx/whpx-cpus.h
+++ b/target/i386/whpx/whpx-accel-ops.h
@@ -12,8 +12,6 @@
 
 #include "sysemu/cpus.h"
 
-extern const CpusAccel whpx_cpus;
-
 int whpx_init_vcpu(CPUState *cpu);
 int whpx_vcpu_exec(CPUState *cpu);
 void whpx_destroy_vcpu(CPUState *cpu);
diff --git a/accel/accel-common.c b/accel/accel-common.c
index ddec8cb5ae..6b59873419 100644
--- a/accel/accel-common.c
+++ b/accel/accel-common.c
@@ -26,6 +26,10 @@
 #include "qemu/osdep.h"
 #include "qemu/accel.h"
 
+#ifndef CONFIG_USER_ONLY
+#include "accel-softmmu.h"
+#endif /* !CONFIG_USER_ONLY */
+
 static const TypeInfo accel_type = {
     .name = TYPE_ACCEL,
     .parent = TYPE_OBJECT,
@@ -42,6 +46,13 @@ AccelClass *accel_find(const char *opt_name)
     return ac;
 }
 
+void accel_init_interfaces(AccelClass *ac)
+{
+#ifndef CONFIG_USER_ONLY
+    accel_init_ops_interfaces(ac);
+#endif /* !CONFIG_USER_ONLY */
+}
+
 static void register_accel_types(void)
 {
     type_register_static(&accel_type);
diff --git a/accel/accel-softmmu.c b/accel/accel-softmmu.c
index f89da8f9d1..2d15d3f2f4 100644
--- a/accel/accel-softmmu.c
+++ b/accel/accel-softmmu.c
@@ -26,9 +26,9 @@
 #include "qemu/osdep.h"
 #include "qemu/accel.h"
 #include "hw/boards.h"
-#include "sysemu/arch_init.h"
-#include "sysemu/sysemu.h"
-#include "qom/object.h"
+#include "sysemu/cpus.h"
+
+#include "accel-softmmu.h"
 
 int accel_init_machine(AccelState *accel, MachineState *ms)
 {
@@ -60,3 +60,40 @@ void accel_setup_post(MachineState *ms)
         acc->setup_post(ms, accel);
     }
 }
+
+/* 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 (ops->ops_init) {
+        ops->ops_init(ops);
+    }
+    cpus_register_accel(ops);
+}
+
+static const TypeInfo accel_ops_type_info = {
+    .name = TYPE_ACCEL_OPS,
+    .parent = TYPE_OBJECT,
+    .abstract = true,
+    .class_size = sizeof(AccelOpsClass),
+};
+static void accel_softmmu_register_types(void)
+{
+    type_register_static(&accel_ops_type_info);
+}
+type_init(accel_softmmu_register_types);
diff --git a/accel/kvm/kvm-cpus.c b/accel/kvm/kvm-accel-ops.c
similarity index 72%
rename from accel/kvm/kvm-cpus.c
rename to accel/kvm/kvm-accel-ops.c
index d809b1e74c..430d09de03 100644
--- a/accel/kvm/kvm-cpus.c
+++ b/accel/kvm/kvm-accel-ops.c
@@ -74,11 +74,25 @@ static void kvm_start_vcpu_thread(CPUState *cpu)
                        cpu, QEMU_THREAD_JOINABLE);
 }
 
-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;
 };
+static const TypeInfo kvm_accel_ops_type = {
+    .name = ACCEL_OPS_NAME("kvm"),
+
+    .parent = TYPE_ACCEL_OPS,
+    .class_init = kvm_accel_ops_class_init,
+    .abstract = true,
+};
+static void kvm_accel_ops_register_types(void)
+{
+    type_register_static(&kvm_accel_ops_type);
+}
+type_init(kvm_accel_ops_register_types);
diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index baaa54249d..18be3cd113 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -2253,8 +2253,6 @@ static int kvm_init(MachineState *ms)
         ret = ram_block_discard_disable(true);
         assert(!ret);
     }
-
-    cpus_register_accel(&kvm_cpus);
     return 0;
 
 err:
diff --git a/accel/qtest/qtest.c b/accel/qtest/qtest.c
index b4e731cb2b..edb29f6fa4 100644
--- a/accel/qtest/qtest.c
+++ b/accel/qtest/qtest.c
@@ -25,14 +25,8 @@
 #include "qemu/main-loop.h"
 #include "hw/core/cpu.h"
 
-const CpusAccel qtest_cpus = {
-    .create_vcpu_thread = dummy_start_vcpu_thread,
-    .get_virtual_clock = qtest_get_virtual_clock,
-};
-
 static int qtest_init_accel(MachineState *ms)
 {
-    cpus_register_accel(&qtest_cpus);
     return 0;
 }
 
@@ -52,9 +46,26 @@ static const TypeInfo qtest_accel_type = {
     .class_init = qtest_accel_class_init,
 };
 
+static void qtest_accel_ops_class_init(ObjectClass *oc, void *data)
+{
+    AccelOpsClass *ops = ACCEL_OPS_CLASS(oc);
+
+    ops->create_vcpu_thread = dummy_start_vcpu_thread;
+    ops->get_virtual_clock = qtest_get_virtual_clock;
+};
+
+static const TypeInfo qtest_accel_ops_type = {
+    .name = ACCEL_OPS_NAME("qtest"),
+
+    .parent = TYPE_ACCEL_OPS,
+    .class_init = qtest_accel_ops_class_init,
+    .abstract = true,
+};
+
 static void qtest_type_init(void)
 {
     type_register_static(&qtest_accel_type);
+    type_register_static(&qtest_accel_ops_type);
 }
 
 type_init(qtest_type_init);
diff --git a/accel/tcg/tcg-cpus-icount.c b/accel/tcg/tcg-accel-ops-icount.c
similarity index 89%
rename from accel/tcg/tcg-cpus-icount.c
rename to accel/tcg/tcg-accel-ops-icount.c
index 9f45432275..87762b469c 100644
--- a/accel/tcg/tcg-cpus-icount.c
+++ b/accel/tcg/tcg-accel-ops-icount.c
@@ -32,9 +32,9 @@
 #include "exec/exec-all.h"
 #include "hw/boards.h"
 
-#include "tcg-cpus.h"
-#include "tcg-cpus-icount.h"
-#include "tcg-cpus-rr.h"
+#include "tcg-accel-ops.h"
+#include "tcg-accel-ops-icount.h"
+#include "tcg-accel-ops-rr.h"
 
 static int64_t icount_get_limit(void)
 {
@@ -93,7 +93,7 @@ void icount_prepare_for_run(CPUState *cpu)
     /*
      * These should always be cleared by icount_process_data after
      * each vCPU execution. However u16.high can be raised
-     * asynchronously by cpu_exit/cpu_interrupt/tcg_cpus_handle_interrupt
+     * asynchronously by cpu_exit/cpu_interrupt/tcg_handle_interrupt
      */
     g_assert(cpu_neg(cpu)->icount_decr.u16.low == 0);
     g_assert(cpu->icount_extra == 0);
@@ -125,23 +125,14 @@ void icount_process_data(CPUState *cpu)
     replay_mutex_unlock();
 }
 
-static void icount_handle_interrupt(CPUState *cpu, int mask)
+void icount_handle_interrupt(CPUState *cpu, int mask)
 {
     int old_mask = cpu->interrupt_request;
 
-    tcg_cpus_handle_interrupt(cpu, mask);
+    tcg_handle_interrupt(cpu, mask);
     if (qemu_cpu_is_self(cpu) &&
         !cpu->can_do_io
         && (mask & ~old_mask) != 0) {
         cpu_abort(cpu, "Raised interrupt while not in I/O function");
     }
 }
-
-const CpusAccel tcg_cpus_icount = {
-    .create_vcpu_thread = rr_start_vcpu_thread,
-    .kick_vcpu_thread = rr_kick_vcpu_thread,
-
-    .handle_interrupt = icount_handle_interrupt,
-    .get_virtual_clock = icount_get,
-    .get_elapsed_ticks = icount_get,
-};
diff --git a/accel/tcg/tcg-cpus-mttcg.c b/accel/tcg/tcg-accel-ops-mttcg.c
similarity index 92%
rename from accel/tcg/tcg-cpus-mttcg.c
rename to accel/tcg/tcg-accel-ops-mttcg.c
index 9c3767d260..42973fb062 100644
--- a/accel/tcg/tcg-cpus-mttcg.c
+++ b/accel/tcg/tcg-accel-ops-mttcg.c
@@ -32,7 +32,8 @@
 #include "exec/exec-all.h"
 #include "hw/boards.h"
 
-#include "tcg-cpus.h"
+#include "tcg-accel-ops.h"
+#include "tcg-accel-ops-mttcg.h"
 
 /*
  * In the multi-threaded case each vCPU has its own thread. The TLS
@@ -103,12 +104,12 @@ static void *mttcg_cpu_thread_fn(void *arg)
     return NULL;
 }
 
-static void mttcg_kick_vcpu_thread(CPUState *cpu)
+void mttcg_kick_vcpu_thread(CPUState *cpu)
 {
     cpu_exit(cpu);
 }
 
-static void mttcg_start_vcpu_thread(CPUState *cpu)
+void mttcg_start_vcpu_thread(CPUState *cpu)
 {
     char thread_name[VCPU_THREAD_NAME_SIZE];
 
@@ -131,10 +132,3 @@ static void mttcg_start_vcpu_thread(CPUState *cpu)
     cpu->hThread = qemu_thread_get_handle(cpu->thread);
 #endif
 }
-
-const CpusAccel tcg_cpus_mttcg = {
-    .create_vcpu_thread = mttcg_start_vcpu_thread,
-    .kick_vcpu_thread = mttcg_kick_vcpu_thread,
-
-    .handle_interrupt = tcg_cpus_handle_interrupt,
-};
diff --git a/accel/tcg/tcg-cpus-rr.c b/accel/tcg/tcg-accel-ops-rr.c
similarity index 97%
rename from accel/tcg/tcg-cpus-rr.c
rename to accel/tcg/tcg-accel-ops-rr.c
index 0181d2e4eb..4a66055e0d 100644
--- a/accel/tcg/tcg-cpus-rr.c
+++ b/accel/tcg/tcg-accel-ops-rr.c
@@ -32,9 +32,9 @@
 #include "exec/exec-all.h"
 #include "hw/boards.h"
 
-#include "tcg-cpus.h"
-#include "tcg-cpus-rr.h"
-#include "tcg-cpus-icount.h"
+#include "tcg-accel-ops.h"
+#include "tcg-accel-ops-rr.h"
+#include "tcg-accel-ops-icount.h"
 
 /* Kick all RR vCPUs */
 void rr_kick_vcpu_thread(CPUState *unused)
@@ -296,10 +296,3 @@ void rr_start_vcpu_thread(CPUState *cpu)
         cpu->created = true;
     }
 }
-
-const CpusAccel tcg_cpus_rr = {
-    .create_vcpu_thread = rr_start_vcpu_thread,
-    .kick_vcpu_thread = rr_kick_vcpu_thread,
-
-    .handle_interrupt = tcg_cpus_handle_interrupt,
-};
diff --git a/accel/tcg/tcg-cpus.c b/accel/tcg/tcg-accel-ops.c
similarity index 63%
rename from accel/tcg/tcg-cpus.c
rename to accel/tcg/tcg-accel-ops.c
index e335f9f155..3017f66dd7 100644
--- a/accel/tcg/tcg-cpus.c
+++ b/accel/tcg/tcg-accel-ops.c
@@ -34,7 +34,10 @@
 #include "exec/exec-all.h"
 #include "hw/boards.h"
 
-#include "tcg-cpus.h"
+#include "tcg-accel-ops.h"
+#include "tcg-accel-ops-mttcg.h"
+#include "tcg-accel-ops-rr.h"
+#include "tcg-accel-ops-icount.h"
 
 /* common functionality among all TCG variants */
 
@@ -64,7 +67,7 @@ int tcg_cpus_exec(CPUState *cpu)
 }
 
 /* mask must never be zero, except for A20 change call */
-void tcg_cpus_handle_interrupt(CPUState *cpu, int mask)
+void tcg_handle_interrupt(CPUState *cpu, int mask)
 {
     g_assert(qemu_mutex_iothread_locked());
 
@@ -80,3 +83,43 @@ void tcg_cpus_handle_interrupt(CPUState *cpu, int mask)
         qatomic_set(&cpu_neg(cpu)->icount_decr.u16.high, -1);
     }
 }
+
+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;
+    }
+}
+
+static void tcg_accel_ops_class_init(ObjectClass *oc, void *data)
+{
+    AccelOpsClass *ops = ACCEL_OPS_CLASS(oc);
+
+    ops->ops_init = tcg_accel_ops_init;
+};
+static const TypeInfo tcg_accel_ops_type = {
+    .name = ACCEL_OPS_NAME("tcg"),
+
+    .parent = TYPE_ACCEL_OPS,
+    .class_init = tcg_accel_ops_class_init,
+    .abstract = true,
+};
+static void tcg_accel_ops_register_types(void)
+{
+    type_register_static(&tcg_accel_ops_type);
+}
+type_init(tcg_accel_ops_register_types);
diff --git a/accel/tcg/tcg-all.c b/accel/tcg/tcg-all.c
index 7125d0cc29..f0d97b4b21 100644
--- a/accel/tcg/tcg-all.c
+++ b/accel/tcg/tcg-all.c
@@ -33,10 +33,6 @@
 #include "qemu/accel.h"
 #include "qapi/qapi-builtin-visit.h"
 
-#ifndef CONFIG_USER_ONLY
-#include "tcg-cpus.h"
-#endif /* CONFIG_USER_ONLY */
-
 struct TCGState {
     AccelState parent_obj;
 
@@ -116,14 +112,6 @@ static int tcg_init(MachineState *ms)
      */
 #ifndef CONFIG_USER_ONLY
     tcg_region_init();
-
-    if (mttcg_enabled) {
-        cpus_register_accel(&tcg_cpus_mttcg);
-    } else if (icount_enabled()) {
-        cpus_register_accel(&tcg_cpus_icount);
-    } else {
-        cpus_register_accel(&tcg_cpus_rr);
-    }
 #endif /* !CONFIG_USER_ONLY */
 
     return 0;
diff --git a/accel/xen/xen-all.c b/accel/xen/xen-all.c
index 594aaf6b49..82fc21f1ce 100644
--- a/accel/xen/xen-all.c
+++ b/accel/xen/xen-all.c
@@ -154,10 +154,6 @@ static void xen_setup_post(MachineState *ms, AccelState *accel)
     }
 }
 
-const CpusAccel xen_cpus = {
-    .create_vcpu_thread = dummy_start_vcpu_thread,
-};
-
 static int xen_init(MachineState *ms)
 {
     MachineClass *mc = MACHINE_GET_CLASS(ms);
@@ -185,9 +181,6 @@ static int xen_init(MachineState *ms)
      * opt out of system RAM being allocated by generic code
      */
     mc->default_ram_id = NULL;
-
-    cpus_register_accel(&xen_cpus);
-
     return 0;
 }
 
@@ -222,9 +215,24 @@ static const TypeInfo xen_accel_type = {
     .class_init = xen_accel_class_init,
 };
 
+static void xen_accel_ops_class_init(ObjectClass *oc, void *data)
+{
+    AccelOpsClass *ops = ACCEL_OPS_CLASS(oc);
+
+    ops->create_vcpu_thread = dummy_start_vcpu_thread;
+};
+static const TypeInfo xen_accel_ops_type = {
+    .name = ACCEL_OPS_NAME("xen"),
+
+    .parent = TYPE_ACCEL_OPS,
+    .class_init = xen_accel_ops_class_init,
+    .abstract = true,
+};
+
 static void xen_type_init(void)
 {
     type_register_static(&xen_accel_type);
+    type_register_static(&xen_accel_ops_type);
 }
 
 type_init(xen_type_init);
diff --git a/bsd-user/main.c b/bsd-user/main.c
index ff295bcb29..a68ce5f446 100644
--- a/bsd-user/main.c
+++ b/bsd-user/main.c
@@ -908,13 +908,14 @@ int main(int argc, char **argv)
 #endif
     }
 
+    cpu_type = parse_cpu_option(cpu_model);
     /* init tcg before creating CPUs and to get qemu_host_page_size */
     {
         AccelClass *ac = ACCEL_GET_CLASS(current_accel());
 
         ac->init_machine(NULL);
+        accel_init_interfaces(ac);
     }
-    cpu_type = parse_cpu_option(cpu_model);
     cpu = cpu_create(cpu_type);
     env = cpu->env_ptr;
 #if defined(TARGET_SPARC) || defined(TARGET_PPC)
diff --git a/linux-user/main.c b/linux-user/main.c
index 5c059a8445..e28d016c6f 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -708,6 +708,7 @@ int main(int argc, char **argv, char **envp)
         AccelClass *ac = ACCEL_GET_CLASS(current_accel());
 
         ac->init_machine(NULL);
+        accel_init_interfaces(ac);
     }
     cpu = cpu_create(cpu_type);
     env = cpu->env_ptr;
diff --git a/softmmu/cpus.c b/softmmu/cpus.c
index e46ac68ad0..659617e7ef 100644
--- a/softmmu/cpus.c
+++ b/softmmu/cpus.c
@@ -127,7 +127,7 @@ void hw_error(const char *fmt, ...)
 /*
  * The chosen accelerator is supposed to register this.
  */
-static const CpusAccel *cpus_accel;
+static const AccelOpsClass *cpus_accel;
 
 void cpu_synchronize_all_states(void)
 {
@@ -593,11 +593,11 @@ void cpu_remove_sync(CPUState *cpu)
     qemu_mutex_lock_iothread();
 }
 
-void cpus_register_accel(const CpusAccel *ca)
+void cpus_register_accel(const AccelOpsClass *ops)
 {
-    assert(ca != NULL);
-    assert(ca->create_vcpu_thread != NULL); /* mandatory */
-    cpus_accel = ca;
+    assert(ops != NULL);
+    assert(ops->create_vcpu_thread != NULL); /* mandatory */
+    cpus_accel = ops;
 }
 
 void qemu_init_vcpu(CPUState *cpu)
@@ -617,7 +617,7 @@ void qemu_init_vcpu(CPUState *cpu)
         cpu_address_space_init(cpu, 0, "cpu-memory", cpu->memory);
     }
 
-    /* accelerators all implement the CpusAccel interface */
+    /* accelerators all implement the AccelOpsClass */
     g_assert(cpus_accel != NULL && cpus_accel->create_vcpu_thread != NULL);
     cpus_accel->create_vcpu_thread(cpu);
 
diff --git a/softmmu/vl.c b/softmmu/vl.c
index bc20c526d2..b97708300e 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -2558,7 +2558,7 @@ static bool object_create_initial(const char *type, QemuOpts *opts)
     }
 
     /* Memory allocation by backends needs to be done
-     * after configure_accelerator() (due to the tcg_enabled()
+     * after do_configure_accelerator() (due to the tcg_enabled()
      * checks at memory_region_init_*()).
      *
      * Also, allocation of large amounts of memory may delay
@@ -4186,7 +4186,7 @@ void qemu_init(int argc, char **argv, char **envp)
      *
      * Machine compat properties: object_set_machine_compat_props().
      * Accelerator compat props: object_set_accelerator_compat_props(),
-     * called from configure_accelerator().
+     * called from do_configure_accelerator().
      */
 
     if (!qtest_enabled() && machine_class->deprecation_reason) {
@@ -4321,6 +4321,8 @@ void qemu_init(int argc, char **argv, char **envp)
     if (cpu_option) {
         current_machine->cpu_type = parse_cpu_option(cpu_option);
     }
+    /* NB: for machine none cpu_type could STILL be NULL here! */
+    accel_init_interfaces(ACCEL_GET_CLASS(current_machine->accelerator));
 
     if (current_machine->ram_memdev_id) {
         Object *backend;
diff --git a/target/i386/hax/hax-cpus.c b/target/i386/hax/hax-accel-ops.c
similarity index 69%
rename from target/i386/hax/hax-cpus.c
rename to target/i386/hax/hax-accel-ops.c
index f72c85bd49..f66042c61e 100644
--- a/target/i386/hax/hax-cpus.c
+++ b/target/i386/hax/hax-accel-ops.c
@@ -26,7 +26,7 @@
 #include "sysemu/cpus.h"
 #include "qemu/guest-random.h"
 
-#include "hax-cpus.h"
+#include "hax-accel-ops.h"
 
 static void *hax_cpu_thread_fn(void *arg)
 {
@@ -74,12 +74,27 @@ static void hax_start_vcpu_thread(CPUState *cpu)
 #endif
 }
 
-const CpusAccel hax_cpus = {
-    .create_vcpu_thread = hax_start_vcpu_thread,
-    .kick_vcpu_thread = hax_kick_vcpu_thread,
+static void hax_accel_ops_class_init(ObjectClass *oc, void *data)
+{
+    AccelOpsClass *ops = ACCEL_OPS_CLASS(oc);
 
-    .synchronize_post_reset = hax_cpu_synchronize_post_reset,
-    .synchronize_post_init = hax_cpu_synchronize_post_init,
-    .synchronize_state = hax_cpu_synchronize_state,
-    .synchronize_pre_loadvm = hax_cpu_synchronize_pre_loadvm,
+    ops->create_vcpu_thread = hax_start_vcpu_thread;
+    ops->kick_vcpu_thread = hax_kick_vcpu_thread;
+
+    ops->synchronize_post_reset = hax_cpu_synchronize_post_reset;
+    ops->synchronize_post_init = hax_cpu_synchronize_post_init;
+    ops->synchronize_state = hax_cpu_synchronize_state;
+    ops->synchronize_pre_loadvm = hax_cpu_synchronize_pre_loadvm;
+};
+static const TypeInfo hax_accel_ops_type = {
+    .name = ACCEL_OPS_NAME("hax"),
+
+    .parent = TYPE_ACCEL_OPS,
+    .class_init = hax_accel_ops_class_init,
+    .abstract = true,
 };
+static void hax_accel_ops_register_types(void)
+{
+    type_register_static(&hax_accel_ops_type);
+}
+type_init(hax_accel_ops_register_types);
diff --git a/target/i386/hax/hax-all.c b/target/i386/hax/hax-all.c
index d7f4bb44a7..bf65ed6fa9 100644
--- a/target/i386/hax/hax-all.c
+++ b/target/i386/hax/hax-all.c
@@ -33,7 +33,7 @@
 #include "sysemu/runstate.h"
 #include "hw/boards.h"
 
-#include "hax-cpus.h"
+#include "hax-accel-ops.h"
 
 #define DEBUG_HAX 0
 
@@ -364,9 +364,6 @@ static int hax_accel_init(MachineState *ms)
                 !ret ? "working" : "not working",
                 !ret ? "fast virt" : "emulation");
     }
-    if (ret == 0) {
-        cpus_register_accel(&hax_cpus);
-    }
     return ret;
 }
 
diff --git a/target/i386/hax/hax-mem.c b/target/i386/hax/hax-mem.c
index 71e637cf16..35495f5e82 100644
--- a/target/i386/hax/hax-mem.c
+++ b/target/i386/hax/hax-mem.c
@@ -13,7 +13,7 @@
 #include "exec/address-spaces.h"
 #include "qemu/error-report.h"
 
-#include "hax-cpus.h"
+#include "hax-accel-ops.h"
 #include "qemu/queue.h"
 
 #define DEBUG_HAX_MEM 0
diff --git a/target/i386/hax/hax-posix.c b/target/i386/hax/hax-posix.c
index 735a749d4b..ac1a51096e 100644
--- a/target/i386/hax/hax-posix.c
+++ b/target/i386/hax/hax-posix.c
@@ -15,7 +15,7 @@
 #include <sys/ioctl.h>
 
 #include "sysemu/cpus.h"
-#include "hax-cpus.h"
+#include "hax-accel-ops.h"
 
 hax_fd hax_mod_open(void)
 {
diff --git a/target/i386/hax/hax-windows.c b/target/i386/hax/hax-windows.c
index 6c82dfb54f..59afa213a6 100644
--- a/target/i386/hax/hax-windows.c
+++ b/target/i386/hax/hax-windows.c
@@ -12,7 +12,7 @@
 
 #include "qemu/osdep.h"
 #include "cpu.h"
-#include "hax-cpus.h"
+#include "hax-accel-ops.h"
 
 /*
  * return 0 when success, -1 when driver not loaded,
diff --git a/target/i386/hvf/hvf-cpus.c b/target/i386/hvf/hvf-accel-ops.c
similarity index 84%
rename from target/i386/hvf/hvf-cpus.c
rename to target/i386/hvf/hvf-accel-ops.c
index 817b3d7452..cbaad238e0 100644
--- a/target/i386/hvf/hvf-cpus.c
+++ b/target/i386/hvf/hvf-accel-ops.c
@@ -55,7 +55,7 @@
 #include "target/i386/cpu.h"
 #include "qemu/guest-random.h"
 
-#include "hvf-cpus.h"
+#include "hvf-accel-ops.h"
 
 /*
  * The HVF-specific vCPU thread function. This one should only run when the host
@@ -121,11 +121,26 @@ static void hvf_start_vcpu_thread(CPUState *cpu)
                        cpu, QEMU_THREAD_JOINABLE);
 }
 
-const CpusAccel hvf_cpus = {
-    .create_vcpu_thread = hvf_start_vcpu_thread,
+static void hvf_accel_ops_class_init(ObjectClass *oc, void *data)
+{
+    AccelOpsClass *ops = ACCEL_OPS_CLASS(oc);
 
-    .synchronize_post_reset = hvf_cpu_synchronize_post_reset,
-    .synchronize_post_init = hvf_cpu_synchronize_post_init,
-    .synchronize_state = hvf_cpu_synchronize_state,
-    .synchronize_pre_loadvm = hvf_cpu_synchronize_pre_loadvm,
+    ops->create_vcpu_thread = hvf_start_vcpu_thread;
+
+    ops->synchronize_post_reset = hvf_cpu_synchronize_post_reset;
+    ops->synchronize_post_init = hvf_cpu_synchronize_post_init;
+    ops->synchronize_state = hvf_cpu_synchronize_state;
+    ops->synchronize_pre_loadvm = hvf_cpu_synchronize_pre_loadvm;
+};
+static const TypeInfo hvf_accel_ops_type = {
+    .name = ACCEL_OPS_NAME("hvf"),
+
+    .parent = TYPE_ACCEL_OPS,
+    .class_init = hvf_accel_ops_class_init,
+    .abstract = true,
 };
+static void hvf_accel_ops_register_types(void)
+{
+    type_register_static(&hvf_accel_ops_type);
+}
+type_init(hvf_accel_ops_register_types);
diff --git a/target/i386/hvf/hvf.c b/target/i386/hvf/hvf.c
index ffc9efa40f..5b90dcdf88 100644
--- a/target/i386/hvf/hvf.c
+++ b/target/i386/hvf/hvf.c
@@ -72,7 +72,7 @@
 #include "qemu/accel.h"
 #include "target/i386/cpu.h"
 
-#include "hvf-cpus.h"
+#include "hvf-accel-ops.h"
 
 HVFState *hvf_state;
 
@@ -887,7 +887,6 @@ static int hvf_accel_init(MachineState *ms)
   
     hvf_state = s;
     memory_listener_register(&hvf_memory_listener, &address_space_memory);
-    cpus_register_accel(&hvf_cpus);
     return 0;
 }
 
diff --git a/target/i386/hvf/x86hvf.c b/target/i386/hvf/x86hvf.c
index bbec412b6c..0d7533742e 100644
--- a/target/i386/hvf/x86hvf.c
+++ b/target/i386/hvf/x86hvf.c
@@ -32,7 +32,7 @@
 #include <Hypervisor/hv.h>
 #include <Hypervisor/hv_vmx.h>
 
-#include "hvf-cpus.h"
+#include "hvf-accel-ops.h"
 
 void hvf_set_segment(struct CPUState *cpu, struct vmx_segment *vmx_seg,
                      SegmentCache *qseg, bool is_tr)
diff --git a/target/i386/whpx/whpx-cpus.c b/target/i386/whpx/whpx-accel-ops.c
similarity index 72%
rename from target/i386/whpx/whpx-cpus.c
rename to target/i386/whpx/whpx-accel-ops.c
index d9bd5a2d36..b880947cc2 100644
--- a/target/i386/whpx/whpx-cpus.c
+++ b/target/i386/whpx/whpx-accel-ops.c
@@ -15,7 +15,7 @@
 #include "qemu/guest-random.h"
 
 #include "sysemu/whpx.h"
-#include "whpx-cpus.h"
+#include "whpx-accel-ops.h"
 
 #include <WinHvPlatform.h>
 #include <WinHvEmulation.h>
@@ -85,12 +85,27 @@ static void whpx_kick_vcpu_thread(CPUState *cpu)
     }
 }
 
-const CpusAccel whpx_cpus = {
-    .create_vcpu_thread = whpx_start_vcpu_thread,
-    .kick_vcpu_thread = whpx_kick_vcpu_thread,
+static void whpx_accel_ops_class_init(ObjectClass *oc, void *data)
+{
+    AccelOpsClass *ops = ACCEL_OPS_CLASS(oc);
 
-    .synchronize_post_reset = whpx_cpu_synchronize_post_reset,
-    .synchronize_post_init = whpx_cpu_synchronize_post_init,
-    .synchronize_state = whpx_cpu_synchronize_state,
-    .synchronize_pre_loadvm = whpx_cpu_synchronize_pre_loadvm,
+    ops->create_vcpu_thread = whpx_start_vcpu_thread;
+    ops->kick_vcpu_thread = whpx_kick_vcpu_thread;
+
+    ops->synchronize_post_reset = whpx_cpu_synchronize_post_reset;
+    ops->synchronize_post_init = whpx_cpu_synchronize_post_init;
+    ops->synchronize_state = whpx_cpu_synchronize_state;
+    ops->synchronize_pre_loadvm = whpx_cpu_synchronize_pre_loadvm;
+};
+static const TypeInfo whpx_accel_ops_type = {
+    .name = ACCEL_OPS_NAME("whpx"),
+
+    .parent = TYPE_ACCEL_OPS,
+    .class_init = whpx_accel_ops_class_init,
+    .abstract = true,
 };
+static void whpx_accel_ops_register_types(void)
+{
+    type_register_static(&whpx_accel_ops_type);
+}
+type_init(whpx_accel_ops_register_types);
diff --git a/target/i386/whpx/whpx-all.c b/target/i386/whpx/whpx-all.c
index ee6b606194..fbac300ada 100644
--- a/target/i386/whpx/whpx-all.c
+++ b/target/i386/whpx/whpx-all.c
@@ -24,7 +24,7 @@
 #include "migration/blocker.h"
 #include "whp-dispatch.h"
 
-#include "whpx-cpus.h"
+#include "whpx-accel-ops.h"
 
 #include <WinHvPlatform.h>
 #include <WinHvEmulation.h>
@@ -1642,8 +1642,6 @@ static int whpx_accel_init(MachineState *ms)
 
     whpx_memory_init();
 
-    cpus_register_accel(&whpx_cpus);
-
     printf("Windows Hypervisor Platform accelerator is operational\n");
     return 0;
 
diff --git a/MAINTAINERS b/MAINTAINERS
index 6235dd3a9f..8f0e773a47 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -435,7 +435,8 @@ M: Richard Henderson <richard.henderson@linaro.org>
 R: Paolo Bonzini <pbonzini@redhat.com>
 S: Maintained
 F: include/qemu/accel.h
-F: accel/accel.c
+F: include/sysemu/accel-ops.h
+F: accel/accel-*.c
 F: accel/Makefile.objs
 F: accel/stubs/Makefile.objs
 
diff --git a/accel/kvm/meson.build b/accel/kvm/meson.build
index 7e9dafe24c..8d219bea50 100644
--- a/accel/kvm/meson.build
+++ b/accel/kvm/meson.build
@@ -1,7 +1,7 @@
 kvm_ss = ss.source_set()
 kvm_ss.add(files(
   'kvm-all.c',
-  'kvm-cpus.c',
+  'kvm-accel-ops.c',
 ))
 kvm_ss.add(when: 'CONFIG_SEV', if_false: files('sev-stub.c'))
 
diff --git a/accel/tcg/meson.build b/accel/tcg/meson.build
index 424d9bb1fc..1236ac7b91 100644
--- a/accel/tcg/meson.build
+++ b/accel/tcg/meson.build
@@ -15,8 +15,8 @@ specific_ss.add_all(when: 'CONFIG_TCG', if_true: tcg_ss)
 
 specific_ss.add(when: ['CONFIG_SOFTMMU', 'CONFIG_TCG'], if_true: files(
   'cputlb.c',
-  'tcg-cpus.c',
-  'tcg-cpus-mttcg.c',
-  'tcg-cpus-icount.c',
-  'tcg-cpus-rr.c'
+  'tcg-accel-ops.c',
+  'tcg-accel-ops-mttcg.c',
+  'tcg-accel-ops-icount.c',
+  'tcg-accel-ops-rr.c'
 ))
diff --git a/target/i386/hax/meson.build b/target/i386/hax/meson.build
index 77ea431b30..d6c520fb6b 100644
--- a/target/i386/hax/meson.build
+++ b/target/i386/hax/meson.build
@@ -1,7 +1,7 @@
 i386_softmmu_ss.add(when: 'CONFIG_HAX', if_true: files(
   'hax-all.c',
   'hax-mem.c',
-  'hax-cpus.c',
+  'hax-accel-ops.c',
 ))
 i386_softmmu_ss.add(when: ['CONFIG_HAX', 'CONFIG_POSIX'], if_true: files('hax-posix.c'))
 i386_softmmu_ss.add(when: ['CONFIG_HAX', 'CONFIG_WIN32'], if_true: files('hax-windows.c'))
diff --git a/target/i386/hvf/meson.build b/target/i386/hvf/meson.build
index 409c9a3f14..e9eb5a5da8 100644
--- a/target/i386/hvf/meson.build
+++ b/target/i386/hvf/meson.build
@@ -1,6 +1,6 @@
 i386_softmmu_ss.add(when: [hvf, 'CONFIG_HVF'], if_true: files(
   'hvf.c',
-  'hvf-cpus.c',
+  'hvf-accel-ops.c',
   'x86.c',
   'x86_cpuid.c',
   'x86_decode.c',
diff --git a/target/i386/whpx/meson.build b/target/i386/whpx/meson.build
index 94a72c8efc..e790b42992 100644
--- a/target/i386/whpx/meson.build
+++ b/target/i386/whpx/meson.build
@@ -1,4 +1,4 @@
 i386_softmmu_ss.add(when: 'CONFIG_WHPX', if_true: files(
   'whpx-all.c',
-  'whpx-cpus.c',
+  'whpx-accel-ops.c',
 ))
-- 
2.26.2



^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v11 3/7] accel: introduce AccelCPUClass extending CPUClass
  2020-12-11 10:09 [PATCH v11 0/7] i386 cleanup PART 2 Claudio Fontana
  2020-12-11 10:09 ` [PATCH v11 1/7] accel: extend AccelState and AccelClass to user-mode Claudio Fontana
  2020-12-11 10:09 ` [PATCH v11 2/7] accel: replace struct CpusAccel with AccelOpsClass Claudio Fontana
@ 2020-12-11 10:09 ` Claudio Fontana
  2020-12-11 10:09 ` [PATCH v11 4/7] i386: split cpu accelerators from cpu.c, using AccelCPUClass Claudio Fontana
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 19+ messages in thread
From: Claudio Fontana @ 2020-12-11 10:09 UTC (permalink / raw)
  To: Paolo Bonzini, Thomas Huth, Richard Henderson,
	Stefano Stabellini, Wenchao Wang, Roman Bolshakov,
	Sunil Muthuswamy, Philippe Mathieu-Daudé
  Cc: Laurent Vivier, Peter Maydell, Eduardo Habkost, Paul Durrant,
	Jason Wang, Marcelo Tosatti, qemu-devel, Peter Xu,
	Dario Faggioli, Cameron Esfahani, haxm-team, Claudio Fontana,
	Anthony Perard, Bruce Rogers, Olaf Hering, Emilio G . Cota,
	Colin Xu

add a new optional interface to CPUClass,
which allows accelerators to extend the CPUClass
with additional accelerator-specific initializations.

Signed-off-by: Claudio Fontana <cfontana@suse.de>
---
 include/hw/core/accel-cpu.h | 25 +++++++++++++++++++++
 include/hw/core/cpu.h       | 13 +++++++++++
 accel/accel-common.c        | 44 +++++++++++++++++++++++++++++++++++++
 MAINTAINERS                 |  1 +
 4 files changed, 83 insertions(+)
 create mode 100644 include/hw/core/accel-cpu.h

diff --git a/include/hw/core/accel-cpu.h b/include/hw/core/accel-cpu.h
new file mode 100644
index 0000000000..dce08a9100
--- /dev/null
+++ b/include/hw/core/accel-cpu.h
@@ -0,0 +1,25 @@
+/*
+ * Accelerator interface, specializes CPUClass
+ *
+ * Copyright 2020 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_CPU_H
+#define ACCEL_CPU_H
+
+/*
+ * these defines cannot be in cpu.h, because we are using
+ * CPU_RESOLVING_TYPE here.
+ * Use this header to define your accelerator-specific
+ * cpu-specific accelerator interfaces.
+ */
+
+#define TYPE_ACCEL_CPU "accel-" CPU_RESOLVING_TYPE
+#define ACCEL_CPU_NAME(name) (name "-" TYPE_ACCEL_CPU)
+typedef struct AccelCPUClass AccelCPUClass;
+DECLARE_CLASS_CHECKERS(AccelCPUClass, ACCEL_CPU, TYPE_ACCEL_CPU)
+
+#endif /* ACCEL_CPU_H */
diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index db54223983..97e1dd8279 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -76,6 +76,17 @@ typedef struct CPUWatchpoint CPUWatchpoint;
 
 struct TranslationBlock;
 
+/* see also accel-cpu.h */
+typedef struct AccelCPUClass {
+    /*< private >*/
+    ObjectClass parent_class;
+    /*< public >*/
+
+    void (*cpu_class_init)(CPUClass *cc);
+    void (*cpu_instance_init)(CPUState *cpu);
+    void (*cpu_realizefn)(CPUState *cpu, Error **errp);
+} AccelCPUClass;
+
 #ifdef CONFIG_TCG
 #include "tcg-cpu-ops.h"
 #endif /* CONFIG_TCG */
@@ -196,6 +207,8 @@ struct CPUClass {
 #ifdef CONFIG_TCG
     TcgCpuOperations tcg_ops;
 #endif /* CONFIG_TCG */
+
+    AccelCPUClass *accel_cpu_interface;
 };
 
 /*
diff --git a/accel/accel-common.c b/accel/accel-common.c
index 6b59873419..ef73c761fc 100644
--- a/accel/accel-common.c
+++ b/accel/accel-common.c
@@ -26,6 +26,9 @@
 #include "qemu/osdep.h"
 #include "qemu/accel.h"
 
+#include "cpu.h"
+#include "hw/core/accel-cpu.h"
+
 #ifndef CONFIG_USER_ONLY
 #include "accel-softmmu.h"
 #endif /* !CONFIG_USER_ONLY */
@@ -46,16 +49,57 @@ AccelClass *accel_find(const char *opt_name)
     return ac;
 }
 
+static void accel_init_cpu_int_aux(ObjectClass *klass, void *opaque)
+{
+    CPUClass *cc = CPU_CLASS(klass);
+    AccelCPUClass *accel_cpu_interface = opaque;
+
+    cc->accel_cpu_interface = accel_cpu_interface;
+    if (accel_cpu_interface->cpu_class_init) {
+        accel_cpu_interface->cpu_class_init(cc);
+    }
+}
+
+/* initialize the arch-specific accel CpuClass interfaces */
+static void accel_init_cpu_interfaces(AccelClass *ac)
+{
+    const char *ac_name; /* AccelClass name */
+    char *acc_name;      /* AccelCPUClass name */
+    ObjectClass *acc;    /* AccelCPUClass */
+
+    ac_name = object_class_get_name(OBJECT_CLASS(ac));
+    g_assert(ac_name != NULL);
+
+    acc_name = g_strdup_printf("%s-%s", ac_name, CPU_RESOLVING_TYPE);
+    acc = object_class_by_name(acc_name);
+    g_free(acc_name);
+
+    if (acc) {
+        object_class_foreach(accel_init_cpu_int_aux,
+                             CPU_RESOLVING_TYPE, false, acc);
+    }
+}
+
 void accel_init_interfaces(AccelClass *ac)
 {
 #ifndef CONFIG_USER_ONLY
     accel_init_ops_interfaces(ac);
 #endif /* !CONFIG_USER_ONLY */
+
+    accel_init_cpu_interfaces(ac);
 }
 
+static const TypeInfo accel_cpu_type = {
+    .name = TYPE_ACCEL_CPU,
+    .parent = TYPE_OBJECT,
+    .abstract = true,
+    .class_size = sizeof(AccelCPUClass),
+};
+
 static void register_accel_types(void)
 {
     type_register_static(&accel_type);
+    type_register_static(&accel_cpu_type);
 }
 
 type_init(register_accel_types);
diff --git a/MAINTAINERS b/MAINTAINERS
index 8f0e773a47..f084d73f6b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -436,6 +436,7 @@ R: Paolo Bonzini <pbonzini@redhat.com>
 S: Maintained
 F: include/qemu/accel.h
 F: include/sysemu/accel-ops.h
+F: include/hw/core/accel-cpu.h
 F: accel/accel-*.c
 F: accel/Makefile.objs
 F: accel/stubs/Makefile.objs
-- 
2.26.2



^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v11 4/7] i386: split cpu accelerators from cpu.c, using AccelCPUClass
  2020-12-11 10:09 [PATCH v11 0/7] i386 cleanup PART 2 Claudio Fontana
                   ` (2 preceding siblings ...)
  2020-12-11 10:09 ` [PATCH v11 3/7] accel: introduce AccelCPUClass extending CPUClass Claudio Fontana
@ 2020-12-11 10:09 ` Claudio Fontana
  2020-12-11 10:09 ` [PATCH v11 5/7] cpu: call AccelCPUClass::cpu_realizefn in cpu_exec_realizefn Claudio Fontana
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 19+ messages in thread
From: Claudio Fontana @ 2020-12-11 10:09 UTC (permalink / raw)
  To: Paolo Bonzini, Thomas Huth, Richard Henderson,
	Stefano Stabellini, Wenchao Wang, Roman Bolshakov,
	Sunil Muthuswamy, Philippe Mathieu-Daudé
  Cc: Laurent Vivier, Peter Maydell, Eduardo Habkost, Paul Durrant,
	Jason Wang, Marcelo Tosatti, qemu-devel, Peter Xu,
	Dario Faggioli, Cameron Esfahani, haxm-team, Claudio Fontana,
	Anthony Perard, Bruce Rogers, Olaf Hering, Emilio G . Cota,
	Colin Xu

i386 is the first user of AccelCPUClass, allowing to split
cpu.c into:

cpu.c            cpuid and common x86 cpu functionality
host-cpu.c       host x86 cpu functions and "host" cpu type
kvm/kvm-cpu.c    KVM x86 AccelCPUClass
hvf/hvf-cpu.c    HVF x86 AccelCPUClass
tcg/tcg-cpu.c    TCG x86 AccelCPUClass

Signed-off-by: Claudio Fontana <cfontana@suse.de>
---
 target/i386/cpu.h           |  20 +-
 target/i386/host-cpu.h      |  19 ++
 target/i386/kvm/kvm-cpu.h   |  41 ++++
 target/i386/tcg/tcg-cpu.h   |  15 --
 hw/i386/pc_piix.c           |   1 +
 target/i386/cpu.c           | 386 ++++--------------------------------
 target/i386/host-cpu.c      | 198 ++++++++++++++++++
 target/i386/hvf/hvf-cpu.c   |  65 ++++++
 target/i386/kvm/kvm-cpu.c   | 148 ++++++++++++++
 target/i386/kvm/kvm.c       |   3 +-
 target/i386/tcg/tcg-cpu.c   | 116 ++++++++++-
 MAINTAINERS                 |   2 +-
 target/i386/hvf/meson.build |   1 +
 target/i386/kvm/meson.build |   7 +-
 target/i386/meson.build     |   6 +-
 15 files changed, 644 insertions(+), 384 deletions(-)
 create mode 100644 target/i386/host-cpu.h
 create mode 100644 target/i386/kvm/kvm-cpu.h
 delete mode 100644 target/i386/tcg/tcg-cpu.h
 create mode 100644 target/i386/host-cpu.c
 create mode 100644 target/i386/hvf/hvf-cpu.c
 create mode 100644 target/i386/kvm/kvm-cpu.c

diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index a0d64613dc..b3e39fc631 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -1905,13 +1905,20 @@ int cpu_x86_signal_handler(int host_signum, void *pinfo,
                            void *puc);
 
 /* cpu.c */
+void x86_cpu_vendor_words2str(char *dst, uint32_t vendor1,
+                              uint32_t vendor2, uint32_t vendor3);
+typedef struct PropValue {
+    const char *prop, *value;
+} PropValue;
+void x86_cpu_apply_props(X86CPU *cpu, PropValue *props);
+
+/* cpu.c other functions (cpuid) */
 void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
                    uint32_t *eax, uint32_t *ebx,
                    uint32_t *ecx, uint32_t *edx);
 void cpu_clear_apic_feature(CPUX86State *env);
 void host_cpuid(uint32_t function, uint32_t count,
                 uint32_t *eax, uint32_t *ebx, uint32_t *ecx, uint32_t *edx);
-void host_vendor_fms(char *vendor, int *family, int *model, int *stepping);
 
 /* helper.c */
 void x86_cpu_set_a20(X86CPU *cpu, int a20_state);
@@ -2111,17 +2118,6 @@ void cpu_report_tpr_access(CPUX86State *env, TPRAccess access);
 void apic_handle_tpr_access_report(DeviceState *d, target_ulong ip,
                                    TPRAccess access);
 
-
-/* Change the value of a KVM-specific default
- *
- * If value is NULL, no default will be set and the original
- * value from the CPU model table will be kept.
- *
- * It is valid to call this function only for properties that
- * are already present in the kvm_default_props table.
- */
-void x86_cpu_change_kvm_default(const char *prop, const char *value);
-
 /* Special values for X86CPUVersion: */
 
 /* Resolve to latest CPU version */
diff --git a/target/i386/host-cpu.h b/target/i386/host-cpu.h
new file mode 100644
index 0000000000..d1f2644422
--- /dev/null
+++ b/target/i386/host-cpu.h
@@ -0,0 +1,19 @@
+/*
+ * x86 host CPU type initialization and host CPU functions
+ *
+ * Copyright 2020 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 HOST_CPU_H
+#define HOST_CPU_H
+
+void host_cpu_instance_init(X86CPU *cpu);
+void host_cpu_max_instance_init(X86CPU *cpu);
+void host_cpu_realizefn(CPUState *cs, Error **errp);
+
+void host_cpu_vendor_fms(char *vendor, int *family, int *model, int *stepping);
+
+#endif /* HOST_CPU_H */
diff --git a/target/i386/kvm/kvm-cpu.h b/target/i386/kvm/kvm-cpu.h
new file mode 100644
index 0000000000..e858ca21e5
--- /dev/null
+++ b/target/i386/kvm/kvm-cpu.h
@@ -0,0 +1,41 @@
+/*
+ * i386 KVM CPU type and functions
+ *
+ *  Copyright (c) 2003 Fabrice Bellard
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef KVM_CPU_H
+#define KVM_CPU_H
+
+#ifdef CONFIG_KVM
+/*
+ * Change the value of a KVM-specific default
+ *
+ * If value is NULL, no default will be set and the original
+ * value from the CPU model table will be kept.
+ *
+ * It is valid to call this function only for properties that
+ * are already present in the kvm_default_props table.
+ */
+void x86_cpu_change_kvm_default(const char *prop, const char *value);
+
+#else /* !CONFIG_KVM */
+
+#define x86_cpu_change_kvm_default(a, b)
+
+#endif /* CONFIG_KVM */
+
+#endif /* KVM_CPU_H */
diff --git a/target/i386/tcg/tcg-cpu.h b/target/i386/tcg/tcg-cpu.h
deleted file mode 100644
index 81f02e562e..0000000000
--- a/target/i386/tcg/tcg-cpu.h
+++ /dev/null
@@ -1,15 +0,0 @@
-/*
- * i386 TCG CPU class initialization
- *
- * Copyright 2020 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_CPU_H
-#define TCG_CPU_H
-
-void tcg_cpu_common_class_init(CPUClass *cc);
-
-#endif /* TCG_CPU_H */
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 6188c3e97e..3abcb425bb 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -64,6 +64,7 @@
 #include "hw/hyperv/vmbus-bridge.h"
 #include "hw/mem/nvdimm.h"
 #include "hw/i386/acpi-build.h"
+#include "kvm/kvm-cpu.h"
 
 #define MAX_IDE_BUS 2
 
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index f0aa6d1841..5414523651 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -22,37 +22,24 @@
 #include "qemu/cutils.h"
 #include "qemu/bitops.h"
 #include "qemu/qemu-print.h"
-
 #include "cpu.h"
-#include "tcg/tcg-cpu.h"
 #include "tcg/helper-tcg.h"
 #include "exec/exec-all.h"
 #include "sysemu/kvm.h"
 #include "sysemu/reset.h"
 #include "sysemu/hvf.h"
-#include "sysemu/cpus.h"
+#include "hw/core/accel-cpu.h"
 #include "sysemu/xen.h"
 #include "kvm/kvm_i386.h"
 #include "sev_i386.h"
-
-#include "qemu/error-report.h"
 #include "qemu/module.h"
-#include "qemu/option.h"
-#include "qemu/config-file.h"
-#include "qapi/error.h"
 #include "qapi/qapi-visit-machine.h"
 #include "qapi/qapi-visit-run-state.h"
 #include "qapi/qmp/qdict.h"
 #include "qapi/qmp/qerror.h"
-#include "qapi/visitor.h"
 #include "qom/qom-qobject.h"
-#include "sysemu/arch_init.h"
 #include "qapi/qapi-commands-machine-target.h"
-
 #include "standard-headers/asm-x86/kvm_para.h"
-
-#include "sysemu/sysemu.h"
-#include "sysemu/tcg.h"
 #include "hw/qdev-properties.h"
 #include "hw/i386/topology.h"
 #ifndef CONFIG_USER_ONLY
@@ -594,8 +581,8 @@ static CPUCacheInfo legacy_l3_cache = {
 #define INTEL_PT_CYCLE_BITMAP    0x1fff         /* Support 0,2^(0~11) */
 #define INTEL_PT_PSB_BITMAP      (0x003f << 16) /* Support 2K,4K,8K,16K,32K,64K */
 
-static void x86_cpu_vendor_words2str(char *dst, uint32_t vendor1,
-                                     uint32_t vendor2, uint32_t vendor3)
+void x86_cpu_vendor_words2str(char *dst, uint32_t vendor1,
+                              uint32_t vendor2, uint32_t vendor3)
 {
     int i;
     for (i = 0; i < 4; i++) {
@@ -1563,25 +1550,6 @@ void host_cpuid(uint32_t function, uint32_t count,
         *edx = vec[3];
 }
 
-void host_vendor_fms(char *vendor, int *family, int *model, int *stepping)
-{
-    uint32_t eax, ebx, ecx, edx;
-
-    host_cpuid(0x0, 0, &eax, &ebx, &ecx, &edx);
-    x86_cpu_vendor_words2str(vendor, ebx, edx, ecx);
-
-    host_cpuid(0x1, 0, &eax, &ebx, &ecx, &edx);
-    if (family) {
-        *family = ((eax >> 8) & 0x0F) + ((eax >> 20) & 0xFF);
-    }
-    if (model) {
-        *model = ((eax >> 4) & 0x0F) | ((eax & 0xF0000) >> 12);
-    }
-    if (stepping) {
-        *stepping = eax & 0x0F;
-    }
-}
-
 /* CPU class name definitions: */
 
 /* Return type name for a given CPU model name
@@ -1606,10 +1574,6 @@ static char *x86_cpu_class_get_model_name(X86CPUClass *cc)
                      strlen(class_name) - strlen(X86_CPU_TYPE_SUFFIX));
 }
 
-typedef struct PropValue {
-    const char *prop, *value;
-} PropValue;
-
 typedef struct X86CPUVersionDefinition {
     X86CPUVersion version;
     const char *alias;
@@ -4106,31 +4070,6 @@ static X86CPUDefinition builtin_x86_defs[] = {
     },
 };
 
-/* KVM-specific features that are automatically added/removed
- * from all CPU models when KVM is enabled.
- */
-static PropValue kvm_default_props[] = {
-    { "kvmclock", "on" },
-    { "kvm-nopiodelay", "on" },
-    { "kvm-asyncpf", "on" },
-    { "kvm-steal-time", "on" },
-    { "kvm-pv-eoi", "on" },
-    { "kvmclock-stable-bit", "on" },
-    { "x2apic", "on" },
-    { "acpi", "off" },
-    { "monitor", "off" },
-    { "svm", "off" },
-    { NULL, NULL },
-};
-
-/* TCG-specific defaults that override all CPU models when using TCG
- */
-static PropValue tcg_default_props[] = {
-    { "vme", "off" },
-    { NULL, NULL },
-};
-
-
 /*
  * We resolve CPU model aliases using -v1 when using "-machine
  * none", but this is just for compatibility while libvirt isn't
@@ -4172,61 +4111,6 @@ static X86CPUVersion x86_cpu_model_resolve_version(const X86CPUModel *model)
     return v;
 }
 
-void x86_cpu_change_kvm_default(const char *prop, const char *value)
-{
-    PropValue *pv;
-    for (pv = kvm_default_props; pv->prop; pv++) {
-        if (!strcmp(pv->prop, prop)) {
-            pv->value = value;
-            break;
-        }
-    }
-
-    /* It is valid to call this function only for properties that
-     * are already present in the kvm_default_props table.
-     */
-    assert(pv->prop);
-}
-
-static bool lmce_supported(void)
-{
-    uint64_t mce_cap = 0;
-
-#ifdef CONFIG_KVM
-    if (kvm_ioctl(kvm_state, KVM_X86_GET_MCE_CAP_SUPPORTED, &mce_cap) < 0) {
-        return false;
-    }
-#endif
-
-    return !!(mce_cap & MCG_LMCE_P);
-}
-
-#define CPUID_MODEL_ID_SZ 48
-
-/**
- * cpu_x86_fill_model_id:
- * Get CPUID model ID string from host CPU.
- *
- * @str should have at least CPUID_MODEL_ID_SZ bytes
- *
- * The function does NOT add a null terminator to the string
- * automatically.
- */
-static int cpu_x86_fill_model_id(char *str)
-{
-    uint32_t eax = 0, ebx = 0, ecx = 0, edx = 0;
-    int i;
-
-    for (i = 0; i < 3; i++) {
-        host_cpuid(0x80000002 + i, 0, &eax, &ebx, &ecx, &edx);
-        memcpy(str + i * 16 +  0, &eax, 4);
-        memcpy(str + i * 16 +  4, &ebx, 4);
-        memcpy(str + i * 16 +  8, &ecx, 4);
-        memcpy(str + i * 16 + 12, &edx, 4);
-    }
-    return 0;
-}
-
 static Property max_x86_cpu_properties[] = {
     DEFINE_PROP_BOOL("migratable", X86CPU, migratable, true),
     DEFINE_PROP_BOOL("host-cache-info", X86CPU, cache_info_passthrough, false),
@@ -4249,61 +4133,25 @@ static void max_x86_cpu_class_init(ObjectClass *oc, void *data)
 static void max_x86_cpu_initfn(Object *obj)
 {
     X86CPU *cpu = X86_CPU(obj);
-    CPUX86State *env = &cpu->env;
-    KVMState *s = kvm_state;
 
     /* We can't fill the features array here because we don't know yet if
      * "migratable" is true or false.
      */
     cpu->max_features = true;
-
-    if (accel_uses_host_cpuid()) {
-        char vendor[CPUID_VENDOR_SZ + 1] = { 0 };
-        char model_id[CPUID_MODEL_ID_SZ + 1] = { 0 };
-        int family, model, stepping;
-
-        host_vendor_fms(vendor, &family, &model, &stepping);
-        cpu_x86_fill_model_id(model_id);
-
-        object_property_set_str(OBJECT(cpu), "vendor", vendor, &error_abort);
-        object_property_set_int(OBJECT(cpu), "family", family, &error_abort);
-        object_property_set_int(OBJECT(cpu), "model", model, &error_abort);
-        object_property_set_int(OBJECT(cpu), "stepping", stepping,
-                                &error_abort);
-        object_property_set_str(OBJECT(cpu), "model-id", model_id,
-                                &error_abort);
-
-        if (kvm_enabled()) {
-            env->cpuid_min_level =
-                kvm_arch_get_supported_cpuid(s, 0x0, 0, R_EAX);
-            env->cpuid_min_xlevel =
-                kvm_arch_get_supported_cpuid(s, 0x80000000, 0, R_EAX);
-            env->cpuid_min_xlevel2 =
-                kvm_arch_get_supported_cpuid(s, 0xC0000000, 0, R_EAX);
-        } else {
-            env->cpuid_min_level =
-                hvf_get_supported_cpuid(0x0, 0, R_EAX);
-            env->cpuid_min_xlevel =
-                hvf_get_supported_cpuid(0x80000000, 0, R_EAX);
-            env->cpuid_min_xlevel2 =
-                hvf_get_supported_cpuid(0xC0000000, 0, R_EAX);
-        }
-
-        if (lmce_supported()) {
-            object_property_set_bool(OBJECT(cpu), "lmce", true, &error_abort);
-        }
-    } else {
-        object_property_set_str(OBJECT(cpu), "vendor", CPUID_VENDOR_AMD,
-                                &error_abort);
-        object_property_set_int(OBJECT(cpu), "family", 6, &error_abort);
-        object_property_set_int(OBJECT(cpu), "model", 6, &error_abort);
-        object_property_set_int(OBJECT(cpu), "stepping", 3, &error_abort);
-        object_property_set_str(OBJECT(cpu), "model-id",
-                                "QEMU TCG CPU version " QEMU_HW_VERSION,
-                                &error_abort);
-    }
-
     object_property_set_bool(OBJECT(cpu), "pmu", true, &error_abort);
+
+    /*
+     * these defaults are used for TCG and all other accelerators
+     * besides KVM and HVF, which overwrite these values
+     */
+    object_property_set_str(OBJECT(cpu), "vendor", CPUID_VENDOR_AMD,
+                            &error_abort);
+    object_property_set_int(OBJECT(cpu), "family", 6, &error_abort);
+    object_property_set_int(OBJECT(cpu), "model", 6, &error_abort);
+    object_property_set_int(OBJECT(cpu), "stepping", 3, &error_abort);
+    object_property_set_str(OBJECT(cpu), "model-id",
+                            "QEMU TCG CPU version " QEMU_HW_VERSION,
+                            &error_abort);
 }
 
 static const TypeInfo max_x86_cpu_type_info = {
@@ -4313,31 +4161,6 @@ static const TypeInfo max_x86_cpu_type_info = {
     .class_init = max_x86_cpu_class_init,
 };
 
-#if defined(CONFIG_KVM) || defined(CONFIG_HVF)
-static void host_x86_cpu_class_init(ObjectClass *oc, void *data)
-{
-    X86CPUClass *xcc = X86_CPU_CLASS(oc);
-
-    xcc->host_cpuid_required = true;
-    xcc->ordering = 8;
-
-#if defined(CONFIG_KVM)
-    xcc->model_description =
-        "KVM processor with all supported host features ";
-#elif defined(CONFIG_HVF)
-    xcc->model_description =
-        "HVF processor with all supported host features ";
-#endif
-}
-
-static const TypeInfo host_x86_cpu_type_info = {
-    .name = X86_CPU_TYPE_NAME("host"),
-    .parent = X86_CPU_TYPE_NAME("max"),
-    .class_init = host_x86_cpu_class_init,
-};
-
-#endif
-
 static char *feature_word_description(FeatureWordInfo *f, uint32_t bit)
 {
     assert(f->type == CPUID_FEATURE_WORD || f->type == MSR_FEATURE_WORD);
@@ -5063,7 +4886,7 @@ static uint64_t x86_cpu_get_supported_feature_word(FeatureWord w,
     return r;
 }
 
-static void x86_cpu_apply_props(X86CPU *cpu, PropValue *props)
+void x86_cpu_apply_props(X86CPU *cpu, PropValue *props)
 {
     PropValue *pv;
     for (pv = props; pv->prop; pv++) {
@@ -5110,8 +4933,6 @@ static void x86_cpu_load_model(X86CPU *cpu, X86CPUModel *model)
 {
     X86CPUDefinition *def = model->cpudef;
     CPUX86State *env = &cpu->env;
-    const char *vendor;
-    char host_vendor[CPUID_VENDOR_SZ + 1];
     FeatureWord w;
 
     /*NOTE: any property set by this function should be returned by
@@ -5138,18 +4959,6 @@ static void x86_cpu_load_model(X86CPU *cpu, X86CPUModel *model)
     /* legacy-cache defaults to 'off' if CPU model provides cache info */
     cpu->legacy_cache = !def->cache_info;
 
-    /* Special cases not set in the X86CPUDefinition structs: */
-    /* TODO: in-kernel irqchip for hvf */
-    if (kvm_enabled()) {
-        if (!kvm_irqchip_in_kernel()) {
-            x86_cpu_change_kvm_default("x2apic", "off");
-        }
-
-        x86_cpu_apply_props(cpu, kvm_default_props);
-    } else if (tcg_enabled()) {
-        x86_cpu_apply_props(cpu, tcg_default_props);
-    }
-
     env->features[FEAT_1_ECX] |= CPUID_EXT_HYPERVISOR;
 
     /* sysenter isn't supported in compatibility mode on AMD,
@@ -5159,15 +4968,12 @@ static void x86_cpu_load_model(X86CPU *cpu, X86CPUModel *model)
      * KVM's sysenter/syscall emulation in compatibility mode and
      * when doing cross vendor migration
      */
-    vendor = def->vendor;
-    if (accel_uses_host_cpuid()) {
-        uint32_t  ebx = 0, ecx = 0, edx = 0;
-        host_cpuid(0, 0, NULL, &ebx, &ecx, &edx);
-        x86_cpu_vendor_words2str(host_vendor, ebx, edx, ecx);
-        vendor = host_vendor;
-    }
 
-    object_property_set_str(OBJECT(cpu), "vendor", vendor, &error_abort);
+    /*
+     * vendor property is set here but then overloaded with the
+     * host cpu vendor for KVM and HVF.
+     */
+    object_property_set_str(OBJECT(cpu), "vendor", def->vendor, &error_abort);
 
     x86_cpu_apply_version_props(cpu, model);
 
@@ -6192,53 +5998,12 @@ static void x86_cpu_apic_realize(X86CPU *cpu, Error **errp)
         apic_mmio_map_once = true;
      }
 }
-
-static void x86_cpu_machine_done(Notifier *n, void *unused)
-{
-    X86CPU *cpu = container_of(n, X86CPU, machine_done);
-    MemoryRegion *smram =
-        (MemoryRegion *) object_resolve_path("/machine/smram", NULL);
-
-    if (smram) {
-        cpu->smram = g_new(MemoryRegion, 1);
-        memory_region_init_alias(cpu->smram, OBJECT(cpu), "smram",
-                                 smram, 0, 4 * GiB);
-        memory_region_set_enabled(cpu->smram, true);
-        memory_region_add_subregion_overlap(cpu->cpu_as_root, 0, cpu->smram, 1);
-    }
-}
 #else
 static void x86_cpu_apic_realize(X86CPU *cpu, Error **errp)
 {
 }
 #endif
 
-/* Note: Only safe for use on x86(-64) hosts */
-static uint32_t x86_host_phys_bits(void)
-{
-    uint32_t eax;
-    uint32_t host_phys_bits;
-
-    host_cpuid(0x80000000, 0, &eax, NULL, NULL, NULL);
-    if (eax >= 0x80000008) {
-        host_cpuid(0x80000008, 0, &eax, NULL, NULL, NULL);
-        /* Note: According to AMD doc 25481 rev 2.34 they have a field
-         * at 23:16 that can specify a maximum physical address bits for
-         * the guest that can override this value; but I've not seen
-         * anything with that set.
-         */
-        host_phys_bits = eax & 0xff;
-    } else {
-        /* It's an odd 64 bit machine that doesn't have the leaf for
-         * physical address bits; fall back to 36 that's most older
-         * Intel.
-         */
-        host_phys_bits = 36;
-    }
-
-    return host_phys_bits;
-}
-
 static void x86_cpu_adjust_level(X86CPU *cpu, uint32_t *min, uint32_t value)
 {
     if (*min < value) {
@@ -6515,33 +6280,22 @@ static void x86_cpu_filter_features(X86CPU *cpu, bool verbose)
 static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
 {
     CPUState *cs = CPU(dev);
+    CPUClass *cc = CPU_GET_CLASS(cs);
     X86CPU *cpu = X86_CPU(dev);
     X86CPUClass *xcc = X86_CPU_GET_CLASS(dev);
     CPUX86State *env = &cpu->env;
     Error *local_err = NULL;
     static bool ht_warned;
 
-    if (xcc->host_cpuid_required) {
-        if (!accel_uses_host_cpuid()) {
-            g_autofree char *name = x86_cpu_class_get_model_name(xcc);
-            error_setg(&local_err, "CPU model '%s' requires KVM", name);
-            goto out;
-        }
+    /* The accelerator realizefn needs to be called first. */
+    if (cc->accel_cpu_interface) {
+        cc->accel_cpu_interface->cpu_realizefn(cs, errp);
     }
 
-    if (cpu->max_features && accel_uses_host_cpuid()) {
-        if (enable_cpu_pm) {
-            host_cpuid(5, 0, &cpu->mwait.eax, &cpu->mwait.ebx,
-                       &cpu->mwait.ecx, &cpu->mwait.edx);
-            env->features[FEAT_1_ECX] |= CPUID_EXT_MONITOR;
-            if (kvm_enabled() && kvm_has_waitpkg()) {
-                env->features[FEAT_7_0_ECX] |= CPUID_7_0_ECX_WAITPKG;
-            }
-        }
-        if (kvm_enabled() && cpu->ucode_rev == 0) {
-            cpu->ucode_rev = kvm_arch_get_supported_msr_feature(kvm_state,
-                                                                MSR_IA32_UCODE_REV);
-        }
+    if (xcc->host_cpuid_required && !accel_uses_host_cpuid()) {
+        g_autofree char *name = x86_cpu_class_get_model_name(xcc);
+        error_setg(&local_err, "CPU model '%s' requires KVM or HVF", name);
+        goto out;
     }
 
     if (cpu->ucode_rev == 0) {
@@ -6593,39 +6347,7 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
      * consumer AMD devices but nothing else.
      */
     if (env->features[FEAT_8000_0001_EDX] & CPUID_EXT2_LM) {
-        if (accel_uses_host_cpuid()) {
-            uint32_t host_phys_bits = x86_host_phys_bits();
-            static bool warned;
-
-            /* Print a warning if the user set it to a value that's not the
-             * host value.
-             */
-            if (cpu->phys_bits != host_phys_bits && cpu->phys_bits != 0 &&
-                !warned) {
-                warn_report("Host physical bits (%u)"
-                            " does not match phys-bits property (%u)",
-                            host_phys_bits, cpu->phys_bits);
-                warned = true;
-            }
-
-            if (cpu->host_phys_bits) {
-                /* The user asked for us to use the host physical bits */
-                cpu->phys_bits = host_phys_bits;
-                if (cpu->host_phys_bits_limit &&
-                    cpu->phys_bits > cpu->host_phys_bits_limit) {
-                    cpu->phys_bits = cpu->host_phys_bits_limit;
-                }
-            }
-
-            if (cpu->phys_bits &&
-                (cpu->phys_bits > TARGET_PHYS_ADDR_SPACE_BITS ||
-                cpu->phys_bits < 32)) {
-                error_setg(errp, "phys-bits should be between 32 and %u "
-                                 " (but is %u)",
-                                 TARGET_PHYS_ADDR_SPACE_BITS, cpu->phys_bits);
-                return;
-            }
-        } else {
+        if (!accel_uses_host_cpuid()) {
             if (cpu->phys_bits && cpu->phys_bits != TCG_PHYS_ADDR_BITS) {
                 error_setg(errp, "TCG only supports phys-bits=%u",
                                   TCG_PHYS_ADDR_BITS);
@@ -6633,8 +6355,8 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
             }
         }
         /* 0 means it was not explicitly set by the user (or by machine
-         * compat_props or by the host code above). In this case, the default
-         * is the value used by TCG (40).
+         * compat_props or by the host code in host-cpu.c).
+         * In this case, the default is the value used by TCG (40).
          */
         if (cpu->phys_bits == 0) {
             cpu->phys_bits = TCG_PHYS_ADDR_BITS;
@@ -6704,33 +6426,6 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
 
     mce_init(cpu);
 
-#ifndef CONFIG_USER_ONLY
-    if (tcg_enabled()) {
-        cpu->cpu_as_mem = g_new(MemoryRegion, 1);
-        cpu->cpu_as_root = g_new(MemoryRegion, 1);
-
-        /* Outer container... */
-        memory_region_init(cpu->cpu_as_root, OBJECT(cpu), "memory", ~0ull);
-        memory_region_set_enabled(cpu->cpu_as_root, true);
-
-        /* ... with two regions inside: normal system memory with low
-         * priority, and...
-         */
-        memory_region_init_alias(cpu->cpu_as_mem, OBJECT(cpu), "memory",
-                                 get_system_memory(), 0, ~0ull);
-        memory_region_add_subregion_overlap(cpu->cpu_as_root, 0, cpu->cpu_as_mem, 0);
-        memory_region_set_enabled(cpu->cpu_as_mem, true);
-
-        cs->num_ases = 2;
-        cpu_address_space_init(cs, 0, "cpu-memory", cs->memory);
-        cpu_address_space_init(cs, 1, "cpu-smm", cpu->cpu_as_root);
-
-        /* ... SMRAM with higher priority, linked from /machine/smram.  */
-        cpu->machine_done.notify = x86_cpu_machine_done;
-        qemu_add_machine_init_done_notifier(&cpu->machine_done);
-    }
-#endif
-
     qemu_init_vcpu(cs);
 
     /*
@@ -6936,6 +6631,8 @@ static void x86_cpu_initfn(Object *obj)
 {
     X86CPU *cpu = X86_CPU(obj);
     X86CPUClass *xcc = X86_CPU_GET_CLASS(obj);
+    CPUClass *cc = CPU_CLASS(xcc);
+
     CPUX86State *env = &cpu->env;
     FeatureWord w;
 
@@ -6992,6 +6689,11 @@ static void x86_cpu_initfn(Object *obj)
     if (xcc->model) {
         x86_cpu_load_model(cpu, xcc->model);
     }
+
+    /* if required, do the accelerator-specific cpu initialization */
+    if (cc->accel_cpu_interface) {
+        cc->accel_cpu_interface->cpu_instance_init(CPU(obj));
+    }
 }
 
 static int64_t x86_cpu_get_arch_id(CPUState *cs)
@@ -7248,11 +6950,6 @@ static void x86_cpu_common_class_init(ObjectClass *oc, void *data)
     cc->class_by_name = x86_cpu_class_by_name;
     cc->parse_features = x86_cpu_parse_featurestr;
     cc->has_work = x86_cpu_has_work;
-
-#ifdef CONFIG_TCG
-    tcg_cpu_common_class_init(cc);
-#endif /* CONFIG_TCG */
-
     cc->dump_state = x86_cpu_dump_state;
     cc->set_pc = x86_cpu_set_pc;
     cc->gdb_read_register = x86_cpu_gdb_read_register;
@@ -7357,9 +7054,6 @@ static void x86_cpu_register_types(void)
     }
     type_register_static(&max_x86_cpu_type_info);
     type_register_static(&x86_base_cpu_type_info);
-#if defined(CONFIG_KVM) || defined(CONFIG_HVF)
-    type_register_static(&host_x86_cpu_type_info);
-#endif
 }
 
 type_init(x86_cpu_register_types)
diff --git a/target/i386/host-cpu.c b/target/i386/host-cpu.c
new file mode 100644
index 0000000000..3ce2bc9a84
--- /dev/null
+++ b/target/i386/host-cpu.c
@@ -0,0 +1,198 @@
+/*
+ * x86 host CPU functions, and "host" cpu type initialization
+ *
+ * Copyright 2020 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.
+ */
+
+#include "qemu/osdep.h"
+#include "cpu.h"
+#include "host-cpu.h"
+#include "qapi/error.h"
+#include "sysemu/sysemu.h"
+
+/* Note: Only safe for use on x86(-64) hosts */
+static uint32_t host_cpu_phys_bits(void)
+{
+    uint32_t eax;
+    uint32_t host_phys_bits;
+
+    host_cpuid(0x80000000, 0, &eax, NULL, NULL, NULL);
+    if (eax >= 0x80000008) {
+        host_cpuid(0x80000008, 0, &eax, NULL, NULL, NULL);
+        /*
+         * Note: According to AMD doc 25481 rev 2.34 they have a field
+         * at 23:16 that can specify a maximum physical address bits for
+         * the guest that can override this value; but I've not seen
+         * anything with that set.
+         */
+        host_phys_bits = eax & 0xff;
+    } else {
+        /*
+         * It's an odd 64 bit machine that doesn't have the leaf for
+         * physical address bits; fall back to 36 that's most older
+         * Intel.
+         */
+        host_phys_bits = 36;
+    }
+
+    return host_phys_bits;
+}
+
+static void host_cpu_enable_cpu_pm(X86CPU *cpu)
+{
+    CPUX86State *env = &cpu->env;
+
+    host_cpuid(5, 0, &cpu->mwait.eax, &cpu->mwait.ebx,
+               &cpu->mwait.ecx, &cpu->mwait.edx);
+    env->features[FEAT_1_ECX] |= CPUID_EXT_MONITOR;
+}
+
+static uint32_t host_cpu_adjust_phys_bits(X86CPU *cpu, Error **errp)
+{
+    uint32_t host_phys_bits = host_cpu_phys_bits();
+    uint32_t phys_bits = cpu->phys_bits;
+    static bool warned;
+
+    /*
+     * Print a warning if the user set it to a value that's not the
+     * host value.
+     */
+    if (phys_bits != host_phys_bits && phys_bits != 0 &&
+        !warned) {
+        warn_report("Host physical bits (%u)"
+                    " does not match phys-bits property (%u)",
+                    host_phys_bits, phys_bits);
+        warned = true;
+    }
+
+    if (cpu->host_phys_bits) {
+        /* The user asked for us to use the host physical bits */
+        phys_bits = host_phys_bits;
+        if (cpu->host_phys_bits_limit &&
+            phys_bits > cpu->host_phys_bits_limit) {
+            phys_bits = cpu->host_phys_bits_limit;
+        }
+    }
+
+    if (phys_bits &&
+        (phys_bits > TARGET_PHYS_ADDR_SPACE_BITS ||
+         phys_bits < 32)) {
+        error_setg(errp, "phys-bits should be between 32 and %u "
+                   " (but is %u)",
+                   TARGET_PHYS_ADDR_SPACE_BITS, phys_bits);
+    }
+
+    return phys_bits;
+}
+
+void host_cpu_realizefn(CPUState *cs, Error **errp)
+{
+    X86CPU *cpu = X86_CPU(cs);
+    CPUX86State *env = &cpu->env;
+
+    if (cpu->max_features && enable_cpu_pm) {
+        host_cpu_enable_cpu_pm(cpu);
+    }
+    if (env->features[FEAT_8000_0001_EDX] & CPUID_EXT2_LM) {
+        cpu->phys_bits = host_cpu_adjust_phys_bits(cpu, errp);
+    }
+}
+
+#define CPUID_MODEL_ID_SZ 48
+/**
+ * cpu_x86_fill_model_id:
+ * Get CPUID model ID string from host CPU.
+ *
+ * @str should have at least CPUID_MODEL_ID_SZ bytes
+ *
+ * The function does NOT add a null terminator to the string
+ * automatically.
+ */
+static int host_cpu_fill_model_id(char *str)
+{
+    uint32_t eax = 0, ebx = 0, ecx = 0, edx = 0;
+    int i;
+
+    for (i = 0; i < 3; i++) {
+        host_cpuid(0x80000002 + i, 0, &eax, &ebx, &ecx, &edx);
+        memcpy(str + i * 16 +  0, &eax, 4);
+        memcpy(str + i * 16 +  4, &ebx, 4);
+        memcpy(str + i * 16 +  8, &ecx, 4);
+        memcpy(str + i * 16 + 12, &edx, 4);
+    }
+    return 0;
+}
+
+void host_cpu_vendor_fms(char *vendor, int *family, int *model, int *stepping)
+{
+    uint32_t eax, ebx, ecx, edx;
+
+    host_cpuid(0x0, 0, &eax, &ebx, &ecx, &edx);
+    x86_cpu_vendor_words2str(vendor, ebx, edx, ecx);
+
+    host_cpuid(0x1, 0, &eax, &ebx, &ecx, &edx);
+    if (family) {
+        *family = ((eax >> 8) & 0x0F) + ((eax >> 20) & 0xFF);
+    }
+    if (model) {
+        *model = ((eax >> 4) & 0x0F) | ((eax & 0xF0000) >> 12);
+    }
+    if (stepping) {
+        *stepping = eax & 0x0F;
+    }
+}
+
+void host_cpu_instance_init(X86CPU *cpu)
+{
+    uint32_t ebx = 0, ecx = 0, edx = 0;
+    char vendor[CPUID_VENDOR_SZ + 1];
+
+    host_cpuid(0, 0, NULL, &ebx, &ecx, &edx);
+    x86_cpu_vendor_words2str(vendor, ebx, edx, ecx);
+
+    object_property_set_str(OBJECT(cpu), "vendor", vendor, &error_abort);
+}
+
+void host_cpu_max_instance_init(X86CPU *cpu)
+{
+    char vendor[CPUID_VENDOR_SZ + 1] = { 0 };
+    char model_id[CPUID_MODEL_ID_SZ + 1] = { 0 };
+    int family, model, stepping;
+
+    host_cpu_vendor_fms(vendor, &family, &model, &stepping);
+    host_cpu_fill_model_id(model_id);
+
+    object_property_set_str(OBJECT(cpu), "vendor", vendor, &error_abort);
+    object_property_set_int(OBJECT(cpu), "family", family, &error_abort);
+    object_property_set_int(OBJECT(cpu), "model", model, &error_abort);
+    object_property_set_int(OBJECT(cpu), "stepping", stepping,
+                            &error_abort);
+    object_property_set_str(OBJECT(cpu), "model-id", model_id,
+                            &error_abort);
+}
+
+static void host_cpu_class_init(ObjectClass *oc, void *data)
+{
+    X86CPUClass *xcc = X86_CPU_CLASS(oc);
+
+    xcc->host_cpuid_required = true;
+    xcc->ordering = 8;
+    xcc->model_description =
+        g_strdup_printf("processor with all supported host features ");
+}
+
+static const TypeInfo host_cpu_type_info = {
+    .name = X86_CPU_TYPE_NAME("host"),
+    .parent = X86_CPU_TYPE_NAME("max"),
+    .class_init = host_cpu_class_init,
+};
+
+static void host_cpu_type_init(void)
+{
+    type_register_static(&host_cpu_type_info);
+}
+
+type_init(host_cpu_type_init);
diff --git a/target/i386/hvf/hvf-cpu.c b/target/i386/hvf/hvf-cpu.c
new file mode 100644
index 0000000000..d6579571f1
--- /dev/null
+++ b/target/i386/hvf/hvf-cpu.c
@@ -0,0 +1,65 @@
+/*
+ * x86 HVF CPU type initialization
+ *
+ * Copyright 2020 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.
+ */
+
+#include "qemu/osdep.h"
+#include "cpu.h"
+#include "host-cpu.h"
+#include "qapi/error.h"
+#include "sysemu/sysemu.h"
+#include "hw/boards.h"
+#include "sysemu/hvf.h"
+#include "hw/core/accel-cpu.h"
+
+static void hvf_cpu_max_instance_init(X86CPU *cpu)
+{
+    CPUX86State *env = &cpu->env;
+
+    host_cpu_max_instance_init(cpu);
+
+    env->cpuid_min_level =
+        hvf_get_supported_cpuid(0x0, 0, R_EAX);
+    env->cpuid_min_xlevel =
+        hvf_get_supported_cpuid(0x80000000, 0, R_EAX);
+    env->cpuid_min_xlevel2 =
+        hvf_get_supported_cpuid(0xC0000000, 0, R_EAX);
+}
+
+static void hvf_cpu_instance_init(CPUState *cs)
+{
+    X86CPU *cpu = X86_CPU(cs);
+
+    host_cpu_instance_init(cpu);
+
+    /* Special cases not set in the X86CPUDefinition structs: */
+    /* TODO: in-kernel irqchip for hvf */
+
+    if (cpu->max_features) {
+        hvf_cpu_max_instance_init(cpu);
+    }
+}
+
+static void hvf_cpu_accel_class_init(ObjectClass *oc, void *data)
+{
+    AccelCPUClass *acc = ACCEL_CPU_CLASS(oc);
+
+    acc->cpu_realizefn = host_cpu_realizefn;
+    acc->cpu_instance_init = hvf_cpu_instance_init;
+};
+static const TypeInfo hvf_cpu_accel_type_info = {
+    .name = ACCEL_CPU_NAME("hvf"),
+
+    .parent = TYPE_ACCEL_CPU,
+    .class_init = hvf_cpu_accel_class_init,
+    .abstract = true,
+};
+static void hvf_cpu_accel_register_types(void)
+{
+    type_register_static(&hvf_cpu_accel_type_info);
+}
+type_init(hvf_cpu_accel_register_types);
diff --git a/target/i386/kvm/kvm-cpu.c b/target/i386/kvm/kvm-cpu.c
new file mode 100644
index 0000000000..adc5120cf6
--- /dev/null
+++ b/target/i386/kvm/kvm-cpu.c
@@ -0,0 +1,148 @@
+/*
+ * x86 KVM CPU type initialization
+ *
+ * Copyright 2020 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.
+ */
+
+#include "qemu/osdep.h"
+#include "cpu.h"
+#include "host-cpu.h"
+#include "kvm-cpu.h"
+#include "qapi/error.h"
+#include "sysemu/sysemu.h"
+#include "hw/boards.h"
+
+#include "kvm_i386.h"
+#include "hw/core/accel-cpu.h"
+
+static void kvm_cpu_realizefn(CPUState *cs, Error **errp)
+{
+    X86CPU *cpu = X86_CPU(cs);
+    CPUX86State *env = &cpu->env;
+
+    /*
+     * The realize order is important, since x86_cpu_realize() checks if
+     * nothing else has been set by the user (or by accelerators) in
+     * cpu->ucode_rev and cpu->phys_bits.
+     *
+     * realize order:
+     * kvm_cpu -> host_cpu -> x86_cpu
+     */
+    if (cpu->max_features) {
+        if (enable_cpu_pm && kvm_has_waitpkg()) {
+            env->features[FEAT_7_0_ECX] |= CPUID_7_0_ECX_WAITPKG;
+        }
+        if (cpu->ucode_rev == 0) {
+            cpu->ucode_rev =
+                kvm_arch_get_supported_msr_feature(kvm_state,
+                                                   MSR_IA32_UCODE_REV);
+        }
+    }
+    host_cpu_realizefn(cs, errp);
+}
+
+/*
+ * KVM-specific features that are automatically added/removed
+ * from all CPU models when KVM is enabled.
+ */
+static PropValue kvm_default_props[] = {
+    { "kvmclock", "on" },
+    { "kvm-nopiodelay", "on" },
+    { "kvm-asyncpf", "on" },
+    { "kvm-steal-time", "on" },
+    { "kvm-pv-eoi", "on" },
+    { "kvmclock-stable-bit", "on" },
+    { "x2apic", "on" },
+    { "acpi", "off" },
+    { "monitor", "off" },
+    { "svm", "off" },
+    { NULL, NULL },
+};
+
+void x86_cpu_change_kvm_default(const char *prop, const char *value)
+{
+    PropValue *pv;
+    for (pv = kvm_default_props; pv->prop; pv++) {
+        if (!strcmp(pv->prop, prop)) {
+            pv->value = value;
+            break;
+        }
+    }
+
+    /*
+     * It is valid to call this function only for properties that
+     * are already present in the kvm_default_props table.
+     */
+    assert(pv->prop);
+}
+
+static bool lmce_supported(void)
+{
+    uint64_t mce_cap = 0;
+
+    if (kvm_ioctl(kvm_state, KVM_X86_GET_MCE_CAP_SUPPORTED, &mce_cap) < 0) {
+        return false;
+    }
+    return !!(mce_cap & MCG_LMCE_P);
+}
+
+static void kvm_cpu_max_instance_init(X86CPU *cpu)
+{
+    CPUX86State *env = &cpu->env;
+    KVMState *s = kvm_state;
+
+    host_cpu_max_instance_init(cpu);
+
+    if (lmce_supported()) {
+        object_property_set_bool(OBJECT(cpu), "lmce", true, &error_abort);
+    }
+
+    env->cpuid_min_level =
+        kvm_arch_get_supported_cpuid(s, 0x0, 0, R_EAX);
+    env->cpuid_min_xlevel =
+        kvm_arch_get_supported_cpuid(s, 0x80000000, 0, R_EAX);
+    env->cpuid_min_xlevel2 =
+        kvm_arch_get_supported_cpuid(s, 0xC0000000, 0, R_EAX);
+}
+
+static void kvm_cpu_instance_init(CPUState *cs)
+{
+    X86CPU *cpu = X86_CPU(cs);
+
+    host_cpu_instance_init(cpu);
+
+    if (!kvm_irqchip_in_kernel()) {
+        x86_cpu_change_kvm_default("x2apic", "off");
+    }
+
+    /* Special cases not set in the X86CPUDefinition structs: */
+
+    x86_cpu_apply_props(cpu, kvm_default_props);
+
+    if (cpu->max_features) {
+        kvm_cpu_max_instance_init(cpu);
+    }
+}
+
+static void kvm_cpu_accel_class_init(ObjectClass *oc, void *data)
+{
+    AccelCPUClass *acc = ACCEL_CPU_CLASS(oc);
+
+    acc->cpu_realizefn = kvm_cpu_realizefn;
+    acc->cpu_instance_init = kvm_cpu_instance_init;
+}
+static const TypeInfo kvm_cpu_accel_type_info = {
+    .name = ACCEL_CPU_NAME("kvm"),
+
+    .parent = TYPE_ACCEL_CPU,
+    .class_init = kvm_cpu_accel_class_init,
+    .abstract = true,
+};
+static void kvm_cpu_accel_register_types(void)
+{
+    type_register_static(&kvm_cpu_accel_type_info);
+}
+type_init(kvm_cpu_accel_register_types);
diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index a2934dda02..35c86fdba6 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -22,6 +22,7 @@
 #include "standard-headers/asm-x86/kvm_para.h"
 
 #include "cpu.h"
+#include "host-cpu.h"
 #include "sysemu/sysemu.h"
 #include "sysemu/hw_accel.h"
 #include "sysemu/kvm_int.h"
@@ -285,7 +286,7 @@ static bool host_tsx_broken(void)
     int family, model, stepping;\
     char vendor[CPUID_VENDOR_SZ + 1];
 
-    host_vendor_fms(vendor, &family, &model, &stepping);
+    host_cpu_vendor_fms(vendor, &family, &model, &stepping);
 
     /* Check if we are running on a Haswell host known to have broken TSX */
     return !strcmp(vendor, CPUID_VENDOR_INTEL) &&
diff --git a/target/i386/tcg/tcg-cpu.c b/target/i386/tcg/tcg-cpu.c
index d2dd521612..45262de5fb 100644
--- a/target/i386/tcg/tcg-cpu.c
+++ b/target/i386/tcg/tcg-cpu.c
@@ -19,13 +19,14 @@
 
 #include "qemu/osdep.h"
 #include "cpu.h"
-#include "tcg-cpu.h"
-#include "exec/exec-all.h"
-#include "sysemu/runstate.h"
 #include "helper-tcg.h"
+#include "qemu/accel.h"
+#include "hw/core/accel-cpu.h"
 
-#if !defined(CONFIG_USER_ONLY)
-#include "hw/i386/apic.h"
+#ifndef CONFIG_USER_ONLY
+#include "sysemu/sysemu.h"
+#include "qemu/units.h"
+#include "exec/address-spaces.h"
 #endif
 
 /* Frob eflags into and out of the CPU temporary format.  */
@@ -56,7 +57,72 @@ static void x86_cpu_synchronize_from_tb(CPUState *cs, TranslationBlock *tb)
     cpu->env.eip = tb->pc - tb->cs_base;
 }
 
-void tcg_cpu_common_class_init(CPUClass *cc)
+#ifndef CONFIG_USER_ONLY
+
+static void x86_cpu_machine_done(Notifier *n, void *unused)
+{
+    X86CPU *cpu = container_of(n, X86CPU, machine_done);
+    MemoryRegion *smram =
+        (MemoryRegion *) object_resolve_path("/machine/smram", NULL);
+
+    if (smram) {
+        cpu->smram = g_new(MemoryRegion, 1);
+        memory_region_init_alias(cpu->smram, OBJECT(cpu), "smram",
+                                 smram, 0, 4 * GiB);
+        memory_region_set_enabled(cpu->smram, true);
+        memory_region_add_subregion_overlap(cpu->cpu_as_root, 0,
+                                            cpu->smram, 1);
+    }
+}
+
+static void tcg_cpu_realizefn(CPUState *cs, Error **errp)
+{
+    X86CPU *cpu = X86_CPU(cs);
+
+    /*
+     * The realize order is important, since x86_cpu_realize() checks if
+     * nothing else has been set by the user (or by accelerators) in
+     * cpu->ucode_rev and cpu->phys_bits, and the memory regions
+     * initialized here are needed for the vcpu initialization.
+     *
+     * realize order:
+     * tcg_cpu -> host_cpu -> x86_cpu
+     */
+    cpu->cpu_as_mem = g_new(MemoryRegion, 1);
+    cpu->cpu_as_root = g_new(MemoryRegion, 1);
+
+    /* Outer container... */
+    memory_region_init(cpu->cpu_as_root, OBJECT(cpu), "memory", ~0ull);
+    memory_region_set_enabled(cpu->cpu_as_root, true);
+
+    /*
+     * ... with two regions inside: normal system memory with low
+     * priority, and...
+     */
+    memory_region_init_alias(cpu->cpu_as_mem, OBJECT(cpu), "memory",
+                             get_system_memory(), 0, ~0ull);
+    memory_region_add_subregion_overlap(cpu->cpu_as_root, 0, cpu->cpu_as_mem, 0);
+    memory_region_set_enabled(cpu->cpu_as_mem, true);
+
+    cs->num_ases = 2;
+    cpu_address_space_init(cs, 0, "cpu-memory", cs->memory);
+    cpu_address_space_init(cs, 1, "cpu-smm", cpu->cpu_as_root);
+
+    /* ... SMRAM with higher priority, linked from /machine/smram.  */
+    cpu->machine_done.notify = x86_cpu_machine_done;
+    qemu_add_machine_init_done_notifier(&cpu->machine_done);
+}
+
+#else /* CONFIG_USER_ONLY */
+
+static void tcg_cpu_realizefn(CPUState *cs, Error **errp)
+{
+}
+
+#endif /* !CONFIG_USER_ONLY */
+
+
+static void tcg_cpu_class_init(CPUClass *cc)
 {
     cc->tcg_ops.do_interrupt = x86_cpu_do_interrupt;
     cc->tcg_ops.cpu_exec_interrupt = x86_cpu_exec_interrupt;
@@ -67,5 +133,41 @@ void tcg_cpu_common_class_init(CPUClass *cc)
     cc->tcg_ops.tlb_fill = x86_cpu_tlb_fill;
 #ifndef CONFIG_USER_ONLY
     cc->tcg_ops.debug_excp_handler = breakpoint_handler;
-#endif
+#endif /* !CONFIG_USER_ONLY */
 }
+
+/*
+ * TCG-specific defaults that override all CPU models when using TCG
+ */
+static PropValue tcg_default_props[] = {
+    { "vme", "off" },
+    { NULL, NULL },
+};
+
+static void tcg_cpu_instance_init(CPUState *cs)
+{
+    X86CPU *cpu = X86_CPU(cs);
+    /* Special cases not set in the X86CPUDefinition structs: */
+    x86_cpu_apply_props(cpu, tcg_default_props);
+}
+
+static void tcg_cpu_accel_class_init(ObjectClass *oc, void *data)
+{
+    AccelCPUClass *acc = ACCEL_CPU_CLASS(oc);
+
+    acc->cpu_realizefn = tcg_cpu_realizefn;
+    acc->cpu_class_init = tcg_cpu_class_init;
+    acc->cpu_instance_init = tcg_cpu_instance_init;
+}
+static const TypeInfo tcg_cpu_accel_type_info = {
+    .name = ACCEL_CPU_NAME("tcg"),
+
+    .parent = TYPE_ACCEL_CPU,
+    .class_init = tcg_cpu_accel_class_init,
+    .abstract = true,
+};
+static void tcg_cpu_accel_register_types(void)
+{
+    type_register_static(&tcg_cpu_accel_type_info);
+}
+type_init(tcg_cpu_accel_register_types);
diff --git a/MAINTAINERS b/MAINTAINERS
index f084d73f6b..4f3f1e8b18 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -337,7 +337,7 @@ M: Paolo Bonzini <pbonzini@redhat.com>
 M: Richard Henderson <richard.henderson@linaro.org>
 M: Eduardo Habkost <ehabkost@redhat.com>
 S: Maintained
-F: target/i386/
+F: target/i386/tcg/
 F: tests/tcg/i386/
 F: tests/tcg/x86_64/
 F: hw/i386/
diff --git a/target/i386/hvf/meson.build b/target/i386/hvf/meson.build
index e9eb5a5da8..d253d5fd10 100644
--- a/target/i386/hvf/meson.build
+++ b/target/i386/hvf/meson.build
@@ -10,4 +10,5 @@ i386_softmmu_ss.add(when: [hvf, 'CONFIG_HVF'], if_true: files(
   'x86_mmu.c',
   'x86_task.c',
   'x86hvf.c',
+  'hvf-cpu.c',
 ))
diff --git a/target/i386/kvm/meson.build b/target/i386/kvm/meson.build
index 1d66559187..0a533411ca 100644
--- a/target/i386/kvm/meson.build
+++ b/target/i386/kvm/meson.build
@@ -1,3 +1,8 @@
 i386_ss.add(when: 'CONFIG_KVM', if_false: files('kvm-stub.c'))
-i386_softmmu_ss.add(when: 'CONFIG_KVM', if_true: files('kvm.c'))
+
+i386_softmmu_ss.add(when: 'CONFIG_KVM', if_true: files(
+  'kvm.c',
+  'kvm-cpu.c',
+))
+
 i386_softmmu_ss.add(when: 'CONFIG_HYPERV', if_true: files('hyperv.c'), if_false: files('hyperv-stub.c'))
diff --git a/target/i386/meson.build b/target/i386/meson.build
index c4bf20b319..fd24479590 100644
--- a/target/i386/meson.build
+++ b/target/i386/meson.build
@@ -6,7 +6,11 @@ i386_ss.add(files(
   'xsave_helper.c',
   'cpu-dump.c',
 ))
-i386_ss.add(when: 'CONFIG_SEV', if_true: files('sev.c'), if_false: files('sev-stub.c'))
+i386_ss.add(when: 'CONFIG_SEV', if_true: files('host-cpu.c', 'sev.c'), if_false: files('sev-stub.c'))
+
+# x86 cpu type
+i386_ss.add(when: 'CONFIG_KVM', if_true: files('host-cpu.c'))
+i386_ss.add(when: 'CONFIG_HVF', if_true: files('host-cpu.c'))
 
 i386_softmmu_ss = ss.source_set()
 i386_softmmu_ss.add(files(
-- 
2.26.2



^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v11 5/7] cpu: call AccelCPUClass::cpu_realizefn in cpu_exec_realizefn
  2020-12-11 10:09 [PATCH v11 0/7] i386 cleanup PART 2 Claudio Fontana
                   ` (3 preceding siblings ...)
  2020-12-11 10:09 ` [PATCH v11 4/7] i386: split cpu accelerators from cpu.c, using AccelCPUClass Claudio Fontana
@ 2020-12-11 10:09 ` Claudio Fontana
  2020-12-11 10:09 ` [PATCH v11 6/7] hw/core/cpu: call qemu_init_vcpu in cpu_common_realizefn Claudio Fontana
  2020-12-11 10:09 ` [PATCH v11 7/7] cpu: introduce cpu_accel_instance_init Claudio Fontana
  6 siblings, 0 replies; 19+ messages in thread
From: Claudio Fontana @ 2020-12-11 10:09 UTC (permalink / raw)
  To: Paolo Bonzini, Thomas Huth, Richard Henderson,
	Stefano Stabellini, Wenchao Wang, Roman Bolshakov,
	Sunil Muthuswamy, Philippe Mathieu-Daudé
  Cc: Laurent Vivier, Peter Maydell, Eduardo Habkost, Paul Durrant,
	Jason Wang, Marcelo Tosatti, qemu-devel, Peter Xu,
	Dario Faggioli, Cameron Esfahani, haxm-team, Claudio Fontana,
	Anthony Perard, Bruce Rogers, Olaf Hering, Emilio G . Cota,
	Colin Xu

move the call to the accel_cpu_interface method to the general
cpu_exec_realizefn from target/i386, so it does not need to be
called for every target explicitly as we enable more targets.

Signed-off-by: Claudio Fontana <cfontana@suse.de>
---
 cpu.c             |  5 +++++
 target/i386/cpu.c | 15 ++++-----------
 2 files changed, 9 insertions(+), 11 deletions(-)

diff --git a/cpu.c b/cpu.c
index 5cc8f181be..a59a909cfe 100644
--- a/cpu.c
+++ b/cpu.c
@@ -130,6 +130,11 @@ void cpu_exec_realizefn(CPUState *cpu, Error **errp)
 
     cpu_list_add(cpu);
 
+    if (cc->accel_cpu_interface) {
+        /* NB: errp parameter is unused currently */
+        cc->accel_cpu_interface->cpu_realizefn(cpu, errp);
+    }
+
 #ifdef CONFIG_TCG
     /* NB: errp parameter is unused currently */
     if (tcg_enabled()) {
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 5414523651..0fedf6c160 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -6280,16 +6280,16 @@ static void x86_cpu_filter_features(X86CPU *cpu, bool verbose)
 static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
 {
     CPUState *cs = CPU(dev);
-    CPUClass *cc = CPU_GET_CLASS(cs);
     X86CPU *cpu = X86_CPU(dev);
     X86CPUClass *xcc = X86_CPU_GET_CLASS(dev);
     CPUX86State *env = &cpu->env;
     Error *local_err = NULL;
     static bool ht_warned;
 
-    /* The accelerator realizefn needs to be called first. */
-    if (cc->accel_cpu_interface) {
-        cc->accel_cpu_interface->cpu_realizefn(cs, errp);
+    cpu_exec_realizefn(cs, &local_err);
+    if (local_err != NULL) {
+        error_propagate(errp, local_err);
+        return;
     }
 
     if (xcc->host_cpuid_required && !accel_uses_host_cpuid()) {
@@ -6405,13 +6405,6 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
         env->cache_info_amd.l3_cache = &legacy_l3_cache;
     }
 
-
-    cpu_exec_realizefn(cs, &local_err);
-    if (local_err != NULL) {
-        error_propagate(errp, local_err);
-        return;
-    }
-
 #ifndef CONFIG_USER_ONLY
     MachineState *ms = MACHINE(qdev_get_machine());
     qemu_register_reset(x86_cpu_machine_reset_cb, cpu);
-- 
2.26.2



^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v11 6/7] hw/core/cpu: call qemu_init_vcpu in cpu_common_realizefn
  2020-12-11 10:09 [PATCH v11 0/7] i386 cleanup PART 2 Claudio Fontana
                   ` (4 preceding siblings ...)
  2020-12-11 10:09 ` [PATCH v11 5/7] cpu: call AccelCPUClass::cpu_realizefn in cpu_exec_realizefn Claudio Fontana
@ 2020-12-11 10:09 ` Claudio Fontana
  2020-12-11 10:09 ` [PATCH v11 7/7] cpu: introduce cpu_accel_instance_init Claudio Fontana
  6 siblings, 0 replies; 19+ messages in thread
From: Claudio Fontana @ 2020-12-11 10:09 UTC (permalink / raw)
  To: Paolo Bonzini, Thomas Huth, Richard Henderson,
	Stefano Stabellini, Wenchao Wang, Roman Bolshakov,
	Sunil Muthuswamy, Philippe Mathieu-Daudé
  Cc: Laurent Vivier, Peter Maydell, Eduardo Habkost, Paul Durrant,
	Jason Wang, Marcelo Tosatti, qemu-devel, Peter Xu,
	Dario Faggioli, Cameron Esfahani, haxm-team, Claudio Fontana,
	Anthony Perard, Bruce Rogers, Olaf Hering, Emilio G . Cota,
	Colin Xu

move the call to qemu_init_vcpu inside cpu_common_realizefn,
so it does not need to be done explicitly in each target cpu.

This makes it a little bit better, but still the way realize
is done continues to be bad; ideally the cpu_list_add would
be done in common_cpu, and in this case we could avoid even
more additional calls in target/xxx/cpu.c,

but this cannot happen because target cpu code, plugins, etc
now all rely on cpu->index, since no particular order was
defined previously, so we are stuck with the freak call order
for the target cpu realizefn.

After this patch the target/xxx/cpu.c realizefn body is now:

void mycpu_realizefn(DeviceState *dev, Error **errp)
{
    /* ... */
    cpu_exec_realizefn(CPU_STATE(dev), errp);

    /* ... anything that needs done pre-qemu_vcpu_init */

    xcc->parent_realize(dev, errp); /* does qemu_vcpu_init */

    /* ... anything that needs to be done after qemu_vcpu_init */
}

Note: better do some testing for all targets for this.

Signed-off-by: Claudio Fontana <cfontana@suse.de>
---
 hw/core/cpu.c                   | 2 ++
 target/alpha/cpu.c              | 5 +----
 target/arm/cpu.c                | 4 +---
 target/avr/cpu.c                | 3 +--
 target/cris/cpu.c               | 2 --
 target/hppa/cpu.c               | 1 -
 target/i386/cpu.c               | 5 +----
 target/lm32/cpu.c               | 3 ---
 target/m68k/cpu.c               | 2 --
 target/microblaze/cpu.c         | 9 +++------
 target/mips/cpu.c               | 2 --
 target/moxie/cpu.c              | 4 +---
 target/nios2/cpu.c              | 4 +---
 target/openrisc/cpu.c           | 4 +---
 target/riscv/cpu.c              | 8 +++-----
 target/rx/cpu.c                 | 8 +++-----
 target/s390x/cpu.c              | 3 +--
 target/sh4/cpu.c                | 2 --
 target/sparc/cpu.c              | 4 +---
 target/tilegx/cpu.c             | 2 --
 target/tricore/cpu.c            | 2 --
 target/unicore32/cpu.c          | 6 +-----
 target/xtensa/cpu.c             | 2 --
 target/ppc/translate_init.c.inc | 5 ++---
 24 files changed, 23 insertions(+), 69 deletions(-)

diff --git a/hw/core/cpu.c b/hw/core/cpu.c
index 1f04aab16b..f41c009e6c 100644
--- a/hw/core/cpu.c
+++ b/hw/core/cpu.c
@@ -318,6 +318,8 @@ static void cpu_common_realizefn(DeviceState *dev, Error **errp)
     CPUState *cpu = CPU(dev);
     Object *machine = qdev_get_machine();
 
+    qemu_init_vcpu(cpu);
+
     /* qdev_get_machine() can return something that's not TYPE_MACHINE
      * if this is one of the user-only emulators; in that case there's
      * no need to check the ignore_memory_transaction_failures board flag.
diff --git a/target/alpha/cpu.c b/target/alpha/cpu.c
index 0710298e5a..97965308a4 100644
--- a/target/alpha/cpu.c
+++ b/target/alpha/cpu.c
@@ -56,18 +56,15 @@ static void alpha_cpu_disas_set_info(CPUState *cpu, disassemble_info *info)
 
 static void alpha_cpu_realizefn(DeviceState *dev, Error **errp)
 {
-    CPUState *cs = CPU(dev);
     AlphaCPUClass *acc = ALPHA_CPU_GET_CLASS(dev);
     Error *local_err = NULL;
 
-    cpu_exec_realizefn(cs, &local_err);
+    cpu_exec_realizefn(CPU(dev), &local_err);
     if (local_err != NULL) {
         error_propagate(errp, local_err);
         return;
     }
 
-    qemu_init_vcpu(cs);
-
     acc->parent_realize(dev, errp);
 }
 
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 1ba11698eb..2472247ee5 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1859,10 +1859,8 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
         }
     }
 
-    qemu_init_vcpu(cs);
-    cpu_reset(cs);
-
     acc->parent_realize(dev, errp);
+    cpu_reset(cs);
 }
 
 static ObjectClass *arm_cpu_class_by_name(const char *cpu_model)
diff --git a/target/avr/cpu.c b/target/avr/cpu.c
index 5a5ae68444..43f91f650d 100644
--- a/target/avr/cpu.c
+++ b/target/avr/cpu.c
@@ -97,10 +97,9 @@ static void avr_cpu_realizefn(DeviceState *dev, Error **errp)
         error_propagate(errp, local_err);
         return;
     }
-    qemu_init_vcpu(cs);
-    cpu_reset(cs);
 
     mcc->parent_realize(dev, errp);
+    cpu_reset(cs);
 }
 
 static void avr_cpu_set_int(void *opaque, int irq, int level)
diff --git a/target/cris/cpu.c b/target/cris/cpu.c
index b65743e8ca..42a497d9fe 100644
--- a/target/cris/cpu.c
+++ b/target/cris/cpu.c
@@ -135,8 +135,6 @@ static void cris_cpu_realizefn(DeviceState *dev, Error **errp)
     }
 
     cpu_reset(cs);
-    qemu_init_vcpu(cs);
-
     ccc->parent_realize(dev, errp);
 }
 
diff --git a/target/hppa/cpu.c b/target/hppa/cpu.c
index 0985b3661f..979714c62e 100644
--- a/target/hppa/cpu.c
+++ b/target/hppa/cpu.c
@@ -101,7 +101,6 @@ static void hppa_cpu_realizefn(DeviceState *dev, Error **errp)
         return;
     }
 
-    qemu_init_vcpu(cs);
     acc->parent_realize(dev, errp);
 
 #ifndef CONFIG_USER_ONLY
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 0fedf6c160..5615d9e8bc 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -6419,8 +6419,7 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
 
     mce_init(cpu);
 
-    qemu_init_vcpu(cs);
-
+    xcc->parent_realize(dev, &local_err);
     /*
      * Most Intel and certain AMD CPUs support hyperthreading. Even though QEMU
      * fixes this issue by adjusting CPUID_0000_0001_EBX and CPUID_8000_0008_ECX
@@ -6447,8 +6446,6 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
     }
     cpu_reset(cs);
 
-    xcc->parent_realize(dev, &local_err);
-
 out:
     if (local_err != NULL) {
         error_propagate(errp, local_err);
diff --git a/target/lm32/cpu.c b/target/lm32/cpu.c
index fb3761b749..99196c22a4 100644
--- a/target/lm32/cpu.c
+++ b/target/lm32/cpu.c
@@ -133,9 +133,6 @@ static void lm32_cpu_realizefn(DeviceState *dev, Error **errp)
     }
 
     cpu_reset(cs);
-
-    qemu_init_vcpu(cs);
-
     lcc->parent_realize(dev, errp);
 }
 
diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c
index 3f60c99865..41d3b2c11e 100644
--- a/target/m68k/cpu.c
+++ b/target/m68k/cpu.c
@@ -248,8 +248,6 @@ static void m68k_cpu_realizefn(DeviceState *dev, Error **errp)
     m68k_cpu_init_gdb(cpu);
 
     cpu_reset(cs);
-    qemu_init_vcpu(cs);
-
     mcc->parent_realize(dev, errp);
 }
 
diff --git a/target/microblaze/cpu.c b/target/microblaze/cpu.c
index 395f4a300f..6e068b2ae0 100644
--- a/target/microblaze/cpu.c
+++ b/target/microblaze/cpu.c
@@ -145,15 +145,14 @@ static void mb_disas_set_info(CPUState *cpu, disassemble_info *info)
 
 static void mb_cpu_realizefn(DeviceState *dev, Error **errp)
 {
-    CPUState *cs = CPU(dev);
     MicroBlazeCPUClass *mcc = MICROBLAZE_CPU_GET_CLASS(dev);
-    MicroBlazeCPU *cpu = MICROBLAZE_CPU(cs);
+    MicroBlazeCPU *cpu = MICROBLAZE_CPU(dev);
     uint8_t version_code = 0;
     const char *version;
     int i = 0;
     Error *local_err = NULL;
 
-    cpu_exec_realizefn(cs, &local_err);
+    cpu_exec_realizefn(CPU(dev), &local_err);
     if (local_err != NULL) {
         error_propagate(errp, local_err);
         return;
@@ -165,7 +164,7 @@ static void mb_cpu_realizefn(DeviceState *dev, Error **errp)
         return;
     }
 
-    qemu_init_vcpu(cs);
+    mcc->parent_realize(dev, errp);
 
     version = cpu->cfg.version ? cpu->cfg.version : DEFAULT_CPU_VERSION;
     for (i = 0; mb_cpu_lookup[i].name && version; i++) {
@@ -231,8 +230,6 @@ static void mb_cpu_realizefn(DeviceState *dev, Error **errp)
     cpu->cfg.mmu_tlb_access = 3;
     cpu->cfg.mmu_zones = 16;
     cpu->cfg.addr_mask = MAKE_64BIT_MASK(0, cpu->cfg.addr_size);
-
-    mcc->parent_realize(dev, errp);
 }
 
 static void mb_cpu_initfn(Object *obj)
diff --git a/target/mips/cpu.c b/target/mips/cpu.c
index e99c692e2d..87cc55f087 100644
--- a/target/mips/cpu.c
+++ b/target/mips/cpu.c
@@ -184,8 +184,6 @@ static void mips_cpu_realizefn(DeviceState *dev, Error **errp)
     cpu_mips_realize_env(&cpu->env);
 
     cpu_reset(cs);
-    qemu_init_vcpu(cs);
-
     mcc->parent_realize(dev, errp);
 }
 
diff --git a/target/moxie/cpu.c b/target/moxie/cpu.c
index 36bef4d357..4dfd2936af 100644
--- a/target/moxie/cpu.c
+++ b/target/moxie/cpu.c
@@ -66,10 +66,8 @@ static void moxie_cpu_realizefn(DeviceState *dev, Error **errp)
         return;
     }
 
-    qemu_init_vcpu(cs);
-    cpu_reset(cs);
-
     mcc->parent_realize(dev, errp);
+    cpu_reset(cs);
 }
 
 static void moxie_cpu_initfn(Object *obj)
diff --git a/target/nios2/cpu.c b/target/nios2/cpu.c
index 059eea8c94..46440d6ad8 100644
--- a/target/nios2/cpu.c
+++ b/target/nios2/cpu.c
@@ -92,10 +92,8 @@ static void nios2_cpu_realizefn(DeviceState *dev, Error **errp)
         return;
     }
 
-    qemu_init_vcpu(cs);
-    cpu_reset(cs);
-
     ncc->parent_realize(dev, errp);
+    cpu_reset(cs);
 }
 
 static bool nios2_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
diff --git a/target/openrisc/cpu.c b/target/openrisc/cpu.c
index 27105c5c09..e16b66fd14 100644
--- a/target/openrisc/cpu.c
+++ b/target/openrisc/cpu.c
@@ -77,10 +77,8 @@ static void openrisc_cpu_realizefn(DeviceState *dev, Error **errp)
         return;
     }
 
-    qemu_init_vcpu(cs);
-    cpu_reset(cs);
-
     occ->parent_realize(dev, errp);
+    cpu_reset(cs);
 }
 
 static void openrisc_cpu_initfn(Object *obj)
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index e5626862c2..6b4eadfdeb 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -341,7 +341,7 @@ static void riscv_cpu_disas_set_info(CPUState *s, disassemble_info *info)
 #endif
 }
 
-static void riscv_cpu_realize(DeviceState *dev, Error **errp)
+static void riscv_cpu_realizefn(DeviceState *dev, Error **errp)
 {
     CPUState *cs = CPU(dev);
     RISCVCPU *cpu = RISCV_CPU(dev);
@@ -486,10 +486,8 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
 
     riscv_cpu_register_gdb_regs_for_features(cs);
 
-    qemu_init_vcpu(cs);
-    cpu_reset(cs);
-
     mcc->parent_realize(dev, errp);
+    cpu_reset(cs);
 }
 
 static void riscv_cpu_init(Object *obj)
@@ -532,7 +530,7 @@ static void riscv_cpu_class_init(ObjectClass *c, void *data)
     CPUClass *cc = CPU_CLASS(c);
     DeviceClass *dc = DEVICE_CLASS(c);
 
-    device_class_set_parent_realize(dc, riscv_cpu_realize,
+    device_class_set_parent_realize(dc, riscv_cpu_realizefn,
                                     &mcc->parent_realize);
 
     device_class_set_parent_reset(dc, riscv_cpu_reset, &mcc->parent_reset);
diff --git a/target/rx/cpu.c b/target/rx/cpu.c
index dc7d1c3c57..75ee2866dc 100644
--- a/target/rx/cpu.c
+++ b/target/rx/cpu.c
@@ -105,7 +105,7 @@ static ObjectClass *rx_cpu_class_by_name(const char *cpu_model)
     return oc;
 }
 
-static void rx_cpu_realize(DeviceState *dev, Error **errp)
+static void rx_cpu_realizefn(DeviceState *dev, Error **errp)
 {
     CPUState *cs = CPU(dev);
     RXCPUClass *rcc = RX_CPU_GET_CLASS(dev);
@@ -117,10 +117,8 @@ static void rx_cpu_realize(DeviceState *dev, Error **errp)
         return;
     }
 
-    qemu_init_vcpu(cs);
-    cpu_reset(cs);
-
     rcc->parent_realize(dev, errp);
+    cpu_reset(cs);
 }
 
 static void rx_cpu_set_irq(void *opaque, int no, int request)
@@ -178,7 +176,7 @@ static void rx_cpu_class_init(ObjectClass *klass, void *data)
     CPUClass *cc = CPU_CLASS(klass);
     RXCPUClass *rcc = RX_CPU_CLASS(klass);
 
-    device_class_set_parent_realize(dc, rx_cpu_realize,
+    device_class_set_parent_realize(dc, rx_cpu_realizefn,
                                     &rcc->parent_realize);
     device_class_set_parent_reset(dc, rx_cpu_reset,
                                   &rcc->parent_reset);
diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index 86f654fd6b..d67ae110af 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -232,8 +232,8 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
     qemu_register_reset(s390_cpu_machine_reset_cb, cpu);
 #endif
     s390_cpu_gdb_init(cs);
-    qemu_init_vcpu(cs);
 
+    scc->parent_realize(dev, &err);
     /*
      * KVM requires the initial CPU reset ioctl to be executed on the target
      * CPU thread. CPU hotplug under single-threaded TCG will not work with
@@ -246,7 +246,6 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
         cpu_reset(cs);
     }
 
-    scc->parent_realize(dev, &err);
 out:
     error_propagate(errp, err);
 }
diff --git a/target/sh4/cpu.c b/target/sh4/cpu.c
index fbd5f42675..b9380cce83 100644
--- a/target/sh4/cpu.c
+++ b/target/sh4/cpu.c
@@ -185,8 +185,6 @@ static void superh_cpu_realizefn(DeviceState *dev, Error **errp)
     }
 
     cpu_reset(cs);
-    qemu_init_vcpu(cs);
-
     scc->parent_realize(dev, errp);
 }
 
diff --git a/target/sparc/cpu.c b/target/sparc/cpu.c
index 3b53ef2390..0b604372d3 100644
--- a/target/sparc/cpu.c
+++ b/target/sparc/cpu.c
@@ -738,9 +738,9 @@ static void sparc_cpu_realizefn(DeviceState *dev, Error **errp)
 {
     CPUState *cs = CPU(dev);
     SPARCCPUClass *scc = SPARC_CPU_GET_CLASS(dev);
-    Error *local_err = NULL;
     SPARCCPU *cpu = SPARC_CPU(dev);
     CPUSPARCState *env = &cpu->env;
+    Error *local_err = NULL;
 
 #if defined(CONFIG_USER_ONLY)
     if ((env->def.features & CPU_FEATURE_FLOAT)) {
@@ -768,8 +768,6 @@ static void sparc_cpu_realizefn(DeviceState *dev, Error **errp)
         return;
     }
 
-    qemu_init_vcpu(cs);
-
     scc->parent_realize(dev, errp);
 }
 
diff --git a/target/tilegx/cpu.c b/target/tilegx/cpu.c
index 7d4ead4ef1..01db5bdbcf 100644
--- a/target/tilegx/cpu.c
+++ b/target/tilegx/cpu.c
@@ -93,8 +93,6 @@ static void tilegx_cpu_realizefn(DeviceState *dev, Error **errp)
     }
 
     cpu_reset(cs);
-    qemu_init_vcpu(cs);
-
     tcc->parent_realize(dev, errp);
 }
 
diff --git a/target/tricore/cpu.c b/target/tricore/cpu.c
index 89a14f81d7..ac57239f87 100644
--- a/target/tricore/cpu.c
+++ b/target/tricore/cpu.c
@@ -93,8 +93,6 @@ static void tricore_cpu_realizefn(DeviceState *dev, Error **errp)
         set_feature(env, TRICORE_FEATURE_13);
     }
     cpu_reset(cs);
-    qemu_init_vcpu(cs);
-
     tcc->parent_realize(dev, errp);
 }
 
diff --git a/target/unicore32/cpu.c b/target/unicore32/cpu.c
index e27ffc571a..fe4ae21700 100644
--- a/target/unicore32/cpu.c
+++ b/target/unicore32/cpu.c
@@ -84,18 +84,14 @@ static void uc32_any_cpu_initfn(Object *obj)
 
 static void uc32_cpu_realizefn(DeviceState *dev, Error **errp)
 {
-    CPUState *cs = CPU(dev);
     UniCore32CPUClass *ucc = UNICORE32_CPU_GET_CLASS(dev);
     Error *local_err = NULL;
 
-    cpu_exec_realizefn(cs, &local_err);
+    cpu_exec_realizefn(CPU(dev), &local_err);
     if (local_err != NULL) {
         error_propagate(errp, local_err);
         return;
     }
-
-    qemu_init_vcpu(cs);
-
     ucc->parent_realize(dev, errp);
 }
 
diff --git a/target/xtensa/cpu.c b/target/xtensa/cpu.c
index 4b6381569f..7387d90cc6 100644
--- a/target/xtensa/cpu.c
+++ b/target/xtensa/cpu.c
@@ -153,8 +153,6 @@ static void xtensa_cpu_realizefn(DeviceState *dev, Error **errp)
 
     cs->gdb_num_regs = xcc->config->gdb_regmap.num_regs;
 
-    qemu_init_vcpu(cs);
-
     xcc->parent_realize(dev, errp);
 }
 
diff --git a/target/ppc/translate_init.c.inc b/target/ppc/translate_init.c.inc
index 3fbec30a65..6a1eefae75 100644
--- a/target/ppc/translate_init.c.inc
+++ b/target/ppc/translate_init.c.inc
@@ -10093,7 +10093,7 @@ static int ppc_fixup_cpu(PowerPCCPU *cpu)
     return 0;
 }
 
-static void ppc_cpu_realize(DeviceState *dev, Error **errp)
+static void ppc_cpu_realizefn(DeviceState *dev, Error **errp)
 {
     CPUState *cs = CPU(dev);
     PowerPCCPU *cpu = POWERPC_CPU(dev);
@@ -10143,7 +10143,6 @@ static void ppc_cpu_realize(DeviceState *dev, Error **errp)
     gdb_register_coprocessor(cs, gdb_get_spr_reg, gdb_set_spr_reg,
                              pcc->gdb_num_sprs, "power-spr.xml", 0);
 #endif
-    qemu_init_vcpu(cs);
 
     pcc->parent_realize(dev, errp);
 
@@ -10898,7 +10897,7 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data)
     CPUClass *cc = CPU_CLASS(oc);
     DeviceClass *dc = DEVICE_CLASS(oc);
 
-    device_class_set_parent_realize(dc, ppc_cpu_realize,
+    device_class_set_parent_realize(dc, ppc_cpu_realizefn,
                                     &pcc->parent_realize);
     device_class_set_parent_unrealize(dc, ppc_cpu_unrealize,
                                       &pcc->parent_unrealize);
-- 
2.26.2



^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v11 7/7] cpu: introduce cpu_accel_instance_init
  2020-12-11 10:09 [PATCH v11 0/7] i386 cleanup PART 2 Claudio Fontana
                   ` (5 preceding siblings ...)
  2020-12-11 10:09 ` [PATCH v11 6/7] hw/core/cpu: call qemu_init_vcpu in cpu_common_realizefn Claudio Fontana
@ 2020-12-11 10:09 ` Claudio Fontana
  2020-12-17 19:46   ` dangers of current NEED_CPU_H, CONFIG_SOFTMMU, CONFIG_USER_ONLY (was: [PATCH v11 7/7] cpu: introduce cpu_accel_instance_init) Claudio Fontana
  6 siblings, 1 reply; 19+ messages in thread
From: Claudio Fontana @ 2020-12-11 10:09 UTC (permalink / raw)
  To: Paolo Bonzini, Thomas Huth, Richard Henderson,
	Stefano Stabellini, Wenchao Wang, Roman Bolshakov,
	Sunil Muthuswamy, Philippe Mathieu-Daudé
  Cc: Laurent Vivier, Peter Maydell, Eduardo Habkost, Paul Durrant,
	Jason Wang, Marcelo Tosatti, qemu-devel, Peter Xu,
	Dario Faggioli, Cameron Esfahani, haxm-team, Claudio Fontana,
	Anthony Perard, Bruce Rogers, Olaf Hering, Emilio G . Cota,
	Colin Xu

centralize the calls to cpu->accel_cpu_interface

Signed-off-by: Claudio Fontana <cfontana@suse.de>
---
 include/hw/core/cpu.h | 6 ++++++
 hw/core/cpu.c         | 9 +++++++++
 target/i386/cpu.c     | 9 ++-------
 3 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index 97e1dd8279..cc05c8fc96 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -664,6 +664,12 @@ void cpu_list_remove(CPUState *cpu);
  */
 void cpu_reset(CPUState *cpu);
 
+/**
+ * cpu_accel_instance_init:
+ * @cpu: The CPU that needs to do accel-specific object initializations.
+ */
+void cpu_accel_instance_init(CPUState *cpu);
+
 /**
  * cpu_class_by_name:
  * @typename: The CPU base type.
diff --git a/hw/core/cpu.c b/hw/core/cpu.c
index f41c009e6c..873cf5e4ef 100644
--- a/hw/core/cpu.c
+++ b/hw/core/cpu.c
@@ -242,6 +242,15 @@ void cpu_reset(CPUState *cpu)
     trace_guest_cpu_reset(cpu);
 }
 
+void cpu_accel_instance_init(CPUState *cpu)
+{
+    CPUClass *cc = CPU_GET_CLASS(cpu);
+
+    if (cc->accel_cpu_interface) {
+        cc->accel_cpu_interface->cpu_instance_init(cpu);
+    }
+}
+
 static void cpu_common_reset(DeviceState *dev)
 {
     CPUState *cpu = CPU(dev);
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 5615d9e8bc..8ee39bea24 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -28,7 +28,6 @@
 #include "sysemu/kvm.h"
 #include "sysemu/reset.h"
 #include "sysemu/hvf.h"
-#include "hw/core/accel-cpu.h"
 #include "sysemu/xen.h"
 #include "kvm/kvm_i386.h"
 #include "sev_i386.h"
@@ -6621,8 +6620,6 @@ static void x86_cpu_initfn(Object *obj)
 {
     X86CPU *cpu = X86_CPU(obj);
     X86CPUClass *xcc = X86_CPU_GET_CLASS(obj);
-    CPUClass *cc = CPU_CLASS(xcc);
-
     CPUX86State *env = &cpu->env;
     FeatureWord w;
 
@@ -6680,10 +6677,8 @@ static void x86_cpu_initfn(Object *obj)
         x86_cpu_load_model(cpu, xcc->model);
     }
 
-    /* if required, do the accelerator-specific cpu initialization */
-    if (cc->accel_cpu_interface) {
-        cc->accel_cpu_interface->cpu_instance_init(CPU(obj));
-    }
+    /* if required, do accelerator-specific cpu initializations */
+    cpu_accel_instance_init(CPU(obj));
 }
 
 static int64_t x86_cpu_get_arch_id(CPUState *cs)
-- 
2.26.2



^ permalink raw reply related	[flat|nested] 19+ messages in thread

* dangers of current NEED_CPU_H, CONFIG_SOFTMMU, CONFIG_USER_ONLY (was: [PATCH v11 7/7] cpu: introduce cpu_accel_instance_init)
  2020-12-11 10:09 ` [PATCH v11 7/7] cpu: introduce cpu_accel_instance_init Claudio Fontana
@ 2020-12-17 19:46   ` Claudio Fontana
  2020-12-17 20:15     ` Peter Maydell
  0 siblings, 1 reply; 19+ messages in thread
From: Claudio Fontana @ 2020-12-17 19:46 UTC (permalink / raw)
  To: Paolo Bonzini, Richard Henderson, Philippe Mathieu-Daudé,
	Peter Maydell, Eduardo Habkost
  Cc: Marc-André Lureau, Alex Bennee, qemu-devel

Hi,

I would like to highlight the current dangerous state of NEED_CPU_H / CONFIG_SOFTMMU / CONFIG_USER_ONLY.

This patch of mine (the last in the i386 cleanup PART 2) breaks check-tcg.

The why is not obvious at all. I'll comment below it.

On 12/11/20 11:09 AM, Claudio Fontana wrote:
> centralize the calls to cpu->accel_cpu_interface
> 
> Signed-off-by: Claudio Fontana <cfontana@suse.de>
> ---
>  include/hw/core/cpu.h | 6 ++++++
>  hw/core/cpu.c         | 9 +++++++++
>  target/i386/cpu.c     | 9 ++-------
>  3 files changed, 17 insertions(+), 7 deletions(-)
> 
> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
> index 97e1dd8279..cc05c8fc96 100644
> --- a/include/hw/core/cpu.h
> +++ b/include/hw/core/cpu.h
> @@ -664,6 +664,12 @@ void cpu_list_remove(CPUState *cpu);
>   */
>  void cpu_reset(CPUState *cpu);
>  
> +/**
> + * cpu_accel_instance_init:
> + * @cpu: The CPU that needs to do accel-specific object initializations.
> + */
> +void cpu_accel_instance_init(CPUState *cpu);
> +
>  /**
>   * cpu_class_by_name:
>   * @typename: The CPU base type.
> diff --git a/hw/core/cpu.c b/hw/core/cpu.c
> index f41c009e6c..873cf5e4ef 100644
> --- a/hw/core/cpu.c
> +++ b/hw/core/cpu.c
> @@ -242,6 +242,15 @@ void cpu_reset(CPUState *cpu)
>      trace_guest_cpu_reset(cpu);
>  }
>  
> +void cpu_accel_instance_init(CPUState *cpu)
> +{
> +    CPUClass *cc = CPU_GET_CLASS(cpu);
> +
> +    if (cc->accel_cpu_interface) {
> +        cc->accel_cpu_interface->cpu_instance_init(cpu);
> +    }
> +}
> +
>  static void cpu_common_reset(DeviceState *dev)
>  {
>      CPUState *cpu = CPU(dev);
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 5615d9e8bc..8ee39bea24 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -28,7 +28,6 @@
>  #include "sysemu/kvm.h"
>  #include "sysemu/reset.h"
>  #include "sysemu/hvf.h"
> -#include "hw/core/accel-cpu.h"
>  #include "sysemu/xen.h"
>  #include "kvm/kvm_i386.h"
>  #include "sev_i386.h"
> @@ -6621,8 +6620,6 @@ static void x86_cpu_initfn(Object *obj)
>  {
>      X86CPU *cpu = X86_CPU(obj);
>      X86CPUClass *xcc = X86_CPU_GET_CLASS(obj);
> -    CPUClass *cc = CPU_CLASS(xcc);
> -
>      CPUX86State *env = &cpu->env;
>      FeatureWord w;
>  
> @@ -6680,10 +6677,8 @@ static void x86_cpu_initfn(Object *obj)
>          x86_cpu_load_model(cpu, xcc->model);
>      }
>  
> -    /* if required, do the accelerator-specific cpu initialization */
> -    if (cc->accel_cpu_interface) {
> -        cc->accel_cpu_interface->cpu_instance_init(CPU(obj));
> -    }
> +    /* if required, do accelerator-specific cpu initializations */
> +    cpu_accel_instance_init(CPU(obj));
>  }
>  
>  static int64_t x86_cpu_get_arch_id(CPUState *cs)
> 


Seems a harmless change right?

Just extract the use of cc->accel_cpu_interface->cpu_instance_init from x86 so it can be a useful function for all architecture targets to start using,
as we continue the refactoring past x86 into arm etc.

Instead, it breaks at least check-tcg (linux-user), if not more.



vvv spoiler below vvvv



The reason comes down in the end to the fact that we have moved code that is using CPUClass from target/i386 to hw/core/cpu.c.

If we look at hw/core/meson.build , we notice that cpu.c is in common_ss.

common_ss code does NOT see CONFIG_USER_ONLY, ever.

So our struct TcgCpuOperations in include/hw/core/cpu.h,
which contains after this series:

#ifndef CONFIG_USER_ONLY
    /**                                                                                                                                     
     * @do_transaction_failed: Callback for handling failed memory transactions                                                             
     * (ie bus faults or external aborts; not MMU faults)                                                                                   
     */
    void (*do_transaction_failed)(CPUState *cpu, hwaddr physaddr, vaddr addr,
                                  unsigned size, MMUAccessType access_type,
                                  int mmu_idx, MemTxAttrs attrs,
                                  MemTxResult response, uintptr_t retaddr);
    /**                                                                                                                                     
     * @do_unaligned_access: Callback for unaligned access handling                                                                         
     */
    void (*do_unaligned_access)(CPUState *cpu, vaddr addr,
                                MMUAccessType access_type,
                                int mmu_idx, uintptr_t retaddr);
#endif /* !CONFIG_USER_ONLY */

Now suddenly will have some of the objects (in target/...) seeing the struct as not having do_transaction_failed and do_unaligned_access,
and some of the objects (common_ss stuff) not seeing CONFIG_USER_ONLY and therefore instead _seeing_ do_transaction_failed and do_unaligned_access.

Result is a set of segfaults.

The reason we went on and tried to protect with CONFIG_USER_ONLY was to make sure that it is a compile time error to try to use these for linux-user,
but we end up making things worse.

Note that include/hw/core/cpu.h already uses CONFIG_USER_ONLY in other parts of the header file, and we might have hidden problems as a result we (or at least I) don't know about,
because code is being compiled in for linux-user which explicitly should not be compiled there.

There are multiple workarounds / fixes possible for my short term problem,
but would it not be a good idea to fix this problem at its root once and for all?

Otherwise, like I fell into this trap, others also probably will, and based on the existing cpu.h code already in mainline, indeed it seems already have.

Thoughts?

Thanks,

Claudio






^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: dangers of current NEED_CPU_H, CONFIG_SOFTMMU, CONFIG_USER_ONLY (was: [PATCH v11 7/7] cpu: introduce cpu_accel_instance_init)
  2020-12-17 19:46   ` dangers of current NEED_CPU_H, CONFIG_SOFTMMU, CONFIG_USER_ONLY (was: [PATCH v11 7/7] cpu: introduce cpu_accel_instance_init) Claudio Fontana
@ 2020-12-17 20:15     ` Peter Maydell
  2020-12-17 20:26       ` Peter Maydell
                         ` (3 more replies)
  0 siblings, 4 replies; 19+ messages in thread
From: Peter Maydell @ 2020-12-17 20:15 UTC (permalink / raw)
  To: Claudio Fontana
  Cc: Eduardo Habkost, Philippe Mathieu-Daudé,
	Richard Henderson, QEMU Developers, Marc-André Lureau,
	Paolo Bonzini, Alex Bennee

On Thu, 17 Dec 2020 at 19:46, Claudio Fontana <cfontana@suse.de> wrote:
>
> Hi,
>
> I would like to highlight the current dangerous state of NEED_CPU_H / CONFIG_SOFTMMU / CONFIG_USER_ONLY.

> So our struct TcgCpuOperations in include/hw/core/cpu.h,
> which contains after this series:
>
> #ifndef CONFIG_USER_ONLY
>     /**
>      * @do_transaction_failed: Callback for handling failed memory transactions
>      * (ie bus faults or external aborts; not MMU faults)
>      */
>     void (*do_transaction_failed)(CPUState *cpu, hwaddr physaddr, vaddr addr,
>                                   unsigned size, MMUAccessType access_type,
>                                   int mmu_idx, MemTxAttrs attrs,
>                                   MemTxResult response, uintptr_t retaddr);
>     /**
>      * @do_unaligned_access: Callback for unaligned access handling
>      */
>     void (*do_unaligned_access)(CPUState *cpu, vaddr addr,
>                                 MMUAccessType access_type,
>                                 int mmu_idx, uintptr_t retaddr);
> #endif /* !CONFIG_USER_ONLY */

Yeah, don't try to ifdef out struct fields in common-compiled code...

> Note that include/hw/core/cpu.h already uses CONFIG_USER_ONLY in other parts of the header file, and we might have hidden problems as a result we (or at least I) don't know about,
> because code is being compiled in for linux-user which explicitly should not be compiled there.

The other CONFIG_USER_ONLY checks in that file are only
ifdeffing out prototypes for functions that exist only in
the softmmu build, or providing do-nothing stubs for functions
that are softmmu only. I think they're safe.

> There are multiple workarounds / fixes possible for my short term problem,
> but would it not be a good idea to fix this problem at its root once and for all?

What's your proposal for fixing things ?

Incidentally, this should not be a problem for CONFIG_SOFTMMU,
because that is listed in include/exec/poison.h so trying to
use it in a common (not compiled-per-target) file will give you
a compile error. (So in theory we could make CONFIG_USER_ONLY
a poisoned identifier but that will require some work to
adjust places where we currently use it in "safe" ways...)

thanks
-- PMM


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: dangers of current NEED_CPU_H, CONFIG_SOFTMMU, CONFIG_USER_ONLY (was: [PATCH v11 7/7] cpu: introduce cpu_accel_instance_init)
  2020-12-17 20:15     ` Peter Maydell
@ 2020-12-17 20:26       ` Peter Maydell
  2020-12-17 21:13         ` Paolo Bonzini
  2020-12-17 20:32       ` Eduardo Habkost
                         ` (2 subsequent siblings)
  3 siblings, 1 reply; 19+ messages in thread
From: Peter Maydell @ 2020-12-17 20:26 UTC (permalink / raw)
  To: Claudio Fontana
  Cc: Eduardo Habkost, Philippe Mathieu-Daudé,
	Richard Henderson, QEMU Developers, Marc-André Lureau,
	Paolo Bonzini, Alex Bennee

On Thu, 17 Dec 2020 at 20:15, Peter Maydell <peter.maydell@linaro.org> wrote:
> (So in theory we could make CONFIG_USER_ONLY
> a poisoned identifier but that will require some work to
> adjust places where we currently use it in "safe" ways...)

Specifically, putting it in poison.h turns up these places
that would need to be made to do what they're doing in a
different way somehow:

../../hw/core/cpu.c:211:14: error: attempt to use poisoned "CONFIG_USER_ONLY"
include/disas/disas.h:27:13: error: attempt to use poisoned "CONFIG_USER_ONLY"
include/exec/address-spaces.h:24:9: error: attempt to use poisoned
"CONFIG_USER_ONLY"
include/exec/cpu-common.h:20:14: error: attempt to use poisoned
"CONFIG_USER_ONLY"
include/exec/cpu-common.h:6:9: error: attempt to use poisoned "CONFIG_USER_ONLY"
include/exec/ioport.h:43:9: error: attempt to use poisoned "CONFIG_USER_ONLY"
include/exec/memory.h:17:9: error: attempt to use poisoned "CONFIG_USER_ONLY"
include/exec/ramblock.h:22:9: error: attempt to use poisoned "CONFIG_USER_ONLY"
include/hw/core/cpu.h:1035:8: error: attempt to use poisoned "CONFIG_USER_ONLY"
include/hw/core/cpu.h:518:14: error: attempt to use poisoned "CONFIG_USER_ONLY"
include/hw/core/cpu.h:602:9: error: attempt to use poisoned "CONFIG_USER_ONLY"
include/hw/hw.h:4:8: error: attempt to use poisoned "CONFIG_USER_ONLY"
include/hw/semihosting/semihost.h:29:8: error: attempt to use poisoned
"CONFIG_USER_ONLY"
include/sysemu/accel.h:40:9: error: attempt to use poisoned "CONFIG_USER_ONLY"
include/sysemu/cpus.h:65:9: error: attempt to use poisoned "CONFIG_USER_ONLY"
include/sysemu/dma.h:34:9: error: attempt to use poisoned "CONFIG_USER_ONLY"
include/sysemu/xen.h:27:9: error: attempt to use poisoned "CONFIG_USER_ONLY"

That cpu.c one is definitely dubious given it's in a C file,
not a header.

thanks
-- PMM


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: dangers of current NEED_CPU_H, CONFIG_SOFTMMU, CONFIG_USER_ONLY (was: [PATCH v11 7/7] cpu: introduce cpu_accel_instance_init)
  2020-12-17 20:15     ` Peter Maydell
  2020-12-17 20:26       ` Peter Maydell
@ 2020-12-17 20:32       ` Eduardo Habkost
  2020-12-17 21:15       ` dangers of current NEED_CPU_H, CONFIG_SOFTMMU, CONFIG_USER_ONLY Claudio Fontana
  2020-12-17 22:45       ` Claudio Fontana
  3 siblings, 0 replies; 19+ messages in thread
From: Eduardo Habkost @ 2020-12-17 20:32 UTC (permalink / raw)
  To: Peter Maydell
  Cc: Philippe Mathieu-Daudé,
	Richard Henderson, QEMU Developers, Claudio Fontana,
	Marc-André Lureau, Paolo Bonzini, Alex Bennee

On Thu, Dec 17, 2020 at 08:15:38PM +0000, Peter Maydell wrote:
> On Thu, 17 Dec 2020 at 19:46, Claudio Fontana <cfontana@suse.de> wrote:
> >
> > Hi,
> >
> > I would like to highlight the current dangerous state of NEED_CPU_H / CONFIG_SOFTMMU / CONFIG_USER_ONLY.
> 
> > So our struct TcgCpuOperations in include/hw/core/cpu.h,
> > which contains after this series:
> >
> > #ifndef CONFIG_USER_ONLY
> >     /**
> >      * @do_transaction_failed: Callback for handling failed memory transactions
> >      * (ie bus faults or external aborts; not MMU faults)
> >      */
> >     void (*do_transaction_failed)(CPUState *cpu, hwaddr physaddr, vaddr addr,
> >                                   unsigned size, MMUAccessType access_type,
> >                                   int mmu_idx, MemTxAttrs attrs,
> >                                   MemTxResult response, uintptr_t retaddr);
> >     /**
> >      * @do_unaligned_access: Callback for unaligned access handling
> >      */
> >     void (*do_unaligned_access)(CPUState *cpu, vaddr addr,
> >                                 MMUAccessType access_type,
> >                                 int mmu_idx, uintptr_t retaddr);
> > #endif /* !CONFIG_USER_ONLY */
> 
> Yeah, don't try to ifdef out struct fields in common-compiled code...
> 
> > Note that include/hw/core/cpu.h already uses CONFIG_USER_ONLY in other parts of the header file, and we might have hidden problems as a result we (or at least I) don't know about,
> > because code is being compiled in for linux-user which explicitly should not be compiled there.
> 
> The other CONFIG_USER_ONLY checks in that file are only
> ifdeffing out prototypes for functions that exist only in
> the softmmu build, or providing do-nothing stubs for functions
> that are softmmu only. I think they're safe.
> 
> > There are multiple workarounds / fixes possible for my short term problem,
> > but would it not be a good idea to fix this problem at its root once and for all?
> 
> What's your proposal for fixing things ?
> 
> Incidentally, this should not be a problem for CONFIG_SOFTMMU,
> because that is listed in include/exec/poison.h so trying to
> use it in a common (not compiled-per-target) file will give you
> a compile error. (So in theory we could make CONFIG_USER_ONLY
> a poisoned identifier but that will require some work to
> adjust places where we currently use it in "safe" ways...)

The risk of introducing this kind of bug seem to outweigh the
benefits of existing safe usage.  Do we know how many of these
cases we have?

-- 
Eduardo



^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: dangers of current NEED_CPU_H, CONFIG_SOFTMMU, CONFIG_USER_ONLY (was: [PATCH v11 7/7] cpu: introduce cpu_accel_instance_init)
  2020-12-17 20:26       ` Peter Maydell
@ 2020-12-17 21:13         ` Paolo Bonzini
  2020-12-17 22:01           ` Eduardo Habkost
  0 siblings, 1 reply; 19+ messages in thread
From: Paolo Bonzini @ 2020-12-17 21:13 UTC (permalink / raw)
  To: Peter Maydell
  Cc: Eduardo Habkost, Philippe Mathieu-Daudé,
	Richard Henderson, QEMU Developers, Claudio Fontana,
	Marc-André Lureau, Alex Bennee

[-- Attachment #1: Type: text/plain, Size: 2103 bytes --]

I will take a look, CONFIG_USER_ONLY is definitely something that should be
poisoned.

Paolo

Il gio 17 dic 2020, 21:26 Peter Maydell <peter.maydell@linaro.org> ha
scritto:

> On Thu, 17 Dec 2020 at 20:15, Peter Maydell <peter.maydell@linaro.org>
> wrote:
> > (So in theory we could make CONFIG_USER_ONLY
> > a poisoned identifier but that will require some work to
> > adjust places where we currently use it in "safe" ways...)
>
> Specifically, putting it in poison.h turns up these places
> that would need to be made to do what they're doing in a
> different way somehow:
>
> ../../hw/core/cpu.c:211:14: error: attempt to use poisoned
> "CONFIG_USER_ONLY"
> include/disas/disas.h:27:13: error: attempt to use poisoned
> "CONFIG_USER_ONLY"
> include/exec/address-spaces.h:24:9: error: attempt to use poisoned
> "CONFIG_USER_ONLY"
> include/exec/cpu-common.h:20:14: error: attempt to use poisoned
> "CONFIG_USER_ONLY"
> include/exec/cpu-common.h:6:9: error: attempt to use poisoned
> "CONFIG_USER_ONLY"
> include/exec/ioport.h:43:9: error: attempt to use poisoned
> "CONFIG_USER_ONLY"
> include/exec/memory.h:17:9: error: attempt to use poisoned
> "CONFIG_USER_ONLY"
> include/exec/ramblock.h:22:9: error: attempt to use poisoned
> "CONFIG_USER_ONLY"
> include/hw/core/cpu.h:1035:8: error: attempt to use poisoned
> "CONFIG_USER_ONLY"
> include/hw/core/cpu.h:518:14: error: attempt to use poisoned
> "CONFIG_USER_ONLY"
> include/hw/core/cpu.h:602:9: error: attempt to use poisoned
> "CONFIG_USER_ONLY"
> include/hw/hw.h:4:8: error: attempt to use poisoned "CONFIG_USER_ONLY"
> include/hw/semihosting/semihost.h:29:8: error: attempt to use poisoned
> "CONFIG_USER_ONLY"
> include/sysemu/accel.h:40:9: error: attempt to use poisoned
> "CONFIG_USER_ONLY"
> include/sysemu/cpus.h:65:9: error: attempt to use poisoned
> "CONFIG_USER_ONLY"
> include/sysemu/dma.h:34:9: error: attempt to use poisoned
> "CONFIG_USER_ONLY"
> include/sysemu/xen.h:27:9: error: attempt to use poisoned
> "CONFIG_USER_ONLY"
>
> That cpu.c one is definitely dubious given it's in a C file,
> not a header.
>
> thanks
> -- PMM
>
>

[-- Attachment #2: Type: text/html, Size: 2780 bytes --]

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: dangers of current NEED_CPU_H, CONFIG_SOFTMMU, CONFIG_USER_ONLY
  2020-12-17 20:15     ` Peter Maydell
  2020-12-17 20:26       ` Peter Maydell
  2020-12-17 20:32       ` Eduardo Habkost
@ 2020-12-17 21:15       ` Claudio Fontana
  2020-12-17 22:45       ` Claudio Fontana
  3 siblings, 0 replies; 19+ messages in thread
From: Claudio Fontana @ 2020-12-17 21:15 UTC (permalink / raw)
  To: Peter Maydell
  Cc: Eduardo Habkost, Philippe Mathieu-Daudé,
	Richard Henderson, QEMU Developers, Marc-André Lureau,
	Paolo Bonzini, Alex Bennee

Hi Peter,

thanks for your answer,

On 12/17/20 9:15 PM, Peter Maydell wrote:
> On Thu, 17 Dec 2020 at 19:46, Claudio Fontana <cfontana@suse.de> wrote:
>>
>> Hi,
>>
>> I would like to highlight the current dangerous state of NEED_CPU_H / CONFIG_SOFTMMU / CONFIG_USER_ONLY.
> 
>> So our struct TcgCpuOperations in include/hw/core/cpu.h,
>> which contains after this series:
>>
>> #ifndef CONFIG_USER_ONLY
>>     /**
>>      * @do_transaction_failed: Callback for handling failed memory transactions
>>      * (ie bus faults or external aborts; not MMU faults)
>>      */
>>     void (*do_transaction_failed)(CPUState *cpu, hwaddr physaddr, vaddr addr,
>>                                   unsigned size, MMUAccessType access_type,
>>                                   int mmu_idx, MemTxAttrs attrs,
>>                                   MemTxResult response, uintptr_t retaddr);
>>     /**
>>      * @do_unaligned_access: Callback for unaligned access handling
>>      */
>>     void (*do_unaligned_access)(CPUState *cpu, vaddr addr,
>>                                 MMUAccessType access_type,
>>                                 int mmu_idx, uintptr_t retaddr);
>> #endif /* !CONFIG_USER_ONLY */
> 
> Yeah, don't try to ifdef out struct fields in common-compiled code...
> 
>> Note that include/hw/core/cpu.h already uses CONFIG_USER_ONLY in other parts of the header file, and we might have hidden problems as a result we (or at least I) don't know about,
>> because code is being compiled in for linux-user which explicitly should not be compiled there.
> 
> The other CONFIG_USER_ONLY checks in that file are only
> ifdeffing out prototypes for functions that exist only in
> the softmmu build, or providing do-nothing stubs for functions
> that are softmmu only. I think they're safe.

right, in cpu.h the extra prototypes do not cause immediate harm, but they lead to believe someone editing the file that CONFIG_USER_ONLY can be used and is effective in the file;
if CONFIG_USER_ONLY is ineffective, why use it?

In the same file, in other places

#ifdef NEED_CPU_H
#ifdef CONFIG_SOFTMMU

is used. Should this pattern be used instead consistently in header files? Is this guaranteed to always do the right thing, from wherever the header file is included?

Also in hw/core/cpu.c we see this:

#if !defined(CONFIG_USER_ONLY)
GuestPanicInformation *cpu_get_crash_info(CPUState *cpu)
{
    CPUClass *cc = CPU_GET_CLASS(cpu);
    GuestPanicInformation *res = NULL;

    if (cc->get_crash_info) {
        res = cc->get_crash_info(cpu);
    }
    return res;
}
#endif

If !CONFIG_USER_ONLY is always ineffective, why have it there? This code should probably then be in $(top_srcdir)/cpu.c ?

These things may be harmless by themselves, but it takes very little to make a false step,
using the existing uses as a reference, as there is no other documentation (I know of).

CONFIG_USER_ONLY is used in a few other places outside of target/, including in other header files,
as you noted in a follow up email.
Are all these uses harmless? Not sure how to determine that for sure..


> 
>> There are multiple workarounds / fixes possible for my short term problem,
>> but would it not be a good idea to fix this problem at its root once and for all?
> 
> What's your proposal for fixing things ?


I don't think I have the full picture yet, so I think the optimal solution can only be figured out together;

I will try to flail in the dark hoping to hit on something that sparks an idea.


- do we need both CONFIG_SOFTMMU and CONFIG_USER_ONLY? (I always wondered about the "ONLY" part of it, why not just CONFIG_USER?)
  Based on previous comments from Richard we might need both in the future, but I fail to detect which places are meaningful for the one or the other.


- Is the NEED_CPU_H + CONFIG_SOFTMMU check always the right thing to do? Is it always right in header files? ...


- is it possible to define very clearly where in the codebase they should be used?
  As an ignorant example from my side: only use CONFIG_USER_ONLY inside of target/,

  Making the rule "don't use this for common_ss" is very difficult to stick to in practice,
  with header files that end up being used from multiple sources, some in common_ss and some not, and it's so hidden from the day to day activities,
  one need to explicitly check.

  Instead if it's something obvious like: only in this subtree, then it is at least realistic to try to stick to it.
  

- is it possible to check the [in]correct use of CONFIG_USER_ONLY and CONFIG_SOFTMMU during compilation or with a script in scripts/ ?
  I think you answered that already, adding it to "poison". Any downside to that?
  Does it still make sense to restrict the uses more, in favor of clarity?


- once we figure out the solution, I would try to document the result of the whole experience clearly, in doc/devel/ for example?


> 
> Incidentally, this should not be a problem for CONFIG_SOFTMMU,
> because that is listed in include/exec/poison.h so trying to
> use it in a common (not compiled-per-target) file will give you
> a compile error. (So in theory we could make CONFIG_USER_ONLY
> a poisoned identifier but that will require some work to
> adjust places where we currently use it in "safe" ways...)
> 
> thanks
> -- PMM

 
Thanks!

Claudio



^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: dangers of current NEED_CPU_H, CONFIG_SOFTMMU, CONFIG_USER_ONLY (was: [PATCH v11 7/7] cpu: introduce cpu_accel_instance_init)
  2020-12-17 21:13         ` Paolo Bonzini
@ 2020-12-17 22:01           ` Eduardo Habkost
  0 siblings, 0 replies; 19+ messages in thread
From: Eduardo Habkost @ 2020-12-17 22:01 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Peter Maydell, Philippe Mathieu-Daudé,
	Richard Henderson, QEMU Developers, Claudio Fontana,
	Marc-André Lureau, Alex Bennee

On Thu, Dec 17, 2020 at 10:13:17PM +0100, Paolo Bonzini wrote:
> I will take a look, CONFIG_USER_ONLY is definitely something that should be
> poisoned.

Thanks!  I started looking at it, but I gave up when I realized
how much work it would required.  :)

In any case, feel free to reuse the 2 small commits I've just pushed to
https://gitlab.com/ehabkost/qemu/-/commits/work/poison-user-only

> 
> Paolo
> 
> Il gio 17 dic 2020, 21:26 Peter Maydell <peter.maydell@linaro.org> ha
> scritto:
> 
> > On Thu, 17 Dec 2020 at 20:15, Peter Maydell <peter.maydell@linaro.org>
> > wrote:
> > > (So in theory we could make CONFIG_USER_ONLY
> > > a poisoned identifier but that will require some work to
> > > adjust places where we currently use it in "safe" ways...)
> >
> > Specifically, putting it in poison.h turns up these places
> > that would need to be made to do what they're doing in a
> > different way somehow:
> >
> > ../../hw/core/cpu.c:211:14: error: attempt to use poisoned
> > "CONFIG_USER_ONLY"
> > include/disas/disas.h:27:13: error: attempt to use poisoned
> > "CONFIG_USER_ONLY"
> > include/exec/address-spaces.h:24:9: error: attempt to use poisoned
> > "CONFIG_USER_ONLY"
> > include/exec/cpu-common.h:20:14: error: attempt to use poisoned
> > "CONFIG_USER_ONLY"
> > include/exec/cpu-common.h:6:9: error: attempt to use poisoned
> > "CONFIG_USER_ONLY"
> > include/exec/ioport.h:43:9: error: attempt to use poisoned
> > "CONFIG_USER_ONLY"
> > include/exec/memory.h:17:9: error: attempt to use poisoned
> > "CONFIG_USER_ONLY"
> > include/exec/ramblock.h:22:9: error: attempt to use poisoned
> > "CONFIG_USER_ONLY"
> > include/hw/core/cpu.h:1035:8: error: attempt to use poisoned
> > "CONFIG_USER_ONLY"
> > include/hw/core/cpu.h:518:14: error: attempt to use poisoned
> > "CONFIG_USER_ONLY"
> > include/hw/core/cpu.h:602:9: error: attempt to use poisoned
> > "CONFIG_USER_ONLY"
> > include/hw/hw.h:4:8: error: attempt to use poisoned "CONFIG_USER_ONLY"
> > include/hw/semihosting/semihost.h:29:8: error: attempt to use poisoned
> > "CONFIG_USER_ONLY"
> > include/sysemu/accel.h:40:9: error: attempt to use poisoned
> > "CONFIG_USER_ONLY"
> > include/sysemu/cpus.h:65:9: error: attempt to use poisoned
> > "CONFIG_USER_ONLY"
> > include/sysemu/dma.h:34:9: error: attempt to use poisoned
> > "CONFIG_USER_ONLY"
> > include/sysemu/xen.h:27:9: error: attempt to use poisoned
> > "CONFIG_USER_ONLY"
> >
> > That cpu.c one is definitely dubious given it's in a C file,
> > not a header.
> >
> > thanks
> > -- PMM
> >
> >

-- 
Eduardo



^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: dangers of current NEED_CPU_H, CONFIG_SOFTMMU, CONFIG_USER_ONLY
  2020-12-17 20:15     ` Peter Maydell
                         ` (2 preceding siblings ...)
  2020-12-17 21:15       ` dangers of current NEED_CPU_H, CONFIG_SOFTMMU, CONFIG_USER_ONLY Claudio Fontana
@ 2020-12-17 22:45       ` Claudio Fontana
  2020-12-17 22:49         ` Peter Maydell
  3 siblings, 1 reply; 19+ messages in thread
From: Claudio Fontana @ 2020-12-17 22:45 UTC (permalink / raw)
  To: Peter Maydell
  Cc: Eduardo Habkost, Philippe Mathieu-Daudé,
	Richard Henderson, QEMU Developers, Marc-André Lureau,
	Paolo Bonzini, Alex Bennee

On 12/17/20 9:15 PM, Peter Maydell wrote:
> On Thu, 17 Dec 2020 at 19:46, Claudio Fontana <cfontana@suse.de> wrote:
>>
>> Hi,
>>
>> I would like to highlight the current dangerous state of NEED_CPU_H / CONFIG_SOFTMMU / CONFIG_USER_ONLY.
> 
>> So our struct TcgCpuOperations in include/hw/core/cpu.h,
>> which contains after this series:
>>
>> #ifndef CONFIG_USER_ONLY
>>     /**
>>      * @do_transaction_failed: Callback for handling failed memory transactions
>>      * (ie bus faults or external aborts; not MMU faults)
>>      */
>>     void (*do_transaction_failed)(CPUState *cpu, hwaddr physaddr, vaddr addr,
>>                                   unsigned size, MMUAccessType access_type,
>>                                   int mmu_idx, MemTxAttrs attrs,
>>                                   MemTxResult response, uintptr_t retaddr);
>>     /**
>>      * @do_unaligned_access: Callback for unaligned access handling
>>      */
>>     void (*do_unaligned_access)(CPUState *cpu, vaddr addr,
>>                                 MMUAccessType access_type,
>>                                 int mmu_idx, uintptr_t retaddr);
>> #endif /* !CONFIG_USER_ONLY */
> 
> Yeah, don't try to ifdef out struct fields in common-compiled code...

or should I? Using

#ifdef NEED_CPU_H
#ifdef CONFIG_SOFTMMU

seems to do what I expect. Is it wrong?

Thanks,

Claudio

> 
>> Note that include/hw/core/cpu.h already uses CONFIG_USER_ONLY in other parts of the header file, and we might have hidden problems as a result we (or at least I) don't know about,
>> because code is being compiled in for linux-user which explicitly should not be compiled there.
> 
> The other CONFIG_USER_ONLY checks in that file are only
> ifdeffing out prototypes for functions that exist only in
> the softmmu build, or providing do-nothing stubs for functions
> that are softmmu only. I think they're safe.
> 
>> There are multiple workarounds / fixes possible for my short term problem,
>> but would it not be a good idea to fix this problem at its root once and for all?
> 
> What's your proposal for fixing things ?
> 
> Incidentally, this should not be a problem for CONFIG_SOFTMMU,
> because that is listed in include/exec/poison.h so trying to
> use it in a common (not compiled-per-target) file will give you
> a compile error. (So in theory we could make CONFIG_USER_ONLY
> a poisoned identifier but that will require some work to
> adjust places where we currently use it in "safe" ways...)
> 
> thanks
> -- PMM
> 



^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: dangers of current NEED_CPU_H, CONFIG_SOFTMMU, CONFIG_USER_ONLY
  2020-12-17 22:45       ` Claudio Fontana
@ 2020-12-17 22:49         ` Peter Maydell
  2020-12-17 23:47           ` Claudio Fontana
  0 siblings, 1 reply; 19+ messages in thread
From: Peter Maydell @ 2020-12-17 22:49 UTC (permalink / raw)
  To: Claudio Fontana
  Cc: Eduardo Habkost, Philippe Mathieu-Daudé,
	Richard Henderson, QEMU Developers, Marc-André Lureau,
	Paolo Bonzini, Alex Bennee

On Thu, 17 Dec 2020 at 22:45, Claudio Fontana <cfontana@suse.de> wrote:
>
> On 12/17/20 9:15 PM, Peter Maydell wrote:
> > On Thu, 17 Dec 2020 at 19:46, Claudio Fontana <cfontana@suse.de> wrote:
> > Yeah, don't try to ifdef out struct fields in common-compiled code...
>
> or should I? Using
>
> #ifdef NEED_CPU_H
> #ifdef CONFIG_SOFTMMU
>
> seems to do what I expect. Is it wrong?

I think that gives you two versions of the struct:
- one seen by compiled-once files and by compiled-per-target softmmu files
- one seen by compiled-per-target user-only files

Since the user-only target executables link both compiled-per-target
and compiled-once files I think they end up with different C files
thinking the same struct has a different layout/size which seems
like it's going to cause problems.

thanks
-- PMM


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: dangers of current NEED_CPU_H, CONFIG_SOFTMMU, CONFIG_USER_ONLY
  2020-12-17 22:49         ` Peter Maydell
@ 2020-12-17 23:47           ` Claudio Fontana
  2020-12-18  0:14             ` Claudio Fontana
  0 siblings, 1 reply; 19+ messages in thread
From: Claudio Fontana @ 2020-12-17 23:47 UTC (permalink / raw)
  To: Peter Maydell
  Cc: Eduardo Habkost, Philippe Mathieu-Daudé,
	Richard Henderson, QEMU Developers, Marc-André Lureau,
	Paolo Bonzini, Alex Bennee

On 12/17/20 11:49 PM, Peter Maydell wrote:
> On Thu, 17 Dec 2020 at 22:45, Claudio Fontana <cfontana@suse.de> wrote:
>>
>> On 12/17/20 9:15 PM, Peter Maydell wrote:
>>> On Thu, 17 Dec 2020 at 19:46, Claudio Fontana <cfontana@suse.de> wrote:
>>> Yeah, don't try to ifdef out struct fields in common-compiled code...
>>
>> or should I? Using
>>
>> #ifdef NEED_CPU_H
>> #ifdef CONFIG_SOFTMMU
>>
>> seems to do what I expect. Is it wrong?
> 
> I think that gives you two versions of the struct:
> - one seen by compiled-once files and by compiled-per-target softmmu files
> - one seen by compiled-per-target user-only files
> 
> Since the user-only target executables link both compiled-per-target
> and compiled-once files I think they end up with different C files
> thinking the same struct has a different layout/size which seems
> like it's going to cause problems.
> 
> thanks
> -- PMM
> 

It doesn't with

#ifdef NEED_CPU_H
#ifdef CONFIG_SOFTMMU

just double checked the pointers from both files compiled per target and "common"; also all tests are ok.

It immediately breaks if I replace those two defines with #ifndef CONFIG_USER_ONLY and recompile.

I thought it was by design, but I guess this is just a "lucky" accident?

Ciao,

Claudio


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: dangers of current NEED_CPU_H, CONFIG_SOFTMMU, CONFIG_USER_ONLY
  2020-12-17 23:47           ` Claudio Fontana
@ 2020-12-18  0:14             ` Claudio Fontana
  0 siblings, 0 replies; 19+ messages in thread
From: Claudio Fontana @ 2020-12-18  0:14 UTC (permalink / raw)
  To: Peter Maydell
  Cc: Eduardo Habkost, Alex Bennee, Richard Henderson, QEMU Developers,
	Marc-André Lureau, Paolo Bonzini,
	Philippe Mathieu-Daudé

On 12/18/20 12:47 AM, Claudio Fontana wrote:
> On 12/17/20 11:49 PM, Peter Maydell wrote:
>> On Thu, 17 Dec 2020 at 22:45, Claudio Fontana <cfontana@suse.de> wrote:
>>>
>>> On 12/17/20 9:15 PM, Peter Maydell wrote:
>>>> On Thu, 17 Dec 2020 at 19:46, Claudio Fontana <cfontana@suse.de> wrote:
>>>> Yeah, don't try to ifdef out struct fields in common-compiled code...
>>>
>>> or should I? Using
>>>
>>> #ifdef NEED_CPU_H
>>> #ifdef CONFIG_SOFTMMU
>>>
>>> seems to do what I expect. Is it wrong?
>>
>> I think that gives you two versions of the struct:
>> - one seen by compiled-once files and by compiled-per-target softmmu files
>> - one seen by compiled-per-target user-only files
>>
>> Since the user-only target executables link both compiled-per-target
>> and compiled-once files I think they end up with different C files
>> thinking the same struct has a different layout/size which seems
>> like it's going to cause problems.
>>
>> thanks
>> -- PMM
>>
> 
> It doesn't with
> 
> #ifdef NEED_CPU_H
> #ifdef CONFIG_SOFTMMU
> 
> just double checked the pointers from both files compiled per target and "common"; also all tests are ok.
> 
> It immediately breaks if I replace those two defines with #ifndef CONFIG_USER_ONLY and recompile.
> 
> I thought it was by design, but I guess this is just a "lucky" accident?

By lucky accident I mean that CONFIG_SOFTMMU is a check in the positive instead of the #if !defined(CONFIG_USER_ONLY), so it ends up working..

> 
> Ciao,
> 
> Claudio
> 



^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2020-12-18  0:15 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-11 10:09 [PATCH v11 0/7] i386 cleanup PART 2 Claudio Fontana
2020-12-11 10:09 ` [PATCH v11 1/7] accel: extend AccelState and AccelClass to user-mode Claudio Fontana
2020-12-11 10:09 ` [PATCH v11 2/7] accel: replace struct CpusAccel with AccelOpsClass Claudio Fontana
2020-12-11 10:09 ` [PATCH v11 3/7] accel: introduce AccelCPUClass extending CPUClass Claudio Fontana
2020-12-11 10:09 ` [PATCH v11 4/7] i386: split cpu accelerators from cpu.c, using AccelCPUClass Claudio Fontana
2020-12-11 10:09 ` [PATCH v11 5/7] cpu: call AccelCPUClass::cpu_realizefn in cpu_exec_realizefn Claudio Fontana
2020-12-11 10:09 ` [PATCH v11 6/7] hw/core/cpu: call qemu_init_vcpu in cpu_common_realizefn Claudio Fontana
2020-12-11 10:09 ` [PATCH v11 7/7] cpu: introduce cpu_accel_instance_init Claudio Fontana
2020-12-17 19:46   ` dangers of current NEED_CPU_H, CONFIG_SOFTMMU, CONFIG_USER_ONLY (was: [PATCH v11 7/7] cpu: introduce cpu_accel_instance_init) Claudio Fontana
2020-12-17 20:15     ` Peter Maydell
2020-12-17 20:26       ` Peter Maydell
2020-12-17 21:13         ` Paolo Bonzini
2020-12-17 22:01           ` Eduardo Habkost
2020-12-17 20:32       ` Eduardo Habkost
2020-12-17 21:15       ` dangers of current NEED_CPU_H, CONFIG_SOFTMMU, CONFIG_USER_ONLY Claudio Fontana
2020-12-17 22:45       ` Claudio Fontana
2020-12-17 22:49         ` Peter Maydell
2020-12-17 23:47           ` Claudio Fontana
2020-12-18  0:14             ` Claudio Fontana

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.