arm: "max" CPU class hierarchy changes possible?

arm: "max" CPU class hierarchy changes possible?

[Claudio Fontana]

Hi Peter,

the "max" cpu in x86 and s390 is a class,

and then "host" has "max" as parent.

This would be a convenient setup for ARM too, as it would allow to put common code between kvm and tcg in the "max" class,
and allow "host" to specialize the behavior for KVM (and in the future HVF probably).

Would changing the class hierarchy this way be acceptable, cause any problems?

I ask specifically because on the x86 side I had to limit my changes to the hierarchy to adding objects,
but I would not change the relationship between them.

Thanks,

Claudio

-- 
Claudio Fontana
Engineering Manager Virtualization, SUSE Labs Core

SUSE Software Solutions Italy Srl

Re: arm: "max" CPU class hierarchy changes possible?

[Peter Maydell]

On Thu, 11 Mar 2021 at 14:27, Claudio Fontana <cfontana@suse.de> wrote:
> the "max" cpu in x86 and s390 is a class,
>
> and then "host" has "max" as parent.
>
> This would be a convenient setup for ARM too, as it would allow to put common code between kvm and tcg in the "max" class,
> and allow "host" to specialize the behavior for KVM (and in the future HVF probably).
>
> Would changing the class hierarchy this way be acceptable, cause any problems?

It's not clear to me why 'host' would be a subtype of 'max':
that doesn't seem like an obvious relationship.

thanks
-- PMM

Re: arm: "max" CPU class hierarchy changes possible?

[Claudio Fontana]

On 3/11/21 4:02 PM, Peter Maydell wrote:
> On Thu, 11 Mar 2021 at 14:27, Claudio Fontana <cfontana@suse.de> wrote:
>> the "max" cpu in x86 and s390 is a class,
>>
>> and then "host" has "max" as parent.
>>
>> This would be a convenient setup for ARM too, as it would allow to put common code between kvm and tcg in the "max" class,
>> and allow "host" to specialize the behavior for KVM (and in the future HVF probably).
>>
>> Would changing the class hierarchy this way be acceptable, cause any problems?
> 
> It's not clear to me why 'host' would be a subtype of 'max':
> that doesn't seem like an obvious relationship.
> 
> thanks
> -- PMM


If there is no blocker on _any_ change to the hierarchy I will put it in the RFC series,
so we can discuss the merits there and investigate alternatives,
if there is no immediate blocker to any change in the object hierarchy.

https://lists.gnu.org/archive/html/qemu-devel/2021-03/msg03864.html

Thanks,

CLaudio

Re: arm: "max" CPU class hierarchy changes possible?

[Peter Maydell]

On Thu, 11 Mar 2021 at 15:21, Claudio Fontana <cfontana@suse.de> wrote:
> If there is no blocker on _any_ change to the hierarchy I will put it in the RFC series,
> so we can discuss the merits there and investigate alternatives,
> if there is no immediate blocker to any change in the object hierarchy.

Well, the blocker is lack of justification. The right place for
"common code between kvm and tcg" is in the base class TYPE_ARM_CPU.

thanks
-- PMM

Re: arm: "max" CPU class hierarchy changes possible?

[Paolo Bonzini]

On 11/03/21 16:02, Peter Maydell wrote:
> On Thu, 11 Mar 2021 at 14:27, Claudio Fontana <cfontana@suse.de> wrote:
>> the "max" cpu in x86 and s390 is a class,
>>
>> and then "host" has "max" as parent.
>>
>> This would be a convenient setup for ARM too, as it would allow to put common code between kvm and tcg in the "max" class,
>> and allow "host" to specialize the behavior for KVM (and in the future HVF probably).
>>
>> Would changing the class hierarchy this way be acceptable, cause any problems?
> 
> It's not clear to me why 'host' would be a subtype of 'max':
> that doesn't seem like an obvious relationship.

On x86, "-cpu host" is essentially the same as "-cpu max" with the only 
difference that it errors out on TCG.  So:

- with TCG: "-cpu max" enables all that can be emulated, "-cpu host" fails

- with KVM: "-cpu max" enables all that can be virtualized, "-cpu host" 
does the same

Paolo

Re: arm: "max" CPU class hierarchy changes possible?

