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

[Andreas Färber]

Am 22.06.2012 21:36, 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.
[snip]

I still don't understand why.

>From your announcement on IRC I thought that you would leave TYPE_CPU as
is and introdruce TYPE_SOFTMMU_CPU / TYPE_USER_CPU as derived
intermediate types. But here you are just replacing my suggested #ifdefs
with code duplication...

The QOM CPUState part 4 series on the list I reminded you of starts
moving more stuff into struct CPUState, which is supposed to be used by
common code, i.e., both softmmu and *-user.

With this patch of yours I'd need to move the fields from central
CPU_COMMON into *two* structs kept in sync, which doesn't feel like an
advantage to me. Apart from fields for include/qemu/cpu.h I was also
planning on placing helper functions into qom/cpu.c, which now would
need to be duplicated, like cpu_common_reset() for the fields added. So
if there's some error it is likely to get fixed in one version but
forgotten in the other one. That would be really bad. If functions do
mostly the same thing they should be compiled from the same source -
basics of Product Line Engineering. If there's functions that only apply
to one of them then I'm fine placing them in a cpu-softmmu or cpu-user
file respectively. But this patch 2/2 just seems to make more work
without real gains to avoid #ifdefs - we'll get some anyway due to w32
and unrolling those into even more file copies sounds even worse.

Apart from tcg/, which you keep iterating, linux-user also reuses most
of target-*/ (except for supervisor- and hypervisor-level instructions)
as well as core CPU execution infrastructure, which I'm trying to
streamline through CPUState compiled only twice rather than per each
*-softmmu and *-*-user.

So IMO the key point is that we don't want them to evolve independently.
We want to code efficiently and to build QEMU efficiently.

Regards,
Andreas

For reference my previous suggestion for CPUState-as-a-DeviceState was:

diff --git a/include/qemu/cpu.h b/include/qemu/cpu.h
index 78b65b3..a4d84a5 100644
--- a/include/qemu/cpu.h
+++ b/include/qemu/cpu.h
@@ -21,6 +21,9 @@
 #define QEMU_CPU_H

 #include "qemu/object.h"
+#ifndef CONFIG_USER_ONLY
+#include "hw/qdev.h"
+#endif

 /**
  * SECTION:cpu
@@ -45,7 +48,11 @@ typedef struct CPUState CPUState;
  */
 typedef struct CPUClass {
     /*< private >*/
+#ifdef CONFIG_USER_ONLY
     ObjectClass parent_class;
+#else
+    DeviceClass parent_class;
+#endif
     /*< public >*/

     void (*reset)(CPUState *cpu);
diff --git a/qom/cpu.c b/qom/cpu.c
index 5b36046..17b796f 100644
--- a/qom/cpu.c
+++ b/qom/cpu.c
@@ -36,14 +36,26 @@ 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),

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