[PATCH v2 0/3] qapi/machine: Make @dump-skeys command generic

[PATCH v2 0/3] 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 generic SysemuCPUOps::qmp_dump_skeys() callback
(only implemented on s390x). No HMP change.

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

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

Philippe Mathieu-Daudé (3):
  hw/s390x: Expose s390_qmp_dump_skeys() prototype
  cpus: Introduce SysemuCPUOps::qmp_dump_skeys() callback
  qapi/machine: Make @dump-skeys command generic

 qapi/machine.json                | 18 ++++++++++++++++++
 qapi/misc-target.json            | 19 -------------------
 include/hw/core/sysemu-cpu-ops.h |  6 ++++++
 include/hw/s390x/storage-keys.h  |  1 +
 hw/core/machine-qmp-cmds.c       | 13 +++++++++++++
 hw/s390x/s390-skeys.c            |  3 ++-
 target/s390x/cpu-system.c        |  2 ++
 7 files changed, 42 insertions(+), 20 deletions(-)

-- 
2.47.1

[PATCH v2 1/3] hw/s390x: Expose s390_qmp_dump_skeys() prototype

[Philippe Mathieu-Daudé]

In preparation to make @dump-skeys command generic,
extract s390_qmp_dump_skeys() out of qmp_dump_skeys().

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

diff --git a/include/hw/s390x/storage-keys.h b/include/hw/s390x/storage-keys.h
index 408d2815d4d..d4c7daae6c1 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 811d892122b..686c118ebcd 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);
-- 
2.47.1

Re: [PATCH v2 1/3] hw/s390x: Expose s390_qmp_dump_skeys() prototype

[Thomas Huth]

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

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

[PATCH v2 2/3] cpus: Introduce SysemuCPUOps::qmp_dump_skeys() callback

[Philippe Mathieu-Daudé]

Allow generic CPUs to dump the architecture storage keys.

Being specific to s390x, it is only implemented there.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/core/sysemu-cpu-ops.h | 6 ++++++
 target/s390x/cpu-system.c        | 2 ++
 2 files changed, 8 insertions(+)

diff --git a/include/hw/core/sysemu-cpu-ops.h b/include/hw/core/sysemu-cpu-ops.h
index 877892373f9..d3534cba65c 100644
--- a/include/hw/core/sysemu-cpu-ops.h
+++ b/include/hw/core/sysemu-cpu-ops.h
@@ -47,6 +47,12 @@ typedef struct SysemuCPUOps {
      *       a memory access with the specified memory transaction attributes.
      */
     int (*asidx_from_attrs)(CPUState *cpu, MemTxAttrs attrs);
+
+    /**
+     * @qmp_dump_skeys: Callback to dump guest's storage keys to @filename.
+     */
+    void (*qmp_dump_skeys)(const char *filename, Error **errp);
+
     /**
      * @get_crash_info: Callback for reporting guest crash information in
      * GUEST_PANICKED events.
diff --git a/target/s390x/cpu-system.c b/target/s390x/cpu-system.c
index 9b380e343c2..ab7bb8d5cf5 100644
--- a/target/s390x/cpu-system.c
+++ b/target/s390x/cpu-system.c
@@ -38,6 +38,7 @@
 #include "system/system.h"
 #include "system/tcg.h"
 #include "hw/core/sysemu-cpu-ops.h"
+#include "hw/s390x/storage-keys.h"
 
 bool s390_cpu_has_work(CPUState *cs)
 {
@@ -179,6 +180,7 @@ static const struct SysemuCPUOps s390_sysemu_ops = {
     .get_phys_page_debug = s390_cpu_get_phys_page_debug,
     .get_crash_info = s390_cpu_get_crash_info,
     .write_elf64_note = s390_cpu_write_elf64_note,
+    .qmp_dump_skeys = s390_qmp_dump_skeys,
     .legacy_vmsd = &vmstate_s390_cpu,
 };
 
-- 
2.47.1

Re: [PATCH v2 2/3] cpus: Introduce SysemuCPUOps::qmp_dump_skeys() callback

[Thomas Huth]

On 10/03/2025 14.31, Philippe Mathieu-Daudé wrote:
> Allow generic CPUs to dump the architecture storage keys.
> 
> Being specific to s390x, it is only implemented there.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   include/hw/core/sysemu-cpu-ops.h | 6 ++++++
>   target/s390x/cpu-system.c        | 2 ++
>   2 files changed, 8 insertions(+)

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

Re: [PATCH v2 2/3] cpus: Introduce SysemuCPUOps::qmp_dump_skeys() callback

[Daniel P. Berrangé]

On Mon, Mar 10, 2025 at 02:31:17PM +0100, Philippe Mathieu-Daudé wrote:
> Allow generic CPUs to dump the architecture storage keys.
> 
> Being specific to s390x, it is only implemented there.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  include/hw/core/sysemu-cpu-ops.h | 6 ++++++
>  target/s390x/cpu-system.c        | 2 ++
>  2 files changed, 8 insertions(+)
> 
> diff --git a/include/hw/core/sysemu-cpu-ops.h b/include/hw/core/sysemu-cpu-ops.h
> index 877892373f9..d3534cba65c 100644
> --- a/include/hw/core/sysemu-cpu-ops.h
> +++ b/include/hw/core/sysemu-cpu-ops.h
> @@ -47,6 +47,12 @@ typedef struct SysemuCPUOps {
>       *       a memory access with the specified memory transaction attributes.
>       */
>      int (*asidx_from_attrs)(CPUState *cpu, MemTxAttrs attrs);
> +
> +    /**
> +     * @qmp_dump_skeys: Callback to dump guest's storage keys to @filename.
> +     */
> +    void (*qmp_dump_skeys)(const char *filename, Error **errp);

Is it right to hook this onto the CPU object ? In the next patch
the code arbitrarily picks the 1st CPU and adds a "FIXME" annotation,
but the actual impl of dump code doesn't seem to be tied to any CPU
object at all, it is getting what looks like a global singleton
object holding the keys.

IOW, should this hook be against the machine type instead, if it
is dumping global state, not tied to a specific CPU ?

> +
>      /**
>       * @get_crash_info: Callback for reporting guest crash information in
>       * GUEST_PANICKED events.
> diff --git a/target/s390x/cpu-system.c b/target/s390x/cpu-system.c
> index 9b380e343c2..ab7bb8d5cf5 100644
> --- a/target/s390x/cpu-system.c
> +++ b/target/s390x/cpu-system.c
> @@ -38,6 +38,7 @@
>  #include "system/system.h"
>  #include "system/tcg.h"
>  #include "hw/core/sysemu-cpu-ops.h"
> +#include "hw/s390x/storage-keys.h"
>  
>  bool s390_cpu_has_work(CPUState *cs)
>  {
> @@ -179,6 +180,7 @@ static const struct SysemuCPUOps s390_sysemu_ops = {
>      .get_phys_page_debug = s390_cpu_get_phys_page_debug,
>      .get_crash_info = s390_cpu_get_crash_info,
>      .write_elf64_note = s390_cpu_write_elf64_note,
> +    .qmp_dump_skeys = s390_qmp_dump_skeys,
>      .legacy_vmsd = &vmstate_s390_cpu,
>  };
>  
> -- 
> 2.47.1
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v2 2/3] cpus: Introduce SysemuCPUOps::qmp_dump_skeys() callback

[Thomas Huth]

On 10/03/2025 14.45, Daniel P. Berrangé wrote:
> On Mon, Mar 10, 2025 at 02:31:17PM +0100, Philippe Mathieu-Daudé wrote:
>> Allow generic CPUs to dump the architecture storage keys.
>>
>> Being specific to s390x, it is only implemented there.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   include/hw/core/sysemu-cpu-ops.h | 6 ++++++
>>   target/s390x/cpu-system.c        | 2 ++
>>   2 files changed, 8 insertions(+)
>>
>> diff --git a/include/hw/core/sysemu-cpu-ops.h b/include/hw/core/sysemu-cpu-ops.h
>> index 877892373f9..d3534cba65c 100644
>> --- a/include/hw/core/sysemu-cpu-ops.h
>> +++ b/include/hw/core/sysemu-cpu-ops.h
>> @@ -47,6 +47,12 @@ typedef struct SysemuCPUOps {
>>        *       a memory access with the specified memory transaction attributes.
>>        */
>>       int (*asidx_from_attrs)(CPUState *cpu, MemTxAttrs attrs);
>> +
>> +    /**
>> +     * @qmp_dump_skeys: Callback to dump guest's storage keys to @filename.
>> +     */
>> +    void (*qmp_dump_skeys)(const char *filename, Error **errp);
> 
> Is it right to hook this onto the CPU object ? In the next patch
> the code arbitrarily picks the 1st CPU and adds a "FIXME" annotation,
> but the actual impl of dump code doesn't seem to be tied to any CPU
> object at all, it is getting what looks like a global singleton
> object holding the keys.
> 
> IOW, should this hook be against the machine type instead, if it
> is dumping global state, not tied to a specific CPU ?

Hmm, you've got a point - the storage keys are part of the memory, not of 
the CPU, so they might rather belong to the machine instead, indeed.

  Thomas

Re: [PATCH v2 2/3] cpus: Introduce SysemuCPUOps::qmp_dump_skeys() callback

[Philippe Mathieu-Daudé]

On 10/3/25 14:48, Thomas Huth wrote:
> On 10/03/2025 14.45, Daniel P. Berrangé wrote:
>> On Mon, Mar 10, 2025 at 02:31:17PM +0100, Philippe Mathieu-Daudé wrote:
>>> Allow generic CPUs to dump the architecture storage keys.
>>>
>>> Being specific to s390x, it is only implemented there.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>>   include/hw/core/sysemu-cpu-ops.h | 6 ++++++
>>>   target/s390x/cpu-system.c        | 2 ++
>>>   2 files changed, 8 insertions(+)
>>>
>>> diff --git a/include/hw/core/sysemu-cpu-ops.h b/include/hw/core/ 
>>> sysemu-cpu-ops.h
>>> index 877892373f9..d3534cba65c 100644
>>> --- a/include/hw/core/sysemu-cpu-ops.h
>>> +++ b/include/hw/core/sysemu-cpu-ops.h
>>> @@ -47,6 +47,12 @@ typedef struct SysemuCPUOps {
>>>        *       a memory access with the specified memory transaction 
>>> attributes.
>>>        */
>>>       int (*asidx_from_attrs)(CPUState *cpu, MemTxAttrs attrs);
>>> +
>>> +    /**
>>> +     * @qmp_dump_skeys: Callback to dump guest's storage keys to 
>>> @filename.
>>> +     */
>>> +    void (*qmp_dump_skeys)(const char *filename, Error **errp);
>>
>> Is it right to hook this onto the CPU object ? In the next patch
>> the code arbitrarily picks the 1st CPU and adds a "FIXME" annotation,
>> but the actual impl of dump code doesn't seem to be tied to any CPU
>> object at all, it is getting what looks like a global singleton
>> object holding the keys.
>>
>> IOW, should this hook be against the machine type instead, if it
>> is dumping global state, not tied to a specific CPU ?

Great analysis!

> Hmm, you've got a point - the storage keys are part of the memory, not 
> of the CPU, so they might rather belong to the machine instead, indeed.
> 
>   Thomas
> 

[PATCH v2 3/3] qapi/machine: Make @dump-skeys command generic

[Philippe Mathieu-Daudé]

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

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

diff --git a/qapi/machine.json b/qapi/machine.json
index a6b8795b09e..53680bf0998 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 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' } }
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..b9ed0963bb4 100644
--- a/hw/core/machine-qmp-cmds.c
+++ b/hw/core/machine-qmp-cmds.c
@@ -10,6 +10,7 @@
 #include "qemu/osdep.h"
 #include "hw/acpi/vmgenid.h"
 #include "hw/boards.h"
+#include "hw/core/sysemu-cpu-ops.h"
 #include "hw/intc/intc.h"
 #include "hw/mem/memory-device.h"
 #include "qapi/error.h"
@@ -406,3 +407,15 @@ 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)
+{
+    CPUState *cpu = first_cpu; /* FIXME */
+
+    if (!cpu->cc->sysemu_ops->qmp_dump_skeys) {
+        error_setg(errp, "Storage keys information not available"
+                         " for this architecture");
+        return;
+    }
+    cpu->cc->sysemu_ops->qmp_dump_skeys(filename, errp);
+}
diff --git a/hw/s390x/s390-skeys.c b/hw/s390x/s390-skeys.c
index 686c118ebcd..6bd608b5aa3 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 v2 3/3] qapi/machine: Make @dump-skeys command generic