[Claudio Fontana]

On 3/11/21 5:18 PM, Paolo Bonzini wrote:
> On 11/03/21 16:02, Peter Maydell wrote:
>> On Thu, 11 Mar 2021 at 14:27, Claudio Fontana <cfontana@suse.de> wrote:
>>> the "max" cpu in x86 and s390 is a class,
>>>
>>> and then "host" has "max" as parent.
>>>
>>> This would be a convenient setup for ARM too, as it would allow to put common code between kvm and tcg in the "max" class,
>>> and allow "host" to specialize the behavior for KVM (and in the future HVF probably).
>>>
>>> Would changing the class hierarchy this way be acceptable, cause any problems?
>>
>> It's not clear to me why 'host' would be a subtype of 'max':
>> that doesn't seem like an obvious relationship.
> 
> On x86, "-cpu host" is essentially the same as "-cpu max" with the only 
> difference that it errors out on TCG.  So:
> 
> - with TCG: "-cpu max" enables all that can be emulated, "-cpu host" fails
> 
> - with KVM: "-cpu max" enables all that can be virtualized, "-cpu host" 
> does the same
> 
> Paolo
> 

Maybe Peter you could clarify similarly what the intended meaning of "max" is on ARM?

TCG: for aarch64, the starting point seems to be Cortex-A57, and then lots of other features are added on top of it,
and for non-aarch64, the starting point seems to be Cortex-A15, plus "old-style VFP short-vector support".

Is the intention to enable all that can be emulated here too, like for X86?


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.

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?

Are "max" and "host" for KVM supposed to be the same like with x86?

Thanks,

Claudio

Re: arm: "max" CPU class hierarchy changes possible?

[Eduardo Habkost]

On Thu, Mar 11, 2021 at 06:16:52PM +0100, Claudio Fontana wrote:
> On 3/11/21 5:18 PM, Paolo Bonzini wrote:
> > On 11/03/21 16:02, Peter Maydell wrote:
> >> On Thu, 11 Mar 2021 at 14:27, Claudio Fontana <cfontana@suse.de> wrote:
> >>> the "max" cpu in x86 and s390 is a class,
> >>>
> >>> and then "host" has "max" as parent.
> >>>
> >>> This would be a convenient setup for ARM too, as it would allow to put common code between kvm and tcg in the "max" class,
> >>> and allow "host" to specialize the behavior for KVM (and in the future HVF probably).
> >>>
> >>> Would changing the class hierarchy this way be acceptable, cause any problems?
> >>
> >> It's not clear to me why 'host' would be a subtype of 'max':
> >> that doesn't seem like an obvious relationship.
> > 
> > On x86, "-cpu host" is essentially the same as "-cpu max" with the only 
> > difference that it errors out on TCG.  So:
> > 
> > - with TCG: "-cpu max" enables all that can be emulated, "-cpu host" fails
> > 
> > - with KVM: "-cpu max" enables all that can be virtualized, "-cpu host" 
> > does the same
> > 
> > Paolo
> > 
> 
> Maybe Peter you could clarify similarly what the intended meaning of "max" is on ARM?
> 
> TCG: for aarch64, the starting point seems to be Cortex-A57, and then lots of other features are added on top of it,
> and for non-aarch64, the starting point seems to be Cortex-A15, plus "old-style VFP short-vector support".
> 
> Is the intention to enable all that can be emulated here too, like for X86?
> 
> 
> 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.
> 
> 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?
> 
> Are "max" and "host" for KVM supposed to be the same like with x86?

Note that even on x86 "max" and "host" are allowed to be
different, because they have different purposes.

This can happen if a feature is supported by the host software
and hardware (so it will be included in "max") but can't be
enabled by default on "host" for some reason (e.g. if a feature
doesn't work out of the box without extra configuration options).

-- 
Eduardo

Re: arm: "max" CPU class hierarchy changes possible?

[Peter Maydell]

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

Maybe; that's a question for Richard or Drew...

> Are "max" and "host" for KVM supposed to be the same like with x86?

Yes.

thanks
-- PMM

Re: arm: "max" CPU class hierarchy changes possible?

[Andrew Jones]

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

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

[Claudio Fontana]

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
> 

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

[Claudio Fontana]

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?


And why do we have a separate arm_cpu_finalize_features()?

Nothing in the ARM cpu classes initializations ever seems to be "final" to me.



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

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

[Andrew Jones]

On Thu, Mar 18, 2021 at 12:32:30PM +0100, Claudio Fontana wrote:
> And why do we have a separate arm_cpu_finalize_features()?

Separate, because it's not just called from arm_cpu_realizefn().

> 
> Nothing in the ARM cpu classes initializations ever seems to be "final" to me.

Some CPU features cannot be simply switched on/off at the property
parse time. For example, there could be dependencies on multiple
properties, the mutual exclusion of properties, or other aspects
that can only be known later than property parse time. That stuff
goes in arm_cpu_finalize_features().

Thanks,
drew

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

[Claudio Fontana]

On 3/18/21 1:08 PM, Andrew Jones wrote:
> On Thu, Mar 18, 2021 at 12:32:30PM +0100, Claudio Fontana wrote:
>> And why do we have a separate arm_cpu_finalize_features()?
> 
> Separate, because it's not just called from arm_cpu_realizefn().

In particular it is also called by the monitor.c in qmp_query_cpu_model_expansion(),

which basically creates an object of the cpu subclass,
and then calls arm_cpu_finalize_[features]() explicitly on the object.

Is the qdev realize() method not called in this case? Should instead it be triggered, rather than initializing/realizing an incomplete object?

> 
>>
>> Nothing in the ARM cpu classes initializations ever seems to be "final" to me.
> 
> Some CPU features cannot be simply switched on/off at the property
> parse time. For example, there could be dependencies on multiple
> properties, the mutual exclusion of properties, or other aspects
> that can only be known later than property parse time. That stuff
> goes in arm_cpu_finalize_features().

Seems like _part_ of that is in arm_cpu_finalize_[features]() (in practice, this ends up being AARCH64-only stuff,
ie SVE, PAUTH and KVM).

After calling that, the arm realizefn() also does further setting and unsetting of features, checking previous feature states.

There is a whole lot following the arm_cpu_finalize_[features]() call,
there are ~300 lines of features initializations happening _after_ the call to arm_cpu_finalize_[features]().

> 
> Thanks,
> drew
> 

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

[Andrew Jones]

On Thu, Mar 18, 2021 at 01:42:36PM +0100, Claudio Fontana wrote:
> On 3/18/21 1:08 PM, Andrew Jones wrote:
> > On Thu, Mar 18, 2021 at 12:32:30PM +0100, Claudio Fontana wrote:
> >> And why do we have a separate arm_cpu_finalize_features()?
> > 
> > Separate, because it's not just called from arm_cpu_realizefn().
> 
> In particular it is also called by the monitor.c in qmp_query_cpu_model_expansion(),
> 
> which basically creates an object of the cpu subclass,
> and then calls arm_cpu_finalize_[features]() explicitly on the object.
> 
> Is the qdev realize() method not called in this case? Should instead it be triggered, rather than initializing/realizing an incomplete object?

Can you elaborate on what you mean by "triggered"? The QMP query does the
least that it can get away with while still reusing the CPU model's
feature initialization code. Any suggestions for improving that,
preferably in the form of a patch, would be welcome. If it works well for
Arm, then it could probably be applied to other architectures. The Arm QMP
query is modeled off the others.

> 
> > 
> >>
> >> Nothing in the ARM cpu classes initializations ever seems to be "final" to me.
> > 
> > Some CPU features cannot be simply switched on/off at the property
> > parse time. For example, there could be dependencies on multiple
> > properties, the mutual exclusion of properties, or other aspects
> > that can only be known later than property parse time. That stuff
> > goes in arm_cpu_finalize_features().
> 
> Seems like _part_ of that is in arm_cpu_finalize_[features]() (in practice, this ends up being AARCH64-only stuff,
> ie SVE, PAUTH and KVM).
> 
> After calling that, the arm realizefn() also does further setting and unsetting of features, checking previous feature states.
> 
> There is a whole lot following the arm_cpu_finalize_[features]() call,
> there are ~300 lines of features initializations happening _after_ the call to arm_cpu_finalize_[features]().
>

Any feature that needs a finalizer (i.e. it can't be managed at property
parsing time) and wants to be probable by the QMP query will need to have
its finalizing moved to arm_cpu_finalize_features(). Some of those ~300
lines might be better pushed into property set/get handlers, others will
never want to be advertised in the QMP query, but the rest will get moved
when the need is great enough.

Thanks,
drew

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

[Eduardo Habkost]

On Thu, Mar 18, 2021 at 01:59:08PM +0100, Andrew Jones wrote:
> On Thu, Mar 18, 2021 at 01:42:36PM +0100, Claudio Fontana wrote:
> > On 3/18/21 1:08 PM, Andrew Jones wrote:
> > > On Thu, Mar 18, 2021 at 12:32:30PM +0100, Claudio Fontana wrote:
> > >> And why do we have a separate arm_cpu_finalize_features()?
> > > 
> > > Separate, because it's not just called from arm_cpu_realizefn().
> > 
> > In particular it is also called by the monitor.c in qmp_query_cpu_model_expansion(),
> > 
> > which basically creates an object of the cpu subclass,
> > and then calls arm_cpu_finalize_[features]() explicitly on the object.
> > 
> > Is the qdev realize() method not called in this case? Should instead it be triggered, rather than initializing/realizing an incomplete object?
> 
> Can you elaborate on what you mean by "triggered"? The QMP query does the
> least that it can get away with while still reusing the CPU model's
> feature initialization code. Any suggestions for improving that,
> preferably in the form of a patch, would be welcome. If it works well for
> Arm, then it could probably be applied to other architectures. The Arm QMP
> query is modeled off the others.

This sound very similar to x86_cpu_expand_features(), so the
approach makes sense to me.

It wouldn't make sense to call realize() inside
qmp_query_cpu_model_expansion().  Realizing the CPU means
plugging it into the guest, and we would never want to do that
when executing a query command.

-- 
Eduardo

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

[Claudio Fontana]

On 3/18/21 2:10 PM, Eduardo Habkost wrote:
> On Thu, Mar 18, 2021 at 01:59:08PM +0100, Andrew Jones wrote:
>> On Thu, Mar 18, 2021 at 01:42:36PM +0100, Claudio Fontana wrote:
>>> On 3/18/21 1:08 PM, Andrew Jones wrote:
>>>> On Thu, Mar 18, 2021 at 12:32:30PM +0100, Claudio Fontana wrote:
>>>>> And why do we have a separate arm_cpu_finalize_features()?
>>>>
>>>> Separate, because it's not just called from arm_cpu_realizefn().
>>>
>>> In particular it is also called by the monitor.c in qmp_query_cpu_model_expansion(),
>>>
>>> which basically creates an object of the cpu subclass,
>>> and then calls arm_cpu_finalize_[features]() explicitly on the object.
>>>
>>> Is the qdev realize() method not called in this case? Should instead it be triggered, rather than initializing/realizing an incomplete object?
>>
>> Can you elaborate on what you mean by "triggered"? The QMP query does the
>> least that it can get away with while still reusing the CPU model's
>> feature initialization code. Any suggestions for improving that,
>> preferably in the form of a patch, would be welcome. If it works well for
>> Arm, then it could probably be applied to other architectures. The Arm QMP
>> query is modeled off the others.
> 
> This sound very similar to x86_cpu_expand_features(), so the
> approach makes sense to me.

Interesting, to me it sounds like a CPUClass method is hiding here, cc->cpu_expand_features(),
I could help kickstart the implementation but would need a good description / comment of exactly which features are supposed to be expanded there. 

> 
> It wouldn't make sense to call realize() inside
> qmp_query_cpu_model_expansion().  Realizing the CPU means
> plugging it into the guest, and we would never want to do that
> when executing a query command.
> 

Makes sense, thanks for the explanation.

Ciao,

Claudio

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

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

[Claudio Fontana]

On 3/19/21 9:23 AM, Claudio Fontana wrote:
> Hi Markus,
> 
> could you help me untangle the arm_cpu_post_init question?

Nevermind, I think I figured it out. The arm_cpu_post_init are indeed called only for the "leaf" class,
via the "instance_init" functions.

I think I can use it to do things reliably "post init" for all classes in there.

Thanks,

Claudio

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