[Qemu-devel] [PATCH v5 0/2] QOM'ify ARM CPU

[Qemu-devel] [PATCH v5 0/2] QOM'ify ARM CPU

[Andreas Färber]

Hello Peter,

Following long discussions about where this series collides with cp15 rework
and whether things should be done declarative as in eepro100.c or imperative,
I have stripped down the series to the bare minimum necessary for proceeding
with QOM'ifying the remaining targets.
This does not mean that all patches were invalid, it's an intermediate step.

Please apply to target-arm.next tree.
Any further target-arm QOM cleanups can then be based onto it.

Available at:
https://github.com/afaerber/qemu-cpu/commits/qom-cpu-arm.v5

Regards,
Andreas

Cc: Anthony Liguori <anthony@codemonkey.ws>
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Paul Brook <paul@codesourcery.com>
Cc: Andrzej Zaborowski <balrogg@gmail.com>

v4 -> v5:
* Use only one non-abstract CPU type for now, leave everything else as is.
* Drop cpu_arm_close() instead of converting it.
* Still make available cpu-qom.h through cpu.h for convenience.

v3 -> v4:
* Rebased on top of type_init() v2, object_class_get_list() v2, qom-cpu v4.

* Rename cpu-core.h to cpu-qom.h. While the term "ARM core" is quite common,
  it is less so for other architectures like s390x so use a neutral term.
* Use container_of() for CPUState -> CPU macros (suggested by Anthony).
* Rework arm_env_get_object() -> arm_env_get_cpu(), avoids some casts
  (suggested by Anthony). Also rename ENV_GET_OBJECT() -> ENV_GET_CPU().
* Sort -cpu ? list.
* Use object_class_get_list() and sort ourselves rather than using
  object_class_foreach_ordered() with callbacks (suggested by Anthony).
* Drop ARMCPUClass jtag_id since it turned out unneeded in QEMU (Peter+Andrzej).

* Drop experimental "halted" property since that should be in common code.
* Introduce "cpuid-variant" and "cpuid-revision" properties.
* Use CPU properties to drop unneeded pxa270-* classes.
* Move "/cpu" child property to integratorcp machine.

v2 -> v3:
* Rebased against qom-upstream.14 branch (and that against master).

* Rename target-arm/cpu-core.c to cpu.c now that we no longer need VPATH.
* Leave cpu-core.h as is to separate from legacy cpu.h.
* Fix -cpu alias "pxa270": handled in cpu_arm_init().
* Use proper GPL headers.

* Start removing CPUID uses in cpu_reset_model_id() and cpu.h.
* Fully convert cpu_reset_model_id() to ARMCPUInfo or per-model code.

* Experiment with adding properties ("halted").
* For testing, add a "/cpu" child property (HACK).

v1 -> v2:

* Cherry-pick Anthony's object_class_foreach() patch.

* Fix ARMCPUClass type name (arm-cpu-core -> arm-cpu).
* Add documentation.
* Rename ARMCPUDef to ARMCPUInfo.
* Use a C99-style table for initializing the classes through class_data
  instead of individual class_init functions (suggested by Anthony).
* Prepare reset callback.

* Make ENV_GET_OBJECT() use an inline function for readability.
* Invoke the CPU's reset method from cpu_reset().

* Do feature initialization via table where sensible.
* Add feature flags to ARMCPU as well (suggested by PMM for future tweaking,
  also simplifies load/save a bit) and initialize them from ARMCPUClass.
* Make feature inference work for ARMCPU as well by not passing the ARMCPUClass.
  Use function-local macros to avoid the ugliness of deferencing the features pointer.

Andreas Färber (2):
  target-arm: Drop cpu_arm_close()
  target-arm: Minimalistic CPU QOM'ification

 Makefile.target      |    1 +
 target-arm/cpu-qom.h |   68 ++++++++++++++++++++++++++++++++++++++++++++++++++
 target-arm/cpu.c     |   37 +++++++++++++++++++++++++++
 target-arm/cpu.h     |    2 +-
 target-arm/helper.c  |    9 ++----
 5 files changed, 110 insertions(+), 7 deletions(-)
 create mode 100644 target-arm/cpu-qom.h
 create mode 100644 target-arm/cpu.c

-- 
1.7.7

[Qemu-devel] [PATCH v5 1/2] target-arm: Drop cpu_arm_close()

[Andreas Färber]

It's unused, so no need to QOM'ify it later.

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 target-arm/cpu.h    |    1 -
 target-arm/helper.c |    5 -----
 2 files changed, 0 insertions(+), 6 deletions(-)

diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index 26c114b..69ef142 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -238,7 +238,6 @@ typedef struct CPUARMState {
 CPUARMState *cpu_arm_init(const char *cpu_model);
 void arm_translate_init(void);
 int cpu_arm_exec(CPUARMState *s);
-void cpu_arm_close(CPUARMState *s);
 void do_interrupt(CPUARMState *);
 void switch_mode(CPUARMState *, int);
 uint32_t do_arm_semihosting(CPUARMState *env);
diff --git a/target-arm/helper.c b/target-arm/helper.c
index 1314f23..1ce8105 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -493,11 +493,6 @@ static uint32_t cpu_arm_find_by_name(const char *name)
     return id;
 }
 
