[PATCH v3 0/4] qapi/machine: Make @dump-skeys command generic

[PATCH v3 0/4] qapi/machine: Make @dump-skeys command generic

[Philippe Mathieu-Daudé]

We are trying to unify all qemu-system-FOO to a single binary.
In order to do that we need to remove QAPI target specific code.

Introduce the TYPE_DUMP_SKEYS_INTERFACE type which provide the
qmp_dump_skeys() callback, have it implemented on the CCW machine.
Machines not supporting this interface report a QMP GenericError.
No HMP change.

Since v2:
- Use machine interface (Daniel)

Since v1 [*]:
- No QMP rename / deprecation

[*] https://lore.kernel.org/qemu-devel/20240530074544.25444-1-philmd@linaro.org/

Philippe Mathieu-Daudé (4):
  hw/s390x/skeys: Declare QOM types using DEFINE_TYPES() macro
  hw/s390x/skeys: Introduce TYPE_DUMP_SKEYS_INTERFACE
  hw/s390x/ccw: Have CCW machine implement a qmp_dump_skeys() callback
  qapi/machine: Make @dump-skeys command generic

 qapi/machine.json               | 18 +++++++++++++
 qapi/misc-target.json           | 19 -------------
 include/hw/s390x/storage-keys.h | 16 +++++++++++
 hw/core/machine-qmp-cmds.c      | 14 ++++++++++
 hw/s390x/s390-skeys.c           | 47 +++++++++++++++++----------------
 hw/s390x/s390-virtio-ccw.c      |  3 +++
 6 files changed, 75 insertions(+), 42 deletions(-)

-- 
2.47.1

[PATCH v3 1/4] hw/s390x/skeys: Declare QOM types using DEFINE_TYPES() macro

[Philippe Mathieu-Daudé]

When multiple QOM types are registered in the same file,
it is simpler to use the the DEFINE_TYPES() macro. In
particular because type array declared with such macro
are easier to review.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/s390x/s390-skeys.c | 39 +++++++++++++++++----------------------
 1 file changed, 17 insertions(+), 22 deletions(-)

diff --git a/hw/s390x/s390-skeys.c b/hw/s390x/s390-skeys.c
index 811d892122b..d50e71b927a 100644
--- a/hw/s390x/s390-skeys.c
+++ b/hw/s390x/s390-skeys.c
@@ -316,14 +316,6 @@ static void qemu_s390_skeys_class_init(ObjectClass *oc, void *data)
     dc->user_creatable = false;
 }
 
-static const TypeInfo qemu_s390_skeys_info = {
-    .name          = TYPE_QEMU_S390_SKEYS,
-    .parent        = TYPE_S390_SKEYS,
-    .instance_size = sizeof(QEMUS390SKeysState),
-    .class_init    = qemu_s390_skeys_class_init,
-    .class_size    = sizeof(S390SKeysClass),
-};
-
 static void s390_storage_keys_save(QEMUFile *f, void *opaque)
 {
     S390SKeysState *ss = S390_SKEYS(opaque);
@@ -481,19 +473,22 @@ static void s390_skeys_class_init(ObjectClass *oc, void *data)
     set_bit(DEVICE_CATEGORY_MISC, dc->categories);
 }
 
