[PATCH 0/4] hw/s390x: Alias @dump-skeys -> @dump-s390-skey and deprecate

[PATCH 0/4] hw/s390x: Alias @dump-skeys -> @dump-s390-skey and deprecate

[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.

@dump-skeys is only available on qemu-system-s390x. This series
rename it as @dump-s390-skey, making it available on other
binaries. We take care of backward compatibility via deprecation.

Philippe Mathieu-Daudé (4):
  hw/s390x: Introduce the @dump-s390-skeys QMP command
  hw/s390x: Introduce the 'dump_s390_skeys' HMP command
  hw/s390x: Deprecate the HMP 'dump_skeys' command
  hw/s390x: Deprecate the QMP @dump-skeys command

 docs/about/deprecated.rst  |  5 +++++
 qapi/misc-target.json      |  5 +++++
 qapi/misc.json             | 18 ++++++++++++++++++
 include/monitor/hmp.h      |  1 +
 hw/s390x/s390-skeys-stub.c | 24 ++++++++++++++++++++++++
 hw/s390x/s390-skeys.c      | 19 +++++++++++++++++--
 hmp-commands.hx            | 17 +++++++++++++++--
 hw/s390x/meson.build       |  5 +++++
 8 files changed, 90 insertions(+), 4 deletions(-)
 create mode 100644 hw/s390x/s390-skeys-stub.c

-- 
2.41.0

[PATCH 1/4] hw/s390x: Introduce the @dump-s390-skeys QMP command

[Philippe Mathieu-Daudé]

@dump-skeys is specific to the qemu-system-s390x binary.
In order to provide it in an unified single binary, add
a equivalent new command named @dump-s390-skeys, which
works identically on s390x and reports an error on other
targets.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 qapi/misc.json             | 18 ++++++++++++++++++
 hw/s390x/s390-skeys-stub.c | 16 ++++++++++++++++
 hw/s390x/s390-skeys.c      |  8 +++++++-
 hw/s390x/meson.build       |  5 +++++
 4 files changed, 46 insertions(+), 1 deletion(-)
 create mode 100644 hw/s390x/s390-skeys-stub.c

diff --git a/qapi/misc.json b/qapi/misc.json
index ec30e5c570..d192dd1bef 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -605,3 +605,21 @@
 { 'event': 'VFU_CLIENT_HANGUP',
   'data': { 'vfu-id': 'str', 'vfu-qom-path': 'str',
             'dev-id': 'str', 'dev-qom-path': 'str' } }
+
+##
+# @dump-s390-skeys:
+#
+# Dump guest's storage keys
+#
+# @filename: the path to the file to dump to
+#
+# Since: 9.1
+#
+# Example:
+#
+#     -> { "execute": "dump-skeys",
+#          "arguments": { "filename": "/tmp/skeys" } }
+#     <- { "return": {} }
+##
+{ 'command': 'dump-s390-skeys',
+  'data': { 'filename': 'str' } }
diff --git a/hw/s390x/s390-skeys-stub.c b/hw/s390x/s390-skeys-stub.c
new file mode 100644
index 0000000000..50b5f83437
--- /dev/null
+++ b/hw/s390x/s390-skeys-stub.c
@@ -0,0 +1,16 @@
+/*
+ * s390 storage key device stubs
+ *
+ * SPDX-FileContributor: Philippe Mathieu-Daudé <philmd@linaro.org>
+ * SPDX-FileCopyrightText: 2023 Linaro Ltd.
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "qapi/qapi-commands-misc.h"
+
+void qmp_dump_s390_skeys(const char *filename, Error **errp)
+{
+    error_setg(errp, "This guest is not using s390 storage keys - "
+                     "nothing to dump");
+}
diff --git a/hw/s390x/s390-skeys.c b/hw/s390x/s390-skeys.c
index 5c535d483e..f9bb08eb92 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-misc.h"
 #include "qapi/qapi-commands-misc-target.h"
 #include "qapi/qmp/qdict.h"
 #include "qemu/error-report.h"
@@ -115,7 +116,7 @@ void hmp_dump_skeys(Monitor *mon, const QDict *qdict)
     }
 }
 
-void qmp_dump_skeys(const char *filename, Error **errp)
+void qmp_dump_s390_skeys(const char *filename, Error **errp)
 {
     S390SKeysState *ss = s390_get_skeys_device();
     S390SKeysClass *skeyclass = S390_SKEYS_GET_CLASS(ss);
@@ -192,6 +193,11 @@ out:
     fclose(f);
 }
 
+void qmp_dump_skeys(const char *filename, Error **errp)
+{
+    qmp_dump_s390_skeys(filename, errp);
+}
+
 static bool qemu_s390_skeys_are_enabled(S390SKeysState *ss)
 {
     QEMUS390SKeysState *skeys = QEMU_S390_SKEYS(ss);
diff --git a/hw/s390x/meson.build b/hw/s390x/meson.build
index 482fd13420..c94b8d819d 100644
--- a/hw/s390x/meson.build
+++ b/hw/s390x/meson.build
@@ -50,6 +50,11 @@ virtio_ss.add(when: 'CONFIG_VHOST_VSOCK', if_true: files('vhost-vsock-ccw.c'))
 virtio_ss.add(when: 'CONFIG_VHOST_USER_FS', if_true: files('vhost-user-fs-ccw.c'))
 s390x_ss.add_all(when: 'CONFIG_VIRTIO_CCW', if_true: virtio_ss)
 
+s390x_stub_ss = ss.source_set()
+s390x_stub_ss.add(when: 'TARGET_S390X', if_false: files('s390-skeys-stub.c'))
+
+specific_ss.add_all(s390x_stub_ss)
+
 hw_arch += {'s390x': s390x_ss}
 
 hw_s390x_modules = {}
-- 
2.41.0

[PATCH 2/4] hw/s390x: Introduce the 'dump_s390_skeys' HMP command

[Philippe Mathieu-Daudé]

'dump_skeys' is specific to the qemu-system-s390x binary.
In order to provide it in an unified single binary, add
a equivalent new command named 'dump_s390_skeys', which
works identically on s390x and reports an error on other
targets.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/monitor/hmp.h      |  1 +
 hw/s390x/s390-skeys-stub.c |  8 ++++++++
 hw/s390x/s390-skeys.c      |  9 ++++++++-
 hmp-commands.hx            | 13 +++++++++++++
 4 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h
index 954f3c83ad..647a1e4a3f 100644
--- a/include/monitor/hmp.h
+++ b/include/monitor/hmp.h
@@ -181,5 +181,6 @@ void hmp_boot_set(Monitor *mon, const QDict *qdict);
 void hmp_info_mtree(Monitor *mon, const QDict *qdict);
 void hmp_info_cryptodev(Monitor *mon, const QDict *qdict);
 void hmp_dumpdtb(Monitor *mon, const QDict *qdict);
+void hmp_dump_s390_skeys(Monitor *mon, const QDict *qdict);
 
 #endif
diff --git a/hw/s390x/s390-skeys-stub.c b/hw/s390x/s390-skeys-stub.c
index 50b5f83437..94b491425b 100644
--- a/hw/s390x/s390-skeys-stub.c
+++ b/hw/s390x/s390-skeys-stub.c
@@ -8,6 +8,14 @@
 #include "qemu/osdep.h"
 #include "qapi/error.h"
 #include "qapi/qapi-commands-misc.h"
+#include "monitor/monitor.h"
+#include "monitor/hmp.h"
+
+void hmp_dump_s390_skeys(Monitor *mon, const QDict *qdict)
+{
+    monitor_printf(mon, "This guest is not using s390 storage keys - "
+                     "nothing to dump\n");
+}
 
 void qmp_dump_s390_skeys(const char *filename, Error **errp)
 {
diff --git a/hw/s390x/s390-skeys.c b/hw/s390x/s390-skeys.c
index f9bb08eb92..e7dab52a6e 100644
--- a/hw/s390x/s390-skeys.c
+++ b/hw/s390x/s390-skeys.c
@@ -24,6 +24,8 @@
 #include "sysemu/kvm.h"
 #include "migration/qemu-file-types.h"
 #include "migration/register.h"
+#include "monitor/monitor.h"
+#include "monitor/hmp.h"
 
 #define S390_SKEYS_BUFFER_SIZE (128 * KiB)  /* Room for 128k storage keys */
 #define S390_SKEYS_SAVE_FLAG_EOS 0x01
@@ -105,7 +107,7 @@ void hmp_info_skeys(Monitor *mon, const QDict *qdict)
     monitor_printf(mon, "  key: 0x%X\n", key);
 }
 
-void hmp_dump_skeys(Monitor *mon, const QDict *qdict)
+void hmp_dump_s390_skeys(Monitor *mon, const QDict *qdict)
 {
     const char *filename = qdict_get_str(qdict, "filename");
     Error *err = NULL;
@@ -116,6 +118,11 @@ void hmp_dump_skeys(Monitor *mon, const QDict *qdict)
     }
 }
 
