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

[Andreas Färber]

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.

> 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.
=> object_initialize() for sub-devices

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.
=> .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.

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.
* 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.)
* 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.
* 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