Re: [PATCH v1 1/2] target/s390x: introduce "host-recommended" option for model expansion

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

On 9/12/23 02:57, David Hildenbrand wrote:
> On 11.09.23 22:52, Collin Walling wrote:
> 
> Patch subject is wrong (should contain "static-recommended")
> 
>> Newer S390 machines may drop support for features completely, rendering
>> guests operating with older CPU models incapable of running on said
>> machines. A manual effort to disable certain CPU features would be
>> required.
>>
>> To alleviate this issue, a list of "deprecated" features are now
>> retained within QEMU, and a new "static-recommended" CPU model expansion
>> type has been created to allow a query of the host-model with deprecated
>> features explicitly disabled.
>>
>> Signed-off-by: Collin Walling <walling@linux.ibm.com>
>> ---
>>   qapi/machine-target.json         |  8 +++++++-
>>   target/s390x/cpu_features.c      | 14 ++++++++++++++
>>   target/s390x/cpu_features.h      |  1 +
>>   target/s390x/cpu_models_sysemu.c | 26 +++++++++++++++++++++-----
>>   4 files changed, 43 insertions(+), 6 deletions(-)
>>
>> diff --git a/qapi/machine-target.json b/qapi/machine-target.json
>> index f0a6b72414..4dc891809d 100644
>> --- a/qapi/machine-target.json
>> +++ b/qapi/machine-target.json
>> @@ -42,6 +42,12 @@
>>   #     to be migration-safe, but allows tooling to get an insight and
>>   #     work with model details.
>>   #
>> +# @static-recommended: Expand to a static CPU model with property
>> +#     changes suggested by the architecture. This is useful for
>> +#     expanding a CPU model expected to operate in mixed
>> +#     CPU-generation environments. The @static-recommended CPU
>> +#     models are migration-safe.
>> +#
> 
> Can we instead make this a new parameter for query-cpu-model-expansion 
> ("no-deprecated-features" ? ), that properly gets rejected from other 
> archs when not supported?
> 
> [...]
> 

So instead of a "type": "static-recommended" add an entirely new
(optional) parameter key-value to the command?
"disable-deprecated-features": "true|false"?

>>   /* convert S390CPUDef into a static CpuModelInfo */
>>   static void cpu_info_from_model(CpuModelInfo *info, const S390CPUModel *model,
>> -                                bool delta_changes)
>> +                                bool delta_changes, bool disable_dep_feats)
> 
> "dep" can be misleading ("depended")
> 
> "no_deprecated_feat" ?
> 
> 

Good call. Tricky one to short-hand :)

With respect to labeling this as "no-deprecated-features", I think that
may also be misleading: it sounds like an exclusion, when in fact we
*want* the deprecated feats to show up paired with the false value. So I
think "disable-deprecated-features" is a better label. Would you agree?

-- 
Regards,
  Collin