+void hmp_dump_skeys(Monitor *mon, const QDict *qdict)
+{
+    hmp_dump_s390_skeys(mon, qdict);
+}
+
 void qmp_dump_s390_skeys(const char *filename, Error **errp)
 {
     S390SKeysState *ss = s390_get_skeys_device();
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 06746f0afc..c12e2c2bd9 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1120,6 +1120,19 @@ SRST
 
 ERST
 
+    {
+        .name       = "dump-s390-skeys",
+        .args_type  = "filename:F",
+        .params     = "",
+        .help       = "Save guest storage keys into file 'filename'.",
+        .cmd        = hmp_dump_s390_skeys,
+    },
+
+SRST
+``dump-s390-skeys`` *filename*
+  Save guest storage keys to a file.
+ERST
+
 #if defined(TARGET_S390X)
     {
         .name       = "dump-skeys",
-- 
2.41.0

[PATCH 3/4] hw/s390x: Deprecate the HMP 'dump_skeys' command

[Philippe Mathieu-Daudé]

Prefer 'dump_s390_skeys' which is target agnostic.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/s390x/s390-skeys.c | 2 ++
 hmp-commands.hx       | 4 ++--
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/s390x/s390-skeys.c b/hw/s390x/s390-skeys.c
index e7dab52a6e..12ec8a808e 100644
--- a/hw/s390x/s390-skeys.c
+++ b/hw/s390x/s390-skeys.c
@@ -120,6 +120,8 @@ void hmp_dump_s390_skeys(Monitor *mon, const QDict *qdict)
 
 void hmp_dump_skeys(Monitor *mon, const QDict *qdict)
 {
+    monitor_printf(mon, "This command is deprecated"
+                        " (replaced by 'dump-s390-skeys')\n");
     hmp_dump_s390_skeys(mon, qdict);
 }
 
diff --git a/hmp-commands.hx b/hmp-commands.hx
index c12e2c2bd9..04ae897134 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1138,14 +1138,14 @@ ERST
         .name       = "dump-skeys",
         .args_type  = "filename:F",
         .params     = "",
-        .help       = "Save guest storage keys into file 'filename'.\n",
+        .help       = "deprecated synonym for dump-s390-skeys.",
         .cmd        = hmp_dump_skeys,
     },
 #endif
 
 SRST
 ``dump-skeys`` *filename*
-  Save guest storage keys to a file.
+  This is a deprecated synonym for the dump-s390-skeys command.
 ERST
 
 #if defined(TARGET_S390X)
-- 
2.41.0

[PATCH 4/4] hw/s390x: Deprecate the QMP @dump-skeys command

[Philippe Mathieu-Daudé]

Prefer @dump-s390-skeys which is target agnostic.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 docs/about/deprecated.rst | 5 +++++
 qapi/misc-target.json     | 5 +++++
 2 files changed, 10 insertions(+)

diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index 40585ca7d5..3cb43085ba 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -148,6 +148,11 @@ accepted incorrect commands will return an error. Users should make sure that
 all arguments passed to ``device_add`` are consistent with the documented
 property types.
 
+``dump-skeys`` (since 9.1)
+''''''''''''''''''''''''''
+
+Use the more generic ``dump-s390-skeys`` command.
+
 QEMU Machine Protocol (QMP) events
 ----------------------------------
 
diff --git a/qapi/misc-target.json b/qapi/misc-target.json
index 4e0a6492a9..e5109b1265 100644
--- a/qapi/misc-target.json
+++ b/qapi/misc-target.json
@@ -237,6 +237,10 @@
 #
 # @filename: the path to the file to dump to
 #
+# Features:
+#
+# @deprecated: This command is deprecated.  Use @dump-s390-skeys instead.
+#
 # Since: 2.5
 #
 # Example:
@@ -247,6 +251,7 @@
 ##
 { 'command': 'dump-skeys',
   'data': { 'filename': 'str' },
+  'features': ['deprecated'],
   'if': 'TARGET_S390X' }
 
 ##
-- 
2.41.0

Re: [PATCH 4/4] hw/s390x: Deprecate the QMP @dump-skeys command

[Thomas Huth]

On 30/05/2024 09.45, Philippe Mathieu-Daudé wrote:
> Prefer @dump-s390-skeys which is target agnostic.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   docs/about/deprecated.rst | 5 +++++
>   qapi/misc-target.json     | 5 +++++
>   2 files changed, 10 insertions(+)
> 
> diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
> index 40585ca7d5..3cb43085ba 100644
> --- a/docs/about/deprecated.rst
> +++ b/docs/about/deprecated.rst
> @@ -148,6 +148,11 @@ accepted incorrect commands will return an error. Users should make sure that
>   all arguments passed to ``device_add`` are consistent with the documented
>   property types.
>   
> +``dump-skeys`` (since 9.1)
> +''''''''''''''''''''''''''
> +
> +Use the more generic ``dump-s390-skeys`` command.

FWIW, that's "more specific", not "more generic".

But as I said in my reply to the cover letter, we should maybe consider a 
more generic command instead, indeed.

  Thomas

Re: [PATCH 0/4] hw/s390x: Alias @dump-skeys -> @dump-s390-skey and deprecate

[Anton Johansson via]

On 30/05/24, Philippe Mathieu-Daudé wrote:
> 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.
> 
> @dump-skeys is only available on qemu-system-s390x. This series
> rename it as @dump-s390-skey, making it available on other
> binaries. We take care of backward compatibility via deprecation.
> 
> Philippe Mathieu-Daudé (4):
>   hw/s390x: Introduce the @dump-s390-skeys QMP command
>   hw/s390x: Introduce the 'dump_s390_skeys' HMP command
>   hw/s390x: Deprecate the HMP 'dump_skeys' command
>   hw/s390x: Deprecate the QMP @dump-skeys command

Series:
Reviewed-by: Anton Johansson <anjo@rev.ng>

Re: [PATCH 0/4] hw/s390x: Alias @dump-skeys -> @dump-s390-skey and deprecate

[Thomas Huth]

On 30/05/2024 09.45, Philippe Mathieu-Daudé wrote:
> 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.
> 
> @dump-skeys is only available on qemu-system-s390x. This series
> rename it as @dump-s390-skey, making it available on other
> binaries. We take care of backward compatibility via deprecation.
> 
> Philippe Mathieu-Daudé (4):
>    hw/s390x: Introduce the @dump-s390-skeys QMP command
>    hw/s390x: Introduce the 'dump_s390_skeys' HMP command
>    hw/s390x: Deprecate the HMP 'dump_skeys' command
>    hw/s390x: Deprecate the QMP @dump-skeys command

Why do we have to rename the command? Just for the sake of it? I think 
renaming HMP commands is maybe ok, but breaking the API in QMP is something 
you should consider twice.

And even if we decide to rename ... maybe we should discuss whether it makes 
sense to come up with a generic command instead: As far as I know, ARM also 
has something similar, called MTE. Maybe we also want to dump MTE keys one 
day? So the new command should maybe be called "dump-memory-keys" instead? 
Or should it maybe rather be an option to the existing "dump-guest-memory" 
command instead?

  Thomas

Re: [PATCH 0/4] hw/s390x: Alias @dump-skeys -> @dump-s390-skey and deprecate

[Dr. David Alan Gilbert]

* Thomas Huth (thuth@redhat.com) wrote:
> On 30/05/2024 09.45, Philippe Mathieu-Daudé wrote:
> > 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.
> > 
> > @dump-skeys is only available on qemu-system-s390x. This series
> > rename it as @dump-s390-skey, making it available on other
> > binaries. We take care of backward compatibility via deprecation.
> > 
> > Philippe Mathieu-Daudé (4):
> >    hw/s390x: Introduce the @dump-s390-skeys QMP command
> >    hw/s390x: Introduce the 'dump_s390_skeys' HMP command
> >    hw/s390x: Deprecate the HMP 'dump_skeys' command
> >    hw/s390x: Deprecate the QMP @dump-skeys command
> 
> Why do we have to rename the command? Just for the sake of it? I think
> renaming HMP commands is maybe ok, but breaking the API in QMP is something
> you should consider twice.
> 
> And even if we decide to rename ... maybe we should discuss whether it makes
> sense to come up with a generic command instead: As far as I know, ARM also
> has something similar, called MTE. Maybe we also want to dump MTE keys one
> day? So the new command should maybe be called "dump-memory-keys" instead?

I think there are at least two different concepts; but I agree it would be
nice to keep a single command for matching concepts across different architectures;
I can't say I know the details of any, but:

  a) Page table things - I think x86 PKRU/PKEY (???) is a page table thing
    where pages marked a special way are associated with keys.
    That sounds similar to what the skeys are???

  b) Upper bit things - where you steal a few bits from the virtual address
    and then use that to associate some security; I think that's closer
    to what MTE is isn't it?

