| Message ID | 1332782909-28412-3-git-send-email-afaerber@suse.de |
|---|---|
| State | New |
| Headers | show |
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
>> +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.
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
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
>>>> +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?
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
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
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
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;
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