| Message ID | 20240726203646.20279-1-walling@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v1] target/s390x: move @deprecated-props to CpuModelExpansion Info | expand |
On 26.07.24 22:36, Collin Walling wrote: > The @deprecated-props array did not make any sense to be a member of the > CpuModelInfo struct, since this field would only be populated by a > query-cpu-model-expansion response and ignored otherwise. Move this > field to the CpuModelExpansionInfo struct where is makes more sense. > > References: > - https://lists.gnu.org/archive/html/qemu-devel/2024-07/msg05996.html > - commit eed0e8ffa38f0695c0519508f6e4f5a3297cbd67 > > Signed-off-by: Collin Walling <walling@linux.ibm.com> > --- > > @David, the previous commit header did not align with the changes made > here, so I tagged this as a "v1" but added the previous conversation as > a reference. I hope this is appropriate? Thanks, I modified the "References" section and converted it to a "Fixes:". It's now: target/s390x: move @deprecated-props to CpuModelExpansion Info The @deprecated-props array did not make any sense to be a member of the CpuModelInfo struct, since this field would only be populated by a query-cpu-model-expansion response and ignored otherwise. Move this field to the CpuModelExpansionInfo struct where is makes more sense. This was identified late during review [1] and we have to fix it up while it's not part of an official QEMU release yet. [1] https://lore.kernel.org/qemu-devel/20240719181741.35146-1-walling@linux.ibm.com/ Message-ID: <20240726203646.20279-1-walling@linux.ibm.com> Fixes: eed0e8ffa38f ("target/s390x: filter deprecated properties based on model expansion type") Signed-off-by: Collin Walling <walling@linux.ibm.com> [ david: add "Fixes", explain why fix is required now and reference to v3 ] Signed-off-by: David Hildenbrand <david@redhat.com> Can you take a quick peek at https://github.com/davidhildenbrand/qemu/tree/s390x-next if everything is alright?
On 7/26/24 5:16 PM, David Hildenbrand wrote: > On 26.07.24 22:36, Collin Walling wrote: >> The @deprecated-props array did not make any sense to be a member of the >> CpuModelInfo struct, since this field would only be populated by a >> query-cpu-model-expansion response and ignored otherwise. Move this >> field to the CpuModelExpansionInfo struct where is makes more sense. >> >> References: >> - https://lists.gnu.org/archive/html/qemu-devel/2024-07/msg05996.html >> - commit eed0e8ffa38f0695c0519508f6e4f5a3297cbd67 >> >> Signed-off-by: Collin Walling <walling@linux.ibm.com> >> --- >> >> @David, the previous commit header did not align with the changes made >> here, so I tagged this as a "v1" but added the previous conversation as >> a reference. I hope this is appropriate? > > Thanks, I modified the "References" section and converted it to a "Fixes:". > It's now: > > target/s390x: move @deprecated-props to CpuModelExpansion Info > > The @deprecated-props array did not make any sense to be a member of the > CpuModelInfo struct, since this field would only be populated by a > query-cpu-model-expansion response and ignored otherwise. Move this > field to the CpuModelExpansionInfo struct where is makes more sense. s/is/it > > This was identified late during review [1] and we have to fix it up > while it's not part of an official QEMU release yet. > > [1] https://lore.kernel.org/qemu-devel/20240719181741.35146-1-walling@linux.ibm.com/ > > Message-ID: <20240726203646.20279-1-walling@linux.ibm.com> > Fixes: eed0e8ffa38f ("target/s390x: filter deprecated properties based on model expansion type") > Signed-off-by: Collin Walling <walling@linux.ibm.com> > [ david: add "Fixes", explain why fix is required now and reference to v3 ] > Signed-off-by: David Hildenbrand <david@redhat.com> > > > > Can you take a quick peek at > https://github.com/davidhildenbrand/qemu/tree/s390x-next > if everything is alright? > Aside from a typo (on my end), everything looks golden. Thanks, David!
Collin Walling <walling@linux.ibm.com> writes: > The @deprecated-props array did not make any sense to be a member of the > CpuModelInfo struct, since this field would only be populated by a > query-cpu-model-expansion response and ignored otherwise. Doesn't query-cpu-model-baseline also return it in its response? It seems to assume the "static" expansion type. > Move this > field to the CpuModelExpansionInfo struct where is makes more sense. > > References: > - https://lists.gnu.org/archive/html/qemu-devel/2024-07/msg05996.html > - commit eed0e8ffa38f0695c0519508f6e4f5a3297cbd67 > > Signed-off-by: Collin Walling <walling@linux.ibm.com> > --- > > @David, the previous commit header did not align with the changes made > here, so I tagged this as a "v1" but added the previous conversation as > a reference. I hope this is appropriate? > > --- > qapi/machine-target.json | 18 ++++++++++-------- > target/s390x/cpu_models_sysemu.c | 31 ++++++++++++++++++++----------- > 2 files changed, 30 insertions(+), 19 deletions(-) > > diff --git a/qapi/machine-target.json b/qapi/machine-target.json > index a552e2b0ce..09dec2b9bb 100644 > --- a/qapi/machine-target.json > +++ b/qapi/machine-target.json > @@ -20,17 +20,11 @@ > # > # @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 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 > ## > { 'struct': 'CpuModelInfo', > 'data': { 'name': 'str', > - '*props': 'any', > - '*deprecated-props': ['str'] } } > + '*props': 'any' } } > > ## > # @CpuModelExpansionType: > @@ -248,10 +242,18 @@ > # > # @model: the expanded CpuModelInfo. > # > +# @deprecated-props: a list of properties that are flagged as deprecated > +# by the CPU vendor. The list depends on the CpuModelExpansionType: > +# "static" properties are a subset of the enabled-properties for > +# the expanded model; "full" properties are a set of properties > +# that are deprecated across all models for the architecture. > +# (since: 9.1). > +# > # Since: 2.8 > ## > { 'struct': 'CpuModelExpansionInfo', > - 'data': { 'model': 'CpuModelInfo' }, > + 'data': { 'model': 'CpuModelInfo', > + '*deprecated-props': ['str'] }, > 'if': { 'any': [ 'TARGET_S390X', > 'TARGET_I386', > 'TARGET_ARM', This solves several interface problems: 1. Removes inappropriate @deprecated-props argument of query-cpu-model-comparison, query-cpu-model-expansion, query-cpu-model-baseline. 2. Removes @deprecated-props return of query-cpu-model-baseline. 3. Properly documents how @deprecated-props depends on the expansion type. Remaining problem: 4. Only S390 implements this. Suggest to capture 1-3 more clearly in the commit message, perhaps like this: CpuModelInfo is used both as command argument and in command returns. Its @deprecated-props array does not make any sense in arguments, and is silently ignored. We actually want it only as return value of query-cpu-model-expansion. Move it from CpuModelInfo to CpuModelExpansionType, and document its dependence on expansion type propetly. The simplest way to address 4 is to tack 'if': 'TARGET_S390X' to @deprecated-props. I recommend to make @deprecated-props mandatory rather than optional then. > diff --git a/target/s390x/cpu_models_sysemu.c b/target/s390x/cpu_models_sysemu.c > index 94dd798b4c..44e7587acb 100644 > --- a/target/s390x/cpu_models_sysemu.c > +++ b/target/s390x/cpu_models_sysemu.c > @@ -174,15 +174,11 @@ static void cpu_info_from_model(CpuModelInfo *info, const S390CPUModel *model, > bool delta_changes) > { > QDict *qdict = qdict_new(); > - S390FeatBitmap bitmap, deprecated; > + S390FeatBitmap bitmap; > > /* 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, > @@ -197,9 +193,6 @@ 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, > @@ -213,9 +206,6 @@ static void cpu_info_from_model(CpuModelInfo *info, const S390CPUModel *model, > } else { > info->props = QOBJECT(qdict); > } > - > - s390_feat_bitmap_to_ascii(deprecated, &info->deprecated_props, list_add_feat); > - info->has_deprecated_props = !!info->deprecated_props; > } > > CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType type, > @@ -226,6 +216,7 @@ CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType type, > CpuModelExpansionInfo *expansion_info = NULL; > S390CPUModel s390_model; > bool delta_changes = false; > + S390FeatBitmap deprecated_feats; > > /* convert it to our internal representation */ > cpu_model_from_info(&s390_model, model, "model", &err); > @@ -245,6 +236,24 @@ CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType type, > 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); > + > + /* populated list of deprecated features */ > + bitmap_zero(deprecated_feats, S390_FEAT_MAX); > + s390_get_deprecated_features(deprecated_feats); > + > + if (delta_changes) { > + /* > + * Only populate deprecated features that are a > + * subset of the features enabled on the CPU model. > + */ > + bitmap_and(deprecated_feats, deprecated_feats, > + s390_model.features, S390_FEAT_MAX); > + } > + > + s390_feat_bitmap_to_ascii(deprecated_feats, > + &expansion_info->deprecated_props, list_add_feat); > + expansion_info->has_deprecated_props = !!expansion_info->deprecated_props; > + > return expansion_info; > } Implementation looks good to me.
On 27.07.24 08:02, Markus Armbruster wrote: > Collin Walling <walling@linux.ibm.com> writes: > >> The @deprecated-props array did not make any sense to be a member of the >> CpuModelInfo struct, since this field would only be populated by a >> query-cpu-model-expansion response and ignored otherwise. > > Doesn't query-cpu-model-baseline also return it in its response? It > seems to assume the "static" expansion type. > >> Move this >> field to the CpuModelExpansionInfo struct where is makes more sense. >> >> References: >> - https://lists.gnu.org/archive/html/qemu-devel/2024-07/msg05996.html >> - commit eed0e8ffa38f0695c0519508f6e4f5a3297cbd67 >> >> Signed-off-by: Collin Walling <walling@linux.ibm.com> >> --- >> >> @David, the previous commit header did not align with the changes made >> here, so I tagged this as a "v1" but added the previous conversation as >> a reference. I hope this is appropriate? >> >> --- >> qapi/machine-target.json | 18 ++++++++++-------- >> target/s390x/cpu_models_sysemu.c | 31 ++++++++++++++++++++----------- >> 2 files changed, 30 insertions(+), 19 deletions(-) >> >> diff --git a/qapi/machine-target.json b/qapi/machine-target.json >> index a552e2b0ce..09dec2b9bb 100644 >> --- a/qapi/machine-target.json >> +++ b/qapi/machine-target.json >> @@ -20,17 +20,11 @@ >> # >> # @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 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 >> ## >> { 'struct': 'CpuModelInfo', >> 'data': { 'name': 'str', >> - '*props': 'any', >> - '*deprecated-props': ['str'] } } >> + '*props': 'any' } } >> >> ## >> # @CpuModelExpansionType: >> @@ -248,10 +242,18 @@ >> # >> # @model: the expanded CpuModelInfo. >> # >> +# @deprecated-props: a list of properties that are flagged as deprecated >> +# by the CPU vendor. The list depends on the CpuModelExpansionType: >> +# "static" properties are a subset of the enabled-properties for >> +# the expanded model; "full" properties are a set of properties >> +# that are deprecated across all models for the architecture. >> +# (since: 9.1). >> +# >> # Since: 2.8 >> ## >> { 'struct': 'CpuModelExpansionInfo', >> - 'data': { 'model': 'CpuModelInfo' }, >> + 'data': { 'model': 'CpuModelInfo', >> + '*deprecated-props': ['str'] }, >> 'if': { 'any': [ 'TARGET_S390X', >> 'TARGET_I386', >> 'TARGET_ARM', > > This solves several interface problems: > > 1. Removes inappropriate @deprecated-props argument of > query-cpu-model-comparison, query-cpu-model-expansion, > query-cpu-model-baseline. > > 2. Removes @deprecated-props return of query-cpu-model-baseline. > > 3. Properly documents how @deprecated-props depends on the expansion > type. > > Remaining problem: > > 4. Only S390 implements this. > > Suggest to capture 1-3 more clearly in the commit message, perhaps like > this: > > CpuModelInfo is used both as command argument and in command > returns. > > Its @deprecated-props array does not make any sense in arguments, > and is silently ignored. We actually want it only as return value > of query-cpu-model-expansion. > > Move it from CpuModelInfo to CpuModelExpansionType, and document > its dependence on expansion type propetly. s/propetly/property/ Sounds good! > > The simplest way to address 4 is to tack 'if': 'TARGET_S390X' to > @deprecated-props. > diff --git a/qapi/machine-target.json b/qapi/machine-target.json index 09dec2b9bb..0be95d559c 100644 --- a/qapi/machine-target.json +++ b/qapi/machine-target.json @@ -253,7 +253,7 @@ ## { 'struct': 'CpuModelExpansionInfo', 'data': { 'model': 'CpuModelInfo', - '*deprecated-props': ['str'] }, + '*deprecated-props' : { 'type': ['str'], 'if': 'TARGET_S390X' } }, 'if': { 'any': [ 'TARGET_S390X', 'TARGET_I386', 'TARGET_ARM', Should do the trick, right? > I recommend to make @deprecated-props mandatory rather than optional > then. Hm, does that make sense judging that previous implementations didn't expose it?
David Hildenbrand <david@redhat.com> writes: > On 27.07.24 08:02, Markus Armbruster wrote: >> Collin Walling <walling@linux.ibm.com> writes: >> >>> The @deprecated-props array did not make any sense to be a member of the >>> CpuModelInfo struct, since this field would only be populated by a >>> query-cpu-model-expansion response and ignored otherwise. >> >> Doesn't query-cpu-model-baseline also return it in its response? It >> seems to assume the "static" expansion type. >> >>> Move this >>> field to the CpuModelExpansionInfo struct where is makes more sense. >>> >>> References: >>> - https://lists.gnu.org/archive/html/qemu-devel/2024-07/msg05996.html >>> - commit eed0e8ffa38f0695c0519508f6e4f5a3297cbd67 >>> >>> Signed-off-by: Collin Walling <walling@linux.ibm.com> >>> --- >>> >>> @David, the previous commit header did not align with the changes made >>> here, so I tagged this as a "v1" but added the previous conversation as >>> a reference. I hope this is appropriate? >>> >>> --- >>> qapi/machine-target.json | 18 ++++++++++-------- >>> target/s390x/cpu_models_sysemu.c | 31 ++++++++++++++++++++----------- >>> 2 files changed, 30 insertions(+), 19 deletions(-) >>> >>> diff --git a/qapi/machine-target.json b/qapi/machine-target.json >>> index a552e2b0ce..09dec2b9bb 100644 >>> --- a/qapi/machine-target.json >>> +++ b/qapi/machine-target.json >>> @@ -20,17 +20,11 @@ >>> # >>> # @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 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 >>> ## >>> { 'struct': 'CpuModelInfo', >>> 'data': { 'name': 'str', >>> - '*props': 'any', >>> - '*deprecated-props': ['str'] } } >>> + '*props': 'any' } } >>> >>> ## >>> # @CpuModelExpansionType: >>> @@ -248,10 +242,18 @@ >>> # >>> # @model: the expanded CpuModelInfo. >>> # >>> +# @deprecated-props: a list of properties that are flagged as deprecated >>> +# by the CPU vendor. The list depends on the CpuModelExpansionType: >>> +# "static" properties are a subset of the enabled-properties for >>> +# the expanded model; "full" properties are a set of properties >>> +# that are deprecated across all models for the architecture. >>> +# (since: 9.1). >>> +# >>> # Since: 2.8 >>> ## >>> { 'struct': 'CpuModelExpansionInfo', >>> - 'data': { 'model': 'CpuModelInfo' }, >>> + 'data': { 'model': 'CpuModelInfo', >>> + '*deprecated-props': ['str'] }, >>> 'if': { 'any': [ 'TARGET_S390X', >>> 'TARGET_I386', >>> 'TARGET_ARM', >> >> This solves several interface problems: >> >> 1. Removes inappropriate @deprecated-props argument of >> query-cpu-model-comparison, query-cpu-model-expansion, >> query-cpu-model-baseline. >> >> 2. Removes @deprecated-props return of query-cpu-model-baseline. >> >> 3. Properly documents how @deprecated-props depends on the expansion >> type. >> >> Remaining problem: >> >> 4. Only S390 implements this. >> >> Suggest to capture 1-3 more clearly in the commit message, perhaps like >> this: >> >> CpuModelInfo is used both as command argument and in command >> returns. >> >> Its @deprecated-props array does not make any sense in arguments, >> and is silently ignored. We actually want it only as return value >> of query-cpu-model-expansion. >> >> Move it from CpuModelInfo to CpuModelExpansionType, and document >> its dependence on expansion type propetly. > > s/propetly/property/ Can't type. > Sounds good! > >> >> The simplest way to address 4 is to tack 'if': 'TARGET_S390X' to >> @deprecated-props. >> > > diff --git a/qapi/machine-target.json b/qapi/machine-target.json > index 09dec2b9bb..0be95d559c 100644 > --- a/qapi/machine-target.json > +++ b/qapi/machine-target.json > @@ -253,7 +253,7 @@ > ## > { 'struct': 'CpuModelExpansionInfo', > 'data': { 'model': 'CpuModelInfo', > - '*deprecated-props': ['str'] }, > + '*deprecated-props' : { 'type': ['str'], 'if': 'TARGET_S390X' } }, > 'if': { 'any': [ 'TARGET_S390X', > 'TARGET_I386', > 'TARGET_ARM', > > > Should do the trick, right? Yes. Break the line before 'if', please. >> I recommend to make @deprecated-props mandatory rather than optional >> then. > > Hm, does that make sense judging that previous implementations didn't expose it? Adding members to the return value is fine. docs/devel/qapi-code-gen.rst section "Compatibility considerations" Clients send commands with argument data, and receive command responses with return data and events with event data. Adding opt-in functionality to the send direction is backwards compatible: adding commands, optional arguments, enumeration values, union and alternate branches; turning an argument type into an alternate of that type; making mandatory arguments optional. Clients oblivious of the new functionality continue to work. Incompatible changes include removing commands, command arguments, enumeration values, union and alternate branches, adding mandatory command arguments, and making optional arguments mandatory. The specified behavior of an absent optional argument should remain the same. With proper documentation, this policy still allows some flexibility; for example, when an optional 'buffer-size' argument is specified to default to a sensible buffer size, the actual default value can still be changed. The specified default behavior is not the exact size of the buffer, only that the default size is sensible. --> Adding functionality to the receive direction is generally backwards --> compatible: adding events, adding return and event data members. --> Clients are expected to ignore the ones they don't know. Removing "unreachable" stuff like events that can't be triggered anymore, optional return or event data members that can't be sent anymore, and return or event data member (enumeration) values that can't be sent anymore makes no difference to clients, except for introspection. The latter can conceivably confuse clients, so tread carefully. Incompatible changes include removing return and event data members. Questions?
>>> The simplest way to address 4 is to tack 'if': 'TARGET_S390X' to >>> @deprecated-props. >>> >> >> diff --git a/qapi/machine-target.json b/qapi/machine-target.json >> index 09dec2b9bb..0be95d559c 100644 >> --- a/qapi/machine-target.json >> +++ b/qapi/machine-target.json >> @@ -253,7 +253,7 @@ >> ## >> { 'struct': 'CpuModelExpansionInfo', >> 'data': { 'model': 'CpuModelInfo', >> - '*deprecated-props': ['str'] }, >> + '*deprecated-props' : { 'type': ['str'], 'if': 'TARGET_S390X' } }, >> 'if': { 'any': [ 'TARGET_S390X', >> 'TARGET_I386', >> 'TARGET_ARM', >> >> >> Should do the trick, right? > > Yes. Break the line before 'if', please. Ack [...] > > Questions? As clear as it can get, thanks! :) That would leave us with: From 8be206168e31b9c3ff89e2b99c57a85d30150194 Mon Sep 17 00:00:00 2001 From: Collin Walling <walling@linux.ibm.com> Date: Fri, 26 Jul 2024 16:36:46 -0400 Subject: [PATCH] target/s390x: move @deprecated-props to CpuModelExpansion Info CpuModelInfo is used both as command argument and in command returns. Its @deprecated-props array does not make any sense in arguments, and is silently ignored. We actually want it only as return value of query-cpu-model-expansion. Move it from CpuModelInfo to CpuModelExpansionType, and document its dependence on expansion type property. This was identified late during review [1] and we have to fix it up while it's not part of an official QEMU release yet. [1] https://lore.kernel.org/qemu-devel/20240719181741.35146-1-walling@linux.ibm.com/ Message-ID: <20240726203646.20279-1-walling@linux.ibm.com> Fixes: eed0e8ffa38f ("target/s390x: filter deprecated properties based on model expansion type") Signed-off-by: Collin Walling <walling@linux.ibm.com> [ david: - add "Fixes", adjust description, reference v3 instead - make property s390x-only and non-optional ] Signed-off-by: David Hildenbrand <david@redhat.com> --- qapi/machine-target.json | 19 +++++++++++-------- target/s390x/cpu_models_sysemu.c | 29 ++++++++++++++++++----------- 2 files changed, 29 insertions(+), 19 deletions(-) diff --git a/qapi/machine-target.json b/qapi/machine-target.json index a552e2b0ce..00bbecc905 100644 --- a/qapi/machine-target.json +++ b/qapi/machine-target.json @@ -20,17 +20,11 @@ # # @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 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 ## { 'struct': 'CpuModelInfo', 'data': { 'name': 'str', - '*props': 'any', - '*deprecated-props': ['str'] } } + '*props': 'any' } } ## # @CpuModelExpansionType: @@ -248,10 +242,19 @@ # # @model: the expanded CpuModelInfo. # +# @deprecated-props: a list of properties that are flagged as deprecated +# by the CPU vendor. The list depends on the CpuModelExpansionType: +# "static" properties are a subset of the enabled-properties for +# the expanded model; "full" properties are a set of properties +# that are deprecated across all models for the architecture. +# (since: 9.1). +# # Since: 2.8 ## { 'struct': 'CpuModelExpansionInfo', - 'data': { 'model': 'CpuModelInfo' }, + 'data': { 'model': 'CpuModelInfo', + 'deprecated-props' : { 'type': ['str'], + 'if': 'TARGET_S390X' } }, 'if': { 'any': [ 'TARGET_S390X', 'TARGET_I386', 'TARGET_ARM', diff --git a/target/s390x/cpu_models_sysemu.c b/target/s390x/cpu_models_sysemu.c index 94dd798b4c..6c8e5c7260 100644 --- a/target/s390x/cpu_models_sysemu.c +++ b/target/s390x/cpu_models_sysemu.c @@ -174,15 +174,11 @@ static void cpu_info_from_model(CpuModelInfo *info, const S390CPUModel *model, bool delta_changes) { QDict *qdict = qdict_new(); - S390FeatBitmap bitmap, deprecated; + S390FeatBitmap bitmap; /* 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, @@ -197,9 +193,6 @@ 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, @@ -213,9 +206,6 @@ static void cpu_info_from_model(CpuModelInfo *info, const S390CPUModel *model, } else { info->props = QOBJECT(qdict); } - - s390_feat_bitmap_to_ascii(deprecated, &info->deprecated_props, list_add_feat); - info->has_deprecated_props = !!info->deprecated_props; } CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType type, @@ -226,6 +216,7 @@ CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType type, CpuModelExpansionInfo *expansion_info = NULL; S390CPUModel s390_model; bool delta_changes = false; + S390FeatBitmap deprecated_feats; /* convert it to our internal representation */ cpu_model_from_info(&s390_model, model, "model", &err); @@ -245,6 +236,22 @@ CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType type, 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); + + /* populated list of deprecated features */ + bitmap_zero(deprecated_feats, S390_FEAT_MAX); + s390_get_deprecated_features(deprecated_feats); + + if (delta_changes) { + /* + * Only populate deprecated features that are a + * subset of the features enabled on the CPU model. + */ + bitmap_and(deprecated_feats, deprecated_feats, + s390_model.features, S390_FEAT_MAX); + } + + s390_feat_bitmap_to_ascii(deprecated_feats, + &expansion_info->deprecated_props, list_add_feat); return expansion_info; }
David Hildenbrand <david@redhat.com> writes: >>>> The simplest way to address 4 is to tack 'if': 'TARGET_S390X' to >>>> @deprecated-props. >>>> >>> >>> diff --git a/qapi/machine-target.json b/qapi/machine-target.json >>> index 09dec2b9bb..0be95d559c 100644 >>> --- a/qapi/machine-target.json >>> +++ b/qapi/machine-target.json >>> @@ -253,7 +253,7 @@ >>> ## >>> { 'struct': 'CpuModelExpansionInfo', >>> 'data': { 'model': 'CpuModelInfo', >>> - '*deprecated-props': ['str'] }, >>> + '*deprecated-props' : { 'type': ['str'], 'if': 'TARGET_S390X' } }, >>> 'if': { 'any': [ 'TARGET_S390X', >>> 'TARGET_I386', >>> 'TARGET_ARM', >>> >>> >>> Should do the trick, right? >> >> Yes. Break the line before 'if', please. > > Ack > > [...] > >> >> Questions? > > As clear as it can get, thanks! :) > > That would leave us with: Looks good to me!
On 7/29/24 10:36 AM, Markus Armbruster wrote: > David Hildenbrand <david@redhat.com> writes: > >>>>> The simplest way to address 4 is to tack 'if': 'TARGET_S390X' to >>>>> @deprecated-props. >>>>> >>>> >>>> diff --git a/qapi/machine-target.json b/qapi/machine-target.json >>>> index 09dec2b9bb..0be95d559c 100644 >>>> --- a/qapi/machine-target.json >>>> +++ b/qapi/machine-target.json >>>> @@ -253,7 +253,7 @@ >>>> ## >>>> { 'struct': 'CpuModelExpansionInfo', >>>> 'data': { 'model': 'CpuModelInfo', >>>> - '*deprecated-props': ['str'] }, >>>> + '*deprecated-props' : { 'type': ['str'], 'if': 'TARGET_S390X' } }, >>>> 'if': { 'any': [ 'TARGET_S390X', >>>> 'TARGET_I386', >>>> 'TARGET_ARM', >>>> >>>> >>>> Should do the trick, right? >>> >>> Yes. Break the line before 'if', please. >> >> Ack >> >> [...] >> >>> >>> Questions? >> >> As clear as it can get, thanks! :) >> >> That would leave us with: > > Looks good to me! > > Fine by me as well. Thanks you both for taking care of this.
On 29.07.24 16:49, Collin Walling wrote: > On 7/29/24 10:36 AM, Markus Armbruster wrote: >> David Hildenbrand <david@redhat.com> writes: >> >>>>>> The simplest way to address 4 is to tack 'if': 'TARGET_S390X' to >>>>>> @deprecated-props. >>>>>> >>>>> >>>>> diff --git a/qapi/machine-target.json b/qapi/machine-target.json >>>>> index 09dec2b9bb..0be95d559c 100644 >>>>> --- a/qapi/machine-target.json >>>>> +++ b/qapi/machine-target.json >>>>> @@ -253,7 +253,7 @@ >>>>> ## >>>>> { 'struct': 'CpuModelExpansionInfo', >>>>> 'data': { 'model': 'CpuModelInfo', >>>>> - '*deprecated-props': ['str'] }, >>>>> + '*deprecated-props' : { 'type': ['str'], 'if': 'TARGET_S390X' } }, >>>>> 'if': { 'any': [ 'TARGET_S390X', >>>>> 'TARGET_I386', >>>>> 'TARGET_ARM', >>>>> >>>>> >>>>> Should do the trick, right? >>>> >>>> Yes. Break the line before 'if', please. >>> >>> Ack >>> >>> [...] >>> >>>> >>>> Questions? >>> >>> As clear as it can get, thanks! :) >>> >>> That would leave us with: >> >> Looks good to me! >> >> > > Fine by me as well. Thanks you both for taking care of this. Okay, I'll let it sit like that in s390x-next for 1 day and then send a PR. Thanks!
On 7/29/24 10:22 AM, David Hildenbrand wrote: >>>> The simplest way to address 4 is to tack 'if': 'TARGET_S390X' to >>>> @deprecated-props. >>>> >>> >>> diff --git a/qapi/machine-target.json b/qapi/machine-target.json >>> index 09dec2b9bb..0be95d559c 100644 >>> --- a/qapi/machine-target.json >>> +++ b/qapi/machine-target.json >>> @@ -253,7 +253,7 @@ >>> ## >>> { 'struct': 'CpuModelExpansionInfo', >>> 'data': { 'model': 'CpuModelInfo', >>> - '*deprecated-props': ['str'] }, >>> + '*deprecated-props' : { 'type': ['str'], 'if': 'TARGET_S390X' } }, >>> 'if': { 'any': [ 'TARGET_S390X', >>> 'TARGET_I386', >>> 'TARGET_ARM', >>> >>> >>> Should do the trick, right? >> >> Yes. Break the line before 'if', please. > > Ack > > [...] > >> >> Questions? > > As clear as it can get, thanks! :) > > That would leave us with: > > > From 8be206168e31b9c3ff89e2b99c57a85d30150194 Mon Sep 17 00:00:00 2001 > From: Collin Walling <walling@linux.ibm.com> > Date: Fri, 26 Jul 2024 16:36:46 -0400 > Subject: [PATCH] target/s390x: move @deprecated-props to CpuModelExpansion > Info > > CpuModelInfo is used both as command argument and in command > returns. > > Its @deprecated-props array does not make any sense in arguments, > and is silently ignored. We actually want it only as return value > of query-cpu-model-expansion. > > Move it from CpuModelInfo to CpuModelExpansionType, and document > its dependence on expansion type property. > > This was identified late during review [1] and we have to fix it up > while it's not part of an official QEMU release yet. > > [1] https://lore.kernel.org/qemu-devel/20240719181741.35146-1-walling@linux.ibm.com/ > > Message-ID: <20240726203646.20279-1-walling@linux.ibm.com> > Fixes: eed0e8ffa38f ("target/s390x: filter deprecated properties based on model expansion type") > Signed-off-by: Collin Walling <walling@linux.ibm.com> > [ david: - add "Fixes", adjust description, reference v3 instead > - make property s390x-only and non-optional ] > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > qapi/machine-target.json | 19 +++++++++++-------- > target/s390x/cpu_models_sysemu.c | 29 ++++++++++++++++++----------- > 2 files changed, 29 insertions(+), 19 deletions(-) > > diff --git a/qapi/machine-target.json b/qapi/machine-target.json > index a552e2b0ce..00bbecc905 100644 > --- a/qapi/machine-target.json > +++ b/qapi/machine-target.json > @@ -20,17 +20,11 @@ > # > # @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 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 > ## > { 'struct': 'CpuModelInfo', > 'data': { 'name': 'str', > - '*props': 'any', > - '*deprecated-props': ['str'] } } > + '*props': 'any' } } > > ## > # @CpuModelExpansionType: > @@ -248,10 +242,19 @@ > # > # @model: the expanded CpuModelInfo. > # > +# @deprecated-props: a list of properties that are flagged as deprecated > +# by the CPU vendor. The list depends on the CpuModelExpansionType: > +# "static" properties are a subset of the enabled-properties for > +# the expanded model; "full" properties are a set of properties > +# that are deprecated across all models for the architecture. > +# (since: 9.1). > +# > # Since: 2.8 > ## > { 'struct': 'CpuModelExpansionInfo', > - 'data': { 'model': 'CpuModelInfo' }, > + 'data': { 'model': 'CpuModelInfo', > + 'deprecated-props' : { 'type': ['str'], > + 'if': 'TARGET_S390X' } }, > 'if': { 'any': [ 'TARGET_S390X', > 'TARGET_I386', > 'TARGET_ARM', > diff --git a/target/s390x/cpu_models_sysemu.c b/target/s390x/cpu_models_sysemu.c > index 94dd798b4c..6c8e5c7260 100644 > --- a/target/s390x/cpu_models_sysemu.c > +++ b/target/s390x/cpu_models_sysemu.c > @@ -174,15 +174,11 @@ static void cpu_info_from_model(CpuModelInfo *info, const S390CPUModel *model, > bool delta_changes) > { > QDict *qdict = qdict_new(); > - S390FeatBitmap bitmap, deprecated; > + S390FeatBitmap bitmap; > > /* 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, > @@ -197,9 +193,6 @@ 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, > @@ -213,9 +206,6 @@ static void cpu_info_from_model(CpuModelInfo *info, const S390CPUModel *model, > } else { > info->props = QOBJECT(qdict); > } > - > - s390_feat_bitmap_to_ascii(deprecated, &info->deprecated_props, list_add_feat); > - info->has_deprecated_props = !!info->deprecated_props; > } > > CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType type, > @@ -226,6 +216,7 @@ CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType type, > CpuModelExpansionInfo *expansion_info = NULL; > S390CPUModel s390_model; > bool delta_changes = false; > + S390FeatBitmap deprecated_feats; > > /* convert it to our internal representation */ > cpu_model_from_info(&s390_model, model, "model", &err); > @@ -245,6 +236,22 @@ CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType type, > 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); > + > + /* populated list of deprecated features */ s/populated/populate > + bitmap_zero(deprecated_feats, S390_FEAT_MAX); > + s390_get_deprecated_features(deprecated_feats); > + > + if (delta_changes) { > + /* > + * Only populate deprecated features that are a > + * subset of the features enabled on the CPU model. > + */ > + bitmap_and(deprecated_feats, deprecated_feats, > + s390_model.features, S390_FEAT_MAX); > + } > + > + s390_feat_bitmap_to_ascii(deprecated_feats, > + &expansion_info->deprecated_props, list_add_feat); > return expansion_info; > } > Eh, just a small nit above due to a typo I made. Other than that, gave it all another run-through just in case and everything is still good.
> >> + bitmap_zero(deprecated_feats, S390_FEAT_MAX); >> + s390_get_deprecated_features(deprecated_feats); >> + >> + if (delta_changes) { >> + /* >> + * Only populate deprecated features that are a >> + * subset of the features enabled on the CPU model. >> + */ >> + bitmap_and(deprecated_feats, deprecated_feats, >> + s390_model.features, S390_FEAT_MAX); >> + } >> + >> + s390_feat_bitmap_to_ascii(deprecated_feats, >> + &expansion_info->deprecated_props, list_add_feat); >> return expansion_info; >> } >> > > Eh, just a small nit above due to a typo I made. Other than that, gave > it all another run-through just in case and everything is still good. > Fixed up, thanks!
diff --git a/qapi/machine-target.json b/qapi/machine-target.json index a552e2b0ce..09dec2b9bb 100644 --- a/qapi/machine-target.json +++ b/qapi/machine-target.json @@ -20,17 +20,11 @@ # # @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 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 ## { 'struct': 'CpuModelInfo', 'data': { 'name': 'str', - '*props': 'any', - '*deprecated-props': ['str'] } } + '*props': 'any' } } ## # @CpuModelExpansionType: @@ -248,10 +242,18 @@ # # @model: the expanded CpuModelInfo. # +# @deprecated-props: a list of properties that are flagged as deprecated +# by the CPU vendor. The list depends on the CpuModelExpansionType: +# "static" properties are a subset of the enabled-properties for +# the expanded model; "full" properties are a set of properties +# that are deprecated across all models for the architecture. +# (since: 9.1). +# # Since: 2.8 ## { 'struct': 'CpuModelExpansionInfo', - 'data': { 'model': 'CpuModelInfo' }, + 'data': { 'model': 'CpuModelInfo', + '*deprecated-props': ['str'] }, 'if': { 'any': [ 'TARGET_S390X', 'TARGET_I386', 'TARGET_ARM', diff --git a/target/s390x/cpu_models_sysemu.c b/target/s390x/cpu_models_sysemu.c index 94dd798b4c..44e7587acb 100644 --- a/target/s390x/cpu_models_sysemu.c +++ b/target/s390x/cpu_models_sysemu.c @@ -174,15 +174,11 @@ static void cpu_info_from_model(CpuModelInfo *info, const S390CPUModel *model, bool delta_changes) { QDict *qdict = qdict_new(); - S390FeatBitmap bitmap, deprecated; + S390FeatBitmap bitmap; /* 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, @@ -197,9 +193,6 @@ 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, @@ -213,9 +206,6 @@ static void cpu_info_from_model(CpuModelInfo *info, const S390CPUModel *model, } else { info->props = QOBJECT(qdict); } - - s390_feat_bitmap_to_ascii(deprecated, &info->deprecated_props, list_add_feat); - info->has_deprecated_props = !!info->deprecated_props; } CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType type, @@ -226,6 +216,7 @@ CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType type, CpuModelExpansionInfo *expansion_info = NULL; S390CPUModel s390_model; bool delta_changes = false; + S390FeatBitmap deprecated_feats; /* convert it to our internal representation */ cpu_model_from_info(&s390_model, model, "model", &err); @@ -245,6 +236,24 @@ CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType type, 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); + + /* populated list of deprecated features */ + bitmap_zero(deprecated_feats, S390_FEAT_MAX); + s390_get_deprecated_features(deprecated_feats); + + if (delta_changes) { + /* + * Only populate deprecated features that are a + * subset of the features enabled on the CPU model. + */ + bitmap_and(deprecated_feats, deprecated_feats, + s390_model.features, S390_FEAT_MAX); + } + + s390_feat_bitmap_to_ascii(deprecated_feats, + &expansion_info->deprecated_props, list_add_feat); + expansion_info->has_deprecated_props = !!expansion_info->deprecated_props; + return expansion_info; }
The @deprecated-props array did not make any sense to be a member of the CpuModelInfo struct, since this field would only be populated by a query-cpu-model-expansion response and ignored otherwise. Move this field to the CpuModelExpansionInfo struct where is makes more sense. References: - https://lists.gnu.org/archive/html/qemu-devel/2024-07/msg05996.html - commit eed0e8ffa38f0695c0519508f6e4f5a3297cbd67 Signed-off-by: Collin Walling <walling@linux.ibm.com> --- @David, the previous commit header did not align with the changes made here, so I tagged this as a "v1" but added the previous conversation as a reference. I hope this is appropriate? --- qapi/machine-target.json | 18 ++++++++++-------- target/s390x/cpu_models_sysemu.c | 31 ++++++++++++++++++++----------- 2 files changed, 30 insertions(+), 19 deletions(-)