-static const TypeInfo s390_skeys_info = {
-    .name          = TYPE_S390_SKEYS,
-    .parent        = TYPE_DEVICE,
-    .instance_size = sizeof(S390SKeysState),
-    .class_init    = s390_skeys_class_init,
-    .class_size    = sizeof(S390SKeysClass),
-    .abstract = true,
+static const TypeInfo s390_skeys_types[] = {
+    {
+        .name           = TYPE_S390_SKEYS,
+        .parent         = TYPE_DEVICE,
+        .instance_size  = sizeof(S390SKeysState),
+        .class_init     = s390_skeys_class_init,
+        .class_size     = sizeof(S390SKeysClass),
+        .abstract       = true,
+    },
+    {
+        .name           = TYPE_QEMU_S390_SKEYS,
+        .parent         = TYPE_S390_SKEYS,
+        .instance_size  = sizeof(QEMUS390SKeysState),
+        .class_init     = qemu_s390_skeys_class_init,
+        .class_size     = sizeof(S390SKeysClass),
+    },
 };
 
-static void qemu_s390_skeys_register_types(void)
-{
-    type_register_static(&s390_skeys_info);
-    type_register_static(&qemu_s390_skeys_info);
-}
-
-type_init(qemu_s390_skeys_register_types)
+DEFINE_TYPES(s390_skeys_types)
-- 
2.47.1

[PATCH v3 2/4] hw/s390x/skeys: Introduce TYPE_DUMP_SKEYS_INTERFACE

[Philippe Mathieu-Daudé]

The storage keys are part of the machine memory.

Introduce the TYPE_DUMP_SKEYS_INTERFACE type,
allowing machine using storage keys to dump them
when a DumpSKeysInterface::qmp_dump_skeys() callback
is provided.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/s390x/storage-keys.h | 15 +++++++++++++++
 hw/s390x/s390-skeys.c           |  5 +++++
 2 files changed, 20 insertions(+)

diff --git a/include/hw/s390x/storage-keys.h b/include/hw/s390x/storage-keys.h
index 408d2815d4d..fb766d4631b 100644
--- a/include/hw/s390x/storage-keys.h
+++ b/include/hw/s390x/storage-keys.h
@@ -125,4 +125,19 @@ S390SKeysState *s390_get_skeys_device(void);
 void hmp_dump_skeys(Monitor *mon, const QDict *qdict);
 void hmp_info_skeys(Monitor *mon, const QDict *qdict);
 
+#define TYPE_DUMP_SKEYS_INTERFACE "dump-skeys-interface"
+
+typedef struct DumpSKeysInterface DumpSKeysInterface;
+DECLARE_CLASS_CHECKERS(DumpSKeysInterface, DUMP_SKEYS_INTERFACE,
+                       TYPE_DUMP_SKEYS_INTERFACE)
+
+struct DumpSKeysInterface {
+    InterfaceClass parent_class;
+
+    /**
+     * @qmp_dump_skeys: Callback to dump guest's storage keys to @filename.
+     */
+    void (*qmp_dump_skeys)(const char *filename, Error **errp);
+};
+
 #endif /* S390_STORAGE_KEYS_H */
diff --git a/hw/s390x/s390-skeys.c b/hw/s390x/s390-skeys.c
index d50e71b927a..0d3d4f74b4c 100644
--- a/hw/s390x/s390-skeys.c
+++ b/hw/s390x/s390-skeys.c
@@ -474,6 +474,11 @@ static void s390_skeys_class_init(ObjectClass *oc, void *data)
 }
 
 static const TypeInfo s390_skeys_types[] = {
+    {
+        .name           = TYPE_DUMP_SKEYS_INTERFACE,
+        .parent         = TYPE_INTERFACE,
+        .class_size     = sizeof(DumpSKeysInterface),
+    },
     {
         .name           = TYPE_S390_SKEYS,
         .parent         = TYPE_DEVICE,
-- 
2.47.1

[PATCH v3 3/4] hw/s390x/ccw: Have CCW machine implement a qmp_dump_skeys() callback

[Philippe Mathieu-Daudé]

In preparation to make @dump-skeys command generic,
extract s390_qmp_dump_skeys() out of qmp_dump_skeys().
Register it as CCW qmp_dump_skeys() callback.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/s390x/storage-keys.h | 1 +
 hw/s390x/s390-skeys.c           | 7 ++++++-
 hw/s390x/s390-virtio-ccw.c      | 3 +++
 3 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/include/hw/s390x/storage-keys.h b/include/hw/s390x/storage-keys.h
index fb766d4631b..ac303001f57 100644
--- a/include/hw/s390x/storage-keys.h
+++ b/include/hw/s390x/storage-keys.h
@@ -122,6 +122,7 @@ int s390_skeys_set(S390SKeysState *ks, uint64_t start_gfn,
 
 S390SKeysState *s390_get_skeys_device(void);
 
+void s390_qmp_dump_skeys(const char *filename, Error **errp);
 void hmp_dump_skeys(Monitor *mon, const QDict *qdict);
 void hmp_info_skeys(Monitor *mon, const QDict *qdict);
 
diff --git a/hw/s390x/s390-skeys.c b/hw/s390x/s390-skeys.c
index 0d3d4f74b4c..fd1123b0f35 100644
--- a/hw/s390x/s390-skeys.c
+++ b/hw/s390x/s390-skeys.c
@@ -142,7 +142,7 @@ void hmp_dump_skeys(Monitor *mon, const QDict *qdict)
     }
 }
 
-void qmp_dump_skeys(const char *filename, Error **errp)
+void s390_qmp_dump_skeys(const char *filename, Error **errp)
 {
     S390SKeysState *ss = s390_get_skeys_device();
     S390SKeysClass *skeyclass = S390_SKEYS_GET_CLASS(ss);
@@ -219,6 +219,11 @@ out:
     fclose(f);
 }
 
+void qmp_dump_skeys(const char *filename, Error **errp)
+{
+    s390_qmp_dump_skeys(filename, errp);
+}
+
 static bool qemu_s390_skeys_are_enabled(S390SKeysState *ss)
 {
     QEMUS390SKeysState *skeys = QEMU_S390_SKEYS(ss);
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index a9b3db19f63..104cd1c79eb 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -809,6 +809,7 @@ static void ccw_machine_class_init(ObjectClass *oc, void *data)
     NMIClass *nc = NMI_CLASS(oc);
     HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc);
     S390CcwMachineClass *s390mc = S390_CCW_MACHINE_CLASS(mc);
+    DumpSKeysInterface *dsi = DUMP_SKEYS_INTERFACE_CLASS(oc);
 
     s390mc->hpage_1m_allowed = true;
     s390mc->max_threads = 1;
@@ -834,6 +835,7 @@ static void ccw_machine_class_init(ObjectClass *oc, void *data)
     nc->nmi_monitor_handler = s390_nmi;
     mc->default_ram_id = "s390.ram";
     mc->default_nic = "virtio-net-ccw";
+    dsi->qmp_dump_skeys = s390_qmp_dump_skeys;
 
     object_class_property_add_bool(oc, "aes-key-wrap",
                                    machine_get_aes_key_wrap,
@@ -875,6 +877,7 @@ static const TypeInfo ccw_machine_info = {
     .interfaces = (InterfaceInfo[]) {
         { TYPE_NMI },
         { TYPE_HOTPLUG_HANDLER},
+        { TYPE_DUMP_SKEYS_INTERFACE},
         { }
     },
 };
-- 
2.47.1

[PATCH v3 4/4] qapi/machine: Make @dump-skeys command generic

[Philippe Mathieu-Daudé]

Reduce misc-target.json by one target specific command.

Error message is returned for machines not implementing
TYPE_DUMP_SKEYS_INTERFACE:

  $ qemu-system-aarch64 -M virt -S -qmp stdio
  {"QMP": {"version": {"qemu": {"micro": 50, "major": 9}}, "capabilities": ["oob"]}}
  { "execute": "qmp_capabilities" }
  {"return": {}}
  { "execute": "dump-skeys", "arguments": { "filename": "/tmp/foo" }  }
  {"error": {"class": "GenericError", "desc": "Storage keys information not available for this architecture"}}

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 qapi/machine.json          | 18 ++++++++++++++++++
 qapi/misc-target.json      | 19 -------------------
 hw/core/machine-qmp-cmds.c | 14 ++++++++++++++
 hw/s390x/s390-skeys.c      |  6 +-----
 4 files changed, 33 insertions(+), 24 deletions(-)

diff --git a/qapi/machine.json b/qapi/machine.json
index a6b8795b09e..a9ff8076317 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -1898,3 +1898,21 @@
 { 'command': 'x-query-interrupt-controllers',
   'returns': 'HumanReadableText',
   'features': [ 'unstable' ]}
+
+##
+# @dump-skeys:
+#
+# Dump the storage keys for an s390x guest
+#
+# @filename: the path to the file to dump to
+#
+# Since: 2.5
+#
+# .. qmp-example::
+#
+#     -> { "execute": "dump-skeys",
+#          "arguments": { "filename": "/tmp/skeys" } }
+#     <- { "return": {} }
+##
+{ 'command': 'dump-skeys',
+  'data': { 'filename': 'str' } }
diff --git a/qapi/misc-target.json b/qapi/misc-target.json
index 8d70bd24d8c..42e4a7417dc 100644
--- a/qapi/misc-target.json
+++ b/qapi/misc-target.json
@@ -274,25 +274,6 @@
   'returns': 'SevAttestationReport',
   'if': 'TARGET_I386' }
 
-##
-# @dump-skeys:
-#
-# Dump guest's storage keys
-#
-# @filename: the path to the file to dump to
-#
-# Since: 2.5
-#
-# .. qmp-example::
-#
-#     -> { "execute": "dump-skeys",
-#          "arguments": { "filename": "/tmp/skeys" } }
-#     <- { "return": {} }
-##
-{ 'command': 'dump-skeys',
-  'data': { 'filename': 'str' },
-  'if': 'TARGET_S390X' }
-
 ##
 # @GICCapability:
 #
diff --git a/hw/core/machine-qmp-cmds.c b/hw/core/machine-qmp-cmds.c
index 3130c5cd456..fd8b4e0b44c 100644
--- a/hw/core/machine-qmp-cmds.c
+++ b/hw/core/machine-qmp-cmds.c
@@ -25,6 +25,7 @@
 #include "system/numa.h"
 #include "system/runstate.h"
 #include "system/system.h"
+#include "hw/s390x/storage-keys.h"
 
 /*
  * fast means: we NEVER interrupt vCPU threads to retrieve
@@ -406,3 +407,16 @@ GuidInfo *qmp_query_vm_generation_id(Error **errp)
     info->guid = qemu_uuid_unparse_strdup(&vms->guid);
     return info;
 }
+
+void qmp_dump_skeys(const char *filename, Error **errp)
+{
+    ObjectClass *mc = object_get_class(qdev_get_machine());
+    ObjectClass *oc = object_class_dynamic_cast(mc, TYPE_DUMP_SKEYS_INTERFACE);
+
+    if (!oc) {
+        error_setg(errp, "Storage keys information not available"
+                         " for this architecture");
+        return;
+    }
+    DUMP_SKEYS_INTERFACE_CLASS(oc)->qmp_dump_skeys(filename, errp);
+}
diff --git a/hw/s390x/s390-skeys.c b/hw/s390x/s390-skeys.c
index fd1123b0f35..067ea037268 100644
--- a/hw/s390x/s390-skeys.c
+++ b/hw/s390x/s390-skeys.c
@@ -15,6 +15,7 @@
 #include "hw/qdev-properties.h"
 #include "hw/s390x/storage-keys.h"
 #include "qapi/error.h"
+#include "qapi/qapi-commands-machine.h"
 #include "qapi/qapi-commands-misc-target.h"
 #include "qobject/qdict.h"
 #include "qemu/error-report.h"
@@ -219,11 +220,6 @@ out:
     fclose(f);
 }
 
-void qmp_dump_skeys(const char *filename, Error **errp)
-{
-    s390_qmp_dump_skeys(filename, errp);
-}
-
 static bool qemu_s390_skeys_are_enabled(S390SKeysState *ss)
 {
     QEMUS390SKeysState *skeys = QEMU_S390_SKEYS(ss);
-- 
2.47.1

Re: [PATCH v3 1/4] hw/s390x/skeys: Declare QOM types using DEFINE_TYPES() macro

[Thomas Huth]

On 10/03/2025 16.14, Philippe Mathieu-Daudé wrote:
> When multiple QOM types are registered in the same file,
> it is simpler to use the the DEFINE_TYPES() macro. In
> particular because type array declared with such macro
> are easier to review.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/s390x/s390-skeys.c | 39 +++++++++++++++++----------------------
>   1 file changed, 17 insertions(+), 22 deletions(-)

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH v3 2/4] hw/s390x/skeys: Introduce TYPE_DUMP_SKEYS_INTERFACE

[Thomas Huth]

On 10/03/2025 16.14, Philippe Mathieu-Daudé wrote:
> The storage keys are part of the machine memory.
> 
> Introduce the TYPE_DUMP_SKEYS_INTERFACE type,
> allowing machine using storage keys to dump them
> when a DumpSKeysInterface::qmp_dump_skeys() callback
> is provided.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   include/hw/s390x/storage-keys.h | 15 +++++++++++++++
>   hw/s390x/s390-skeys.c           |  5 +++++
>   2 files changed, 20 insertions(+)

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH v3 3/4] hw/s390x/ccw: Have CCW machine implement a qmp_dump_skeys() callback

[Thomas Huth]

On 10/03/2025 16.14, Philippe Mathieu-Daudé wrote:
> In preparation to make @dump-skeys command generic,
> extract s390_qmp_dump_skeys() out of qmp_dump_skeys().
> Register it as CCW qmp_dump_skeys() callback.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   include/hw/s390x/storage-keys.h | 1 +
>   hw/s390x/s390-skeys.c           | 7 ++++++-
>   hw/s390x/s390-virtio-ccw.c      | 3 +++
>   3 files changed, 10 insertions(+), 1 deletion(-)

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH v3 4/4] qapi/machine: Make @dump-skeys command generic

[Thomas Huth]

On 10/03/2025 16.14, Philippe Mathieu-Daudé wrote:
> Reduce misc-target.json by one target specific command.
> 
> Error message is returned for machines not implementing
> TYPE_DUMP_SKEYS_INTERFACE:
> 
>    $ qemu-system-aarch64 -M virt -S -qmp stdio
>    {"QMP": {"version": {"qemu": {"micro": 50, "major": 9}}, "capabilities": ["oob"]}}
>    { "execute": "qmp_capabilities" }
>    {"return": {}}
>    { "execute": "dump-skeys", "arguments": { "filename": "/tmp/foo" }  }
>    {"error": {"class": "GenericError", "desc": "Storage keys information not available for this architecture"}}
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH v3 1/4] hw/s390x/skeys: Declare QOM types using DEFINE_TYPES() macro

[Eric Farman]

On Mon, 2025-03-10 at 16:14 +0100, Philippe Mathieu-Daudé wrote:
> When multiple QOM types are registered in the same file,
> it is simpler to use the the DEFINE_TYPES() macro. In

s/the the/the/

> particular because type array declared with such macro
> are easier to review.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/s390x/s390-skeys.c | 39 +++++++++++++++++----------------------
>  1 file changed, 17 insertions(+), 22 deletions(-)

Reviewed-by: Eric Farman <farman@linux.ibm.com>

Re: [PATCH v3 2/4] hw/s390x/skeys: Introduce TYPE_DUMP_SKEYS_INTERFACE

[Eric Farman]

On Mon, 2025-03-10 at 16:14 +0100, Philippe Mathieu-Daudé wrote:
> The storage keys are part of the machine memory.
> 
> Introduce the TYPE_DUMP_SKEYS_INTERFACE type,
> allowing machine using storage keys to dump them
> when a DumpSKeysInterface::qmp_dump_skeys() callback
> is provided.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  include/hw/s390x/storage-keys.h | 15 +++++++++++++++
>  hw/s390x/s390-skeys.c           |  5 +++++
>  2 files changed, 20 insertions(+)

Reviewed-by: Eric Farman <farman@linux.ibm.com>

Re: [PATCH v3 3/4] hw/s390x/ccw: Have CCW machine implement a qmp_dump_skeys() callback

[Eric Farman]

On Mon, 2025-03-10 at 16:14 +0100, Philippe Mathieu-Daudé wrote:
> In preparation to make @dump-skeys command generic,
> extract s390_qmp_dump_skeys() out of qmp_dump_skeys().
> Register it as CCW qmp_dump_skeys() callback.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  include/hw/s390x/storage-keys.h | 1 +
>  hw/s390x/s390-skeys.c           | 7 ++++++-
>  hw/s390x/s390-virtio-ccw.c      | 3 +++
>  3 files changed, 10 insertions(+), 1 deletion(-)

Reviewed-by: Eric Farman <farman@linux.ibm.com>