Re: [Qemu-devel] [PATCH 2/2] cpu: for cpu-user and cpu-softmmu and make cpu-softmmu a child of DeviceState

[Anthony Liguori]

Andreas Färber <address@hidden> writes:

> Am 10.08.2012 19:00, schrieb Anthony Liguori:
>> The line between linux-user and softmmu is not very well defined right now.
>> linux-user really don't want to include devices and making CpuState a child 
>> of
>> DeviceState would require pulling lots of stuff into linux-user.
>> 
>> To solve this, we simply fork cpu-user and cpu-softmmu letting them evolve
>> independently.
>
> In the KVM call discussion there was no consensus that such independent
> evolution is actually desirable.
>
> For example, moving forward with eliminating the CPU_COMMON mess this
> will mean duplicating the implementation of cpu_common_reset() -
> currently empty but in need of at least some zero'ing in the future. My
> fear is that such duplication will lead to fixes getting applied to the
> softmmu version only and contributors forgetting / being unaware that
> there is a second place to apply the same change.

Well the alternative is to build qdev for linux-user.

I tried very hard to make that work but I gave up.  That is without a
doubt the Right Solution.

>
> Putting different TypeInfos into seperate source files is fine with me,
> but I would rather have shared implementations with an #ifdef than
> duplicating things.

I fear a tremendous amount of #ifdef's becoming necessary to make this
work long term.  That's why I prefer divergence.  linux-user can stay
simple and softmmu will be unaffected.

> By comparison, forgetting to add a new field in either struct would lead
> to a compilation failure, getting noticed, so that seems doable.
>
>> We also need to push a qdev_init_nofail() into all callers of cpu_init().  
>> This
>> is needed eventually anyway so might as well do this now.
>
> Patch would be better readable doing that as a follow-up...

Pretty sure that causes a breakage although I don't remember why so I
don't think splitting up the patch is possible.

