[PATCH] qom: reverse order of instance_post_init calls

[PATCH] qom: reverse order of instance_post_init calls

[Paolo Bonzini]

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);
+    }
 }
 
 bool object_apply_global_props(Object *obj, const GPtrArray *props,
-- 
2.48.1

Re: [PATCH] qom: reverse order of instance_post_init calls

[Philippe Mathieu-Daudé]

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.

Re: [PATCH] qom: reverse order of instance_post_init calls

[Peter Maydell]

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

Re: [PATCH] qom: reverse order of instance_post_init calls

[Paolo Bonzini]

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