Re: arm_cpu_post_init (Was: Re: arm: "max" CPU class hierarchy changes possible?)

[Claudio Fontana]

Hi Markus,

could you help me untangle the arm_cpu_post_init question?

I am trying to cleanup a bit the initialization path for ARM,
and it seems that arm_cpu_post_init is called numerous times for AArch64 in 
particular,

while for "tcg cpus", 32bit it is called only once.

Any reason for the multiple calls in the hierarchy?
Was the intention to actually call this just once from the final leaf classes?

The ability to execute code after the initialization would come in handy in an 
ARM CPU class refactoring I am doing,
but I stopped short of adding anything to arm_cpu_post_init since I noticed the 
inconsistencies.

Thanks,

Claudio


On 3/18/21 12:06 PM, Claudio Fontana wrote:
> On 3/11/21 8:10 PM, Andrew Jones wrote:
>> On Thu, Mar 11, 2021 at 06:33:15PM +0000, Peter Maydell wrote:
>>> On Thu, 11 Mar 2021 at 17:16, Claudio Fontana <cfontana@suse.de> wrote:
>>>> Maybe Peter you could clarify similarly what the intended meaning of "max" 
>>>> is on ARM?
>>>
>>> "max" is "best we can do, whatever that is". (On KVM this is "same as
>>> the host".)
>>> "host" is "whatever the host is (KVM only)".
>>>
>>>> KVM: (aarch64-only): aarch64_max_initfn():
>>>>
>>>> The following comment in the code seems wrong to me:
>>>>
>>>> /* -cpu max: if KVM is enabled, like -cpu host (best possible with this 
>>>> host); */
>>>>
>>>> This is not exactly true:
>>>>
>>>> "-cpu max" calls kvm_arm_set_cpu_features_from_host(), (which checks 
>>>> "dtb_compatible", and if not set gets the features from the host, if set 
>>>> ...?)
>>>> After that, calls aarch64_add_sve_properties() and then adds also 
>>>> "svw-max-vq". This code is common with TCG.
> 
> 
> As part of this research I noticed that arm_cpu_post_init() is quite 
> confusing, seems really inconsistent to me.
> 
> Apparently the intention was to call it from the leaf classes:
> 
> commit 51e5ef459eca045d7e8afe880ee60190f0b75b26
> Author: Marc-André Lureau <marcandre.lureau@redhat.com>
> Date:   Tue Nov 27 12:55:59 2018 +0400
> 
>     arm: replace instance_post_init()
>     
>     Replace arm_cpu_post_init() instance callback by calling it from leaf
>     classes, to avoid potential ordering issue with other post_init callbacks.
>     
>     Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>     Suggested-by: Igor Mammedov <imammedo@redhat.com>
>     Reviewed-by: Igor Mammedov <imammedo@redhat.com>
>     Acked-by: Eduardo Habkost <ehabkost@redhat.com>
> 
> 
> but then we end up calling it multiple times in the class hierarch, which is 
> a recipe for bugs, and makes it difficult to understand what 
> arm_cpu_post_init()
> even means, what calling this function is supposed to do.
> 
> For a "max" or "host" cpu on AArch64, this function is called:
> 
> for the ARM CPU base class, TYPE_ARM_CPU, in
> 
> cpu.c::arm_cpu_instance_init,
> 
> then later again for the TYPE_AARCH64_CPU class, child of TYPE_ARM_CPU, in
> 
> cpu64.c::aarch64_cpu_instance_init,
> 
> then later again for the TYPE_ARM_HOST_CPU class, child of TYPE_AARCH64_CPU, 
> in
> 
> cpu.c::arm_host_initfn.
> 
> Same for "max".
> 
> When looking at 32bit CPUs instead, only the ARM CPU base class ends up 
> calling arm_cpu_post_init.
> "Leaf" classes do not do it (see cpu_tcg.c).
> 
> What is then arm_cpu_post_init even supposed to mean?
> 
> Thanks,
> 
> Claudio
> 
> 
>>>>
>>>> In the case of cpu host instead,
>>>>
>>>> "-cpu host" calls kvm_arm_set_cpu_features_from_host(), same as max, then 
>>>> calls aarch64_add_sve_properties() but does NOT add "svw-max-vq".
>>>>
>>>> Is this a bug?
>>
>> It was left out intentionally. More below.
>>
>>>
>>> Maybe; that's a question for Richard or Drew...
>>>
>>>> Are "max" and "host" for KVM supposed to be the same like with x86?
>>
>> Yes, but my understanding of "max" == "host" for KVM is that that only
>> applies to the perspective of the guest. What CPU and what CPU features
>> the guest can see should be exactly the same with either "max" or "host",
>> depending on the enabling/disabling of any optional CPU properties.
>>
>> The question here seems to be that, if one has a CPU property, does that
>> imply the other should have the same? Which would effectively allow the
>> two to be aliases (when KVM is enabled). I don't know, does x86 ensure
>> 100% property compatibility?
>>
>> I opted not to support sve-max-vq for "host" because I consider it a
>> legacy CPU property, one I didn't want to propagate. Indeed it may
>> make more sense to depreciate sve-max-vq than to "fix" this issue
>> by adding it to "host". Note, we can already create equivalent SVE
>> CPUs. The following are the same from the perspective of the guest
>>
>>  -accel kvm -cpu host,sve512=on
>>  -accel kvm -cpu max,sve512=on
>>
>> And, for TCG, these are the same from the perspective of the guest
>>  
>>  -accel tcg -cpu max,sve512=on
>>  -accel tcg -cpu max,sve-max-vq=4
>>
>> So we already don't need sve-max-vq.
>>
>> Thanks,
>> drew
>>
> 
>