[Thomas Huth]

On 10/03/2025 14.31, Philippe Mathieu-Daudé wrote:
> Reduce misc-target.json by one target specific command.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   qapi/machine.json          | 18 ++++++++++++++++++
>   qapi/misc-target.json      | 19 -------------------
>   hw/core/machine-qmp-cmds.c | 13 +++++++++++++
>   hw/s390x/s390-skeys.c      |  6 +-----
>   4 files changed, 32 insertions(+), 24 deletions(-)

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

Re: [PATCH v2 3/3] qapi/machine: Make @dump-skeys command generic

[Daniel P. Berrangé]

On Mon, Mar 10, 2025 at 02:31:18PM +0100, Philippe Mathieu-Daudé wrote:
> Reduce misc-target.json by one target specific command.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  qapi/machine.json          | 18 ++++++++++++++++++
>  qapi/misc-target.json      | 19 -------------------
>  hw/core/machine-qmp-cmds.c | 13 +++++++++++++
>  hw/s390x/s390-skeys.c      |  6 +-----
>  4 files changed, 32 insertions(+), 24 deletions(-)
> 
> diff --git a/qapi/machine.json b/qapi/machine.json
> index a6b8795b09e..53680bf0998 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 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": {} }
> +##

Currently the 'if: TARGET_S390X' ends up in the documentation
so users can see it is s390x-onl:

 https://www.qemu.org/docs/master/interop/qemu-qmp-ref.html#qapidoc-2890

Given there's no more conditional, the docs will loose this target
info, which could be considered a regression. We could tweak the
docs text to call this out:

 ##
 # @dump-skeys:
 #
 # Dump the storage keys for an s390x guest
 #




With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|