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

[Paolo Bonzini <pbonzini@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2699 bytes --]

Il mer 2 lug 2025, 23:36 Xiaoyao Li <xiaoyao.li@intel.com> ha scritto:

> 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")
>

You're right and this can be a problem with the simple split that Zhao
proposed. But the root cause is that post_init is confusing many kinds of
defaults (the KVM vendor case, globals which are different for compat
properties and -global and which CPUs also abuse to implement -cpu by the
way, max_features handling, maybe more; all of which have different
priorities).

TDX added checks on top, and instance_post_init worked when applying class
defaults but not for checks. But as mentioned in the commit message for
this patch, cxl_dsp_instance_post_init and
rp_instance_post_init have similar issues so reverting is also incorrect.

Maybe DeviceClass needs another callback that is called before Device's own
instance_post_init. The accelerator could use it.

Or maybe, more long term, instance_post_init could be replaced by a set of
Notifiers that are registered by instance_init and that have a priority
(FIXUP_CLASS_DEFAULTS, APPLY_GLOBAL_DEFAULTS, and a third for TDX).

Paolo

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.
>
>
>

[-- Attachment #2: Type: text/html, Size: 4042 bytes --]