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....@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.zh...@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.
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...@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...@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));
}
static void x86_cpu_init_default_topo(X86CPU *cpu)
