[v3] target/s390x: filter deprecated properties based on model expansion type

Currently, there is no way to execute the query-cpu-model-expansion
command to retrieve a comprehenisve list of deprecated properties, as
the result is dependent per-model. To enable this, the expansion output
is modified as such:

When reporting a "full" CPU model, show the *entire* list of deprecated
properties regardless if they are supported on the model. A full
expansion outputs all known CPU model properties anyway, so it makes
sense to report all deprecated properties here too.

This allows management apps to query a single model (e.g. host) to
acquire the full list of deprecated properties.

Additionally, when reporting a "static" CPU model, the command will
only show deprecated properties that are a subset of the model's
*enabled* properties. This is more accurate than how the query was
handled before, which blindly reported deprecated properties that
were never otherwise introduced for certain models.

Acked-by: David Hildenbrand <david@redhat.com>
Suggested-by: Jiri Denemark <jdenemar@redhat.com>
Signed-off-by: Collin Walling <walling@linux.ibm.com>
---

Changelog:

    v3
    - Removed the 'note' and cleaned up documentation
    - Revised commit message

    v2
    - Changed commit message
    - Added documentation reflecting this change
    - Made code changes that more accurately filter the deprecated
        properties based on expansion type.  This change makes it
        so that the deprecated-properties reported for a static model
        expansion are a subset of the model's properties instead of
        the model's full-definition properties.

---
 qapi/machine-target.json         |  5 +++--
 target/s390x/cpu_models_sysemu.c | 16 +++++++++-------
 2 files changed, 12 insertions(+), 9 deletions(-)

Collin Walling <walling@linux.ibm.com> writes:

> Currently, there is no way to execute the query-cpu-model-expansion
> command to retrieve a comprehenisve list of deprecated properties, as
> the result is dependent per-model. To enable this, the expansion output
> is modified as such:
>
> When reporting a "full" CPU model, show the *entire* list of deprecated
> properties regardless if they are supported on the model. A full
> expansion outputs all known CPU model properties anyway, so it makes
> sense to report all deprecated properties here too.
>
> This allows management apps to query a single model (e.g. host) to
> acquire the full list of deprecated properties.
>
> Additionally, when reporting a "static" CPU model, the command will
> only show deprecated properties that are a subset of the model's
> *enabled* properties. This is more accurate than how the query was
> handled before, which blindly reported deprecated properties that
> were never otherwise introduced for certain models.
>
> Acked-by: David Hildenbrand <david@redhat.com>
> Suggested-by: Jiri Denemark <jdenemar@redhat.com>
> Signed-off-by: Collin Walling <walling@linux.ibm.com>
> ---
>
> Changelog:
>
>     v3
>     - Removed the 'note' and cleaned up documentation
>     - Revised commit message
>
>     v2
>     - Changed commit message
>     - Added documentation reflecting this change
>     - Made code changes that more accurately filter the deprecated
>         properties based on expansion type.  This change makes it
>         so that the deprecated-properties reported for a static model
>         expansion are a subset of the model's properties instead of
>         the model's full-definition properties.
>
> ---
>  qapi/machine-target.json         |  5 +++--
>  target/s390x/cpu_models_sysemu.c | 16 +++++++++-------
>  2 files changed, 12 insertions(+), 9 deletions(-)
>
> diff --git a/qapi/machine-target.json b/qapi/machine-target.json
> index a8d9ec87f5..67086f006f 100644
> --- a/qapi/machine-target.json
> +++ b/qapi/machine-target.json
> @@ -21,8 +21,9 @@
>  # @props: a dictionary of QOM properties to be applied
>  #
>  # @deprecated-props: a list of properties that are flagged as deprecated
> -#     by the CPU vendor.  These props are a subset of the full model's
> -#     definition list of properties. (since 9.1)
> +#     by the CPU vendor.  These properties are either a subset of the
> +#     properties enabled on the CPU model, or a set of properties
> +#     deprecated across all models for the architecture.


When is it "a subset of the properties enabled on the CPU model", and
when is it "a set of properties deprecated across all models for the
architecture"?

My guess based on the commit message: it's the former when
query-cpu-model-expansion's type is "static", and the latter when it's
"full".

