From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46772) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V4jEY-0006mp-AT for qemu-devel@nongnu.org; Wed, 31 Jul 2013 23:07:29 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1V4jEQ-0004G3-Cp for qemu-devel@nongnu.org; Wed, 31 Jul 2013 23:07:22 -0400 Message-ID: <51F9D0DA.40304@suse.de> Date: Thu, 01 Aug 2013 05:07:06 +0200 From: =?UTF-8?B?QW5kcmVhcyBGw6RyYmVy?= MIME-Version: 1.0 References: <1374043057-27208-1-git-send-email-aik@ozlabs.ru> <1374043057-27208-5-git-send-email-aik@ozlabs.ru> <51F96B13.1060201@suse.de> <51F9A855.9000702@ozlabs.ru> <51F9BA0E.6050003@suse.de> <51F9C31C.9060802@ozlabs.ru> In-Reply-To: <51F9C31C.9060802@ozlabs.ru> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 4/4] xics: Support for in-kernel XICS interrupt controller List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexey Kardashevskiy Cc: Peter Maydell , Anthony Liguori , qemu-devel@nongnu.org, Alexander Graf , Paul Mackerras , qemu-ppc@nongnu.org, David Gibson Am 01.08.2013 04:08, schrieb Alexey Kardashevskiy: > On 08/01/2013 11:29 AM, Andreas F=C3=A4rber wrote: >> Am 01.08.2013 02:14, schrieb Alexey Kardashevskiy: >>> On 08/01/2013 05:52 AM, Andreas F=C3=A4rber wrote: >>>> Am 17.07.2013 08:37, schrieb Alexey Kardashevskiy: >>>>> +/* >>>>> + * XICS-KVM >>>>> + */ >>>>> +static void xics_kvm_cpu_setup(XICSState *icp, PowerPCCPU *cpu) >>>>> +{ >>>>> + CPUState *cs; >>>>> + ICPState *ss; >>>>> + XICSStateKVM *icpkvm =3D (XICSStateKVM *) object_dynamic_cast( >>>>> + OBJECT(icp), TYPE_XICS_KVM); >>>>> + XICSStateClass *xics_info =3D XICS_CLASS(object_class_by_name(= TYPE_XICS)); >>>> >>>> Are you intentionally accessing that class by name rather than using >>>> XICS_GET_CLASS(icp), which allows the KVM variant to overwrite thing= s? >>> >>> >>> This is KVM's CPU_setup(). I want to call non-KVM CPU_setup afterward= s, >>> i.e. "call parent method". XICS_GET_CLASS will return XICS_KVM class = but >>> not XICS, no? >> >> OK, then I'll CC you on my upcoming virtio v2 series that introduces a >> more comprehensable macro for this purpose: I would/will recommend to >> use a local macro KVM_XICS_GET_PARENT_CLASS(obj) - where you could mov= e >> your current inline implementation - to make more obvious that it's no= t >> a mistake. >=20 > Oh. So. This has to wait till that virtio thing gets to upstream. Corre= ct? Not quite, there is no dependency on virtio. a) You could do #define KVM_XICS_GET_PARENT_CLASS(obj) \ object_class_by_name(TYPE_KVM_XICS) for now, where you do #define TYPE_KVM_XICS etc. Note that this is a proposal and not a rule. So far we agreed that adding classes with fields was not working well for variable depths of hierarchy and that open-coding the access was not ideal either. Nothing more, nothing less. b) You could cherry-pick just http://patchwork.ozlabs.org/patch/263863/ to update the implementation of a), but since that has not yet been acked I would advise against doing that immediately. But upstream is in freeze for the next two weeks anyway, so no need to ru= sh. c) You should participate in the review of Peter's and my proposals rather than silently inventing your own solution. :) Advantage of our approaches is hardcoding the current type rather than the parent's outside of TypeInfo. >>>>> + >>>>> + icp->ss =3D g_malloc0(icp->nr_servers*sizeof(ICPState)); >>>>> + for (i =3D 0; i < icp->nr_servers; i++) { >>>>> + char buffer[32]; >>>>> + object_initialize(&icp->ss[i], TYPE_ICP_KVM); >>>>> + snprintf(buffer, sizeof(buffer), "icp[%d]", i); >>>>> + object_property_add_child(OBJECT(icp), buffer, OBJECT(&icp= ->ss[i]), NULL); >>>>> + qdev_init_nofail(DEVICE(&icp->ss[i])); >>>> >>>> object_property_set_bool() >>> >>> >>> ? Anthony did XICS refactoring recently and that has qdev_init_nofail= (). >> >> Nobody is perfect. ;) >=20 > That's ok, my question is more about whether I should use set_bool here= and > leave emulated XICS as is or you expect me to fix emulated XICS as well= and > post an additional patch or what? If that is so then yes, cleaning up your existing emulation in a patch before this one would be a good idea. >>>> Is there no way to split this into >>>> instance_init and realize? >>> >>> Why would we want to split? >> >> Because realize is too late to create new devices: With our targetted >> late, recursive realization model it will not be possible to see and >> modify such objects from management interface - only before realize. >> >> I even have a patch on the list that would assert when that happens >> during final recursive realization. >=20 >=20 > So most this stuff has to go to instance_init and since there is no way= to > prevent parent's instance_init from being called, you are basically for= cing > me to introduce an abstract XICS class and inherit emulated XICS and KV= M > XICS from it. Besides that, I do not any use of it. Is that correct? Sorry, I don't follow. x86 and arm do use an abstract base class, e500 doesn't iirc. But whether instance_init or realize, the parent's implementation will/should be called. Andreas --=20 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=C3=BCrnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imend=C3=B6rffer; HRB 16746 AG N=C3=BC= rnberg