[PATCH v1] target/s390x: move @deprecated-props to CpuModelExpansion Info

[PATCH v1] target/s390x: move @deprecated-props to CpuModelExpansion Info

[Collin Walling]

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(-)

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;
 }
 
-- 
2.45.1

Re: [PATCH v1] target/s390x: move @deprecated-props to CpuModelExpansion Info

[David Hildenbrand]

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?

-- 
Cheers,

David / dhildenb

Re: [PATCH v1] target/s390x: move @deprecated-props to CpuModelExpansion Info

[Collin Walling]

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!

-- 
Regards,
  Collin

Re: [PATCH v1] target/s390x: move @deprecated-props to CpuModelExpansion Info

[Markus Armbruster]

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.

Re: [PATCH v1] target/s390x: move @deprecated-props to CpuModelExpansion Info

[David Hildenbrand]

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?

-- 
Cheers,

David / dhildenb

Re: [PATCH v1] target/s390x: move @deprecated-props to CpuModelExpansion Info

[Markus Armbruster]

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?

Re: [PATCH v1] target/s390x: move @deprecated-props to CpuModelExpansion Info

[David Hildenbrand]

>>> 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;
  }
  
-- 
2.45.2


-- 
Cheers,

David / dhildenb

Re: [PATCH v1] target/s390x: move @deprecated-props to CpuModelExpansion Info

[Markus Armbruster]

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!

Re: [PATCH v1] target/s390x: move @deprecated-props to CpuModelExpansion Info

[Collin Walling]

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.

-- 
Regards,
  Collin

Re: [PATCH v1] target/s390x: move @deprecated-props to CpuModelExpansion Info

[David Hildenbrand]

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!

-- 
Cheers,

David / dhildenb

Re: [PATCH v1] target/s390x: move @deprecated-props to CpuModelExpansion Info

[Collin Walling]

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.

-- 
Regards,
  Collin

Re: [PATCH v1] target/s390x: move @deprecated-props to CpuModelExpansion Info

[David Hildenbrand]

> 
>> +    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!

-- 
Cheers,

David / dhildenb