From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:57969) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SiQtE-0000Fm-Ci for qemu-devel@nongnu.org; Sat, 23 Jun 2012 10:00:41 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SiQtC-0001da-1F for qemu-devel@nongnu.org; Sat, 23 Jun 2012 10:00:39 -0400 Received: from cantor2.suse.de ([195.135.220.15]:50235 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SiQtB-0001d9-O0 for qemu-devel@nongnu.org; Sat, 23 Jun 2012 10:00:37 -0400 Message-ID: <4FE5CBFC.6010902@suse.de> Date: Sat, 23 Jun 2012 16:00:28 +0200 From: =?ISO-8859-15?Q?Andreas_F=E4rber?= MIME-Version: 1.0 References: <1340393807-15224-1-git-send-email-aliguori@us.ibm.com> <1340393807-15224-2-git-send-email-aliguori@us.ibm.com> In-Reply-To: <1340393807-15224-2-git-send-email-aliguori@us.ibm.com> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 2/2] cpu: for cpu-user and cpu-softmmu and make cpu-softmmu a child of DeviceState List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Anthony Liguori Cc: Peter Maydell , Riku Voipio , Alexander Graf , qemu-devel@nongnu.org, Blue Swirl , Paolo Bonzini 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 c= hild of > DeviceState would require pulling lots of stuff into linux-user. >=20 > To solve this, we simply fork cpu-user and cpu-softmmu letting them evo= lve > 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 =3D DEVICE_CLASS(klass); +#endif CPUClass *k =3D CPU_CLASS(klass); +#ifndef CONFIG_USER_ONLY + /* Overwrite this in subclasses for which hotplug is supported. */ + dc->no_user =3D 1; +#endif + k->reset =3D cpu_common_reset; } static TypeInfo cpu_type_info =3D { .name =3D TYPE_CPU, +#ifdef CONFIG_USER_ONLY .parent =3D TYPE_OBJECT, +#else + .parent =3D TYPE_DEVICE, +#endif .instance_size =3D sizeof(CPUState), .abstract =3D true, .class_size =3D sizeof(CPUClass), --=20 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=FCrnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imend=F6rffer; HRB 16746 AG N=FCrnbe= rg