target/s390x: filter deprecated features based on model expansion type

[PATCH v1] target/s390x: filter deprecated features based on model expansion type

Posted by Collin Walling 4 months, 2 weeks ago

It is beneficial to provide an interface to retrieve *all* deprecated
features in one go. Management applications will need this information
to determine which features need to be disabled regardless of the
host-model's capabilities.

To remedy this, deprecated features are only filtered during a static
expansion. All deperecated features are reported on a full expansion.

Suggested-by: Jiri Denemark <jdenemar@redhat.com>
Signed-off-by: Collin Walling <walling@linux.ibm.com>
---
 target/s390x/cpu_models_sysemu.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/target/s390x/cpu_models_sysemu.c b/target/s390x/cpu_models_sysemu.c
index 977fbc6522..76d15f2e4d 100644
--- a/target/s390x/cpu_models_sysemu.c
+++ b/target/s390x/cpu_models_sysemu.c
@@ -211,7 +211,15 @@ static void cpu_info_from_model(CpuModelInfo *info, const S390CPUModel *model,
     bitmap_zero(bitmap, S390_FEAT_MAX);
     s390_get_deprecated_features(bitmap);
 
-    bitmap_and(bitmap, bitmap, model->def->full_feat, S390_FEAT_MAX);
+    /*
+     * For static model expansion, filter out deprecated features that are
+     * not a subset of the model's feature set. Otherwise, report the entire 
+     * deprecated features list.
+     */
+    if (delta_changes) {
+        bitmap_and(bitmap, bitmap, model->def->full_feat, S390_FEAT_MAX);
+    }
+
     s390_feat_bitmap_to_ascii(bitmap, &info->deprecated_props, list_add_feat);
     info->has_deprecated_props = !!info->deprecated_props;
 }
-- 
2.45.1

Re: [PATCH v1] target/s390x: filter deprecated features based on model expansion type

Posted by Markus Armbruster 4 months, 2 weeks ago

Collin Walling <walling@linux.ibm.com> writes:

> It is beneficial to provide an interface to retrieve *all* deprecated
> features in one go. Management applications will need this information
> to determine which features need to be disabled regardless of the
> host-model's capabilities.
>
> To remedy this, deprecated features are only filtered during a static
> expansion. All deperecated features are reported on a full expansion.
>
> Suggested-by: Jiri Denemark <jdenemar@redhat.com>
> Signed-off-by: Collin Walling <walling@linux.ibm.com>

Which command(s) exactly are affected?

Do they need a doc update?

> ---
>  target/s390x/cpu_models_sysemu.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/target/s390x/cpu_models_sysemu.c b/target/s390x/cpu_models_sysemu.c
> index 977fbc6522..76d15f2e4d 100644
> --- a/target/s390x/cpu_models_sysemu.c
> +++ b/target/s390x/cpu_models_sysemu.c
> @@ -211,7 +211,15 @@ static void cpu_info_from_model(CpuModelInfo *info, const S390CPUModel *model,
>      bitmap_zero(bitmap, S390_FEAT_MAX);
>      s390_get_deprecated_features(bitmap);
>  
> -    bitmap_and(bitmap, bitmap, model->def->full_feat, S390_FEAT_MAX);
> +    /*
> +     * For static model expansion, filter out deprecated features that are
> +     * not a subset of the model's feature set. Otherwise, report the entire 
> +     * deprecated features list.
> +     */
> +    if (delta_changes) {
> +        bitmap_and(bitmap, bitmap, model->def->full_feat, S390_FEAT_MAX);
> +    }
> +
>      s390_feat_bitmap_to_ascii(bitmap, &info->deprecated_props, list_add_feat);
>      info->has_deprecated_props = !!info->deprecated_props;
>  }

Re: [PATCH v1] target/s390x: filter deprecated features based on model expansion type

Posted by Collin Walling 4 months, 1 week ago

