From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41007) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V4iJp-00014x-EG for qemu-devel@nongnu.org; Wed, 31 Jul 2013 22:08:53 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1V4iJh-0000qu-SO for qemu-devel@nongnu.org; Wed, 31 Jul 2013 22:08:45 -0400 Received: from mail-pd0-f182.google.com ([209.85.192.182]:51612) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V4iJh-0000qJ-IU for qemu-devel@nongnu.org; Wed, 31 Jul 2013 22:08:37 -0400 Received: by mail-pd0-f182.google.com with SMTP id r10so1444413pdi.13 for ; Wed, 31 Jul 2013 19:08:36 -0700 (PDT) Message-ID: <51F9C31C.9060802@ozlabs.ru> Date: Thu, 01 Aug 2013 12:08:28 +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> In-Reply-To: <51F9BA0E.6050003@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 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? >>>> + >>>> + 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? > The point is, this is an object, and in realize you shouldn't abort but > set errp and leave error printing and handling to your caller. The QOM > API as opposed to qdev works with an Error object that you can > error_propagate() to your caller. > > (Also using qdev_* for something that is new-style QOM is ugly IMO.) > >>> Where does icp->nr_servers come from? >> >> Via properties in try_create_xics() (hw/ppc/spapr.c). > > Sounds tricky... Peter introduced static array properties for a similar > purpose, I believe. Don't know if that would help here. > >> >>> 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? -- Alexey