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

[Xiaoyao Li <xiaoyao.li@intel.com>]

On 7/2/2025 3:56 PM, Zhao Liu wrote:
>>> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
>>> index 0d35e95430fe..bf290262cbfe 100644
>>> --- a/target/i386/cpu.c
>>> +++ b/target/i386/cpu.c
>>> @@ -9044,6 +9044,12 @@ static void x86_cpu_post_initfn(Object *obj)
>>>                X86_CONFIDENTIAL_GUEST(current_machine->cgs), (CPU(obj)));
>>>        }
>>>    #endif
>>> +
>>> +    /*
>>> +     * Re-apply the "feature[=foo]" from '-cpu' option since they might
>>> +     * be overwritten by above
>>> +     */
>>> +    qdev_prop_set_globals(DEVICE(obj));
>>>    }
>>
>> This patch LGTM.
> 
> This solution will call qdev_prop_set_globals() twice. My concern is
> that this masks the problem: previous x86 CPU assumptions about the
> order of global property initialization break down...
> 
> Per the commit message of Paolo's commit:
> 
> "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."

After checking the history of why .post_instance_init() was introduced 
in the first place: 8231c2dd2203 ("qom: Introduce instance_post_init 
hook"). It turns out to be used for qdev_prop_set_globals(), to ensure 
global properties being applied after all sub-device specific 
instance_init() were called. And I think the order from child to parent 
was defined purposely.

And the reverse of Paolo's patch breaks the usage of "-global" than it 
won't take effect if the sub-device changes the property in its 
post_instance_init() callback.

Back to Paolo's example of "a compat property will be set on a device 
*after* the class's post_init callback has run". I think the behavior of 
compat property is applied after the class's post_init callback is also 
what we want. If reversing the order, then compat prop can be 
overwritten by subclass's post_init callback, and doesn't it fail the 
purpose of compat prop?

So I think we might revert this patch, and document clearly the reverse 
order of .post_instance_init() callback.

> X86 CPUs have the issue (e.g., "vendor" doesn't work) now because
> they - as leaf class, don't care about the property values of
> superclass - the DeviceState. If a property is just for initialization,
> like "vendor", it should be placed in the instance_init() instead of
> instance_post_init().
> 
> In addition, if other places handle it similarly, the device's
> post_init seems pointless. :-(
> 
> Thanks,
> Zhao
>