qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 08/11] mos6522: add "info via" HMP command for debugging


From: Philippe Mathieu-Daudé
Subject: Re: [PATCH 08/11] mos6522: add "info via" HMP command for debugging
Date: Mon, 21 Feb 2022 13:20:48 +0100
User-agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0) Gecko/20100101 Thunderbird/91.6.0

+Thomas

On 20/2/22 18:18, Mark Cave-Ayland wrote:
On 08/02/2022 13:10, Daniel P. Berrangé wrote:

On Tue, Feb 08, 2022 at 01:06:59PM +0000, Mark Cave-Ayland wrote:
On 08/02/2022 12:49, Daniel P. Berrangé wrote:

I was under the impression that monitor_register_hmp_info_hrt() does all the magic here i.e. it declares the underlying QMP command with an x- prefix and
effectively encapsulates the text field in a way that says "this is an
unreliable text opaque for humans"?

The monitor_register_hmp_info_hrt only does the HMP glue side, and
that's only needed if you must dynamically register the HMP command.
For statically registered commands set '.cmd_info_hrt' directly in
the hml-commands-info.hx for the HMP side.

If a qapi/ schema is needed could you explain what it should look like for this example and where it should go? Looking at the existing .json files I
can't immediately see one which is the right place for this to live.

Take a look in qapi/machine.json for anyof the 'x-query-XXXX' commands
there. The QAPI bit is fairly simple.

if you want to see an illustration of what's different from a previous
pure HMP impl, look at:

    commit dd98234c059e6bdb05a52998270df6d3d990332e
    Author: Daniel P. Berrangé <berrange@redhat.com>
    Date:   Wed Sep 8 10:35:43 2021 +0100

      qapi: introduce x-query-roms QMP command

I see, thanks for the reference. So qapi/machine.json would be the right
place to declare the QMP part even for a specific device?

Even this approach still wouldn't work in its current form though, since as
mentioned in my previous email it seems that only the target CONFIG_*
defines and not the device CONFIG_* defines are present when processing
hmp-commands-info.hx.

Yeah, that's where the pain comes in.  While QAPI schema can be made
conditional on a few CONFIG_* parameters - basically those derived
from global configure time options, it is impossible for this to be
with with target specific options like the device CONFIG_* defines.

This is why I suggested in my othuer reply that it would need to be
done with a generic 'info dev-debug' / 'x-query-dev-debug' command
that can be registered unconditionally, and then individual devices
plug into it.

After some more experiments this afternoon I still seem to be falling through the gaps on this one. This is based upon my understanding that all new HMP commands should use a QMP HumanReadableText implementation and the new command should be restricted according to target.

Currently I am working with this change to hmp-commands-info.hx and qapi/misc-target.json:


diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
index 7e6bd30395..aac86d5473 100644
--- a/hmp-commands-info.hx
+++ b/hmp-commands-info.hx
@@ -880,15 +880,15 @@ SRST
      Show intel SGX information.
  ERST

#if defined(TARGET_M68K) || defined(TARGET_PPC)
      {
          .name         = "via",
          .args_type    = "",
          .params       = "",
          .help         = "show guest mos6522 VIA devices",
          .cmd_info_hrt = qmp_x_query_via,
      },
diff --git a/qapi/misc-target.json b/qapi/misc-target.json
index 4bc45d2474..72bf71888e 100644
--- a/qapi/misc-target.json
+++ b/qapi/misc-target.json
@@ -2,6 +2,8 @@
  # vim: filetype=python
  #

+{ 'include': 'common.json' }
+
  ##
  # @RTC_CHANGE:
  #
@@ -424,3 +426,19 @@
  #
  ##
 { 'command': 'query-sgx-capabilities', 'returns': 'SGXInfo', 'if': 'TARGET_I386' }
+
+##
+# @x-query-via:
+#
+# Query information on the registered mos6522 VIAs
+#
+# Features:
+# @unstable: This command is meant for debugging.
+#
+# Returns: internal mos6522 information
+#
+# Since: 7.0
+##
+{ 'command': 'x-query-via',
+  'returns': 'HumanReadableText',
+  'features': [ 'unstable' ], 'if': { 'any': [ 'TARGET_M68K', 'TARGET_PPC' ] } }


