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

[Markus Armbruster]

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.

[...]