Re: [Qemu-devel] Object instantiation vs. device realization: what to do when?
On Thu, 14 Feb 2019 17:58:06 +0100 Paolo Bonzini wrote: > On 14/02/19 17:21, Markus Armbruster wrote: > > * As an interim step, the #DeviceState:realized property can also be > > * set with qdev_init_nofail(). > > * In the future, devices will propagate this state change to their children > > * and along busses they expose. > > > > This sentence is five years old. Any progress? If not, any intentions > > to make progress? > > Good news, it's done! What we still don't do is removing > qdev_init_nofail and realizing the whole machine in one fell swoop. > > Bad news, it's been done for five years: > > commit 5c21ce77d7e5643089ceec556c0408445d017f32 > Author: Bandan Das > Date: Wed Mar 12 21:02:12 2014 +0100 > > qdev: Realize buses on device realization > > Integrate (un)realization of child buses with > realization/unrealization > of the device hosting them. Code in device_unparent() is reordered > for unrealization of buses to work as part of device unrealization. > > That way no changes need to be made to bus instantiation. > > Signed-off-by: Bandan Das > Signed-off-by: Andreas Färber > > so I don't expect that the next step will ever happen... that isn't really used (realize part) last time we've looked at it https://www.mail-archive.com/qemu-devel@nongnu.org/msg570424.html > > * The point in time will be deferred to machine creation, so that values > > * set in @realize will not be introspectable beforehand. Therefore devices > > * must not create children during @realize; they should initialize them via > > * object_initialize() in their own #TypeInfo.instance_init and forward the > > * realization events appropriately. > > > > This is mostly greek to me. Pity the developer who knows less about > > qdev than I do. > > The first part refers to what virtio_instance_init_common does: > > object_initialize(vdev, vdev_size, vdev_name); > object_property_add_child(proxy_obj, "virtio-backend", OBJECT(vdev), > NULL); > object_unref(OBJECT(vdev)); > > The second part doesn't apply to virtio because it has a bus (so the > code from the above commit handles recursive realization automatically). > > hw/sd/milkymist-memcard.c has an example of this: > > object_property_set_bool(OBJECT(carddev), true, "realized", &err); > > but it should create the device in milkymist_memcard_init rather than > milkymist_memcard_realize, in order to obey the directives of the sacred > book of QEMU. > > Paolo >
Re: [Qemu-devel] Object instantiation vs. device realization: what to do when?
Peter Maydell writes: > On Thu, 14 Feb 2019 at 16:21, Markus Armbruster wrote: >> >> One of qdev's perennial sources of confusion is what to do at object >> instantiation time, i.e. in TypeInfo::instance_init(), and what to do at >> device realization time, i.e. in DeviceClass::realize(). > > Thanks for opening this topic. It's been on my todo list for a > long time to try to figure out what the answer is... > >> qdev-core.h's >> comment falls short: >> >> * # Realization # >> * Devices are constructed in two stages, >> * 1) object instantiation via object_initialize() and >> * 2) device realization via #DeviceState:realized property. >> * The former may not fail (and must not abort or exit, since it is called >> * during device introspection already), and the latter may return error >> * information to the caller and must be re-entrant. >> * Trivial field initializations should go into #TypeInfo.instance_init. >> >> As usual, "trivial" was too trivial to explain; the reader is trusted to >> figure it out himself. Well, I'm afraid I'm not to be trusted. >> >> The easy part is "can fail means it's not trivial", because >> ::instance_init() must not fail. >> >> What about side effects? If you already understand how introspection >> works, or perhaps even if you read carefully and paranoidly, then "since >> it is called during device introspection already" implies "guest-visible >> side-effects would be disastrous". So guest-visible side-effect also >> means it's not trivial. > > I think the other question here is "what about stuff that has > to be undone/freed/etc". Can I, for instance, allocate memory > in the initialize function? Yes, but since .instance_init() can't fail, you have to treat OOM as fatal. > If so, where do I need to put the > corresponding memory-free? .instance_finalize(). >Similar but harder to figure out: > what about things which are ref-counted like memory_region_init()? > Can I pass a pointer to 'obj' as the "owner" to memory_region_init() > from its initialize function and trust that the refcounting will > result in the MR being suitably destroyed when whatever the > reverse of initialize happens? Or does this only work for really > initialized objects and so has to be postponed to realize time? Hmm. I'm not really familiar with this stuff, but I can try to reason anyway. Passing @dev memory_region_init() as owner along with a non-null name makes the memory region a child of @dev. When object_finalize() destroys @dev, it deletes all properties, which runs their release() method. I figure this is the automatic destruction you mean. Since object_initialize() cannot fail, destruction must run object_finalize(). I guess the answer to your question is "can do in instance_init()". Perhaps we should ask another question first: is it useful to do these things in .instance_init()? > A lot of our devices get away with shortcuts because (apart from > the introspection special case) they're created once at startup > and never destroyed, but perhaps we should be stricter about requiring > everything to support the full create-and-destroy lifecycle (and > testing it)? I'm afraid the shortcuts sow confusion and breed bugs. They get imitated in places where they're invalid. They also bite us in the posterior when we make devices unpluggable. However, we might have dug ourselves into a pretty deep hole there. > At least, we should ensure our documentation describes > the 'destroy' part of things so that if you are writing a properly > destroyable device you can get it right. Documentation and good examples will certainly help. Sadly, bad examples will continue to hurt anyway. >> * Operations depending on @props static properties should go into @realize. >> >> Actually, these *have* to go into realize(), because properties get set >> between ::instance_init() and ::realize(). > > Yes; this is using 'should' in the sense "is required to", > not in the RFC2119 sense. We could reasonably tighten the wording > to be clearer though. > >> Even if I had a precise definition of "trivial field initializations", >> I'd still wonder where operations should go that are neither such >> trivial field initializations, nor depend on @props. > > Yes. We should have some kind of design rule-of-thumb, either > "put things in initialize unless they must go in realize", or > "put things in realize unless they must go in initialize". Ideally with an exhaustive list of the "must go" things. Let's try to start with an easy question: what must go into .instance_init()? Here's my guess: * Property default values * Children with properties Please add to this list. >> * After successful realization, setting static properties will fail. >> * >> * As an interim step, the #DeviceState:realized property can also be >> * set with qdev_init_nofail(). >> * In the future, devices will propagate this state change to the
Re: [Qemu-devel] Object instantiation vs. device realization: what to do when?
On 18/02/19 17:01, Markus Armbruster wrote: > > Out of curiosity, what would have to be done? Any particular problems > other than the practical problem of manhandling 300+ qdev_init_nofail() > calls? Well, that's the problem. No one knows what would break, if anything. Paolo
Re: [Qemu-devel] Object instantiation vs. device realization: what to do when?
Paolo Bonzini writes: > On 14/02/19 17:21, Markus Armbruster wrote: >> * As an interim step, the #DeviceState:realized property can also be >> * set with qdev_init_nofail(). >> * In the future, devices will propagate this state change to their children >> * and along busses they expose. >> >> This sentence is five years old. Any progress? If not, any intentions >> to make progress? > > Good news, it's done! What we still don't do is removing > qdev_init_nofail and realizing the whole machine in one fell swoop. > > Bad news, it's been done for five years: > > commit 5c21ce77d7e5643089ceec556c0408445d017f32 > Author: Bandan Das > Date: Wed Mar 12 21:02:12 2014 +0100 > > qdev: Realize buses on device realization > > Integrate (un)realization of child buses with > realization/unrealization > of the device hosting them. Code in device_unparent() is reordered > for unrealization of buses to work as part of device unrealization. > > That way no changes need to be made to bus instantiation. > > Signed-off-by: Bandan Das > Signed-off-by: Andreas Färber > > so I don't expect that the next step will ever happen... Sigh... can you suggest an update to the comment then? Out of curiosity, what would have to be done? Any particular problems other than the practical problem of manhandling 300+ qdev_init_nofail() calls? >> * The point in time will be deferred to machine creation, so that values >> * set in @realize will not be introspectable beforehand. Therefore devices >> * must not create children during @realize; they should initialize them via >> * object_initialize() in their own #TypeInfo.instance_init and forward the >> * realization events appropriately. >> >> This is mostly greek to me. Pity the developer who knows less about >> qdev than I do. > > The first part refers to what virtio_instance_init_common does: > > object_initialize(vdev, vdev_size, vdev_name); > object_property_add_child(proxy_obj, "virtio-backend", OBJECT(vdev), > NULL); > object_unref(OBJECT(vdev)); > > The second part doesn't apply to virtio because it has a bus (so the > code from the above commit handles recursive realization automatically). > > hw/sd/milkymist-memcard.c has an example of this: > > object_property_set_bool(OBJECT(carddev), true, "realized", &err); > > but it should create the device in milkymist_memcard_init rather than > milkymist_memcard_realize, in order to obey the directives of the sacred > book of QEMU. I see. Thanks!
Re: [Qemu-devel] Object instantiation vs. device realization: what to do when?
On 14/02/2019 17.33, Peter Maydell wrote: > On Thu, 14 Feb 2019 at 16:21, Markus Armbruster wrote: >> >> One of qdev's perennial sources of confusion is what to do at object >> instantiation time, i.e. in TypeInfo::instance_init(), and what to do at >> device realization time, i.e. in DeviceClass::realize(). > > Thanks for opening this topic. It's been on my todo list for a > long time to try to figure out what the answer is... I also got no proper answers here, but FWIW, I wrote down some things I noticed last year here: http://people.redhat.com/~thuth/blog/qemu/2018/09/10/instance-init-realize.html Thomas
Re: [Qemu-devel] Object instantiation vs. device realization: what to do when?
On 14/02/19 17:21, Markus Armbruster wrote: > * As an interim step, the #DeviceState:realized property can also be > * set with qdev_init_nofail(). > * In the future, devices will propagate this state change to their children > * and along busses they expose. > > This sentence is five years old. Any progress? If not, any intentions > to make progress? Good news, it's done! What we still don't do is removing qdev_init_nofail and realizing the whole machine in one fell swoop. Bad news, it's been done for five years: commit 5c21ce77d7e5643089ceec556c0408445d017f32 Author: Bandan Das Date: Wed Mar 12 21:02:12 2014 +0100 qdev: Realize buses on device realization Integrate (un)realization of child buses with realization/unrealization of the device hosting them. Code in device_unparent() is reordered for unrealization of buses to work as part of device unrealization. That way no changes need to be made to bus instantiation. Signed-off-by: Bandan Das Signed-off-by: Andreas Färber so I don't expect that the next step will ever happen... > * The point in time will be deferred to machine creation, so that values > * set in @realize will not be introspectable beforehand. Therefore devices > * must not create children during @realize; they should initialize them via > * object_initialize() in their own #TypeInfo.instance_init and forward the > * realization events appropriately. > > This is mostly greek to me. Pity the developer who knows less about > qdev than I do. The first part refers to what virtio_instance_init_common does: object_initialize(vdev, vdev_size, vdev_name); object_property_add_child(proxy_obj, "virtio-backend", OBJECT(vdev), NULL); object_unref(OBJECT(vdev)); The second part doesn't apply to virtio because it has a bus (so the code from the above commit handles recursive realization automatically). hw/sd/milkymist-memcard.c has an example of this: object_property_set_bool(OBJECT(carddev), true, "realized", &err); but it should create the device in milkymist_memcard_init rather than milkymist_memcard_realize, in order to obey the directives of the sacred book of QEMU. Paolo
Re: [Qemu-devel] Object instantiation vs. device realization: what to do when?
On Thu, 14 Feb 2019 at 16:21, Markus Armbruster wrote: > > One of qdev's perennial sources of confusion is what to do at object > instantiation time, i.e. in TypeInfo::instance_init(), and what to do at > device realization time, i.e. in DeviceClass::realize(). Thanks for opening this topic. It's been on my todo list for a long time to try to figure out what the answer is... > qdev-core.h's > comment falls short: > > * # Realization # > * Devices are constructed in two stages, > * 1) object instantiation via object_initialize() and > * 2) device realization via #DeviceState:realized property. > * The former may not fail (and must not abort or exit, since it is called > * during device introspection already), and the latter may return error > * information to the caller and must be re-entrant. > * Trivial field initializations should go into #TypeInfo.instance_init. > > As usual, "trivial" was too trivial to explain; the reader is trusted to > figure it out himself. Well, I'm afraid I'm not to be trusted. > > The easy part is "can fail means it's not trivial", because > ::instance_init() must not fail. > > What about side effects? If you already understand how introspection > works, or perhaps even if you read carefully and paranoidly, then "since > it is called during device introspection already" implies "guest-visible > side-effects would be disastrous". So guest-visible side-effect also > means it's not trivial. I think the other question here is "what about stuff that has to be undone/freed/etc". Can I, for instance, allocate memory in the initialize function? If so, where do I need to put the corresponding memory-free? Similar but harder to figure out: what about things which are ref-counted like memory_region_init()? Can I pass a pointer to 'obj' as the "owner" to memory_region_init() from its initialize function and trust that the refcounting will result in the MR being suitably destroyed when whatever the reverse of initialize happens? Or does this only work for really initialized objects and so has to be postponed to realize time? A lot of our devices get away with shortcuts because (apart from the introspection special case) they're created once at startup and never destroyed, but perhaps we should be stricter about requiring everything to support the full create-and-destroy lifecycle (and testing it)? At least, we should ensure our documentation describes the 'destroy' part of things so that if you are writing a properly destroyable device you can get it right. > * Operations depending on @props static properties should go into @realize. > > Actually, these *have* to go into realize(), because properties get set > between ::instance_init() and ::realize(). Yes; this is using 'should' in the sense "is required to", not in the RFC2119 sense. We could reasonably tighten the wording to be clearer though. > Even if I had a precise definition of "trivial field initializations", > I'd still wonder where operations should go that are neither such > trivial field initializations, nor depend on @props. Yes. We should have some kind of design rule-of-thumb, either "put things in initialize unless they must go in realize", or "put things in realize unless they must go in initialize". > * After successful realization, setting static properties will fail. > * > * As an interim step, the #DeviceState:realized property can also be > * set with qdev_init_nofail(). > * In the future, devices will propagate this state change to their children > * and along busses they expose. > > This sentence is five years old. Any progress? If not, any intentions > to make progress? > > * The point in time will be deferred to machine creation, so that values > * set in @realize will not be introspectable beforehand. Therefore devices > * must not create children during @realize; they should initialize them via > * object_initialize() in their own #TypeInfo.instance_init and forward the > * realization events appropriately. > > This is mostly greek to me. Pity the developer who knows less about > qdev than I do. Also, if you need to create a child based on the value of a static property then you're a bit stuck because you have a rule saying it must be done in realize and also one that says it must not be done in realize. (Not a hypothetical -- see for instance the TYPE_ARMV7M object, which creates the CPU with a type dependent on a QOM property.) > * > * Any type may override the @realize and/or @unrealize callbacks but needs > * to call the parent type's implementation if keeping their functionality > * is desired. Refer to QOM documentation for further discussion and examples. > > "Refer to QOM documentation"... okay, but this comment needs to make > sense without having to read the 3000+ lines under include/qom/ (which > by the way requires finding it first; I'd expect the naive reader to > look under docs/ and draw a blank). The parent_realize mechanism is also very clunky... than
