Patchwork [v1,2/6] a9mpcore: localised temporary init-only variables

login
register
mail settings
Submitter Peter Crosthwaite
Date Feb. 8, 2013, 4:03 a.m.
Message ID <5f322e56-bdc7-47a6-988c-188b61578de0@VA3EHSMHS021.ehs.local>
Download mbox | patch
Permalink /patch/219039/
State New
Headers show

Comments

Peter Crosthwaite - Feb. 8, 2013, 4:03 a.m.
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.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/a9mpcore.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)
Peter Maydell - Feb. 18, 2013, 6:12 p.m.
On 8 February 2013 04:03, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> 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.

-- PMM
Peter Crosthwaite - Feb. 19, 2013, 12:39 a.m.
On Tue, Feb 19, 2013 at 4:12 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 8 February 2013 04:03, Peter Crosthwaite
> <peter.crosthwaite@xilinx.com> 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(). 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.

CC Andreas, as his comment on patch 6 ties in.

Regards,
Peter

> -- PMM
>
Andreas Färber - Feb. 19, 2013, 9:56 a.m.
Am 19.02.2013 01:39, schrieb Peter Crosthwaite:
> On Tue, Feb 19, 2013 at 4:12 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 8 February 2013 04:03, Peter Crosthwaite
>> <peter.crosthwaite@xilinx.com> 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
Peter Crosthwaite - Feb. 19, 2013, 12:43 p.m.
Hi Andreas,

On Tue, Feb 19, 2013 at 7:56 PM, Andreas Färber <afaerber@suse.de> wrote:
> Am 19.02.2013 01:39, schrieb Peter Crosthwaite:
>> On Tue, Feb 19, 2013 at 4:12 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> On 8 February 2013 04:03, Peter Crosthwaite
>>> <peter.crosthwaite@xilinx.com> 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
>
Peter Maydell - Feb. 19, 2013, 12:52 p.m.
On 19 February 2013 12:43, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> On Tue, Feb 19, 2013 at 7:56 PM, Andreas Färber <afaerber@suse.de> wrote:
>> 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.

IIRC we decided that the answer to this is that you create the
children in the "property value has been set" hook...

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

I strongly dislike the inline-struct approach because it makes
public internal-device-state (ie the struct fields) which were
previously in QDEV entirely private. I don't want to go backwards
here so we need a solution which doesn't just move all these
structs into public header files.

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

I didn't mean "all ARM IP in a container"!  I meant that we should
have a "cortex-a9 with all the CPUs and the private peripherals
and the GIC" container, corresponding exactly to the way that in
hardware this all comes as a complete composed lump. At the moment
in QEMU we model the private peripherals in a container but the
CPU cores themselves are instantiated separately by the board
and wired up. That's pointless duplication.

-- PMM
Peter Crosthwaite - Feb. 20, 2013, 12:23 a.m.
On Tue, Feb 19, 2013 at 4:12 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 8 February 2013 04:03, Peter Crosthwaite
> <peter.crosthwaite@xilinx.com> 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.
>

Patch dropped pending resolution of QOM container inline-struct discussion.

Regards,
Peter

> -- PMM
>

Patch

diff --git a/hw/a9mpcore.c b/hw/a9mpcore.c
index 673bbd8..1f6c985 100644
--- a/hw/a9mpcore.c
+++ b/hw/a9mpcore.c
@@ -20,7 +20,6 @@  typedef struct a9mp_priv_state {
     uint32_t num_cpu;
     MemoryRegion scu_iomem;
     MemoryRegion container;
-    DeviceState *mptimer;
     DeviceState *gic;
     uint32_t num_irq;
 } a9mp_priv_state;
@@ -130,6 +129,7 @@  static int a9mp_priv_init(SysBusDevice *dev)
 {
     a9mp_priv_state *s = FROM_SYSBUS(a9mp_priv_state, dev);
     SysBusDevice *busdev, *gicbusdev;
+    DeviceState *qdev;
     int i;
 
     s->gic = qdev_create(NULL, "arm_gic");
@@ -144,10 +144,10 @@  static int a9mp_priv_init(SysBusDevice *dev)
     /* Pass through inbound GPIO lines to the GIC */
     qdev_init_gpio_in(&s->busdev.qdev, a9mp_priv_set_irq, s->num_irq - 32);
 
-    s->mptimer = qdev_create(NULL, "arm_mptimer");
-    qdev_prop_set_uint32(s->mptimer, "num-cpu", s->num_cpu);
-    qdev_init_nofail(s->mptimer);
-    busdev = SYS_BUS_DEVICE(s->mptimer);
+    qdev = qdev_create(NULL, "arm_mptimer");
+    qdev_prop_set_uint32(qdev, "num-cpu", s->num_cpu);
+    qdev_init_nofail(qdev);
+    busdev = SYS_BUS_DEVICE(qdev);
 
     /* Memory map (addresses are offsets from PERIPHBASE):
      *  0x0000-0x00ff -- Snoop Control Unit