[PATCH v4 16/22] qapi: introduce x-query-skeys QMP command

Daniel P. Berrangé posted 22 patches 4 years, 3 months ago
[PATCH v4 16/22] qapi: introduce x-query-skeys QMP command
Posted by Daniel P. Berrangé 4 years, 3 months ago
This is a counterpart to the HMP "info skeys" command. It is being
added with an "x-" prefix because this QMP command is intended as an
adhoc debugging tool and will thus not be modelled in QAPI as fully
structured data, nor will it have long term guaranteed stability.
The existing HMP command is rewritten to call the QMP command.

Including 'common.json' into 'machine-target.json' created a little
problem because the static marshalling method for HumanReadableText
is generated unconditionally. It is only used, however, conditionally
on certain target architectures.

To deal with this we change the QAPI code generator to simply mark
all static marshalling functions with G_GNUC_UNSED to hide the
compiler warning.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 hw/s390x/s390-skeys.c    | 35 +++++++++++++++++++++++++++--------
 qapi/machine-target.json | 17 +++++++++++++++++
 scripts/qapi/commands.py |  1 +
 3 files changed, 45 insertions(+), 8 deletions(-)

diff --git a/hw/s390x/s390-skeys.c b/hw/s390x/s390-skeys.c
index 5024faf411..62eff8c88b 100644
--- a/hw/s390x/s390-skeys.c
+++ b/hw/s390x/s390-skeys.c
@@ -15,6 +15,8 @@
 #include "hw/s390x/storage-keys.h"
 #include "qapi/error.h"
 #include "qapi/qapi-commands-misc-target.h"
+#include "qapi/qapi-commands-machine-target.h"
+#include "qapi/type-helpers.h"
 #include "qapi/qmp/qdict.h"
 #include "qemu/error-report.h"
 #include "sysemu/memory_mapping.h"
@@ -73,34 +75,51 @@ static void write_keys(FILE *f, uint8_t *keys, uint64_t startgfn,
     }
 }
 
-void hmp_info_skeys(Monitor *mon, const QDict *qdict)
+HumanReadableText *qmp_x_query_skeys(int64_t addr, Error **errp)
 {
+    g_autoptr(GString) buf = g_string_new("");
     S390SKeysState *ss = s390_get_skeys_device();
     S390SKeysClass *skeyclass = S390_SKEYS_GET_CLASS(ss);
-    uint64_t addr = qdict_get_int(qdict, "addr");
     uint8_t key;
     int r;
 
     /* Quick check to see if guest is using storage keys*/
     if (!skeyclass->skeys_are_enabled(ss)) {
-        monitor_printf(mon, "Error: This guest is not using storage keys\n");
-        return;
+        error_setg(errp, "this guest is not using storage keys");
+        return NULL;
     }
 
     if (!address_space_access_valid(&address_space_memory,
                                     addr & TARGET_PAGE_MASK, TARGET_PAGE_SIZE,
                                     false, MEMTXATTRS_UNSPECIFIED)) {
-        monitor_printf(mon, "Error: The given address is not valid\n");
-        return;
+        error_setg(errp, "the given address is not valid");
+        return NULL;
     }
 
     r = skeyclass->get_skeys(ss, addr / TARGET_PAGE_SIZE, 1, &key);
     if (r < 0) {
-        monitor_printf(mon, "Error: %s\n", strerror(-r));
+        error_setg_errno(errp, r, "unable to query storage keys");
+        return NULL;
+    }
+
+    g_string_append_printf(buf, "  key: 0x%X\n", key);
+
+    return human_readable_text_from_str(buf);
+}
+
+void hmp_info_skeys(Monitor *mon, const QDict *qdict)
+{
+    Error *err = NULL;
+    g_autoptr(HumanReadableText) info = NULL;
+    uint64_t addr = qdict_get_int(qdict, "addr");
+
+    info = qmp_x_query_skeys(addr, &err);
+    if (err) {
+        error_report_err(err);
         return;
     }
 
-    monitor_printf(mon, "  key: 0x%X\n", key);
+    monitor_printf(mon, "%s", info->human_readable_text);
 }
 
 void hmp_dump_skeys(Monitor *mon, const QDict *qdict)
diff --git a/qapi/machine-target.json b/qapi/machine-target.json
index f5ec4bc172..7d4e73462f 100644
--- a/qapi/machine-target.json
+++ b/qapi/machine-target.json
@@ -4,6 +4,8 @@
 # This work is licensed under the terms of the GNU GPL, version 2 or later.
 # See the COPYING file in the top-level directory.
 
+{ 'include': 'common.json' }
+
 ##
 # @CpuModelInfo:
 #
@@ -341,3 +343,18 @@
                    'TARGET_I386',
                    'TARGET_S390X',
                    'TARGET_MIPS' ] } }