-void cpu_arm_close(CPUARMState *env)
-{
-    g_free(env);
-}
-
 static int bad_mode_switch(CPUARMState *env, int mode)
 {
     /* Return true if it is not valid for us to switch to
-- 
1.7.7

[Qemu-devel] [PATCH v5 2/2] target-arm: Minimalistic CPU QOM'ification

[Andreas Färber]

Introduce only one non-abstract type TYPE_ARM_CPU and do not touch
cp15 registers to not interfere with Peter's ongoing remodelling.
Embed CPUARMState as first (additional) field of ARMCPU.

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 Makefile.target      |    1 +
 target-arm/cpu-qom.h |   68 ++++++++++++++++++++++++++++++++++++++++++++++++++
 target-arm/cpu.c     |   37 +++++++++++++++++++++++++++
 target-arm/cpu.h     |    1 +
 target-arm/helper.c  |    4 ++-
 5 files changed, 110 insertions(+), 1 deletions(-)
 create mode 100644 target-arm/cpu-qom.h
 create mode 100644 target-arm/cpu.c

diff --git a/Makefile.target b/Makefile.target
index 63cf769..4fa03e4 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -90,6 +90,7 @@ endif
 libobj-$(TARGET_SPARC64) += vis_helper.o
 libobj-$(CONFIG_NEED_MMU) += mmu.o
 libobj-$(TARGET_ARM) += neon_helper.o iwmmxt_helper.o
+libobj-$(TARGET_ARM) += cpu.o
 ifeq ($(TARGET_BASE_ARCH), sparc)
 libobj-y += fop_helper.o cc_helper.o win_helper.o mmu_helper.o ldst_helper.o
 libobj-y += cpu_init.o
diff --git a/target-arm/cpu-qom.h b/target-arm/cpu-qom.h
new file mode 100644
index 0000000..464a29b
--- /dev/null
+++ b/target-arm/cpu-qom.h
@@ -0,0 +1,68 @@
+/*
+ * QEMU ARM CPU
+ *
+ * Copyright (c) 2012 SUSE LINUX Products GmbH
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program 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 General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see
+ * <http://www.gnu.org/licenses/gpl-2.0.html>
+ */
+#ifndef QEMU_ARM_CPU_QOM_H
+#define QEMU_ARM_CPU_QOM_H
+
+#include "qemu/cpu.h"
+#include "cpu.h"
+
+#define TYPE_ARM_CPU "arm-cpu"
+
+#define ARM_CPU_CLASS(klass) \
+    OBJECT_CLASS_CHECK(ARMCPUClass, (klass), TYPE_ARM_CPU)
+#define ARM_CPU(obj) \
+    OBJECT_CHECK(ARMCPU, (obj), TYPE_ARM_CPU)
+#define ARM_CPU_GET_CLASS(obj) \
+    OBJECT_GET_CLASS(ARMCPUClass, (obj), TYPE_ARM_CPU)
+
+/**
+ * ARMCPUClass:
+ *
+ * An ARM CPU model.
+ */
+typedef struct ARMCPUClass {
+    /*< private >*/
+    CPUClass parent_class;
+    /*< public >*/
+} ARMCPUClass;
+
+/**
+ * ARMCPU:
+ * @env: #CPUARMState
+ *
+ * An ARM CPU core.
+ */
+typedef struct ARMCPU {
+    /*< private >*/
+    CPUState parent_obj;
+    /*< public >*/
+
+    CPUARMState env;
+} ARMCPU;
+
+static inline ARMCPU *arm_env_get_cpu(CPUARMState *env)
+{
+    return ARM_CPU(container_of(env, ARMCPU, env));
+}
+
+#define ENV_GET_CPU(e) CPU(arm_env_get_cpu(e))
+
+
+#endif
diff --git a/target-arm/cpu.c b/target-arm/cpu.c
new file mode 100644
index 0000000..8019be0
--- /dev/null
+++ b/target-arm/cpu.c
@@ -0,0 +1,37 @@
+/*
+ * QEMU ARM CPU
+ *
+ * Copyright (c) 2012 SUSE LINUX Products GmbH
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program 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 General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see
+ * <http://www.gnu.org/licenses/gpl-2.0.html>
+ */
+
+#include "cpu-qom.h"
+#include "qemu-common.h"
+
+static const TypeInfo arm_cpu_type_info = {
+    .name = TYPE_ARM_CPU,
+    .parent = TYPE_CPU,
+    .instance_size = sizeof(ARMCPU),
+    .abstract = false, /* TODO Reconsider once cp15 reworked. */
+    .class_size = sizeof(ARMCPUClass),
+};
+
+static void arm_cpu_register_types(void)
+{
+    type_register_static(&arm_cpu_type_info);
+}
+
+type_init(arm_cpu_register_types)
diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index 69ef142..a68df61 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -475,6 +475,7 @@ static inline void cpu_clone_regs(CPUARMState *env, target_ulong newsp)
 #endif
 
 #include "cpu-all.h"
+#include "cpu-qom.h"
 
 /* Bit usage in the TB flags field: */
 #define ARM_TBFLAG_THUMB_SHIFT      0
diff --git a/target-arm/helper.c b/target-arm/helper.c
index 1ce8105..709de52 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -400,6 +400,7 @@ static int vfp_gdb_set_reg(CPUARMState *env, uint8_t *buf, int reg)
 
 CPUARMState *cpu_arm_init(const char *cpu_model)
 {
+    ARMCPU *cpu;
     CPUARMState *env;
     uint32_t id;
     static int inited = 0;
@@ -407,7 +408,8 @@ CPUARMState *cpu_arm_init(const char *cpu_model)
     id = cpu_arm_find_by_name(cpu_model);
     if (id == 0)
         return NULL;
-    env = g_malloc0(sizeof(CPUARMState));
+    cpu = ARM_CPU(object_new(TYPE_ARM_CPU));
+    env = &cpu->env;
     cpu_exec_init(env);
     if (tcg_enabled() && !inited) {
         inited = 1;
-- 
1.7.7

Re: [Qemu-devel] [PATCH v5 1/2] target-arm: Drop cpu_arm_close()

[Peter Maydell]

On 23 March 2012 16:24, Andreas Färber <afaerber@suse.de> wrote:
> It's unused, so no need to QOM'ify it later.
>
> Signed-off-by: Andreas Färber <afaerber@suse.de>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

Re: [Qemu-devel] [PATCH v5 2/2] target-arm: Minimalistic CPU QOM'ification

[Peter Maydell]

On 23 March 2012 16:24, Andreas Färber <afaerber@suse.de> wrote:
> Introduce only one non-abstract type TYPE_ARM_CPU and do not touch
> cp15 registers to not interfere with Peter's ongoing remodelling.
> Embed CPUARMState as first (additional) field of ARMCPU.
>
> Signed-off-by: Andreas Färber <afaerber@suse.de>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

-- PMM

Re: [Qemu-devel] [PATCH v5 0/2] QOM'ify ARM CPU

[Peter Maydell]

2012/3/23 Andreas Färber <afaerber@suse.de>:
> Following long discussions about where this series collides with cp15 rework
> and whether things should be done declarative as in eepro100.c or imperative,
> I have stripped down the series to the bare minimum necessary for proceeding
> with QOM'ifying the remaining targets.
> This does not mean that all patches were invalid, it's an intermediate step.
>
> Please apply to target-arm.next tree.
> Any further target-arm QOM cleanups can then be based onto it.

Looks OK to me. I managed to balance my wobbly stack of cp15 related
patches on top of it:
 http://git.linaro.org/gitweb?p=people/pmaydell/qemu-arm.git;a=shortlog;h=refs/heads/cp15-on-qom-on-miniqom

(the third patch which creates QOM subclasses for each cpu implementation
is doing too many things and needs splitting, and there's still plenty
of stuff TODO but it's where I currently think we ought to be heading...)

-- PMM

Re: [Qemu-devel] [PATCH v5 0/2] QOM'ify ARM CPU

[Peter Maydell]

On 23 March 2012 17:17, Peter Maydell <peter.maydell@linaro.org> wrote:
> 2012/3/23 Andreas Färber <afaerber@suse.de>:
>> Following long discussions about where this series collides with cp15 rework
>> and whether things should be done declarative as in eepro100.c or imperative,
>> I have stripped down the series to the bare minimum necessary for proceeding
>> with QOM'ifying the remaining targets.
>> This does not mean that all patches were invalid, it's an intermediate step.
>>
>> Please apply to target-arm.next tree.
>> Any further target-arm QOM cleanups can then be based onto it.
>
> Looks OK to me.

Oh, these two patches are currently the only thing in target-arm.next.
At the usual rate I would probably send out a pullreq end of next week;
feel free to get them committed earlier by other means if you need them
to unblock something else.

-- PMM

Re: [Qemu-devel] [PATCH v5 2/2] target-arm: Minimalistic CPU QOM'ification

[Andreas Färber]

Am 23.03.2012 18:13, schrieb Peter Maydell:
> On 23 March 2012 16:24, Andreas Färber <afaerber@suse.de> wrote:
>> Introduce only one non-abstract type TYPE_ARM_CPU and do not touch
>> cp15 registers to not interfere with Peter's ongoing remodelling.
>> Embed CPUARMState as first (additional) field of ARMCPU.
>>
>> Signed-off-by: Andreas Färber <afaerber@suse.de>
> 
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

Sorry, I'll need to amend this patch: CPUState::reset() must call
cpu_state_reset() in this minimal conversion.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg