| Message ID | 20240423210655.66656-3-walling@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | query-cpu-model-expansion: add disable-deprecated-feats arg | expand |
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 <walling@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?
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 <walling@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... ... 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. Thanks for your feedback! Will work on the next proposal that implements the above. If you have any strong suggestions otherwise, please let me know :)
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 <walling@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?
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 <walling@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.
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); + } + if (!qdict_size(qdict)) { qobject_unref(qdict); } else { @@ -219,11 +227,6 @@ CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType type, S390CPUModel s390_model; bool delta_changes = false; - if (has_disable_deprecated_feats) { - error_setg(&err, "Unsupported option 'disable-deprecated-feats'"); - return NULL; - } - /* convert it to our internal representation */ cpu_model_from_info(&s390_model, model, "model", &err); if (err) { @@ -241,7 +244,8 @@ CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType type, /* convert it back to a static representation */ expansion_info = g_new0(CpuModelExpansionInfo, 1); expansion_info->model = g_malloc0(sizeof(*expansion_info->model)); - cpu_info_from_model(expansion_info->model, &s390_model, delta_changes); + cpu_info_from_model(expansion_info->model, &s390_model, + delta_changes, disable_deprecated_feats); return expansion_info; } @@ -390,7 +394,7 @@ CpuModelBaselineInfo *qmp_query_cpu_model_baseline(CpuModelInfo *infoa, baseline_info = g_new0(CpuModelBaselineInfo, 1); baseline_info->model = g_malloc0(sizeof(*baseline_info->model)); - cpu_info_from_model(baseline_info->model, &model, true); + cpu_info_from_model(baseline_info->model, &model, true, false); return baseline_info; }
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 <walling@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(-)