diff mbox series

hw/usb: Introduce x-query-usbhost QMP command

Message ID 20240610063848.51027-1-philmd@linaro.org
State New
Headers show
Series hw/usb: Introduce x-query-usbhost QMP command | expand

Commit Message

Philippe Mathieu-Daudé June 10, 2024, 6:38 a.m. UTC 
This is a counterpart to the HMP "info usbhost" 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.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 qapi/machine.json          | 18 ++++++++++++++++++
 include/hw/usb.h           |  3 ---
 hw/usb/bus-stub.c          |  7 ++++++-
 hw/usb/host-libusb.c       | 38 +++++++++++++++++++++++++-------------
 tests/qtest/qmp-cmd-test.c |  3 +++
 5 files changed, 52 insertions(+), 17 deletions(-)

Comments

Daniel P. Berrangé June 10, 2024, 8:33 a.m. UTC | #1 
On Mon, Jun 10, 2024 at 08:38:47AM +0200, Philippe Mathieu-Daudé wrote:
> This is a counterpart to the HMP "info usbhost" 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.

'info usb' host is one of the problem scenarios i mentioned in

https://lore.kernel.org/qemu-devel/20211028155457.967291-1-berrange@redhat.com/

Its HMP impl is within a dynamically loadable module. So if you
run it initially you'll get

(qemu) info usbhost
Command "info usbhost" is not available.


but if you have a usb host on the cli:

(qemu) info usbhost
  Bus 3, Addr 6, Port 8, Speed 480 Mb/s
    Class ef: USB device 04f2:b74f
  Bus 3, Addr 11, Port 7.3, Speed 1.5 Mb/s
  ...snip...


Anyway, the end result is that this patch fails to link when modules
are enabled:

cc -m64 @qemu-system-x86_64.rsp
/usr/bin/ld: libqemuutil.a.p/meson-generated_.._qapi_qapi-commands-machine.c.o: in function `qmp_marshal_x_query_usbhost':
/var/home/berrange/src/virt/qemu/build/qapi/qapi-commands-machine.c:1514: undefined reference to `qmp_x_query_usbhost'


IMHO the solution to this is to refactor the cdoe to split
hw/usb/host-libusb.c into two parts.

One part provides the monitor API impls, and some callbacks
for feeding data to them. The other part provides the actual
impl, and registers the callbacks needed by the monitor cmd.

> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  qapi/machine.json          | 18 ++++++++++++++++++
>  include/hw/usb.h           |  3 ---
>  hw/usb/bus-stub.c          |  7 ++++++-
>  hw/usb/host-libusb.c       | 38 +++++++++++++++++++++++++-------------
>  tests/qtest/qmp-cmd-test.c |  3 +++
>  5 files changed, 52 insertions(+), 17 deletions(-)
> 
> diff --git a/qapi/machine.json b/qapi/machine.json
> index 1283d14493..1b428f29d4 100644
> --- a/qapi/machine.json
> +++ b/qapi/machine.json
> @@ -1813,6 +1813,24 @@
>    'returns': 'HumanReadableText',
>    'features': [ 'unstable' ] }
>  
> +##
> +# @x-query-usbhost:
> +#
> +# Query information on host USB devices
> +#
> +# Features:
> +#
> +# @unstable: This command is meant for debugging.
> +#
> +# Returns: Host USB device information
> +#
> +# Since: 9.1
> +##
> +{ 'command': 'x-query-usbhost',
> +  'returns': 'HumanReadableText',
> +  'features': [ 'unstable' ],
> +  'if': 'CONFIG_USB_LIBUSB' }
> +
>  ##
>  # @SmbiosEntryPointType:
>  #
> diff --git a/include/hw/usb.h b/include/hw/usb.h
> index d46d96779a..c0b34af518 100644
> --- a/include/hw/usb.h
> +++ b/include/hw/usb.h
> @@ -465,9 +465,6 @@ void usb_device_reset(USBDevice *dev);
>  void usb_wakeup(USBEndpoint *ep, unsigned int stream);
>  void usb_generic_async_ctrl_complete(USBDevice *s, USBPacket *p);
>  
> -/* usb-linux.c */
> -void hmp_info_usbhost(Monitor *mon, const QDict *qdict);
> -
>  /* usb ports of the VM */
>  
>  #define VM_USB_HUB_SIZE 8
> diff --git a/hw/usb/bus-stub.c b/hw/usb/bus-stub.c
> index fcabe8429e..948429bb33 100644
> --- a/hw/usb/bus-stub.c
> +++ b/hw/usb/bus-stub.c
> @@ -11,7 +11,6 @@
>  #include "qapi/error.h"
>  #include "qapi/qapi-commands-machine.h"
>  #include "sysemu/sysemu.h"
> -#include "monitor/monitor.h"
>  #include "hw/usb.h"
>  
>  USBDevice *usbdevice_create(const char *driver)
> @@ -26,3 +25,9 @@ HumanReadableText *qmp_x_query_usb(Error **errp)
>      error_setg(errp, "Support for USB devices not built-in");
>      return NULL;
>  }
> +
> +HumanReadableText *qmp_x_query_usbhost(Error **errp)
> +{
> +    error_setg(errp, "Support for USB devices not built-in");
> +    return NULL;
> +}
> diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c
> index 80122b4125..5781d7fa7c 100644
> --- a/hw/usb/host-libusb.c
> +++ b/hw/usb/host-libusb.c
> @@ -46,6 +46,8 @@
>  #endif
>  
>  #include "qapi/error.h"
> +#include "qapi/qapi-commands-machine.h"
> +#include "qapi/type-helpers.h"
>  #include "migration/vmstate.h"
>  #include "monitor/monitor.h"
>  #include "qemu/error-report.h"
> @@ -1816,7 +1818,7 @@ module_kconfig(USB);
>  static void usb_host_register_types(void)
>  {
>      type_register_static(&usb_host_dev_info);
> -    monitor_register_hmp("usbhost", true, hmp_info_usbhost);
> +    monitor_register_hmp_info_hrt("usbhost", qmp_x_query_usbhost);
>  }
>  
>  type_init(usb_host_register_types)
> @@ -1921,18 +1923,25 @@ static void usb_host_auto_check(void *unused)
>      timer_mod(usb_auto_timer, qemu_clock_get_ms(QEMU_CLOCK_REALTIME) + 2000);
>  }
>  
> -void hmp_info_usbhost(Monitor *mon, const QDict *qdict)
> +HumanReadableText *qmp_x_query_usbhost(Error **errp)
>  {
> +    g_autoptr(GString) buf = g_string_new("");
>      libusb_device **devs = NULL;
>      struct libusb_device_descriptor ddesc;
>      char port[16];
>      int i, n;
>  
>      if (usb_host_init() != 0) {
> -        return;
> +        error_setg(errp, "Failed to init libusb");
> +        return NULL;
>      }
>  
>      n = libusb_get_device_list(ctx, &devs);
> +    if (!n) {
> +        error_setg(errp, "No host USB device");
> +        return NULL;
> +    }
> +
>      for (i = 0; i < n; i++) {
>          if (libusb_get_device_descriptor(devs[i], &ddesc) != 0) {
>              continue;
> @@ -1941,14 +1950,15 @@ void hmp_info_usbhost(Monitor *mon, const QDict *qdict)
>              continue;
>          }
>          usb_host_get_port(devs[i], port, sizeof(port));
> -        monitor_printf(mon, "  Bus %d, Addr %d, Port %s, Speed %s Mb/s\n",
> -                       libusb_get_bus_number(devs[i]),
> -                       libusb_get_device_address(devs[i]),
> -                       port,
> -                       speed_name[libusb_get_device_speed(devs[i])]);
> -        monitor_printf(mon, "    Class %02x:", ddesc.bDeviceClass);
> -        monitor_printf(mon, " USB device %04x:%04x",
> -                       ddesc.idVendor, ddesc.idProduct);
> +        g_string_append_printf(buf,
> +                               "  Bus %d, Addr %d, Port %s, Speed %s Mb/s\n",
> +                               libusb_get_bus_number(devs[i]),
> +                               libusb_get_device_address(devs[i]),
> +                               port,
> +                               speed_name[libusb_get_device_speed(devs[i])]);
> +        g_string_append_printf(buf, "    Class %02x:", ddesc.bDeviceClass);
> +        g_string_append_printf(buf, " USB device %04x:%04x",
> +                               ddesc.idVendor, ddesc.idProduct);
>          if (ddesc.iProduct) {
>              libusb_device_handle *handle;
>              if (libusb_open(devs[i], &handle) == 0) {
> @@ -1957,10 +1967,12 @@ void hmp_info_usbhost(Monitor *mon, const QDict *qdict)
>                                                     ddesc.iProduct,
>                                                     name, sizeof(name));
>                  libusb_close(handle);
> -                monitor_printf(mon, ", %s", name);
> +                g_string_append_printf(buf, ", %s", name);
>              }
>          }
> -        monitor_printf(mon, "\n");
> +        g_string_append_c(buf, '\n');
>      }
>      libusb_free_device_list(devs, 1);
> +
> +    return human_readable_text_from_str(buf);
>  }
> diff --git a/tests/qtest/qmp-cmd-test.c b/tests/qtest/qmp-cmd-test.c
> index 2c15f60958..731d3c6c59 100644
> --- a/tests/qtest/qmp-cmd-test.c
> +++ b/tests/qtest/qmp-cmd-test.c
> @@ -49,6 +49,9 @@ static int query_error_class(const char *cmd)
>          { "query-vm-generation-id", ERROR_CLASS_GENERIC_ERROR },
>          /* Only valid with a USB bus added */
>          { "x-query-usb", ERROR_CLASS_GENERIC_ERROR },
> +#ifdef CONFIG_USB_LIBUSB
> +        { "x-query-usbhost", ERROR_CLASS_GENERIC_ERROR },
> +#endif
>          /* Only valid with accel=tcg */
>          { "x-query-jit", ERROR_CLASS_GENERIC_ERROR },
>          { "x-query-opcount", ERROR_CLASS_GENERIC_ERROR },
> -- 
> 2.41.0
> 

