| Message ID | 5f322e56-bdc7-47a6-988c-188b61578de0@VA3EHSMHS021.ehs.local |
|---|---|
| State | New |
| Headers | show |
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
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 >
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
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 >
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
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 >
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
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(-)