diff mbox series

[v2,2/3] hw/usb: Introduce x-query-usbhost QMP command

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

Commit Message

Philippe Mathieu-Daudé June 11, 2024, 10:23 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.

Since host-libusb.c can be built as part of the 'hw-usb' module,
we introduce the libusb_register_hmp_info_hrt() helper to allow late
registration when the module is loaded.

Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 qapi/machine.json           | 18 ++++++++++++++++
 hw/usb/host-libusb.h        | 16 ++++++++++++++
 include/hw/usb.h            |  3 ---
 hw/usb/bus-stub.c           |  7 +++++-
 hw/usb/host-libusb-common.c | 31 ++++++++++++++++++++++++++
 hw/usb/host-libusb.c        | 43 +++++++++++++++++++++++++------------
 tests/qtest/qmp-cmd-test.c  |  3 +++
 hmp-commands-info.hx        |  2 ++
 hw/usb/meson.build          |  1 +
 9 files changed, 106 insertions(+), 18 deletions(-)
 create mode 100644 hw/usb/host-libusb.h
 create mode 100644 hw/usb/host-libusb-common.c

Comments

Daniel P. Berrangé June 11, 2024, 12:28 p.m. UTC | #1 
On Tue, Jun 11, 2024 at 12:23:04PM +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.
> 
> Since host-libusb.c can be built as part of the 'hw-usb' module,
> we introduce the libusb_register_hmp_info_hrt() helper to allow late
> registration when the module is loaded.
> 
> Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  qapi/machine.json           | 18 ++++++++++++++++
>  hw/usb/host-libusb.h        | 16 ++++++++++++++
>  include/hw/usb.h            |  3 ---
>  hw/usb/bus-stub.c           |  7 +++++-
>  hw/usb/host-libusb-common.c | 31 ++++++++++++++++++++++++++
>  hw/usb/host-libusb.c        | 43 +++++++++++++++++++++++++------------
>  tests/qtest/qmp-cmd-test.c  |  3 +++
>  hmp-commands-info.hx        |  2 ++
>  hw/usb/meson.build          |  1 +
>  9 files changed, 106 insertions(+), 18 deletions(-)
>  create mode 100644 hw/usb/host-libusb.h
>  create mode 100644 hw/usb/host-libusb-common.c

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

With regards,
Daniel
Philippe Mathieu-Daudé June 18, 2024, 2:15 p.m. UTC | #2 
On 11/6/24 12:23, 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.
> 
> Since host-libusb.c can be built as part of the 'hw-usb' module,
> we introduce the libusb_register_hmp_info_hrt() helper to allow late
> registration when the module is loaded.
> 
> Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   qapi/machine.json           | 18 ++++++++++++++++
>   hw/usb/host-libusb.h        | 16 ++++++++++++++
>   include/hw/usb.h            |  3 ---
>   hw/usb/bus-stub.c           |  7 +++++-
>   hw/usb/host-libusb-common.c | 31 ++++++++++++++++++++++++++
>   hw/usb/host-libusb.c        | 43 +++++++++++++++++++++++++------------
>   tests/qtest/qmp-cmd-test.c  |  3 +++
>   hmp-commands-info.hx        |  2 ++
>   hw/usb/meson.build          |  1 +
>   9 files changed, 106 insertions(+), 18 deletions(-)
>   create mode 100644 hw/usb/host-libusb.h
>   create mode 100644 hw/usb/host-libusb-common.c