On 7/12/24 1:23 AM, Markus Armbruster wrote:
> Collin Walling <walling@linux.ibm.com> writes:
> 
>> It is beneficial to provide an interface to retrieve *all* deprecated
>> features in one go. Management applications will need this information
>> to determine which features need to be disabled regardless of the
>> host-model's capabilities.
>>
>> To remedy this, deprecated features are only filtered during a static
>> expansion. All deperecated features are reported on a full expansion.
>>
>> Suggested-by: Jiri Denemark <jdenemar@redhat.com>
>> Signed-off-by: Collin Walling <walling@linux.ibm.com>
> 
> Which command(s) exactly are affected?
>

The query-cpu-model-expansion result will now report all deprecated
features when a user requests a full expansion.  The inputs are not
affects, but the output is modified.  I will make this more concise on
the v2 commit message.

> Do they need a doc update?
> 

Yes, I forgot to add this.  This is what is currently documented:

##
# @CpuModelInfo:
#
...
#
# @deprecated-props: a list of properties that are flagged as deprecated
#     by the CPU vendor.  These props are a subset of the full model's
#     definition list of properties. (since 9.1)
#

I will change to:

#
# @deprecated-props: a list of properties that are flagged as deprecated
#     by the CPU vendor. These are a subset of the reported @props.
#     (since 9.1)
#

(I will also the correct typo in my commit message).

[...]

Thanks!

-- 
Regards,
  Collin

Re: [PATCH v1] target/s390x: filter deprecated features based on model expansion type

Posted by Markus Armbruster 4 months, 1 week ago

Collin Walling <walling@linux.ibm.com> writes:

> On 7/12/24 1:23 AM, Markus Armbruster wrote:
>> Collin Walling <walling@linux.ibm.com> writes:
>> 
>>> It is beneficial to provide an interface to retrieve *all* deprecated
>>> features in one go. Management applications will need this information
>>> to determine which features need to be disabled regardless of the
>>> host-model's capabilities.
>>>
>>> To remedy this, deprecated features are only filtered during a static
>>> expansion. All deperecated features are reported on a full expansion.
>>>
>>> Suggested-by: Jiri Denemark <jdenemar@redhat.com>
>>> Signed-off-by: Collin Walling <walling@linux.ibm.com>
>> 
>> Which command(s) exactly are affected?
>>
>
> The query-cpu-model-expansion result will now report all deprecated
> features when a user requests a full expansion.  The inputs are not
> affects, but the output is modified.  I will make this more concise on
> the v2 commit message.

Yes, please.  Consider including an example.

>> Do they need a doc update?
>> 
>
> Yes, I forgot to add this.  This is what is currently documented:
>
> ##
> # @CpuModelInfo:
> #
> ...
> #
> # @deprecated-props: a list of properties that are flagged as deprecated
> #     by the CPU vendor.  These props are a subset of the full model's
> #     definition list of properties. (since 9.1)
> #
>
> I will change to:
>
> #
> # @deprecated-props: a list of properties that are flagged as deprecated
> #     by the CPU vendor. These are a subset of the reported @props.
> #     (since 9.1)
> #

Hasn't made it into a release, so we don't have to document the old
behavior.  Fortunate!

Separate sentences with two spaces for consistency, please.

> (I will also the correct typo in my commit message).
>
> [...]
>
> Thanks!

Re: [PATCH v1] target/s390x: filter deprecated features based on model expansion type

Posted by David Hildenbrand 4 months, 2 weeks ago

