From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:34503) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TvkSG-0007wo-1O for qemu-devel@nongnu.org; Thu, 17 Jan 2013 03:04:09 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TvkSD-0007oN-I7 for qemu-devel@nongnu.org; Thu, 17 Jan 2013 03:04:07 -0500 Received: from cantor2.suse.de ([195.135.220.15]:33761 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TvkSD-0007oH-8v for qemu-devel@nongnu.org; Thu, 17 Jan 2013 03:04:05 -0500 Message-ID: <50F7B06F.1050507@suse.de> Date: Thu, 17 Jan 2013 09:03:59 +0100 From: =?ISO-8859-1?Q?Andreas_F=E4rber?= MIME-Version: 1.0 References: <1358314380-9400-1-git-send-email-afaerber@suse.de> <1358314380-9400-4-git-send-email-afaerber@suse.de> <20130116160424.GH20133@otherpad.lan.raisama.net> <50F72F3F.5060908@suse.de> <20130116234332.GH10683@otherpad.lan.raisama.net> In-Reply-To: <20130116234332.GH10683@otherpad.lan.raisama.net> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [RFC qom-cpu 03/15] target-i386: Update CPU to QOM realizefn List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eduardo Habkost Cc: Igor Mammedov , qemu-devel@nongnu.org Am 17.01.2013 00:43, schrieb Eduardo Habkost: > On Wed, Jan 16, 2013 at 11:52:47PM +0100, Andreas F=E4rber wrote: >> Am 16.01.2013 17:04, schrieb Eduardo Habkost: >>> On Wed, Jan 16, 2013 at 06:32:48AM +0100, Andreas F=E4rber wrote: >>> [...] >>>> @@ -2247,6 +2247,9 @@ static void x86_cpu_common_class_init(ObjectCl= ass *oc, void *data) >>>> { >>>> X86CPUClass *xcc =3D X86_CPU_CLASS(oc); >>>> CPUClass *cc =3D CPU_CLASS(oc); >>>> + DeviceClass *dc =3D DEVICE_CLASS(oc); >>>> + >>>> + dc->realize =3D x86_cpu_realizefn; >>> >>> The DeviceClass documenation says: >>> >>> "Any type may override the @realize and/or @unrealize callbacks but >>> needs to call (and thus save) the parent type's implementation if so >>> desired." >>> >>> Why are you not following it? >> >> "if so desired" - I didn't desire or need to call code that calls an >> initfn that no longer exists after this patch. Same as the ISADevice >> conversion series did not unnecessarily call the DeviceClass-level >> backwards-compatibility realizefn: to save time-consuming >> ...Class::parent_realizefn field additions and to not in the end call >> code that doesn't NULL-check ...DeviceClass::init. That's qdev's old >> "leaf type" concept mentioned in the same documentation. >=20 > I had read "if so desired" as "if it's desired to override the realize > callback", not as "if it's desired to call the parent realize function"= . Sorry, and I thought my documentation was too verbose already. ;) > I believed every class could assume that subclasses would never overrid= e > realize() without calling the parent class' realize function (so we > could add stuff to DeviceClass.realize and CPUClass.realize in the > future and be sure that the code would be always called). >=20 > But from the documentation mentioning "new leaf types should consult > their respective parent type", it looks like this decision would be > taken/documented in each base class. If that's the case, then OK. I've sent out a patch improving QOM and DeviceClass documentation. :) >> I mentioned in the cover letter that this needs to be changed once a >> CPUClass-level realizefn is introduced. I could introduce a no-op >> realizefn there and do the regular store+call. >=20 > That was the semantics I was expecting: base classes would safely > introduce realize functions without worrying if subclasses would > override it incorrectly and break it. We could do that if we fix up the respective DeviceClass::init, SysBusDeviceClass::init etc. code. Question is (just as with some x86 CPU code) whether it's worth cleaning up when we know that it is to be refactored later. > Anyway, saving the parent function in every subclass is so cumbersome > that simply documenting it as "CPUClass subclasses must call > qemu_init_vcpu()" sounds easier than "CPUClass subclasses must save the > parent's realize() and call it". [snip] Actually that particular piece of code is unrelated to this discussion since qemu_init_vcpu() still operates on CPUArchState and thus cannot be moved into CPUClass yet. The reason is that cpus.c:qemu_kvm_cpu_thread_fn sets cpu_single_env, and I do not see a solution for that - suggestions or patches welcome. However, I see that kvm-all.c:kvm_on_sigbus_vcpu() can be switched to CPUState now, so that cpus.c:qemu_kvm_eat_signals() can be changed to CPUState, used from cpus.c:qemu_kvm_wait_io_event(). But cpus.c:cpu_thread_is_idle() still uses env->halted, which is blocked by the search for an acceptable solution to flush the TLB at CPUState level (exec.c:cpu_common_post_load()). Andreas --=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