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

[Collin Walling]

On 7/20/24 1:33 AM, Markus Armbruster wrote:
> Collin Walling <walling@linux.ibm.com> writes:
> 
>> Currently, there is no way to execute the query-cpu-model-expansion
>> command to retrieve a comprehenisve list of deprecated properties, as
>> the result is dependent per-model. To enable this, the expansion output
>> is modified as such:
>>
>> When reporting a "full" CPU model, show the *entire* list of deprecated
>> properties regardless if they are supported on the model. A full
>> expansion outputs all known CPU model properties anyway, so it makes
>> sense to report all deprecated properties here too.
>>
>> This allows management apps to query a single model (e.g. host) to
>> acquire the full list of deprecated properties.
>>
>> Additionally, when reporting a "static" CPU model, the command will
>> only show deprecated properties that are a subset of the model's
>> *enabled* properties. This is more accurate than how the query was
>> handled before, which blindly reported deprecated properties that
>> were never otherwise introduced for certain models.
>>
>> Acked-by: David Hildenbrand <david@redhat.com>
>> Suggested-by: Jiri Denemark <jdenemar@redhat.com>
>> Signed-off-by: Collin Walling <walling@linux.ibm.com>
>> ---
>>
>> Changelog:
>>
>>     v3
>>     - Removed the 'note' and cleaned up documentation
>>     - Revised commit message
>>
>>     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.
>>
>> ---
>>  qapi/machine-target.json         |  5 +++--
>>  target/s390x/cpu_models_sysemu.c | 16 +++++++++-------
>>  2 files changed, 12 insertions(+), 9 deletions(-)
>>
>> diff --git a/qapi/machine-target.json b/qapi/machine-target.json
>> index a8d9ec87f5..67086f006f 100644
>> --- a/qapi/machine-target.json
>> +++ b/qapi/machine-target.json
>> @@ -21,8 +21,9 @@
>>  # @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.  These properties are either a subset of the
>> +#     properties enabled on the CPU model, or a set of properties
>> +#     deprecated across all models for the architecture.
> 
> 
> When is it "a subset of the properties enabled on the CPU model", and
> when is it "a set of properties deprecated across all models for the
> architecture"?
> 
> My guess based on the commit message: it's the former when
> query-cpu-model-expansion's type is "static", and the latter when it's
> "full".
> 

Correct.  I'm not confident where or how to document this dependency
since cross-referencing commands/data structures does not seem to be the
convention here.  My first thought is to mention how deprecated
properties are expanded under the @CpuModelExpansionType.  Something like:

diff --git a/qapi/machine-target.json b/qapi/machine-target.json
index 67086f006f..3f38c5229f 100644
--- a/qapi/machine-target.json
+++ b/qapi/machine-target.json
@@ -44,11 +44,15 @@
 #     options, and accelerator options.  Therefore, the resulting
 #     model can be used by tooling without having to specify a
 #     compatibility machine - e.g. when displaying the "host" model.
-#     The @static CPU models are migration-safe.
+#     The @static CPU models are migration-safe.  Deprecated
+#     properties are a subset of the properties enabled for the
+#     expanded model.
 #
 # @full: Expand all properties.  The produced model is not guaranteed
 #     to be migration-safe, but allows tooling to get an insight and
-#     work with model details.
+#     work with model details.  Deprecated properties are a set of
+#     properties that are deprecated across all models for the
+#     architecture.
 #
 # .. note:: When a non-migration-safe CPU model is expanded in static
 #    mode, some features enabled by the CPU model may be omitted,

Thoughts?

[...]

>> +
>> +        /* deprecated features that are a subset of the model's enabled 
>> features */
> 
> Recommend to wrap this line for legibility.
> 

[...]

Thanks for the feedback!  Pending your response to the above, I'll post
a v4.

-- 
Regards,
  Collin