Re: [PATCH v2 1/3] cpu-models: add "disable-deprecated-feats" option to cpu model expansion

[Collin Walling]

On 4/25/24 09:35, Daniel P. Berrangé wrote:
> On Wed, Apr 24, 2024 at 03:12:42PM -0400, Collin Walling wrote:
>> On 4/24/24 13:51, Collin Walling wrote:
>>> On 4/24/24 04:20, Daniel P. Berrangé wrote:
>>>> On Tue, Apr 23, 2024 at 05:06:53PM -0400, Collin Walling wrote:
>>>>> This optional parameter for query-cpu-model-expansion enables CPU
>>>>> model features flagged as deprecated to appear in the resulting
>>>>> list of properties.
>>>>>
>>>>> This commit does not add support beyond adding a new argument
>>>>> to the query. All queries with this option present will result
>>>>> in an error claiming this option is not supported.
>>>>>
>>>>> Signed-off-by: Collin Walling <walling@linux.ibm.com>
>>>>> ---
>>>>>  qapi/machine-target.json         | 7 ++++++-
>>>>>  target/arm/arm-qmp-cmds.c        | 7 +++++++
>>>>>  target/i386/cpu-sysemu.c         | 7 +++++++
>>>>>  target/s390x/cpu_models_sysemu.c | 7 +++++++
>>>>>  4 files changed, 27 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/qapi/machine-target.json b/qapi/machine-target.json
>>>>> index 29e695aa06..b9da284d2d 100644
>>>>> --- a/qapi/machine-target.json
>>>>> +++ b/qapi/machine-target.json
>>>>> @@ -285,6 +285,10 @@
>>>>>  #
>>>>>  # @type: expansion type, specifying how to expand the CPU model
>>>>>  #
>>>>> +# @disable-deprecated-feats: include CPU model features that are
>>>>> +#     flagged as deprecated. If supported, these features will appear
>>>>> +#     in the properties list paired with false.
>>>>> +#
>>>>>  # Returns: a CpuModelExpansionInfo describing the expanded CPU model
>>>>>  #
>>>>>  # Errors:
>>>>> @@ -298,7 +302,8 @@
>>>>>  ##
>>>>>  { 'command': 'query-cpu-model-expansion',
>>>>>    'data': { 'type': 'CpuModelExpansionType',
>>>>> -            'model': 'CpuModelInfo' },
>>>>> +            'model': 'CpuModelInfo',
>>>>> +            '*disable-deprecated-feats': 'bool' },
>>>>>    'returns': 'CpuModelExpansionInfo',
>>>>>    'if': { 'any': [ 'TARGET_S390X',
>>>>>                     'TARGET_I386',
>>>>
>>>> I think this is an odd design approach. Lets consider the
>>>> current output:
>>>>
>>>> (QEMU) query-cpu-model-expansion type=static model={"name":"z14"}
>>>> {
>>>>     "return": {
>>>>         "model": {
>>>>             "name": "z14-base",
>>>>             "props": {
>>>>                 "aefsi": true,
>>>>                 "aen": true,
>>>>                 ...snip...
>>>>                 "vxpd": true,
>>>>                 "zpci": true
>>>>             }
>>>>         }
>>>>     }
>>>> }
>>>>
>>>>
>>>> If we want to inform a mgmt app of some features being deprecated,
>>>> why not just unconditionally include that info in the reply thus:
>>>>
>>>>
>>>> (QEMU) query-cpu-model-expansion type=static model={"name":"z14"}
>>>> {
>>>>     "return": {
>>>>         "model": {
>>>>             "name": "z14-base",
>>>>             "props": {
>>>>                 "aefsi": true,
>>>>                 "aen": true,
>>>>                 ...snip...
>>>>                 "vxpd": true,
>>>>                 "zpci": true
>>>>             }
>>>>             "deprecated-props": ["ppa15", "ri"]
>>>>         }
>>>>     }
>>>> }
>>>>
>>>>
>>>>
>>>> With regards,
>>>> Daniel
>>>
>>> That's a good idea. In this way, we're not mucking up any of the CPU
>>> model information and this makes it much more clear as to which features
>>> are actually deprecated... I like this more.
>>>
>>> I'll work on this.
>>>
>>
>> Follow-up question as I look more closely to the QMP response data
>> structures: should the "deprecated-props" list be added to the
>> CpuModelInfo struct, or to the CpuModelExpansionInfo struct?
>>
>> The former makes more sense to me, as the deprecated features are tied
>> to the actual CPU model... but unsure if other QMP commands would even
>> care about this info? I will begin with this approach, and if feedback
>> in the interim strongly sways in the other direction, then it should be
>> an easy change :)
> 
> I hink CpuModelInfo makes more sense than CpuModelExpansionInfo.
> The CpuModelExpansionInfo struct feels pretty pointless to me
> in fact, since the only thing it contains is CpuModelInfo !
> 

Agreed! :)

> I think it should also be added to 'CpuDefinitionInfo', which
> is the return type of 'query-cpu-defintions'.  This command already
> has a 'unavailable-features' array listing features which the host
> does not support. Conceptually having a 'deprecated-features' array
> alongside that is a nice fit.
> 

Okay. Pending review on the v3 I posted yesterday -- if those patches
look like they're on the right track, then I can add this
"deprecated-props" array to CpuDefinitionInfo as well for the next
iteration.

> 
> 
> With regards,
> Daniel

-- 
Regards,
  Collin