qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: David Hildenbrand
Subject: Re: [PATCH v1] target/s390x: move @deprecated-props to CpuModelExpansion Info
Date: Mon, 29 Jul 2024 11:52:19 +0200
User-agent: Mozilla Thunderbird

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




reply via email to

[Prev in Thread] Current Thread [Next in Thread]