I'm not sure the two fit in the same command.

Dave

> Or should it maybe rather be an option to the existing "dump-guest-memory"
> command instead?
> 
>  Thomas
> 
-- 
 -----Open up your eyes, open up your mind, open up your code -------   
/ Dr. David Alan Gilbert    |       Running GNU/Linux       | Happy  \ 
\        dave @ treblig.org |                               | In Hex /
 \ _________________________|_____ http://www.treblig.org   |_______/

Re: [PATCH 0/4] hw/s390x: Alias @dump-skeys -> @dump-s390-skey and deprecate

[Thomas Huth]

On 31/05/2024 16.02, Dr. David Alan Gilbert wrote:
> * Thomas Huth (thuth@redhat.com) wrote:
>> On 30/05/2024 09.45, Philippe Mathieu-Daudé wrote:
>>> 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.
>>>
>>> @dump-skeys is only available on qemu-system-s390x. This series
>>> rename it as @dump-s390-skey, making it available on other
>>> binaries. We take care of backward compatibility via deprecation.
>>>
>>> Philippe Mathieu-Daudé (4):
>>>     hw/s390x: Introduce the @dump-s390-skeys QMP command
>>>     hw/s390x: Introduce the 'dump_s390_skeys' HMP command
>>>     hw/s390x: Deprecate the HMP 'dump_skeys' command
>>>     hw/s390x: Deprecate the QMP @dump-skeys command
>>
>> Why do we have to rename the command? Just for the sake of it? I think
>> renaming HMP commands is maybe ok, but breaking the API in QMP is something
>> you should consider twice.
>>
>> And even if we decide to rename ... maybe we should discuss whether it makes
>> sense to come up with a generic command instead: As far as I know, ARM also
>> has something similar, called MTE. Maybe we also want to dump MTE keys one
>> day? So the new command should maybe be called "dump-memory-keys" instead?
> 
> I think there are at least two different concepts; but I agree it would be
> nice to keep a single command for matching concepts across different architectures;
> I can't say I know the details of any, but:
> 
>    a) Page table things - I think x86 PKRU/PKEY (???) is a page table thing
>      where pages marked a special way are associated with keys.
>      That sounds similar to what the skeys are???

Sounds a little bit similar, but s390 storage keys are independent from page 
tables. It's rather that each page (4096 bytes) of RAM has an additional 
7-bit value that contains the storage key and some additional bits. It's 
also usable when page tables are still disabled.

 > I'm not sure the two fit in the same command.

Does it make sense to dump all the MTE or x86 keys all at once? If so, we 
could maybe come up with an unified command. Otherwise it might not make 
sense, indeed.

  Thomas

Re: [PATCH 0/4] hw/s390x: Alias @dump-skeys -> @dump-s390-skey and deprecate

[Dr. David Alan Gilbert]

* Thomas Huth (thuth@redhat.com) wrote:
> On 31/05/2024 16.02, Dr. David Alan Gilbert wrote:
> > * Thomas Huth (thuth@redhat.com) wrote:
> > > On 30/05/2024 09.45, Philippe Mathieu-Daudé wrote:
> > > > 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.
> > > > 
> > > > @dump-skeys is only available on qemu-system-s390x. This series
> > > > rename it as @dump-s390-skey, making it available on other
> > > > binaries. We take care of backward compatibility via deprecation.
> > > > 
> > > > Philippe Mathieu-Daudé (4):
> > > >     hw/s390x: Introduce the @dump-s390-skeys QMP command
> > > >     hw/s390x: Introduce the 'dump_s390_skeys' HMP command
> > > >     hw/s390x: Deprecate the HMP 'dump_skeys' command
> > > >     hw/s390x: Deprecate the QMP @dump-skeys command
> > > 
> > > Why do we have to rename the command? Just for the sake of it? I think
> > > renaming HMP commands is maybe ok, but breaking the API in QMP is something
> > > you should consider twice.
> > > 
> > > And even if we decide to rename ... maybe we should discuss whether it makes
> > > sense to come up with a generic command instead: As far as I know, ARM also
> > > has something similar, called MTE. Maybe we also want to dump MTE keys one
> > > day? So the new command should maybe be called "dump-memory-keys" instead?
> > 
> > I think there are at least two different concepts; but I agree it would be
> > nice to keep a single command for matching concepts across different architectures;
> > I can't say I know the details of any, but:
> > 
> >    a) Page table things - I think x86 PKRU/PKEY (???) is a page table thing
> >      where pages marked a special way are associated with keys.
> >      That sounds similar to what the skeys are???
> 
> Sounds a little bit similar, but s390 storage keys are independent from page
> tables. It's rather that each page (4096 bytes) of RAM has an additional
> 7-bit value that contains the storage key and some additional bits. It's
> also usable when page tables are still disabled.
> 
> > I'm not sure the two fit in the same command.
> 
> Does it make sense to dump all the MTE or x86 keys all at once? If so, we
> could maybe come up with an unified command. Otherwise it might not make
> sense, indeed.

So they're all really different granularities:
    s390 - one key/physical page
    ARM MTE - one tag/16 bytes physical
    x86 - per virtual page; then a current register with 16 permission
         sets

For x86 I guess it would be easy to dump the register, and then the
values for a range of virtual addresses.
But then if you dumped a range of physical addresses on ARM MTE you'd
get hundreds times more output than for the equivalent s390.

Dave

>  Thomas
> 
-- 
 -----Open up your eyes, open up your mind, open up your code -------   
/ Dr. David Alan Gilbert    |       Running GNU/Linux       | Happy  \ 
\        dave @ treblig.org |                               | In Hex /
 \ _________________________|_____ http://www.treblig.org   |_______/

Re: [PATCH 0/4] hw/s390x: Alias @dump-skeys -> @dump-s390-skey and deprecate

[Daniel P. Berrangé]

On Fri, May 31, 2024 at 06:47:45AM +0200, Thomas Huth wrote:
> On 30/05/2024 09.45, Philippe Mathieu-Daudé wrote:
> > 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.
> > 
> > @dump-skeys is only available on qemu-system-s390x. This series
> > rename it as @dump-s390-skey, making it available on other
> > binaries. We take care of backward compatibility via deprecation.
> > 
> > Philippe Mathieu-Daudé (4):
> >    hw/s390x: Introduce the @dump-s390-skeys QMP command
> >    hw/s390x: Introduce the 'dump_s390_skeys' HMP command
> >    hw/s390x: Deprecate the HMP 'dump_skeys' command
> >    hw/s390x: Deprecate the QMP @dump-skeys command
> 
> Why do we have to rename the command? Just for the sake of it? I think
> renaming HMP commands is maybe ok, but breaking the API in QMP is something
> you should consider twice.

That was going to be my question too. Seems like its possible to simply
stub out the existing command for other targets.

The renaming is just window dressing.

> 
> And even if we decide to rename ... maybe we should discuss whether it makes
> sense to come up with a generic command instead: As far as I know, ARM also
> has something similar, called MTE. Maybe we also want to dump MTE keys one
> day? So the new command should maybe be called "dump-memory-keys" instead?
> Or should it maybe rather be an option to the existing "dump-guest-memory"
> command instead?

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 0/4] hw/s390x: Alias @dump-skeys -> @dump-s390-skey and deprecate

[Dr. David Alan Gilbert]

* Daniel P. Berrangé (berrange@redhat.com) wrote:
> On Fri, May 31, 2024 at 06:47:45AM +0200, Thomas Huth wrote:
> > On 30/05/2024 09.45, Philippe Mathieu-Daudé wrote:
> > > 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.
> > > 
> > > @dump-skeys is only available on qemu-system-s390x. This series
> > > rename it as @dump-s390-skey, making it available on other
> > > binaries. We take care of backward compatibility via deprecation.
> > > 
> > > Philippe Mathieu-Daudé (4):
> > >    hw/s390x: Introduce the @dump-s390-skeys QMP command
> > >    hw/s390x: Introduce the 'dump_s390_skeys' HMP command
> > >    hw/s390x: Deprecate the HMP 'dump_skeys' command
> > >    hw/s390x: Deprecate the QMP @dump-skeys command
> > 
> > Why do we have to rename the command? Just for the sake of it? I think
> > renaming HMP commands is maybe ok, but breaking the API in QMP is something
> > you should consider twice.
> 
> That was going to be my question too. Seems like its possible to simply
> stub out the existing command for other targets.

Are these commands really supposed to be stable, or are they just debug
commands?  If they are debug, then add the x- and don't worry too much.

Dave

> The renaming is just window dressing.
> 
> > 
> > And even if we decide to rename ... maybe we should discuss whether it makes
> > sense to come up with a generic command instead: As far as I know, ARM also
> > has something similar, called MTE. Maybe we also want to dump MTE keys one
> > day? So the new command should maybe be called "dump-memory-keys" instead?
> > Or should it maybe rather be an option to the existing "dump-guest-memory"
> > command instead?
> 
> 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 :|
> 
-- 
 -----Open up your eyes, open up your mind, open up your code -------   
