Re: [Qemu-devel] [PATCH v1 2/6] a9mpcore: localised temporary init-only variables

[Peter Crosthwaite]

Hi Andreas,

On Tue, Feb 19, 2013 at 7:56 PM, Andreas Färber <address@hidden> wrote:
> Am 19.02.2013 01:39, schrieb Peter Crosthwaite:
>> On Tue, Feb 19, 2013 at 4:12 AM, Peter Maydell <address@hidden> wrote:
>>> On 8 February 2013 04:03, Peter Crosthwaite
>>> <address@hidden> wrote:
>>>> The DeviceState *mptimer var in a9mp_priv_state was only used by the init
>>>> function and had no reason for persistence. Made a local variable and 
>>>> removed
>>>> from state struct.
>>>
>>> Nope. We're a container object, we can't just forget about our
>>> children. Granted (like many QEMU devices) we don't actually have
>>> any implementation of device destruction, but in principle we
>>> need to keep hold of a pointer to the things we create.
>>>
>>
>> Shouldn't this be handled on the QOM level? Rather than every device
>> needing to explicitly track its children and propagate destructor
>> calls, QOM should just do this for me. Then as the container, when my
>> destructor is called my childrens is automatically called for me.
>> Keeping these pointers for the sake of heirachy strikes me as
>> replication of the QOM canonical path and its data structures.
>>
>> Plan B is container devices such as this could call
>> object_property_add_child() when they create their children. Then they
>> can get that pointer back later with object_property_get_link().
>
> I think it was suggested to add an object_property_get_child() that
> internally reuses object_property_get_link() a while back. Probably the
> same thread where I borrowed the object_property_is_{child,link}()
> helpers from.
>

+1. Its just object_property_get_link and some assertions to check
that its actually a child right?

>> This
>> is more robust, but more verbose as devices still need to manage their
>> childrens destruction. However that may be needed as whether or nor
>> parent destruction implies child destruction is not clear to me.
>>
>> I think either solution is preferrable to adding pointers to
>> DeviceState for children, as that linkage is already made by QOM.
>
> QOM is no magic weapon you can throw at things and make everything work.
> It requires some advance planning and footwork.
>
> [CC'ing Kuo-Jung for the Faraday SoCs]
>
> The main point of concern being QMP. QOM realize has the purpose of
> decoupling device creation from device modification and emulation start.
> Thus devices may not be created in initfn-turning-into-realize
> functions. Otherwise they wouldn't be visible for inspection and for
> setting properties.

This is an annoying limitation, as containter devices should be able
to create a variable number of children depending on their property
values.

> => object_initialize() for sub-devices
>

OK, so in this flow, the containers Object->init function creates all
the sub-devices and thus calls all the child Object->init functions.
What Im proposing here is that at that point in time, the container
must add the newly created object as a child:

ChildState *c = CHILD(object_new("child_type"));
object_initialize(OBJECT(c));
object_property_add_child(self, "child...", OBJECT(c));

No need to take a handle to c in the parent Object struct as I can
just pull it out anytime with

ChildState *c = CHILD(object_property_get_child(self, "child..."))

Also, there are neat interators (object_child_for_each()), that could
be use for performing the routine tasks (like destruction) on all
children.

> When creating a device, the device itself has no knowledge of on whose
> memory it is being created. Might be via -device, via board config file
> or via QMP/libvirt. Thus the instantiating code needs to know how much
> memory to allocate since instance_init may not fail, only realize.

Hang on though, this is not about creating a device inline in the
parents struct, this is about maintaining pointers to a new device.
This patch is deleting a pointer, not an inline struct. The memory for
the child device is create using object_new() or some code path that
eventually gets to object_new().

I can see in your tegra series you are creating all your childrens
memory inline in the parent struct, but what exactly is your
motivation there? It seems far less flexible than heap allocation
(object_new() and the status quo) of child devices. This idea falls
over when I want to create a number of child devices dynamically as
well (e.g. flip the "cortex" object from single core to dual core
based on a property or some other mechanism).

> => .instance_size = sizeof(MyState) needs to tell
>
> Using object_initialize() sets reference count to 1 since v1.3.
> => object_property_add_child() for QOM to finalize it when the property
> gets deleted so that you don't need to do it yourself
>
> Note when working with containers that the container has no canonical
> path at instance_init time, so you can't use link<> properties, only
> child<> properties. link<> properties only work at machine/global level.
>

Thats ok. Im proposing that these creation "links" are only ever children.

> Probably we should put some of that into the Wiki to avoid repeated
> explanations:
> http://wiki.qemu.org/QOMConventions
>
> Anyway, this is how it works today until someone wades through the code
> and makes it compile in C++ or some other language with native compiler
> support for whatever-feature-you're-missing. Anthony has clearly stated
> that there's no point is complaining about this over and over.
> What conclusions you draw wrt struct placement is up to you, be it an
> a9mpcore.h or per-file headers or named ..._int.h or whatever is to your
> liking.
>
> In-tree example devices to reference are prep_pci and macio.
> NB: QOM realize is supposed to propagate to child devices but this is
> not yet implemented since a) doing it at DeviceState level complicates
> things and b) there's still devices that allocate children in initfn
> rather than following the pattern outlined above.
>
> My tegra branch [1] is still being polished but is the closest thing in
> the ARM world that can give you some ideas:
> * Some stock devices like UARTs didn't support embedding yet but all
> Tegra2 devices were designed in such a way.
> * My Tegra2 SoC device is still an Object rather than a DeviceState,
> thereby lacking realize support.

And fundamental blockers there? why cant TYPE_DEVICE objects be pure containers?

> * My AC100 machine was badly hacked in for demonstration purposes at
> oSC'12, it should not go into the SoC file but into its own ac100.c to
> cleanly separate the two.
> * Tegra2 SoC sub-devices are currently in hw/arm/ but recent conclusion
> was not to do that and to place them into to-be-created subdirectories
> of hw/ by function. (Using #include "hw/..." is thus recommended for new
> files to aid future file movements.)

+1. Function is a much better organisational scheme than vendor.

> * I added all Tegra device structs to a central hw/arm/tegra2.h header;
> if we choose to split SoC devices into hw/i2c/tegra2.c etc. as proposed,
> it would be better to split it IMO and just keep the SoC struct there
> and do #include "hw/i2c/tegra2.h" (to allow embedding in SoM/CoM, here
> Toradex' Colibri and Apalis; Qseven etc. elsewhere).
> * PMM had requested a "cortex" sub-object for grouping the CPU cores and
> any ARM (Ltd./Holdings plc) stuff; I left them in the main SoC object
> though, not sure if that causes lifetime issues in theory.

I dont agree. Grouping IPs by vendor is a little artificial. There is
no architectural reason why PL330, PL35x should go in a special
container with the CPUs while other vendor IP (SDHCI USB etc) live
outside. Also this object is going to vary from SoC to SoC. Zynq's
"cortex" object is going to be completely different to Tegras so reuse
would be difficult.

> * QOM realize support for ARMCPU has just landed in qemu.git yesterday
> but Tegra2 still has an ARMCPU* and still uses cpu_arm_init().
> Thus, the instance_init is the main thing to look at for now, showing
> how instantiating custom sub-devices is supposed to work today.
>
> An earlier patch series that I need to dig out was the SH7750 SoC.
>
> Cheers,
> Andreas
>
> [1] http://repo.or.cz/w/qemu/afaerber.git/shortlog/refs/heads/tegra
>
> --
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
>