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

[Collin Walling]

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."

This patch modifies what is populated in the list by filtering it via an
intersection of the model's /default/ feature set.  The reason for this
change was that the deprecated-props list reported for very old s390
models was showing features that were not *actually* a subset of the
model's feature set.  One of the changes proposed by this patch corrects
this for static model expansions.  Explained by:

"Now, this list is populated with the model's enabled property set when
delta changes are applied."

The other change introduced by this patch is that the entire list of
deprecated-properties is now reported via a full model expansion,
explained by:

"All deprecated properties are reported otherwise."

My thoughts were to acknowledge this change in case a user sees
different results between QEMU versions.  However, as this
@deprecated-props thing is relatively new (a few months), perhaps it
does not make sense to include this note?  What would you suggest?

> 
> docs/devel/qapi-code-gen.rst section "Documentation markup":
> 
>     For legibility, wrap text paragraphs so every line is at most 70
>     characters long.
> 
>     Separate sentences with two spaces.
> 

Forgot about the 70 rule for these docs.  Missed the double space.
Thanks for reminding me.  Will update.

>>  #
>>  # Since: 2.8
>>  ##
> 
> [...]
> 
> 

-- 
Regards,
  Collin