/ Dr. David Alan Gilbert    |       Running GNU/Linux       | Happy  \ 
\        dave @ treblig.org |                               | In Hex /
 \ _________________________|_____ http://www.treblig.org   |_______/

Re: [PATCH 0/4] hw/s390x: Alias @dump-skeys -> @dump-s390-skey and deprecate

[Markus Armbruster]

"Dr. David Alan Gilbert" <dave@treblig.org> writes:

> * Daniel P. Berrangé (berrange@redhat.com) wrote:
>> On Fri, May 31, 2024 at 06:47:45AM +0200, Thomas Huth wrote:
>> > On 30/05/2024 09.45, Philippe Mathieu-Daudé wrote:
>> > > 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.
>> > > 
>> > > @dump-skeys is only available on qemu-system-s390x. This series
>> > > rename it as @dump-s390-skey, making it available on other
>> > > binaries. We take care of backward compatibility via deprecation.
>> > > 
>> > > Philippe Mathieu-Daudé (4):
>> > >    hw/s390x: Introduce the @dump-s390-skeys QMP command
>> > >    hw/s390x: Introduce the 'dump_s390_skeys' HMP command
>> > >    hw/s390x: Deprecate the HMP 'dump_skeys' command
>> > >    hw/s390x: Deprecate the QMP @dump-skeys command
>> > 
>> > Why do we have to rename the command? Just for the sake of it? I think
>> > renaming HMP commands is maybe ok, but breaking the API in QMP is something
>> > you should consider twice.

PRO rename: the command's tie to S390 is them immediately obvious, which
may be useful when the command becomes available in qemu-systems capable
of running other targets.

CON rename: users need to adapt.

What are the users?  Not libvirt, as far as I can tell.

>> That was going to be my question too. Seems like its possible to simply
>> stub out the existing command for other targets.

That's going to happen whether we rename the commands or not.

> Are these commands really supposed to be stable, or are they just debug
> commands?  If they are debug, then add the x- and don't worry too much.

docs/devel/qapi-code-gen.rst:

    Names beginning with ``x-`` used to signify "experimental".  This
    convention has been replaced by special feature "unstable".

Feature "unstable" is what makes something unstable, and is what
machines should check.

An "x-" prefix may still be useful for humans.  Machines should *not*
key on the prefix.  It's unreliable anyway: InputBarrierProperties
member @x-origin is stable despite it's name.  Renames to gain or lose
the prefix may or may not be worth the bother.

Making an existing part of the interface unstable should be treated
similar to deprecating it: we keep it stable for at least the
deprecation period.  Not written policy, because we didn't consider such
changes when we documented our deprecation policy in
docs/about/deprecated.rst.

Alternative path to a renamed command (*if* we want that):

1. Make it unstable.

2. But keep it stable for two releases.

3. Rename.

[...]

Re: [PATCH 0/4] hw/s390x: Alias @dump-skeys -> @dump-s390-skey and deprecate

[Philippe Mathieu-Daudé]

Hi Daniel, Dave, Markus & Thomas.

On 4/6/24 06:58, Markus Armbruster wrote:
> "Dr. David Alan Gilbert" <dave@treblig.org> writes:
>> * Daniel P. Berrangé (berrange@redhat.com) wrote:
>>> On Fri, May 31, 2024 at 06:47:45AM +0200, Thomas Huth wrote:
>>>> On 30/05/2024 09.45, Philippe Mathieu-Daudé wrote:
>>>>> 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.
>>>>>
>>>>> @dump-skeys is only available on qemu-system-s390x. This series
>>>>> rename it as @dump-s390-skey, making it available on other
>>>>> binaries. We take care of backward compatibility via deprecation.
>>>>>
>>>>> Philippe Mathieu-Daudé (4):
>>>>>     hw/s390x: Introduce the @dump-s390-skeys QMP command
>>>>>     hw/s390x: Introduce the 'dump_s390_skeys' HMP command
>>>>>     hw/s390x: Deprecate the HMP 'dump_skeys' command
>>>>>     hw/s390x: Deprecate the QMP @dump-skeys command
>>>>
>>>> Why do we have to rename the command? Just for the sake of it? I think
>>>> renaming HMP commands is maybe ok, but breaking the API in QMP is something
>>>> you should consider twice.

I'm looking at how to include this command in the new "single binary".

Markus explained in an earlier series, just expanding this command as
stub to targets that don't implement it is not backward compatible and
breaks QMP introspection. Currently on s390x we get a result, on other
targets the command doesn't exist. If we add a stubs, then other targets
return something (even if it is an empty list), confusing management
interface.

So this approach use to deprecate process to include a new command
which behaves differently on non-s390x targets.

If we don't care for this particular case, better. However I'd still
like to discuss this approach for other target-specific commands.

> PRO rename: the command's tie to S390 is them immediately obvious, which
> may be useful when the command becomes available in qemu-systems capable
> of running other targets.
> 
> CON rename: users need to adapt.
> 
> What are the users?  Not libvirt, as far as I can tell.

Years ago we said, "all HMP must be based on QMP". Now we realize HMP
became stable because QMP-exposed, although not consumed externally...

Does the concept of "internal QMP commands" makes sense for HMP debug
ones? (Looking at a way to not expose them). We could use the "x-"
prefix to not care about stable / backward compat, but what is the point
of exposing to QMP commands that will never be accessed there?

>>> That was going to be my question too. Seems like its possible to simply
>>> stub out the existing command for other targets.
> 
> That's going to happen whether we rename the commands or not.
> 
>> Are these commands really supposed to be stable, or are they just debug
>> commands?  If they are debug, then add the x- and don't worry too much.

OK.

> docs/devel/qapi-code-gen.rst:
> 
>      Names beginning with ``x-`` used to signify "experimental".  This
>      convention has been replaced by special feature "unstable".
> 
> Feature "unstable" is what makes something unstable, and is what
> machines should check.

What I mentioned earlier could be 'Feature "internal" or "debug"'.

> An "x-" prefix may still be useful for humans.  Machines should *not*
> key on the prefix.  It's unreliable anyway: InputBarrierProperties
> member @x-origin is stable despite it's name.  Renames to gain or lose
> the prefix may or may not be worth the bother.

Could follow the rules and be renamed as "origin-coordinate-x".

> Making an existing part of the interface unstable should be treated
> similar to deprecating it: we keep it stable for at least the
> deprecation period.  Not written policy, because we didn't consider such
> changes when we documented our deprecation policy in
> docs/about/deprecated.rst.
> 
> Alternative path to a renamed command (*if* we want that):
> 
> 1. Make it unstable.
> 
> 2. But keep it stable for two releases.
> 
> 3. Rename.
> 
> [...]
> 

Re: [PATCH 0/4] hw/s390x: Alias @dump-skeys -> @dump-s390-skey and deprecate

[Daniel P. Berrangé]

On Tue, Jun 04, 2024 at 11:45:11AM +0200, Philippe Mathieu-Daudé wrote:
> Hi Daniel, Dave, Markus & Thomas.
> 
> On 4/6/24 06:58, Markus Armbruster wrote:
> > "Dr. David Alan Gilbert" <dave@treblig.org> writes:
> > > * Daniel P. Berrangé (berrange@redhat.com) wrote:
> > > > On Fri, May 31, 2024 at 06:47:45AM +0200, Thomas Huth wrote:
> > > > > On 30/05/2024 09.45, Philippe Mathieu-Daudé wrote:
> > > > > > 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.
> > > > > > 
> > > > > > @dump-skeys is only available on qemu-system-s390x. This series
> > > > > > rename it as @dump-s390-skey, making it available on other
> > > > > > binaries. We take care of backward compatibility via deprecation.
> > > > > > 
> > > > > > Philippe Mathieu-Daudé (4):
> > > > > >     hw/s390x: Introduce the @dump-s390-skeys QMP command
> > > > > >     hw/s390x: Introduce the 'dump_s390_skeys' HMP command
> > > > > >     hw/s390x: Deprecate the HMP 'dump_skeys' command
> > > > > >     hw/s390x: Deprecate the QMP @dump-skeys command
> > > > > 
> > > > > Why do we have to rename the command? Just for the sake of it? I think
> > > > > renaming HMP commands is maybe ok, but breaking the API in QMP is something
> > > > > you should consider twice.
> 
> I'm looking at how to include this command in the new "single binary".
> 
> Markus explained in an earlier series, just expanding this command as
> stub to targets that don't implement it is not backward compatible and
> breaks QMP introspection. Currently on s390x we get a result, on other
> targets the command doesn't exist. If we add a stubs, then other targets
> return something (even if it is an empty list), confusing management
> interface.

I'm not convinced about calling this "not backward compatible".

