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

[Daniel P . Berrangé]

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 !

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.



With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|