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

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

[Andreas Färber]

Hello Peter,

Here's the revised mini-conversion with CPUState::reset supported again.

Please apply to target-arm.next tree.

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

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>

v5 -> v6:
* By dropping some of the patches in the series, cpu_reset() would no longer
  reset the CPUARMState. Fix this by re-adding a reset handler and calling
  cpu_state_reset() for now.

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 |   70 ++++++++++++++++++++++++++++++++++++++++++++++++++
 target-arm/cpu.c     |   59 ++++++++++++++++++++++++++++++++++++++++++
 target-arm/cpu.h     |    2 +-
 target-arm/helper.c  |    9 ++----
 5 files changed, 134 insertions(+), 7 deletions(-)
 create mode 100644 target-arm/cpu-qom.h
 create mode 100644 target-arm/cpu.c

-- 
1.7.7

[Qemu-devel] [PATCH v6 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>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 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 v6 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.

Let reset call cpu_state_reset() for now.

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 Makefile.target      |    1 +
 target-arm/cpu-qom.h |   70 ++++++++++++++++++++++++++++++++++++++++++++++++++
 target-arm/cpu.c     |   59 ++++++++++++++++++++++++++++++++++++++++++
 target-arm/cpu.h     |    1 +
 target-arm/helper.c  |    4 ++-
 5 files changed, 134 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 44b2e83..6e8b997 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -92,6 +92,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..cf107c9
--- /dev/null
+++ b/target-arm/cpu-qom.h
@@ -0,0 +1,70 @@
+/*
+ * 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 >*/
+
+    void (*parent_reset)(CPUState *cpu);
+} 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..dda6b09
--- /dev/null
+++ b/target-arm/cpu.c
@@ -0,0 +1,59 @@
+/*
+ * 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 void arm_cpu_reset(CPUState *c)
+{
+    ARMCPU *cpu = ARM_CPU(c);
+    ARMCPUClass *class = ARM_CPU_GET_CLASS(cpu);
+
+    class->parent_reset(c);
+
+    /* TODO Drop this in favor of cpu_arm_reset() calling cpu_reset()
+     *      once cpu_reset_model_id() is gone. */
+    cpu_state_reset(&cpu->env);
+}
+
+static void arm_cpu_class_init(ObjectClass *c, void *data)
+{
+    ARMCPUClass *class = ARM_CPU_CLASS(c);
+    CPUClass *cpu_class = CPU_CLASS(class);
+
+    class->parent_reset = cpu_class->reset;
+    cpu_class->reset = arm_cpu_reset;
+}
+
+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),
+    .class_init = arm_cpu_class_init,
+};
+
+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 v6 2/2] target-arm: Minimalistic CPU QOM'ification

[Peter Maydell]

On 26 March 2012 18:28, Andreas Färber <afaerber@suse.de> wrote:

> +static void arm_cpu_reset(CPUState *c)
> +{
> +    ARMCPU *cpu = ARM_CPU(c);
> +    ARMCPUClass *class = ARM_CPU_GET_CLASS(cpu);
> +
> +    class->parent_reset(c);

I thought we were avoiding 'class' in favour of 'klass'?

> +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. */

As it happens I'm planning to create the per-implementation
subclasses first and do the cp15 rework second.

-- PMM

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

[Max Filippov]

>> +static void arm_cpu_reset(CPUState *c)
>> +{
>> +    ARMCPU *cpu = ARM_CPU(c);
>> +    ARMCPUClass *class = ARM_CPU_GET_CLASS(cpu);
>> +
>> +    class->parent_reset(c);
>
> I thought we were avoiding 'class' in favour of 'klass'?

I have suggested it once and I can only say it again,
please, call it 'cpu_class'. It is the least surprising name.

-- 
Thanks.
-- Max

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

[Andreas Färber]

Am 28.03.2012 15:40, schrieb Peter Maydell:
> On 26 March 2012 18:28, Andreas Färber <afaerber@suse.de> wrote:
> 
>> +static void arm_cpu_reset(CPUState *c)
>> +{
>> +    ARMCPU *cpu = ARM_CPU(c);
>> +    ARMCPUClass *class = ARM_CPU_GET_CLASS(cpu);
>> +
>> +    class->parent_reset(c);
> 
> I thought we were avoiding 'class' in favour of 'klass'?

Max complained about that and no one argued against him, so I avoided it
in the .c file where it's not strictly necessary. It's really only
necessary in the headers. But I don't mind either way.

For me, the convention is cpu_class => CPUClass, so it would be unwise
here, thus one of class, clazz, klass.

>> +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. */
> 
> As it happens I'm planning to create the per-implementation
> subclasses first and do the cp15 rework second.

Suggest a rephrase? :)

Andreas

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

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

[Andreas Färber]

Am 28.03.2012 15:46, schrieb Max Filippov:
>>> +static void arm_cpu_reset(CPUState *c)
>>> +{
>>> +    ARMCPU *cpu = ARM_CPU(c);
>>> +    ARMCPUClass *class = ARM_CPU_GET_CLASS(cpu);
>>> +
>>> +    class->parent_reset(c);
>>
>> I thought we were avoiding 'class' in favour of 'klass'?
> 
> I have suggested it once and I can only say it again,
> please, call it 'cpu_class'. It is the least surprising name.

No, cpu_class is being used for a different class, CPUClass, when
twiddling with reset handlers of the parent class, for instance.

We could call it arm_cpu_class, but is that any better?

Andreas

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

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

[Max Filippov]

>>>> +static void arm_cpu_reset(CPUState *c)
>>>> +{
>>>> +    ARMCPU *cpu = ARM_CPU(c);
>>>> +    ARMCPUClass *class = ARM_CPU_GET_CLASS(cpu);
>>>> +
>>>> +    class->parent_reset(c);
>>>
>>> I thought we were avoiding 'class' in favour of 'klass'?
>>
>> I have suggested it once and I can only say it again,
>> please, call it 'cpu_class'. It is the least surprising name.
>
> No, cpu_class is being used for a different class, CPUClass, when
> twiddling with reset handlers of the parent class, for instance.
>
> We could call it arm_cpu_class, but is that any better?

There's no other class in this context, so why more specific name than
would be enough?
It's only a matter of long enough suffix, isn't it?

-- 
Thanks.
-- Max

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

[Andreas Färber]

Am 28.03.2012 16:00, schrieb Max Filippov:
>>>>> +static void arm_cpu_reset(CPUState *c)
>>>>> +{
>>>>> +    ARMCPU *cpu = ARM_CPU(c);
>>>>> +    ARMCPUClass *class = ARM_CPU_GET_CLASS(cpu);
>>>>> +
>>>>> +    class->parent_reset(c);
>>>>
>>>> I thought we were avoiding 'class' in favour of 'klass'?
>>>
>>> I have suggested it once and I can only say it again,
>>> please, call it 'cpu_class'. It is the least surprising name.
>>
>> No, cpu_class is being used for a different class, CPUClass, when
>> twiddling with reset handlers of the parent class, for instance.
>>
>> We could call it arm_cpu_class, but is that any better?
> 
> There's no other class in this context, so why more specific name than
> would be enough?
> It's only a matter of long enough suffix, isn't it?

My point was that using cpu_class for two very different things is not
"least surprising" when reading patches containing minimal context. You
don't always see the declaration, so I'd like to keep it consistent
across functions.

Andreas

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

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

[Peter Maydell]

On 26 March 2012 18:28, Andreas Färber <afaerber@suse.de> wrote:
> +static void arm_cpu_reset(CPUState *c)
> +{
> +    ARMCPU *cpu = ARM_CPU(c);
> +    ARMCPUClass *class = ARM_CPU_GET_CLASS(cpu);
> +
> +    class->parent_reset(c);
> +
> +    /* TODO Drop this in favor of cpu_arm_reset() calling cpu_reset()
> +     *      once cpu_reset_model_id() is gone. */
> +    cpu_state_reset(&cpu->env);
> +}

...there is no cpu_arm_reset(), do you mean arm_cpu_reset()
in this comment?

-- PMM

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

[Peter Maydell]

On 28 March 2012 14:46, Andreas Färber <afaerber@suse.de> wrote:
> Am 28.03.2012 15:40, schrieb Peter Maydell:
>> On 26 March 2012 18:28, Andreas Färber <afaerber@suse.de> wrote:
>>
>>> +static void arm_cpu_reset(CPUState *c)
>>> +{
>>> +    ARMCPU *cpu = ARM_CPU(c);
>>> +    ARMCPUClass *class = ARM_CPU_GET_CLASS(cpu);
>>> +
>>> +    class->parent_reset(c);
>>
>> I thought we were avoiding 'class' in favour of 'klass'?
>
> Max complained about that and no one argued against him, so I avoided it
> in the .c file where it's not strictly necessary. It's really only
> necessary in the headers. But I don't mind either way.
>
> For me, the convention is cpu_class => CPUClass, so it would be unwise
> here, thus one of class, clazz, klass.

I don't particularly care but I'd rather we were consistent.
Mostly the devices seem to go for short variable names, like:
 sc = I2C_SLAVE_GET_CLASS(dev);
 IDEDeviceClass *dc = IDE_DEVICE_GET_CLASS(dev);
 cdc = HDA_CODEC_DEVICE_GET_CLASS(codec);
 DeviceClass *dc = DEVICE_GET_CLASS(dev);
 VirtIOSerialPortClass *k = VIRTIO_SERIAL_PORT_GET_CLASS(port);

and more rarely 'klass':
 ISADeviceClass *klass = ISA_DEVICE_GET_CLASS(dev);
and never 'class' or 'foo_class'. (all examples obtained via
'git grep _GET_CLASS'.)

That would suggest 'k' or 'acc' here.

>>> +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. */
>>
>> As it happens I'm planning to create the per-implementation
>> subclasses first and do the cp15 rework second.
>
> Suggest a rephrase? :)

Dunno. /* TODO Replace with per-implementation subclasses later */ ?

-- PMM