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

[Collin Walling]

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.

> [...]
> 
> 

-- 
Regards,
  Collin