On 4/25/24 04:10, David Hildenbrand wrote:
> On 24.04.24 20:33, Collin Walling wrote:
>> On 4/24/24 03:24, David Hildenbrand wrote:
>>> On 23.04.24 23:06, Collin Walling wrote:
>>>> Retain a list of deprecated features disjoint from any particular
>>>> CPU model. When a query-cpu-model-expansion is provided with the
>>>> "disable-deprecated-feats" option set, the resulting properties list
>>>> will include all deprecated features paired with false. Example:
>>>>
>>>> { ... "bpb": false, "csske": false, ...}
>>>>
>>>> It is recommended that s390 guests operate with these features
>>>> explicitly disabled to ensure compatability with future hardware.
>>>>
>>>> Signed-off-by: Collin Walling <wall...@linux.ibm.com>
>>>> ---
>>>> target/s390x/cpu_features.c | 14 ++++++++++++++
>>>> target/s390x/cpu_features.h | 1 +
>>>> target/s390x/cpu_models_sysemu.c | 20 ++++++++++++--------
>>>> 3 files changed, 27 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/target/s390x/cpu_features.c b/target/s390x/cpu_features.c
>>>> index d28eb65845..efafc9711c 100644
>>>> --- a/target/s390x/cpu_features.c
>>>> +++ b/target/s390x/cpu_features.c
>>>> @@ -212,6 +212,20 @@ void s390_feat_bitmap_to_ascii(const S390FeatBitmap
>>>> features, void *opaque,
>>>> };
>>>> }
>>>>
>>>> +void s390_get_deprecated_features(S390FeatBitmap features)
>>>> +{
>>>> + static const int feats[] = {
>>>> + /* CSSKE is deprecated on newer generations */
>>>> + S390_FEAT_CONDITIONAL_SSKE,
>>>> + S390_FEAT_BPB,
>>>> + };
>>>> + int i;
>>>> +
>>>> + for (i = 0; i < ARRAY_SIZE(feats); i++) {
>>>> + set_bit(feats[i], features);
>>>> + }
>>>> +}
>>>> +
>>>> #define FEAT_GROUP_INIT(_name, _group, _desc) \
>>>> { \
>>>> .name = _name, \
>>>> diff --git a/target/s390x/cpu_features.h b/target/s390x/cpu_features.h
>>>> index a9bd68a2e1..661a8cd6db 100644
>>>> --- a/target/s390x/cpu_features.h
>>>> +++ b/target/s390x/cpu_features.h
>>>> @@ -69,6 +69,7 @@ void s390_add_from_feat_block(S390FeatBitmap features,
>>>> S390FeatType type,
>>>> uint8_t *data);
>>>> void s390_feat_bitmap_to_ascii(const S390FeatBitmap features, void
>>>> *opaque,
>>>> void (*fn)(const char *name, void
>>>> *opaque));
>>>> +void s390_get_deprecated_features(S390FeatBitmap features);
>>>>
>>>> /* Definition of a CPU feature group */
>>>> typedef struct {
>>>> diff --git a/target/s390x/cpu_models_sysemu.c
>>>> b/target/s390x/cpu_models_sysemu.c
>>>> index ef9fa80efd..b002819188 100644
>>>> --- a/target/s390x/cpu_models_sysemu.c
>>>> +++ b/target/s390x/cpu_models_sysemu.c
>>>> @@ -171,7 +171,8 @@ static void qdict_add_enabled_feat(const char *name,
>>>> void *opaque)
>>>>
>>>> /* 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_deprecated_feats)
>>>> {
>>>> QDict *qdict = qdict_new();
>>>> S390FeatBitmap bitmap;
>>>> @@ -201,6 +202,13 @@ static void cpu_info_from_model(CpuModelInfo *info,
>>>> const S390CPUModel *model,
>>>> s390_feat_bitmap_to_ascii(bitmap, qdict,
>>>> qdict_add_disabled_feat);
>>>> }
>>>>
>>>> + /* features flagged as deprecated */
>>>> + if (disable_deprecated_feats) {
>>>> + bitmap_zero(bitmap, S390_FEAT_MAX);
>>>> + s390_get_deprecated_features(bitmap);
>>>> + s390_feat_bitmap_to_ascii(bitmap, qdict, qdict_add_disabled_feat);
>>>> + }
>>>
>>> Likely, we should remove these features from the actual bitmap, such
>>> that they won't appear twice in the output? I'd expect the
>>> cpu_info_from_model() caller to handle that.
>>>
>>> Just adding them to the list as disabled is likely wrong.
>>>
>>> For example, if someone were to expend a given model with "... bpb:
>>> true" with disable-deprecated-feat=on, that should be remove from
>>> "bpb:true", and only replaced by "bpb=false" if it would be part of the
>>> CPU model we would be expanding to.
>>>
>>> Or am I missing something?
>>>
>>
>> qdict_add_disabled_feat will handle updating the feature if it already
>> exists. I placed the code to process deprecated features as the last
>> step of cpu_info_from_model to override any features that have already
>> been added to the bitmap. Whether it should be the deprecated feats that
>> take priority, or what the user requests is up in the air, however...
>
> Yes, that's one of my concern. IIRC, if the user specifies the same
> property multiple times, it's unclear which one will win.
>
> "bpb=true,bpb=false" might mean that bpb=true might win.
>
> I think this is something that this interface should sort out, so it
> will be actually usable!
>
>>
>> ... Daniel's suggestion to modify the QMP response to include a separate
>> list of "deprecated-props" seems like a much more efficient and readable
>> way that alleviates both your and Markus' concerns.
>
> Would you only include properties that would apply to the current model
> and would be set to true in the current model?
>
> How would libvirt make use of this interface, and could it run into the
> issue spelled out above?
>
The way I see having a "deprecated-props" array included in the
expansion response is that it just simply signals to the user/management
app which features are deprecated. From there, the next step to set up a
CPU model with these features turned off can be performed.
I've worked on the libvirt portion for the v1 of this QEMU patch set,
and the idea is to introduce a new CPU model type called
"host-recommended". When a libvirt domain is defined with this type,
then the CPU model will become the host-model with deprecated features
turned off.
This is, of course, a big discussion to be had once the patches are
upstream. They will be an RFC since I imagine a lot of ideas will be
thrown around.
Once the QEMU portion is accepted, I'll fix up what I have for libvirt
to properly query for the deprecated features and work them into my design.
--
Regards,
Collin
