[PATCH 0/4] hw/s390x: Alias @dump-skeys -> @dump-s390-skey and deprecate
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
@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
'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
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
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
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
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>
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
* 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 |_______/
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
* 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 |_______/
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 :|
* 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 |_______/
"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. [...]
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.
[...]
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... 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 :|
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. [...]
* 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 |_______/
"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.
[...]
* 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 |_______/
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
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
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 :|
* 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 |_______/
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
participants (7)
-
Anton Johansson -
Daniel P. Berrangé -
Dr. David Alan Gilbert -
Markus Armbruster -
Philippe Mathieu-Daudé -
Pierrick Bouvier -
Thomas Huth