We're always free to add new commands, and there's never any
guarantee that you can execute a given command under every
possible QEMU configuration.

IOW, adding 'dump-skeys' and having it always return an error
on non-s390x targets is valid IMHO, and I wouldn't call that
a backwards compatibilit problem.

Apps shouldn't assume that just because a command is reported
in introspection, it can be used in any scenario. An app is
expected to itself understand the behaviour of any given command
sufficiently well, to know when they can execute it, or be prpared
to accept errors.

> > PRO rename: the command's tie to S390 is them immediately obvious, which
> > may be useful when the command becomes available in qemu-systems capable
> > of running other targets.
> > 
> > CON rename: users need to adapt.
> > 
> > What are the users?  Not libvirt, as far as I can tell.
> 
> Years ago we said, "all HMP must be based on QMP". Now we realize HMP
> became stable because QMP-exposed, although not consumed externally...

That's not the case though. We've added plenty of conversions of
HMP to QMP, while declaring them unstable. We might have missed
that in some cases, but that's just a bug, and we can fix that
any time by adding the 'unstable' feature tag to the schema.

> Does the concept of "internal QMP commands" makes sense for HMP debug
> ones? (Looking at a way to not expose them). We could use the "x-"
> prefix to not care about stable / backward compat, but what is the point
> of exposing to QMP commands that will never be accessed there?

Debug only, unstable commands are totally valid for QMP. There's nothing
that requires them to be inherantly HMP only.

QAPI modelling can benefit many debug only, unstable commands, but we
can also still just return printf formatted strings as documented here:

  https://www.qemu.org/docs/master/devel/writing-monitor-commands.html#writing-a-debugging-aid-returning-unstructured-text


I remain convinced that we should eventually delete the HMP impl as it
exists today, and just provide a QMP client whose interactive interface
matches what HMP provides today. There's no compelling need for HMP to
run inside QEMU, if QMP exposes all the required functionality.

> > docs/devel/qapi-code-gen.rst:
> > 
> >      Names beginning with ``x-`` used to signify "experimental".  This
> >      convention has been replaced by special feature "unstable".
> > 
> > Feature "unstable" is what makes something unstable, and is what
> > machines should check.
> 
> What I mentioned earlier could be 'Feature "internal" or "debug"'.

IMHO we don't need to invent new terms for this.

"unstable" is sufficient as it expresses that the command's
inputs and outputs are liable to change their format, which
is the case for most of the historical debug only commands.


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 0/4] hw/s390x: Alias @dump-skeys -> @dump-s390-skey and deprecate

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@linaro.org> writes:

> Hi Daniel, Dave, Markus & Thomas.
>
> On 4/6/24 06:58, Markus Armbruster wrote:
>> "Dr. David Alan Gilbert" <dave@treblig.org> writes:
>>> * Daniel P. Berrangé (berrange@redhat.com) wrote:
>>>> On Fri, May 31, 2024 at 06:47:45AM +0200, Thomas Huth wrote:
>>>>> On 30/05/2024 09.45, Philippe Mathieu-Daudé wrote:
>>>>>> 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.
>>>>>>
>>>>>> @dump-skeys is only available on qemu-system-s390x. This series
>>>>>> rename it as @dump-s390-skey, making it available on other
>>>>>> binaries. We take care of backward compatibility via deprecation.
>>>>>>
>>>>>> Philippe Mathieu-Daudé (4):
>>>>>>     hw/s390x: Introduce the @dump-s390-skeys QMP command
>>>>>>     hw/s390x: Introduce the 'dump_s390_skeys' HMP command
>>>>>>     hw/s390x: Deprecate the HMP 'dump_skeys' command
>>>>>>     hw/s390x: Deprecate the QMP @dump-skeys command
>>>>>
>>>>> Why do we have to rename the command? Just for the sake of it? I think
>>>>> renaming HMP commands is maybe ok, but breaking the API in QMP is something
>>>>> you should consider twice.
>
> I'm looking at how to include this command in the new "single binary".
>
> Markus explained in an earlier series, just expanding this command as
> stub to targets that don't implement it is not backward compatible and
> breaks QMP introspection. Currently on s390x we get a result, on other
> targets the command doesn't exist. If we add a stubs, then other targets
> return something (even if it is an empty list), confusing management
> interface.

Loss of introspection precision is a concern, not a hard "no".

We weigh all the concerns, and pick a solution we hate the least :)

> So this approach use to deprecate process to include a new command
> which behaves differently on non-s390x targets.
>
> If we don't care for this particular case, better. However I'd still
> like to discuss this approach for other target-specific commands.
>
>> PRO rename: the command's tie to S390 is them immediately obvious, which
>> may be useful when the command becomes available in qemu-systems capable
>> of running other targets.
>>
>> CON rename: users need to adapt.
>>
>> What are the users?  Not libvirt, as far as I can tell.
>
> Years ago we said, "all HMP must be based on QMP".

In practice, it's closer to "HMP must be base on QMP when the
functionality does or should exist in QMP."

>                                                    Now we realize HMP
> became stable because QMP-exposed, although not consumed externally...

I'm afraid I didn't get this part.

> Does the concept of "internal QMP commands" makes sense for HMP debug
> ones? (Looking at a way to not expose them). We could use the "x-"
> prefix to not care about stable / backward compat, but what is the point
> of exposing to QMP commands that will never be accessed there?
>
>>>> That was going to be my question too. Seems like its possible to simply
>>>> stub out the existing command for other targets.
>>
>> That's going to happen whether we rename the commands or not.
>> 
>>> Are these commands really supposed to be stable, or are they just debug
>>> commands?  If they are debug, then add the x- and don't worry too much.
>
> OK.
>
>> docs/devel/qapi-code-gen.rst:
>>
>>      Names beginning with ``x-`` used to signify "experimental".  This
>>      convention has been replaced by special feature "unstable".
>>
>> Feature "unstable" is what makes something unstable, and is what
>> machines should check.
>
> What I mentioned earlier could be 'Feature "internal" or "debug"'.

What's the difference to "unstable"?

>> An "x-" prefix may still be useful for humans.  Machines should *not*
>> key on the prefix.  It's unreliable anyway: InputBarrierProperties
>> member @x-origin is stable despite it's name.  Renames to gain or lose
>> the prefix may or may not be worth the bother.
>
> Could follow the rules and be renamed as "origin-coordinate-x".

I don't think it's worth the trouble.  The "x-" prefix is now strictly
for humans, and humans can figure out what the x- in @x-origin,
@y-origin means.

[...]

Re: [PATCH 0/4] hw/s390x: Alias @dump-skeys -> @dump-s390-skey and deprecate

[Dr. David Alan Gilbert]

* Markus Armbruster (armbru@redhat.com) wrote:
> Philippe Mathieu-Daudé <philmd@linaro.org> writes:
> 
> > Hi Daniel, Dave, Markus & Thomas.
> >
> > On 4/6/24 06:58, Markus Armbruster wrote:
> >> "Dr. David Alan Gilbert" <dave@treblig.org> writes:
> >>> * Daniel P. Berrangé (berrange@redhat.com) wrote:
> >>>> On Fri, May 31, 2024 at 06:47:45AM +0200, Thomas Huth wrote:
> >>>>> On 30/05/2024 09.45, Philippe Mathieu-Daudé wrote:
> >>>>>> 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.
> >>>>>>
> >>>>>> @dump-skeys is only available on qemu-system-s390x. This series
> >>>>>> rename it as @dump-s390-skey, making it available on other
> >>>>>> binaries. We take care of backward compatibility via deprecation.
> >>>>>>
> >>>>>> Philippe Mathieu-Daudé (4):
> >>>>>>     hw/s390x: Introduce the @dump-s390-skeys QMP command
> >>>>>>     hw/s390x: Introduce the 'dump_s390_skeys' HMP command
> >>>>>>     hw/s390x: Deprecate the HMP 'dump_skeys' command
> >>>>>>     hw/s390x: Deprecate the QMP @dump-skeys command
> >>>>>
> >>>>> Why do we have to rename the command? Just for the sake of it? I think
> >>>>> renaming HMP commands is maybe ok, but breaking the API in QMP is something
> >>>>> you should consider twice.
> >
> > I'm looking at how to include this command in the new "single binary".
> >
> > Markus explained in an earlier series, just expanding this command as
> > stub to targets that don't implement it is not backward compatible and
> > breaks QMP introspection. Currently on s390x we get a result, on other
> > targets the command doesn't exist. If we add a stubs, then other targets
> > return something (even if it is an empty list), confusing management
> > interface.
> 
> Loss of introspection precision is a concern, not a hard "no".
> 
> We weigh all the concerns, and pick a solution we hate the least :)
> 
> > So this approach use to deprecate process to include a new command
> > which behaves differently on non-s390x targets.
> >
> > If we don't care for this particular case, better. However I'd still
> > like to discuss this approach for other target-specific commands.
> >
> >> PRO rename: the command's tie to S390 is them immediately obvious, which
> >> may be useful when the command becomes available in qemu-systems capable
> >> of running other targets.
> >>
> >> CON rename: users need to adapt.
> >>
> >> What are the users?  Not libvirt, as far as I can tell.
> >
> > Years ago we said, "all HMP must be based on QMP".
> 
> In practice, it's closer to "HMP must be base on QMP when the
> functionality does or should exist in QMP."
> 
> >                                                    Now we realize HMP
> > became stable because QMP-exposed, although not consumed externally...
> 
> I'm afraid I didn't get this part.
> 
> > Does the concept of "internal QMP commands" makes sense for HMP debug
> > ones? (Looking at a way to not expose them). We could use the "x-"
> > prefix to not care about stable / backward compat, but what is the point
> > of exposing to QMP commands that will never be accessed there?
> >
> >>>> That was going to be my question too. Seems like its possible to simply
> >>>> stub out the existing command for other targets.
> >>
> >> That's going to happen whether we rename the commands or not.
> >> 
> >>> Are these commands really supposed to be stable, or are they just debug
> >>> commands?  If they are debug, then add the x- and don't worry too much.
> >
> > OK.
> >
> >> docs/devel/qapi-code-gen.rst:
> >>
> >>      Names beginning with ``x-`` used to signify "experimental".  This
> >>      convention has been replaced by special feature "unstable".
> >>
> >> Feature "unstable" is what makes something unstable, and is what
> >> machines should check.
> >
> > What I mentioned earlier could be 'Feature "internal" or "debug"'.
> 
> What's the difference to "unstable"?

