From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50040) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V7OTy-0005kb-GF for qemu-devel@nongnu.org; Thu, 08 Aug 2013 07:34:25 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1V7OTk-00022h-UX for qemu-devel@nongnu.org; Thu, 08 Aug 2013 07:34:18 -0400 Message-ID: <52038226.1000801@suse.de> Date: Thu, 08 Aug 2013 13:33:58 +0200 From: =?UTF-8?B?QW5kcmVhcyBGw6RyYmVy?= MIME-Version: 1.0 References: <1375777673-20274-1-git-send-email-aik@ozlabs.ru> <1375777673-20274-6-git-send-email-aik@ozlabs.ru> <5200C789.5030802@suse.de> <5201E3F4.8070409@ozlabs.ru> <5201F146.5060400@suse.de> <5201F6BE.9090804@ozlabs.ru> <5202581C.3070700@suse.de> <52030C0B.8020705@ozlabs.ru> In-Reply-To: <52030C0B.8020705@ozlabs.ru> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 5/6] xics: split to xics and xics-common List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexey Kardashevskiy Cc: Anthony Liguori , Alexander Graf , qemu-devel@nongnu.org, qemu-ppc@nongnu.org, Paul Mackerras , David Gibson Am 08.08.2013 05:10, schrieb Alexey Kardashevskiy: > On 08/08/2013 12:22 AM, Andreas F=C3=A4rber wrote: >> Setting a canonical path is done via object_property_add_child() after >> instantiating and before realizing a device. >> For x86 the northbridge is used as name, i.e. "i440fx" for pc and "q35= " >> for q35. Suggest to discuss with Anthony how to structure the >> composition tree for sPAPR. >=20 > I tried inserting this: > object_property_add_child(qdev_get_machine(), busname, OBJECT(dev), NUL= L); >=20 > in TYPE_SPAPR_PCI_HOST_BRIDGE's spapr_phb_init() (I thought this is wha= t > i440fx_init() does in the very beginning) but when I do that, spapr_phb > already has a parent of a "container" type so assert happens. See above: After instantiating (instance_init) and before realizing (realize/init). It needs to be done by the parent, not by the device itself - unless I'm completely misunderstanding what you're trying, in absence of code to look at. > What is the aim of all of this? Is it not to have "unattached" in a pat= h > starting with /machine? Correct. To have a canonical path for management tools, qtest, etc. similar to /sys filesystem in Linux. > btw I have added notes in the previous response against splitting ICS > initialization into 2 functions and you skipped it. Does this mean you = do > not want to waste more of your time persuading me and this is the real > stopped or you just gave up? :) Thanks. "Never give up, never surrender!" :P I believe I had already answered to that: I have no clue what ICS, ICP, XICS acronyms actually stand for, but I explained friendly and verbose why doing QOM things the way I asked you to makes sense. Ultimately I was told by Anthony when and how to do these things, and I do not have all day to argue with you, so if you don't like my feedback then please discuss with your IBM colleague directly. There is by definition a functional split between instance_init and realize, not my invention, and management tools should be able to tweak properties of objects, such as you setting nr_servers to 1 on machine init and QMP qom-set bumping it to 2. It is IMO not essential to implement that from the start because having a performant interrupt implementation is more important than our QOM'ish refactorings, therefore I said error_setg() should be okay, but my feedback is targeted at keeping our design future-proof, which means that instantiation of a single object that was in instance_init before should not be moved into a property setter that may be called multiple times because it would then need to be moved back anyway. Use of object_initialize() was requested by Anthony to have the QOM composition reflected in the allocation model, i.e. in this case g_try_malloc0(sizeof(XICSState)) including the ICS(?) thingy. So since it was created once independent of properties before your patch, I assume we should keep that behavior, while only changing the storage location. Still a whole week left for cleaning up 1.7 patches! :) 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