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

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

On 7/19/24 7:11 AM, Markus Armbruster wrote:
> Thomas Huth <thuth@redhat.com> writes:
> 
>> 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>
> 
> [...]
> 
>>>>> 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.
> 
> User-visible changes between releases need to be documented in release
> notes and the manual.  Don't for changes within a release, instead
> update documentation to reflect the new state of things.
> 
> Regardless, do explain the change in the commit message.
> 
> Specifics are more helpful than such generalities, so let me try despite
> my relative ignorance of the subject matter.
> 
> 1. Update the description of @deprecated-props to match the new code.
> Ask yourself what people need to know to use this interface.
> 
> 2. Explain in the commit message how semantics of @deprecated-props
> change in this patch.
> 
> 

Understood, thank you.  As per Thomas' comment, I did not do my due
diligence to check the QEMU release schedule.  My comment does not make
sense anyway, since 9.1 is not even released.  I will remove it `note`
entirely.

Apologies for all of this confusion, and I appreciate your patience.

I will post v3 with the appropriate corrections.

-- 
Regards,
  Collin