Re: [Regression] Re: [PULL 35/35] qom: reverse order of instance_post_init calls

[Zhao Liu <zhao1.liu@intel.com>]

On Thu, Jul 03, 2025 at 09:03:10AM +0800, Xiaoyao Li wrote:
> Date: Thu, 3 Jul 2025 09:03:10 +0800
> From: Xiaoyao Li <xiaoyao.li@intel.com>
> Subject: Re: [Regression] Re: [PULL 35/35] qom: reverse order of
>  instance_post_init calls
> 
> On 7/3/2025 2:54 AM, Paolo Bonzini wrote:
> > Il mer 2 lug 2025, 09:25 Xiaoyao Li <xiaoyao.li@intel.com> ha scritto:
> > 
> > > IIRC that's on rhel QEMU which ports the TDX code before it's merged
> > > upstream. Now TDX is upstreamed, it works with upstream compat property
> > > and I think future new compat property won't affect TDX or anything,
> > > since it's compat property and it's to guarantee the existing behavior
> > > when introducing new behavior?
> > > 
> > 
> > It's a compat property that is only added by RHEL-specific machine types.
> > But the bug is not specific to RHEL, it just happens that no upstream
> > machine type has compat properties that overlap with TDX adjustments of
> > CPUID.
> > 
> > > In general I don't see how the reverse order makes sense: the subclass
> > > > knows what the superclass does, so it can do the right thing if it runs
> > > > last; but the superclass cannot know what all of its subclasses do in
> > > > post_init, so it makes less sense to run it last.
> > > 
> > > I agree in general the parent to child order makes more sense,
> > > especially when we treat .instance_init() as the phase 1 init and
> > > .post_instance_init() as the phase 2 init.
> > > 
> > > But the original purpose of introducing .post_instance_init() was to
> > > ensure qdev_prop_set_globals() is called at last for Device. Reverse the
> > > order breaks this purpose.
> > > 
> > 
> > Not "last", but "after instance_init". Anything that happens in the child
> > class's instance_post_init can be moved at the end of instance_init, just
> > like the refactoring that I did for risc-v.
> 
> Move into the end of instance_init() can surely work. But it requires to
> split the code in instance_post_init() to different child's instance_init()
> or making sure the code in instance_post_init() is called at the end of each
> lowest child class.

Initially, when I proposed the split approach, it wasn't about
splitting for the sake of splitting, nor for the sake of "work".

A more granular split is just a means, and the goal is to place things
at different stages in the most appropriate locations.

> Besides, it also leads to a rule that child of Device's
> .post_instance_init() needs to be careful about changing the property or
> anything that might affect the property,

I believe that's how things should be. instance_post_init() provides an
opportunity to tweak properties. The order of instance_post_init()
reflects the dependency relationships for property adjustments. As I
mentioned earlier, if a property doesn't need to consider other factors
and is simply being initialized, instance_init() is the most appropriate
place for it.

> because it might break the usage of global properties.

Breaking global properties is just one example. Essentially, properties
like "vendor" don't adhere well to the semantics of QOM.

> This can surely work. But to me, it seems to make code worse not better.

IMO, it's not the split that makes things worse, but rather the improper
use of instance_post_init() that makes everything about the x86 CPU
fragile. Ideally, QOM/qdev should focus on their own abstraction order,
while the leaf-class should do the right thing in the right place, which
is the most solid situation.