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 <wall...@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