It should be clear *why* something is marked x- - something that's
marked 'x-' because the feature is still in development is expected to shake
out at some point, and the interface designed so it can.
(and at some point the developer should get a prod to be asked whethere the
x- can be removed).
That's different from it permenantly being x- because it's expected to
change as the needs of the people debugging change.

Dave

> >> An "x-" prefix may still be useful for humans.  Machines should *not*
> >> key on the prefix.  It's unreliable anyway: InputBarrierProperties
> >> member @x-origin is stable despite it's name.  Renames to gain or lose
> >> the prefix may or may not be worth the bother.
> >
> > Could follow the rules and be renamed as "origin-coordinate-x".
> 
> I don't think it's worth the trouble.  The "x-" prefix is now strictly
> for humans, and humans can figure out what the x- in @x-origin,
> @y-origin means.
> 
> [...]
> 
-- 
 -----Open up your eyes, open up your mind, open up your code -------   
/ Dr. David Alan Gilbert    |       Running GNU/Linux       | Happy  \ 
\        dave @ treblig.org |                               | In Hex /
 \ _________________________|_____ http://www.treblig.org   |_______/

Re: [PATCH 0/4] hw/s390x: Alias @dump-skeys -> @dump-s390-skey and deprecate

[Markus Armbruster]

"Dr. David Alan Gilbert" <dave@treblig.org> writes:

> * Markus Armbruster (armbru@redhat.com) wrote:
>> Philippe Mathieu-Daudé <philmd@linaro.org> writes:
>> 
>> > Hi Daniel, Dave, Markus & Thomas.
>> >
>> > On 4/6/24 06:58, Markus Armbruster wrote:
>> >> "Dr. David Alan Gilbert" <dave@treblig.org> writes:
>> >>> * Daniel P. Berrangé (berrange@redhat.com) wrote:
>> >>>> On Fri, May 31, 2024 at 06:47:45AM +0200, Thomas Huth wrote:
>> >>>>> On 30/05/2024 09.45, Philippe Mathieu-Daudé wrote:
>> >>>>>> 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.
>> >>>>>>
>> >>>>>> @dump-skeys is only available on qemu-system-s390x. This series
>> >>>>>> rename it as @dump-s390-skey, making it available on other
>> >>>>>> binaries. We take care of backward compatibility via deprecation.
>> >>>>>>
>> >>>>>> Philippe Mathieu-Daudé (4):
>> >>>>>>     hw/s390x: Introduce the @dump-s390-skeys QMP command
>> >>>>>>     hw/s390x: Introduce the 'dump_s390_skeys' HMP command
>> >>>>>>     hw/s390x: Deprecate the HMP 'dump_skeys' command
>> >>>>>>     hw/s390x: Deprecate the QMP @dump-skeys command
>> >>>>>
>> >>>>> Why do we have to rename the command? Just for the sake of it? I think
>> >>>>> renaming HMP commands is maybe ok, but breaking the API in QMP is something
>> >>>>> you should consider twice.
>> >
>> > I'm looking at how to include this command in the new "single binary".
>> >
>> > Markus explained in an earlier series, just expanding this command as
>> > stub to targets that don't implement it is not backward compatible and
>> > breaks QMP introspection. Currently on s390x we get a result, on other
>> > targets the command doesn't exist. If we add a stubs, then other targets
>> > return something (even if it is an empty list), confusing management
>> > interface.
>> 
>> Loss of introspection precision is a concern, not a hard "no".
>> 
>> We weigh all the concerns, and pick a solution we hate the least :)
>> 
>> > So this approach use to deprecate process to include a new command
>> > which behaves differently on non-s390x targets.
>> >
>> > If we don't care for this particular case, better. However I'd still
>> > like to discuss this approach for other target-specific commands.
>> >
>> >> PRO rename: the command's tie to S390 is them immediately obvious, which
>> >> may be useful when the command becomes available in qemu-systems capable
>> >> of running other targets.
>> >>
>> >> CON rename: users need to adapt.
>> >>
>> >> What are the users?  Not libvirt, as far as I can tell.
>> >
>> > Years ago we said, "all HMP must be based on QMP".
>> 
>> In practice, it's closer to "HMP must be base on QMP when the
>> functionality does or should exist in QMP."
>> 
>> >                                                    Now we realize HMP
>> > became stable because QMP-exposed, although not consumed externally...
>> 
>> I'm afraid I didn't get this part.
>> 
>> > Does the concept of "internal QMP commands" makes sense for HMP debug
>> > ones? (Looking at a way to not expose them). We could use the "x-"
>> > prefix to not care about stable / backward compat, but what is the point
>> > of exposing to QMP commands that will never be accessed there?
>> >
>> >>>> That was going to be my question too. Seems like its possible to simply
>> >>>> stub out the existing command for other targets.
>> >>
>> >> That's going to happen whether we rename the commands or not.
>> >> 
>> >>> Are these commands really supposed to be stable, or are they just debug
>> >>> commands?  If they are debug, then add the x- and don't worry too much.
>> >
>> > OK.
>> >
>> >> docs/devel/qapi-code-gen.rst:
>> >>
>> >>      Names beginning with ``x-`` used to signify "experimental".  This
>> >>      convention has been replaced by special feature "unstable".
>> >>
>> >> Feature "unstable" is what makes something unstable, and is what
>> >> machines should check.
>> >
>> > What I mentioned earlier could be 'Feature "internal" or "debug"'.
>> 
>> What's the difference to "unstable"?
>
> It should be clear *why* something is marked x- - something that's
> marked 'x-' because the feature is still in development is expected to shake
> out at some point, and the interface designed so it can.
> (and at some point the developer should get a prod to be asked whethere the
> x- can be removed).
> That's different from it permenantly being x- because it's expected to
> change as the needs of the people debugging change.

When you add special feature 'unstable', the tooling insists you cover
it in the doc comment.  Review should then ensure the doc comment
explains why it is unstable.  Examples:

   # @unstable: Member @x-perf is experimental.

   # @unstable: This command is meant for debugging.

> Dave
>
>> >> An "x-" prefix may still be useful for humans.  Machines should *not*
>> >> key on the prefix.  It's unreliable anyway: InputBarrierProperties
>> >> member @x-origin is stable despite it's name.  Renames to gain or lose
>> >> the prefix may or may not be worth the bother.
>> >
>> > Could follow the rules and be renamed as "origin-coordinate-x".
>> 
>> I don't think it's worth the trouble.  The "x-" prefix is now strictly
>> for humans, and humans can figure out what the x- in @x-origin,
>> @y-origin means.
>> 
>> [...]
>> 

Re: [PATCH 0/4] hw/s390x: Alias @dump-skeys -> @dump-s390-skey and deprecate

[Dr. David Alan Gilbert]