>
>> Signed-off-by: Anthony Liguori <address@hidden>
>> ---
>>  Makefile.objs                 |    6 ---
>>  hw/armv7m.c                   |    1 +
>>  hw/axis_dev88.c               |    1 +
>>  hw/exynos4210.c               |    1 +
>>  hw/highbank.c                 |    1 +
>>  hw/integratorcp.c             |    1 +
>>  hw/leon3.c                    |    1 +
>>  hw/lm32_boards.c              |    2 +
>>  hw/milkymist.c                |    1 +
>>  hw/mips_fulong2e.c            |    1 +
>>  hw/mips_jazz.c                |    1 +
>>  hw/mips_malta.c               |    1 +
>>  hw/mips_mipssim.c             |    1 +
>>  hw/mips_r4k.c                 |    1 +
>>  hw/musicpal.c                 |    1 +
>>  hw/omap1.c                    |    1 +
>>  hw/omap2.c                    |    1 +
>>  hw/pc.c                       |    1 +
>>  hw/petalogix_ml605_mmu.c      |    1 +
>>  hw/petalogix_s3adsp1800_mmu.c |    1 +
>>  hw/ppc440_bamboo.c            |    1 +
>>  hw/ppc4xx_devs.c              |    1 +
>>  hw/ppc_newworld.c             |    1 +
>>  hw/ppc_oldworld.c             |    1 +
>>  hw/ppc_prep.c                 |    1 +
>>  hw/ppce500_mpc8544ds.c        |    1 +
>>  hw/pxa2xx.c                   |    1 +
>>  hw/r2d.c                      |    1 +
>>  hw/realview.c                 |    1 +
>>  hw/s390-virtio.c              |    1 +
>>  hw/spapr.c                    |    1 +
>>  hw/strongarm.c                |    1 +
>>  hw/sun4m.c                    |    1 +
>>  hw/sun4u.c                    |    1 +
>>  hw/versatilepb.c              |    1 +
>>  hw/vexpress.c                 |    2 +
>>  hw/virtex_ml507.c             |    1 +
>>  hw/xen_machine_pv.c           |    1 +
>>  hw/xilinx_zynq.c              |    1 +
>>  hw/xtensa_lx60.c              |    1 +
>>  hw/xtensa_sim.c               |    1 +
>>  include/qemu/cpu-softmmu.h    |   82 +++++++++++++++++++++++++++++++++++++++
>>  include/qemu/cpu-user.h       |   75 ++++++++++++++++++++++++++++++++++++
>>  include/qemu/cpu.h            |   85 
>> ++---------------------------------------
>>  qemu-common.h                 |    3 +-
>>  qom/Makefile.objs             |    5 +-
>>  qom/cpu-softmmu.c             |   66 +++++++++++++++++++++++++++++++
>>  qom/cpu-user.c                |   70 +++++++++++++++++++++++++++++++++
>>  48 files changed, 343 insertions(+), 91 deletions(-)
>>  create mode 100644 include/qemu/cpu-softmmu.h
>>  create mode 100644 include/qemu/cpu-user.h
>>  create mode 100644 qom/cpu-softmmu.c
>>  create mode 100644 qom/cpu-user.c
>> 
>> diff --git a/Makefile.objs b/Makefile.objs
>> index 5ebbcfa..f285926 100644
>> --- a/Makefile.objs
>> +++ b/Makefile.objs
>> @@ -12,12 +12,6 @@ qobject-obj-y += qerror.o error.o qemu-error.o
>>  universal-obj-y += $(qobject-obj-y)
>>  
>>  #######################################################################
>> -# QOM
>> -qom-obj-y = qom/
>> -
>> -universal-obj-y += $(qom-obj-y)
>> -
>> -#######################################################################
>>  # oslib-obj-y is code depending on the OS (win32 vs posix)
>>  oslib-obj-y = osdep.o
>>  oslib-obj-$(CONFIG_WIN32) += oslib-win32.o qemu-thread-win32.o
> [...]
>> diff --git a/include/qemu/cpu-softmmu.h b/include/qemu/cpu-softmmu.h
>> new file mode 100644
>> index 0000000..4f7b602
>> --- /dev/null
>> +++ b/include/qemu/cpu-softmmu.h
>> @@ -0,0 +1,82 @@
>> +/*
>> + * 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/gpl-2.0.html>
>> + */
>> +#ifndef QEMU_CPU_H
>> +#define QEMU_CPU_H
>> +
>> +#include "hw/qdev-core.h"
>> +#include "qemu-thread.h"
>> +
>> +/**
>> + * SECTION:cpu
>> + * @section_id: QEMU-cpu
>> + * @title: CPU Class
>> + * @short_description: Base class for all CPUs
>> + */
>> +
>> +#define TYPE_CPU "cpu"
>> +
>> +#define CPU(obj) OBJECT_CHECK(CPUState, (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 CPUState CPUState;
>> +
>> +/**
>> + * CPUClass:
>> + * @reset: Callback to reset the #CPUState to its initial state.
>> + *
>> + * Represents a CPU family or model.
>> + */
>> +typedef struct CPUClass {
>> +    /*< private >*/
>> +    DeviceClass parent_class;
>> +    /*< public >*/
>> +
>> +    void (*reset)(CPUState *cpu);
>> +} CPUClass;
>> +
>> +/**
>> + * CPUState:
>> + *
>> + * State of one CPU core or thread.
>> + */
>> +struct CPUState {
>> +    /*< private >*/
>> +    DeviceState parent_obj;
>> +    /*< public >*/
>> +
>> +    struct QemuThread *thread;
>> +#ifdef _WIN32
>> +   HANDLE hThread;
>> +#endif
>> +    bool thread_kicked;
>> +
>> +    /* TODO Move common fields from CPUArchState here. */
>> +};
>> +
>> +
>> +/**
>> + * cpu_reset:
>> + * @cpu: The CPU whose state is to be reset.
>> + */
>> +void cpu_reset(CPUState *cpu);
>> +
>> +
>> +#endif
>> diff --git a/include/qemu/cpu-user.h b/include/qemu/cpu-user.h
>> new file mode 100644
>> index 0000000..78b65b3
>> --- /dev/null
>> +++ b/include/qemu/cpu-user.h
>> @@ -0,0 +1,75 @@
>> +/*
>> + * 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/gpl-2.0.html>
>> + */
>> +#ifndef QEMU_CPU_H
>> +#define QEMU_CPU_H
>> +
>> +#include "qemu/object.h"
>> +
>> +/**
>> + * SECTION:cpu
>> + * @section_id: QEMU-cpu
>> + * @title: CPU Class
>> + * @short_description: Base class for all CPUs
>> + */
>> +
>> +#define TYPE_CPU "cpu"
>> +
>> +#define CPU(obj) OBJECT_CHECK(CPUState, (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 CPUState CPUState;
>> +
>> +/**
>> + * CPUClass:
>> + * @reset: Callback to reset the #CPUState to its initial state.
>> + *
>> + * Represents a CPU family or model.
>> + */
>> +typedef struct CPUClass {
>> +    /*< private >*/
>> +    ObjectClass parent_class;
>> +    /*< public >*/
>> +
>> +    void (*reset)(CPUState *cpu);
>> +} CPUClass;
>> +
>> +/**
>> + * CPUState:
>> + *
>> + * State of one CPU core or thread.
>> + */
>> +struct CPUState {
>> +    /*< private >*/
>> +    Object parent_obj;
>> +    /*< public >*/
>> +
>> +    /* TODO Move common fields from CPUArchState here. */
>> +};
>> +
>> +
>> +/**
>> + * cpu_reset:
>> + * @cpu: The CPU whose state is to be reset.
>> + */
>> +void cpu_reset(CPUState *cpu);
>> +
>> +
>> +#endif
>> diff --git a/include/qemu/cpu.h b/include/qemu/cpu.h
>> index ad706a6..862988a 100644
>> --- a/include/qemu/cpu.h
>> +++ b/include/qemu/cpu.h
>> @@ -1,82 +1,5 @@
>> -/*
>> - * 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/gpl-2.0.html>
>> - */
>
> If we empty this file, suggest to use either a GPLv2+ or MIT header to
> clarify licensing.
>
>> -#ifndef QEMU_CPU_H
>> -#define QEMU_CPU_H
>> -
>> -#include "qemu/object.h"
>> -#include "qemu-thread.h"
>> -
>> -/**
>> - * SECTION:cpu
>> - * @section_id: QEMU-cpu
>> - * @title: CPU Class
>> - * @short_description: Base class for all CPUs
>> - */
>> -
>> -#define TYPE_CPU "cpu"
>> -
>> -#define CPU(obj) OBJECT_CHECK(CPUState, (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 CPUState CPUState;
>> -
>> -/**
>> - * CPUClass:
>> - * @reset: Callback to reset the #CPUState to its initial state.
>> - *
>> - * Represents a CPU family or model.
>> - */
>> -typedef struct CPUClass {
>> -    /*< private >*/
>> -    ObjectClass parent_class;
>> -    /*< public >*/
>> -
>> -    void (*reset)(CPUState *cpu);
>> -} CPUClass;
>> -
>> -/**
>> - * CPUState:
>> - *
>> - * State of one CPU core or thread.
>> - */
>> -struct CPUState {
>> -    /*< private >*/
>> -    Object parent_obj;
>> -    /*< public >*/
>> -
>> -    struct QemuThread *thread;
>> -#ifdef _WIN32
>> -    HANDLE hThread;
>> -#endif
>> -    bool thread_kicked;
>> -
>> -    /* TODO Move common fields from CPUArchState here. */
>> -};
>> -
>> -
>> -/**
>> - * cpu_reset:
>> - * @cpu: The CPU whose state is to be reset.
>> - */
>> -void cpu_reset(CPUState *cpu);
>
> As commented on IRC, I would prefer this to stay in a shared cpu.h since
> it is used identically for softmmu and *-user. Simplifies rewriting
> documentation or adding Error** or whatever.
>
> Same for TYPE_CPU and cast macros, they do not differ.
>
>> -
>> -
>> +#ifdef CONFIG_USER_ONLY
>> +#include "qemu/cpu-user.h"
>> +#else
>> +#include "qemu/cpu-softmmu.h"
>>  #endif
>> diff --git a/qemu-common.h b/qemu-common.h
>> index f9deca6..47a3aab 100644
>> --- a/qemu-common.h
>> +++ b/qemu-common.h
>> @@ -270,7 +270,6 @@ typedef struct PCIEPort PCIEPort;
>>  typedef struct PCIESlot PCIESlot;
>>  typedef struct MSIMessage MSIMessage;
>>  typedef struct SerialState SerialState;
>> -typedef struct IRQState *qemu_irq;
>>  typedef struct PCMCIACardState PCMCIACardState;
>>  typedef struct MouseTransformInfo MouseTransformInfo;
>>  typedef struct uWireSlave uWireSlave;
>> @@ -281,6 +280,8 @@ typedef struct VirtIODevice VirtIODevice;
>>  typedef struct QEMUSGList QEMUSGList;
>>  typedef struct SHPCDevice SHPCDevice;
>>  
>> +#include "hw/irq.h"
>
> Independent of cpu split?
>
>> +
>>  typedef uint64_t pcibus_t;
>>  
>>  typedef enum LostTickPolicy {
>> diff --git a/qom/Makefile.objs b/qom/Makefile.objs
>> index 5ef060a..a24fadf 100644
>> --- a/qom/Makefile.objs
>> +++ b/qom/Makefile.objs
>> @@ -1,4 +1,3 @@
>>  qom-obj-y = object.o container.o qom-qobject.o
>> -qom-obj-twice-y = cpu.o
>> -common-obj-y = $(qom-obj-twice-y)
>> -user-obj-y = $(qom-obj-twice-y)
>> +common-obj-y += $(qom-obj-y) cpu-softmmu.o
>> +user-obj-y += $(qom-obj-y) cpu-user.o
>
> This is silently making cpu.c unused without deleting it, no?
>
>> diff --git a/qom/cpu-softmmu.c b/qom/cpu-softmmu.c
>> new file mode 100644
>> index 0000000..5790944
>> --- /dev/null
>> +++ b/qom/cpu-softmmu.c
>> @@ -0,0 +1,66 @@
>> +/*
>> + * 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/gpl-2.0.html>
>> + */
>> +
>> +#include "qemu/cpu.h"
>> +#include "qemu-common.h"
>> +#include "hw/qdev.h"
>> +
>> +void cpu_reset(CPUState *cpu)
>> +{
>> +    CPUClass *klass = CPU_GET_CLASS(cpu);
>> +
>> +    if (klass->reset != NULL) {
>> +        (*klass->reset)(cpu);
>> +    }
>> +}
>> +
>> +static void cpu_common_reset(CPUState *cpu)
>> +{
>> +}
>> +
>> +static int cpu_realize(DeviceState *dev)
>> +{
>> +    return 0;
>> +}
>> +
>> +static void cpu_class_init(ObjectClass *klass, void *data)
>> +{
>> +    CPUClass *k = CPU_CLASS(klass);
>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>> +
>> +    k->reset = cpu_common_reset;
>> +    dc->init = cpu_realize;
>> +}
>> +
>> +static TypeInfo cpu_type_info = {
>
> const was/is missing... o;-)

Ack.

>
> Since this has been getting late - sorry - and there being no follow-ups
> for v1.2, what about applying patch 1/2 and deferring 2/2 to v1.3 /
> cpu-next?

I think we should apply 1/2 for 1.2.  For 1.3, we need a better solution
and I think the better solution is to do this split and insist on people
that care about linux-user doing the work to make qdev buildable.

I was concerned about this from the beginning (introducing qom into
linux-user).  I think we need to draw a line here and say that
linux-user needs to keep up with the rate of change here.  We can't
burden softmmu to do all the work on linux-user.

If anyone has a better idea about how to isolate linux-user here I'm all
for it but if it's going to need to interact with QOM, it needs to be
able to handle qdev and everything else that comes with it.

Regards,

Anthony Liguori

>
> Regards,
> Andreas
>
>> +    .name = TYPE_CPU,
>> +    .parent = TYPE_DEVICE,
>> +    .instance_size = sizeof(CPUState),
>> +    .abstract = true,
>> +    .class_size = sizeof(CPUClass),
>> +    .class_init = cpu_class_init,
>> +};
>> +
>> +static void cpu_register_types(void)
>> +{
>> +    type_register_static(&cpu_type_info);
>> +}
>> +
>> +type_init(cpu_register_types)
>> diff --git a/qom/cpu-user.c b/qom/cpu-user.c
>> new file mode 100644
>> index 0000000..17b796f
>> --- /dev/null
>> +++ b/qom/cpu-user.c
>> @@ -0,0 +1,70 @@
>> +/*
>> + * 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/gpl-2.0.html>
>> + */
>> +
>> +#include "qemu/cpu.h"
>> +#include "qemu-common.h"
>> +
>> +void cpu_reset(CPUState *cpu)
>> +{
>> +    CPUClass *klass = CPU_GET_CLASS(cpu);
>> +
>> +    if (klass->reset != NULL) {
>> +        (*klass->reset)(cpu);
>> +    }
>> +}
>> +
>> +static void cpu_common_reset(CPUState *cpu)
>> +{
>> +}
>> +
>> +static void cpu_class_init(ObjectClass *klass, void *data)
>> +{
>> +#ifndef CONFIG_USER_ONLY
>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>> +#endif
>> +    CPUClass *k = CPU_CLASS(klass);
>> +
>> +#ifndef CONFIG_USER_ONLY
>> +    /* Overwrite this in subclasses for which hotplug is supported. */
>> +    dc->no_user = 1;
>> +#endif
>> +
>> +    k->reset = cpu_common_reset;
>> +}
>> +
>> +static TypeInfo cpu_type_info = {
>> +    .name = TYPE_CPU,
>> +#ifdef CONFIG_USER_ONLY
>> +    .parent = TYPE_OBJECT,
>> +#else
>> +    .parent = TYPE_DEVICE,
>> +#endif
>> +    .instance_size = sizeof(CPUState),
>> +    .abstract = true,
>> +    .class_size = sizeof(CPUClass),
>> +    .class_init = cpu_class_init,
>> +};
>> +
>> +static void cpu_register_types(void)
>> +{
>> +    type_register_static(&cpu_type_info);
>> +}
>> +
>> +type_init(cpu_register_types)
>
>
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg