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

[Markus Armbruster]

Collin Walling <walling@linux.ibm.com> writes:

> On 4/25/24 02:31, Markus Armbruster wrote:
>> Collin Walling <walling@linux.ibm.com> writes:
>> 
>>> On 4/24/24 02:19, Markus Armbruster wrote:
>>>> Collin Walling <walling@linux.ibm.com> writes:
>>>>
>>>>> 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.
>>>>
>>>> What's the default?
>>>>
>>>> Which command result(s) does this affect?  Suggest to explain using
>>>> unabridged example QMP input and output before and after this series.
>>>
>>> Fair enough. Bool defaults to false but that's not apparent in the
>>> description. I will add more detail.
>> 
>> I didn't mean to ask for example QMP in the doc comment.  I need you to
>> explain the new member to me.  Once I understand what the thing does, I
>> may have suggestions on improving the doc comment.
>> 
>> [...]
>> 
>
> Ah, I misunderstood.  The idea behind this "disable-deprecated-feats"
> option is to add/override any CPU features that are flagged as
> deprecated to the props dictionary (found in the CpuModelInfo struct)
> paired with the value false.
>
> For example, the csske feature on s390x is flagged as deprecated.  If a
> query-cpu-model-expansion command is created with
> "disable-deprecated-feats": true, then the csske feature will show up in
> the props list as "csske": false.  This also overrides any user defined
> features and props that would show up in the response normally. E.g. if
> the same command was executed and "csske": true was provided in the
> model's properties by the user, the response will still show "csske":
> false since the "disable-deprecated-feats" option takes priority (there
> is a discussion with David H regarding which should take precedence --
> this is a flaw in this design).
>
> In the below QMP samples I provide a static expansion on a host model.
> Pay close attention to the bpb, te, cte, and csske properties.

[...]

> Hope this helps to provide more context.  Please let me know if you'd
> like more info.  I am leaning towards v3's design more, as it seems a
> lot cleaner overall.  I would appreciate your feedback there as well if
> you have the time.
>
> Thanks!

Okay, I'll look at v3 then.

Thank you!