> 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-common.c b/hw/usb/host-libusb-common.c
> new file mode 100644
> index 0000000000..406a2b25be
> --- /dev/null
> +++ b/hw/usb/host-libusb-common.c
> @@ -0,0 +1,31 @@
> +/*
> + * QEMU USB host redirector helpers
> + *
> + * SPDX-FileContributor: Philippe Mathieu-Daudé <philmd@linaro.org>
> + * SPDX-FileCopyrightText: 2024 Linaro Ltd.
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/module.h"
> +#include "qapi/error.h"
> +#include "qapi/qapi-commands-machine.h"
> +#include "monitor/monitor.h"
> +#include "host-libusb.h"
> +
> +static HumanReadableText *(*qmp_x_query_usbhost_handler)(Error **errp);
> +
> +void libusb_register_hmp_info_hrt(HumanReadableText *(*handler)(Error **errp))
> +{
> +    qmp_x_query_usbhost_handler = handler;
> +    monitor_register_hmp_info_hrt("usbhost", handler);
> +}
> +
> +HumanReadableText *qmp_x_query_usbhost(Error **errp)
> +{
> +    if (module_load("hw-usb-", "host", errp) <= 0) {
> +        return NULL;
> +    }
> +    assert(qmp_x_query_usbhost_handler);
> +    return qmp_x_query_usbhost_handler(errp);
> +}



> diff --git a/hw/usb/meson.build b/hw/usb/meson.build
> index d7de1003e3..af92b504fd 100644
> --- a/hw/usb/meson.build
> +++ b/hw/usb/meson.build
> @@ -7,6 +7,7 @@ system_ss.add(when: 'CONFIG_USB', if_true: files(
>     'core.c',
>     'desc.c',
>     'desc-msos.c',
> +  'host-libusb-common.c',
>     'libhw.c',
>     'pcap.c',
>   ), if_false: files('bus-stub.c'))

Since this files depends on libusb, squashing:

-- >8 --
diff --git a/hw/usb/meson.build b/hw/usb/meson.build
index af92b504fd..89b197fbd8 100644
--- a/hw/usb/meson.build
+++ b/hw/usb/meson.build
@@ -9,3 +9,2 @@ system_ss.add(when: 'CONFIG_USB', if_true: files(
    'desc-msos.c',
-  'host-libusb-common.c',
    'libhw.c',
@@ -86,2 +85,3 @@ endif
  if libusb.found()
+  system_ss.add(when: 'CONFIG_USB', if_true: files('host-libusb-common.c'))
    usbhost_ss = ss.source_set()
---
Philippe Mathieu-Daudé June 18, 2024, 3:33 p.m. UTC | #3 
On 18/6/24 16:15, Philippe Mathieu-Daudé wrote:
> On 11/6/24 12:23, 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.
>>
>> Since host-libusb.c can be built as part of the 'hw-usb' module,
>> we introduce the libusb_register_hmp_info_hrt() helper to allow late
>> registration when the module is loaded.
>>
>> Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   qapi/machine.json           | 18 ++++++++++++++++
>>   hw/usb/host-libusb.h        | 16 ++++++++++++++
>>   include/hw/usb.h            |  3 ---
>>   hw/usb/bus-stub.c           |  7 +++++-
>>   hw/usb/host-libusb-common.c | 31 ++++++++++++++++++++++++++
>>   hw/usb/host-libusb.c        | 43 +++++++++++++++++++++++++------------
>>   tests/qtest/qmp-cmd-test.c  |  3 +++
>>   hmp-commands-info.hx        |  2 ++
>>   hw/usb/meson.build          |  1 +
>>   9 files changed, 106 insertions(+), 18 deletions(-)
>>   create mode 100644 hw/usb/host-libusb.h
>>   create mode 100644 hw/usb/host-libusb-common.c


>> 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;
>>   }

Also missing:

    #ifdef CONFIG_USB_LIBUSB

>> +HumanReadableText *qmp_x_query_usbhost(Error **errp)
>> +{
>> +    error_setg(errp, "Support for USB devices not built-in");
>> +    return NULL;
>> +}

    #endif

But still failing the build-without-defaults job:

▶ 59/61 /x86_64/qmp/x-query-usbhost - 
ERROR:../tests/qtest/qmp-cmd-test.c:88:test_query: assertion failed: 
(error) FAIL

So dropping patches 2 & 3 for now.
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/hw/usb/host-libusb.h b/hw/usb/host-libusb.h
new file mode 100644
index 0000000000..eaada081a5
--- /dev/null
+++ b/hw/usb/host-libusb.h
@@ -0,0 +1,16 @@ 
+/*
+ * QEMU USB host redirector
+ *
+ * SPDX-FileContributor: Philippe Mathieu-Daudé <philmd@linaro.org>
+ * SPDX-FileCopyrightText: 2024 Linaro Ltd.
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#ifndef HW_USB_HOST_LIBUSB_H
+#define HW_USB_HOST_LIBUSB_H
+
+#include "qapi/qapi-types-common.h"
+
+void libusb_register_hmp_info_hrt(HumanReadableText *(*handler)(Error **errp));
+
+#endif
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-common.c b/hw/usb/host-libusb-common.c
new file mode 100644
index 0000000000..406a2b25be
--- /dev/null
+++ b/hw/usb/host-libusb-common.c
@@ -0,0 +1,31 @@ 
+/*
+ * QEMU USB host redirector helpers
+ *
+ * SPDX-FileContributor: Philippe Mathieu-Daudé <philmd@linaro.org>
+ * SPDX-FileCopyrightText: 2024 Linaro Ltd.
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/module.h"
+#include "qapi/error.h"
+#include "qapi/qapi-commands-machine.h"
+#include "monitor/monitor.h"
+#include "host-libusb.h"
+
+static HumanReadableText *(*qmp_x_query_usbhost_handler)(Error **errp);
+
+void libusb_register_hmp_info_hrt(HumanReadableText *(*handler)(Error **errp))
+{
+    qmp_x_query_usbhost_handler = handler;
+    monitor_register_hmp_info_hrt("usbhost", handler);
+}
+
+HumanReadableText *qmp_x_query_usbhost(Error **errp)
+{
+    if (module_load("hw-usb-", "host", errp) <= 0) {
+        return NULL;
+    }
+    assert(qmp_x_query_usbhost_handler);
+    return qmp_x_query_usbhost_handler(errp);
+}
diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c
index 80122b4125..b3e1443794 100644
--- a/hw/usb/host-libusb.c
+++ b/hw/usb/host-libusb.c
@@ -1,5 +1,5 @@ 
 /*
- * Linux host USB redirector
+ * QEMU USB host redirector
  *
  * Copyright (c) 2005 Fabrice Bellard
  *
@@ -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"
@@ -57,6 +59,7 @@ 
 
 #include "hw/qdev-properties.h"
 #include "hw/usb.h"
+#include "host-libusb.h"
 
 /* ------------------------------------------------------------------------ */
 