+
+
+##
+# @x-query-skeys:
+#
+# Query the value of a storage key
+#
+# Returns: storage key value
+#
+# Since: 6.2
+##
+{ 'command': 'x-query-skeys',
+  'data': { 'addr': 'int' },
+  'returns': 'HumanReadableText',
+  'if': 'TARGET_S390X' }
diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
index 3654825968..01d8d1ea2c 100644
--- a/scripts/qapi/commands.py
+++ b/scripts/qapi/commands.py
@@ -91,6 +91,7 @@ def gen_call(name: str,
 def gen_marshal_output(ret_type: QAPISchemaType) -> str:
     return mcgen('''
 
+G_GNUC_UNUSED
 static void qmp_marshal_output_%(c_name)s(%(c_type)s ret_in,
                                 QObject **ret_out, Error **errp)
 {
-- 
2.31.1


Re: [PATCH v4 16/22] qapi: introduce x-query-skeys QMP command
Posted by Philippe Mathieu-Daudé 4 years, 3 months ago
On 10/28/21 17:54, Daniel P. Berrangé wrote:
> This is a counterpart to the HMP "info skeys" command. It is being
> added with an "x-" prefix because this QMP command is intended as an
> adhoc debugging tool and will thus not be modelled in QAPI as fully
> structured data, nor will it have long term guaranteed stability.
> The existing HMP command is rewritten to call the QMP command.
> 
> Including 'common.json' into 'machine-target.json' created a little
> problem because the static marshalling method for HumanReadableText
> is generated unconditionally. It is only used, however, conditionally
> on certain target architectures.
> 
> To deal with this we change the QAPI code generator to simply mark
> all static marshalling functions with G_GNUC_UNSED to hide the
> compiler warning.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  hw/s390x/s390-skeys.c    | 35 +++++++++++++++++++++++++++--------
>  qapi/machine-target.json | 17 +++++++++++++++++
>  scripts/qapi/commands.py |  1 +
>  3 files changed, 45 insertions(+), 8 deletions(-)

> diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
> index 3654825968..01d8d1ea2c 100644
> --- a/scripts/qapi/commands.py
> +++ b/scripts/qapi/commands.py
> @@ -91,6 +91,7 @@ def gen_call(name: str,
>  def gen_marshal_output(ret_type: QAPISchemaType) -> str:
>      return mcgen('''
>  
> +G_GNUC_UNUSED
>  static void qmp_marshal_output_%(c_name)s(%(c_type)s ret_in,
>                                  QObject **ret_out, Error **errp)
>  {
> 

I think 1/ this change should be in a separate patch,
but 2/ Markus is not going to accept it:
https://lore.kernel.org/qemu-devel/87r1haasht.fsf@dusky.pond.sub.org/

I'll see if we can get ride of it with Kconfig rules.

Meanwhile, could we get the series merged without it?


Re: [PATCH v4 16/22] qapi: introduce x-query-skeys QMP command
Posted by Daniel P. Berrangé 4 years, 3 months ago
On Tue, Nov 02, 2021 at 04:02:42PM +0100, Philippe Mathieu-Daudé wrote:
> On 10/28/21 17:54, Daniel P. Berrangé wrote:
> > This is a counterpart to the HMP "info skeys" command. It is being
> > added with an "x-" prefix because this QMP command is intended as an
> > adhoc debugging tool and will thus not be modelled in QAPI as fully
> > structured data, nor will it have long term guaranteed stability.
> > The existing HMP command is rewritten to call the QMP command.
> > 
> > Including 'common.json' into 'machine-target.json' created a little
> > problem because the static marshalling method for HumanReadableText
> > is generated unconditionally. It is only used, however, conditionally
> > on certain target architectures.
> > 
> > To deal with this we change the QAPI code generator to simply mark
> > all static marshalling functions with G_GNUC_UNSED to hide the
> > compiler warning.
> > 
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >  hw/s390x/s390-skeys.c    | 35 +++++++++++++++++++++++++++--------
> >  qapi/machine-target.json | 17 +++++++++++++++++
> >  scripts/qapi/commands.py |  1 +
> >  3 files changed, 45 insertions(+), 8 deletions(-)
> 
> > diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
> > index 3654825968..01d8d1ea2c 100644
> > --- a/scripts/qapi/commands.py
> > +++ b/scripts/qapi/commands.py
> > @@ -91,6 +91,7 @@ def gen_call(name: str,
> >  def gen_marshal_output(ret_type: QAPISchemaType) -> str:
> >      return mcgen('''
> >  
> > +G_GNUC_UNUSED
> >  static void qmp_marshal_output_%(c_name)s(%(c_type)s ret_in,
> >                                  QObject **ret_out, Error **errp)
> >  {
> > 
> 
> I think 1/ this change should be in a separate patch,
> but 2/ Markus is not going to accept it:
> https://lore.kernel.org/qemu-devel/87r1haasht.fsf@dusky.pond.sub.org/
> 
> I'll see if we can get ride of it with Kconfig rules.
> 
> Meanwhile, could we get the series merged without it?

Yeah, since soft freeze is today I'm going to drop the 3 patches that
touch machine-target.json, so we can debate this later.

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 :|