Il gio 3 lug 2025, 06:51 Paolo Bonzini <pbonz...@redhat.com> ha scritto:
>
>
> Il mer 2 lug 2025, 23:36 Xiaoyao Li <xiaoyao...@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).
>
I checked and it seems to me that all of the accelerator-specific
initialization should be doable in instance_init (which risc-v already
does), the only problem is the max_features field. But max_features is
*not* a qdev property so it can be moved to the class side.
I will try to send patches tomorrow.
Paolo
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.
>>
>>
>>
