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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 1/7/25 08:50, Xiaoyao Li wrote:
> On 6/30/2025 11:22 PM, Zhao Liu wrote:
>> (cc Thomas for bug reporting on kvm-unit-test...)
>>
>> On Tue, Jun 24, 2025 at 04:57:21PM +0800, Zhao Liu wrote:
>>> Date: Tue, 24 Jun 2025 16:57:21 +0800
>>> From: Zhao Liu <zhao1.liu@intel.com>
>>> Subject: Re: [Regression] Re: [PULL 35/35] qom: reverse order of
>>>   instance_post_init calls
>>>
>>> On Mon, Jun 23, 2025 at 09:56:14AM -0700, Dongli Zhang wrote:
>>>> Date: Mon, 23 Jun 2025 09:56:14 -0700
>>>> From: Dongli Zhang <dongli.zhang@oracle.com>
>>>> Subject: [Regression] Re: [PULL 35/35] qom: reverse order of
>>>>   instance_post_init calls
>>>>
>>>> This commit may broken the "vendor=" configuration.
>>>>
>>>> For instance, the hypervisor CPU vendor is AMD.
>>>>
>>>> I am going to use "-cpu Skylake-Server,vendor=GenuineIntel".
>>>>
>>>>
>>>> Because of the commit, the vendor is still AMD.
>>>>
>>>> [root@vm ~]# cpuid -1 -l 0x0
>>>> CPU:
>>>>     vendor_id = "AuthenticAMD"
>>>>
>>>>
>>>> If I revert this patch, the vendor because the expected Intel.
>>>>
>>>> [root@vm ~]# cpuid -1 -l 0x0
>>>> CPU:
>>>>     vendor_id = "GenuineIntel"
>>>>
>>>>
>>>> Thank you very much!
>>>
>>> Thank you Dongli!
>>>
>>> (+Like)
>>>
>>> While testing my cache model series, I also noticed the similar behavior
>>> for KVM. Additionally, Like Xu reported to me that this commit caused
>>> a failure in a KVM unit test case. Your report helped me connect these
>>> two issues I met (though due to my environment issues, I haven't
>>> confirmed yet).
>>
>> Ok, now I can confirm this commit cause KUT failure:
>>   * On AMD platform, the "msr.flat" case fails since this case requires
>>     vendor=GenuineIntel (tested by Like).
>>   * On Intel platform, the "syscall.flat" case fails because it requires
>>     vendor=AuthenticAMD (tested by myself).
>>
>>> The "vendor" property from cli is registered as the global property in
>>> x86_cpu_parse_featurestr(), and is applied to x86 CPUs in
>>> device_post_init().
>>>
>>> With this commit, now KVM will override the "vendor" in
>>> host_cpu_instance_init() (called in x86_cpu_post_initfn()) after
>>> device_post_init(), regardless the previous global "vendor" property.
>>
>> This is the root cause for the above failure.
>>
>>> Back to this commit, I think current order of post_init  makes sense.
>>> Instead, the place of host_cpu_instance_init() doesn't seem quite
>>> right. So, I think this commit might have exposed some drawbacks in the
>>> previous x86 CPU initialization order:
>>>
>>> f5cc5a5c1686 ("i386: split cpu accelerators from cpu.c, using 
>>> AccelCPUClass")
>>> 5b8978d80426 ("i386: do not call cpudef-only models functions for 
>>> max, host, base")
>>
>> To fix this issue, we need to initialize "vendor" property in the initfn
>> of max/host/named CPUs instead of current post_initfn.
>>
>> This will need to split the cpu_instance_init() of x86 kvm (and maybe 
>> hvf/tcg)
>> into 2 hooks:
>>   * AccelCPUClass.cpu_instance_init() - called in x86 CPUs' initfn.
>>   * AccelCPUClass.cpu_instance_post_init() - called in x86 CPUs'
>>     post_initfn.
> 
> Split accel.cpu_instance_init() into cpu's instance_init() and 
> post_instance_init() does not seem right way to go.

Yeah, please don't. I'm trying to consolidate this code but it takes
(too) long.

> The reason .post_instance_init() was implemented and put 
> accel_cpu_instance_init() in it for x86 cpu was that, we don't want to 
> scatter acceletor specific instance_init operation into different 
> subclass of x86 cpu (max/host/named cpu model).
> 
> I think something like below should be enough.
> 
> -----------8<-------------
> Author: Xiaoyao Li <xiaoyao.li@intel.com>
> Date:   Tue Jul 1 13:33:43 2025 +0800
> 
>      i386/cpu: Re-apply the global props as the last step of post_init
> 
>      Commit 220c739903ce ("qom: reverse order of instance_post_init calls")
>      reverses the order instance_post_init calls, which leads to
>      device_post_init() called before x86 cpu 
> specific .instance_post_init().
> 
>      However, x86 cpu replies on qdev_prop_set_globals() (inside
>      device_post_init()) to apply the cpu option like "feature[=foo]" 
> passed
>      via '-cpu' as the last step to make the '-cpu' option highest 
> priority.
> 
>      After the order change of .instance_post_init(), x86_cpu_post_initfn()
>      is called after device_post_init(), and it will change some property
>      value even though "-cpu" option specify a different one.
> 
>      Re-apply the global props as the last step to ensure "-cpu" option
>      always takes highest priority.
> 
>      Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> 
> 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.

Regards,

Phil.