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