Re: [PATCH v2] target/s390x: filter deprecated properties based on model expansion type

[Collin Walling <walling@linux.ibm.com>]

On 7/19/24 6:16 AM, Thomas Huth wrote:
> On 18/07/2024 20.22, Collin Walling wrote:
>> On 7/18/24 9:39 AM, Markus Armbruster wrote:
>>> Collin Walling <walling@linux.ibm.com> writes:
>>>
>>>> As s390 CPU models progress and deprecated properties are dropped
>>>> outright, it will be cumbersome for management apps to query the host
>>>> for a comprehensive list of deprecated properties that will need to be
>>>> disabled on older models. To remedy this, the query-cpu-model-expansion
>>>> output now behaves by filtering deprecated properties based on the
>>>> expansion type instead of filtering based off of the model's full set
>>>> of features:
>>>>
>>>> When reporting a static CPU model, only show deprecated properties that
>>>> are a subset of the model's enabled features.
>>>>
>>>> When reporting a full CPU model, show the entire list of deprecated
>>>> properties regardless if they are supported on the model.
>>>>
>>>> Suggested-by: Jiri Denemark <jdenemar@redhat.com>
>>>> Signed-off-by: Collin Walling <walling@linux.ibm.com>
>>>> ---
>>>>
>>>> Changelog:
>>>>
>>>>      v2
>>>>      - Changed commit message
>>>>      - Added documentation reflecting this change
>>>>      - Made code changes that more accurately filter the deprecated
>>>>          properties based on expansion type.  This change makes it
>>>>          so that the deprecated-properties reported for a static model
>>>>          expansion are a subset of the model's properties instead of
>>>>          the model's full-definition properties.
>>>>
>>>>          For example:
>>>>
>>>>          Previously, the z900 static model would report 'bpb' in the
>>>>          list of deprecated-properties.  However, this prop is *not*
>>>>          a part of the model's feature set, leading to some inaccuracy
>>>>          (albeit harmless).
>>>>
>>>>          Now, this feature will not show during a static expansion.
>>>>          It will, however, show up in a full expansion (along with
>>>>          the rest of the list: 'csske', 'te', 'cte').
>>>>
>>>> @David, I've elected to respectully forgo adding your ack-by on this
>>>> iteration since I have changed the code (and therefore the behavior)
>>>> between this version and the previous in case you do not agree with
>>>> these adjustments.
>>>>
>>>> ---
>>>>   qapi/machine-target.json         |  8 ++++++--
>>>>   target/s390x/cpu_models_sysemu.c | 16 +++++++++-------
>>>>   2 files changed, 15 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/qapi/machine-target.json b/qapi/machine-target.json
>>>> index a8d9ec87f5..d151504f25 100644
>>>> --- a/qapi/machine-target.json
>>>> +++ b/qapi/machine-target.json
>>>> @@ -21,8 +21,12 @@
>>>>   # @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 props are a subset of the full model's
>>>> -#     definition list of properties. (since 9.1)
>>>> +#     by the CPU vendor.  (since 9.1).
>>>> +#
>>>> +# .. note:: Since 9.1, the list of deprecated props were always a subset
>>>> +#    of the model's full-definition list of properites. Now, this list is
>>>> +#    populated with the model's enabled property set when delta changes
>>>> +#    are applied. All deprecated properties are reported otherwise.
>>>
>>> I'm confused.
>>>
>>> "Since 9.1, the list of deprecated props were ..." and "Now, this list
>>> is" sounds like you're explaining behavior before and after a change.
>>> What change?  Since only released behavior matters, and
>>> @deprecated-props is new, there is no old behavior to document, isn't
>>> it?
>>
>> I admittedly had some difficulty articulating the change introduced by
>> this patch.  The @deprecated-props array, as well as a way for s390x to
>> populate it, was introduced in release 9.1.  Prior to this patch, the
>> deprecated-props list was filtered by the CPU model's full feature set.
>> I attempted to explain this with:
>>
>> "Since 9.1, the list of deprecated props were always a subset of the
>> model's full-definition list of properties."
> 
> Version 9.1 has not been released yet (see 
> https://wiki.qemu.org/Planning/9.1), so I agree with Markus, this sounds 
> confusing/wrong to me, too.
> 
>   Thomas
> 
> 
> 

Ah, well then pardon my ignorance of the release schedule.  With that
said my `note` makes 0 sense to keep here.

-- 
Regards,
  Collin