On 7/3/2025 11:08 AM, Zhao Liu wrote:
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...@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...@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.
The reason why accelerator's instance_init() was moved to post_init, was
just it needs to consider other factors. Please see commit 4db4385a7ab6
("i386: run accel_cpu_instance_init as post_init")
accelerator needs to do different tweak for "max/host", "xcc->model".
Of course we can split it and put specific handling at the end of each
sub-class's instance_init(), like below:
- in TYPE_X86_CPU instance_init()
if (accelerator_kvm) {
kvm_instance_init_common_for_x86_cpu();
}
- in "base" instance_init()
if (accelerator_kvm) {
kvm_instance_init_for_base();
}
- in "max" instance_init()
if (accelerator_kvm) {
kvm_instance_init_for_max();
}
- in "host" instance_init()
if (accelerator_kvm) {
kvm_instance_init_for_host();
}
- in "named cpu model" instance_init()
if (xcc->model) {
kvm_instance_init_for_xcc_model();
}
Contrast to the current implementation in post_init()
if (accelerator_kvm) {
kvm_instance_init_common_for_x86_cpu();
if (base)
...
if (max)
...
if (host)
...
if (xcc->model)
...
}
The reality for the former might be simpler since "base" doesn't have
instance_init(), and "max/host" are same to KVM as "cpu->max_features"
But I still like the latter.
