Re: [PATCH v1] target/s390x: move @deprecated-props to CpuModelExpansion Info

[Markus Armbruster]

David Hildenbrand <david@redhat.com> writes:

> On 27.07.24 08:02, Markus Armbruster wrote:
>> Collin Walling <walling@linux.ibm.com> writes:
>> 
>>> The @deprecated-props array did not make any sense to be a member of the
>>> CpuModelInfo struct, since this field would only be populated by a
>>> query-cpu-model-expansion response and ignored otherwise.
>> 
>> Doesn't query-cpu-model-baseline also return it in its response?  It
>> seems to assume the "static" expansion type.
>> 
>>>                                                            Move this
>>> field to the CpuModelExpansionInfo struct where is makes more sense.
>>>
>>> References:
>>>   - https://lists.gnu.org/archive/html/qemu-devel/2024-07/msg05996.html
>>>   - commit eed0e8ffa38f0695c0519508f6e4f5a3297cbd67
>>>
>>> Signed-off-by: Collin Walling <walling@linux.ibm.com>
>>> ---
>>>
>>> @David, the previous commit header did not align with the changes made
>>> here, so I tagged this as a "v1" but added the previous conversation as
>>> a reference.  I hope this is appropriate?
>>>
>>> ---
>>>   qapi/machine-target.json         | 18 ++++++++++--------
>>>   target/s390x/cpu_models_sysemu.c | 31 ++++++++++++++++++++-----------
>>>   2 files changed, 30 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/qapi/machine-target.json b/qapi/machine-target.json
>>> index a552e2b0ce..09dec2b9bb 100644
>>> --- a/qapi/machine-target.json
>>> +++ b/qapi/machine-target.json
>>> @@ -20,17 +20,11 @@
>>>   #
>>>   # @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 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.
>>> -#
>>>   # Since: 2.8
>>>   ##
>>>   { 'struct': 'CpuModelInfo',
>>>     'data': { 'name': 'str',
>>> -            '*props': 'any',
>>> -            '*deprecated-props': ['str'] } }
>>> +            '*props': 'any' } }
>>>   
>>>   ##
>>>   # @CpuModelExpansionType:
>>> @@ -248,10 +242,18 @@
>>>   #
>>>   # @model: the expanded CpuModelInfo.
>>>   #
>>> +# @deprecated-props: a list of properties that are flagged as deprecated
>>> +#     by the CPU vendor.  The list depends on the CpuModelExpansionType:
>>> +#     "static" properties are a subset of the enabled-properties for
>>> +#     the expanded model; "full" properties are a set of properties
>>> +#     that are deprecated across all models for the architecture.
>>> +#     (since: 9.1).
>>> +#
>>>   # Since: 2.8
>>>   ##
>>>   { 'struct': 'CpuModelExpansionInfo',
>>> -  'data': { 'model': 'CpuModelInfo' },
>>> +  'data': { 'model': 'CpuModelInfo',
>>> +            '*deprecated-props': ['str'] },
>>>     'if': { 'any': [ 'TARGET_S390X',
>>>                      'TARGET_I386',
>>>                      'TARGET_ARM',
>> 
>> This solves several interface problems:
>> 
>> 1. Removes inappropriate @deprecated-props argument of
>>     query-cpu-model-comparison, query-cpu-model-expansion,
>>     query-cpu-model-baseline.
>> 
>> 2. Removes @deprecated-props return of query-cpu-model-baseline.
>> 
>> 3. Properly documents how @deprecated-props depends on the expansion
>>     type.
>> 
>> Remaining problem:
>> 
>> 4. Only S390 implements this.
>> 
>> Suggest to capture 1-3 more clearly in the commit message, perhaps like
>> this:
>> 
>>      CpuModelInfo is used both as command argument and in command
>>      returns.
>> 
>>      Its @deprecated-props array does not make any sense in arguments,
>>      and is silently ignored.  We actually want it only as return value
>>      of query-cpu-model-expansion.
>> 
>>      Move it from CpuModelInfo to CpuModelExpansionType, and document
>>      its dependence on expansion type propetly.
>
> s/propetly/property/

Can't type.

> Sounds good!
>
>> 
>> The simplest way to address 4 is to tack 'if': 'TARGET_S390X' to
>> @deprecated-props.
>> 
>
> diff --git a/qapi/machine-target.json b/qapi/machine-target.json
> index 09dec2b9bb..0be95d559c 100644
> --- a/qapi/machine-target.json
> +++ b/qapi/machine-target.json
> @@ -253,7 +253,7 @@
>   ##
>   { 'struct': 'CpuModelExpansionInfo',
>     'data': { 'model': 'CpuModelInfo',
> -            '*deprecated-props': ['str'] },
> +            '*deprecated-props' : { 'type': ['str'], 'if': 'TARGET_S390X' } 
> },
>     'if': { 'any': [ 'TARGET_S390X',
>                      'TARGET_I386',
>                      'TARGET_ARM',
>
>
> Should do the trick, right?

Yes.  Break the line before 'if', please.

>> I recommend to make @deprecated-props mandatory rather than optional
>> then.
>
> Hm, does that make sense judging that previous implementations didn't expose 
> it?

Adding members to the return value is fine.

docs/devel/qapi-code-gen.rst section "Compatibility considerations"

    Clients send commands with argument data, and receive command
    responses with return data and events with event data.

    Adding opt-in functionality to the send direction is backwards
    compatible: adding commands, optional arguments, enumeration values,
    union and alternate branches; turning an argument type into an
    alternate of that type; making mandatory arguments optional.  Clients
    oblivious of the new functionality continue to work.

    Incompatible changes include removing commands, command arguments,
    enumeration values, union and alternate branches, adding mandatory
    command arguments, and making optional arguments mandatory.

    The specified behavior of an absent optional argument should remain
    the same.  With proper documentation, this policy still allows some
    flexibility; for example, when an optional 'buffer-size' argument is
    specified to default to a sensible buffer size, the actual default
    value can still be changed.  The specified default behavior is not the
    exact size of the buffer, only that the default size is sensible.

--> Adding functionality to the receive direction is generally backwards
--> compatible: adding events, adding return and event data members.
--> Clients are expected to ignore the ones they don't know.

    Removing "unreachable" stuff like events that can't be triggered
    anymore, optional return or event data members that can't be sent
    anymore, and return or event data member (enumeration) values that
    can't be sent anymore makes no difference to clients, except for
    introspection.  The latter can conceivably confuse clients, so tread
    carefully.

    Incompatible changes include removing return and event data members.

Questions?