Re: [Qemu-devel] [PATCH 5/6] xics: split to xics and xics-common

[Andreas Färber]

Am 08.08.2013 05:10, schrieb Alexey Kardashevskiy:
> On 08/08/2013 12:22 AM, Andreas Färber 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.
> 
> I tried inserting this:
> object_property_add_child(qdev_get_machine(), busname, OBJECT(dev), NULL);
> 
> in TYPE_SPAPR_PCI_HOST_BRIDGE's spapr_phb_init() (I thought this is what
> 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 path
> 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

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg