[PATCH] target/loongarch: Add full type support for query-cpu-model-expansion

[PATCH] target/loongarch: Add full type support for query-cpu-model-expansion

[Bibo Mao]

With full type for query-cpu-model-expansion qmp command, it shows that
it is not supported. For instance,
  query-cpu-model-expansion type=full model={"name":"max"}

Here is is output,
  {"error": {"class": "GenericError", "desc": "The requested expansion type is not supported"}}

Since full type is not supported and only static type is supported, Here
replace static type with full type for command query-cpu-model-expansion.

And there is result with this patch,
  {"return": {"model": {"name": "max", "props": {"lbt": true, "lasx": true, "pmu": true, "lsx": true}}}}

Signed-off-by: Bibo Mao <maobibo@loongson.cn>
---
 target/loongarch/loongarch-qmp-cmds.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/loongarch/loongarch-qmp-cmds.c b/target/loongarch/loongarch-qmp-cmds.c
index 3fde5a5a20..429c6d35fd 100644
--- a/target/loongarch/loongarch-qmp-cmds.c
+++ b/target/loongarch/loongarch-qmp-cmds.c
@@ -56,7 +56,7 @@ CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType type,
     const char *name;
     int i;
 
-    if (type != CPU_MODEL_EXPANSION_TYPE_STATIC) {
+    if (type != CPU_MODEL_EXPANSION_TYPE_FULL) {
         error_setg(errp, "The requested expansion type is not supported");
         return NULL;
     }

base-commit: de278e54aefed143526174335f8286f7437d20be
-- 
2.39.3

Re: [PATCH] target/loongarch: Add full type support for query-cpu-model-expansion

[bibo mao]

please discard this patch :(

libvirt uses static type already on LoongArch platform, there is 
compatibility issue. Static type cannot be removed, although I do
not know the difference between static type and full type.

Regards
Bibo Mao

On 2025/2/13 下午5:16, Bibo Mao wrote:
> With full type for query-cpu-model-expansion qmp command, it shows that
> it is not supported. For instance,
>    query-cpu-model-expansion type=full model={"name":"max"}
> 
> Here is is output,
>    {"error": {"class": "GenericError", "desc": "The requested expansion type is not supported"}}
> 
> Since full type is not supported and only static type is supported, Here
> replace static type with full type for command query-cpu-model-expansion.
> 
> And there is result with this patch,
>    {"return": {"model": {"name": "max", "props": {"lbt": true, "lasx": true, "pmu": true, "lsx": true}}}}
> 
> Signed-off-by: Bibo Mao <maobibo@loongson.cn>
> ---
>   target/loongarch/loongarch-qmp-cmds.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/loongarch/loongarch-qmp-cmds.c b/target/loongarch/loongarch-qmp-cmds.c
> index 3fde5a5a20..429c6d35fd 100644
> --- a/target/loongarch/loongarch-qmp-cmds.c
> +++ b/target/loongarch/loongarch-qmp-cmds.c
> @@ -56,7 +56,7 @@ CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType type,
>       const char *name;
>       int i;
>   
> -    if (type != CPU_MODEL_EXPANSION_TYPE_STATIC) {
> +    if (type != CPU_MODEL_EXPANSION_TYPE_FULL) {
>           error_setg(errp, "The requested expansion type is not supported");
>           return NULL;
>       }
> 
> base-commit: de278e54aefed143526174335f8286f7437d20be
> 

Re: [PATCH] target/loongarch: Add full type support for query-cpu-model-expansion

[Igor Mammedov]

On Thu, 13 Feb 2025 17:40:56 +0800
bibo mao <maobibo@loongson.cn> wrote:

> please discard this patch :(
> 
> libvirt uses static type already on LoongArch platform, there is 
> compatibility issue. Static type cannot be removed, although I do
> not know the difference between static type and full type.

looking at loongarch-qmp-cmds.c:qmp_query_cpu_model_expansion(),
you do filter reported features by global cpu_model_advertised_features[] list.
Also cpu 'definition' is mainly governed by its initfn() and
properties mentioned in cpu_model_advertised_features[] are created
only by loongarch_cpu_post_init() , which is part of la464 and max models.
The later call however mutates depending on accelerator, which
sort of contradicts 'static' description and feets more to 'full' category.

What I'd suggest doing is to support both
i.e. do keep 'static' for compat reasons  and run the same code for 'full'

In nutshell drop if(type) check or explicitly allow both types.

> 
> Regards
> Bibo Mao
> 
> On 2025/2/13 下午5:16, Bibo Mao wrote:
> > With full type for query-cpu-model-expansion qmp command, it shows that
> > it is not supported. For instance,
> >    query-cpu-model-expansion type=full model={"name":"max"}
> > 
> > Here is is output,
> >    {"error": {"class": "GenericError", "desc": "The requested expansion type is not supported"}}
> > 
> > Since full type is not supported and only static type is supported, Here
> > replace static type with full type for command query-cpu-model-expansion.
> > 
> > And there is result with this patch,
> >    {"return": {"model": {"name": "max", "props": {"lbt": true, "lasx": true, "pmu": true, "lsx": true}}}}
> > 
> > Signed-off-by: Bibo Mao <maobibo@loongson.cn>
> > ---
> >   target/loongarch/loongarch-qmp-cmds.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/target/loongarch/loongarch-qmp-cmds.c b/target/loongarch/loongarch-qmp-cmds.c
> > index 3fde5a5a20..429c6d35fd 100644
> > --- a/target/loongarch/loongarch-qmp-cmds.c
> > +++ b/target/loongarch/loongarch-qmp-cmds.c
> > @@ -56,7 +56,7 @@ CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType type,
> >       const char *name;
> >       int i;
> >   
> > -    if (type != CPU_MODEL_EXPANSION_TYPE_STATIC) {


> > +    if (type != CPU_MODEL_EXPANSION_TYPE_FULL) {
> >           error_setg(errp, "The requested expansion type is not supported");
> >           return NULL;
> >       }
> > 
> > base-commit: de278e54aefed143526174335f8286f7437d20be
> >   
> 
> 

Re: [PATCH] target/loongarch: Add full type support for query-cpu-model-expansion

[bibo mao]

On 2025/2/14 上午12:52, Igor Mammedov wrote:
> On Thu, 13 Feb 2025 17:40:56 +0800
> bibo mao <maobibo@loongson.cn> wrote:
> 
>> please discard this patch :(
>>
>> libvirt uses static type already on LoongArch platform, there is
>> compatibility issue. Static type cannot be removed, although I do
>> not know the difference between static type and full type.
> 
> looking at loongarch-qmp-cmds.c:qmp_query_cpu_model_expansion(),
> you do filter reported features by global cpu_model_advertised_features[] list.
> Also cpu 'definition' is mainly governed by its initfn() and
> properties mentioned in cpu_model_advertised_features[] are created
> only by loongarch_cpu_post_init() , which is part of la464 and max models.
> The later call however mutates depending on accelerator, which
> sort of contradicts 'static' description and feets more to 'full' category.
> 
> What I'd suggest doing is to support both
> i.e. do keep 'static' for compat reasons  and run the same code for 'full'
That is a good idea. will keep static type the same with full type for 
the present, and implement real static type iff all basic features are 
implemented and compatible machine requirement is added in future.

Regards
Bibo Mao
> 
> In nutshell drop if(type) check or explicitly allow both types.
> 
>>
>> Regards
>> Bibo Mao
>>
>> On 2025/2/13 下午5:16, Bibo Mao wrote:
>>> With full type for query-cpu-model-expansion qmp command, it shows that
>>> it is not supported. For instance,
>>>     query-cpu-model-expansion type=full model={"name":"max"}
>>>
>>> Here is is output,
>>>     {"error": {"class": "GenericError", "desc": "The requested expansion type is not supported"}}
>>>
>>> Since full type is not supported and only static type is supported, Here
>>> replace static type with full type for command query-cpu-model-expansion.
>>>
>>> And there is result with this patch,
>>>     {"return": {"model": {"name": "max", "props": {"lbt": true, "lasx": true, "pmu": true, "lsx": true}}}}
>>>
>>> Signed-off-by: Bibo Mao <maobibo@loongson.cn>
>>> ---
>>>    target/loongarch/loongarch-qmp-cmds.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/target/loongarch/loongarch-qmp-cmds.c b/target/loongarch/loongarch-qmp-cmds.c
>>> index 3fde5a5a20..429c6d35fd 100644
>>> --- a/target/loongarch/loongarch-qmp-cmds.c
>>> +++ b/target/loongarch/loongarch-qmp-cmds.c
>>> @@ -56,7 +56,7 @@ CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType type,
>>>        const char *name;
>>>        int i;
>>>    
>>> -    if (type != CPU_MODEL_EXPANSION_TYPE_STATIC) {
> 
> 
>>> +    if (type != CPU_MODEL_EXPANSION_TYPE_FULL) {
>>>            error_setg(errp, "The requested expansion type is not supported");
>>>            return NULL;
>>>        }
>>>
>>> base-commit: de278e54aefed143526174335f8286f7437d20be
>>>    
>>
>>

Re: [PATCH] target/loongarch: Add full type support for query-cpu-model-expansion

[gaosong]

Cc: Markus

hi, Markus

What is the difference between CPU_MODEL_EXPANSION_TYPE_STATIC and 
CPU_MODEL_EXPANSION_TYPE_FULL?

thanks.
Song Gao.

在 2025/2/13 下午5:16, Bibo Mao 写道:
> With full type for query-cpu-model-expansion qmp command, it shows that
> it is not supported. For instance,
>    query-cpu-model-expansion type=full model={"name":"max"}
>
> Here is is output,
>    {"error": {"class": "GenericError", "desc": "The requested expansion type is not supported"}}
>
> Since full type is not supported and only static type is supported, Here
> replace static type with full type for command query-cpu-model-expansion.
>
> And there is result with this patch,
>    {"return": {"model": {"name": "max", "props": {"lbt": true, "lasx": true, "pmu": true, "lsx": true}}}}
>
> Signed-off-by: Bibo Mao <maobibo@loongson.cn>
> ---
>   target/loongarch/loongarch-qmp-cmds.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/loongarch/loongarch-qmp-cmds.c b/target/loongarch/loongarch-qmp-cmds.c
> index 3fde5a5a20..429c6d35fd 100644
> --- a/target/loongarch/loongarch-qmp-cmds.c
> +++ b/target/loongarch/loongarch-qmp-cmds.c
> @@ -56,7 +56,7 @@ CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType type,
>       const char *name;
>       int i;
>   
> -    if (type != CPU_MODEL_EXPANSION_TYPE_STATIC) {
> +    if (type != CPU_MODEL_EXPANSION_TYPE_FULL) {
>           error_setg(errp, "The requested expansion type is not supported");
>           return NULL;
>       }
>
> base-commit: de278e54aefed143526174335f8286f7437d20be

Re: [PATCH] target/loongarch: Add full type support for query-cpu-model-expansion

[Markus Armbruster]

gaosong <gaosong@loongson.cn> writes:

> Cc: Markus
>
> hi, Markus
>
> What is the difference between CPU_MODEL_EXPANSION_TYPE_STATIC and 
> CPU_MODEL_EXPANSION_TYPE_FULL?

I don't know :)

Here's the documentation:

    ##
    # @CpuModelExpansionType:
    #
    # An enumeration of CPU model expansion types.
    #
    # @static: Expand to a static CPU model, a combination of a static
    #     base model name and property delta changes.  As the static base
    #     model will never change, the expanded CPU model will be the
    #     same, independent of QEMU version, machine type, machine
    #     options, and accelerator options.  Therefore, the resulting
    #     model can be used by tooling without having to specify a
    #     compatibility machine - e.g. when displaying the "host" model.
    #     The @static CPU models are migration-safe.
    #
    # @full: Expand all properties.  The produced model is not guaranteed
    #     to be migration-safe, but allows tooling to get an insight and
    #     work with model details.
    #
    # .. note:: When a non-migration-safe CPU model is expanded in static
    #    mode, some features enabled by the CPU model may be omitted,
    #    because they can't be implemented by a static CPU model
    #    definition (e.g. cache info passthrough and PMU passthrough in
    #    x86).  If you need an accurate representation of the features
    #    enabled by a non-migration-safe CPU model, use @full.  If you
    #    need a static representation that will keep ABI compatibility
    #    even when changing QEMU version or machine-type, use @static (but
    #    keep in mind that some features may be omitted).
    #
    # Since: 2.8
    ##

If you have further questions, David Hildenbrand or Eduardo Habkost
(cc'ed) might be able to help.

Re: [PATCH] target/loongarch: Add full type support for query-cpu-model-expansion

[Igor Mammedov]

On Thu, 13 Feb 2025 12:48:30 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> gaosong <gaosong@loongson.cn> writes:
> 
> > Cc: Markus
> >
> > hi, Markus
> >
> > What is the difference between CPU_MODEL_EXPANSION_TYPE_STATIC and 
> > CPU_MODEL_EXPANSION_TYPE_FULL?  

the only difference is that 'static' expansion will not report properties
not mentioned in hard-codded CPU model definition see: builtin_x86_defs

while 'full' will iterate over/report all rw properties of CPU object
created from provided model name.

> I don't know :)
> 
> Here's the documentation:
> 
>     ##
>     # @CpuModelExpansionType:
>     #
>     # An enumeration of CPU model expansion types.
>     #
>     # @static: Expand to a static CPU model, a combination of a static
>     #     base model name and property delta changes.  As the static base
>     #     model will never change, the expanded CPU model will be the
>     #     same, independent of QEMU version, machine type, machine
>     #     options, and accelerator options.  Therefore, the resulting
>     #     model can be used by tooling without having to specify a
>     #     compatibility machine - e.g. when displaying the "host" model.
>     #     The @static CPU models are migration-safe.

Looking at related x86 code above description sounds like a fiction.
Both 'static' and 'full' do use
 qmp_query_cpu_model_expansion()
     x86_cpu_from_model()
         cpu = object_new(foo_cpu_class)
         x86_cpu_expand_features(cpu)
and the later 2 calls are subjects to being influenced by:
    "QEMU version, machine type (compat machinery), machine options, and accelerator options"
and -cpu option

>     #
>     # @full: Expand all properties.  The produced model is not guaranteed
>     #     to be migration-safe, but allows tooling to get an insight and
>     #     work with model details.
>     #
>     # .. note:: When a non-migration-safe CPU model is expanded in static
>     #    mode, some features enabled by the CPU model may be omitted,
>     #    because they can't be implemented by a static CPU model
>     #    definition (e.g. cache info passthrough and PMU passthrough in
>     #    x86).  If you need an accurate representation of the features
>     #    enabled by a non-migration-safe CPU model, use @full.  If you
>     #    need a static representation that will keep ABI compatibility
>     #    even when changing QEMU version or machine-type, use @static (but
>     #    keep in mind that some features may be omitted).
>     #
>     # Since: 2.8
>     ##
> 
> If you have further questions, David Hildenbrand or Eduardo Habkost
> (cc'ed) might be able to help.
> 
> 

Re: [PATCH] target/loongarch: Add full type support for query-cpu-model-expansion

[David Hildenbrand]

On 13.02.25 17:17, Igor Mammedov wrote:
> On Thu, 13 Feb 2025 12:48:30 +0100
> Markus Armbruster <armbru@redhat.com> wrote:
> 
>> gaosong <gaosong@loongson.cn> writes:
>>
>>> Cc: Markus
>>>
>>> hi, Markus
>>>
>>> What is the difference between CPU_MODEL_EXPANSION_TYPE_STATIC and
>>> CPU_MODEL_EXPANSION_TYPE_FULL?
> 
> the only difference is that 'static' expansion will not report properties
> not mentioned in hard-codded CPU model definition see: builtin_x86_defs
> 
> while 'full' will iterate over/report all rw properties of CPU object
> created from provided model name.
> 
>> I don't know :)
>>
>> Here's the documentation:
>>
>>      ##
>>      # @CpuModelExpansionType:
>>      #
>>      # An enumeration of CPU model expansion types.
>>      #
>>      # @static: Expand to a static CPU model, a combination of a static
>>      #     base model name and property delta changes.  As the static base
>>      #     model will never change, the expanded CPU model will be the
>>      #     same, independent of QEMU version, machine type, machine
>>      #     options, and accelerator options.  Therefore, the resulting
>>      #     model can be used by tooling without having to specify a
>>      #     compatibility machine - e.g. when displaying the "host" model.
>>      #     The @static CPU models are migration-safe.
> 
> Looking at related x86 code above description sounds like a fiction.

On s390x, which added that interface, that is how it's work.

It resolves to "-base" models that are fixed for all eternity.

x86-64 probably didn't adhere to the description because they were not 
interested in adding stable base models.

-- 
Cheers,

David / dhildenb

Re: [PATCH] target/loongarch: Add full type support for query-cpu-model-expansion

[Markus Armbruster]

David Hildenbrand <david@redhat.com> writes:

> On 13.02.25 17:17, Igor Mammedov wrote:
>> On Thu, 13 Feb 2025 12:48:30 +0100
>> Markus Armbruster <armbru@redhat.com> wrote:
>> 
>>> gaosong <gaosong@loongson.cn> writes:
>>>
>>>> Cc: Markus
>>>>
>>>> hi, Markus
>>>>
>>>> What is the difference between CPU_MODEL_EXPANSION_TYPE_STATIC and
>>>> CPU_MODEL_EXPANSION_TYPE_FULL?
>>
>> the only difference is that 'static' expansion will not report properties
>> not mentioned in hard-codded CPU model definition see: builtin_x86_defs
>> while 'full' will iterate over/report all rw properties of CPU object
>> created from provided model name.
>> 
>>> I don't know :)
>>>
>>> Here's the documentation:
>>>
>>>      ##
>>>      # @CpuModelExpansionType:
>>>      #
>>>      # An enumeration of CPU model expansion types.
>>>      #
>>>      # @static: Expand to a static CPU model, a combination of a static
>>>      #     base model name and property delta changes.  As the static base
>>>      #     model will never change, the expanded CPU model will be the
>>>      #     same, independent of QEMU version, machine type, machine
>>>      #     options, and accelerator options.  Therefore, the resulting
>>>      #     model can be used by tooling without having to specify a
>>>      #     compatibility machine - e.g. when displaying the "host" model.
>>>      #     The @static CPU models are migration-safe.
>>
>> Looking at related x86 code above description sounds like a fiction.
>
> On s390x, which added that interface, that is how it's work.
>
> It resolves to "-base" models that are fixed for all eternity.
>
> x86-64 probably didn't adhere to the description because they were not interested in adding stable base models.

Looks like we made a mess.

Having the same command work differently on different targets is less
than ideal.  I'm sure there were reasons both for the initial s390x
design, and for x86-64 deviating from it later.  I'm not qualified to
debate this.

Regardless, actual behavior should match documented behavior.  If actual
behavior depends on the target, then documentation needs to explain that
in sufficient detail.

Could we get this fixed, please?