* Markus Armbruster (armbru@redhat.com) wrote:
> "Dr. David Alan Gilbert" <dave@treblig.org> writes:
> 
> > * Markus Armbruster (armbru@redhat.com) wrote:
> >> Philippe Mathieu-Daudé <philmd@linaro.org> writes:
> >> 
> >> > Hi Daniel, Dave, Markus & Thomas.
> >> >
> >> > On 4/6/24 06:58, Markus Armbruster wrote:
> >> >> "Dr. David Alan Gilbert" <dave@treblig.org> writes:
> >> >>> * Daniel P. Berrangé (berrange@redhat.com) wrote:
> >> >>>> On Fri, May 31, 2024 at 06:47:45AM +0200, Thomas Huth wrote:
> >> >>>>> On 30/05/2024 09.45, Philippe Mathieu-Daudé wrote:
> >> >>>>>> 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.
> >> >>>>>>
> >> >>>>>> @dump-skeys is only available on qemu-system-s390x. This series
> >> >>>>>> rename it as @dump-s390-skey, making it available on other
> >> >>>>>> binaries. We take care of backward compatibility via deprecation.
> >> >>>>>>
> >> >>>>>> Philippe Mathieu-Daudé (4):
> >> >>>>>>     hw/s390x: Introduce the @dump-s390-skeys QMP command
> >> >>>>>>     hw/s390x: Introduce the 'dump_s390_skeys' HMP command
> >> >>>>>>     hw/s390x: Deprecate the HMP 'dump_skeys' command
> >> >>>>>>     hw/s390x: Deprecate the QMP @dump-skeys command
> >> >>>>>
> >> >>>>> Why do we have to rename the command? Just for the sake of it? I think
> >> >>>>> renaming HMP commands is maybe ok, but breaking the API in QMP is something
> >> >>>>> you should consider twice.
> >> >
> >> > I'm looking at how to include this command in the new "single binary".
> >> >
> >> > Markus explained in an earlier series, just expanding this command as
> >> > stub to targets that don't implement it is not backward compatible and
> >> > breaks QMP introspection. Currently on s390x we get a result, on other
> >> > targets the command doesn't exist. If we add a stubs, then other targets
> >> > return something (even if it is an empty list), confusing management
> >> > interface.
> >> 
> >> Loss of introspection precision is a concern, not a hard "no".
> >> 
> >> We weigh all the concerns, and pick a solution we hate the least :)
> >> 
> >> > So this approach use to deprecate process to include a new command
> >> > which behaves differently on non-s390x targets.
> >> >
> >> > If we don't care for this particular case, better. However I'd still
> >> > like to discuss this approach for other target-specific commands.
> >> >
> >> >> PRO rename: the command's tie to S390 is them immediately obvious, which
> >> >> may be useful when the command becomes available in qemu-systems capable
> >> >> of running other targets.
> >> >>
> >> >> CON rename: users need to adapt.
> >> >>
> >> >> What are the users?  Not libvirt, as far as I can tell.
> >> >
> >> > Years ago we said, "all HMP must be based on QMP".
> >> 
> >> In practice, it's closer to "HMP must be base on QMP when the
> >> functionality does or should exist in QMP."
> >> 
> >> >                                                    Now we realize HMP
> >> > became stable because QMP-exposed, although not consumed externally...
> >> 
> >> I'm afraid I didn't get this part.
> >> 
> >> > Does the concept of "internal QMP commands" makes sense for HMP debug
> >> > ones? (Looking at a way to not expose them). We could use the "x-"
> >> > prefix to not care about stable / backward compat, but what is the point
> >> > of exposing to QMP commands that will never be accessed there?
> >> >
> >> >>>> That was going to be my question too. Seems like its possible to simply
> >> >>>> stub out the existing command for other targets.
> >> >>
> >> >> That's going to happen whether we rename the commands or not.
> >> >> 
> >> >>> Are these commands really supposed to be stable, or are they just debug
> >> >>> commands?  If they are debug, then add the x- and don't worry too much.
> >> >
> >> > OK.
> >> >
> >> >> docs/devel/qapi-code-gen.rst:
> >> >>
> >> >>      Names beginning with ``x-`` used to signify "experimental".  This
> >> >>      convention has been replaced by special feature "unstable".
> >> >>
> >> >> Feature "unstable" is what makes something unstable, and is what
> >> >> machines should check.
> >> >
> >> > What I mentioned earlier could be 'Feature "internal" or "debug"'.
> >> 
> >> What's the difference to "unstable"?
> >
> > It should be clear *why* something is marked x- - something that's
> > marked 'x-' because the feature is still in development is expected to shake
> > out at some point, and the interface designed so it can.
> > (and at some point the developer should get a prod to be asked whethere the
> > x- can be removed).
> > That's different from it permenantly being x- because it's expected to
> > change as the needs of the people debugging change.
> 
> When you add special feature 'unstable', the tooling insists you cover
> it in the doc comment.  Review should then ensure the doc comment
> explains why it is unstable.  Examples:
> 
>    # @unstable: Member @x-perf is experimental.
> 
>    # @unstable: This command is meant for debugging.

OK, that makes some sense.

Dave

> > Dave
> >
> >> >> An "x-" prefix may still be useful for humans.  Machines should *not*
> >> >> key on the prefix.  It's unreliable anyway: InputBarrierProperties
> >> >> member @x-origin is stable despite it's name.  Renames to gain or lose
> >> >> the prefix may or may not be worth the bother.
> >> >
> >> > Could follow the rules and be renamed as "origin-coordinate-x".
> >> 
> >> I don't think it's worth the trouble.  The "x-" prefix is now strictly
> >> for humans, and humans can figure out what the x- in @x-origin,
> >> @y-origin means.
> >> 
> >> [...]
> >> 
> 
-- 
 -----Open up your eyes, open up your mind, open up your code -------   
/ Dr. David Alan Gilbert    |       Running GNU/Linux       | Happy  \ 
\        dave @ treblig.org |                               | In Hex /
 \ _________________________|_____ http://www.treblig.org   |_______/

Re: [PATCH 0/4] hw/s390x: Alias @dump-skeys -> @dump-s390-skey and deprecate

[Pierrick Bouvier]

On 6/3/24 05:18, Daniel P. Berrangé wrote:
> On Fri, May 31, 2024 at 06:47:45AM +0200, Thomas Huth wrote:
>> On 30/05/2024 09.45, Philippe Mathieu-Daudé wrote:
>>> 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.
>>>
>>> @dump-skeys is only available on qemu-system-s390x. This series
>>> rename it as @dump-s390-skey, making it available on other
>>> binaries. We take care of backward compatibility via deprecation.
>>>
>>> Philippe Mathieu-Daudé (4):
>>>     hw/s390x: Introduce the @dump-s390-skeys QMP command
>>>     hw/s390x: Introduce the 'dump_s390_skeys' HMP command
>>>     hw/s390x: Deprecate the HMP 'dump_skeys' command
>>>     hw/s390x: Deprecate the QMP @dump-skeys command
>>
>> Why do we have to rename the command? Just for the sake of it? I think
>> renaming HMP commands is maybe ok, but breaking the API in QMP is something
>> you should consider twice.
> 
> That was going to be my question too. Seems like its possible to simply
> stub out the existing command for other targets.
> 
> The renaming is just window dressing.
> 

Working on single-binary topic means specificities from every qemu 
binary/architecture has to be merged together. Despite appearing has a 
bad thing now, it's definitely a step forward for QEMU, and will allow 
to enable new usages.

