Patchwork [v6,2/2] target-arm: Minimalistic CPU QOM'ification

login
register
mail settings
Submitter Andreas Färber
Date March 26, 2012, 5:28 p.m.
Message ID <1332782909-28412-3-git-send-email-afaerber@suse.de>
Download mbox | patch
Permalink /patch/148784/
State New
Headers show

Comments

Andreas Färber - March 26, 2012, 5:28 p.m.
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
Peter Maydell - March 28, 2012, 1:40 p.m.
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
Max Filippov - March 28, 2012, 1:46 p.m.
>> +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.
Andreas Färber - March 28, 2012, 1:46 p.m.
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
Andreas Färber - March 28, 2012, 1:49 p.m.
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
Max Filippov - March 28, 2012, 2 p.m.
>>>> +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?
Andreas Färber - March 28, 2012, 2:05 p.m.
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
Peter Maydell - March 28, 2012, 2:22 p.m.
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
Peter Maydell - March 28, 2012, 2:31 p.m.
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

Patch

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;