The main problem with trying to restrict the new command to a target (or targets) is that the autogenerated qapi/qapi-commands-misc-target.h QAPI header cannot be included in a device header such as mos6522.h without getting poison errors like below (which does actually make sense):


In file included from ./qapi/qapi-commands-misc-target.h:17,
                 from /home/build/src/qemu/git/qemu/include/hw/misc/mos6522.h:35,
                  from ../hw/misc/mos6522.c:30:
./qapi/qapi-types-misc-target.h:19:13: error: attempt to use poisoned "TARGET_ALPHA"

Can be kludged by making this device target-specific:

-- >8 --
diff --git a/hw/misc/meson.build b/hw/misc/meson.build
index 6dcbe044f3..65837b1dfe 100644
--- a/hw/misc/meson.build
+++ b/hw/misc/meson.build
@@ -23 +23 @@ softmmu_ss.add(when: 'CONFIG_ARM_V7M', if_true: files('armv7m_ras.c'))
-softmmu_ss.add(when: 'CONFIG_MOS6522', if_true: files('mos6522.c'))
+specific_ss.add(when: 'CONFIG_MOS6522', if_true: files('mos6522.c'))
---

I'd rather keep devices generic, but sometime we can't. In this case
VIA is only used on m68k so it could be acceptable.

I can work around that by declaring the prototype for qmp_x_query_via() manually i.e.:


diff --git a/include/hw/misc/mos6522.h b/include/hw/misc/mos6522.h
index 9c21da2ddd..9677293ad0 100644
--- a/include/hw/misc/mos6522.h
+++ b/include/hw/misc/mos6522.h
@@ -30,7 +30,7 @@
  #include "exec/memory.h"
  #include "hw/sysbus.h"
  #include "hw/input/adb.h"
+#include "qapi/qapi-commands-common.h"
  #include "qom/object.h"

  /* Bits in ACR */
@@ -156,4 +156,6 @@ extern const VMStateDescription vmstate_mos6522;
  uint64_t mos6522_read(void *opaque, hwaddr addr, unsigned size);
 void mos6522_write(void *opaque, hwaddr addr, uint64_t val, unsigned size);

+HumanReadableText *qmp_x_query_via(Error **errp);
+
  #endif /* MOS6522_H */


This works fine for targets where CONFIG_MOS6522 is defined, but if I try a target such as x86_64 where the device isn't used then I hit another compilation error:


qapi/qapi-commands-misc-target.c:598:13: error: ‘qmp_marshal_output_HumanReadableText’ defined but not used [-Werror=unused-function]  static void qmp_marshal_output_HumanReadableText(HumanReadableText *ret_in,
              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors


Looking at the generated qapi/qapi-commands-misc-target.c file in question I see this:


static void qmp_marshal_output_HumanReadableText(HumanReadableText *ret_in,
                                 QObject **ret_out, Error **errp)
{
     Visitor *v;

     v = qobject_output_visitor_new_qmp(ret_out);
     if (visit_type_HumanReadableText(v, "unused", &ret_in, errp)) {
         visit_complete(v, ret_out);
     }
     visit_free(v);
     v = qapi_dealloc_visitor_new();
     visit_type_HumanReadableText(v, "unused", &ret_in, NULL);
     visit_free(v);
}

#if defined(TARGET_M68K) || defined(TARGET_PPC)
void qmp_marshal_x_query_via(QDict *args, QObject **ret, Error **errp)
{
     ...
     ...
}
#endif

i.e. qmp_marshal_output_HumanReadableText() isn't protected by the #if TARGET guards and since HumanReadableText is only used by the new qmp_x_query_via() functionality then the compiler complains and aborts the compilation.

Possibly this is an error in the QAPI generator for types hidden behind commands using "if"? Otherwise I'm not sure what is the best way to proceed, so I'd be grateful for some further pointers.


ATB,

Mark.





reply via email to

[Prev in Thread] Current Thread [Next in Thread]