>  #
>  # Since: 2.8
>  ##
> diff --git a/target/s390x/cpu_models_sysemu.c b/target/s390x/cpu_models_sysemu.c
> index 977fbc6522..e28ecf7ab9 100644
> --- a/target/s390x/cpu_models_sysemu.c
> +++ b/target/s390x/cpu_models_sysemu.c
> @@ -174,11 +174,15 @@ static void cpu_info_from_model(CpuModelInfo *info, const S390CPUModel *model,
>                                  bool delta_changes)
>  {
>      QDict *qdict = qdict_new();
> -    S390FeatBitmap bitmap;
> +    S390FeatBitmap bitmap, deprecated;
>  
>      /* always fallback to the static base model */
>      info->name = g_strdup_printf("%s-base", model->def->name);
>  
> +    /* features flagged as deprecated */
> +    bitmap_zero(deprecated, S390_FEAT_MAX);
> +    s390_get_deprecated_features(deprecated);
> +
>      if (delta_changes) {
>          /* features deleted from the base feature set */
>          bitmap_andnot(bitmap, model->def->base_feat, model->features,
> @@ -193,6 +197,9 @@ static void cpu_info_from_model(CpuModelInfo *info, const S390CPUModel *model,
>          if (!bitmap_empty(bitmap, S390_FEAT_MAX)) {
>              s390_feat_bitmap_to_ascii(bitmap, qdict, qdict_add_enabled_feat);
>          }
> +
> +        /* deprecated features that are a subset of the model's enabled features */

Recommend to wrap this line for legibility.

> +        bitmap_and(deprecated, deprecated, model->features, S390_FEAT_MAX);
>      } else {
>          /* expand all features */
>          s390_feat_bitmap_to_ascii(model->features, qdict,
> @@ -207,12 +214,7 @@ static void cpu_info_from_model(CpuModelInfo *info, const S390CPUModel *model,
>          info->props = QOBJECT(qdict);
>      }
>  
> -    /* features flagged as deprecated */
> -    bitmap_zero(bitmap, S390_FEAT_MAX);
> -    s390_get_deprecated_features(bitmap);
> -
> -    bitmap_and(bitmap, bitmap, model->def->full_feat, S390_FEAT_MAX);
> -    s390_feat_bitmap_to_ascii(bitmap, &info->deprecated_props, list_add_feat);
> +    s390_feat_bitmap_to_ascii(deprecated, &info->deprecated_props, list_add_feat);
>      info->has_deprecated_props = !!info->deprecated_props;
>  }

On 7/20/24 1:33 AM, Markus Armbruster wrote:
> Collin Walling <walling@linux.ibm.com> writes:
> 
>> Currently, there is no way to execute the query-cpu-model-expansion
>> command to retrieve a comprehenisve list of deprecated properties, as
>> the result is dependent per-model. To enable this, the expansion output
>> is modified as such:
>>
>> When reporting a "full" CPU model, show the *entire* list of deprecated
>> properties regardless if they are supported on the model. A full
>> expansion outputs all known CPU model properties anyway, so it makes
>> sense to report all deprecated properties here too.
>>
>> This allows management apps to query a single model (e.g. host) to
>> acquire the full list of deprecated properties.
>>
>> Additionally, when reporting a "static" CPU model, the command will
>> only show deprecated properties that are a subset of the model's
>> *enabled* properties. This is more accurate than how the query was
>> handled before, which blindly reported deprecated properties that
>> were never otherwise introduced for certain models.
>>
>> Acked-by: David Hildenbrand <david@redhat.com>
>> Suggested-by: Jiri Denemark <jdenemar@redhat.com>
>> Signed-off-by: Collin Walling <walling@linux.ibm.com>
>> ---
>>
>> Changelog:
>>
>>     v3
>>     - Removed the 'note' and cleaned up documentation
>>     - Revised commit message
>>
>>     v2
>>     - Changed commit message
>>     - Added documentation reflecting this change
>>     - Made code changes that more accurately filter the deprecated
>>         properties based on expansion type.  This change makes it
>>         so that the deprecated-properties reported for a static model
>>         expansion are a subset of the model's properties instead of
>>         the model's full-definition properties.
>>
>> ---
>>  qapi/machine-target.json         |  5 +++--
>>  target/s390x/cpu_models_sysemu.c | 16 +++++++++-------
>>  2 files changed, 12 insertions(+), 9 deletions(-)
>>
>> diff --git a/qapi/machine-target.json b/qapi/machine-target.json
>> index a8d9ec87f5..67086f006f 100644
>> --- a/qapi/machine-target.json
>> +++ b/qapi/machine-target.json
>> @@ -21,8 +21,9 @@
>>  # @props: a dictionary of QOM properties to be applied
>>  #
>>  # @deprecated-props: a list of properties that are flagged as deprecated
>> -#     by the CPU vendor.  These props are a subset of the full model's
>> -#     definition list of properties. (since 9.1)
>> +#     by the CPU vendor.  These properties are either a subset of the
>> +#     properties enabled on the CPU model, or a set of properties
>> +#     deprecated across all models for the architecture.
> 
> 
> When is it "a subset of the properties enabled on the CPU model", and
> when is it "a set of properties deprecated across all models for the
> architecture"?
> 
> My guess based on the commit message: it's the former when
> query-cpu-model-expansion's type is "static", and the latter when it's
> "full".
> 

Correct.  I'm not confident where or how to document this dependency
since cross-referencing commands/data structures does not seem to be the
convention here.  My first thought is to mention how deprecated
properties are expanded under the @CpuModelExpansionType.  Something like:

diff --git a/qapi/machine-target.json b/qapi/machine-target.json
index 67086f006f..3f38c5229f 100644
--- a/qapi/machine-target.json
+++ b/qapi/machine-target.json
@@ -44,11 +44,15 @@
 #     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.
+#     The @static CPU models are migration-safe.  Deprecated
+#     properties are a subset of the properties enabled for the
+#     expanded model.
 #
 # @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.
+#     work with model details.  Deprecated properties are a set of
+#     properties that are deprecated across all models for the
+#     architecture.
 #
 # .. note:: When a non-migration-safe CPU model is expanded in static
 #    mode, some features enabled by the CPU model may be omitted,

Thoughts?

[...]

>> +
>> +        /* deprecated features that are a subset of the model's enabled features */
> 
> Recommend to wrap this line for legibility.
> 

[...]

Thanks for the feedback!  Pending your response to the above, I'll post
a v4.

On 22/07/2024 16.50, Collin Walling wrote:
> On 7/20/24 1:33 AM, Markus Armbruster wrote:
>> Collin Walling <walling@linux.ibm.com> writes:
>>
>>> Currently, there is no way to execute the query-cpu-model-expansion
>>> command to retrieve a comprehenisve list of deprecated properties, as
>>> the result is dependent per-model. To enable this, the expansion output
>>> is modified as such:
>>>
>>> When reporting a "full" CPU model, show the *entire* list of deprecated
>>> properties regardless if they are supported on the model. A full
>>> expansion outputs all known CPU model properties anyway, so it makes
>>> sense to report all deprecated properties here too.
>>>
>>> This allows management apps to query a single model (e.g. host) to
>>> acquire the full list of deprecated properties.
>>>
>>> Additionally, when reporting a "static" CPU model, the command will
>>> only show deprecated properties that are a subset of the model's
>>> *enabled* properties. This is more accurate than how the query was
>>> handled before, which blindly reported deprecated properties that
>>> were never otherwise introduced for certain models.
>>>
>>> Acked-by: David Hildenbrand <david@redhat.com>
>>> Suggested-by: Jiri Denemark <jdenemar@redhat.com>
>>> Signed-off-by: Collin Walling <walling@linux.ibm.com>
>>> ---
>>>
>>> Changelog:
>>>
>>>      v3
>>>      - Removed the 'note' and cleaned up documentation
>>>      - Revised commit message
>>>
>>>      v2
>>>      - Changed commit message
>>>      - Added documentation reflecting this change
>>>      - Made code changes that more accurately filter the deprecated
>>>          properties based on expansion type.  This change makes it
>>>          so that the deprecated-properties reported for a static model
>>>          expansion are a subset of the model's properties instead of
>>>          the model's full-definition properties.
>>>
>>> ---
>>>   qapi/machine-target.json         |  5 +++--
>>>   target/s390x/cpu_models_sysemu.c | 16 +++++++++-------
>>>   2 files changed, 12 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/qapi/machine-target.json b/qapi/machine-target.json
>>> index a8d9ec87f5..67086f006f 100644
>>> --- a/qapi/machine-target.json
>>> +++ b/qapi/machine-target.json
>>> @@ -21,8 +21,9 @@
>>>   # @props: a dictionary of QOM properties to be applied
>>>   #
>>>   # @deprecated-props: a list of properties that are flagged as deprecated
>>> -#     by the CPU vendor.  These props are a subset of the full model's
>>> -#     definition list of properties. (since 9.1)
>>> +#     by the CPU vendor.  These properties are either a subset of the
>>> +#     properties enabled on the CPU model, or a set of properties
>>> +#     deprecated across all models for the architecture.
>>
>>
>> When is it "a subset of the properties enabled on the CPU model", and
>> when is it "a set of properties deprecated across all models for the
>> architecture"?
...
> 
> Thanks for the feedback!  Pending your response to the above, I'll post
> a v4.

Since we've got soft-freeze for 9.1 today, I went ahead and put v3 into my 
last pull-request before the freeze period starts already. Please post the 
update to the comments as diff on top of that instead - updates to comments 
should still be fine for merging during the freeze period.

  Thanks,
   Thomas

On 7/23/24 5:23 AM, Thomas Huth wrote:
> On 22/07/2024 16.50, Collin Walling wrote:
>> On 7/20/24 1:33 AM, Markus Armbruster wrote:
>>> Collin Walling <walling@linux.ibm.com> writes:
>>>
>>>> Currently, there is no way to execute the query-cpu-model-expansion
>>>> command to retrieve a comprehenisve list of deprecated properties, as
>>>> the result is dependent per-model. To enable this, the expansion output
>>>> is modified as such:
>>>>
>>>> When reporting a "full" CPU model, show the *entire* list of deprecated
>>>> properties regardless if they are supported on the model. A full
>>>> expansion outputs all known CPU model properties anyway, so it makes
>>>> sense to report all deprecated properties here too.
>>>>
>>>> This allows management apps to query a single model (e.g. host) to
>>>> acquire the full list of deprecated properties.
>>>>
>>>> Additionally, when reporting a "static" CPU model, the command will
>>>> only show deprecated properties that are a subset of the model's
>>>> *enabled* properties. This is more accurate than how the query was
>>>> handled before, which blindly reported deprecated properties that
>>>> were never otherwise introduced for certain models.
>>>>
>>>> Acked-by: David Hildenbrand <david@redhat.com>
>>>> Suggested-by: Jiri Denemark <jdenemar@redhat.com>
>>>> Signed-off-by: Collin Walling <walling@linux.ibm.com>
>>>> ---
>>>>
>>>> Changelog:
>>>>
>>>>      v3
>>>>      - Removed the 'note' and cleaned up documentation
>>>>      - Revised commit message
>>>>
>>>>      v2
>>>>      - Changed commit message
>>>>      - Added documentation reflecting this change
>>>>      - Made code changes that more accurately filter the deprecated
>>>>          properties based on expansion type.  This change makes it
>>>>          so that the deprecated-properties reported for a static model
>>>>          expansion are a subset of the model's properties instead of
>>>>          the model's full-definition properties.
>>>>
>>>> ---
>>>>   qapi/machine-target.json         |  5 +++--
>>>>   target/s390x/cpu_models_sysemu.c | 16 +++++++++-------
>>>>   2 files changed, 12 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/qapi/machine-target.json b/qapi/machine-target.json
>>>> index a8d9ec87f5..67086f006f 100644
>>>> --- a/qapi/machine-target.json
>>>> +++ b/qapi/machine-target.json
>>>> @@ -21,8 +21,9 @@
>>>>   # @props: a dictionary of QOM properties to be applied
>>>>   #
>>>>   # @deprecated-props: a list of properties that are flagged as deprecated
>>>> -#     by the CPU vendor.  These props are a subset of the full model's
>>>> -#     definition list of properties. (since 9.1)
>>>> +#     by the CPU vendor.  These properties are either a subset of the
>>>> +#     properties enabled on the CPU model, or a set of properties
>>>> +#     deprecated across all models for the architecture.
>>>
>>>
>>> When is it "a subset of the properties enabled on the CPU model", and
>>> when is it "a set of properties deprecated across all models for the
>>> architecture"?
> ...
>>
>> Thanks for the feedback!  Pending your response to the above, I'll post
>> a v4.
> 
> Since we've got soft-freeze for 9.1 today, I went ahead and put v3 into my 
> last pull-request before the freeze period starts already. Please post the 
> update to the comments as diff on top of that instead - updates to comments 
> should still be fine for merging during the freeze period.
> 
>   Thanks,
>    Thomas
> 
> 

This is greatly appreciated, Thomas.  Thank you!

Collin Walling <walling@linux.ibm.com> writes:

> On 7/20/24 1:33 AM, Markus Armbruster wrote:
>> Collin Walling <walling@linux.ibm.com> writes:
>> 
>>> Currently, there is no way to execute the query-cpu-model-expansion
>>> command to retrieve a comprehenisve list of deprecated properties, as
>>> the result is dependent per-model. To enable this, the expansion output
>>> is modified as such:
>>>
>>> When reporting a "full" CPU model, show the *entire* list of deprecated
>>> properties regardless if they are supported on the model. A full
>>> expansion outputs all known CPU model properties anyway, so it makes
>>> sense to report all deprecated properties here too.
>>>
>>> This allows management apps to query a single model (e.g. host) to
>>> acquire the full list of deprecated properties.
>>>
>>> Additionally, when reporting a "static" CPU model, the command will
>>> only show deprecated properties that are a subset of the model's
>>> *enabled* properties. This is more accurate than how the query was
>>> handled before, which blindly reported deprecated properties that
>>> were never otherwise introduced for certain models.
>>>
>>> Acked-by: David Hildenbrand <david@redhat.com>
>>> Suggested-by: Jiri Denemark <jdenemar@redhat.com>
>>> Signed-off-by: Collin Walling <walling@linux.ibm.com>
>>> ---
>>>
>>> Changelog:
>>>
>>>     v3
>>>     - Removed the 'note' and cleaned up documentation
>>>     - Revised commit message
>>>
>>>     v2
>>>     - Changed commit message
>>>     - Added documentation reflecting this change
>>>     - Made code changes that more accurately filter the deprecated
>>>         properties based on expansion type.  This change makes it
>>>         so that the deprecated-properties reported for a static model
>>>         expansion are a subset of the model's properties instead of
>>>         the model's full-definition properties.
>>>
>>> ---
>>>  qapi/machine-target.json         |  5 +++--
>>>  target/s390x/cpu_models_sysemu.c | 16 +++++++++-------
>>>  2 files changed, 12 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/qapi/machine-target.json b/qapi/machine-target.json
>>> index a8d9ec87f5..67086f006f 100644
>>> --- a/qapi/machine-target.json
>>> +++ b/qapi/machine-target.json
>>> @@ -21,8 +21,9 @@
>>>  # @props: a dictionary of QOM properties to be applied
>>>  #
>>>  # @deprecated-props: a list of properties that are flagged as deprecated
>>> -#     by the CPU vendor.  These props are a subset of the full model's
>>> -#     definition list of properties. (since 9.1)
>>> +#     by the CPU vendor.  These properties are either a subset of the
>>> +#     properties enabled on the CPU model, or a set of properties
>>> +#     deprecated across all models for the architecture.
>> 
>> 
>> When is it "a subset of the properties enabled on the CPU model", and
>> when is it "a set of properties deprecated across all models for the
>> architecture"?
>> 
>> My guess based on the commit message: it's the former when
>> query-cpu-model-expansion's type is "static", and the latter when it's
>> "full".
>> 
>
> Correct.  I'm not confident where or how to document this dependency
> since cross-referencing commands/data structures does not seem to be the
> convention here.  My first thought is to mention how deprecated
> properties are expanded under the @CpuModelExpansionType.  Something like:
>
> diff --git a/qapi/machine-target.json b/qapi/machine-target.json
> index 67086f006f..3f38c5229f 100644
> --- a/qapi/machine-target.json
> +++ b/qapi/machine-target.json
> @@ -44,11 +44,15 @@
>  #     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.
> +#     The @static CPU models are migration-safe.  Deprecated
> +#     properties are a subset of the properties enabled for the
> +#     expanded model.
>  #
>  # @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.
> +#     work with model details.  Deprecated properties are a set of
> +#     properties that are deprecated across all models for the
> +#     architecture.
>  #
>  # .. note:: When a non-migration-safe CPU model is expanded in static
>  #    mode, some features enabled by the CPU model may be omitted,
>
> Thoughts?

The distance between @deprecated-props and parts of its documentation
bothers me a bit.

On closer examination, more questions on CpuModelInfo emerge.  Uses:

* query-cpu-model-comparison both arguments

  Documentation doesn't say how exactly the command uses the members of
  CpuModelInfo, i.e. @name, @props, @deprecated-props.  Can you tell me?

* query-cpu-model-expansion argument @model and return value member
  @model.

  The other argument is the expansion type, on which the value of return
  value model.deprecated-props depends, I believe.  Fine.

  Documentation doesn't say how exactly the command uses the members of
  CpuModelInfo arguments, i.e. @name, @props, @deprecated-props.  Can
  you tell me?

* query-cpu-model-baseline both arguments and return value member
  @model.

  Same, except we don't have an expansion type here.  So same question,
  plus another one: how does return value model.deprecated-props behave?

If you can't answer my questions, we need to find someone who can.

[...]

On 7/24/24 3:56 AM, Markus Armbruster wrote:
> Collin Walling <walling@linux.ibm.com> writes:
> 
>> On 7/20/24 1:33 AM, Markus Armbruster wrote:
>>> Collin Walling <walling@linux.ibm.com> writes:
>>>
>>>> Currently, there is no way to execute the query-cpu-model-expansion
>>>> command to retrieve a comprehenisve list of deprecated properties, as
>>>> the result is dependent per-model. To enable this, the expansion output
>>>> is modified as such:
>>>>
>>>> When reporting a "full" CPU model, show the *entire* list of deprecated
>>>> properties regardless if they are supported on the model. A full
>>>> expansion outputs all known CPU model properties anyway, so it makes
>>>> sense to report all deprecated properties here too.
>>>>
>>>> This allows management apps to query a single model (e.g. host) to
>>>> acquire the full list of deprecated properties.
>>>>
>>>> Additionally, when reporting a "static" CPU model, the command will
>>>> only show deprecated properties that are a subset of the model's
>>>> *enabled* properties. This is more accurate than how the query was
>>>> handled before, which blindly reported deprecated properties that
>>>> were never otherwise introduced for certain models.
>>>>
>>>> Acked-by: David Hildenbrand <david@redhat.com>
>>>> Suggested-by: Jiri Denemark <jdenemar@redhat.com>
>>>> Signed-off-by: Collin Walling <walling@linux.ibm.com>
>>>> ---
>>>>
>>>> Changelog:
>>>>
>>>>     v3
>>>>     - Removed the 'note' and cleaned up documentation
>>>>     - Revised commit message
>>>>
>>>>     v2
>>>>     - Changed commit message
>>>>     - Added documentation reflecting this change
>>>>     - Made code changes that more accurately filter the deprecated
>>>>         properties based on expansion type.  This change makes it
>>>>         so that the deprecated-properties reported for a static model
>>>>         expansion are a subset of the model's properties instead of
>>>>         the model's full-definition properties.
>>>>
>>>> ---
>>>>  qapi/machine-target.json         |  5 +++--
>>>>  target/s390x/cpu_models_sysemu.c | 16 +++++++++-------
>>>>  2 files changed, 12 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/qapi/machine-target.json b/qapi/machine-target.json
>>>> index a8d9ec87f5..67086f006f 100644
>>>> --- a/qapi/machine-target.json
>>>> +++ b/qapi/machine-target.json
>>>> @@ -21,8 +21,9 @@
>>>>  # @props: a dictionary of QOM properties to be applied
>>>>  #
>>>>  # @deprecated-props: a list of properties that are flagged as deprecated
>>>> -#     by the CPU vendor.  These props are a subset of the full model's
>>>> -#     definition list of properties. (since 9.1)
>>>> +#     by the CPU vendor.  These properties are either a subset of the
>>>> +#     properties enabled on the CPU model, or a set of properties
>>>> +#     deprecated across all models for the architecture.
>>>
>>>
>>> When is it "a subset of the properties enabled on the CPU model", and
>>> when is it "a set of properties deprecated across all models for the
>>> architecture"?
>>>
>>> My guess based on the commit message: it's the former when
>>> query-cpu-model-expansion's type is "static", and the latter when it's
>>> "full".
>>>
>>
>> Correct.  I'm not confident where or how to document this dependency
>> since cross-referencing commands/data structures does not seem to be the
>> convention here.  My first thought is to mention how deprecated
>> properties are expanded under the @CpuModelExpansionType.  Something like:
>>
>> diff --git a/qapi/machine-target.json b/qapi/machine-target.json
>> index 67086f006f..3f38c5229f 100644
>> --- a/qapi/machine-target.json
>> +++ b/qapi/machine-target.json
>> @@ -44,11 +44,15 @@
>>  #     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.
>> +#     The @static CPU models are migration-safe.  Deprecated
>> +#     properties are a subset of the properties enabled for the
>> +#     expanded model.
>>  #
>>  # @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.
>> +#     work with model details.  Deprecated properties are a set of
>> +#     properties that are deprecated across all models for the
>> +#     architecture.
>>  #
>>  # .. note:: When a non-migration-safe CPU model is expanded in static
>>  #    mode, some features enabled by the CPU model may be omitted,
>>
>> Thoughts?
> 
> The distance between @deprecated-props and parts of its documentation
> bothers me a bit.

Let me try to explain the purpose of @deprecated-props and see if it
helps bring us closer to some semblance of a mutual understanding so we
can work together on a concise documentation for this field.

s390 has been announcing features as deprecated for some time now, which
was fine as a way to let users know that they should tune their guests
to no longer user these features.  Now that we are approaching the
release of generations that will drop these deprecated features
outright, we encounter an issue: if users have not been mindful with
disabling these announced-deprecated-features, then their guests running
on older models will not be able to migrate to machines running on newer
hardware.

To alleviate this, I've added the @deprecated-props array to the
CpuModelInfo struct, and this field is populated by a
query-cpu-model-expansion* return.  It is up the the user/management app
to make use of this data.

On the libvirt side (currently in development), I am able to easily
retrieve the host-model with a full expansion, parse the
@deprecated-props, and then cache them for later use (e.g. when
reporting the host-model with these features disabled, or enabling a
user to define their domain with deprecated-features disabled via a
convenient XML attribute).

tl;dr @deprecated-props is only reported via a
query-cpu-model-expansion, and it is up to the user/management app to
figure out what to do with them.

> 
> On closer examination, more questions on CpuModelInfo emerge.  Uses:
> 

I will attempt to expand on each input @model (CpuModelInfo) as if they
were documented in the file.

> * query-cpu-model-comparison both arguments
> 
>   Documentation doesn't say how exactly the command uses the members of
>   CpuModelInfo, i.e. @name, @props, @deprecated-props.  Can you tell me?
> 

Note: Compares ModelA and ModelB.

Both @models must include @name.  @props is optional.  @deprecated-props
is ignored.

@name: the name of the CPU model definition to look up.  The definition
will be compared against the generation, GA level, and a static set of
properties of the opposing model.

@props: a set of additional properties to include in the model's set of
properties to be compared.

@deprecated-props: ignored.  The user should consider these properties
beforehand and decide if these properties should be disabled/omitted on
the respective model.

> * query-cpu-model-expansion argument @model and return value member
>   @model.
> 
>   The other argument is the expansion type, on which the value of return
>   value model.deprecated-props depends, I believe.  Fine.
> 
>   Documentation doesn't say how exactly the command uses the members of
>   CpuModelInfo arguments, i.e. @name, @props, @deprecated-props.  Can
>   you tell me?
> 

The @model must include @name.  @props is optional.  @deprecated-props
is ignored.

@name: the name of the CPU model definition to look up.  The definition
is associated with a set of properties that will populate the return data.

@props: a set of additional properties to include in the model's set of
expanded properties.

@deprecated-props: ignored.  The user should consider these properties
beforehand and decide if these properties should be disabled/omitted on
the model.

> * query-cpu-model-baseline both arguments and return value member
>   @model.
> 
>   Same, except we don't have an expansion type here.  So same question,
>   plus another one: how does return value model.deprecated-props behave?
> 

Note: Creates a baseline model based on ModelA and ModelB.

The @models must include @name.  @props is optional.  @deprecated-props
is ignored.

@name: the name of the CPU model definition to look up.  The definition,
GA level, and a static set of properties will be used to determine the
maximum model between ModelA and ModelB.

@props: a set of additional properties to include in the model's set of
properties to be baselined.

@deprecated-props: ignored.  The user should consider these properties
beforehand and decide if these properties should be disabled/omitted on
the respective model.

> If you can't answer my questions, we need to find someone who can.
> 

Hopefully this provides clarity on how CpuModelInfo and its respective
fields are used in each command.  @David should be able to fill in any
missing areas / expand / offer corrections.

> [...]
> 
>

Collin Walling <walling@linux.ibm.com> writes:

> On 7/24/24 3:56 AM, Markus Armbruster wrote:
>> Collin Walling <walling@linux.ibm.com> writes:
> Let me try to explain the purpose of @deprecated-props and see if it
> helps bring us closer to some semblance of a mutual understanding so we
> can work together on a concise documentation for this field.
>
> s390 has been announcing features as deprecated for some time now, which
> was fine as a way to let users know that they should tune their guests
> to no longer user these features.  Now that we are approaching the
> release of generations that will drop these deprecated features
> outright, we encounter an issue: if users have not been mindful with
> disabling these announced-deprecated-features, then their guests running
> on older models will not be able to migrate to machines running on newer
> hardware.
>
> To alleviate this, I've added the @deprecated-props array to the
> CpuModelInfo struct, and this field is populated by a
> query-cpu-model-expansion* return.  It is up the the user/management app
> to make use of this data.
>
> On the libvirt side (currently in development), I am able to easily
> retrieve the host-model with a full expansion, parse the
> @deprecated-props, and then cache them for later use (e.g. when
> reporting the host-model with these features disabled, or enabling a
> user to define their domain with deprecated-features disabled via a
> convenient XML attribute).
>
> tl;dr @deprecated-props is only reported via a
> query-cpu-model-expansion, and it is up to the user/management app to
> figure out what to do with them.

Got it.

Permit me a digression.  In QAPI/QMP, we do something similar: we expose
deprecation in introspection (query-qmp-schema), and what to do with the
information is up to the management application.  We provide one more
tool to it: policy for handling deprecated interfaces, set with -compat.
It permits "testing the future".  See qapi/compat.json for details.
Whether such a thing would be usful in your case I can't say.

>> On closer examination, more questions on CpuModelInfo emerge.  Uses:
>> 
>
> I will attempt to expand on each input @model (CpuModelInfo) as if they
> were documented in the file.
>
>> * query-cpu-model-comparison both arguments
>> 
>>   Documentation doesn't say how exactly the command uses the members of
>>   CpuModelInfo, i.e. @name, @props, @deprecated-props.  Can you tell me?
>> 
>
> Note: Compares ModelA and ModelB.
>
> Both @models must include @name.  @props is optional.  @deprecated-props
> is ignored.
>
> @name: the name of the CPU model definition to look up.  The definition
> will be compared against the generation, GA level, and a static set of
> properties of the opposing model.
>
> @props: a set of additional properties to include in the model's set of
> properties to be compared.
>
> @deprecated-props: ignored.  The user should consider these properties
> beforehand and decide if these properties should be disabled/omitted on
> the respective model.
>
>> * query-cpu-model-expansion argument @model and return value member
>>   @model.
>> 
>>   The other argument is the expansion type, on which the value of return
>>   value model.deprecated-props depends, I believe.  Fine.
>> 
>>   Documentation doesn't say how exactly the command uses the members of
>>   CpuModelInfo arguments, i.e. @name, @props, @deprecated-props.  Can
>>   you tell me?
>> 
>
> The @model must include @name.  @props is optional.  @deprecated-props
> is ignored.
>
> @name: the name of the CPU model definition to look up.  The definition
> is associated with a set of properties that will populate the return data.
>
> @props: a set of additional properties to include in the model's set of
> expanded properties.
>
> @deprecated-props: ignored.  The user should consider these properties
> beforehand and decide if these properties should be disabled/omitted on
> the model.

Return value member @model will have @name, may have @props and
@deprecated-props.

Absent @props is the same as {}.  Only x86 uses {}.

Absent @deprecated-props is the same as {}.  No target uses {}.  Can be
present only on S390.

Aside: returning the same thing in two different ways, like absent and
{}, is slightly more complex than necessary.  But let's ignore that
here.

>> * query-cpu-model-baseline both arguments and return value member
>>   @model.
>> 
>>   Same, except we don't have an expansion type here.  So same question,
>>   plus another one: how does return value model.deprecated-props behave?
>> 
>
> Note: Creates a baseline model based on ModelA and ModelB.
>
> The @models must include @name.  @props is optional.  @deprecated-props
> is ignored.
>
> @name: the name of the CPU model definition to look up.  The definition,
> GA level, and a static set of properties will be used to determine the
> maximum model between ModelA and ModelB.
>
> @props: a set of additional properties to include in the model's set of
> properties to be baselined.
>
> @deprecated-props: ignored.  The user should consider these properties
> beforehand and decide if these properties should be disabled/omitted on
> the respective model.

Return value member @model is just like in query-cpu-model-expansion.

Unlike query-cpu-model-expansion, we don't have an expansion type.  The
value of @deprecated-props depends on the expansion type.  Do we assume
a type?  Which one?

>> If you can't answer my questions, we need to find someone who can.
>> 
>
> Hopefully this provides clarity on how CpuModelInfo and its respective
> fields are used in each command.  @David should be able to fill in any
> missing areas / expand / offer corrections.
>
>> [...]

This helps, thanks!

Arguments that are silently ignored is bad interface design.

Observe: when CpuModelInfo is an argument, @deprecated-props is always
ignored.  When it's a return value, absent means {}, and it can be
present only for certain targets (currently S390).

The reason we end up with an argument we ignore is laziness: we use the
same type for both roles.  We can fix that easily:

    { 'struct': 'CpuModel',
      'data': { 'name': 'str',
                '*props': 'any' } }

    { 'struct': 'CpuModelInfo',
      'base': 'CpuModel',
      'data': { '*deprecated-props': ['str'] } }

Use CpuModel for arguments, CpuModelInfo for return values.

Since @deprecated-props is used only by some targets, I'd make it
conditional, i.e. 'if': 'TARGET_S390X'.

Thoughts?

Markus Armbruster <armbru@redhat.com> writes:

> Collin Walling <walling@linux.ibm.com> writes:
>
>> On 7/24/24 3:56 AM, Markus Armbruster wrote:
>>> Collin Walling <walling@linux.ibm.com> writes:
>> Let me try to explain the purpose of @deprecated-props and see if it
>> helps bring us closer to some semblance of a mutual understanding so we
>> can work together on a concise documentation for this field.
>>
>> s390 has been announcing features as deprecated for some time now, which
>> was fine as a way to let users know that they should tune their guests
>> to no longer user these features.  Now that we are approaching the
>> release of generations that will drop these deprecated features
>> outright, we encounter an issue: if users have not been mindful with
>> disabling these announced-deprecated-features, then their guests running
>> on older models will not be able to migrate to machines running on newer
>> hardware.
>>
>> To alleviate this, I've added the @deprecated-props array to the
>> CpuModelInfo struct, and this field is populated by a
>> query-cpu-model-expansion* return.  It is up the the user/management app
>> to make use of this data.
>>
>> On the libvirt side (currently in development), I am able to easily
>> retrieve the host-model with a full expansion, parse the
>> @deprecated-props, and then cache them for later use (e.g. when
>> reporting the host-model with these features disabled, or enabling a
>> user to define their domain with deprecated-features disabled via a
>> convenient XML attribute).
>>
>> tl;dr @deprecated-props is only reported via a
>> query-cpu-model-expansion, and it is up to the user/management app to
>> figure out what to do with them.
>
> Got it.
>
> Permit me a digression.  In QAPI/QMP, we do something similar: we expose
> deprecation in introspection (query-qmp-schema), and what to do with the
> information is up to the management application.  We provide one more
> tool to it: policy for handling deprecated interfaces, set with -compat.
> It permits "testing the future".  See qapi/compat.json for details.
> Whether such a thing would be usful in your case I can't say.
>
>>> On closer examination, more questions on CpuModelInfo emerge.  Uses:
>>> 
>>
>> I will attempt to expand on each input @model (CpuModelInfo) as if they
>> were documented in the file.
>>
>>> * query-cpu-model-comparison both arguments
>>> 
>>>   Documentation doesn't say how exactly the command uses the members of
>>>   CpuModelInfo, i.e. @name, @props, @deprecated-props.  Can you tell me?
>>> 
>>
>> Note: Compares ModelA and ModelB.
>>
>> Both @models must include @name.  @props is optional.  @deprecated-props
>> is ignored.
>>
>> @name: the name of the CPU model definition to look up.  The definition
>> will be compared against the generation, GA level, and a static set of
>> properties of the opposing model.
>>
>> @props: a set of additional properties to include in the model's set of
>> properties to be compared.
>>
>> @deprecated-props: ignored.  The user should consider these properties
>> beforehand and decide if these properties should be disabled/omitted on
>> the respective model.
>>
>>> * query-cpu-model-expansion argument @model and return value member
>>>   @model.
>>> 
>>>   The other argument is the expansion type, on which the value of return
>>>   value model.deprecated-props depends, I believe.  Fine.
>>> 
>>>   Documentation doesn't say how exactly the command uses the members of
>>>   CpuModelInfo arguments, i.e. @name, @props, @deprecated-props.  Can
>>>   you tell me?
>>> 
>>
>> The @model must include @name.  @props is optional.  @deprecated-props
>> is ignored.
>>
>> @name: the name of the CPU model definition to look up.  The definition
>> is associated with a set of properties that will populate the return data.
>>
>> @props: a set of additional properties to include in the model's set of
>> expanded properties.
>>
>> @deprecated-props: ignored.  The user should consider these properties
>> beforehand and decide if these properties should be disabled/omitted on
>> the model.
>
> Return value member @model will have @name, may have @props and
> @deprecated-props.
>
> Absent @props is the same as {}.  Only x86 uses {}.
>
> Absent @deprecated-props is the same as {}.  No target uses {}.  Can be
> present only on S390.
>
> Aside: returning the same thing in two different ways, like absent and
> {}, is slightly more complex than necessary.  But let's ignore that
> here.
>
>>> * query-cpu-model-baseline both arguments and return value member
>>>   @model.
>>> 
>>>   Same, except we don't have an expansion type here.  So same question,
>>>   plus another one: how does return value model.deprecated-props behave?
>>> 
>>
>> Note: Creates a baseline model based on ModelA and ModelB.
>>
>> The @models must include @name.  @props is optional.  @deprecated-props
>> is ignored.
>>
>> @name: the name of the CPU model definition to look up.  The definition,
>> GA level, and a static set of properties will be used to determine the
>> maximum model between ModelA and ModelB.
>>
>> @props: a set of additional properties to include in the model's set of
>> properties to be baselined.
>>
>> @deprecated-props: ignored.  The user should consider these properties
>> beforehand and decide if these properties should be disabled/omitted on
>> the respective model.
>
> Return value member @model is just like in query-cpu-model-expansion.
>
> Unlike query-cpu-model-expansion, we don't have an expansion type.  The
> value of @deprecated-props depends on the expansion type.  Do we assume
> a type?  Which one?
>
>>> If you can't answer my questions, we need to find someone who can.
>>> 
>>
>> Hopefully this provides clarity on how CpuModelInfo and its respective
>> fields are used in each command.  @David should be able to fill in any
>> missing areas / expand / offer corrections.
>>
>>> [...]
>
> This helps, thanks!
>
> Arguments that are silently ignored is bad interface design.
>
> Observe: when CpuModelInfo is an argument, @deprecated-props is always
> ignored.  When it's a return value, absent means {}, and it can be
> present only for certain targets (currently S390).
>
> The reason we end up with an argument we ignore is laziness: we use the
> same type for both roles.  We can fix that easily:
>
>     { 'struct': 'CpuModel',
>       'data': { 'name': 'str',
>                 '*props': 'any' } }
>
>     { 'struct': 'CpuModelInfo',
>       'base': 'CpuModel',
>       'data': { '*deprecated-props': ['str'] } }
>
> Use CpuModel for arguments, CpuModelInfo for return values.
>
> Since @deprecated-props is used only by some targets, I'd make it
> conditional, i.e. 'if': 'TARGET_S390X'.

If we want just query-cpu-model-expansion return deprecated properties,
we can instead move @deprecated-props from CpuModelInfo to
CpuModelExpansionInfo.

> Thoughts?

On 25.07.24 09:35, Markus Armbruster wrote:
> Markus Armbruster <armbru@redhat.com> writes:
> 
>> Collin Walling <walling@linux.ibm.com> writes:
>>
>>> On 7/24/24 3:56 AM, Markus Armbruster wrote:
>>>> Collin Walling <walling@linux.ibm.com> writes:
>>> Let me try to explain the purpose of @deprecated-props and see if it
>>> helps bring us closer to some semblance of a mutual understanding so we
>>> can work together on a concise documentation for this field.
>>>
>>> s390 has been announcing features as deprecated for some time now, which
>>> was fine as a way to let users know that they should tune their guests
>>> to no longer user these features.  Now that we are approaching the
>>> release of generations that will drop these deprecated features
>>> outright, we encounter an issue: if users have not been mindful with
>>> disabling these announced-deprecated-features, then their guests running
>>> on older models will not be able to migrate to machines running on newer
>>> hardware.
>>>
>>> To alleviate this, I've added the @deprecated-props array to the
>>> CpuModelInfo struct, and this field is populated by a
>>> query-cpu-model-expansion* return.  It is up the the user/management app
>>> to make use of this data.
>>>
>>> On the libvirt side (currently in development), I am able to easily
>>> retrieve the host-model with a full expansion, parse the
>>> @deprecated-props, and then cache them for later use (e.g. when
>>> reporting the host-model with these features disabled, or enabling a
>>> user to define their domain with deprecated-features disabled via a
>>> convenient XML attribute).
>>>
>>> tl;dr @deprecated-props is only reported via a
>>> query-cpu-model-expansion, and it is up to the user/management app to
>>> figure out what to do with them.
>>
>> Got it.
>>
>> Permit me a digression.  In QAPI/QMP, we do something similar: we expose
>> deprecation in introspection (query-qmp-schema), and what to do with the
>> information is up to the management application.  We provide one more
>> tool to it: policy for handling deprecated interfaces, set with -compat.
>> It permits "testing the future".  See qapi/compat.json for details.
>> Whether such a thing would be usful in your case I can't say.
>>
>>>> On closer examination, more questions on CpuModelInfo emerge.  Uses:
>>>>
>>>
>>> I will attempt to expand on each input @model (CpuModelInfo) as if they
>>> were documented in the file.
>>>
>>>> * query-cpu-model-comparison both arguments
>>>>
>>>>    Documentation doesn't say how exactly the command uses the members of
>>>>    CpuModelInfo, i.e. @name, @props, @deprecated-props.  Can you tell me?
>>>>
>>>
>>> Note: Compares ModelA and ModelB.
>>>
>>> Both @models must include @name.  @props is optional.  @deprecated-props
>>> is ignored.
>>>
>>> @name: the name of the CPU model definition to look up.  The definition
>>> will be compared against the generation, GA level, and a static set of
>>> properties of the opposing model.
>>>
>>> @props: a set of additional properties to include in the model's set of
>>> properties to be compared.
>>>
>>> @deprecated-props: ignored.  The user should consider these properties
>>> beforehand and decide if these properties should be disabled/omitted on
>>> the respective model.
>>>
>>>> * query-cpu-model-expansion argument @model and return value member
>>>>    @model.
>>>>
>>>>    The other argument is the expansion type, on which the value of return
>>>>    value model.deprecated-props depends, I believe.  Fine.
>>>>
>>>>    Documentation doesn't say how exactly the command uses the members of
>>>>    CpuModelInfo arguments, i.e. @name, @props, @deprecated-props.  Can
>>>>    you tell me?
>>>>
>>>
>>> The @model must include @name.  @props is optional.  @deprecated-props
>>> is ignored.
>>>
>>> @name: the name of the CPU model definition to look up.  The definition
>>> is associated with a set of properties that will populate the return data.
>>>
>>> @props: a set of additional properties to include in the model's set of
>>> expanded properties.
>>>
>>> @deprecated-props: ignored.  The user should consider these properties
>>> beforehand and decide if these properties should be disabled/omitted on
>>> the model.
>>
>> Return value member @model will have @name, may have @props and
>> @deprecated-props.
>>
>> Absent @props is the same as {}.  Only x86 uses {}.
>>
>> Absent @deprecated-props is the same as {}.  No target uses {}.  Can be
>> present only on S390.
>>
>> Aside: returning the same thing in two different ways, like absent and
>> {}, is slightly more complex than necessary.  But let's ignore that
>> here.
>>
>>>> * query-cpu-model-baseline both arguments and return value member
>>>>    @model.
>>>>
>>>>    Same, except we don't have an expansion type here.  So same question,
>>>>    plus another one: how does return value model.deprecated-props behave?
>>>>
>>>
>>> Note: Creates a baseline model based on ModelA and ModelB.
>>>
>>> The @models must include @name.  @props is optional.  @deprecated-props
>>> is ignored.
>>>
>>> @name: the name of the CPU model definition to look up.  The definition,
>>> GA level, and a static set of properties will be used to determine the
>>> maximum model between ModelA and ModelB.
>>>
>>> @props: a set of additional properties to include in the model's set of
>>> properties to be baselined.
>>>
>>> @deprecated-props: ignored.  The user should consider these properties
>>> beforehand and decide if these properties should be disabled/omitted on
>>> the respective model.
>>
>> Return value member @model is just like in query-cpu-model-expansion.
>>
>> Unlike query-cpu-model-expansion, we don't have an expansion type.  The
>> value of @deprecated-props depends on the expansion type.  Do we assume
>> a type?  Which one?
>>
>>>> If you can't answer my questions, we need to find someone who can.
>>>>
>>>
>>> Hopefully this provides clarity on how CpuModelInfo and its respective
>>> fields are used in each command.  @David should be able to fill in any
>>> missing areas / expand / offer corrections.
>>>
>>>> [...]
>>
>> This helps, thanks!
>>
>> Arguments that are silently ignored is bad interface design.
>>
>> Observe: when CpuModelInfo is an argument, @deprecated-props is always
>> ignored.  When it's a return value, absent means {}, and it can be
>> present only for certain targets (currently S390).
>>
>> The reason we end up with an argument we ignore is laziness: we use the
>> same type for both roles.  We can fix that easily:
>>
>>      { 'struct': 'CpuModel',
>>        'data': { 'name': 'str',
>>                  '*props': 'any' } }
>>
>>      { 'struct': 'CpuModelInfo',
>>        'base': 'CpuModel',
>>        'data': { '*deprecated-props': ['str'] } }
>>
>> Use CpuModel for arguments, CpuModelInfo for return values.
>>
>> Since @deprecated-props is used only by some targets, I'd make it
>> conditional, i.e. 'if': 'TARGET_S390X'.
> 
> If we want just query-cpu-model-expansion return deprecated properties,
> we can instead move @deprecated-props from CpuModelInfo to
> CpuModelExpansionInfo.

That might a bit more sense, because deprecated-props does not make any 
sense as input parameter, for example.

On 7/25/24 3:39 AM, David Hildenbrand wrote:
> On 25.07.24 09:35, Markus Armbruster wrote:
>> Markus Armbruster <armbru@redhat.com> writes:
>>
>>> Collin Walling <walling@linux.ibm.com> writes:
>>>
>>>> On 7/24/24 3:56 AM, Markus Armbruster wrote:
>>>>> Collin Walling <walling@linux.ibm.com> writes:
>>>> Let me try to explain the purpose of @deprecated-props and see if it
>>>> helps bring us closer to some semblance of a mutual understanding so we
>>>> can work together on a concise documentation for this field.
>>>>
>>>> s390 has been announcing features as deprecated for some time now, which
>>>> was fine as a way to let users know that they should tune their guests
>>>> to no longer user these features.  Now that we are approaching the
>>>> release of generations that will drop these deprecated features
>>>> outright, we encounter an issue: if users have not been mindful with
>>>> disabling these announced-deprecated-features, then their guests running
>>>> on older models will not be able to migrate to machines running on newer
>>>> hardware.
>>>>
>>>> To alleviate this, I've added the @deprecated-props array to the
>>>> CpuModelInfo struct, and this field is populated by a
>>>> query-cpu-model-expansion* return.  It is up the the user/management app
>>>> to make use of this data.
>>>>
>>>> On the libvirt side (currently in development), I am able to easily
>>>> retrieve the host-model with a full expansion, parse the
>>>> @deprecated-props, and then cache them for later use (e.g. when
>>>> reporting the host-model with these features disabled, or enabling a
>>>> user to define their domain with deprecated-features disabled via a
>>>> convenient XML attribute).
>>>>
>>>> tl;dr @deprecated-props is only reported via a
>>>> query-cpu-model-expansion, and it is up to the user/management app to
>>>> figure out what to do with them.
>>>
>>> Got it.
>>>
>>> Permit me a digression.  In QAPI/QMP, we do something similar: we expose
>>> deprecation in introspection (query-qmp-schema), and what to do with the
>>> information is up to the management application.  We provide one more
>>> tool to it: policy for handling deprecated interfaces, set with -compat.
>>> It permits "testing the future".  See qapi/compat.json for details.
>>> Whether such a thing would be usful in your case I can't say.
>>>
>>>>> On closer examination, more questions on CpuModelInfo emerge.  Uses:
>>>>>
>>>>
>>>> I will attempt to expand on each input @model (CpuModelInfo) as if they
>>>> were documented in the file.
>>>>
>>>>> * query-cpu-model-comparison both arguments
>>>>>
>>>>>    Documentation doesn't say how exactly the command uses the members of
>>>>>    CpuModelInfo, i.e. @name, @props, @deprecated-props.  Can you tell me?
>>>>>
>>>>
>>>> Note: Compares ModelA and ModelB.
>>>>
>>>> Both @models must include @name.  @props is optional.  @deprecated-props
>>>> is ignored.
>>>>
>>>> @name: the name of the CPU model definition to look up.  The definition
>>>> will be compared against the generation, GA level, and a static set of
>>>> properties of the opposing model.
>>>>
>>>> @props: a set of additional properties to include in the model's set of
>>>> properties to be compared.
>>>>
>>>> @deprecated-props: ignored.  The user should consider these properties
>>>> beforehand and decide if these properties should be disabled/omitted on
>>>> the respective model.
>>>>
>>>>> * query-cpu-model-expansion argument @model and return value member
>>>>>    @model.
>>>>>
>>>>>    The other argument is the expansion type, on which the value of return
>>>>>    value model.deprecated-props depends, I believe.  Fine.
>>>>>
>>>>>    Documentation doesn't say how exactly the command uses the members of
>>>>>    CpuModelInfo arguments, i.e. @name, @props, @deprecated-props.  Can
>>>>>    you tell me?
>>>>>
>>>>
>>>> The @model must include @name.  @props is optional.  @deprecated-props
>>>> is ignored.
>>>>
>>>> @name: the name of the CPU model definition to look up.  The definition
>>>> is associated with a set of properties that will populate the return data.
>>>>
>>>> @props: a set of additional properties to include in the model's set of
>>>> expanded properties.
>>>>
>>>> @deprecated-props: ignored.  The user should consider these properties
>>>> beforehand and decide if these properties should be disabled/omitted on
>>>> the model.
>>>
>>> Return value member @model will have @name, may have @props and
>>> @deprecated-props.
>>>
>>> Absent @props is the same as {}.  Only x86 uses {}.
>>>
>>> Absent @deprecated-props is the same as {}.  No target uses {}.  Can be
>>> present only on S390.
>>>
>>> Aside: returning the same thing in two different ways, like absent and
>>> {}, is slightly more complex than necessary.  But let's ignore that
>>> here.
>>>
>>>>> * query-cpu-model-baseline both arguments and return value member
>>>>>    @model.
>>>>>
>>>>>    Same, except we don't have an expansion type here.  So same question,
>>>>>    plus another one: how does return value model.deprecated-props behave?
>>>>>
>>>>
>>>> Note: Creates a baseline model based on ModelA and ModelB.
>>>>
>>>> The @models must include @name.  @props is optional.  @deprecated-props
>>>> is ignored.
>>>>
>>>> @name: the name of the CPU model definition to look up.  The definition,
>>>> GA level, and a static set of properties will be used to determine the
>>>> maximum model between ModelA and ModelB.
>>>>
>>>> @props: a set of additional properties to include in the model's set of
>>>> properties to be baselined.
>>>>
>>>> @deprecated-props: ignored.  The user should consider these properties
>>>> beforehand and decide if these properties should be disabled/omitted on
>>>> the respective model.
>>>
>>> Return value member @model is just like in query-cpu-model-expansion.
>>>
>>> Unlike query-cpu-model-expansion, we don't have an expansion type.  The
>>> value of @deprecated-props depends on the expansion type.  Do we assume
>>> a type?  Which one?
>>>
>>>>> If you can't answer my questions, we need to find someone who can.
>>>>>
>>>>
>>>> Hopefully this provides clarity on how CpuModelInfo and its respective
>>>> fields are used in each command.  @David should be able to fill in any
>>>> missing areas / expand / offer corrections.
>>>>
>>>>> [...]
>>>
>>> This helps, thanks!
>>>
>>> Arguments that are silently ignored is bad interface design.
>>>
>>> Observe: when CpuModelInfo is an argument, @deprecated-props is always
>>> ignored.  When it's a return value, absent means {}, and it can be
>>> present only for certain targets (currently S390).
>>>
>>> The reason we end up with an argument we ignore is laziness: we use the
>>> same type for both roles.  We can fix that easily:
>>>
>>>      { 'struct': 'CpuModel',
>>>        'data': { 'name': 'str',
>>>                  '*props': 'any' } }
>>>
>>>      { 'struct': 'CpuModelInfo',
>>>        'base': 'CpuModel',
>>>        'data': { '*deprecated-props': ['str'] } }
>>>
>>> Use CpuModel for arguments, CpuModelInfo for return values.
>>>
>>> Since @deprecated-props is used only by some targets, I'd make it
>>> conditional, i.e. 'if': 'TARGET_S390X'.
>>
>> If we want just query-cpu-model-expansion return deprecated properties,
>> we can instead move @deprecated-props from CpuModelInfo to
>> CpuModelExpansionInfo.
> 
> That might a bit more sense, because deprecated-props does not make any 
> sense as input parameter, for example.
> 

Will do.  Thanks for the feedback.  v4 in the works.

Collin Walling <walling@linux.ibm.com> writes:

> On 7/25/24 3:39 AM, David Hildenbrand wrote:
>> On 25.07.24 09:35, Markus Armbruster wrote:
>>> Markus Armbruster <armbru@redhat.com> writes:

[...]

>>>> Arguments that are silently ignored is bad interface design.
>>>>
>>>> Observe: when CpuModelInfo is an argument, @deprecated-props is always
>>>> ignored.  When it's a return value, absent means {}, and it can be
>>>> present only for certain targets (currently S390).
>>>>
>>>> The reason we end up with an argument we ignore is laziness: we use the
>>>> same type for both roles.  We can fix that easily:
>>>>
>>>>      { 'struct': 'CpuModel',
>>>>        'data': { 'name': 'str',
>>>>                  '*props': 'any' } }
>>>>
>>>>      { 'struct': 'CpuModelInfo',
>>>>        'base': 'CpuModel',
>>>>        'data': { '*deprecated-props': ['str'] } }
>>>>
>>>> Use CpuModel for arguments, CpuModelInfo for return values.
>>>>
>>>> Since @deprecated-props is used only by some targets, I'd make it
>>>> conditional, i.e. 'if': 'TARGET_S390X'.
>>>
>>> If we want just query-cpu-model-expansion return deprecated properties,
>>> we can instead move @deprecated-props from CpuModelInfo to
>>> CpuModelExpansionInfo.
>> 
>> That might a bit more sense, because deprecated-props does not make any 
>> sense as input parameter, for example.
>
> Will do.  Thanks for the feedback.  v4 in the works.

We better get this into 9.1.  Plan B: mark @deprecated-props unstable to
avoid backward compatibility pain.

On 7/26/24 3:15 AM, Markus Armbruster wrote:
> Collin Walling <walling@linux.ibm.com> writes:
> 
>> On 7/25/24 3:39 AM, David Hildenbrand wrote:
>>> On 25.07.24 09:35, Markus Armbruster wrote:
>>>> Markus Armbruster <armbru@redhat.com> writes:
> 
> [...]
> 
>>>>> Arguments that are silently ignored is bad interface design.
>>>>>
>>>>> Observe: when CpuModelInfo is an argument, @deprecated-props is always
>>>>> ignored.  When it's a return value, absent means {}, and it can be
>>>>> present only for certain targets (currently S390).
>>>>>
>>>>> The reason we end up with an argument we ignore is laziness: we use the
>>>>> same type for both roles.  We can fix that easily:
>>>>>
>>>>>      { 'struct': 'CpuModel',
>>>>>        'data': { 'name': 'str',
>>>>>                  '*props': 'any' } }
>>>>>
>>>>>      { 'struct': 'CpuModelInfo',
>>>>>        'base': 'CpuModel',
>>>>>        'data': { '*deprecated-props': ['str'] } }
>>>>>
>>>>> Use CpuModel for arguments, CpuModelInfo for return values.
>>>>>
>>>>> Since @deprecated-props is used only by some targets, I'd make it
>>>>> conditional, i.e. 'if': 'TARGET_S390X'.
>>>>
>>>> If we want just query-cpu-model-expansion return deprecated properties,
>>>> we can instead move @deprecated-props from CpuModelInfo to
>>>> CpuModelExpansionInfo.
>>>
>>> That might a bit more sense, because deprecated-props does not make any 
>>> sense as input parameter, for example.
>>
>> Will do.  Thanks for the feedback.  v4 in the works.
> 
> We better get this into 9.1.  Plan B: mark @deprecated-props unstable to
> avoid backward compatibility pain.
> 
> 

Agreed, it would go a long way to squeeze this in before things get too
messy.

v4 is posted.  I think Thomas is unavailable, so if @David would not
mind handling this?  I'm confident that including this data within the
expansion response is the right way to go.  If there are any lingering
discrepancies WRT documentation, that can be fixed later.

On 26.07.24 21:11, Collin Walling wrote:
> On 7/26/24 3:15 AM, Markus Armbruster wrote:
>> Collin Walling <walling@linux.ibm.com> writes:
>>
>>> On 7/25/24 3:39 AM, David Hildenbrand wrote:
>>>> On 25.07.24 09:35, Markus Armbruster wrote:
>>>>> Markus Armbruster <armbru@redhat.com> writes:
>>
>> [...]
>>
>>>>>> Arguments that are silently ignored is bad interface design.
>>>>>>
>>>>>> Observe: when CpuModelInfo is an argument, @deprecated-props is always
>>>>>> ignored.  When it's a return value, absent means {}, and it can be
>>>>>> present only for certain targets (currently S390).
>>>>>>
>>>>>> The reason we end up with an argument we ignore is laziness: we use the
>>>>>> same type for both roles.  We can fix that easily:
>>>>>>
>>>>>>       { 'struct': 'CpuModel',
>>>>>>         'data': { 'name': 'str',
>>>>>>                   '*props': 'any' } }
>>>>>>
>>>>>>       { 'struct': 'CpuModelInfo',
>>>>>>         'base': 'CpuModel',
>>>>>>         'data': { '*deprecated-props': ['str'] } }
>>>>>>
>>>>>> Use CpuModel for arguments, CpuModelInfo for return values.
>>>>>>
>>>>>> Since @deprecated-props is used only by some targets, I'd make it
>>>>>> conditional, i.e. 'if': 'TARGET_S390X'.
>>>>>
>>>>> If we want just query-cpu-model-expansion return deprecated properties,
>>>>> we can instead move @deprecated-props from CpuModelInfo to
>>>>> CpuModelExpansionInfo.
>>>>
>>>> That might a bit more sense, because deprecated-props does not make any
>>>> sense as input parameter, for example.
>>>
>>> Will do.  Thanks for the feedback.  v4 in the works.
>>
>> We better get this into 9.1.  Plan B: mark @deprecated-props unstable to
>> avoid backward compatibility pain.
>>
>>
> 
> Agreed, it would go a long way to squeeze this in before things get too
> messy.
> 
> v4 is posted.  I think Thomas is unavailable, so if @David would not
> mind handling this?

Will do!