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

[David Hildenbrand <david@redhat.com>]

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/

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?

> I recommend to make @deprecated-props mandatory rather than optional
> then.

Hm, does that make sense judging that previous implementations didn't expose it?

-- 
Cheers,

David / dhildenb