With regards,
Daniel
Philippe Mathieu-Daudé June 11, 2024, 9:59 a.m. UTC | #2 
On 10/6/24 10:33, Daniel P. Berrangé wrote:
> On Mon, Jun 10, 2024 at 08:38:47AM +0200, Philippe Mathieu-Daudé wrote:
>> This is a counterpart to the HMP "info usbhost" 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.
> 
> 'info usb' host is one of the problem scenarios i mentioned in
> 
> https://lore.kernel.org/qemu-devel/20211028155457.967291-1-berrange@redhat.com/
> 
> Its HMP impl is within a dynamically loadable module. So if you
> run it initially you'll get
> 
> (qemu) info usbhost
> Command "info usbhost" is not available.
> 
> 
> but if you have a usb host on the cli:
> 
> (qemu) info usbhost
>    Bus 3, Addr 6, Port 8, Speed 480 Mb/s
>      Class ef: USB device 04f2:b74f
>    Bus 3, Addr 11, Port 7.3, Speed 1.5 Mb/s
>    ...snip...
> 
> 
> Anyway, the end result is that this patch fails to link when modules
> are enabled:
> 
> cc -m64 @qemu-system-x86_64.rsp
> /usr/bin/ld: libqemuutil.a.p/meson-generated_.._qapi_qapi-commands-machine.c.o: in function `qmp_marshal_x_query_usbhost':
> /var/home/berrange/src/virt/qemu/build/qapi/qapi-commands-machine.c:1514: undefined reference to `qmp_x_query_usbhost'

Indeed I missed that.

> IMHO the solution to this is to refactor the cdoe to split
> hw/usb/host-libusb.c into two parts.
> 
> One part provides the monitor API impls, and some callbacks
> for feeding data to them. The other part provides the actual
> impl, and registers the callbacks needed by the monitor cmd.

Yeah something like that will do.
diff mbox series

Patch

diff --git a/qapi/machine.json b/qapi/machine.json
index 1283d14493..1b428f29d4 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -1813,6 +1813,24 @@ 
   'returns': 'HumanReadableText',
   'features': [ 'unstable' ] }
 
+##
+# @x-query-usbhost:
+#
+# Query information on host USB devices
+#
+# Features:
+#
+# @unstable: This command is meant for debugging.
+#
+# Returns: Host USB device information
+#
+# Since: 9.1
+##
+{ 'command': 'x-query-usbhost',
+  'returns': 'HumanReadableText',
+  'features': [ 'unstable' ],
+  'if': 'CONFIG_USB_LIBUSB' }
+
 ##
 # @SmbiosEntryPointType:
 #
diff --git a/include/hw/usb.h b/include/hw/usb.h
index d46d96779a..c0b34af518 100644
--- a/include/hw/usb.h
+++ b/include/hw/usb.h
@@ -465,9 +465,6 @@  void usb_device_reset(USBDevice *dev);
 void usb_wakeup(USBEndpoint *ep, unsigned int stream);
 void usb_generic_async_ctrl_complete(USBDevice *s, USBPacket *p);
 
-/* usb-linux.c */
-void hmp_info_usbhost(Monitor *mon, const QDict *qdict);
-
 /* usb ports of the VM */
 
 #define VM_USB_HUB_SIZE 8
diff --git a/hw/usb/bus-stub.c b/hw/usb/bus-stub.c
index fcabe8429e..948429bb33 100644
--- a/hw/usb/bus-stub.c
+++ b/hw/usb/bus-stub.c
@@ -11,7 +11,6 @@ 
 #include "qapi/error.h"
 #include "qapi/qapi-commands-machine.h"
 #include "sysemu/sysemu.h"
-#include "monitor/monitor.h"
 #include "hw/usb.h"
 
 USBDevice *usbdevice_create(const char *driver)
@@ -26,3 +25,9 @@  HumanReadableText *qmp_x_query_usb(Error **errp)
     error_setg(errp, "Support for USB devices not built-in");
     return NULL;
 }