The hard way is to trigger a deep refactoring, involving lengthy 
conversations where compromises have to be found ("let's implement this 
for all arch"). The pragmatic way is to eliminate obvious stuff.

This command is specific to an arch, so renaming is a good and obvious 
strategy. For the backward compatible anxious developer, another 
strategy would be to simply declare this command if the running target 
is s390x. But then, you create a precedent to do something that should 
not have existed in the first place.

+1 for the renaming, and hope that users of this command are able to 
change a line in their script to adapt to the new command.

>>
>> And even if we decide to rename ... maybe we should discuss whether it makes
>> sense to come up with a generic command instead: As far as I know, ARM also
>> has something similar, called MTE. Maybe we also want to dump MTE keys one
>> day? So the new command should maybe be called "dump-memory-keys" instead?
>> Or should it maybe rather be an option to the existing "dump-guest-memory"
>> command instead?
> 
> With regards,
> Daniel

Re: [PATCH 0/4] hw/s390x: Alias @dump-skeys -> @dump-s390-skey and deprecate

[Thomas Huth]

On 09/03/2025 19.55, Pierrick Bouvier wrote:
> On 6/3/24 05:18, Daniel P. Berrangé wrote:
>> On Fri, May 31, 2024 at 06:47:45AM +0200, Thomas Huth wrote:
>>> On 30/05/2024 09.45, Philippe Mathieu-Daudé wrote:
>>>> 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.
>>>>
>>>> @dump-skeys is only available on qemu-system-s390x. This series
>>>> rename it as @dump-s390-skey, making it available on other
>>>> binaries. We take care of backward compatibility via deprecation.
>>>>
>>>> Philippe Mathieu-Daudé (4):
>>>>     hw/s390x: Introduce the @dump-s390-skeys QMP command
>>>>     hw/s390x: Introduce the 'dump_s390_skeys' HMP command
>>>>     hw/s390x: Deprecate the HMP 'dump_skeys' command
>>>>     hw/s390x: Deprecate the QMP @dump-skeys command
>>>
>>> Why do we have to rename the command? Just for the sake of it? I think
>>> renaming HMP commands is maybe ok, but breaking the API in QMP is something
>>> you should consider twice.
>>
>> That was going to be my question too. Seems like its possible to simply
>> stub out the existing command for other targets.
>>
>> The renaming is just window dressing.
>>
> 
> Working on single-binary topic means specificities from every qemu binary/ 
> architecture has to be merged together. Despite appearing has a bad thing 
> now, it's definitely a step forward for QEMU, and will allow to enable new 
> usages.
> 
> The hard way is to trigger a deep refactoring, involving lengthy 
> conversations where compromises have to be found ("let's implement this for 
> all arch"). The pragmatic way is to eliminate obvious stuff.
> 
> This command is specific to an arch, so renaming is a good and obvious 
> strategy. For the backward compatible anxious developer, another strategy 
> would be to simply declare this command if the running target is s390x. But 
> then, you create a precedent to do something that should not have existed in 
> the first place. 
> 
> +1 for the renaming, and hope that users of this command are able to change 
> a line in their script to adapt to the new command.

Sorry, but no: We've got plenty of other target specific commands... 
rtc-reset-reinjection , query-sev, query-gic-capabilities, just to name some 
few. So unless you provide a patch series to rename *all* of them and 
deprecate the previous names, I don't see the point why changing just one 
single s390x command is necessary.

  Thomas

Re: [PATCH 0/4] hw/s390x: Alias @dump-skeys -> @dump-s390-skey and deprecate

[Daniel P. Berrangé]

On Mon, Mar 10, 2025 at 07:50:57AM +0100, Thomas Huth wrote:
> On 09/03/2025 19.55, Pierrick Bouvier wrote:
> > On 6/3/24 05:18, Daniel P. Berrangé wrote:
> > > On Fri, May 31, 2024 at 06:47:45AM +0200, Thomas Huth wrote:
> > > > On 30/05/2024 09.45, Philippe Mathieu-Daudé wrote:
> > > > > 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.
> > > > > 
> > > > > @dump-skeys is only available on qemu-system-s390x. This series
> > > > > rename it as @dump-s390-skey, making it available on other
> > > > > binaries. We take care of backward compatibility via deprecation.
> > > > > 
> > > > > Philippe Mathieu-Daudé (4):
> > > > >     hw/s390x: Introduce the @dump-s390-skeys QMP command
> > > > >     hw/s390x: Introduce the 'dump_s390_skeys' HMP command
> > > > >     hw/s390x: Deprecate the HMP 'dump_skeys' command
> > > > >     hw/s390x: Deprecate the QMP @dump-skeys command
> > > > 
> > > > Why do we have to rename the command? Just for the sake of it? I think
> > > > renaming HMP commands is maybe ok, but breaking the API in QMP is something
> > > > you should consider twice.
> > > 
> > > That was going to be my question too. Seems like its possible to simply
> > > stub out the existing command for other targets.
> > > 
> > > The renaming is just window dressing.
> > > 
> > 
> > Working on single-binary topic means specificities from every qemu
> > binary/ architecture has to be merged together. Despite appearing has a
> > bad thing now, it's definitely a step forward for QEMU, and will allow
> > to enable new usages.
> > 
> > The hard way is to trigger a deep refactoring, involving lengthy
> > conversations where compromises have to be found ("let's implement this
> > for all arch"). The pragmatic way is to eliminate obvious stuff.
> > 
> > This command is specific to an arch, so renaming is a good and obvious
> > strategy. For the backward compatible anxious developer, another
> > strategy would be to simply declare this command if the running target
> > is s390x. But then, you create a precedent to do something that should
> > not have existed in the first place.
> > 
> > +1 for the renaming, and hope that users of this command are able to
> > change a line in their script to adapt to the new command.
> 
> Sorry, but no: We've got plenty of other target specific commands...
> rtc-reset-reinjection , query-sev, query-gic-capabilities, just to name some
> few. So unless you provide a patch series to rename *all* of them and
> deprecate the previous names, I don't see the point why changing just one
> single s390x command is necessary.

Agreed, I don't see a need to special case s390 and rename this command,
nor extend it to all the others you mention.

The QAPI docs will show users that it is s390x specific if they didn't
already work it out for themselves.

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 0/4] hw/s390x: Alias @dump-skeys -> @dump-s390-skey and deprecate

[Dr. David Alan Gilbert]

* Thomas Huth (thuth@redhat.com) wrote:
> On 09/03/2025 19.55, Pierrick Bouvier wrote:
> > On 6/3/24 05:18, Daniel P. Berrangé wrote:
> > > On Fri, May 31, 2024 at 06:47:45AM +0200, Thomas Huth wrote:
> > > > On 30/05/2024 09.45, Philippe Mathieu-Daudé wrote:
> > > > > 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.
> > > > > 
> > > > > @dump-skeys is only available on qemu-system-s390x. This series
> > > > > rename it as @dump-s390-skey, making it available on other
> > > > > binaries. We take care of backward compatibility via deprecation.
> > > > > 
> > > > > Philippe Mathieu-Daudé (4):
> > > > >     hw/s390x: Introduce the @dump-s390-skeys QMP command
> > > > >     hw/s390x: Introduce the 'dump_s390_skeys' HMP command
> > > > >     hw/s390x: Deprecate the HMP 'dump_skeys' command
> > > > >     hw/s390x: Deprecate the QMP @dump-skeys command
> > > > 
> > > > Why do we have to rename the command? Just for the sake of it? I think
> > > > renaming HMP commands is maybe ok, but breaking the API in QMP is something
> > > > you should consider twice.
> > > 
> > > That was going to be my question too. Seems like its possible to simply
> > > stub out the existing command for other targets.
> > > 
> > > The renaming is just window dressing.
> > > 
> > 
> > Working on single-binary topic means specificities from every qemu
> > binary/ architecture has to be merged together. Despite appearing has a
> > bad thing now, it's definitely a step forward for QEMU, and will allow
> > to enable new usages.
> > 
> > The hard way is to trigger a deep refactoring, involving lengthy
> > conversations where compromises have to be found ("let's implement this
> > for all arch"). The pragmatic way is to eliminate obvious stuff.
> > 
> > This command is specific to an arch, so renaming is a good and obvious
> > strategy. For the backward compatible anxious developer, another
> > strategy would be to simply declare this command if the running target
> > is s390x. But then, you create a precedent to do something that should
> > not have existed in the first place.
> > 
> > +1 for the renaming, and hope that users of this command are able to
> > change a line in their script to adapt to the new command.
> 
> Sorry, but no: We've got plenty of other target specific commands...
> rtc-reset-reinjection , query-sev, query-gic-capabilities, just to name some
> few. So unless you provide a patch series to rename *all* of them and
> deprecate the previous names, I don't see the point why changing just one
> single s390x command is necessary.

I'd probably agree; mind you, it wouldn't be a bad convention to
adopt in general.
For HMP, since there's no need to have a fixed schema for a command,
it would be fine to have a generic command for all architectures
that have a similar idea even if their data is very different.

Dave

>  Thomas
> 
-- 
 -----Open up your eyes, open up your mind, open up your code -------   
/ Dr. David Alan Gilbert    |       Running GNU/Linux       | Happy  \ 
\        dave @ treblig.org |                               | In Hex /
 \ _________________________|_____ http://www.treblig.org   |_______/

Re: [PATCH 0/4] hw/s390x: Alias @dump-skeys -> @dump-s390-skey and deprecate

[Philippe Mathieu-Daudé]

Cc'ing Pierrick

On 30/5/24 09:45, Philippe Mathieu-Daudé wrote:
> 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.
> 
> @dump-skeys is only available on qemu-system-s390x. This series
> rename it as @dump-s390-skey, making it available on other
> binaries. We take care of backward compatibility via deprecation.
> 
> Philippe Mathieu-Daudé (4):
>    hw/s390x: Introduce the @dump-s390-skeys QMP command
>    hw/s390x: Introduce the 'dump_s390_skeys' HMP command
>    hw/s390x: Deprecate the HMP 'dump_skeys' command
>    hw/s390x: Deprecate the QMP @dump-skeys command
> 
>   docs/about/deprecated.rst  |  5 +++++
>   qapi/misc-target.json      |  5 +++++
>   qapi/misc.json             | 18 ++++++++++++++++++
>   include/monitor/hmp.h      |  1 +
>   hw/s390x/s390-skeys-stub.c | 24 ++++++++++++++++++++++++
>   hw/s390x/s390-skeys.c      | 19 +++++++++++++++++--
>   hmp-commands.hx            | 17 +++++++++++++++--
>   hw/s390x/meson.build       |  5 +++++
>   8 files changed, 90 insertions(+), 4 deletions(-)
>   create mode 100644 hw/s390x/s390-skeys-stub.c
>