On 11.07.24 22:32, Collin Walling wrote:
> It is beneficial to provide an interface to retrieve *all* deprecated
> features in one go. Management applications will need this information
> to determine which features need to be disabled regardless of the
> host-model's capabilities.
> 
> To remedy this, deprecated features are only filtered during a static
> expansion. All deperecated features are reported on a full expansion.
> 
> Suggested-by: Jiri Denemark <jdenemar@redhat.com>
> Signed-off-by: Collin Walling <walling@linux.ibm.com>
> ---
>   target/s390x/cpu_models_sysemu.c | 10 +++++++++-
>   1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/target/s390x/cpu_models_sysemu.c b/target/s390x/cpu_models_sysemu.c
> index 977fbc6522..76d15f2e4d 100644
> --- a/target/s390x/cpu_models_sysemu.c
> +++ b/target/s390x/cpu_models_sysemu.c
> @@ -211,7 +211,15 @@ static void cpu_info_from_model(CpuModelInfo *info, const S390CPUModel *model,
>       bitmap_zero(bitmap, S390_FEAT_MAX);
>       s390_get_deprecated_features(bitmap);
>   
> -    bitmap_and(bitmap, bitmap, model->def->full_feat, S390_FEAT_MAX);
> +    /*
> +     * For static model expansion, filter out deprecated features that are
> +     * not a subset of the model's feature set. Otherwise, report the entire
> +     * deprecated features list.
> +     */
> +    if (delta_changes) {
> +        bitmap_and(bitmap, bitmap, model->def->full_feat, S390_FEAT_MAX);
> +    }
> +

This would likely be the only interface where we expose "more" features 
than a CPU model actually understands? I guess it wouldn't be that bad 
because disabling unsupported features will just work, even if the CPU 
model is not aware of them.

So no strong opinion.

Just noting that libvirt cannot really rely on that information because 
the behavior would change between QEMU versions? Maybe not an issue.

Acked-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb

Re: [PATCH v1] target/s390x: filter deprecated features based on model expansion type

Posted by Collin Walling 4 months, 1 week ago

On 7/11/24 5:10 PM, David Hildenbrand wrote:
> On 11.07.24 22:32, Collin Walling wrote:
>> It is beneficial to provide an interface to retrieve *all* deprecated
>> features in one go. Management applications will need this information
>> to determine which features need to be disabled regardless of the
>> host-model's capabilities.
>>
>> To remedy this, deprecated features are only filtered during a static
>> expansion. All deperecated features are reported on a full expansion.
>>
>> Suggested-by: Jiri Denemark <jdenemar@redhat.com>
>> Signed-off-by: Collin Walling <walling@linux.ibm.com>
>> ---
>>   target/s390x/cpu_models_sysemu.c | 10 +++++++++-
>>   1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/target/s390x/cpu_models_sysemu.c b/target/s390x/cpu_models_sysemu.c
>> index 977fbc6522..76d15f2e4d 100644
>> --- a/target/s390x/cpu_models_sysemu.c
>> +++ b/target/s390x/cpu_models_sysemu.c
>> @@ -211,7 +211,15 @@ static void cpu_info_from_model(CpuModelInfo *info, const S390CPUModel *model,
>>       bitmap_zero(bitmap, S390_FEAT_MAX);
>>       s390_get_deprecated_features(bitmap);
>>   
>> -    bitmap_and(bitmap, bitmap, model->def->full_feat, S390_FEAT_MAX);
>> +    /*
>> +     * For static model expansion, filter out deprecated features that are
>> +     * not a subset of the model's feature set. Otherwise, report the entire
>> +     * deprecated features list.
>> +     */
>> +    if (delta_changes) {
>> +        bitmap_and(bitmap, bitmap, model->def->full_feat, S390_FEAT_MAX);
>> +    }
>> +
> 
> This would likely be the only interface where we expose "more" features 
> than a CPU model actually understands? I guess it wouldn't be that bad 
> because disabling unsupported features will just work, even if the CPU 
> model is not aware of them.
> 

Yes, and for a full expansion an exhaustive list of features are
reported, even the ones that the model does not support (they're paired
with "false").  So I think it makes sense to report *all* features that
are deprecated as well.

> So no strong opinion.
> 
> Just noting that libvirt cannot really rely on that information because 
> the behavior would change between QEMU versions? Maybe not an issue.
> 

Right.  If it's appropriate, I could handle back porting of this patch
if requested.  But that's not for me to decide :)

> Acked-by: David Hildenbrand <david@redhat.com>
> 

Thanks!

-- 
Regards,
  Collin