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 <wall...@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
