| Message ID | 20250203114132.259155-1-pbonzini@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | qom: reverse order of instance_post_init calls | expand |
Hi Paolo, On 3/2/25 12:41, Paolo Bonzini wrote: > Currently, the instance_post_init calls are performed from the leaf > class and all the way up to Object. This is incorrect because the > leaf class cannot observe property values applied by the superclasses; > for example, a compat property will be set on a device *after* > the class's post_init callback has run. > > In particular this makes it impossible for implementations of > accel_cpu_instance_init() to operate based on the actual values of > the properties, though it seems that cxl_dsp_instance_post_init and > rp_instance_post_init might have similar issues. > > Follow instead the same order as instance_init, starting with Object > and running the child class's instance_post_init after the parent. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > qom/object.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/qom/object.c b/qom/object.c > index 157a45c5f8b..c03cd3c7339 100644 > --- a/qom/object.c > +++ b/qom/object.c > @@ -423,13 +423,13 @@ static void object_init_with_type(Object *obj, TypeImpl *ti) > > static void object_post_init_with_type(Object *obj, TypeImpl *ti) > { > - if (ti->instance_post_init) { > - ti->instance_post_init(obj); > - } > - > if (type_has_parent(ti)) { > object_post_init_with_type(obj, type_get_parent(ti)); > } > + > + if (ti->instance_post_init) { > + ti->instance_post_init(obj); > + } > } I'm not opposed to this change as I had a similar issue there few weeks ago, but I feel we are changing one problem by another. IIRC some class post_init() handlers check the instance correctly did something. But I don't recall any example in particular. The documentation isn't clear about order (include/qom/object.h): * @instance_post_init: This function is called to finish * initialization of an object, after * all @instance_init functions were * called.
On Tue, 4 Feb 2025 at 15:08, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > > Hi Paolo, > > On 3/2/25 12:41, Paolo Bonzini wrote: > > Currently, the instance_post_init calls are performed from the leaf > > class and all the way up to Object. This is incorrect because the > > leaf class cannot observe property values applied by the superclasses; > > for example, a compat property will be set on a device *after* > > the class's post_init callback has run. > > > > In particular this makes it impossible for implementations of > > accel_cpu_instance_init() to operate based on the actual values of > > the properties, though it seems that cxl_dsp_instance_post_init and > > rp_instance_post_init might have similar issues. > > > > Follow instead the same order as instance_init, starting with Object > > and running the child class's instance_post_init after the parent. > > > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > > --- > > qom/object.c | 8 ++++---- > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/qom/object.c b/qom/object.c > > index 157a45c5f8b..c03cd3c7339 100644 > > --- a/qom/object.c > > +++ b/qom/object.c > > @@ -423,13 +423,13 @@ static void object_init_with_type(Object *obj, TypeImpl *ti) > > > > static void object_post_init_with_type(Object *obj, TypeImpl *ti) > > { > > - if (ti->instance_post_init) { > > - ti->instance_post_init(obj); > > - } > > - > > if (type_has_parent(ti)) { > > object_post_init_with_type(obj, type_get_parent(ti)); > > } > > + > > + if (ti->instance_post_init) { > > + ti->instance_post_init(obj); > > + } > > } > > I'm not opposed to this change as I had a similar issue there few weeks > ago, but I feel we are changing one problem by another. IIRC some class > post_init() handlers check the instance correctly did something. But I > don't recall any example in particular. The documentation isn't clear > about order (include/qom/object.h): > > * @instance_post_init: This function is called to finish > * initialization of an object, after > * all @instance_init functions were > * called. We have five users of instance_post_init in the tree, if I'm not miscounting. So we should be able to audit them all for whether they care about the order and/or are currently doing things in the wrong order. And yes, we should update the documentation if we're picking a specific ordering :-) thanks -- PMM
On 2/4/25 16:08, Philippe Mathieu-Daudé wrote: > Hi Paolo, > > On 3/2/25 12:41, Paolo Bonzini wrote: >> Currently, the instance_post_init calls are performed from the leaf >> class and all the way up to Object. This is incorrect because the >> leaf class cannot observe property values applied by the superclasses; >> for example, a compat property will be set on a device *after* >> the class's post_init callback has run. >> >> In particular this makes it impossible for implementations of >> accel_cpu_instance_init() to operate based on the actual values of >> the properties, though it seems that cxl_dsp_instance_post_init and >> rp_instance_post_init might have similar issues. > > I'm not opposed to this change as I had a similar issue there few weeks > ago, but I feel we are changing one problem by another. IIRC some class > post_init() handlers check the instance correctly did something. There are five - one does not have any subclass and the other four are all mentioned in the commit message: - x86 and risc-v use accel_cpu_instance_init(), which is where I found the bug - the other two seem broken too > * @instance_post_init: This function is called to finish > * initialization of an object, after > * all @instance_init functions were > * called. Yeah I didn't adjust it because it now is simply the same order as instance_init (and the opposite as instance_finalize). I can change it to "after all @instance_init functions were called, as well as the @instance_post_init functions for the parent classes". Paolo
diff --git a/qom/object.c b/qom/object.c index 157a45c5f8b..c03cd3c7339 100644 --- a/qom/object.c +++ b/qom/object.c @@ -423,13 +423,13 @@ static void object_init_with_type(Object *obj, TypeImpl *ti) static void object_post_init_with_type(Object *obj, TypeImpl *ti) { - if (ti->instance_post_init) { - ti->instance_post_init(obj); - } - if (type_has_parent(ti)) { object_post_init_with_type(obj, type_get_parent(ti)); } + + if (ti->instance_post_init) { + ti->instance_post_init(obj); + } } bool object_apply_global_props(Object *obj, const GPtrArray *props,
Currently, the instance_post_init calls are performed from the leaf class and all the way up to Object. This is incorrect because the leaf class cannot observe property values applied by the superclasses; for example, a compat property will be set on a device *after* the class's post_init callback has run. In particular this makes it impossible for implementations of accel_cpu_instance_init() to operate based on the actual values of the properties, though it seems that cxl_dsp_instance_post_init and rp_instance_post_init might have similar issues. Follow instead the same order as instance_init, starting with Object and running the child class's instance_post_init after the parent. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- qom/object.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)