+
+HumanReadableText *qmp_x_query_usbhost(Error **errp)
+{
+    error_setg(errp, "Support for USB devices not built-in");
+    return NULL;
+}
diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c
index 80122b4125..5781d7fa7c 100644
--- a/hw/usb/host-libusb.c
+++ b/hw/usb/host-libusb.c
@@ -46,6 +46,8 @@ 
 #endif
 
 #include "qapi/error.h"
+#include "qapi/qapi-commands-machine.h"
+#include "qapi/type-helpers.h"
 #include "migration/vmstate.h"
 #include "monitor/monitor.h"
 #include "qemu/error-report.h"
@@ -1816,7 +1818,7 @@  module_kconfig(USB);
 static void usb_host_register_types(void)
 {
     type_register_static(&usb_host_dev_info);
-    monitor_register_hmp("usbhost", true, hmp_info_usbhost);
+    monitor_register_hmp_info_hrt("usbhost", qmp_x_query_usbhost);
 }
 
 type_init(usb_host_register_types)
@@ -1921,18 +1923,25 @@  static void usb_host_auto_check(void *unused)
     timer_mod(usb_auto_timer, qemu_clock_get_ms(QEMU_CLOCK_REALTIME) + 2000);
 }
 
-void hmp_info_usbhost(Monitor *mon, const QDict *qdict)
+HumanReadableText *qmp_x_query_usbhost(Error **errp)
 {
+    g_autoptr(GString) buf = g_string_new("");
     libusb_device **devs = NULL;
     struct libusb_device_descriptor ddesc;
     char port[16];
     int i, n;
 
     if (usb_host_init() != 0) {
-        return;
+        error_setg(errp, "Failed to init libusb");
+        return NULL;
     }
 
     n = libusb_get_device_list(ctx, &devs);
+    if (!n) {
+        error_setg(errp, "No host USB device");
+        return NULL;
+    }
+
     for (i = 0; i < n; i++) {
         if (libusb_get_device_descriptor(devs[i], &ddesc) != 0) {
             continue;
@@ -1941,14 +1950,15 @@  void hmp_info_usbhost(Monitor *mon, const QDict *qdict)
             continue;
         }
         usb_host_get_port(devs[i], port, sizeof(port));
-        monitor_printf(mon, "  Bus %d, Addr %d, Port %s, Speed %s Mb/s\n",
-                       libusb_get_bus_number(devs[i]),
-                       libusb_get_device_address(devs[i]),
-                       port,
-                       speed_name[libusb_get_device_speed(devs[i])]);
-        monitor_printf(mon, "    Class %02x:", ddesc.bDeviceClass);
-        monitor_printf(mon, " USB device %04x:%04x",
-                       ddesc.idVendor, ddesc.idProduct);
+        g_string_append_printf(buf,
+                               "  Bus %d, Addr %d, Port %s, Speed %s Mb/s\n",
+                               libusb_get_bus_number(devs[i]),
+                               libusb_get_device_address(devs[i]),
+                               port,
+                               speed_name[libusb_get_device_speed(devs[i])]);
+        g_string_append_printf(buf, "    Class %02x:", ddesc.bDeviceClass);
+        g_string_append_printf(buf, " USB device %04x:%04x",
+                               ddesc.idVendor, ddesc.idProduct);
         if (ddesc.iProduct) {
             libusb_device_handle *handle;
             if (libusb_open(devs[i], &handle) == 0) {
@@ -1957,10 +1967,12 @@  void hmp_info_usbhost(Monitor *mon, const QDict *qdict)
                                                    ddesc.iProduct,
                                                    name, sizeof(name));
                 libusb_close(handle);
-                monitor_printf(mon, ", %s", name);
+                g_string_append_printf(buf, ", %s", name);
             }
         }
-        monitor_printf(mon, "\n");
+        g_string_append_c(buf, '\n');
     }
     libusb_free_device_list(devs, 1);
+
+    return human_readable_text_from_str(buf);
 }
diff --git a/tests/qtest/qmp-cmd-test.c b/tests/qtest/qmp-cmd-test.c
index 2c15f60958..731d3c6c59 100644
--- a/tests/qtest/qmp-cmd-test.c
+++ b/tests/qtest/qmp-cmd-test.c
@@ -49,6 +49,9 @@  static int query_error_class(const char *cmd)
         { "query-vm-generation-id", ERROR_CLASS_GENERIC_ERROR },
         /* Only valid with a USB bus added */
         { "x-query-usb", ERROR_CLASS_GENERIC_ERROR },
+#ifdef CONFIG_USB_LIBUSB
+        { "x-query-usbhost", ERROR_CLASS_GENERIC_ERROR },
+#endif
         /* Only valid with accel=tcg */
         { "x-query-jit", ERROR_CLASS_GENERIC_ERROR },
         { "x-query-opcount", ERROR_CLASS_GENERIC_ERROR },