| Message ID | 20240719181741.35146-1-walling@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] target/s390x: filter deprecated properties based on model expansion type | expand |
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". > # > # Since: 2.8 > ## > diff --git a/target/s390x/cpu_models_sysemu.c b/target/s390x/cpu_models_sysemu.c > index 977fbc6522..e28ecf7ab9 100644 > --- a/target/s390x/cpu_models_sysemu.c > +++ b/target/s390x/cpu_models_sysemu.c > @@ -174,11 +174,15 @@ static void cpu_info_from_model(CpuModelInfo *info, const S390CPUModel *model, > bool delta_changes) > { > QDict *qdict = qdict_new(); > - S390FeatBitmap bitmap; > + S390FeatBitmap bitmap, deprecated; > > /* always fallback to the static base model */ > info->name = g_strdup_printf("%s-base", model->def->name); > > + /* features flagged as deprecated */ > + bitmap_zero(deprecated, S390_FEAT_MAX); > + s390_get_deprecated_features(deprecated); > + > if (delta_changes) { > /* features deleted from the base feature set */ > bitmap_andnot(bitmap, model->def->base_feat, model->features, > @@ -193,6 +197,9 @@ static void cpu_info_from_model(CpuModelInfo *info, const S390CPUModel *model, > if (!bitmap_empty(bitmap, S390_FEAT_MAX)) { > s390_feat_bitmap_to_ascii(bitmap, qdict, qdict_add_enabled_feat); > } > + > + /* deprecated features that are a subset of the model's enabled features */ Recommend to wrap this line for legibility. > + bitmap_and(deprecated, deprecated, model->features, S390_FEAT_MAX); > } else { > /* expand all features */ > s390_feat_bitmap_to_ascii(model->features, qdict, > @@ -207,12 +214,7 @@ static void cpu_info_from_model(CpuModelInfo *info, const S390CPUModel *model, > info->props = QOBJECT(qdict); > } > > - /* features flagged as deprecated */ > - bitmap_zero(bitmap, S390_FEAT_MAX); > - s390_get_deprecated_features(bitmap); > - > - bitmap_and(bitmap, bitmap, model->def->full_feat, S390_FEAT_MAX); > - s390_feat_bitmap_to_ascii(bitmap, &info->deprecated_props, list_add_feat); > + s390_feat_bitmap_to_ascii(deprecated, &info->deprecated_props, list_add_feat); > info->has_deprecated_props = !!info->deprecated_props; > }
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.
On 22/07/2024 16.50, Collin Walling wrote: > 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"? ... > > Thanks for the feedback! Pending your response to the above, I'll post > a v4. Since we've got soft-freeze for 9.1 today, I went ahead and put v3 into my last pull-request before the freeze period starts already. Please post the update to the comments as diff on top of that instead - updates to comments should still be fine for merging during the freeze period. Thanks, Thomas
On 7/23/24 5:23 AM, Thomas Huth wrote: > On 22/07/2024 16.50, Collin Walling wrote: >> 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"? > ... >> >> Thanks for the feedback! Pending your response to the above, I'll post >> a v4. > > Since we've got soft-freeze for 9.1 today, I went ahead and put v3 into my > last pull-request before the freeze period starts already. Please post the > update to the comments as diff on top of that instead - updates to comments > should still be fine for merging during the freeze period. > > Thanks, > Thomas > > This is greatly appreciated, Thomas. Thank you!
Collin Walling <walling@linux.ibm.com> writes: > 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? The distance between @deprecated-props and parts of its documentation bothers me a bit. On closer examination, more questions on CpuModelInfo emerge. Uses: * query-cpu-model-comparison both arguments Documentation doesn't say how exactly the command uses the members of CpuModelInfo, i.e. @name, @props, @deprecated-props. Can you tell me? * query-cpu-model-expansion argument @model and return value member @model. The other argument is the expansion type, on which the value of return value model.deprecated-props depends, I believe. Fine. Documentation doesn't say how exactly the command uses the members of CpuModelInfo arguments, i.e. @name, @props, @deprecated-props. Can you tell me? * query-cpu-model-baseline both arguments and return value member @model. Same, except we don't have an expansion type here. So same question, plus another one: how does return value model.deprecated-props behave? If you can't answer my questions, we need to find someone who can. [...]
On 7/24/24 3:56 AM, Markus Armbruster wrote: > Collin Walling <walling@linux.ibm.com> writes: > >> 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? > > The distance between @deprecated-props and parts of its documentation > bothers me a bit. Let me try to explain the purpose of @deprecated-props and see if it helps bring us closer to some semblance of a mutual understanding so we can work together on a concise documentation for this field. s390 has been announcing features as deprecated for some time now, which was fine as a way to let users know that they should tune their guests to no longer user these features. Now that we are approaching the release of generations that will drop these deprecated features outright, we encounter an issue: if users have not been mindful with disabling these announced-deprecated-features, then their guests running on older models will not be able to migrate to machines running on newer hardware. To alleviate this, I've added the @deprecated-props array to the CpuModelInfo struct, and this field is populated by a query-cpu-model-expansion* return. It is up the the user/management app to make use of this data. On the libvirt side (currently in development), I am able to easily retrieve the host-model with a full expansion, parse the @deprecated-props, and then cache them for later use (e.g. when reporting the host-model with these features disabled, or enabling a user to define their domain with deprecated-features disabled via a convenient XML attribute). tl;dr @deprecated-props is only reported via a query-cpu-model-expansion, and it is up to the user/management app to figure out what to do with them. > > On closer examination, more questions on CpuModelInfo emerge. Uses: > I will attempt to expand on each input @model (CpuModelInfo) as if they were documented in the file. > * query-cpu-model-comparison both arguments > > Documentation doesn't say how exactly the command uses the members of > CpuModelInfo, i.e. @name, @props, @deprecated-props. Can you tell me? > Note: Compares ModelA and ModelB. Both @models must include @name. @props is optional. @deprecated-props is ignored. @name: the name of the CPU model definition to look up. The definition will be compared against the generation, GA level, and a static set of properties of the opposing model. @props: a set of additional properties to include in the model's set of properties to be compared. @deprecated-props: ignored. The user should consider these properties beforehand and decide if these properties should be disabled/omitted on the respective model. > * query-cpu-model-expansion argument @model and return value member > @model. > > The other argument is the expansion type, on which the value of return > value model.deprecated-props depends, I believe. Fine. > > Documentation doesn't say how exactly the command uses the members of > CpuModelInfo arguments, i.e. @name, @props, @deprecated-props. Can > you tell me? > The @model must include @name. @props is optional. @deprecated-props is ignored. @name: the name of the CPU model definition to look up. The definition is associated with a set of properties that will populate the return data. @props: a set of additional properties to include in the model's set of expanded properties. @deprecated-props: ignored. The user should consider these properties beforehand and decide if these properties should be disabled/omitted on the model. > * query-cpu-model-baseline both arguments and return value member > @model. > > Same, except we don't have an expansion type here. So same question, > plus another one: how does return value model.deprecated-props behave? > Note: Creates a baseline model based on ModelA and ModelB. The @models must include @name. @props is optional. @deprecated-props is ignored. @name: the name of the CPU model definition to look up. The definition, GA level, and a static set of properties will be used to determine the maximum model between ModelA and ModelB. @props: a set of additional properties to include in the model's set of properties to be baselined. @deprecated-props: ignored. The user should consider these properties beforehand and decide if these properties should be disabled/omitted on the respective model. > If you can't answer my questions, we need to find someone who can. > Hopefully this provides clarity on how CpuModelInfo and its respective fields are used in each command. @David should be able to fill in any missing areas / expand / offer corrections. > [...] > >
Collin Walling <walling@linux.ibm.com> writes: > On 7/24/24 3:56 AM, Markus Armbruster wrote: >> Collin Walling <walling@linux.ibm.com> writes: > Let me try to explain the purpose of @deprecated-props and see if it > helps bring us closer to some semblance of a mutual understanding so we > can work together on a concise documentation for this field. > > s390 has been announcing features as deprecated for some time now, which > was fine as a way to let users know that they should tune their guests > to no longer user these features. Now that we are approaching the > release of generations that will drop these deprecated features > outright, we encounter an issue: if users have not been mindful with > disabling these announced-deprecated-features, then their guests running > on older models will not be able to migrate to machines running on newer > hardware. > > To alleviate this, I've added the @deprecated-props array to the > CpuModelInfo struct, and this field is populated by a > query-cpu-model-expansion* return. It is up the the user/management app > to make use of this data. > > On the libvirt side (currently in development), I am able to easily > retrieve the host-model with a full expansion, parse the > @deprecated-props, and then cache them for later use (e.g. when > reporting the host-model with these features disabled, or enabling a > user to define their domain with deprecated-features disabled via a > convenient XML attribute). > > tl;dr @deprecated-props is only reported via a > query-cpu-model-expansion, and it is up to the user/management app to > figure out what to do with them. Got it. Permit me a digression. In QAPI/QMP, we do something similar: we expose deprecation in introspection (query-qmp-schema), and what to do with the information is up to the management application. We provide one more tool to it: policy for handling deprecated interfaces, set with -compat. It permits "testing the future". See qapi/compat.json for details. Whether such a thing would be usful in your case I can't say. >> On closer examination, more questions on CpuModelInfo emerge. Uses: >> > > I will attempt to expand on each input @model (CpuModelInfo) as if they > were documented in the file. > >> * query-cpu-model-comparison both arguments >> >> Documentation doesn't say how exactly the command uses the members of >> CpuModelInfo, i.e. @name, @props, @deprecated-props. Can you tell me? >> > > Note: Compares ModelA and ModelB. > > Both @models must include @name. @props is optional. @deprecated-props > is ignored. > > @name: the name of the CPU model definition to look up. The definition > will be compared against the generation, GA level, and a static set of > properties of the opposing model. > > @props: a set of additional properties to include in the model's set of > properties to be compared. > > @deprecated-props: ignored. The user should consider these properties > beforehand and decide if these properties should be disabled/omitted on > the respective model. > >> * query-cpu-model-expansion argument @model and return value member >> @model. >> >> The other argument is the expansion type, on which the value of return >> value model.deprecated-props depends, I believe. Fine. >> >> Documentation doesn't say how exactly the command uses the members of >> CpuModelInfo arguments, i.e. @name, @props, @deprecated-props. Can >> you tell me? >> > > The @model must include @name. @props is optional. @deprecated-props > is ignored. > > @name: the name of the CPU model definition to look up. The definition > is associated with a set of properties that will populate the return data. > > @props: a set of additional properties to include in the model's set of > expanded properties. > > @deprecated-props: ignored. The user should consider these properties > beforehand and decide if these properties should be disabled/omitted on > the model. Return value member @model will have @name, may have @props and @deprecated-props. Absent @props is the same as {}. Only x86 uses {}. Absent @deprecated-props is the same as {}. No target uses {}. Can be present only on S390. Aside: returning the same thing in two different ways, like absent and {}, is slightly more complex than necessary. But let's ignore that here. >> * query-cpu-model-baseline both arguments and return value member >> @model. >> >> Same, except we don't have an expansion type here. So same question, >> plus another one: how does return value model.deprecated-props behave? >> > > Note: Creates a baseline model based on ModelA and ModelB. > > The @models must include @name. @props is optional. @deprecated-props > is ignored. > > @name: the name of the CPU model definition to look up. The definition, > GA level, and a static set of properties will be used to determine the > maximum model between ModelA and ModelB. > > @props: a set of additional properties to include in the model's set of > properties to be baselined. > > @deprecated-props: ignored. The user should consider these properties > beforehand and decide if these properties should be disabled/omitted on > the respective model. Return value member @model is just like in query-cpu-model-expansion. Unlike query-cpu-model-expansion, we don't have an expansion type. The value of @deprecated-props depends on the expansion type. Do we assume a type? Which one? >> If you can't answer my questions, we need to find someone who can. >> > > Hopefully this provides clarity on how CpuModelInfo and its respective > fields are used in each command. @David should be able to fill in any > missing areas / expand / offer corrections. > >> [...] This helps, thanks! Arguments that are silently ignored is bad interface design. Observe: when CpuModelInfo is an argument, @deprecated-props is always ignored. When it's a return value, absent means {}, and it can be present only for certain targets (currently S390). The reason we end up with an argument we ignore is laziness: we use the same type for both roles. We can fix that easily: { 'struct': 'CpuModel', 'data': { 'name': 'str', '*props': 'any' } } { 'struct': 'CpuModelInfo', 'base': 'CpuModel', 'data': { '*deprecated-props': ['str'] } } Use CpuModel for arguments, CpuModelInfo for return values. Since @deprecated-props is used only by some targets, I'd make it conditional, i.e. 'if': 'TARGET_S390X'. Thoughts?
Markus Armbruster <armbru@redhat.com> writes: > Collin Walling <walling@linux.ibm.com> writes: > >> On 7/24/24 3:56 AM, Markus Armbruster wrote: >>> Collin Walling <walling@linux.ibm.com> writes: >> Let me try to explain the purpose of @deprecated-props and see if it >> helps bring us closer to some semblance of a mutual understanding so we >> can work together on a concise documentation for this field. >> >> s390 has been announcing features as deprecated for some time now, which >> was fine as a way to let users know that they should tune their guests >> to no longer user these features. Now that we are approaching the >> release of generations that will drop these deprecated features >> outright, we encounter an issue: if users have not been mindful with >> disabling these announced-deprecated-features, then their guests running >> on older models will not be able to migrate to machines running on newer >> hardware. >> >> To alleviate this, I've added the @deprecated-props array to the >> CpuModelInfo struct, and this field is populated by a >> query-cpu-model-expansion* return. It is up the the user/management app >> to make use of this data. >> >> On the libvirt side (currently in development), I am able to easily >> retrieve the host-model with a full expansion, parse the >> @deprecated-props, and then cache them for later use (e.g. when >> reporting the host-model with these features disabled, or enabling a >> user to define their domain with deprecated-features disabled via a >> convenient XML attribute). >> >> tl;dr @deprecated-props is only reported via a >> query-cpu-model-expansion, and it is up to the user/management app to >> figure out what to do with them. > > Got it. > > Permit me a digression. In QAPI/QMP, we do something similar: we expose > deprecation in introspection (query-qmp-schema), and what to do with the > information is up to the management application. We provide one more > tool to it: policy for handling deprecated interfaces, set with -compat. > It permits "testing the future". See qapi/compat.json for details. > Whether such a thing would be usful in your case I can't say. > >>> On closer examination, more questions on CpuModelInfo emerge. Uses: >>> >> >> I will attempt to expand on each input @model (CpuModelInfo) as if they >> were documented in the file. >> >>> * query-cpu-model-comparison both arguments >>> >>> Documentation doesn't say how exactly the command uses the members of >>> CpuModelInfo, i.e. @name, @props, @deprecated-props. Can you tell me? >>> >> >> Note: Compares ModelA and ModelB. >> >> Both @models must include @name. @props is optional. @deprecated-props >> is ignored. >> >> @name: the name of the CPU model definition to look up. The definition >> will be compared against the generation, GA level, and a static set of >> properties of the opposing model. >> >> @props: a set of additional properties to include in the model's set of >> properties to be compared. >> >> @deprecated-props: ignored. The user should consider these properties >> beforehand and decide if these properties should be disabled/omitted on >> the respective model. >> >>> * query-cpu-model-expansion argument @model and return value member >>> @model. >>> >>> The other argument is the expansion type, on which the value of return >>> value model.deprecated-props depends, I believe. Fine. >>> >>> Documentation doesn't say how exactly the command uses the members of >>> CpuModelInfo arguments, i.e. @name, @props, @deprecated-props. Can >>> you tell me? >>> >> >> The @model must include @name. @props is optional. @deprecated-props >> is ignored. >> >> @name: the name of the CPU model definition to look up. The definition >> is associated with a set of properties that will populate the return data. >> >> @props: a set of additional properties to include in the model's set of >> expanded properties. >> >> @deprecated-props: ignored. The user should consider these properties >> beforehand and decide if these properties should be disabled/omitted on >> the model. > > Return value member @model will have @name, may have @props and > @deprecated-props. > > Absent @props is the same as {}. Only x86 uses {}. > > Absent @deprecated-props is the same as {}. No target uses {}. Can be > present only on S390. > > Aside: returning the same thing in two different ways, like absent and > {}, is slightly more complex than necessary. But let's ignore that > here. > >>> * query-cpu-model-baseline both arguments and return value member >>> @model. >>> >>> Same, except we don't have an expansion type here. So same question, >>> plus another one: how does return value model.deprecated-props behave? >>> >> >> Note: Creates a baseline model based on ModelA and ModelB. >> >> The @models must include @name. @props is optional. @deprecated-props >> is ignored. >> >> @name: the name of the CPU model definition to look up. The definition, >> GA level, and a static set of properties will be used to determine the >> maximum model between ModelA and ModelB. >> >> @props: a set of additional properties to include in the model's set of >> properties to be baselined. >> >> @deprecated-props: ignored. The user should consider these properties >> beforehand and decide if these properties should be disabled/omitted on >> the respective model. > > Return value member @model is just like in query-cpu-model-expansion. > > Unlike query-cpu-model-expansion, we don't have an expansion type. The > value of @deprecated-props depends on the expansion type. Do we assume > a type? Which one? > >>> If you can't answer my questions, we need to find someone who can. >>> >> >> Hopefully this provides clarity on how CpuModelInfo and its respective >> fields are used in each command. @David should be able to fill in any >> missing areas / expand / offer corrections. >> >>> [...] > > This helps, thanks! > > Arguments that are silently ignored is bad interface design. > > Observe: when CpuModelInfo is an argument, @deprecated-props is always > ignored. When it's a return value, absent means {}, and it can be > present only for certain targets (currently S390). > > The reason we end up with an argument we ignore is laziness: we use the > same type for both roles. We can fix that easily: > > { 'struct': 'CpuModel', > 'data': { 'name': 'str', > '*props': 'any' } } > > { 'struct': 'CpuModelInfo', > 'base': 'CpuModel', > 'data': { '*deprecated-props': ['str'] } } > > Use CpuModel for arguments, CpuModelInfo for return values. > > Since @deprecated-props is used only by some targets, I'd make it > conditional, i.e. 'if': 'TARGET_S390X'. If we want just query-cpu-model-expansion return deprecated properties, we can instead move @deprecated-props from CpuModelInfo to CpuModelExpansionInfo. > Thoughts?
On 25.07.24 09:35, Markus Armbruster wrote: > Markus Armbruster <armbru@redhat.com> writes: > >> Collin Walling <walling@linux.ibm.com> writes: >> >>> On 7/24/24 3:56 AM, Markus Armbruster wrote: >>>> Collin Walling <walling@linux.ibm.com> writes: >>> Let me try to explain the purpose of @deprecated-props and see if it >>> helps bring us closer to some semblance of a mutual understanding so we >>> can work together on a concise documentation for this field. >>> >>> s390 has been announcing features as deprecated for some time now, which >>> was fine as a way to let users know that they should tune their guests >>> to no longer user these features. Now that we are approaching the >>> release of generations that will drop these deprecated features >>> outright, we encounter an issue: if users have not been mindful with >>> disabling these announced-deprecated-features, then their guests running >>> on older models will not be able to migrate to machines running on newer >>> hardware. >>> >>> To alleviate this, I've added the @deprecated-props array to the >>> CpuModelInfo struct, and this field is populated by a >>> query-cpu-model-expansion* return. It is up the the user/management app >>> to make use of this data. >>> >>> On the libvirt side (currently in development), I am able to easily >>> retrieve the host-model with a full expansion, parse the >>> @deprecated-props, and then cache them for later use (e.g. when >>> reporting the host-model with these features disabled, or enabling a >>> user to define their domain with deprecated-features disabled via a >>> convenient XML attribute). >>> >>> tl;dr @deprecated-props is only reported via a >>> query-cpu-model-expansion, and it is up to the user/management app to >>> figure out what to do with them. >> >> Got it. >> >> Permit me a digression. In QAPI/QMP, we do something similar: we expose >> deprecation in introspection (query-qmp-schema), and what to do with the >> information is up to the management application. We provide one more >> tool to it: policy for handling deprecated interfaces, set with -compat. >> It permits "testing the future". See qapi/compat.json for details. >> Whether such a thing would be usful in your case I can't say. >> >>>> On closer examination, more questions on CpuModelInfo emerge. Uses: >>>> >>> >>> I will attempt to expand on each input @model (CpuModelInfo) as if they >>> were documented in the file. >>> >>>> * query-cpu-model-comparison both arguments >>>> >>>> Documentation doesn't say how exactly the command uses the members of >>>> CpuModelInfo, i.e. @name, @props, @deprecated-props. Can you tell me? >>>> >>> >>> Note: Compares ModelA and ModelB. >>> >>> Both @models must include @name. @props is optional. @deprecated-props >>> is ignored. >>> >>> @name: the name of the CPU model definition to look up. The definition >>> will be compared against the generation, GA level, and a static set of >>> properties of the opposing model. >>> >>> @props: a set of additional properties to include in the model's set of >>> properties to be compared. >>> >>> @deprecated-props: ignored. The user should consider these properties >>> beforehand and decide if these properties should be disabled/omitted on >>> the respective model. >>> >>>> * query-cpu-model-expansion argument @model and return value member >>>> @model. >>>> >>>> The other argument is the expansion type, on which the value of return >>>> value model.deprecated-props depends, I believe. Fine. >>>> >>>> Documentation doesn't say how exactly the command uses the members of >>>> CpuModelInfo arguments, i.e. @name, @props, @deprecated-props. Can >>>> you tell me? >>>> >>> >>> The @model must include @name. @props is optional. @deprecated-props >>> is ignored. >>> >>> @name: the name of the CPU model definition to look up. The definition >>> is associated with a set of properties that will populate the return data. >>> >>> @props: a set of additional properties to include in the model's set of >>> expanded properties. >>> >>> @deprecated-props: ignored. The user should consider these properties >>> beforehand and decide if these properties should be disabled/omitted on >>> the model. >> >> Return value member @model will have @name, may have @props and >> @deprecated-props. >> >> Absent @props is the same as {}. Only x86 uses {}. >> >> Absent @deprecated-props is the same as {}. No target uses {}. Can be >> present only on S390. >> >> Aside: returning the same thing in two different ways, like absent and >> {}, is slightly more complex than necessary. But let's ignore that >> here. >> >>>> * query-cpu-model-baseline both arguments and return value member >>>> @model. >>>> >>>> Same, except we don't have an expansion type here. So same question, >>>> plus another one: how does return value model.deprecated-props behave? >>>> >>> >>> Note: Creates a baseline model based on ModelA and ModelB. >>> >>> The @models must include @name. @props is optional. @deprecated-props >>> is ignored. >>> >>> @name: the name of the CPU model definition to look up. The definition, >>> GA level, and a static set of properties will be used to determine the >>> maximum model between ModelA and ModelB. >>> >>> @props: a set of additional properties to include in the model's set of >>> properties to be baselined. >>> >>> @deprecated-props: ignored. The user should consider these properties >>> beforehand and decide if these properties should be disabled/omitted on >>> the respective model. >> >> Return value member @model is just like in query-cpu-model-expansion. >> >> Unlike query-cpu-model-expansion, we don't have an expansion type. The >> value of @deprecated-props depends on the expansion type. Do we assume >> a type? Which one? >> >>>> If you can't answer my questions, we need to find someone who can. >>>> >>> >>> Hopefully this provides clarity on how CpuModelInfo and its respective >>> fields are used in each command. @David should be able to fill in any >>> missing areas / expand / offer corrections. >>> >>>> [...] >> >> This helps, thanks! >> >> Arguments that are silently ignored is bad interface design. >> >> Observe: when CpuModelInfo is an argument, @deprecated-props is always >> ignored. When it's a return value, absent means {}, and it can be >> present only for certain targets (currently S390). >> >> The reason we end up with an argument we ignore is laziness: we use the >> same type for both roles. We can fix that easily: >> >> { 'struct': 'CpuModel', >> 'data': { 'name': 'str', >> '*props': 'any' } } >> >> { 'struct': 'CpuModelInfo', >> 'base': 'CpuModel', >> 'data': { '*deprecated-props': ['str'] } } >> >> Use CpuModel for arguments, CpuModelInfo for return values. >> >> Since @deprecated-props is used only by some targets, I'd make it >> conditional, i.e. 'if': 'TARGET_S390X'. > > If we want just query-cpu-model-expansion return deprecated properties, > we can instead move @deprecated-props from CpuModelInfo to > CpuModelExpansionInfo. That might a bit more sense, because deprecated-props does not make any sense as input parameter, for example.
On 7/25/24 3:39 AM, David Hildenbrand wrote: > On 25.07.24 09:35, Markus Armbruster wrote: >> Markus Armbruster <armbru@redhat.com> writes: >> >>> Collin Walling <walling@linux.ibm.com> writes: >>> >>>> On 7/24/24 3:56 AM, Markus Armbruster wrote: >>>>> Collin Walling <walling@linux.ibm.com> writes: >>>> Let me try to explain the purpose of @deprecated-props and see if it >>>> helps bring us closer to some semblance of a mutual understanding so we >>>> can work together on a concise documentation for this field. >>>> >>>> s390 has been announcing features as deprecated for some time now, which >>>> was fine as a way to let users know that they should tune their guests >>>> to no longer user these features. Now that we are approaching the >>>> release of generations that will drop these deprecated features >>>> outright, we encounter an issue: if users have not been mindful with >>>> disabling these announced-deprecated-features, then their guests running >>>> on older models will not be able to migrate to machines running on newer >>>> hardware. >>>> >>>> To alleviate this, I've added the @deprecated-props array to the >>>> CpuModelInfo struct, and this field is populated by a >>>> query-cpu-model-expansion* return. It is up the the user/management app >>>> to make use of this data. >>>> >>>> On the libvirt side (currently in development), I am able to easily >>>> retrieve the host-model with a full expansion, parse the >>>> @deprecated-props, and then cache them for later use (e.g. when >>>> reporting the host-model with these features disabled, or enabling a >>>> user to define their domain with deprecated-features disabled via a >>>> convenient XML attribute). >>>> >>>> tl;dr @deprecated-props is only reported via a >>>> query-cpu-model-expansion, and it is up to the user/management app to >>>> figure out what to do with them. >>> >>> Got it. >>> >>> Permit me a digression. In QAPI/QMP, we do something similar: we expose >>> deprecation in introspection (query-qmp-schema), and what to do with the >>> information is up to the management application. We provide one more >>> tool to it: policy for handling deprecated interfaces, set with -compat. >>> It permits "testing the future". See qapi/compat.json for details. >>> Whether such a thing would be usful in your case I can't say. >>> >>>>> On closer examination, more questions on CpuModelInfo emerge. Uses: >>>>> >>>> >>>> I will attempt to expand on each input @model (CpuModelInfo) as if they >>>> were documented in the file. >>>> >>>>> * query-cpu-model-comparison both arguments >>>>> >>>>> Documentation doesn't say how exactly the command uses the members of >>>>> CpuModelInfo, i.e. @name, @props, @deprecated-props. Can you tell me? >>>>> >>>> >>>> Note: Compares ModelA and ModelB. >>>> >>>> Both @models must include @name. @props is optional. @deprecated-props >>>> is ignored. >>>> >>>> @name: the name of the CPU model definition to look up. The definition >>>> will be compared against the generation, GA level, and a static set of >>>> properties of the opposing model. >>>> >>>> @props: a set of additional properties to include in the model's set of >>>> properties to be compared. >>>> >>>> @deprecated-props: ignored. The user should consider these properties >>>> beforehand and decide if these properties should be disabled/omitted on >>>> the respective model. >>>> >>>>> * query-cpu-model-expansion argument @model and return value member >>>>> @model. >>>>> >>>>> The other argument is the expansion type, on which the value of return >>>>> value model.deprecated-props depends, I believe. Fine. >>>>> >>>>> Documentation doesn't say how exactly the command uses the members of >>>>> CpuModelInfo arguments, i.e. @name, @props, @deprecated-props. Can >>>>> you tell me? >>>>> >>>> >>>> The @model must include @name. @props is optional. @deprecated-props >>>> is ignored. >>>> >>>> @name: the name of the CPU model definition to look up. The definition >>>> is associated with a set of properties that will populate the return data. >>>> >>>> @props: a set of additional properties to include in the model's set of >>>> expanded properties. >>>> >>>> @deprecated-props: ignored. The user should consider these properties >>>> beforehand and decide if these properties should be disabled/omitted on >>>> the model. >>> >>> Return value member @model will have @name, may have @props and >>> @deprecated-props. >>> >>> Absent @props is the same as {}. Only x86 uses {}. >>> >>> Absent @deprecated-props is the same as {}. No target uses {}. Can be >>> present only on S390. >>> >>> Aside: returning the same thing in two different ways, like absent and >>> {}, is slightly more complex than necessary. But let's ignore that >>> here. >>> >>>>> * query-cpu-model-baseline both arguments and return value member >>>>> @model. >>>>> >>>>> Same, except we don't have an expansion type here. So same question, >>>>> plus another one: how does return value model.deprecated-props behave? >>>>> >>>> >>>> Note: Creates a baseline model based on ModelA and ModelB. >>>> >>>> The @models must include @name. @props is optional. @deprecated-props >>>> is ignored. >>>> >>>> @name: the name of the CPU model definition to look up. The definition, >>>> GA level, and a static set of properties will be used to determine the >>>> maximum model between ModelA and ModelB. >>>> >>>> @props: a set of additional properties to include in the model's set of >>>> properties to be baselined. >>>> >>>> @deprecated-props: ignored. The user should consider these properties >>>> beforehand and decide if these properties should be disabled/omitted on >>>> the respective model. >>> >>> Return value member @model is just like in query-cpu-model-expansion. >>> >>> Unlike query-cpu-model-expansion, we don't have an expansion type. The >>> value of @deprecated-props depends on the expansion type. Do we assume >>> a type? Which one? >>> >>>>> If you can't answer my questions, we need to find someone who can. >>>>> >>>> >>>> Hopefully this provides clarity on how CpuModelInfo and its respective >>>> fields are used in each command. @David should be able to fill in any >>>> missing areas / expand / offer corrections. >>>> >>>>> [...] >>> >>> This helps, thanks! >>> >>> Arguments that are silently ignored is bad interface design. >>> >>> Observe: when CpuModelInfo is an argument, @deprecated-props is always >>> ignored. When it's a return value, absent means {}, and it can be >>> present only for certain targets (currently S390). >>> >>> The reason we end up with an argument we ignore is laziness: we use the >>> same type for both roles. We can fix that easily: >>> >>> { 'struct': 'CpuModel', >>> 'data': { 'name': 'str', >>> '*props': 'any' } } >>> >>> { 'struct': 'CpuModelInfo', >>> 'base': 'CpuModel', >>> 'data': { '*deprecated-props': ['str'] } } >>> >>> Use CpuModel for arguments, CpuModelInfo for return values. >>> >>> Since @deprecated-props is used only by some targets, I'd make it >>> conditional, i.e. 'if': 'TARGET_S390X'. >> >> If we want just query-cpu-model-expansion return deprecated properties, >> we can instead move @deprecated-props from CpuModelInfo to >> CpuModelExpansionInfo. > > That might a bit more sense, because deprecated-props does not make any > sense as input parameter, for example. > Will do. Thanks for the feedback. v4 in the works.
Collin Walling <walling@linux.ibm.com> writes: > On 7/25/24 3:39 AM, David Hildenbrand wrote: >> On 25.07.24 09:35, Markus Armbruster wrote: >>> Markus Armbruster <armbru@redhat.com> writes: [...] >>>> Arguments that are silently ignored is bad interface design. >>>> >>>> Observe: when CpuModelInfo is an argument, @deprecated-props is always >>>> ignored. When it's a return value, absent means {}, and it can be >>>> present only for certain targets (currently S390). >>>> >>>> The reason we end up with an argument we ignore is laziness: we use the >>>> same type for both roles. We can fix that easily: >>>> >>>> { 'struct': 'CpuModel', >>>> 'data': { 'name': 'str', >>>> '*props': 'any' } } >>>> >>>> { 'struct': 'CpuModelInfo', >>>> 'base': 'CpuModel', >>>> 'data': { '*deprecated-props': ['str'] } } >>>> >>>> Use CpuModel for arguments, CpuModelInfo for return values. >>>> >>>> Since @deprecated-props is used only by some targets, I'd make it >>>> conditional, i.e. 'if': 'TARGET_S390X'. >>> >>> If we want just query-cpu-model-expansion return deprecated properties, >>> we can instead move @deprecated-props from CpuModelInfo to >>> CpuModelExpansionInfo. >> >> That might a bit more sense, because deprecated-props does not make any >> sense as input parameter, for example. > > Will do. Thanks for the feedback. v4 in the works. We better get this into 9.1. Plan B: mark @deprecated-props unstable to avoid backward compatibility pain.
On 7/26/24 3:15 AM, Markus Armbruster wrote: > Collin Walling <walling@linux.ibm.com> writes: > >> On 7/25/24 3:39 AM, David Hildenbrand wrote: >>> On 25.07.24 09:35, Markus Armbruster wrote: >>>> Markus Armbruster <armbru@redhat.com> writes: > > [...] > >>>>> Arguments that are silently ignored is bad interface design. >>>>> >>>>> Observe: when CpuModelInfo is an argument, @deprecated-props is always >>>>> ignored. When it's a return value, absent means {}, and it can be >>>>> present only for certain targets (currently S390). >>>>> >>>>> The reason we end up with an argument we ignore is laziness: we use the >>>>> same type for both roles. We can fix that easily: >>>>> >>>>> { 'struct': 'CpuModel', >>>>> 'data': { 'name': 'str', >>>>> '*props': 'any' } } >>>>> >>>>> { 'struct': 'CpuModelInfo', >>>>> 'base': 'CpuModel', >>>>> 'data': { '*deprecated-props': ['str'] } } >>>>> >>>>> Use CpuModel for arguments, CpuModelInfo for return values. >>>>> >>>>> Since @deprecated-props is used only by some targets, I'd make it >>>>> conditional, i.e. 'if': 'TARGET_S390X'. >>>> >>>> If we want just query-cpu-model-expansion return deprecated properties, >>>> we can instead move @deprecated-props from CpuModelInfo to >>>> CpuModelExpansionInfo. >>> >>> That might a bit more sense, because deprecated-props does not make any >>> sense as input parameter, for example. >> >> Will do. Thanks for the feedback. v4 in the works. > > We better get this into 9.1. Plan B: mark @deprecated-props unstable to > avoid backward compatibility pain. > > Agreed, it would go a long way to squeeze this in before things get too messy. v4 is posted. I think Thomas is unavailable, so if @David would not mind handling this? I'm confident that including this data within the expansion response is the right way to go. If there are any lingering discrepancies WRT documentation, that can be fixed later.
On 26.07.24 21:11, Collin Walling wrote: > On 7/26/24 3:15 AM, Markus Armbruster wrote: >> Collin Walling <walling@linux.ibm.com> writes: >> >>> On 7/25/24 3:39 AM, David Hildenbrand wrote: >>>> On 25.07.24 09:35, Markus Armbruster wrote: >>>>> Markus Armbruster <armbru@redhat.com> writes: >> >> [...] >> >>>>>> Arguments that are silently ignored is bad interface design. >>>>>> >>>>>> Observe: when CpuModelInfo is an argument, @deprecated-props is always >>>>>> ignored. When it's a return value, absent means {}, and it can be >>>>>> present only for certain targets (currently S390). >>>>>> >>>>>> The reason we end up with an argument we ignore is laziness: we use the >>>>>> same type for both roles. We can fix that easily: >>>>>> >>>>>> { 'struct': 'CpuModel', >>>>>> 'data': { 'name': 'str', >>>>>> '*props': 'any' } } >>>>>> >>>>>> { 'struct': 'CpuModelInfo', >>>>>> 'base': 'CpuModel', >>>>>> 'data': { '*deprecated-props': ['str'] } } >>>>>> >>>>>> Use CpuModel for arguments, CpuModelInfo for return values. >>>>>> >>>>>> Since @deprecated-props is used only by some targets, I'd make it >>>>>> conditional, i.e. 'if': 'TARGET_S390X'. >>>>> >>>>> If we want just query-cpu-model-expansion return deprecated properties, >>>>> we can instead move @deprecated-props from CpuModelInfo to >>>>> CpuModelExpansionInfo. >>>> >>>> That might a bit more sense, because deprecated-props does not make any >>>> sense as input parameter, for example. >>> >>> Will do. Thanks for the feedback. v4 in the works. >> >> We better get this into 9.1. Plan B: mark @deprecated-props unstable to >> avoid backward compatibility pain. >> >> > > Agreed, it would go a long way to squeeze this in before things get too > messy. > > v4 is posted. I think Thomas is unavailable, so if @David would not > mind handling this? Will do!
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. # # Since: 2.8 ## diff --git a/target/s390x/cpu_models_sysemu.c b/target/s390x/cpu_models_sysemu.c index 977fbc6522..e28ecf7ab9 100644 --- a/target/s390x/cpu_models_sysemu.c +++ b/target/s390x/cpu_models_sysemu.c @@ -174,11 +174,15 @@ static void cpu_info_from_model(CpuModelInfo *info, const S390CPUModel *model, bool delta_changes) { QDict *qdict = qdict_new(); - S390FeatBitmap bitmap; + S390FeatBitmap bitmap, deprecated; /* always fallback to the static base model */ info->name = g_strdup_printf("%s-base", model->def->name); + /* features flagged as deprecated */ + bitmap_zero(deprecated, S390_FEAT_MAX); + s390_get_deprecated_features(deprecated); + if (delta_changes) { /* features deleted from the base feature set */ bitmap_andnot(bitmap, model->def->base_feat, model->features, @@ -193,6 +197,9 @@ static void cpu_info_from_model(CpuModelInfo *info, const S390CPUModel *model, if (!bitmap_empty(bitmap, S390_FEAT_MAX)) { s390_feat_bitmap_to_ascii(bitmap, qdict, qdict_add_enabled_feat); } + + /* deprecated features that are a subset of the model's enabled features */ + bitmap_and(deprecated, deprecated, model->features, S390_FEAT_MAX); } else { /* expand all features */ s390_feat_bitmap_to_ascii(model->features, qdict, @@ -207,12 +214,7 @@ static void cpu_info_from_model(CpuModelInfo *info, const S390CPUModel *model, info->props = QOBJECT(qdict); } - /* features flagged as deprecated */ - bitmap_zero(bitmap, S390_FEAT_MAX); - s390_get_deprecated_features(bitmap); - - bitmap_and(bitmap, bitmap, model->def->full_feat, S390_FEAT_MAX); - s390_feat_bitmap_to_ascii(bitmap, &info->deprecated_props, list_add_feat); + s390_feat_bitmap_to_ascii(deprecated, &info->deprecated_props, list_add_feat); info->has_deprecated_props = !!info->deprecated_props; }