@@ -1813,10 +1816,12 @@  static const TypeInfo usb_host_dev_info = {
 module_obj(TYPE_USB_HOST_DEVICE);
 module_kconfig(USB);
 
+static HumanReadableText *qmp_mod_query_usbhost(Error **errp);
+
 static void usb_host_register_types(void)
 {
     type_register_static(&usb_host_dev_info);
-    monitor_register_hmp("usbhost", true, hmp_info_usbhost);
+    libusb_register_hmp_info_hrt(qmp_mod_query_usbhost);
 }
 
 type_init(usb_host_register_types)
@@ -1921,18 +1926,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)
+static HumanReadableText *qmp_mod_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 +1953,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 +1970,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 },
diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
index cfd4ad5651..134f970584 100644
--- a/hmp-commands-info.hx
+++ b/hmp-commands-info.hx
@@ -335,12 +335,14 @@  SRST
     Show guest USB devices.
 ERST
 
+#ifdef CONFIG_USB_LIBUSB
     {
         .name       = "usbhost",
         .args_type  = "",
         .params     = "",
         .help       = "show host USB devices",
     },
+#endif
 
 SRST
   ``info usbhost``
diff --git a/hw/usb/meson.build b/hw/usb/meson.build
index d7de1003e3..af92b504fd 100644
--- a/hw/usb/meson.build
+++ b/hw/usb/meson.build
@@ -7,6 +7,7 @@  system_ss.add(when: 'CONFIG_USB', if_true: files(
   'core.c',
   'desc.c',
   'desc-msos.c',
+  'host-libusb-common.c',
   'libhw.c',
   'pcap.c',
 ), if_false: files('bus-stub.c'))