Re: [Qemu-devel] [PATCH RFC v3 03/21] qom: Introduce CPU class

[Anthony Liguori]

On 02/02/2012 08:59 PM, Andreas Färber wrote:
> It's abstract and derived directly from TYPE_OBJECT.
> Prepare a virtual reset method.
>
> Signed-off-by: Andreas Färber<address@hidden>
> Cc: Anthony Liguori<address@hidden>
> Cc: Peter Maydell<address@hidden>
> ---
>   include/qemu/cpu.h |   73 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>   qom/Makefile       |    1 +
>   qom/cpu.c          |   50 +++++++++++++++++++++++++++++++++++
>   3 files changed, 124 insertions(+), 0 deletions(-)
>   create mode 100644 include/qemu/cpu.h
>   create mode 100644 qom/cpu.c
>
> diff --git a/include/qemu/cpu.h b/include/qemu/cpu.h
> new file mode 100644
> index 0000000..22c7404
> --- /dev/null
> +++ b/include/qemu/cpu.h
> @@ -0,0 +1,73 @@
> +/*
> + * QEMU CPU model
> + *
> + * 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/old-licenses/gpl-2.0>
> + */
> +#ifndef QEMU_CPU_H
> +#define QEMU_CPU_H
> +
> +#include "qemu/object.h"
> +
> +#define TYPE_CPU "cpu"
> +
> +#define CPU(obj) OBJECT_CHECK(CPU, (obj), TYPE_CPU)
> +#define CPU_CLASS(class) OBJECT_CLASS_CHECK(CPUClass, (class), TYPE_CPU)
> +#define CPU_GET_CLASS(obj) OBJECT_GET_CLASS(CPUClass, (obj), TYPE_CPU)
> +
> +typedef struct CPU CPU;

If this doesn't result in a build error, then C is a worse language than I 
thought it was :-/

But we definitely shouldn't have a typename and macro name of the same thing and 
expect it to work...

> +
> +/**
> + * CPUClass:
> + * @reset: Callback to reset the #CPU to its initial state.
> + *
> + * Represents a CPU family or model.
> + */
> +typedef struct CPUClass {
> +    ObjectClass parent_class;
> +
> +    void (*reset)(CPU *cpu);
> +} CPUClass;
> +
> +/**
> + * CPU:
> + *
> + * State of one CPU core or thread.
> + */
> +struct CPU {
> +    Object parent_obj;
> +
> +    /* TODO Move common fields from CPUState here. */
> +};
> +
> +
> +/* TODO Rename to cpu_reset once all CPUState is converted to QOM. */
> +/**
> + * cpu_do_reset:
> + * @cpu: The CPU whose state is to be reset.
> + */
> +void cpu_do_reset(CPU *cpu);

Better to sed-away the current cpu_reset() callers to cpu_reset_legacy() and 
then make this cpu_reset().  Otherwise, you're introducing new code that has the 
wrong usage which is never a good strategy.

> +
> +/**
> + * cpu_common_reset:
> + * @cpu: The CPU whose common state is to be reset.
> + *
> + * To be used by derived classes.
> + */
> +void cpu_common_reset(CPU *cpu);

Make this static, initialize reset = cpu_common_reset in cpu_class_initfn, then 
in the derived class initfn, save the pointer to the parent reset function so it 
can be called later.

At some point, I had an:

ObjectClass *object_get_super_class(Object *obj);

Such that you could do:

CPUClass *super = CPU_CLASS(object_get_super_class(OBJECT(s)));

super->reset(CPU(s));

But my fear was that the subtle semantic of the object representing the class of 
your super class (which is different than a casted form of your class) would 
lead to confusion.

Regards,

Anthony Liguori

> +
> +
> +#endif
> diff --git a/qom/Makefile b/qom/Makefile
> index f33f0be..0a1c8e0 100644
> --- a/qom/Makefile
> +++ b/qom/Makefile
> @@ -1 +1,2 @@
>   qom-y = object.o container.o
> +qom-y += cpu.o
> diff --git a/qom/cpu.c b/qom/cpu.c
> new file mode 100644
> index 0000000..a60823f
> --- /dev/null
> +++ b/qom/cpu.c
> @@ -0,0 +1,50 @@
> +/*
> + * QEMU CPU model
> + *
> + * 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/old-licenses/gpl-2.0>
> + */
> +
> +#include "qemu/cpu.h"
> +#include "qemu-common.h"
> +
> +void cpu_do_reset(CPU *cpu)
> +{
> +    CPUClass *klass = CPU_GET_CLASS(cpu);
> +
> +    if (klass->reset != NULL) {
> +        (*klass->reset)(cpu);
> +    }
> +}
> +
> +void cpu_common_reset(CPU *cpu)
> +{
> +}
> +
> +static TypeInfo cpu_type_info = {
> +    .name = TYPE_CPU,
> +    .parent = TYPE_OBJECT,
> +    .instance_size = sizeof(CPU),
> +    .abstract = true,
> +    .class_size = sizeof(CPUClass),
> +};
> +
> +static void cpu_register_types(void)
> +{
> +    type_register_static(&cpu_type_info);
> +}
> +
> +type_init(cpu_register_types)