From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42302) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V4jwS-0005DO-1p for qemu-devel@nongnu.org; Wed, 31 Jul 2013 23:52:50 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1V4jwM-00027G-0r for qemu-devel@nongnu.org; Wed, 31 Jul 2013 23:52:43 -0400 Received: from mail-pd0-f180.google.com ([209.85.192.180]:48087) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V4jTJ-0004FT-EE for qemu-devel@nongnu.org; Wed, 31 Jul 2013 23:22:37 -0400 Received: by mail-pd0-f180.google.com with SMTP id 15so1482332pdi.39 for ; Wed, 31 Jul 2013 20:22:36 -0700 (PDT) Message-ID: <51F9D475.9060907@ozlabs.ru> Date: Thu, 01 Aug 2013 13:22:29 +1000 From: Alexey Kardashevskiy 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> <51F9D0DA.40304@suse.de> In-Reply-To: <51F9D0DA.40304@suse.de> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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: =?UTF-8?B?QW5kcmVhcyBGw6RyYmVy?= Cc: Peter Maydell , Anthony Liguori , qemu-devel@nongnu.org, Alexander Graf , Paul Mackerras , qemu-ppc@nongnu.org, David Gibson On 08/01/2013 01:07 PM, Andreas Färber wrote: > Am 01.08.2013 04:08, schrieb Alexey Kardashevskiy: >> On 08/01/2013 11:29 AM, Andreas Färber wrote: >>> Am 01.08.2013 02:14, schrieb Alexey Kardashevskiy: >>>> On 08/01/2013 05:52 AM, Andreas Färber 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 = (XICSStateKVM *) object_dynamic_cast( >>>>>> + OBJECT(icp), TYPE_XICS_KVM); >>>>>> + XICSStateClass *xics_info = 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 things? >>>> >>>> >>>> This is KVM's CPU_setup(). I want to call non-KVM CPU_setup afterwards, >>>> 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 move >>> your current inline implementation - to make more obvious that it's not >>> a mistake. >> >> Oh. So. This has to wait till that virtio thing gets to upstream. Correct? > > 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) TYPE_KVM_XICS or TYPE_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 rush. > > 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. I do not understand most of the stuff you do for QOM. I mean I know C++/RTTI pretty good and even worse - I still remember M$ DCOM/OLE2 but I do not know to what extent you want to implement that C++ in QEMU :) >>>>>> + >>>>>> + icp->ss = g_malloc0(icp->nr_servers*sizeof(ICPState)); >>>>>> + for (i = 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. ;) >> >> 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. >> >> >> 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 forcing >> me to introduce an abstract XICS class and inherit emulated XICS and KVM >> 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. Openpic does not but its init() does not do much and openpic does not have child devices but XICS does now (after Anthony's rework). Unlike instance_init(), parent's realize() is not called automatically, this was the main reason why I put everything into realize(). -- Alexey