[Qemu-devel] [Patch V1 0/3] usb, xen: add pvUSB backend

[Qemu-devel] [Patch V1 0/3] usb, xen: add pvUSB backend

[Juergen Gross]

This series adds a Xen pvUSB backend driver to qemu. USB devices
connected to the host can be passed through to a Xen guest. The
devices are specified via Xenstore. Access to the devices is done
via host-libusb.c

I've tested the backend with various USB devices (memory sticks,
keyboard, phono preamp, ...).

Changes since RFC:
- open device via qdev_device_add(), making patch 1 obsolete
- modify patch 2 to use isoc passthrough via libusb instead of one
  job per frame

Juergen Gross (3):
  xen: introduce dummy system device
  xen/usb: add capability for passing through isoc jobs to host devices
  xen: add pvUSB backend

 hw/usb/Makefile.objs         |    4 +
 hw/usb/core.c                |   11 +-
 hw/usb/host-libusb.c         |   51 +-
 hw/usb/xen-usb.c             | 1120 ++++++++++++++++++++++++++++++++++++++++++
 hw/xenpv/xen_machine_pv.c    |   42 ++
 include/hw/xen/xen_backend.h |   11 +
 6 files changed, 1229 insertions(+), 10 deletions(-)
 create mode 100644 hw/usb/xen-usb.c

-- 
2.1.4

[Qemu-devel] [Patch V1 1/3] xen: introduce dummy system device

[Juergen Gross]

Introduce a new dummy system device serving as parent for virtual
buses. This will enable new pv backends to introduce virtual buses
which are removable again opposed to system buses which are meant
to stay once added.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 hw/xenpv/xen_machine_pv.c    | 39 +++++++++++++++++++++++++++++++++++++++
 include/hw/xen/xen_backend.h |  1 +
 2 files changed, 40 insertions(+)

diff --git a/hw/xenpv/xen_machine_pv.c b/hw/xenpv/xen_machine_pv.c
index 2e545d2..57bc071 100644
--- a/hw/xenpv/xen_machine_pv.c
+++ b/hw/xenpv/xen_machine_pv.c
@@ -24,10 +24,15 @@
 
 #include "hw/hw.h"
 #include "hw/boards.h"
+#include "hw/sysbus.h"
 #include "hw/xen/xen_backend.h"
 #include "xen_domainbuild.h"
 #include "sysemu/block-backend.h"
 
+#define TYPE_XENSYSDEV "xensysdev"
+
+DeviceState *xen_sysdev;
+
 static void xen_init_pv(MachineState *machine)
 {
     const char *kernel_filename = machine->kernel_filename;
@@ -59,6 +64,9 @@ static void xen_init_pv(MachineState *machine)
         break;
     }
 
+    xen_sysdev = qdev_create(NULL, TYPE_XENSYSDEV);
+    qdev_init_nofail(xen_sysdev);
+
     xen_be_register("console", &xen_console_ops);
     xen_be_register("vkbd", &xen_kbdmouse_ops);
     xen_be_register("vfb", &xen_framebuffer_ops);
@@ -93,6 +101,31 @@ static void xen_init_pv(MachineState *machine)
     xen_init_display(xen_domid);
 }
 
+static int xen_sysdev_init(SysBusDevice *dev)
+{
+    return 0;
+}
+
+static Property xen_sysdev_properties[] = {
+    {/* end of property list */},
+};
+
+static void xen_sysdev_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
+
+    k->init = xen_sysdev_init;
+    dc->props = xen_sysdev_properties;
+}
+
+static const TypeInfo xensysdev_info = {
+    .name          = TYPE_XENSYSDEV,
+    .parent        = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(SysBusDevice),
+    .class_init    = xen_sysdev_class_init,
+};
+
 static QEMUMachine xenpv_machine = {
     .name = "xenpv",
     .desc = "Xen Para-virtualized PC",
@@ -101,9 +134,15 @@ static QEMUMachine xenpv_machine = {
     .default_machine_opts = "accel=xen",
 };
 
+static void xenpv_register_types(void)
+{
+    type_register_static(&xensysdev_info);
+}
+
 static void xenpv_machine_init(void)
 {
     qemu_register_machine(&xenpv_machine);
 }
 
+type_init(xenpv_register_types)
 machine_init(xenpv_machine_init);
diff --git a/include/hw/xen/xen_backend.h b/include/hw/xen/xen_backend.h
index 3b4125e..911ba6d 100644
--- a/include/hw/xen/xen_backend.h
+++ b/include/hw/xen/xen_backend.h
@@ -59,6 +59,7 @@ struct XenDevice {
 extern XenXC xen_xc;
 extern struct xs_handle *xenstore;
 extern const char *xen_protocol;
+extern DeviceState *xen_sysdev;
 
 /* xenstore helper functions */
 int xenstore_write_str(const char *base, const char *node, const char *val);
-- 
2.1.4

Re: [Qemu-devel] [Patch V1 1/3] xen: introduce dummy system device

[Stefano Stabellini]

On Thu, 3 Sep 2015, Juergen Gross wrote:
> Introduce a new dummy system device serving as parent for virtual
> buses. This will enable new pv backends to introduce virtual buses
> which are removable again opposed to system buses which are meant
> to stay once added.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
>  hw/xenpv/xen_machine_pv.c    | 39 +++++++++++++++++++++++++++++++++++++++
>  include/hw/xen/xen_backend.h |  1 +
>  2 files changed, 40 insertions(+)
> 
> diff --git a/hw/xenpv/xen_machine_pv.c b/hw/xenpv/xen_machine_pv.c
> index 2e545d2..57bc071 100644
> --- a/hw/xenpv/xen_machine_pv.c
> +++ b/hw/xenpv/xen_machine_pv.c
> @@ -24,10 +24,15 @@
>  
>  #include "hw/hw.h"
>  #include "hw/boards.h"
> +#include "hw/sysbus.h"
>  #include "hw/xen/xen_backend.h"
>  #include "xen_domainbuild.h"
>  #include "sysemu/block-backend.h"
>  
> +#define TYPE_XENSYSDEV "xensysdev"
> +
> +DeviceState *xen_sysdev;
> +
>  static void xen_init_pv(MachineState *machine)
>  {
>      const char *kernel_filename = machine->kernel_filename;
> @@ -59,6 +64,9 @@ static void xen_init_pv(MachineState *machine)
>          break;
>      }
>  
> +    xen_sysdev = qdev_create(NULL, TYPE_XENSYSDEV);
> +    qdev_init_nofail(xen_sysdev);
> +
>      xen_be_register("console", &xen_console_ops);
>      xen_be_register("vkbd", &xen_kbdmouse_ops);
>      xen_be_register("vfb", &xen_framebuffer_ops);
> @@ -93,6 +101,31 @@ static void xen_init_pv(MachineState *machine)
>      xen_init_display(xen_domid);
>  }
>  
> +static int xen_sysdev_init(SysBusDevice *dev)
> +{
> +    return 0;
> +}
> +
> +static Property xen_sysdev_properties[] = {
> +    {/* end of property list */},
> +};
> +
> +static void xen_sysdev_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
> +
> +    k->init = xen_sysdev_init;
> +    dc->props = xen_sysdev_properties;
> +}
> +
> +static const TypeInfo xensysdev_info = {
> +    .name          = TYPE_XENSYSDEV,
> +    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(SysBusDevice),
> +    .class_init    = xen_sysdev_class_init,
> +};
> +
>  static QEMUMachine xenpv_machine = {
>      .name = "xenpv",
>      .desc = "Xen Para-virtualized PC",
> @@ -101,9 +134,15 @@ static QEMUMachine xenpv_machine = {
>      .default_machine_opts = "accel=xen",
>  };
>  
> +static void xenpv_register_types(void)
> +{
> +    type_register_static(&xensysdev_info);
> +}

Given that you need this just for usbback, I wonder if you could
move xen_sysdev and its initalization to usbback_init.

Re: [Qemu-devel] [Xen-devel] [Patch V1 1/3] xen: introduce dummy system device

[Juergen Gross]

On 09/07/2015 05:29 PM, Stefano Stabellini wrote:
> On Thu, 3 Sep 2015, Juergen Gross wrote:
>> Introduce a new dummy system device serving as parent for virtual
>> buses. This will enable new pv backends to introduce virtual buses
>> which are removable again opposed to system buses which are meant
>> to stay once added.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>>   hw/xenpv/xen_machine_pv.c    | 39 +++++++++++++++++++++++++++++++++++++++
>>   include/hw/xen/xen_backend.h |  1 +
>>   2 files changed, 40 insertions(+)
>>
>> diff --git a/hw/xenpv/xen_machine_pv.c b/hw/xenpv/xen_machine_pv.c
>> index 2e545d2..57bc071 100644
>> --- a/hw/xenpv/xen_machine_pv.c
>> +++ b/hw/xenpv/xen_machine_pv.c
>> @@ -24,10 +24,15 @@
>>
>>   #include "hw/hw.h"
>>   #include "hw/boards.h"
>> +#include "hw/sysbus.h"
>>   #include "hw/xen/xen_backend.h"
>>   #include "xen_domainbuild.h"
>>   #include "sysemu/block-backend.h"
>>
>> +#define TYPE_XENSYSDEV "xensysdev"
>> +
>> +DeviceState *xen_sysdev;
>> +
>>   static void xen_init_pv(MachineState *machine)
>>   {
>>       const char *kernel_filename = machine->kernel_filename;
>> @@ -59,6 +64,9 @@ static void xen_init_pv(MachineState *machine)
>>           break;
>>       }
>>
>> +    xen_sysdev = qdev_create(NULL, TYPE_XENSYSDEV);
>> +    qdev_init_nofail(xen_sysdev);
>> +
>>       xen_be_register("console", &xen_console_ops);
>>       xen_be_register("vkbd", &xen_kbdmouse_ops);
>>       xen_be_register("vfb", &xen_framebuffer_ops);
>> @@ -93,6 +101,31 @@ static void xen_init_pv(MachineState *machine)
>>       xen_init_display(xen_domid);
>>   }
>>
>> +static int xen_sysdev_init(SysBusDevice *dev)
>> +{
>> +    return 0;
>> +}
>> +
>> +static Property xen_sysdev_properties[] = {
>> +    {/* end of property list */},
>> +};
>> +
>> +static void xen_sysdev_class_init(ObjectClass *klass, void *data)
>> +{
>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>> +    SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
>> +
>> +    k->init = xen_sysdev_init;
>> +    dc->props = xen_sysdev_properties;
>> +}
>> +
>> +static const TypeInfo xensysdev_info = {
>> +    .name          = TYPE_XENSYSDEV,
>> +    .parent        = TYPE_SYS_BUS_DEVICE,
>> +    .instance_size = sizeof(SysBusDevice),
>> +    .class_init    = xen_sysdev_class_init,
>> +};
>> +
>>   static QEMUMachine xenpv_machine = {
>>       .name = "xenpv",
>>       .desc = "Xen Para-virtualized PC",
>> @@ -101,9 +134,15 @@ static QEMUMachine xenpv_machine = {
>>       .default_machine_opts = "accel=xen",
>>   };
>>
>> +static void xenpv_register_types(void)
>> +{
>> +    type_register_static(&xensysdev_info);
>> +}
>
> Given that you need this just for usbback, I wonder if you could
> move xen_sysdev and its initalization to usbback_init.

Sure I could. OTOH I think this system device could really be of common
interest.


Juergen

[Qemu-devel] [Patch V1 2/3] xen/usb: add capability for passing through isoc jobs to host devices

[Juergen Gross]

When Xen is using the qemu usb framework for pure passthrough of I/Os
to host devices the handling of isoc jobs is rather complicated if
multiple isoc frames are transferred with one call.

Instead of calling the framework with each frame individually, using
timers to avoid polling in a loop and sampling all responses to
construct a sum response for the user, just add a capability to
use the libusb isoc framework instead. This capability is selected
via a device specific property.

When the property is selected the host usb driver will use xen specific
callbacks to signal the end of isoc I/Os. For now these callbacks will
just be nops, they'll be filled with sensible actions when the xen
pv-usb backend is being added.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 hw/usb/core.c                | 11 ++++++----
 hw/usb/host-libusb.c         | 51 ++++++++++++++++++++++++++++++++++++++------
 include/hw/xen/xen_backend.h |  3 +++
 3 files changed, 55 insertions(+), 10 deletions(-)

diff --git a/hw/usb/core.c b/hw/usb/core.c
index cf34755..ed2255c 100644
--- a/hw/usb/core.c
+++ b/hw/usb/core.c
@@ -427,10 +427,13 @@ void usb_handle_packet(USBDevice *dev, USBPacket *p)
     if (QTAILQ_EMPTY(&p->ep->queue) || p->ep->pipeline || p->stream) {
         usb_process_one(p);
         if (p->status == USB_RET_ASYNC) {
-            /* hcd drivers cannot handle async for isoc */
-            assert(p->ep->type != USB_ENDPOINT_XFER_ISOC);
-            /* using async for interrupt packets breaks migration */
-            assert(p->ep->type != USB_ENDPOINT_XFER_INT ||
+            /*
+             * hcd drivers cannot handle async for isoc.
+             * Using async for interrupt packets breaks migration.
+             * Host devices are okay in any case.
+             */
+            assert((p->ep->type != USB_ENDPOINT_XFER_ISOC &&
+                    p->ep->type != USB_ENDPOINT_XFER_INT) ||
                    (dev->flags & (1 << USB_DEV_FLAG_IS_HOST)));
             usb_packet_set_state(p, USB_PACKET_ASYNC);
             QTAILQ_INSERT_TAIL(&p->ep->queue, p, queue);
diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c
index a5f9dab..ce644c3 100644
--- a/hw/usb/host-libusb.c
+++ b/hw/usb/host-libusb.c
@@ -42,6 +42,7 @@
 #include "trace.h"
 
 #include "hw/usb.h"
+#include "hw/xen/xen_backend.h"
 
 /* ------------------------------------------------------------------------ */
 
@@ -64,6 +65,7 @@ struct USBAutoFilter {
 
 enum USBHostDeviceOptions {
     USB_HOST_OPT_PIPELINE,
+    USB_HOST_XEN_ISO_PASSTHROUGH,
 };
 
 struct USBHostDevice {
@@ -152,6 +154,7 @@ static void usb_host_attach_kernel(USBHostDevice *s);
 #define CONTROL_TIMEOUT  10000        /* 10 sec    */
 #define BULK_TIMEOUT         0        /* unlimited */
 #define INTR_TIMEOUT         0        /* unlimited */
+#define ISO_TIMEOUT          0        /* unlimited */
 
 #if LIBUSBX_API_VERSION >= 0x01000103
 # define HAVE_STREAMS 1
@@ -306,14 +309,14 @@ static bool usb_host_use_combining(USBEndpoint *ep)
 /* ------------------------------------------------------------------------ */
 
 static USBHostRequest *usb_host_req_alloc(USBHostDevice *s, USBPacket *p,
-                                          bool in, size_t bufsize)
+                                          bool in, size_t bufsize, int packets)
 {
     USBHostRequest *r = g_new0(USBHostRequest, 1);
 
     r->host = s;
     r->p = p;
     r->in = in;
-    r->xfer = libusb_alloc_transfer(0);
+    r->xfer = libusb_alloc_transfer(packets);
     if (bufsize) {
         r->buffer = g_malloc(bufsize);
     }
@@ -376,6 +379,29 @@ out:
     }
 }
 
+static void usb_host_req_complete_iso_xen(struct libusb_transfer *xfer)
+{
+    USBHostRequest *r = xfer->user_data;
+    USBHostDevice  *s = r->host;
+    bool disconnect = (xfer->status == LIBUSB_TRANSFER_NO_DEVICE);
+
+    if (r->p == NULL) {
+        goto out; /* request was canceled */
+    }
+
+    r->p->status = status_map[xfer->status];
+    trace_usb_host_req_complete(s->bus_num, s->addr, r->p,
+                                r->p->status, r->p->actual_length);
+    /* copying of buffer is done in complete callback of xen */
+    usb_packet_complete(USB_DEVICE(s), r->p);
+
+out:
+    usb_host_req_free(r);
+    if (disconnect) {
+        usb_host_nodev(s);
+    }
+}
+
 static void usb_host_req_complete_data(struct libusb_transfer *xfer)
 {
     USBHostRequest *r = xfer->user_data;
@@ -1226,7 +1252,7 @@ static void usb_host_handle_control(USBDevice *udev, USBPacket *p,
         }
     }
 
-    r = usb_host_req_alloc(s, p, (request >> 8) & USB_DIR_IN, length + 8);
+    r = usb_host_req_alloc(s, p, (request >> 8) & USB_DIR_IN, length + 8, 0);
     r->cbuf = data;
     r->clen = length;
     memcpy(r->buffer, udev->setup_buf, 8);
@@ -1264,7 +1290,7 @@ static void usb_host_handle_data(USBDevice *udev, USBPacket *p)
     USBHostDevice *s = USB_HOST_DEVICE(udev);
     USBHostRequest *r;
     size_t size;
-    int ep, rc;
+    int ep, rc, packets;
 
     if (usb_host_use_combining(p->ep) && p->state == USB_PACKET_SETUP) {
         p->status = USB_RET_ADD_TO_QUEUE;
@@ -1289,7 +1315,7 @@ static void usb_host_handle_data(USBDevice *udev, USBPacket *p)
     switch (usb_ep_get_type(udev, p->pid, p->ep->nr)) {
     case USB_ENDPOINT_XFER_BULK:
         size = usb_packet_size(p);
-        r = usb_host_req_alloc(s, p, p->pid == USB_TOKEN_IN, size);
+        r = usb_host_req_alloc(s, p, p->pid == USB_TOKEN_IN, size, 0);
         if (!r->in) {
             usb_packet_copy(p, r->buffer, size);
         }
@@ -1313,7 +1339,7 @@ static void usb_host_handle_data(USBDevice *udev, USBPacket *p)
         }
         break;
     case USB_ENDPOINT_XFER_INT:
-        r = usb_host_req_alloc(s, p, p->pid == USB_TOKEN_IN, p->iov.size);
+        r = usb_host_req_alloc(s, p, p->pid == USB_TOKEN_IN, p->iov.size, 0);
         if (!r->in) {
             usb_packet_copy(p, r->buffer, p->iov.size);
         }
@@ -1324,6 +1350,17 @@ static void usb_host_handle_data(USBDevice *udev, USBPacket *p)
                                        INTR_TIMEOUT);
         break;
     case USB_ENDPOINT_XFER_ISOC:
+        if (s->options & (1 << USB_HOST_XEN_ISO_PASSTHROUGH)) {
+            packets = usbback_get_packets(p);
+            r = usb_host_req_alloc(s, p, p->pid == USB_TOKEN_IN, p->iov.size,
+                                   packets);
+            ep = p->ep->nr | (r->in ? USB_DIR_IN : 0);
+            libusb_fill_iso_transfer(r->xfer, s->dh, ep, r->buffer, p->iov.size,
+                                     packets, usb_host_req_complete_iso_xen, r,
+                                     ISO_TIMEOUT);
+            usbback_set_iso_desc(p, r->xfer);
+            break;
+        }
         if (p->pid == USB_TOKEN_IN) {
             usb_host_iso_data_in(s, p);
         } else {
@@ -1484,6 +1521,8 @@ static Property usb_host_dev_properties[] = {
                        LIBUSB_LOG_LEVEL_WARNING),
     DEFINE_PROP_BIT("pipeline",    USBHostDevice, options,
                     USB_HOST_OPT_PIPELINE, true),
+    DEFINE_PROP_BIT("xen-iso-passthrough", USBHostDevice, options,
+                    USB_HOST_XEN_ISO_PASSTHROUGH, false),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/include/hw/xen/xen_backend.h b/include/hw/xen/xen_backend.h
index 911ba6d..f194aae 100644
--- a/include/hw/xen/xen_backend.h
+++ b/include/hw/xen/xen_backend.h
@@ -55,6 +55,9 @@ struct XenDevice {
 
 /* ------------------------------------------------------------- */
 
+#define usbback_get_packets(p) 0
+#define usbback_set_iso_desc(p, xfer)
+
 /* variables */
 extern XenXC xen_xc;
 extern struct xs_handle *xenstore;
-- 
2.1.4

Re: [Qemu-devel] [Patch V1 2/3] xen/usb: add capability for passing through isoc jobs to host devices

[Gerd Hoffmann]

On Do, 2015-09-03 at 12:45 +0200, Juergen Gross wrote:
> When Xen is using the qemu usb framework for pure passthrough of I/Os
> to host devices the handling of isoc jobs is rather complicated if
> multiple isoc frames are transferred with one call.
> 
> Instead of calling the framework with each frame individually, using
> timers to avoid polling in a loop and sampling all responses to
> construct a sum response for the user, just add a capability to
> use the libusb isoc framework instead. This capability is selected
> via a device specific property.
> 
> When the property is selected the host usb driver will use xen specific
> callbacks to signal the end of isoc I/Os. For now these callbacks will
> just be nops, they'll be filled with sensible actions when the xen
> pv-usb backend is being added.

So you basically add support for async isoc requests.  Fine.

There is nothing xen specific in this though, except that xen is (so
far) the only user.  It isn't going to work for uhci and ehci, put
possibly xhci can join the party.

So, the signaling needs to be different.  The host adapter needs to
signal somehow that it can handle async iso packets.  One way would be
to flag this per usb bus, another one per usb packet.  Also all xen
naming and the xen inlude should go away.  BTW: does this build without
xen-devel installed?

Can we get rid of the callbacks?  By filling the USBPacket iovec with
the iso request chunks for example?

cheers,
  Gerd

Re: [Qemu-devel] [Patch V1 2/3] xen/usb: add capability for passing through isoc jobs to host devices

[Juergen Gross]

On 09/04/2015 03:25 PM, Gerd Hoffmann wrote:
> On Do, 2015-09-03 at 12:45 +0200, Juergen Gross wrote:
>> When Xen is using the qemu usb framework for pure passthrough of I/Os
>> to host devices the handling of isoc jobs is rather complicated if
>> multiple isoc frames are transferred with one call.
>>
>> Instead of calling the framework with each frame individually, using
>> timers to avoid polling in a loop and sampling all responses to
>> construct a sum response for the user, just add a capability to
>> use the libusb isoc framework instead. This capability is selected
>> via a device specific property.
>>
>> When the property is selected the host usb driver will use xen specific
>> callbacks to signal the end of isoc I/Os. For now these callbacks will
>> just be nops, they'll be filled with sensible actions when the xen
>> pv-usb backend is being added.
>
> So you basically add support for async isoc requests.  Fine.
>
> There is nothing xen specific in this though, except that xen is (so
> far) the only user.  It isn't going to work for uhci and ehci, put
> possibly xhci can join the party.
>
> So, the signaling needs to be different.  The host adapter needs to
> signal somehow that it can handle async iso packets.  One way would be
> to flag this per usb bus, another one per usb packet.  Also all xen
> naming and the xen inlude should go away.  BTW: does this build without
> xen-devel installed?

Okay, I'll try to make it more generic. I think the async iso capability
should be a bus attribute.

> Can we get rid of the callbacks?  By filling the USBPacket iovec with
> the iso request chunks for example?

Difficult. One iso request chunk could require multiple iovec entries.

The RFC version tried to avoid the callbacks and there you didn't like
exposing the additional structures. I could, of course, try to combine
both variants by using the device property to enable async isoc and
add generic callbacks with some new structures to replace the xen/libusb
specific variants. This would still leave USBPacket unmodified and all
hcds capable of supporting the new feature could join.

What do you think?


Juergen

Re: [Qemu-devel] [Patch V1 2/3] xen/usb: add capability for passing through isoc jobs to host devices

[Gerd Hoffmann]

  Hi,

> > So, the signaling needs to be different.  The host adapter needs to
> > signal somehow that it can handle async iso packets.  One way would be
> > to flag this per usb bus, another one per usb packet.  Also all xen
> > naming and the xen inlude should go away.  BTW: does this build without
> > xen-devel installed?
> 
> Okay, I'll try to make it more generic. I think the async iso capability
> should be a bus attribute.

Makes sense.

> > Can we get rid of the callbacks?  By filling the USBPacket iovec with
> > the iso request chunks for example?
> 
> Difficult. One iso request chunk could require multiple iovec entries.

Why multiple small iovecs instead of one big iovec?

usb_host_req_complete_iso_xen() returns a single status for the whole
USBPacket anyway ...

> The RFC version tried to avoid the callbacks and there you didn't like
> exposing the additional structures.

-ENOPATCH.

cheers,
  Gerd

Re: [Qemu-devel] [Patch V1 2/3] xen/usb: add capability for passing through isoc jobs to host devices

[Juergen Gross]

On 09/09/2015 02:01 PM, Gerd Hoffmann wrote:
>    Hi,
>
>>> So, the signaling needs to be different.  The host adapter needs to
>>> signal somehow that it can handle async iso packets.  One way would be
>>> to flag this per usb bus, another one per usb packet.  Also all xen
>>> naming and the xen inlude should go away.  BTW: does this build without
>>> xen-devel installed?
>>
>> Okay, I'll try to make it more generic. I think the async iso capability
>> should be a bus attribute.
>
> Makes sense.
>
>>> Can we get rid of the callbacks?  By filling the USBPacket iovec with
>>> the iso request chunks for example?
>>
>> Difficult. One iso request chunk could require multiple iovec entries.
>
> Why multiple small iovecs instead of one big iovec?

The guest buffer might span multiple physical non contiguous pages. I
don't want to copy data to a new buffer due to performance reasons
(there is already at least one copy operation done by qemu).

> usb_host_req_complete_iso_xen() returns a single status for the whole
> USBPacket anyway ...

I need status per iso request, and libusb does deliver that.

>> The RFC version tried to avoid the callbacks and there you didn't like
>> exposing the additional structures.
>
> -ENOPATCH.

Aah, sorry, my fault. I think this was part of the off-list discussion
we had.


Juergen

Re: [Qemu-devel] [Patch V1 2/3] xen/usb: add capability for passing through isoc jobs to host devices

[Gerd Hoffmann]

> > Why multiple small iovecs instead of one big iovec?
> 
> The guest buffer might span multiple physical non contiguous pages.

Sure, thats why we have iovecs in the first place.

>  I
> don't want to copy data to a new buffer due to performance reasons
> (there is already at least one copy operation done by qemu).

We can walk the iovec and fill the libusb struct with the pointers and
lengths instead of using usb_copy_packet ...

> > usb_host_req_complete_iso_xen() returns a single status for the whole
> > USBPacket anyway ...
> 
> I need status per iso request, and libusb does deliver that.

So one usbpacket per iso request should make this work.  And of you
enable pipelining for the endpoint qemu allows multiple outstanding
async packages, so you can queue up a batch of requests ...

cheers,
  Gerd

Re: [Qemu-devel] [Xen-devel] [Patch V1 2/3] xen/usb: add capability for passing through isoc jobs to host devices

[Konrad Rzeszutek Wilk]

On Thu, Sep 03, 2015 at 12:45:12PM +0200, Juergen Gross wrote:
> When Xen is using the qemu usb framework for pure passthrough of I/Os
> to host devices the handling of isoc jobs is rather complicated if
> multiple isoc frames are transferred with one call.
> 
> Instead of calling the framework with each frame individually, using
> timers to avoid polling in a loop and sampling all responses to
> construct a sum response for the user, just add a capability to
> use the libusb isoc framework instead. This capability is selected
> via a device specific property.
> 
> When the property is selected the host usb driver will use xen specific

You mean it will use generic callbacks - but call Xen specific code?

> callbacks to signal the end of isoc I/Os. For now these callbacks will
> just be nops, they'll be filled with sensible actions when the xen
> pv-usb backend is being added.

The PV USB backend being in the QEMU driver?

But this patch by itself will avoid some complex code right?
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
>  hw/usb/core.c                | 11 ++++++----
>  hw/usb/host-libusb.c         | 51 ++++++++++++++++++++++++++++++++++++++------
>  include/hw/xen/xen_backend.h |  3 +++
>  3 files changed, 55 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/usb/core.c b/hw/usb/core.c
> index cf34755..ed2255c 100644
> --- a/hw/usb/core.c
> +++ b/hw/usb/core.c
> @@ -427,10 +427,13 @@ void usb_handle_packet(USBDevice *dev, USBPacket *p)
>      if (QTAILQ_EMPTY(&p->ep->queue) || p->ep->pipeline || p->stream) {
>          usb_process_one(p);
>          if (p->status == USB_RET_ASYNC) {
> -            /* hcd drivers cannot handle async for isoc */
> -            assert(p->ep->type != USB_ENDPOINT_XFER_ISOC);
> -            /* using async for interrupt packets breaks migration */
> -            assert(p->ep->type != USB_ENDPOINT_XFER_INT ||
> +            /*
> +             * hcd drivers cannot handle async for isoc.
> +             * Using async for interrupt packets breaks migration.
> +             * Host devices are okay in any case.
> +             */
> +            assert((p->ep->type != USB_ENDPOINT_XFER_ISOC &&
> +                    p->ep->type != USB_ENDPOINT_XFER_INT) ||
>                     (dev->flags & (1 << USB_DEV_FLAG_IS_HOST)));
>              usb_packet_set_state(p, USB_PACKET_ASYNC);
>              QTAILQ_INSERT_TAIL(&p->ep->queue, p, queue);
> diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c
> index a5f9dab..ce644c3 100644
> --- a/hw/usb/host-libusb.c
> +++ b/hw/usb/host-libusb.c
> @@ -42,6 +42,7 @@
>  #include "trace.h"
>  
>  #include "hw/usb.h"
> +#include "hw/xen/xen_backend.h"
>  
>  /* ------------------------------------------------------------------------ */
>  
> @@ -64,6 +65,7 @@ struct USBAutoFilter {
>  
>  enum USBHostDeviceOptions {
>      USB_HOST_OPT_PIPELINE,
> +    USB_HOST_XEN_ISO_PASSTHROUGH,
>  };
>  
>  struct USBHostDevice {
> @@ -152,6 +154,7 @@ static void usb_host_attach_kernel(USBHostDevice *s);
>  #define CONTROL_TIMEOUT  10000        /* 10 sec    */
>  #define BULK_TIMEOUT         0        /* unlimited */
>  #define INTR_TIMEOUT         0        /* unlimited */
> +#define ISO_TIMEOUT          0        /* unlimited */
>  
>  #if LIBUSBX_API_VERSION >= 0x01000103
>  # define HAVE_STREAMS 1
> @@ -306,14 +309,14 @@ static bool usb_host_use_combining(USBEndpoint *ep)
>  /* ------------------------------------------------------------------------ */
>  
>  static USBHostRequest *usb_host_req_alloc(USBHostDevice *s, USBPacket *p,
> -                                          bool in, size_t bufsize)
> +                                          bool in, size_t bufsize, int packets)
>  {
>      USBHostRequest *r = g_new0(USBHostRequest, 1);
>  
>      r->host = s;
>      r->p = p;
>      r->in = in;
> -    r->xfer = libusb_alloc_transfer(0);
> +    r->xfer = libusb_alloc_transfer(packets);
>      if (bufsize) {
>          r->buffer = g_malloc(bufsize);
>      }
> @@ -376,6 +379,29 @@ out:
>      }
>  }
>  
> +static void usb_host_req_complete_iso_xen(struct libusb_transfer *xfer)
> +{
> +    USBHostRequest *r = xfer->user_data;
> +    USBHostDevice  *s = r->host;
> +    bool disconnect = (xfer->status == LIBUSB_TRANSFER_NO_DEVICE);
> +
> +    if (r->p == NULL) {
> +        goto out; /* request was canceled */
> +    }
> +
> +    r->p->status = status_map[xfer->status];
> +    trace_usb_host_req_complete(s->bus_num, s->addr, r->p,
> +                                r->p->status, r->p->actual_length);
> +    /* copying of buffer is done in complete callback of xen */
> +    usb_packet_complete(USB_DEVICE(s), r->p);
> +
> +out:
> +    usb_host_req_free(r);
> +    if (disconnect) {
> +        usb_host_nodev(s);
> +    }
> +}
> +
>  static void usb_host_req_complete_data(struct libusb_transfer *xfer)
>  {
>      USBHostRequest *r = xfer->user_data;
> @@ -1226,7 +1252,7 @@ static void usb_host_handle_control(USBDevice *udev, USBPacket *p,
>          }
>      }
>  
> -    r = usb_host_req_alloc(s, p, (request >> 8) & USB_DIR_IN, length + 8);
> +    r = usb_host_req_alloc(s, p, (request >> 8) & USB_DIR_IN, length + 8, 0);
>      r->cbuf = data;
>      r->clen = length;
>      memcpy(r->buffer, udev->setup_buf, 8);
> @@ -1264,7 +1290,7 @@ static void usb_host_handle_data(USBDevice *udev, USBPacket *p)
>      USBHostDevice *s = USB_HOST_DEVICE(udev);
>      USBHostRequest *r;
>      size_t size;
> -    int ep, rc;
> +    int ep, rc, packets;
>  
>      if (usb_host_use_combining(p->ep) && p->state == USB_PACKET_SETUP) {
>          p->status = USB_RET_ADD_TO_QUEUE;
> @@ -1289,7 +1315,7 @@ static void usb_host_handle_data(USBDevice *udev, USBPacket *p)
>      switch (usb_ep_get_type(udev, p->pid, p->ep->nr)) {
>      case USB_ENDPOINT_XFER_BULK:
>          size = usb_packet_size(p);
> -        r = usb_host_req_alloc(s, p, p->pid == USB_TOKEN_IN, size);
> +        r = usb_host_req_alloc(s, p, p->pid == USB_TOKEN_IN, size, 0);
>          if (!r->in) {
>              usb_packet_copy(p, r->buffer, size);
>          }
> @@ -1313,7 +1339,7 @@ static void usb_host_handle_data(USBDevice *udev, USBPacket *p)
>          }
>          break;
>      case USB_ENDPOINT_XFER_INT:
> -        r = usb_host_req_alloc(s, p, p->pid == USB_TOKEN_IN, p->iov.size);
> +        r = usb_host_req_alloc(s, p, p->pid == USB_TOKEN_IN, p->iov.size, 0);
>          if (!r->in) {
>              usb_packet_copy(p, r->buffer, p->iov.size);
>          }
> @@ -1324,6 +1350,17 @@ static void usb_host_handle_data(USBDevice *udev, USBPacket *p)
>                                         INTR_TIMEOUT);
>          break;
>      case USB_ENDPOINT_XFER_ISOC:
> +        if (s->options & (1 << USB_HOST_XEN_ISO_PASSTHROUGH)) {
> +            packets = usbback_get_packets(p);
> +            r = usb_host_req_alloc(s, p, p->pid == USB_TOKEN_IN, p->iov.size,
> +                                   packets);
> +            ep = p->ep->nr | (r->in ? USB_DIR_IN : 0);
> +            libusb_fill_iso_transfer(r->xfer, s->dh, ep, r->buffer, p->iov.size,
> +                                     packets, usb_host_req_complete_iso_xen, r,
> +                                     ISO_TIMEOUT);
> +            usbback_set_iso_desc(p, r->xfer);
> +            break;
> +        }
>          if (p->pid == USB_TOKEN_IN) {
>              usb_host_iso_data_in(s, p);
>          } else {
> @@ -1484,6 +1521,8 @@ static Property usb_host_dev_properties[] = {
>                         LIBUSB_LOG_LEVEL_WARNING),
>      DEFINE_PROP_BIT("pipeline",    USBHostDevice, options,
>                      USB_HOST_OPT_PIPELINE, true),
> +    DEFINE_PROP_BIT("xen-iso-passthrough", USBHostDevice, options,
> +                    USB_HOST_XEN_ISO_PASSTHROUGH, false),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> diff --git a/include/hw/xen/xen_backend.h b/include/hw/xen/xen_backend.h
> index 911ba6d..f194aae 100644
> --- a/include/hw/xen/xen_backend.h
> +++ b/include/hw/xen/xen_backend.h
> @@ -55,6 +55,9 @@ struct XenDevice {
>  
>  /* ------------------------------------------------------------- */
>  
> +#define usbback_get_packets(p) 0
> +#define usbback_set_iso_desc(p, xfer)
> +
>  /* variables */
>  extern XenXC xen_xc;
>  extern struct xs_handle *xenstore;
> -- 
> 2.1.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

[Qemu-devel] [Patch V1 3/3] xen: add pvUSB backend

[Juergen Gross]

Add a backend for para-virtualized USB devices for xen domains.

The backend is using host-libusb to forward USB requests from a
domain via libusb to the real device(s) passed through.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 hw/usb/Makefile.objs         |    4 +
 hw/usb/xen-usb.c             | 1120 ++++++++++++++++++++++++++++++++++++++++++
 hw/xenpv/xen_machine_pv.c    |    3 +
 include/hw/xen/xen_backend.h |   13 +-
 4 files changed, 1137 insertions(+), 3 deletions(-)
 create mode 100644 hw/usb/xen-usb.c

diff --git a/hw/usb/Makefile.objs b/hw/usb/Makefile.objs
index 3fe4dff..0253184 100644
--- a/hw/usb/Makefile.objs
+++ b/hw/usb/Makefile.objs
@@ -36,3 +36,7 @@ common-obj-$(CONFIG_USB_REDIR) += redirect.o quirks.o
 
 # usb pass-through
 common-obj-y += $(patsubst %,host-%.o,$(HOST_USB))
+
+ifeq ($(CONFIG_USB_LIBUSB),y)
+common-obj-$(CONFIG_XEN_BACKEND) += xen-usb.o
+endif
diff --git a/hw/usb/xen-usb.c b/hw/usb/xen-usb.c
new file mode 100644
index 0000000..2570bd7
--- /dev/null
+++ b/hw/usb/xen-usb.c
@@ -0,0 +1,1120 @@
+/*
+ *  xen paravirt usb device backend
+ *
+ *  (c) Juergen Gross <jgross@suse.com>
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; under version 2 of the License.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License along
+ *  with this program; if not, see <http://www.gnu.org/licenses/>.
+ *
+ *  Contributions after 2012-01-13 are licensed under the terms of the
+ *  GNU GPL, version 2 or (at your option) any later version.
+ */
+
+#include <libusb.h>
+#include <sys/types.h>
+#include <sys/mman.h>
+#include <sys/time.h>
+
+#include "qemu-common.h"
+#include "qemu/config-file.h"
+#include "hw/sysbus.h"
+#include "hw/usb.h"
+#include "hw/xen/xen_backend.h"
+#include "monitor/qdev.h"
+#include "qapi/qmp/qbool.h"
+#include "qapi/qmp/qint.h"
+#include "qapi/qmp/qstring.h"
+#include "sys/user.h"
+
+#include <xen/io/ring.h>
+#include <xen/io/usbif.h>
+
+#define TR(fmt, args...)                                            \
+    {                                                               \
+        struct timeval tv;                                          \
+                                                                    \
+        gettimeofday(&tv, NULL);                                    \
+        fprintf(stderr, "%8ld.%06ld xen-usb(%s):" fmt, tv.tv_sec,   \
+                tv.tv_usec, __func__, ##args);                      \
+    }
+#define TR_REQ(fmt, args...) { if (tr_debug & 1) TR(fmt, ##args) }
+#define TR_BUS(fmt, args...) { if (tr_debug & 2) TR(fmt, ##args) }
+
+#define USBBACK_MAXPORTS        USBIF_PIPE_PORT_MASK
+#define USBBACK_DEVNAME_SIZE    32
+#define USB_DEV_ADDR_SIZE       128
+
+struct usbif_ctrlrequest {
+    uint8_t    bRequestType;
+    uint8_t    bRequest;
+    uint16_t   wValue;
+    uint16_t   wIndex;
+    uint16_t   wLength;
+};
+
+struct usbif_isoc_descriptor {
+    uint32_t   offset;
+    uint32_t   length;
+    uint32_t   actual_length;
+    int32_t    status;
+};
+
+struct usbback_info;
+struct usbback_req;
+
+struct usbback_stub {
+    USBDevice  *dev;
+    USBPort    port;
+    unsigned   speed;
+    bool       attached;
+    QTAILQ_HEAD(submit_q_head, usbback_req) submit_q;
+};
+
+struct usbback_req {
+    struct usbback_info      *usbif;
+    struct usbback_stub      *stub;
+    struct usbif_urb_request req;
+    USBPacket                packet;
+
+    unsigned int             nr_buffer_segs; /* # of transfer_buffer segments */
+    unsigned int             nr_extra_segs;  /* # of iso_frame_desc segments  */
+
+    QTAILQ_ENTRY(usbback_req) q;
+
+    void                     *buffer;
+    void                     *isoc_buffer;
+    struct libusb_transfer   *xfer;
+};
+
+struct usbback_info {
+    struct XenDevice         xendev;  /* must be first */
+    USBBus                   bus;
+    void                     *urb_sring;
+    void                     *conn_sring;
+    struct usbif_urb_back_ring urb_ring;
+    struct usbif_conn_back_ring conn_ring;
+    int                      num_ports;
+    int                      usb_ver;
+    bool                     ring_error;
+    QTAILQ_HEAD(req_free_q_head, usbback_req) req_free_q;
+    struct usbback_stub      ports[USBBACK_MAXPORTS];
+    struct usbback_stub      *addr_table[USB_DEV_ADDR_SIZE];
+    QEMUBH                   *bh;
+};
+
+static unsigned int tr_debug = 3;
+
+static void usbback_copy_buffer(struct usbback_req *usbback_req,
+                                struct usbif_isoc_descriptor *iso, bool out,
+                                unsigned len, unsigned off)
+{
+    struct usbif_request_segment *seg;
+    unsigned s, offset, copy_len, copy_off;
+    void *addr;
+
+    offset = 0;
+    copy_off = iso->offset;
+    for (s = 0; len && s < usbback_req->nr_buffer_segs; s++) {
+        seg = usbback_req->req.seg + s;
+        if (offset + seg->length > copy_off) {
+            addr = usbback_req->buffer + s * PAGE_SIZE + seg->offset +
+                   copy_off - offset;
+            copy_len = len;
+            if (copy_len > seg->length - copy_off + offset) {
+                copy_len = seg->length - copy_off + offset;
+            }
+            if (out) {
+                memcpy(usbback_req->xfer->buffer + off, addr, copy_len);
+            } else {
+                memcpy(addr, usbback_req->xfer->buffer + off, copy_len);
+            }
+            len -= copy_len;
+            off += copy_len;
+        }
+        offset += usbback_req->req.seg[s].length;
+    }
+    assert(!len);
+}
+
+int usbback_get_packets(USBPacket *p)
+{
+    struct usbback_req *usbback_req;
+
+    usbback_req = container_of(p, struct usbback_req, packet);
+    assert(usbif_pipeisoc(usbback_req->req.pipe));
+    return usbback_req->req.u.isoc.nr_frame_desc_segs;
+}
+
+void usbback_set_iso_desc(USBPacket *p, struct libusb_transfer *xfer)
+{
+    struct usbback_req *usbback_req;
+    struct usbif_isoc_descriptor *iso;
+    struct usbif_request_segment *seg;
+    unsigned i, j, np, descr, off;
+
+    usbback_req = container_of(p, struct usbback_req, packet);
+    assert(usbif_pipeisoc(usbback_req->req.pipe));
+    usbback_req->xfer = xfer;
+    np = usbback_req->req.u.isoc.nr_frame_desc_segs;
+    descr = 0;
+    off = 0;
+
+    for (i = 0; i < usbback_req->nr_extra_segs; i++) {
+        seg = usbback_req->req.seg + usbback_req->nr_buffer_segs + i;
+        iso = usbback_req->isoc_buffer + i * PAGE_SIZE + seg->offset;
+        if ((seg->length % sizeof(*iso)) ||
+            (seg->length / sizeof(*iso) > np - descr) ||
+            (iso->offset + iso->length > usbback_req->req.buffer_length)) {
+            xen_be_printf(&usbback_req->usbif->xendev, 0,
+                          "iso segment length invalid\n");
+            xfer->num_iso_packets = descr;
+            while (descr < usbback_req->req.u.isoc.nr_frame_desc_segs) {
+                xfer->iso_packet_desc[descr].length = 0;
+                xfer->iso_packet_desc[descr].actual_length = 0;
+                descr++;
+            }
+            return;
+        }
+        for (j = 0; j < seg->length / sizeof(*iso); j++) {
+            xfer->iso_packet_desc[descr].length = iso->length;
+            xfer->iso_packet_desc[descr].actual_length = 0;
+            if (!usbif_pipein(usbback_req->req.pipe)) {
+                usbback_copy_buffer(usbback_req, iso, true, iso->length, off);
+                off += iso->length;
+            }
+            iso++;
+            descr++;
+        }
+    }
+}
+
+static struct usbback_req *usbback_get_req(struct usbback_info *usbif)
+{
+    struct usbback_req *usbback_req;
+
+    if (QTAILQ_EMPTY(&usbif->req_free_q)) {
+        usbback_req = g_malloc0(sizeof(*usbback_req));
+    } else {
+        usbback_req = QTAILQ_FIRST(&usbif->req_free_q);
+        QTAILQ_REMOVE(&usbif->req_free_q, usbback_req, q);
+    }
+    return usbback_req;
+}
+
+static void usbback_put_req(struct usbback_req *usbback_req)
+{
+    struct usbback_info *usbif;
+
+    usbif = usbback_req->usbif;
+    memset(usbback_req, 0, sizeof(*usbback_req));
+    QTAILQ_INSERT_HEAD(&usbif->req_free_q, usbback_req, q);
+}
+
+static int usbback_gnttab_map(struct usbback_info *usbif,
+                              struct usbback_req *usbback_req)
+{
+    unsigned int nr_segs, i, prot;
+    uint32_t ref[USBIF_MAX_SEGMENTS_PER_REQUEST];
+    struct XenDevice *xendev = &usbif->xendev;
+    struct usbif_request_segment *seg;
+    void *addr;
+
+    nr_segs = usbback_req->nr_buffer_segs + usbback_req->nr_extra_segs;
+    if (!nr_segs) {
+        return 0;
+    }
+
+    if (nr_segs > USBIF_MAX_SEGMENTS_PER_REQUEST) {
+        xen_be_printf(xendev, 0, "bad number of segments in request (%d)\n",
+                      nr_segs);
+        return -EINVAL;
+    }
+
+    for (i = 0; i < nr_segs; i++) {
+        if ((unsigned)usbback_req->req.seg[i].offset +
+            (unsigned)usbback_req->req.seg[i].length > PAGE_SIZE) {
+            xen_be_printf(xendev, 0, "segment crosses page boundary\n");
+            return -EINVAL;
+        }
+    }
+
+    if (usbback_req->nr_buffer_segs) {
+        prot = PROT_READ;
+        if (usbif_pipein(usbback_req->req.pipe)) {
+                prot |= PROT_WRITE;
+        }
+        for (i = 0; i < usbback_req->nr_buffer_segs; i++) {
+            ref[i] = usbback_req->req.seg[i].gref;
+        }
+        usbback_req->buffer = xc_gnttab_map_domain_grant_refs(xendev->gnttabdev,
+            usbback_req->nr_buffer_segs, xendev->dom, ref, prot);
+
+        if (!usbback_req->buffer) {
+            return -ENOMEM;
+        }
+
+        for (i = 0; i < usbback_req->nr_buffer_segs; i++) {
+            seg = usbback_req->req.seg + i;
+            addr = usbback_req->buffer + i * PAGE_SIZE + seg->offset;
+            qemu_iovec_add(&usbback_req->packet.iov, addr, seg->length);
+        }
+    }
+
+    if (!usbif_pipeisoc(usbback_req->req.pipe)) {
+        return 0;
+    }
+
+    if (!usbback_req->nr_extra_segs) {
+        xen_be_printf(xendev, 0, "iso request without descriptor segments\n");
+        return -EINVAL;
+    }
+
+    prot = PROT_READ | PROT_WRITE;
+    for (i = 0; i < usbback_req->nr_extra_segs; i++) {
+        ref[i] = usbback_req->req.seg[i + usbback_req->req.nr_buffer_segs].gref;
+    }
+    usbback_req->isoc_buffer = xc_gnttab_map_domain_grant_refs(
+         xendev->gnttabdev, usbback_req->nr_extra_segs, xendev->dom, ref, prot);
+
+    if (!usbback_req->isoc_buffer) {
+        return -ENOMEM;
+    }
+
+    return 0;
+}
+
+static int usbback_init_packet(struct usbback_req *usbback_req)
+{
+    USBPacket *packet = &usbback_req->packet;
+    USBDevice *dev = usbback_req->stub->dev;
+    USBEndpoint *ep;
+    unsigned int pid, ep_nr;
+    bool sok;
+
+    qemu_iovec_init(&packet->iov, USBIF_MAX_SEGMENTS_PER_REQUEST);
+    pid = usbif_pipein(usbback_req->req.pipe) ? USB_TOKEN_IN : USB_TOKEN_OUT;
+    ep_nr = usbif_pipeendpoint(usbback_req->req.pipe);
+    sok = !!(usbback_req->req.transfer_flags & 1);
+    if (usbif_pipetype(usbback_req->req.pipe) == USBIF_PIPE_TYPE_CTRL) {
+        ep_nr = 0;
+        sok = false;
+    }
+    ep = usb_ep_get(dev, pid, ep_nr);
+    usb_packet_setup(packet, pid, ep, 0, 1, sok, true);
+
+    switch (usbif_pipetype(usbback_req->req.pipe)) {
+    case USBIF_PIPE_TYPE_ISOC:
+        TR_REQ("iso transfer %s: buflen: %x, %d frames\n",
+               (pid == USB_TOKEN_IN) ? "in" : "out",
+               usbback_req->req.buffer_length,
+               usbback_req->req.u.isoc.nr_frame_desc_segs);
+        break;
+
+    case USBIF_PIPE_TYPE_INT:
+        TR_REQ("int transfer %s: buflen: %x\n",
+               (pid == USB_TOKEN_IN) ? "in" : "out",
+               usbback_req->req.buffer_length);
+        break;
+
+    case USBIF_PIPE_TYPE_CTRL:
+        packet->parameter = *(uint64_t *)usbback_req->req.u.ctrl;
+        TR_REQ("ctrl parameter: %lx, buflen: %x\n", packet->parameter,
+               usbback_req->req.buffer_length);
+        break;
+
+    case USBIF_PIPE_TYPE_BULK:
+        TR_REQ("bulk transfer %s: buflen: %x\n",
+               (pid == USB_TOKEN_IN) ? "in" : "out",
+               usbback_req->req.buffer_length);
+        break;
+    default:
+        return -EINVAL;
+    }
+
+    return 0;
+}
+
+static void usbback_do_response(struct usbback_req *usbback_req,
+                                int32_t status, int32_t actual_length,
+                                int32_t error_count)
+{
+    struct usbback_info *usbif;
+    struct usbif_urb_response *res;
+    struct XenDevice *xendev;
+    unsigned int notify;
+
+    TR_REQ("id %d, status %d, length %d, errcnt %d\n", usbback_req->req.id,
+           status, actual_length, error_count);
+
+    usbif = usbback_req->usbif;
+    xendev = &usbif->xendev;
+
+    if (usbback_req->packet.iov.iov) {
+        qemu_iovec_destroy(&usbback_req->packet.iov);
+    }
+
+    if (usbback_req->buffer) {
+        xc_gnttab_munmap(xendev->gnttabdev, usbback_req->buffer,
+                         usbback_req->nr_buffer_segs);
+        usbback_req->buffer = NULL;
+    }
+
+    if (usbback_req->isoc_buffer) {
+        xc_gnttab_munmap(xendev->gnttabdev, usbback_req->isoc_buffer,
+                         usbback_req->nr_extra_segs);
+    }
+
+    res = RING_GET_RESPONSE(&usbif->urb_ring, usbif->urb_ring.rsp_prod_pvt);
+    res->id = usbback_req->req.id;
+    res->status = status;
+    res->actual_length = actual_length;
+    res->error_count = error_count;
+    res->start_frame = 0;
+    usbif->urb_ring.rsp_prod_pvt++;
+    RING_PUSH_RESPONSES_AND_CHECK_NOTIFY(&usbif->urb_ring, notify);
+
+    if (notify) {
+        xen_be_send_notify(xendev);
+    }
+
+    usbback_put_req(usbback_req);
+}
+
+static void usbback_do_response_ret(struct usbback_req *usbback_req,
+                                    int32_t status)
+{
+    usbback_do_response(usbback_req, status, 0, 0);
+}
+
+static int32_t usbback_xlat_status(int32_t status)
+{
+    int32_t ret = -ESHUTDOWN;
+
+    switch (status) {
+    case USB_RET_SUCCESS:
+        ret = 0;
+        break;
+    case USB_RET_NODEV:
+        ret = -ENODEV;
+        break;
+    case USB_RET_STALL:
+        ret = -EPIPE;
+        break;
+    case USB_RET_BABBLE:
+        ret = -EOVERFLOW;
+        break;
+    case USB_RET_IOERROR:
+        ret = -EPROTO;
+        break;
+    }
+
+    return ret;
+}
+
+static void usbback_packet_complete(USBPacket *packet)
+{
+    struct usbback_req *usbback_req;
+    int32_t status, error_count, actual_length;
+    unsigned int i, j, off;
+    struct usbif_isoc_descriptor *iso;
+    struct usbif_request_segment *seg;
+    struct libusb_iso_packet_descriptor *libusb_iso;
+
+    error_count = 0;
+    actual_length = 0;
+    off = 0;
+    usbback_req = container_of(packet, struct usbback_req, packet);
+
+    QTAILQ_REMOVE(&usbback_req->stub->submit_q, usbback_req, q);
+
+    status = usbback_xlat_status(packet->status);
+    if (usbback_req->isoc_buffer) {
+        libusb_iso = usbback_req->xfer->iso_packet_desc;
+        j = 0;
+        for (i = 0; i < usbback_req->nr_extra_segs; i++) {
+            seg = usbback_req->req.seg + usbback_req->nr_buffer_segs + i;
+            iso = usbback_req->isoc_buffer + i * PAGE_SIZE + seg->offset;
+            for (j = 0; j < seg->length / sizeof(*iso); j++) {
+                iso->actual_length = libusb_iso->actual_length;
+                iso->status = usbback_xlat_status(libusb_iso->status);
+                actual_length += libusb_iso->actual_length;
+                error_count += iso->status ? 1 : 0;
+                if (usbif_pipein(usbback_req->req.pipe)) {
+                    usbback_copy_buffer(usbback_req, iso, false,
+                                        iso->actual_length, off);
+                    off += iso->length;
+                }
+                libusb_iso++;
+                iso++;
+            }
+        }
+    } else {
+        actual_length = packet->actual_length;
+    }
+
+    usbback_do_response(usbback_req, status, actual_length, error_count);
+}
+
+static void usbback_set_address(struct usbback_info *usbif,
+                                struct usbback_stub *stub, int cur_addr,
+                                int new_addr)
+{
+    if (cur_addr) {
+        usbif->addr_table[cur_addr] = NULL;
+    }
+    if (new_addr) {
+        usbif->addr_table[new_addr] = stub;
+    }
+}
+
+static bool usbback_cancel_req(struct usbback_req *usbback_req)
+{
+    bool ret = false;
+
+    if (usb_packet_is_inflight(&usbback_req->packet)) {
+        usb_cancel_packet(&usbback_req->packet);
+        ret = true;
+    }
+    return ret;
+}
+
+static void usbback_process_unlink_req(struct usbback_req *usbback_req)
+{
+    struct usbback_info *usbif;
+    struct usbback_req *unlink_req;
+    unsigned int id, devnum;
+    int ret;
+
+    usbif = usbback_req->usbif;
+    ret = 0;
+    id = usbback_req->req.u.unlink.unlink_id;
+    TR_REQ("unlink id %d\n", id);
+    devnum = usbif_pipedevice(usbback_req->req.pipe);
+    if (unlikely(devnum == 0)) {
+        usbback_req->stub = usbif->ports +
+                            usbif_pipeportnum(usbback_req->req.pipe);
+        if (unlikely(!usbback_req->stub)) {
+            ret = -ENODEV;
+            goto fail_response;
+        }
+    } else {
+        if (unlikely(!usbif->addr_table[devnum])) {
+            ret = -ENODEV;
+            goto fail_response;
+        }
+        usbback_req->stub = usbif->addr_table[devnum];
+    }
+
+    QTAILQ_FOREACH(unlink_req, &usbback_req->stub->submit_q, q) {
+        if (unlink_req->req.id == id) {
+            if (usbback_cancel_req(unlink_req)) {
+                usbback_do_response_ret(unlink_req, -EPROTO);
+            }
+            break;
+        }
+    }
+
+fail_response:
+    usbback_do_response_ret(usbback_req, ret);
+}
+
+static int usbback_check_and_submit(struct usbback_req *usbback_req)
+{
+    struct usbback_info *usbif;
+    unsigned int devnum;
+    struct usbback_stub *stub;
+    struct usbif_ctrlrequest *ctrl;
+    int ret;
+    uint16_t wValue;
+
+    usbif = usbback_req->usbif;
+    stub = NULL;
+    devnum = usbif_pipedevice(usbback_req->req.pipe);
+    ctrl = (struct usbif_ctrlrequest *)usbback_req->req.u.ctrl;
+    wValue = le16_to_cpu(ctrl->wValue);
+
+    /*
+     * When the device is first connected or resetted, USB device has no
+     * address. In this initial state, following requests are sent to device
+     * address (#0),
+     *
+     *  1. GET_DESCRIPTOR (with Descriptor Type is "DEVICE") is sent,
+     *     and OS knows what device is connected to.
+     *
+     *  2. SET_ADDRESS is sent, and then device has its address.
+     *
+     * In the next step, SET_CONFIGURATION is sent to addressed device, and
+     * then the device is finally ready to use.
+     */
+    if (unlikely(devnum == 0)) {
+        stub = usbif->ports + usbif_pipeportnum(usbback_req->req.pipe) - 1;
+        if (!stub->dev || !stub->attached) {
+            ret = -ENODEV;
+            goto do_response;
+        }
+
+        switch (ctrl->bRequest) {
+        case USB_REQ_GET_DESCRIPTOR:
+            /*
+             * GET_DESCRIPTOR request to device #0.
+             * through to normal transfer.
+             */
+            TR_REQ("devnum 0 GET_DESCRIPTOR\n");
+            usbback_req->stub = stub;
+            return 0;
+        case USB_REQ_SET_ADDRESS:
+            /*
+             * SET_ADDRESS request to device #0.
+             * add attached device to addr_table.
+             */
+            TR_REQ("devnum 0 SET_ADDRESS\n");
+            usbback_set_address(usbif, stub, 0, wValue);
+            ret = 0;
+            break;
+        default:
+            ret = -EINVAL;
+            break;
+        }
+        goto do_response;
+    }
+
+    if (unlikely(!usbif->addr_table[devnum])) {
+            ret = -ENODEV;
+            goto do_response;
+    }
+    usbback_req->stub = usbif->addr_table[devnum];
+
+    /*
+     * Check special request
+     */
+    if (ctrl->bRequest != USB_REQ_SET_ADDRESS) {
+        return 0;
+    }
+
+    /*
+     * SET_ADDRESS request to addressed device.
+     * change addr or remove from addr_table.
+     */
+    usbback_set_address(usbif, usbback_req->stub, devnum, wValue);
+    ret = 0;
+
+do_response:
+    usbback_do_response_ret(usbback_req, ret);
+    return 1;
+}
+
+static void usbback_dispatch(struct usbback_req *usbback_req)
+{
+    int ret;
+    unsigned int devnum;
+    struct usbback_info *usbif;
+
+    TR_REQ("start req_id %d pipe %08x\n", usbback_req->req.id,
+           usbback_req->req.pipe);
+
+    usbif = usbback_req->usbif;
+
+    /* unlink request */
+    if (unlikely(usbif_pipeunlink(usbback_req->req.pipe))) {
+        usbback_process_unlink_req(usbback_req);
+        return;
+    }
+
+    if (usbif_pipectrl(usbback_req->req.pipe)) {
+        if (usbback_check_and_submit(usbback_req)) {
+            return;
+        }
+    } else {
+        devnum = usbif_pipedevice(usbback_req->req.pipe);
+        usbback_req->stub = usbif->addr_table[devnum];
+
+        if (!usbback_req->stub || !usbback_req->stub->attached) {
+            ret = -ENODEV;
+            goto fail_response;
+        }
+    }
+
+    QTAILQ_INSERT_TAIL(&usbback_req->stub->submit_q, usbback_req, q);
+
+    usbback_req->nr_buffer_segs = usbback_req->req.nr_buffer_segs;
+    usbback_req->nr_extra_segs = usbif_pipeisoc(usbback_req->req.pipe) ?
+                                 usbback_req->req.u.isoc.nr_frame_desc_segs : 0;
+
+    ret = usbback_init_packet(usbback_req);
+    if (ret) {
+        xen_be_printf(&usbif->xendev, 0, "invalid request\n");
+        ret = -ESHUTDOWN;
+        goto fail_free_urb;
+    }
+
+    ret = usbback_gnttab_map(usbif, usbback_req);
+    if (ret) {
+        xen_be_printf(&usbif->xendev, 0, "invalid buffer\n");
+        ret = -ESHUTDOWN;
+        goto fail_free_urb;
+    }
+
+    usb_handle_packet(usbback_req->stub->dev, &usbback_req->packet);
+    if (usbback_req->packet.status != USB_RET_ASYNC) {
+        usbback_packet_complete(&usbback_req->packet);
+    }
+    return;
+
+fail_free_urb:
+    QTAILQ_REMOVE(&usbback_req->stub->submit_q, usbback_req, q);
+
+fail_response:
+    usbback_do_response_ret(usbback_req, ret);
+}
+
+static void usbback_bh(void *opaque)
+{
+    struct usbback_info *usbif;
+    struct usbif_urb_back_ring *urb_ring;
+    struct usbback_req *usbback_req;
+    RING_IDX rc, rp;
+    unsigned int more_to_do;
+
+    usbif = opaque;
+    if (usbif->ring_error) {
+        return;
+    }
+
+    urb_ring = &usbif->urb_ring;
+    rc = urb_ring->req_cons;
+    rp = urb_ring->sring->req_prod;
+    xen_rmb(); /* Ensure we see queued requests up to 'rp'. */
+
+    if (RING_REQUEST_PROD_OVERFLOW(urb_ring, rp)) {
+        rc = urb_ring->rsp_prod_pvt;
+        xen_be_printf(&usbif->xendev, 0, "domU provided bogus ring requests "
+                      "(%#x - %#x = %u). Halting ring processing.\n",
+                      rp, rc, rp - rc);
+        usbif->ring_error = true;
+        return;
+    }
+
+    while (rc != rp) {
+        if (RING_REQUEST_CONS_OVERFLOW(urb_ring, rc)) {
+            break;
+        }
+        usbback_req = usbback_get_req(usbif);
+
+        usbback_req->req = *RING_GET_REQUEST(urb_ring, rc);
+        usbback_req->usbif = usbif;
+
+        usbback_dispatch(usbback_req);
+
+        urb_ring->req_cons = ++rc;
+    }
+
+    RING_FINAL_CHECK_FOR_REQUESTS(urb_ring, more_to_do);
+    if (more_to_do) {
+        qemu_bh_schedule(usbif->bh);
+    }
+}
+
+static void usbback_hotplug_notify(struct usbback_info *usbif, unsigned port)
+{
+    struct usbif_conn_back_ring *ring = &usbif->conn_ring;
+    struct usbif_conn_request *req;
+    struct usbif_conn_response *res;
+    uint16_t id;
+    unsigned int notify;
+
+    if (!usbif->conn_sring) {
+        return;
+    }
+
+    req = RING_GET_REQUEST(ring, ring->req_cons);
+    id = req->id;
+    ring->req_cons++;
+    ring->sring->req_event = ring->req_cons + 1;
+
+    res = RING_GET_RESPONSE(ring, ring->rsp_prod_pvt);
+    res->id = id;
+    res->portnum = port;
+    res->speed = usbif->ports[port - 1].speed;
+    ring->rsp_prod_pvt++;
+    RING_PUSH_RESPONSES_AND_CHECK_NOTIFY(ring, notify);
+
+    if (notify) {
+        xen_be_send_notify(&usbif->xendev);
+    }
+
+    TR_BUS("hotplug port %d speed %d\n", port, res->speed);
+}
+
+static void usbback_portid_remove(struct usbback_info *usbif, unsigned port)
+{
+    USBPort *p;
+
+    if (!usbif->ports[port - 1].dev) {
+        return;
+    }
+
+    p = &(usbif->ports[port - 1].port);
+    snprintf(p->path, sizeof(p->path), "%d", 99);
+
+    object_unparent(OBJECT(usbif->ports[port - 1].dev));
+    usbif->ports[port - 1].dev = NULL;
+    usbif->ports[port - 1].speed = USBIF_SPEED_NONE;
+    usbif->ports[port - 1].attached = false;
+    usbback_hotplug_notify(usbif, port);
+
+    TR_BUS("port %d removed\n", port);
+}
+
+static void usbback_portid_add(struct usbback_info *usbif, unsigned port,
+                               char *busid)
+{
+    unsigned speed;
+    char *portname;
+    USBPort *p;
+    Error *local_err = NULL;
+    QDict *qdict;
+    QemuOpts *opts;
+
+    if (usbif->ports[port - 1].dev) {
+        return;
+    }
+
+    portname = strchr(busid, '-');
+    if (!portname) {
+        xen_be_printf(&usbif->xendev, 0, "device %s illegal specification\n",
+                      busid);
+        return;
+    }
+    portname++;
+    p = &(usbif->ports[port - 1].port);
+    snprintf(p->path, sizeof(p->path), "%s", portname);
+
+    qdict = qdict_new();
+    qdict_put(qdict, "driver", qstring_from_str("usb-host"));
+    qdict_put(qdict, "hostbus", qint_from_int(atoi(busid)));
+    qdict_put(qdict, "hostport", qstring_from_str(portname));
+    qdict_put(qdict, "xen-iso-passthrough", qbool_from_int(1));
+    opts = qemu_opts_from_qdict(qemu_find_opts("device"), qdict, &local_err);
+    if (local_err) {
+        goto err;
+    }
+    usbif->ports[port - 1].dev = USB_DEVICE(qdev_device_add(opts));
+    if (!usbif->ports[port - 1].dev) {
+        goto err;
+    }
+    QDECREF(qdict);
+    snprintf(p->path, sizeof(p->path), "%d", port);
+    speed = usbif->ports[port - 1].dev->speed;
+    switch (speed) {
+    case USB_SPEED_LOW:
+        speed = USBIF_SPEED_LOW;
+        break;
+    case USB_SPEED_FULL:
+        speed = USBIF_SPEED_FULL;
+        break;
+    case USB_SPEED_HIGH:
+        speed = (usbif->usb_ver < USB_VER_USB20) ?
+                USBIF_SPEED_NONE : USBIF_SPEED_HIGH;
+        break;
+    default:
+        speed = USBIF_SPEED_NONE;
+        break;
+    }
+    if (speed == USBIF_SPEED_NONE) {
+        xen_be_printf(&usbif->xendev, 0, "device %s wrong speed\n", busid);
+        object_unparent(OBJECT(usbif->ports[port - 1].dev));
+        usbif->ports[port - 1].dev = NULL;
+        return;
+    }
+    usb_device_reset(usbif->ports[port - 1].dev);
+    usbif->ports[port - 1].speed = speed;
+    usbif->ports[port - 1].attached = true;
+    QTAILQ_INIT(&usbif->ports[port - 1].submit_q);
+    usbback_hotplug_notify(usbif, port);
+
+    TR_BUS("port %d attached\n", port);
+    return;
+
+err:
+    QDECREF(qdict);
+    snprintf(p->path, sizeof(p->path), "%d", 99);
+    xen_be_printf(&usbif->xendev, 0, "device %s could not be opened\n", busid);
+}
+
+static void usbback_process_port(struct usbback_info *usbif, unsigned port)
+{
+    char node[8];
+    char *busid;
+
+    snprintf(node, sizeof(node), "port/%d", port);
+    busid = xenstore_read_be_str(&usbif->xendev, node);
+    if (busid == NULL) {
+        xen_be_printf(&usbif->xendev, 0, "xenstore_read %s failed\n", node);
+        return;
+    }
+
+    /* Remove portid, if the port is not connected.  */
+    if (strlen(busid) == 0) {
+        usbback_portid_remove(usbif, port);
+    } else {
+        usbback_portid_add(usbif, port, busid);
+    }
+
+    g_free(busid);
+}
+
+static void usbback_disconnect(struct XenDevice *xendev)
+{
+    struct usbback_info *usbif;
+    struct usbback_req *req, *tmp;
+    unsigned int i;
+
+    TR_BUS("start\n");
+
+    usbif = container_of(xendev, struct usbback_info, xendev);
+
+    xen_be_unbind_evtchn(xendev);
+
+    if (usbif->urb_sring) {
+        xc_gnttab_munmap(xendev->gnttabdev, usbif->urb_sring, 1);
+        usbif->urb_sring = NULL;
+    }
+    if (usbif->conn_sring) {
+        xc_gnttab_munmap(xendev->gnttabdev, usbif->conn_sring, 1);
+        usbif->conn_sring = NULL;
+    }
+
+    for (i = 0; i < usbif->num_ports; i++) {
+        if (!usbif->ports[i].dev) {
+            continue;
+        }
+        QTAILQ_FOREACH_SAFE(req, &usbif->ports[i].submit_q, q, tmp) {
+            usbback_cancel_req(req);
+        }
+    }
+
+    TR_BUS("finished\n");
+}
+
+static int usbback_connect(struct XenDevice *xendev)
+{
+    struct usbback_info *usbif;
+    struct usbif_urb_sring *urb_sring;
+    struct usbif_conn_sring *conn_sring;
+    int urb_ring_ref;
+    int conn_ring_ref;
+    unsigned int i;
+
+    TR_BUS("start\n");
+
+    usbif = container_of(xendev, struct usbback_info, xendev);
+
+    if (xenstore_read_fe_int(xendev, "urb-ring-ref", &urb_ring_ref)) {
+        TR("error reading urb-ring-ref\n");
+        return -1;
+    }
+    if (xenstore_read_fe_int(xendev, "conn-ring-ref", &conn_ring_ref)) {
+        TR("error reading conn-ring-ref\n");
+        return -1;
+    }
+    if (xenstore_read_fe_int(xendev, "event-channel", &xendev->remote_port)) {
+        TR("error reading event-channel\n");
+        return -1;
+    }
+
+    usbif->urb_sring = xc_gnttab_map_grant_ref(xendev->gnttabdev, xendev->dom,
+                                               urb_ring_ref,
+                                               PROT_READ | PROT_WRITE);
+    usbif->conn_sring = xc_gnttab_map_grant_ref(xendev->gnttabdev, xendev->dom,
+                                                conn_ring_ref,
+                                                PROT_READ | PROT_WRITE);
+    if (!usbif->urb_sring || !usbif->conn_sring) {
+        TR("error mapping rings\n");
+        usbback_disconnect(xendev);
+        return -1;
+    }
+
+    urb_sring = usbif->urb_sring;
+    conn_sring = usbif->conn_sring;
+    BACK_RING_INIT(&usbif->urb_ring, urb_sring, XC_PAGE_SIZE);
+    BACK_RING_INIT(&usbif->conn_ring, conn_sring, XC_PAGE_SIZE);
+
+    xen_be_bind_evtchn(xendev);
+
+    xen_be_printf(xendev, 1, "urb-ring-ref %d, conn-ring-ref %d, "
+                  "remote port %d, local port %d\n", urb_ring_ref,
+                  conn_ring_ref, xendev->remote_port, xendev->local_port);
+
+    for (i = 1; i <= usbif->num_ports; i++) {
+        if (usbif->ports[i - 1].dev) {
+            usbback_hotplug_notify(usbif, i);
+        }
+    }
+
+    return 0;
+}
+
+static void usbback_backend_changed(struct XenDevice *xendev, const char *node)
+{
+    struct usbback_info *usbif;
+    unsigned int i;
+
+    TR_BUS("path %s\n", node);
+
+    usbif = container_of(xendev, struct usbback_info, xendev);
+    for (i = 1; i <= usbif->num_ports; i++) {
+        usbback_process_port(usbif, i);
+    }
+}
+
+static int usbback_init(struct XenDevice *xendev)
+{
+    struct usbback_info *usbif;
+
+    TR_BUS("start\n");
+
+    usbif = container_of(xendev, struct usbback_info, xendev);
+
+    if (xenstore_read_be_int(xendev, "num-ports", &usbif->num_ports) ||
+        usbif->num_ports < 1 || usbif->num_ports > USBBACK_MAXPORTS) {
+        xen_be_printf(xendev, 0, "num-ports not readable or out of bounds\n");
+        return -1;
+    }
+    if (xenstore_read_be_int(xendev, "usb-ver", &usbif->usb_ver) ||
+        (usbif->usb_ver != USB_VER_USB11 && usbif->usb_ver != USB_VER_USB20)) {
+        xen_be_printf(xendev, 0, "usb-ver not readable or out of bounds\n");
+        return -1;
+    }
+
+    usbback_backend_changed(xendev, "port");
+
+    TR_BUS("finished\n");
+
+    return 0;
+}
+
+static void xen_bus_attach(USBPort *port)
+{
+    struct usbback_info *usbif;
+
+    TR_BUS("called\n");
+    usbif = port->opaque;
+    usbif->ports[port->index].attached = true;
+    usbback_hotplug_notify(usbif, port->index + 1);
+}
+
+static void xen_bus_detach(USBPort *port)
+{
+    struct usbback_info *usbif;
+
+    TR_BUS("called\n");
+    usbif = port->opaque;
+    usbif->ports[port->index].attached = false;
+    usbback_hotplug_notify(usbif, port->index + 1);
+}
+
+static void xen_bus_child_detach(USBPort *port, USBDevice *child)
+{
+    TR_BUS("called\n");
+}
+
+static void xen_bus_complete(USBPort *port, USBPacket *packet)
+{
+    TR_REQ("called\n");
+    usbback_packet_complete(packet);
+}
+
+static USBPortOps xen_usb_port_ops = {
+    .attach = xen_bus_attach,
+    .detach = xen_bus_detach,
+    .child_detach = xen_bus_child_detach,
+    .complete = xen_bus_complete,
+};
+
+static USBBusOps xen_usb_bus_ops = {
+};
+
+static void usbback_alloc(struct XenDevice *xendev)
+{
+    struct usbback_info *usbif;
+    USBPort *p;
+    unsigned int i, max_grants;
+
+    usbif = container_of(xendev, struct usbback_info, xendev);
+
+    usb_bus_new(&usbif->bus, sizeof(usbif->bus), &xen_usb_bus_ops, xen_sysdev);
+    for (i = 0; i < USBBACK_MAXPORTS; i++) {
+        p = &(usbif->ports[i].port);
+        usb_register_port(&usbif->bus, p, usbif, i, &xen_usb_port_ops,
+                          USB_SPEED_MASK_LOW | USB_SPEED_MASK_FULL |
+                          USB_SPEED_MASK_HIGH);
+        snprintf(p->path, sizeof(p->path), "%d", 99);
+    }
+
+    QTAILQ_INIT(&usbif->req_free_q);
+    usbif->bh = qemu_bh_new(usbback_bh, usbif);
+
+    max_grants = USBIF_MAX_SEGMENTS_PER_REQUEST * USB_URB_RING_SIZE + 2;
+    if (xc_gnttab_set_max_grants(xendev->gnttabdev, max_grants) < 0) {
+        xen_be_printf(xendev, 0, "xc_gnttab_set_max_grants failed: %s\n",
+                      strerror(errno));
+    }
+}
+
+static int usbback_free(struct XenDevice *xendev)
+{
+    struct usbback_info *usbif;
+    struct usbback_req *usbback_req;
+    unsigned int i;
+
+    TR_BUS("start\n");
+
+    usbback_disconnect(xendev);
+    usbif = container_of(xendev, struct usbback_info, xendev);
+    for (i = 1; i <= usbif->num_ports; i++) {
+        usbback_portid_remove(usbif, i);
+    }
+
+    while (!QTAILQ_EMPTY(&usbif->req_free_q)) {
+        usbback_req = QTAILQ_FIRST(&usbif->req_free_q);
+        QTAILQ_REMOVE(&usbif->req_free_q, usbback_req, q);
+        g_free(usbback_req);
+    }
+
+    qemu_bh_delete(usbif->bh);
+
+    usb_bus_release(&usbif->bus);
+
+    TR_BUS("finished\n");
+
+    return 0;
+}
+
+static void usbback_event(struct XenDevice *xendev)
+{
+    struct usbback_info *usbif;
+
+    usbif = container_of(xendev, struct usbback_info, xendev);
+    qemu_bh_schedule(usbif->bh);
+}
+
+struct XenDevOps xen_usb_ops = {
+    .size            = sizeof(struct usbback_info),
+    .flags           = DEVOPS_FLAG_NEED_GNTDEV,
+    .init            = usbback_init,
+    .alloc           = usbback_alloc,
+    .free            = usbback_free,
+    .backend_changed = usbback_backend_changed,
+    .initialise      = usbback_connect,
+    .disconnect      = usbback_disconnect,
+    .event           = usbback_event,
+};
diff --git a/hw/xenpv/xen_machine_pv.c b/hw/xenpv/xen_machine_pv.c
index 57bc071..fef1e7b 100644
--- a/hw/xenpv/xen_machine_pv.c
+++ b/hw/xenpv/xen_machine_pv.c
@@ -72,6 +72,9 @@ static void xen_init_pv(MachineState *machine)
     xen_be_register("vfb", &xen_framebuffer_ops);
     xen_be_register("qdisk", &xen_blkdev_ops);
     xen_be_register("qnic", &xen_netdev_ops);
+#ifdef CONFIG_USB_LIBUSB
+    xen_be_register("qusb", &xen_usb_ops);
+#endif
 
     /* configure framebuffer */
     if (xenfb_enabled) {
diff --git a/include/hw/xen/xen_backend.h b/include/hw/xen/xen_backend.h
index f194aae..3d44dec 100644
--- a/include/hw/xen/xen_backend.h
+++ b/include/hw/xen/xen_backend.h
@@ -4,6 +4,10 @@
 #include "hw/xen/xen_common.h"
 #include "sysemu/sysemu.h"
 #include "net/net.h"
+#ifdef CONFIG_USB_LIBUSB
+#include <libusb.h>
+#include "hw/usb.h"
+#endif
 
 /* ------------------------------------------------------------- */
 
@@ -55,9 +59,6 @@ struct XenDevice {
 
 /* ------------------------------------------------------------- */
 
-#define usbback_get_packets(p) 0
-#define usbback_set_iso_desc(p, xfer)
-
 /* variables */
 extern XenXC xen_xc;
 extern struct xs_handle *xenstore;
@@ -101,6 +102,12 @@ extern struct XenDevOps xen_kbdmouse_ops;     /* xen_framebuffer.c */
 extern struct XenDevOps xen_framebuffer_ops;  /* xen_framebuffer.c */
 extern struct XenDevOps xen_blkdev_ops;       /* xen_disk.c        */
 extern struct XenDevOps xen_netdev_ops;       /* xen_nic.c         */
+#ifdef CONFIG_USB_LIBUSB
+extern struct XenDevOps xen_usb_ops;          /* xen-usb.c         */
+
+int usbback_get_packets(USBPacket *p);
+void usbback_set_iso_desc(USBPacket *p, struct libusb_transfer *xfer);
+#endif
 
 void xen_init_display(int domid);
 
-- 
2.1.4

Re: [Qemu-devel] [Patch V1 3/3] xen: add pvUSB backend

[Stefano Stabellini]

On Thu, 3 Sep 2015, Juergen Gross wrote:
> Add a backend for para-virtualized USB devices for xen domains.
> 
> The backend is using host-libusb to forward USB requests from a
> domain via libusb to the real device(s) passed through.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

Aside from a few minor comments below, this looks pretty good from a Xen
perspective, however I am not too qualified to review the details of the
pvusb protocol or the way the requests are handled internally in QEMU.

I would be glad if Gerd could take a look at this too.

Juergen, if we commit this, would you be happy to maintain it going
forward?


> diff --git a/hw/usb/Makefile.objs b/hw/usb/Makefile.objs
> index 3fe4dff..0253184 100644
> --- a/hw/usb/Makefile.objs
> +++ b/hw/usb/Makefile.objs
> @@ -36,3 +36,7 @@ common-obj-$(CONFIG_USB_REDIR) += redirect.o quirks.o
>  
>  # usb pass-through
>  common-obj-y += $(patsubst %,host-%.o,$(HOST_USB))
> +
> +ifeq ($(CONFIG_USB_LIBUSB),y)
> +common-obj-$(CONFIG_XEN_BACKEND) += xen-usb.o
> +endif
> diff --git a/hw/usb/xen-usb.c b/hw/usb/xen-usb.c
> new file mode 100644
> index 0000000..2570bd7
> --- /dev/null
> +++ b/hw/usb/xen-usb.c
> @@ -0,0 +1,1120 @@
> +/*
> + *  xen paravirt usb device backend
> + *
> + *  (c) Juergen Gross <jgross@suse.com>
> + *
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation; under version 2 of the License.
> + *
> + *  This program is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License along
> + *  with this program; if not, see <http://www.gnu.org/licenses/>.
> + *
> + *  Contributions after 2012-01-13 are licensed under the terms of the
> + *  GNU GPL, version 2 or (at your option) any later version.
> + */
> +
> +#include <libusb.h>
> +#include <sys/types.h>
> +#include <sys/mman.h>
> +#include <sys/time.h>
> +
> +#include "qemu-common.h"
> +#include "qemu/config-file.h"
> +#include "hw/sysbus.h"
> +#include "hw/usb.h"
> +#include "hw/xen/xen_backend.h"
> +#include "monitor/qdev.h"
> +#include "qapi/qmp/qbool.h"
> +#include "qapi/qmp/qint.h"
> +#include "qapi/qmp/qstring.h"
> +#include "sys/user.h"
> +
> +#include <xen/io/ring.h>
> +#include <xen/io/usbif.h>
> +
> +#define TR(fmt, args...)                                            \
> +    {                                                               \
> +        struct timeval tv;                                          \
> +                                                                    \
> +        gettimeofday(&tv, NULL);                                    \
> +        fprintf(stderr, "%8ld.%06ld xen-usb(%s):" fmt, tv.tv_sec,   \
> +                tv.tv_usec, __func__, ##args);                      \

Please use xen_be_printf. I am not sure I would go as far as using
gettimeofday, but I can see it could be useful here.


> +    }
> +#define TR_REQ(fmt, args...) { if (tr_debug & 1) TR(fmt, ##args) }
> +#define TR_BUS(fmt, args...) { if (tr_debug & 2) TR(fmt, ##args) }

I would drop these and just use the loglevels of xen_be_printf.


> +#define USBBACK_MAXPORTS        USBIF_PIPE_PORT_MASK
> +#define USBBACK_DEVNAME_SIZE    32
> +#define USB_DEV_ADDR_SIZE       128
> +
> +struct usbif_ctrlrequest {
> +    uint8_t    bRequestType;
> +    uint8_t    bRequest;
> +    uint16_t   wValue;
> +    uint16_t   wIndex;
> +    uint16_t   wLength;
> +};
> +
> +struct usbif_isoc_descriptor {
> +    uint32_t   offset;
> +    uint32_t   length;
> +    uint32_t   actual_length;
> +    int32_t    status;
> +};
> +
> +struct usbback_info;
> +struct usbback_req;
> +
> +struct usbback_stub {
> +    USBDevice  *dev;
> +    USBPort    port;
> +    unsigned   speed;
> +    bool       attached;
> +    QTAILQ_HEAD(submit_q_head, usbback_req) submit_q;
> +};
> +
> +struct usbback_req {
> +    struct usbback_info      *usbif;
> +    struct usbback_stub      *stub;
> +    struct usbif_urb_request req;
> +    USBPacket                packet;
> +
> +    unsigned int             nr_buffer_segs; /* # of transfer_buffer segments */
> +    unsigned int             nr_extra_segs;  /* # of iso_frame_desc segments  */
> +
> +    QTAILQ_ENTRY(usbback_req) q;
> +
> +    void                     *buffer;
> +    void                     *isoc_buffer;
> +    struct libusb_transfer   *xfer;
> +};
> +
> +struct usbback_info {
> +    struct XenDevice         xendev;  /* must be first */
> +    USBBus                   bus;
> +    void                     *urb_sring;
> +    void                     *conn_sring;
> +    struct usbif_urb_back_ring urb_ring;
> +    struct usbif_conn_back_ring conn_ring;
> +    int                      num_ports;
> +    int                      usb_ver;
> +    bool                     ring_error;
> +    QTAILQ_HEAD(req_free_q_head, usbback_req) req_free_q;
> +    struct usbback_stub      ports[USBBACK_MAXPORTS];
> +    struct usbback_stub      *addr_table[USB_DEV_ADDR_SIZE];
> +    QEMUBH                   *bh;
> +};
> +
> +static unsigned int tr_debug = 3;
> +
> +static void usbback_copy_buffer(struct usbback_req *usbback_req,
> +                                struct usbif_isoc_descriptor *iso, bool out,
> +                                unsigned len, unsigned off)
> +{
> +    struct usbif_request_segment *seg;
> +    unsigned s, offset, copy_len, copy_off;
> +    void *addr;
> +
> +    offset = 0;
> +    copy_off = iso->offset;
> +    for (s = 0; len && s < usbback_req->nr_buffer_segs; s++) {
> +        seg = usbback_req->req.seg + s;
> +        if (offset + seg->length > copy_off) {
> +            addr = usbback_req->buffer + s * PAGE_SIZE + seg->offset +
> +                   copy_off - offset;
> +            copy_len = len;
> +            if (copy_len > seg->length - copy_off + offset) {
> +                copy_len = seg->length - copy_off + offset;
> +            }
> +            if (out) {
> +                memcpy(usbback_req->xfer->buffer + off, addr, copy_len);
> +            } else {
> +                memcpy(addr, usbback_req->xfer->buffer + off, copy_len);
> +            }
> +            len -= copy_len;
> +            off += copy_len;
> +        }
> +        offset += usbback_req->req.seg[s].length;
> +    }
> +    assert(!len);
> +}
> +
> +int usbback_get_packets(USBPacket *p)
> +{
> +    struct usbback_req *usbback_req;
> +
> +    usbback_req = container_of(p, struct usbback_req, packet);
> +    assert(usbif_pipeisoc(usbback_req->req.pipe));
> +    return usbback_req->req.u.isoc.nr_frame_desc_segs;
> +}
> +
> +void usbback_set_iso_desc(USBPacket *p, struct libusb_transfer *xfer)
> +{
> +    struct usbback_req *usbback_req;
> +    struct usbif_isoc_descriptor *iso;
> +    struct usbif_request_segment *seg;
> +    unsigned i, j, np, descr, off;
> +
> +    usbback_req = container_of(p, struct usbback_req, packet);
> +    assert(usbif_pipeisoc(usbback_req->req.pipe));
> +    usbback_req->xfer = xfer;
> +    np = usbback_req->req.u.isoc.nr_frame_desc_segs;
> +    descr = 0;
> +    off = 0;
> +
> +    for (i = 0; i < usbback_req->nr_extra_segs; i++) {
> +        seg = usbback_req->req.seg + usbback_req->nr_buffer_segs + i;
> +        iso = usbback_req->isoc_buffer + i * PAGE_SIZE + seg->offset;
> +        if ((seg->length % sizeof(*iso)) ||
> +            (seg->length / sizeof(*iso) > np - descr) ||
> +            (iso->offset + iso->length > usbback_req->req.buffer_length)) {
> +            xen_be_printf(&usbback_req->usbif->xendev, 0,
> +                          "iso segment length invalid\n");
> +            xfer->num_iso_packets = descr;
> +            while (descr < usbback_req->req.u.isoc.nr_frame_desc_segs) {
> +                xfer->iso_packet_desc[descr].length = 0;
> +                xfer->iso_packet_desc[descr].actual_length = 0;
> +                descr++;
> +            }
> +            return;
> +        }
> +        for (j = 0; j < seg->length / sizeof(*iso); j++) {
> +            xfer->iso_packet_desc[descr].length = iso->length;
> +            xfer->iso_packet_desc[descr].actual_length = 0;
> +            if (!usbif_pipein(usbback_req->req.pipe)) {
> +                usbback_copy_buffer(usbback_req, iso, true, iso->length, off);
> +                off += iso->length;
> +            }
> +            iso++;
> +            descr++;
> +        }
> +    }
> +}
> +
> +static struct usbback_req *usbback_get_req(struct usbback_info *usbif)
> +{
> +    struct usbback_req *usbback_req;
> +
> +    if (QTAILQ_EMPTY(&usbif->req_free_q)) {
> +        usbback_req = g_malloc0(sizeof(*usbback_req));
> +    } else {
> +        usbback_req = QTAILQ_FIRST(&usbif->req_free_q);
> +        QTAILQ_REMOVE(&usbif->req_free_q, usbback_req, q);
> +    }
> +    return usbback_req;
> +}
> +
> +static void usbback_put_req(struct usbback_req *usbback_req)
> +{
> +    struct usbback_info *usbif;
> +
> +    usbif = usbback_req->usbif;
> +    memset(usbback_req, 0, sizeof(*usbback_req));
> +    QTAILQ_INSERT_HEAD(&usbif->req_free_q, usbback_req, q);
> +}
> +
> +static int usbback_gnttab_map(struct usbback_info *usbif,
> +                              struct usbback_req *usbback_req)
> +{
> +    unsigned int nr_segs, i, prot;
> +    uint32_t ref[USBIF_MAX_SEGMENTS_PER_REQUEST];
> +    struct XenDevice *xendev = &usbif->xendev;
> +    struct usbif_request_segment *seg;
> +    void *addr;
> +
> +    nr_segs = usbback_req->nr_buffer_segs + usbback_req->nr_extra_segs;
> +    if (!nr_segs) {
> +        return 0;
> +    }
> +
> +    if (nr_segs > USBIF_MAX_SEGMENTS_PER_REQUEST) {
> +        xen_be_printf(xendev, 0, "bad number of segments in request (%d)\n",
> +                      nr_segs);
> +        return -EINVAL;
> +    }
> +
> +    for (i = 0; i < nr_segs; i++) {
> +        if ((unsigned)usbback_req->req.seg[i].offset +
> +            (unsigned)usbback_req->req.seg[i].length > PAGE_SIZE) {
> +            xen_be_printf(xendev, 0, "segment crosses page boundary\n");
> +            return -EINVAL;
> +        }
> +    }
> +
> +    if (usbback_req->nr_buffer_segs) {
> +        prot = PROT_READ;
> +        if (usbif_pipein(usbback_req->req.pipe)) {
> +                prot |= PROT_WRITE;
> +        }
> +        for (i = 0; i < usbback_req->nr_buffer_segs; i++) {
> +            ref[i] = usbback_req->req.seg[i].gref;
> +        }
> +        usbback_req->buffer = xc_gnttab_map_domain_grant_refs(xendev->gnttabdev,
> +            usbback_req->nr_buffer_segs, xendev->dom, ref, prot);
> +
> +        if (!usbback_req->buffer) {
> +            return -ENOMEM;
> +        }
> +
> +        for (i = 0; i < usbback_req->nr_buffer_segs; i++) {
> +            seg = usbback_req->req.seg + i;
> +            addr = usbback_req->buffer + i * PAGE_SIZE + seg->offset;
> +            qemu_iovec_add(&usbback_req->packet.iov, addr, seg->length);
> +        }
> +    }
> +
> +    if (!usbif_pipeisoc(usbback_req->req.pipe)) {
> +        return 0;
> +    }
> +
> +    if (!usbback_req->nr_extra_segs) {
> +        xen_be_printf(xendev, 0, "iso request without descriptor segments\n");
> +        return -EINVAL;
> +    }
> +
> +    prot = PROT_READ | PROT_WRITE;
> +    for (i = 0; i < usbback_req->nr_extra_segs; i++) {
> +        ref[i] = usbback_req->req.seg[i + usbback_req->req.nr_buffer_segs].gref;
> +    }
> +    usbback_req->isoc_buffer = xc_gnttab_map_domain_grant_refs(
> +         xendev->gnttabdev, usbback_req->nr_extra_segs, xendev->dom, ref, prot);
> +
> +    if (!usbback_req->isoc_buffer) {
> +        return -ENOMEM;
> +    }
> +
> +    return 0;
> +}
> +
> +static int usbback_init_packet(struct usbback_req *usbback_req)
> +{
> +    USBPacket *packet = &usbback_req->packet;
> +    USBDevice *dev = usbback_req->stub->dev;
> +    USBEndpoint *ep;
> +    unsigned int pid, ep_nr;
> +    bool sok;
> +
> +    qemu_iovec_init(&packet->iov, USBIF_MAX_SEGMENTS_PER_REQUEST);
> +    pid = usbif_pipein(usbback_req->req.pipe) ? USB_TOKEN_IN : USB_TOKEN_OUT;
> +    ep_nr = usbif_pipeendpoint(usbback_req->req.pipe);
> +    sok = !!(usbback_req->req.transfer_flags & 1);
> +    if (usbif_pipetype(usbback_req->req.pipe) == USBIF_PIPE_TYPE_CTRL) {
> +        ep_nr = 0;
> +        sok = false;
> +    }
> +    ep = usb_ep_get(dev, pid, ep_nr);
> +    usb_packet_setup(packet, pid, ep, 0, 1, sok, true);
> +
> +    switch (usbif_pipetype(usbback_req->req.pipe)) {
> +    case USBIF_PIPE_TYPE_ISOC:
> +        TR_REQ("iso transfer %s: buflen: %x, %d frames\n",
> +               (pid == USB_TOKEN_IN) ? "in" : "out",
> +               usbback_req->req.buffer_length,
> +               usbback_req->req.u.isoc.nr_frame_desc_segs);
> +        break;
> +
> +    case USBIF_PIPE_TYPE_INT:
> +        TR_REQ("int transfer %s: buflen: %x\n",
> +               (pid == USB_TOKEN_IN) ? "in" : "out",
> +               usbback_req->req.buffer_length);
> +        break;
> +
> +    case USBIF_PIPE_TYPE_CTRL:
> +        packet->parameter = *(uint64_t *)usbback_req->req.u.ctrl;
> +        TR_REQ("ctrl parameter: %lx, buflen: %x\n", packet->parameter,
> +               usbback_req->req.buffer_length);
> +        break;
> +
> +    case USBIF_PIPE_TYPE_BULK:
> +        TR_REQ("bulk transfer %s: buflen: %x\n",
> +               (pid == USB_TOKEN_IN) ? "in" : "out",
> +               usbback_req->req.buffer_length);
> +        break;
> +    default:
> +        return -EINVAL;
> +    }
> +
> +    return 0;
> +}
> +
> +static void usbback_do_response(struct usbback_req *usbback_req,
> +                                int32_t status, int32_t actual_length,
> +                                int32_t error_count)
> +{
> +    struct usbback_info *usbif;
> +    struct usbif_urb_response *res;
> +    struct XenDevice *xendev;
> +    unsigned int notify;
> +
> +    TR_REQ("id %d, status %d, length %d, errcnt %d\n", usbback_req->req.id,
> +           status, actual_length, error_count);
> +
> +    usbif = usbback_req->usbif;
> +    xendev = &usbif->xendev;
> +
> +    if (usbback_req->packet.iov.iov) {
> +        qemu_iovec_destroy(&usbback_req->packet.iov);
> +    }
> +
> +    if (usbback_req->buffer) {
> +        xc_gnttab_munmap(xendev->gnttabdev, usbback_req->buffer,
> +                         usbback_req->nr_buffer_segs);
> +        usbback_req->buffer = NULL;
> +    }
> +
> +    if (usbback_req->isoc_buffer) {
> +        xc_gnttab_munmap(xendev->gnttabdev, usbback_req->isoc_buffer,
> +                         usbback_req->nr_extra_segs);
> +    }
> +
> +    res = RING_GET_RESPONSE(&usbif->urb_ring, usbif->urb_ring.rsp_prod_pvt);
> +    res->id = usbback_req->req.id;
> +    res->status = status;
> +    res->actual_length = actual_length;
> +    res->error_count = error_count;
> +    res->start_frame = 0;
> +    usbif->urb_ring.rsp_prod_pvt++;
> +    RING_PUSH_RESPONSES_AND_CHECK_NOTIFY(&usbif->urb_ring, notify);
> +
> +    if (notify) {
> +        xen_be_send_notify(xendev);
> +    }
> +
> +    usbback_put_req(usbback_req);
> +}
> +
> +static void usbback_do_response_ret(struct usbback_req *usbback_req,
> +                                    int32_t status)
> +{
> +    usbback_do_response(usbback_req, status, 0, 0);
> +}
> +
> +static int32_t usbback_xlat_status(int32_t status)
> +{
> +    int32_t ret = -ESHUTDOWN;
> +
> +    switch (status) {
> +    case USB_RET_SUCCESS:
> +        ret = 0;
> +        break;
> +    case USB_RET_NODEV:
> +        ret = -ENODEV;
> +        break;
> +    case USB_RET_STALL:
> +        ret = -EPIPE;
> +        break;
> +    case USB_RET_BABBLE:
> +        ret = -EOVERFLOW;
> +        break;
> +    case USB_RET_IOERROR:
> +        ret = -EPROTO;
> +        break;
> +    }
> +
> +    return ret;
> +}
> +
> +static void usbback_packet_complete(USBPacket *packet)
> +{
> +    struct usbback_req *usbback_req;
> +    int32_t status, error_count, actual_length;
> +    unsigned int i, j, off;
> +    struct usbif_isoc_descriptor *iso;
> +    struct usbif_request_segment *seg;
> +    struct libusb_iso_packet_descriptor *libusb_iso;
> +
> +    error_count = 0;
> +    actual_length = 0;
> +    off = 0;
> +    usbback_req = container_of(packet, struct usbback_req, packet);
> +
> +    QTAILQ_REMOVE(&usbback_req->stub->submit_q, usbback_req, q);
> +
> +    status = usbback_xlat_status(packet->status);
> +    if (usbback_req->isoc_buffer) {
> +        libusb_iso = usbback_req->xfer->iso_packet_desc;
> +        j = 0;
> +        for (i = 0; i < usbback_req->nr_extra_segs; i++) {
> +            seg = usbback_req->req.seg + usbback_req->nr_buffer_segs + i;
> +            iso = usbback_req->isoc_buffer + i * PAGE_SIZE + seg->offset;
> +            for (j = 0; j < seg->length / sizeof(*iso); j++) {
> +                iso->actual_length = libusb_iso->actual_length;
> +                iso->status = usbback_xlat_status(libusb_iso->status);
> +                actual_length += libusb_iso->actual_length;
> +                error_count += iso->status ? 1 : 0;
> +                if (usbif_pipein(usbback_req->req.pipe)) {
> +                    usbback_copy_buffer(usbback_req, iso, false,
> +                                        iso->actual_length, off);
> +                    off += iso->length;
> +                }
> +                libusb_iso++;
> +                iso++;
> +            }
> +        }
> +    } else {
> +        actual_length = packet->actual_length;
> +    }
> +
> +    usbback_do_response(usbback_req, status, actual_length, error_count);
> +}
> +
> +static void usbback_set_address(struct usbback_info *usbif,
> +                                struct usbback_stub *stub, int cur_addr,
> +                                int new_addr)
> +{
> +    if (cur_addr) {
> +        usbif->addr_table[cur_addr] = NULL;
> +    }
> +    if (new_addr) {
> +        usbif->addr_table[new_addr] = stub;
> +    }
> +}
> +
> +static bool usbback_cancel_req(struct usbback_req *usbback_req)
> +{
> +    bool ret = false;
> +
> +    if (usb_packet_is_inflight(&usbback_req->packet)) {
> +        usb_cancel_packet(&usbback_req->packet);
> +        ret = true;
> +    }
> +    return ret;
> +}
> +
> +static void usbback_process_unlink_req(struct usbback_req *usbback_req)
> +{
> +    struct usbback_info *usbif;
> +    struct usbback_req *unlink_req;
> +    unsigned int id, devnum;
> +    int ret;
> +
> +    usbif = usbback_req->usbif;
> +    ret = 0;
> +    id = usbback_req->req.u.unlink.unlink_id;
> +    TR_REQ("unlink id %d\n", id);
> +    devnum = usbif_pipedevice(usbback_req->req.pipe);
> +    if (unlikely(devnum == 0)) {
> +        usbback_req->stub = usbif->ports +
> +                            usbif_pipeportnum(usbback_req->req.pipe);
> +        if (unlikely(!usbback_req->stub)) {
> +            ret = -ENODEV;
> +            goto fail_response;
> +        }
> +    } else {
> +        if (unlikely(!usbif->addr_table[devnum])) {
> +            ret = -ENODEV;
> +            goto fail_response;
> +        }
> +        usbback_req->stub = usbif->addr_table[devnum];
> +    }
> +
> +    QTAILQ_FOREACH(unlink_req, &usbback_req->stub->submit_q, q) {
> +        if (unlink_req->req.id == id) {
> +            if (usbback_cancel_req(unlink_req)) {
> +                usbback_do_response_ret(unlink_req, -EPROTO);
> +            }
> +            break;
> +        }
> +    }
> +
> +fail_response:
> +    usbback_do_response_ret(usbback_req, ret);
> +}
> +
> +static int usbback_check_and_submit(struct usbback_req *usbback_req)
> +{
> +    struct usbback_info *usbif;
> +    unsigned int devnum;
> +    struct usbback_stub *stub;
> +    struct usbif_ctrlrequest *ctrl;
> +    int ret;
> +    uint16_t wValue;
> +
> +    usbif = usbback_req->usbif;
> +    stub = NULL;
> +    devnum = usbif_pipedevice(usbback_req->req.pipe);
> +    ctrl = (struct usbif_ctrlrequest *)usbback_req->req.u.ctrl;
> +    wValue = le16_to_cpu(ctrl->wValue);
> +
> +    /*
> +     * When the device is first connected or resetted, USB device has no
> +     * address. In this initial state, following requests are sent to device
> +     * address (#0),
> +     *
> +     *  1. GET_DESCRIPTOR (with Descriptor Type is "DEVICE") is sent,
> +     *     and OS knows what device is connected to.
> +     *
> +     *  2. SET_ADDRESS is sent, and then device has its address.
> +     *
> +     * In the next step, SET_CONFIGURATION is sent to addressed device, and
> +     * then the device is finally ready to use.
> +     */
> +    if (unlikely(devnum == 0)) {
> +        stub = usbif->ports + usbif_pipeportnum(usbback_req->req.pipe) - 1;
> +        if (!stub->dev || !stub->attached) {
> +            ret = -ENODEV;
> +            goto do_response;
> +        }
> +
> +        switch (ctrl->bRequest) {
> +        case USB_REQ_GET_DESCRIPTOR:
> +            /*
> +             * GET_DESCRIPTOR request to device #0.
> +             * through to normal transfer.
> +             */
> +            TR_REQ("devnum 0 GET_DESCRIPTOR\n");
> +            usbback_req->stub = stub;
> +            return 0;
> +        case USB_REQ_SET_ADDRESS:
> +            /*
> +             * SET_ADDRESS request to device #0.
> +             * add attached device to addr_table.
> +             */
> +            TR_REQ("devnum 0 SET_ADDRESS\n");
> +            usbback_set_address(usbif, stub, 0, wValue);
> +            ret = 0;
> +            break;
> +        default:
> +            ret = -EINVAL;
> +            break;
> +        }
> +        goto do_response;
> +    }
> +
> +    if (unlikely(!usbif->addr_table[devnum])) {
> +            ret = -ENODEV;
> +            goto do_response;
> +    }
> +    usbback_req->stub = usbif->addr_table[devnum];
> +
> +    /*
> +     * Check special request
> +     */
> +    if (ctrl->bRequest != USB_REQ_SET_ADDRESS) {
> +        return 0;
> +    }
> +
> +    /*
> +     * SET_ADDRESS request to addressed device.
> +     * change addr or remove from addr_table.
> +     */
> +    usbback_set_address(usbif, usbback_req->stub, devnum, wValue);
> +    ret = 0;
> +
> +do_response:
> +    usbback_do_response_ret(usbback_req, ret);
> +    return 1;
> +}
> +
> +static void usbback_dispatch(struct usbback_req *usbback_req)
> +{
> +    int ret;
> +    unsigned int devnum;
> +    struct usbback_info *usbif;
> +
> +    TR_REQ("start req_id %d pipe %08x\n", usbback_req->req.id,
> +           usbback_req->req.pipe);
> +
> +    usbif = usbback_req->usbif;
> +
> +    /* unlink request */
> +    if (unlikely(usbif_pipeunlink(usbback_req->req.pipe))) {
> +        usbback_process_unlink_req(usbback_req);
> +        return;
> +    }
> +
> +    if (usbif_pipectrl(usbback_req->req.pipe)) {
> +        if (usbback_check_and_submit(usbback_req)) {
> +            return;
> +        }
> +    } else {
> +        devnum = usbif_pipedevice(usbback_req->req.pipe);
> +        usbback_req->stub = usbif->addr_table[devnum];
> +
> +        if (!usbback_req->stub || !usbback_req->stub->attached) {
> +            ret = -ENODEV;
> +            goto fail_response;
> +        }
> +    }
> +
> +    QTAILQ_INSERT_TAIL(&usbback_req->stub->submit_q, usbback_req, q);
> +
> +    usbback_req->nr_buffer_segs = usbback_req->req.nr_buffer_segs;
> +    usbback_req->nr_extra_segs = usbif_pipeisoc(usbback_req->req.pipe) ?
> +                                 usbback_req->req.u.isoc.nr_frame_desc_segs : 0;
> +
> +    ret = usbback_init_packet(usbback_req);
> +    if (ret) {
> +        xen_be_printf(&usbif->xendev, 0, "invalid request\n");
> +        ret = -ESHUTDOWN;
> +        goto fail_free_urb;
> +    }
> +
> +    ret = usbback_gnttab_map(usbif, usbback_req);
> +    if (ret) {
> +        xen_be_printf(&usbif->xendev, 0, "invalid buffer\n");
> +        ret = -ESHUTDOWN;
> +        goto fail_free_urb;
> +    }
> +
> +    usb_handle_packet(usbback_req->stub->dev, &usbback_req->packet);
> +    if (usbback_req->packet.status != USB_RET_ASYNC) {
> +        usbback_packet_complete(&usbback_req->packet);
> +    }
> +    return;
> +
> +fail_free_urb:
> +    QTAILQ_REMOVE(&usbback_req->stub->submit_q, usbback_req, q);
> +
> +fail_response:
> +    usbback_do_response_ret(usbback_req, ret);
> +}
> +
> +static void usbback_bh(void *opaque)
> +{
> +    struct usbback_info *usbif;
> +    struct usbif_urb_back_ring *urb_ring;
> +    struct usbback_req *usbback_req;
> +    RING_IDX rc, rp;
> +    unsigned int more_to_do;
> +
> +    usbif = opaque;
> +    if (usbif->ring_error) {
> +        return;
> +    }
> +
> +    urb_ring = &usbif->urb_ring;
> +    rc = urb_ring->req_cons;
> +    rp = urb_ring->sring->req_prod;
> +    xen_rmb(); /* Ensure we see queued requests up to 'rp'. */
> +
> +    if (RING_REQUEST_PROD_OVERFLOW(urb_ring, rp)) {
> +        rc = urb_ring->rsp_prod_pvt;
> +        xen_be_printf(&usbif->xendev, 0, "domU provided bogus ring requests "
> +                      "(%#x - %#x = %u). Halting ring processing.\n",
> +                      rp, rc, rp - rc);
> +        usbif->ring_error = true;
> +        return;
> +    }
> +
> +    while (rc != rp) {
> +        if (RING_REQUEST_CONS_OVERFLOW(urb_ring, rc)) {
> +            break;
> +        }
> +        usbback_req = usbback_get_req(usbif);
> +
> +        usbback_req->req = *RING_GET_REQUEST(urb_ring, rc);
> +        usbback_req->usbif = usbif;
> +
> +        usbback_dispatch(usbback_req);
> +
> +        urb_ring->req_cons = ++rc;
> +    }
> +
> +    RING_FINAL_CHECK_FOR_REQUESTS(urb_ring, more_to_do);
> +    if (more_to_do) {
> +        qemu_bh_schedule(usbif->bh);
> +    }
> +}
> +
> +static void usbback_hotplug_notify(struct usbback_info *usbif, unsigned port)
> +{
> +    struct usbif_conn_back_ring *ring = &usbif->conn_ring;
> +    struct usbif_conn_request *req;
> +    struct usbif_conn_response *res;
> +    uint16_t id;
> +    unsigned int notify;
> +
> +    if (!usbif->conn_sring) {
> +        return;
> +    }
> +
> +    req = RING_GET_REQUEST(ring, ring->req_cons);
> +    id = req->id;
> +    ring->req_cons++;
> +    ring->sring->req_event = ring->req_cons + 1;
> +
> +    res = RING_GET_RESPONSE(ring, ring->rsp_prod_pvt);
> +    res->id = id;
> +    res->portnum = port;
> +    res->speed = usbif->ports[port - 1].speed;
> +    ring->rsp_prod_pvt++;
> +    RING_PUSH_RESPONSES_AND_CHECK_NOTIFY(ring, notify);
> +
> +    if (notify) {
> +        xen_be_send_notify(&usbif->xendev);
> +    }
> +
> +    TR_BUS("hotplug port %d speed %d\n", port, res->speed);
> +}
> +static void usbback_portid_remove(struct usbback_info *usbif, unsigned port)
> +{
> +    USBPort *p;
> +
> +    if (!usbif->ports[port - 1].dev) {
> +        return;
> +    }
> +
> +    p = &(usbif->ports[port - 1].port);
> +    snprintf(p->path, sizeof(p->path), "%d", 99);
> +
> +    object_unparent(OBJECT(usbif->ports[port - 1].dev));
> +    usbif->ports[port - 1].dev = NULL;
> +    usbif->ports[port - 1].speed = USBIF_SPEED_NONE;
> +    usbif->ports[port - 1].attached = false;
> +    usbback_hotplug_notify(usbif, port);
> +
> +    TR_BUS("port %d removed\n", port);
> +}
> +
> +static void usbback_portid_add(struct usbback_info *usbif, unsigned port,
> +                               char *busid)
> +{
> +    unsigned speed;
> +    char *portname;
> +    USBPort *p;
> +    Error *local_err = NULL;
> +    QDict *qdict;
> +    QemuOpts *opts;
> +
> +    if (usbif->ports[port - 1].dev) {
> +        return;
> +    }
> +
> +    portname = strchr(busid, '-');
> +    if (!portname) {
> +        xen_be_printf(&usbif->xendev, 0, "device %s illegal specification\n",
> +                      busid);
> +        return;
> +    }
> +    portname++;
> +    p = &(usbif->ports[port - 1].port);
> +    snprintf(p->path, sizeof(p->path), "%s", portname);
> +
> +    qdict = qdict_new();
> +    qdict_put(qdict, "driver", qstring_from_str("usb-host"));
> +    qdict_put(qdict, "hostbus", qint_from_int(atoi(busid)));
> +    qdict_put(qdict, "hostport", qstring_from_str(portname));
> +    qdict_put(qdict, "xen-iso-passthrough", qbool_from_int(1));
> +    opts = qemu_opts_from_qdict(qemu_find_opts("device"), qdict, &local_err);
> +    if (local_err) {
> +        goto err;
> +    }
> +    usbif->ports[port - 1].dev = USB_DEVICE(qdev_device_add(opts));
> +    if (!usbif->ports[port - 1].dev) {
> +        goto err;
> +    }
> +    QDECREF(qdict);
> +    snprintf(p->path, sizeof(p->path), "%d", port);
> +    speed = usbif->ports[port - 1].dev->speed;
> +    switch (speed) {
> +    case USB_SPEED_LOW:
> +        speed = USBIF_SPEED_LOW;
> +        break;
> +    case USB_SPEED_FULL:
> +        speed = USBIF_SPEED_FULL;
> +        break;
> +    case USB_SPEED_HIGH:
> +        speed = (usbif->usb_ver < USB_VER_USB20) ?
> +                USBIF_SPEED_NONE : USBIF_SPEED_HIGH;
> +        break;
> +    default:
> +        speed = USBIF_SPEED_NONE;
> +        break;
> +    }
> +    if (speed == USBIF_SPEED_NONE) {
> +        xen_be_printf(&usbif->xendev, 0, "device %s wrong speed\n", busid);
> +        object_unparent(OBJECT(usbif->ports[port - 1].dev));
> +        usbif->ports[port - 1].dev = NULL;
> +        return;
> +    }
> +    usb_device_reset(usbif->ports[port - 1].dev);
> +    usbif->ports[port - 1].speed = speed;
> +    usbif->ports[port - 1].attached = true;
> +    QTAILQ_INIT(&usbif->ports[port - 1].submit_q);
> +    usbback_hotplug_notify(usbif, port);
> +
> +    TR_BUS("port %d attached\n", port);
> +    return;
> +
> +err:
> +    QDECREF(qdict);
> +    snprintf(p->path, sizeof(p->path), "%d", 99);
> +    xen_be_printf(&usbif->xendev, 0, "device %s could not be opened\n", busid);
> +}
> +
> +static void usbback_process_port(struct usbback_info *usbif, unsigned port)
> +{
> +    char node[8];
> +    char *busid;
> +
> +    snprintf(node, sizeof(node), "port/%d", port);
> +    busid = xenstore_read_be_str(&usbif->xendev, node);
> +    if (busid == NULL) {
> +        xen_be_printf(&usbif->xendev, 0, "xenstore_read %s failed\n", node);
> +        return;
> +    }
> +
> +    /* Remove portid, if the port is not connected.  */
> +    if (strlen(busid) == 0) {
> +        usbback_portid_remove(usbif, port);
> +    } else {
> +        usbback_portid_add(usbif, port, busid);
> +    }
> +
> +    g_free(busid);
> +}
> +
> +static void usbback_disconnect(struct XenDevice *xendev)
> +{
> +    struct usbback_info *usbif;
> +    struct usbback_req *req, *tmp;
> +    unsigned int i;
> +
> +    TR_BUS("start\n");
> +
> +    usbif = container_of(xendev, struct usbback_info, xendev);
> +
> +    xen_be_unbind_evtchn(xendev);
> +
> +    if (usbif->urb_sring) {
> +        xc_gnttab_munmap(xendev->gnttabdev, usbif->urb_sring, 1);
> +        usbif->urb_sring = NULL;
> +    }
> +    if (usbif->conn_sring) {
> +        xc_gnttab_munmap(xendev->gnttabdev, usbif->conn_sring, 1);
> +        usbif->conn_sring = NULL;
> +    }
> +
> +    for (i = 0; i < usbif->num_ports; i++) {
> +        if (!usbif->ports[i].dev) {
> +            continue;
> +        }
> +        QTAILQ_FOREACH_SAFE(req, &usbif->ports[i].submit_q, q, tmp) {
> +            usbback_cancel_req(req);
> +        }
> +    }
> +
> +    TR_BUS("finished\n");
> +}
> +
> +static int usbback_connect(struct XenDevice *xendev)
> +{
> +    struct usbback_info *usbif;
> +    struct usbif_urb_sring *urb_sring;
> +    struct usbif_conn_sring *conn_sring;
> +    int urb_ring_ref;
> +    int conn_ring_ref;
> +    unsigned int i;
> +
> +    TR_BUS("start\n");
> +
> +    usbif = container_of(xendev, struct usbback_info, xendev);
> +
> +    if (xenstore_read_fe_int(xendev, "urb-ring-ref", &urb_ring_ref)) {
> +        TR("error reading urb-ring-ref\n");
> +        return -1;
> +    }
> +    if (xenstore_read_fe_int(xendev, "conn-ring-ref", &conn_ring_ref)) {
> +        TR("error reading conn-ring-ref\n");
> +        return -1;
> +    }
> +    if (xenstore_read_fe_int(xendev, "event-channel", &xendev->remote_port)) {
> +        TR("error reading event-channel\n");
> +        return -1;
> +    }
> +
> +    usbif->urb_sring = xc_gnttab_map_grant_ref(xendev->gnttabdev, xendev->dom,
> +                                               urb_ring_ref,
> +                                               PROT_READ | PROT_WRITE);
> +    usbif->conn_sring = xc_gnttab_map_grant_ref(xendev->gnttabdev, xendev->dom,
> +                                                conn_ring_ref,
> +                                                PROT_READ | PROT_WRITE);
> +    if (!usbif->urb_sring || !usbif->conn_sring) {
> +        TR("error mapping rings\n");
> +        usbback_disconnect(xendev);
> +        return -1;
> +    }
> +
> +    urb_sring = usbif->urb_sring;
> +    conn_sring = usbif->conn_sring;
> +    BACK_RING_INIT(&usbif->urb_ring, urb_sring, XC_PAGE_SIZE);
> +    BACK_RING_INIT(&usbif->conn_ring, conn_sring, XC_PAGE_SIZE);
> +
> +    xen_be_bind_evtchn(xendev);
> +
> +    xen_be_printf(xendev, 1, "urb-ring-ref %d, conn-ring-ref %d, "
> +                  "remote port %d, local port %d\n", urb_ring_ref,
> +                  conn_ring_ref, xendev->remote_port, xendev->local_port);
> +
> +    for (i = 1; i <= usbif->num_ports; i++) {
> +        if (usbif->ports[i - 1].dev) {
> +            usbback_hotplug_notify(usbif, i);
> +        }
> +    }
> +
> +    return 0;
> +}
> +
> +static void usbback_backend_changed(struct XenDevice *xendev, const char *node)
> +{
> +    struct usbback_info *usbif;
> +    unsigned int i;
> +
> +    TR_BUS("path %s\n", node);
> +
> +    usbif = container_of(xendev, struct usbback_info, xendev);
> +    for (i = 1; i <= usbif->num_ports; i++) {
> +        usbback_process_port(usbif, i);
> +    }
> +}
> +
> +static int usbback_init(struct XenDevice *xendev)
> +{
> +    struct usbback_info *usbif;
> +
> +    TR_BUS("start\n");
> +
> +    usbif = container_of(xendev, struct usbback_info, xendev);
> +
> +    if (xenstore_read_be_int(xendev, "num-ports", &usbif->num_ports) ||
> +        usbif->num_ports < 1 || usbif->num_ports > USBBACK_MAXPORTS) {
> +        xen_be_printf(xendev, 0, "num-ports not readable or out of bounds\n");
> +        return -1;
> +    }
> +    if (xenstore_read_be_int(xendev, "usb-ver", &usbif->usb_ver) ||
> +        (usbif->usb_ver != USB_VER_USB11 && usbif->usb_ver != USB_VER_USB20)) {
> +        xen_be_printf(xendev, 0, "usb-ver not readable or out of bounds\n");
> +        return -1;
> +    }
> +
> +    usbback_backend_changed(xendev, "port");
> +
> +    TR_BUS("finished\n");
> +
> +    return 0;
> +}
> +
> +static void xen_bus_attach(USBPort *port)
> +{
> +    struct usbback_info *usbif;
> +
> +    TR_BUS("called\n");
> +    usbif = port->opaque;
> +    usbif->ports[port->index].attached = true;
> +    usbback_hotplug_notify(usbif, port->index + 1);
> +}
> +
> +static void xen_bus_detach(USBPort *port)
> +{
> +    struct usbback_info *usbif;
> +
> +    TR_BUS("called\n");
> +    usbif = port->opaque;
> +    usbif->ports[port->index].attached = false;
> +    usbback_hotplug_notify(usbif, port->index + 1);
> +}
> +
> +static void xen_bus_child_detach(USBPort *port, USBDevice *child)
> +{
> +    TR_BUS("called\n");
> +}
> +
> +static void xen_bus_complete(USBPort *port, USBPacket *packet)
> +{
> +    TR_REQ("called\n");

Could you please either remove these debug messages or make them more
informative.


> +    usbback_packet_complete(packet);
> +}
> +
> +static USBPortOps xen_usb_port_ops = {
> +    .attach = xen_bus_attach,
> +    .detach = xen_bus_detach,
> +    .child_detach = xen_bus_child_detach,
> +    .complete = xen_bus_complete,
> +};
> +
> +static USBBusOps xen_usb_bus_ops = {
> +};
> +
> +static void usbback_alloc(struct XenDevice *xendev)
> +{
> +    struct usbback_info *usbif;
> +    USBPort *p;
> +    unsigned int i, max_grants;
> +
> +    usbif = container_of(xendev, struct usbback_info, xendev);
> +
> +    usb_bus_new(&usbif->bus, sizeof(usbif->bus), &xen_usb_bus_ops, xen_sysdev);
> +    for (i = 0; i < USBBACK_MAXPORTS; i++) {
> +        p = &(usbif->ports[i].port);
> +        usb_register_port(&usbif->bus, p, usbif, i, &xen_usb_port_ops,
> +                          USB_SPEED_MASK_LOW | USB_SPEED_MASK_FULL |
> +                          USB_SPEED_MASK_HIGH);
> +        snprintf(p->path, sizeof(p->path), "%d", 99);
> +    }
> +
> +    QTAILQ_INIT(&usbif->req_free_q);
> +    usbif->bh = qemu_bh_new(usbback_bh, usbif);
> +
> +    max_grants = USBIF_MAX_SEGMENTS_PER_REQUEST * USB_URB_RING_SIZE + 2;
> +    if (xc_gnttab_set_max_grants(xendev->gnttabdev, max_grants) < 0) {
> +        xen_be_printf(xendev, 0, "xc_gnttab_set_max_grants failed: %s\n",
> +                      strerror(errno));
> +    }
> +}
> +
> +static int usbback_free(struct XenDevice *xendev)
> +{
> +    struct usbback_info *usbif;
> +    struct usbback_req *usbback_req;
> +    unsigned int i;
> +
> +    TR_BUS("start\n");
> +
> +    usbback_disconnect(xendev);
> +    usbif = container_of(xendev, struct usbback_info, xendev);
> +    for (i = 1; i <= usbif->num_ports; i++) {
> +        usbback_portid_remove(usbif, i);
> +    }
> +
> +    while (!QTAILQ_EMPTY(&usbif->req_free_q)) {
> +        usbback_req = QTAILQ_FIRST(&usbif->req_free_q);
> +        QTAILQ_REMOVE(&usbif->req_free_q, usbback_req, q);
> +        g_free(usbback_req);
> +    }
> +
> +    qemu_bh_delete(usbif->bh);
> +
> +    usb_bus_release(&usbif->bus);
> +
> +    TR_BUS("finished\n");
> +
> +    return 0;
> +}
> +
> +static void usbback_event(struct XenDevice *xendev)
> +{
> +    struct usbback_info *usbif;
> +
> +    usbif = container_of(xendev, struct usbback_info, xendev);
> +    qemu_bh_schedule(usbif->bh);
> +}
> +
> +struct XenDevOps xen_usb_ops = {
> +    .size            = sizeof(struct usbback_info),
> +    .flags           = DEVOPS_FLAG_NEED_GNTDEV,
> +    .init            = usbback_init,
> +    .alloc           = usbback_alloc,
> +    .free            = usbback_free,
> +    .backend_changed = usbback_backend_changed,
> +    .initialise      = usbback_connect,
> +    .disconnect      = usbback_disconnect,
> +    .event           = usbback_event,
> +};

Re: [Qemu-devel] [Xen-devel] [Patch V1 3/3] xen: add pvUSB backend

[Juergen Gross]

On 09/07/2015 07:38 PM, Stefano Stabellini wrote:
> On Thu, 3 Sep 2015, Juergen Gross wrote:
>> Add a backend for para-virtualized USB devices for xen domains.
>>
>> The backend is using host-libusb to forward USB requests from a
>> domain via libusb to the real device(s) passed through.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>
> Aside from a few minor comments below, this looks pretty good from a Xen
> perspective, however I am not too qualified to review the details of the
> pvusb protocol or the way the requests are handled internally in QEMU.
>
> I would be glad if Gerd could take a look at this too.
>
> Juergen, if we commit this, would you be happy to maintain it going
> forward?

Sure.

>> diff --git a/hw/usb/xen-usb.c b/hw/usb/xen-usb.c
>> new file mode 100644
>> index 0000000..2570bd7
>> --- /dev/null
>> +++ b/hw/usb/xen-usb.c
...
>> +#define TR(fmt, args...)                                            \
>> +    {                                                               \
>> +        struct timeval tv;                                          \
>> +                                                                    \
>> +        gettimeofday(&tv, NULL);                                    \
>> +        fprintf(stderr, "%8ld.%06ld xen-usb(%s):" fmt, tv.tv_sec,   \
>> +                tv.tv_usec, __func__, ##args);                      \
>
> Please use xen_be_printf. I am not sure I would go as far as using
> gettimeofday, but I can see it could be useful here.

Okay. I'll keep the time information as it has been proved to be really
valuable.

>> +    }
>> +#define TR_REQ(fmt, args...) { if (tr_debug & 1) TR(fmt, ##args) }
>> +#define TR_BUS(fmt, args...) { if (tr_debug & 2) TR(fmt, ##args) }
>
> I would drop these and just use the loglevels of xen_be_printf.

Hmm, something like:

TR     -> level 1
TR_BUS -> level 2
TR_REQ -> level 3

Is it possible to control trace levels per device? If not, trying to
catch an error requiring to trace on request level might be hard as
concurrent qdisk trace entries could affect timing severely.

...
>> +static void xen_bus_child_detach(USBPort *port, USBDevice *child)
>> +{
>> +    TR_BUS("called\n");
>> +}
>> +
>> +static void xen_bus_complete(USBPort *port, USBPacket *packet)
>> +{
>> +    TR_REQ("called\n");
>
> Could you please either remove these debug messages or make them more
> informative.

Which information are you missing? As long as the TR_* macros aren't
changed they'll trace the function as well. In the context of other
traces written they have been completely valuable.


Juergen

Re: [Qemu-devel] [Xen-devel] [Patch V1 3/3] xen: add pvUSB backend

[Stefano Stabellini]

On Wed, 9 Sep 2015, Juergen Gross wrote:
> On 09/07/2015 07:38 PM, Stefano Stabellini wrote:
> > On Thu, 3 Sep 2015, Juergen Gross wrote:
> > > Add a backend for para-virtualized USB devices for xen domains.
> > > 
> > > The backend is using host-libusb to forward USB requests from a
> > > domain via libusb to the real device(s) passed through.
> > > 
> > > Signed-off-by: Juergen Gross <jgross@suse.com>
> > 
> > Aside from a few minor comments below, this looks pretty good from a Xen
> > perspective, however I am not too qualified to review the details of the
> > pvusb protocol or the way the requests are handled internally in QEMU.
> > 
> > I would be glad if Gerd could take a look at this too.
> > 
> > Juergen, if we commit this, would you be happy to maintain it going
> > forward?
> 
> Sure.
> 
> > > diff --git a/hw/usb/xen-usb.c b/hw/usb/xen-usb.c
> > > new file mode 100644
> > > index 0000000..2570bd7
> > > --- /dev/null
> > > +++ b/hw/usb/xen-usb.c
> ...
> > > +#define TR(fmt, args...)                                            \
> > > +    {                                                               \
> > > +        struct timeval tv;                                          \
> > > +                                                                    \
> > > +        gettimeofday(&tv, NULL);                                    \
> > > +        fprintf(stderr, "%8ld.%06ld xen-usb(%s):" fmt, tv.tv_sec,   \
> > > +                tv.tv_usec, __func__, ##args);                      \
> > 
> > Please use xen_be_printf. I am not sure I would go as far as using
> > gettimeofday, but I can see it could be useful here.
> 
> Okay. I'll keep the time information as it has been proved to be really
> valuable.
> 
> > > +    }
> > > +#define TR_REQ(fmt, args...) { if (tr_debug & 1) TR(fmt, ##args) }
> > > +#define TR_BUS(fmt, args...) { if (tr_debug & 2) TR(fmt, ##args) }
> > 
> > I would drop these and just use the loglevels of xen_be_printf.
> 
> Hmm, something like:
> 
> TR     -> level 1
> TR_BUS -> level 2
> TR_REQ -> level 3
> 
> Is it possible to control trace levels per device?

Yes, with xendev->debug.


> If not, trying to
> catch an error requiring to trace on request level might be hard as
> concurrent qdisk trace entries could affect timing severely.
> 
> ...
> > > +static void xen_bus_child_detach(USBPort *port, USBDevice *child)
> > > +{
> > > +    TR_BUS("called\n");
> > > +}
> > > +
> > > +static void xen_bus_complete(USBPort *port, USBPacket *packet)
> > > +{
> > > +    TR_REQ("called\n");
> > 
> > Could you please either remove these debug messages or make them more
> > informative.
> 
> Which information are you missing? As long as the TR_* macros aren't
> changed they'll trace the function as well. In the context of other
> traces written they have been completely valuable.

I am saying that "called" is not very informative as a debug message,
you might as well remove it and rearrage the calls to be just:

TS_BUS();

But if you want to trace the call, maybe you could use the trace_*
framework?

Re: [Qemu-devel] [Xen-devel] [Patch V1 3/3] xen: add pvUSB backend

[Konrad Rzeszutek Wilk]

On Thu, Sep 03, 2015 at 12:45:13PM +0200, Juergen Gross wrote:
> Add a backend for para-virtualized USB devices for xen domains.
> 
> The backend is using host-libusb to forward USB requests from a
> domain via libusb to the real device(s) passed through.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
>  hw/usb/Makefile.objs         |    4 +
>  hw/usb/xen-usb.c             | 1120 ++++++++++++++++++++++++++++++++++++++++++
>  hw/xenpv/xen_machine_pv.c    |    3 +
>  include/hw/xen/xen_backend.h |   13 +-
>  4 files changed, 1137 insertions(+), 3 deletions(-)
>  create mode 100644 hw/usb/xen-usb.c
> 
> diff --git a/hw/usb/Makefile.objs b/hw/usb/Makefile.objs
> index 3fe4dff..0253184 100644
> --- a/hw/usb/Makefile.objs
> +++ b/hw/usb/Makefile.objs
> @@ -36,3 +36,7 @@ common-obj-$(CONFIG_USB_REDIR) += redirect.o quirks.o
>  
>  # usb pass-through
>  common-obj-y += $(patsubst %,host-%.o,$(HOST_USB))
> +
> +ifeq ($(CONFIG_USB_LIBUSB),y)
> +common-obj-$(CONFIG_XEN_BACKEND) += xen-usb.o
> +endif
> diff --git a/hw/usb/xen-usb.c b/hw/usb/xen-usb.c
> new file mode 100644
> index 0000000..2570bd7
> --- /dev/null
> +++ b/hw/usb/xen-usb.c
> @@ -0,0 +1,1120 @@
> +/*
> + *  xen paravirt usb device backend
> + *
> + *  (c) Juergen Gross <jgross@suse.com>
> + *
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation; under version 2 of the License.
> + *
> + *  This program is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License along
> + *  with this program; if not, see <http://www.gnu.org/licenses/>.
> + *
> + *  Contributions after 2012-01-13 are licensed under the terms of the
> + *  GNU GPL, version 2 or (at your option) any later version.
> + */
> +
> +#include <libusb.h>
> +#include <sys/types.h>
> +#include <sys/mman.h>
> +#include <sys/time.h>
> +
> +#include "qemu-common.h"
> +#include "qemu/config-file.h"
> +#include "hw/sysbus.h"
> +#include "hw/usb.h"
> +#include "hw/xen/xen_backend.h"
> +#include "monitor/qdev.h"
> +#include "qapi/qmp/qbool.h"
> +#include "qapi/qmp/qint.h"
> +#include "qapi/qmp/qstring.h"
> +#include "sys/user.h"
> +
> +#include <xen/io/ring.h>
> +#include <xen/io/usbif.h>
> +
> +#define TR(fmt, args...)                                            \
> +    {                                                               \
> +        struct timeval tv;                                          \
> +                                                                    \
> +        gettimeofday(&tv, NULL);                                    \
> +        fprintf(stderr, "%8ld.%06ld xen-usb(%s):" fmt, tv.tv_sec,   \
> +                tv.tv_usec, __func__, ##args);                      \
> +    }
> +#define TR_REQ(fmt, args...) { if (tr_debug & 1) TR(fmt, ##args) }
> +#define TR_BUS(fmt, args...) { if (tr_debug & 2) TR(fmt, ##args) }
> +
> +#define USBBACK_MAXPORTS        USBIF_PIPE_PORT_MASK
> +#define USBBACK_DEVNAME_SIZE    32

That does not seem to be used
> +#define USB_DEV_ADDR_SIZE       128
> +
> +struct usbif_ctrlrequest {
> +    uint8_t    bRequestType;
> +    uint8_t    bRequest;
> +    uint16_t   wValue;
> +    uint16_t   wIndex;
> +    uint16_t   wLength;
> +};

Would it make sense to mention that this is part of the ABI?
And if so perhaps a pointer where this in the Xen code base?

> +
> +struct usbif_isoc_descriptor {
> +    uint32_t   offset;
> +    uint32_t   length;
> +    uint32_t   actual_length;
> +    int32_t    status;
> +};

Ditto?

> +
> +struct usbback_info;
> +struct usbback_req;
> +
> +struct usbback_stub {
> +    USBDevice  *dev;
> +    USBPort    port;
> +    unsigned   speed;

unsigned int?
> +    bool       attached;
> +    QTAILQ_HEAD(submit_q_head, usbback_req) submit_q;
> +};
> +
> +struct usbback_req {
> +    struct usbback_info      *usbif;
> +    struct usbback_stub      *stub;
> +    struct usbif_urb_request req;
> +    USBPacket                packet;
> +
> +    unsigned int             nr_buffer_segs; /* # of transfer_buffer segments */
> +    unsigned int             nr_extra_segs;  /* # of iso_frame_desc segments  */
> +
> +    QTAILQ_ENTRY(usbback_req) q;
> +
> +    void                     *buffer;
> +    void                     *isoc_buffer;
> +    struct libusb_transfer   *xfer;
> +};
> +
> +struct usbback_info {
> +    struct XenDevice         xendev;  /* must be first */
> +    USBBus                   bus;
> +    void                     *urb_sring;
> +    void                     *conn_sring;
> +    struct usbif_urb_back_ring urb_ring;
> +    struct usbif_conn_back_ring conn_ring;
> +    int                      num_ports;
> +    int                      usb_ver;
> +    bool                     ring_error;
> +    QTAILQ_HEAD(req_free_q_head, usbback_req) req_free_q;
> +    struct usbback_stub      ports[USBBACK_MAXPORTS];
> +    struct usbback_stub      *addr_table[USB_DEV_ADDR_SIZE];
> +    QEMUBH                   *bh;
> +};
> +
> +static unsigned int tr_debug = 3;

This surely should be zero :-)

> +
> +static void usbback_copy_buffer(struct usbback_req *usbback_req,
> +                                struct usbif_isoc_descriptor *iso, bool out,
> +                                unsigned len, unsigned off)
> +{
> +    struct usbif_request_segment *seg;
> +    unsigned s, offset, copy_len, copy_off;
> +    void *addr;
> +
> +    offset = 0;
> +    copy_off = iso->offset;
> +    for (s = 0; len && s < usbback_req->nr_buffer_segs; s++) {
> +        seg = usbback_req->req.seg + s;
> +        if (offset + seg->length > copy_off) {
> +            addr = usbback_req->buffer + s * PAGE_SIZE + seg->offset +
> +                   copy_off - offset;
> +            copy_len = len;
> +            if (copy_len > seg->length - copy_off + offset) {
> +                copy_len = seg->length - copy_off + offset;
> +            }
> +            if (out) {
> +                memcpy(usbback_req->xfer->buffer + off, addr, copy_len);
> +            } else {
> +                memcpy(addr, usbback_req->xfer->buffer + off, copy_len);
> +            }
> +            len -= copy_len;
> +            off += copy_len;
> +        }
> +        offset += usbback_req->req.seg[s].length;
> +    }
> +    assert(!len);
> +}
> +
> +int usbback_get_packets(USBPacket *p)

static ?

> +{
> +    struct usbback_req *usbback_req;
> +
> +    usbback_req = container_of(p, struct usbback_req, packet);
> +    assert(usbif_pipeisoc(usbback_req->req.pipe));
> +    return usbback_req->req.u.isoc.nr_frame_desc_segs;
> +}
> +
> +void usbback_set_iso_desc(USBPacket *p, struct libusb_transfer *xfer)
> +{
> +    struct usbback_req *usbback_req;
> +    struct usbif_isoc_descriptor *iso;
> +    struct usbif_request_segment *seg;
> +    unsigned i, j, np, descr, off;
> +
> +    usbback_req = container_of(p, struct usbback_req, packet);
> +    assert(usbif_pipeisoc(usbback_req->req.pipe));
> +    usbback_req->xfer = xfer;
> +    np = usbback_req->req.u.isoc.nr_frame_desc_segs;
> +    descr = 0;
> +    off = 0;
> +
> +    for (i = 0; i < usbback_req->nr_extra_segs; i++) {
> +        seg = usbback_req->req.seg + usbback_req->nr_buffer_segs + i;
> +        iso = usbback_req->isoc_buffer + i * PAGE_SIZE + seg->offset;
> +        if ((seg->length % sizeof(*iso)) ||
> +            (seg->length / sizeof(*iso) > np - descr) ||
> +            (iso->offset + iso->length > usbback_req->req.buffer_length)) {
> +            xen_be_printf(&usbback_req->usbif->xendev, 0,
> +                          "iso segment length invalid\n");
> +            xfer->num_iso_packets = descr;
> +            while (descr < usbback_req->req.u.isoc.nr_frame_desc_segs) {
> +                xfer->iso_packet_desc[descr].length = 0;
> +                xfer->iso_packet_desc[descr].actual_length = 0;
> +                descr++;
> +            }
> +            return;
> +        }
> +        for (j = 0; j < seg->length / sizeof(*iso); j++) {
> +            xfer->iso_packet_desc[descr].length = iso->length;
> +            xfer->iso_packet_desc[descr].actual_length = 0;
> +            if (!usbif_pipein(usbback_req->req.pipe)) {
> +                usbback_copy_buffer(usbback_req, iso, true, iso->length, off);
> +                off += iso->length;
> +            }
> +            iso++;
> +            descr++;
> +        }
> +    }
> +}
> +
> +static struct usbback_req *usbback_get_req(struct usbback_info *usbif)
> +{
> +    struct usbback_req *usbback_req;
> +
> +    if (QTAILQ_EMPTY(&usbif->req_free_q)) {
> +        usbback_req = g_malloc0(sizeof(*usbback_req));
> +    } else {
> +        usbback_req = QTAILQ_FIRST(&usbif->req_free_q);
> +        QTAILQ_REMOVE(&usbif->req_free_q, usbback_req, q);
> +    }
> +    return usbback_req;
> +}
> +
> +static void usbback_put_req(struct usbback_req *usbback_req)
> +{
> +    struct usbback_info *usbif;
> +
> +    usbif = usbback_req->usbif;
> +    memset(usbback_req, 0, sizeof(*usbback_req));
> +    QTAILQ_INSERT_HEAD(&usbif->req_free_q, usbback_req, q);
> +}
> +
> +static int usbback_gnttab_map(struct usbback_info *usbif,
> +                              struct usbback_req *usbback_req)
> +{
> +    unsigned int nr_segs, i, prot;
> +    uint32_t ref[USBIF_MAX_SEGMENTS_PER_REQUEST];
> +    struct XenDevice *xendev = &usbif->xendev;
> +    struct usbif_request_segment *seg;
> +    void *addr;
> +
> +    nr_segs = usbback_req->nr_buffer_segs + usbback_req->nr_extra_segs;
> +    if (!nr_segs) {
> +        return 0;
> +    }
> +
> +    if (nr_segs > USBIF_MAX_SEGMENTS_PER_REQUEST) {
> +        xen_be_printf(xendev, 0, "bad number of segments in request (%d)\n",
> +                      nr_segs);
> +        return -EINVAL;
> +    }
> +
> +    for (i = 0; i < nr_segs; i++) {
> +        if ((unsigned)usbback_req->req.seg[i].offset +
> +            (unsigned)usbback_req->req.seg[i].length > PAGE_SIZE) {
> +            xen_be_printf(xendev, 0, "segment crosses page boundary\n");
> +            return -EINVAL;
> +        }
> +    }
> +
> +    if (usbback_req->nr_buffer_segs) {
> +        prot = PROT_READ;
> +        if (usbif_pipein(usbback_req->req.pipe)) {
> +                prot |= PROT_WRITE;
> +        }
> +        for (i = 0; i < usbback_req->nr_buffer_segs; i++) {
> +            ref[i] = usbback_req->req.seg[i].gref;
> +        }
> +        usbback_req->buffer = xc_gnttab_map_domain_grant_refs(xendev->gnttabdev,
> +            usbback_req->nr_buffer_segs, xendev->dom, ref, prot);
> +
> +        if (!usbback_req->buffer) {
> +            return -ENOMEM;
> +        }
> +
> +        for (i = 0; i < usbback_req->nr_buffer_segs; i++) {
> +            seg = usbback_req->req.seg + i;
> +            addr = usbback_req->buffer + i * PAGE_SIZE + seg->offset;
> +            qemu_iovec_add(&usbback_req->packet.iov, addr, seg->length);
> +        }
> +    }
> +
> +    if (!usbif_pipeisoc(usbback_req->req.pipe)) {
> +        return 0;
> +    }
> +
> +    if (!usbback_req->nr_extra_segs) {
> +        xen_be_printf(xendev, 0, "iso request without descriptor segments\n");
> +        return -EINVAL;

Could this be moved before the xc_gnttab_map_domain_grant_refs and qemu_iovec_add
is done? And should you unmap ->buffer?

> +    }
> +
> +    prot = PROT_READ | PROT_WRITE;
> +    for (i = 0; i < usbback_req->nr_extra_segs; i++) {
> +        ref[i] = usbback_req->req.seg[i + usbback_req->req.nr_buffer_segs].gref;
> +    }
> +    usbback_req->isoc_buffer = xc_gnttab_map_domain_grant_refs(
> +         xendev->gnttabdev, usbback_req->nr_extra_segs, xendev->dom, ref, prot);
> +
> +    if (!usbback_req->isoc_buffer) {

No unmapping of the ->buffer?

> +        return -ENOMEM;
> +    }
> +
> +    return 0;
> +}
> +
> +static int usbback_init_packet(struct usbback_req *usbback_req)
> +{
> +    USBPacket *packet = &usbback_req->packet;
> +    USBDevice *dev = usbback_req->stub->dev;
> +    USBEndpoint *ep;
> +    unsigned int pid, ep_nr;
> +    bool sok;
> +
> +    qemu_iovec_init(&packet->iov, USBIF_MAX_SEGMENTS_PER_REQUEST);
> +    pid = usbif_pipein(usbback_req->req.pipe) ? USB_TOKEN_IN : USB_TOKEN_OUT;
> +    ep_nr = usbif_pipeendpoint(usbback_req->req.pipe);
> +    sok = !!(usbback_req->req.transfer_flags & 1);
> +    if (usbif_pipetype(usbback_req->req.pipe) == USBIF_PIPE_TYPE_CTRL) {
> +        ep_nr = 0;
> +        sok = false;
> +    }
> +    ep = usb_ep_get(dev, pid, ep_nr);
> +    usb_packet_setup(packet, pid, ep, 0, 1, sok, true);
> +
> +    switch (usbif_pipetype(usbback_req->req.pipe)) {
> +    case USBIF_PIPE_TYPE_ISOC:
> +        TR_REQ("iso transfer %s: buflen: %x, %d frames\n",
> +               (pid == USB_TOKEN_IN) ? "in" : "out",
> +               usbback_req->req.buffer_length,
> +               usbback_req->req.u.isoc.nr_frame_desc_segs);
> +        break;
> +
> +    case USBIF_PIPE_TYPE_INT:
> +        TR_REQ("int transfer %s: buflen: %x\n",
> +               (pid == USB_TOKEN_IN) ? "in" : "out",
> +               usbback_req->req.buffer_length);
> +        break;
> +
> +    case USBIF_PIPE_TYPE_CTRL:
> +        packet->parameter = *(uint64_t *)usbback_req->req.u.ctrl;
> +        TR_REQ("ctrl parameter: %lx, buflen: %x\n", packet->parameter,
> +               usbback_req->req.buffer_length);
> +        break;
> +
> +    case USBIF_PIPE_TYPE_BULK:
> +        TR_REQ("bulk transfer %s: buflen: %x\n",
> +               (pid == USB_TOKEN_IN) ? "in" : "out",
> +               usbback_req->req.buffer_length);
> +        break;
> +    default:
> +        return -EINVAL;
> +    }
> +
> +    return 0;
> +}
> +
> +static void usbback_do_response(struct usbback_req *usbback_req,
> +                                int32_t status, int32_t actual_length,
> +                                int32_t error_count)

Should the 'actual_length' and 'error_count' be unsigned int?
> +{
> +    struct usbback_info *usbif;
> +    struct usbif_urb_response *res;
> +    struct XenDevice *xendev;
> +    unsigned int notify;
> +
> +    TR_REQ("id %d, status %d, length %d, errcnt %d\n", usbback_req->req.id,
> +           status, actual_length, error_count);
> +
> +    usbif = usbback_req->usbif;
> +    xendev = &usbif->xendev;
> +
> +    if (usbback_req->packet.iov.iov) {

qemu_iovec_is_zero ?

> +        qemu_iovec_destroy(&usbback_req->packet.iov);
> +    }
> +
> +    if (usbback_req->buffer) {
> +        xc_gnttab_munmap(xendev->gnttabdev, usbback_req->buffer,
> +                         usbback_req->nr_buffer_segs);
> +        usbback_req->buffer = NULL;

nr_buffer_segs = 0?

> +    }
> +
> +    if (usbback_req->isoc_buffer) {
> +        xc_gnttab_munmap(xendev->gnttabdev, usbback_req->isoc_buffer,
> +                         usbback_req->nr_extra_segs);

nr_extra_segts = 0 ?
Should 'isoc_buffer' be set to NULL as well?

> +    }
> +
> +    res = RING_GET_RESPONSE(&usbif->urb_ring, usbif->urb_ring.rsp_prod_pvt);
> +    res->id = usbback_req->req.id;
> +    res->status = status;
> +    res->actual_length = actual_length;
> +    res->error_count = error_count;
> +    res->start_frame = 0;
> +    usbif->urb_ring.rsp_prod_pvt++;
> +    RING_PUSH_RESPONSES_AND_CHECK_NOTIFY(&usbif->urb_ring, notify);
> +
> +    if (notify) {
> +        xen_be_send_notify(xendev);
> +    }
> +
> +    usbback_put_req(usbback_req);
> +}
> +
> +static void usbback_do_response_ret(struct usbback_req *usbback_req,
> +                                    int32_t status)
> +{
> +    usbback_do_response(usbback_req, status, 0, 0);
> +}
> +
> +static int32_t usbback_xlat_status(int32_t status)
> +{
> +    int32_t ret = -ESHUTDOWN;
> +
> +    switch (status) {
> +    case USB_RET_SUCCESS:
> +        ret = 0;
> +        break;
> +    case USB_RET_NODEV:
> +        ret = -ENODEV;
> +        break;
> +    case USB_RET_STALL:
> +        ret = -EPIPE;
> +        break;
> +    case USB_RET_BABBLE:
> +        ret = -EOVERFLOW;
> +        break;
> +    case USB_RET_IOERROR:
> +        ret = -EPROTO;
> +        break;
> +    }
> +
> +    return ret;
> +}
> +
> +static void usbback_packet_complete(USBPacket *packet)
> +{
> +    struct usbback_req *usbback_req;
> +    int32_t status, error_count, actual_length;
> +    unsigned int i, j, off;
> +    struct usbif_isoc_descriptor *iso;
> +    struct usbif_request_segment *seg;
> +    struct libusb_iso_packet_descriptor *libusb_iso;
> +
> +    error_count = 0;
> +    actual_length = 0;
> +    off = 0;
> +    usbback_req = container_of(packet, struct usbback_req, packet);
> +
> +    QTAILQ_REMOVE(&usbback_req->stub->submit_q, usbback_req, q);
> +
> +    status = usbback_xlat_status(packet->status);
> +    if (usbback_req->isoc_buffer) {
> +        libusb_iso = usbback_req->xfer->iso_packet_desc;
> +        j = 0;
> +        for (i = 0; i < usbback_req->nr_extra_segs; i++) {
> +            seg = usbback_req->req.seg + usbback_req->nr_buffer_segs + i;
> +            iso = usbback_req->isoc_buffer + i * PAGE_SIZE + seg->offset;
> +            for (j = 0; j < seg->length / sizeof(*iso); j++) {
> +                iso->actual_length = libusb_iso->actual_length;
> +                iso->status = usbback_xlat_status(libusb_iso->status);
> +                actual_length += libusb_iso->actual_length;
> +                error_count += iso->status ? 1 : 0;
> +                if (usbif_pipein(usbback_req->req.pipe)) {
> +                    usbback_copy_buffer(usbback_req, iso, false,
> +                                        iso->actual_length, off);
> +                    off += iso->length;
> +                }
> +                libusb_iso++;
> +                iso++;
> +            }
> +        }
> +    } else {
> +        actual_length = packet->actual_length;
> +    }
> +
> +    usbback_do_response(usbback_req, status, actual_length, error_count);
> +}
> +
> +static void usbback_set_address(struct usbback_info *usbif,
> +                                struct usbback_stub *stub, int cur_addr,
> +                                int new_addr)
> +{
> +    if (cur_addr) {
> +        usbif->addr_table[cur_addr] = NULL;
> +    }
> +    if (new_addr) {
> +        usbif->addr_table[new_addr] = stub;
> +    }
> +}
> +
> +static bool usbback_cancel_req(struct usbback_req *usbback_req)
> +{
> +    bool ret = false;
> +
> +    if (usb_packet_is_inflight(&usbback_req->packet)) {
> +        usb_cancel_packet(&usbback_req->packet);
> +        ret = true;
> +    }
> +    return ret;
> +}
> +
> +static void usbback_process_unlink_req(struct usbback_req *usbback_req)
> +{
> +    struct usbback_info *usbif;
> +    struct usbback_req *unlink_req;
> +    unsigned int id, devnum;
> +    int ret;

Why 'ret'? You are not returning the value.

> +
> +    usbif = usbback_req->usbif;
> +    ret = 0;
> +    id = usbback_req->req.u.unlink.unlink_id;
> +    TR_REQ("unlink id %d\n", id);
> +    devnum = usbif_pipedevice(usbback_req->req.pipe);
> +    if (unlikely(devnum == 0)) {
> +        usbback_req->stub = usbif->ports +
> +                            usbif_pipeportnum(usbback_req->req.pipe);

Should you check that usbif_pipeporntnum value does not exceeed USBBACK_MAXPORTS ?
Ah, no. The macro usbif_piportnum will mask it so it will always be within
ports[0..USBBACK_MAXPORTS].

> +        if (unlikely(!usbback_req->stub)) {
> +            ret = -ENODEV;
> +            goto fail_response;
> +        }
> +    } else {
> +        if (unlikely(!usbif->addr_table[devnum])) {

Should we doublecheck that devnm < USB_DEV_ADDR_SIZE ?

> +            ret = -ENODEV;
> +            goto fail_response;
> +        }
> +        usbback_req->stub = usbif->addr_table[devnum];
> +    }
> +
> +    QTAILQ_FOREACH(unlink_req, &usbback_req->stub->submit_q, q) {
> +        if (unlink_req->req.id == id) {
> +            if (usbback_cancel_req(unlink_req)) {
> +                usbback_do_response_ret(unlink_req, -EPROTO);
> +            }
> +            break;
> +        }
> +    }
> +
> +fail_response:
> +    usbback_do_response_ret(usbback_req, ret);
> +}
> +

Could you add a comment explaining what positive return values mean?

> +static int usbback_check_and_submit(struct usbback_req *usbback_req)
> +{
> +    struct usbback_info *usbif;
> +    unsigned int devnum;
> +    struct usbback_stub *stub;
> +    struct usbif_ctrlrequest *ctrl;
> +    int ret;
> +    uint16_t wValue;
> +
> +    usbif = usbback_req->usbif;
> +    stub = NULL;
> +    devnum = usbif_pipedevice(usbback_req->req.pipe);
> +    ctrl = (struct usbif_ctrlrequest *)usbback_req->req.u.ctrl;
> +    wValue = le16_to_cpu(ctrl->wValue);
> +
> +    /*
> +     * When the device is first connected or resetted, USB device has no
> +     * address. In this initial state, following requests are sent to device
> +     * address (#0),
> +     *
> +     *  1. GET_DESCRIPTOR (with Descriptor Type is "DEVICE") is sent,
> +     *     and OS knows what device is connected to.
> +     *
> +     *  2. SET_ADDRESS is sent, and then device has its address.
> +     *
> +     * In the next step, SET_CONFIGURATION is sent to addressed device, and
> +     * then the device is finally ready to use.
> +     */
> +    if (unlikely(devnum == 0)) {
> +        stub = usbif->ports + usbif_pipeportnum(usbback_req->req.pipe) - 1;
> +        if (!stub->dev || !stub->attached) {
> +            ret = -ENODEV;
> +            goto do_response;
> +        }
> +
> +        switch (ctrl->bRequest) {
> +        case USB_REQ_GET_DESCRIPTOR:
> +            /*
> +             * GET_DESCRIPTOR request to device #0.
> +             * through to normal transfer.

s/to//
> +             */
> +            TR_REQ("devnum 0 GET_DESCRIPTOR\n");
> +            usbback_req->stub = stub;
> +            return 0;
> +        case USB_REQ_SET_ADDRESS:
> +            /*
> +             * SET_ADDRESS request to device #0.
> +             * add attached device to addr_table.
> +             */
> +            TR_REQ("devnum 0 SET_ADDRESS\n");
> +            usbback_set_address(usbif, stub, 0, wValue);
> +            ret = 0;
> +            break;
> +        default:
> +            ret = -EINVAL;
> +            break;
> +        }
> +        goto do_response;
> +    }
> +
> +    if (unlikely(!usbif->addr_table[devnum])) {
> +            ret = -ENODEV;
> +            goto do_response;
> +    }
> +    usbback_req->stub = usbif->addr_table[devnum];
> +
> +    /*
> +     * Check special request
> +     */
> +    if (ctrl->bRequest != USB_REQ_SET_ADDRESS) {
> +        return 0;
> +    }
> +
> +    /*
> +     * SET_ADDRESS request to addressed device.
> +     * change addr or remove from addr_table.
> +     */
> +    usbback_set_address(usbif, usbback_req->stub, devnum, wValue);
> +    ret = 0;
> +
> +do_response:
> +    usbback_do_response_ret(usbback_req, ret);
> +    return 1;
> +}
> +
> +static void usbback_dispatch(struct usbback_req *usbback_req)
> +{
> +    int ret;
> +    unsigned int devnum;
> +    struct usbback_info *usbif;
> +
> +    TR_REQ("start req_id %d pipe %08x\n", usbback_req->req.id,
> +           usbback_req->req.pipe);
> +
> +    usbif = usbback_req->usbif;
> +
> +    /* unlink request */
> +    if (unlikely(usbif_pipeunlink(usbback_req->req.pipe))) {
> +        usbback_process_unlink_req(usbback_req);
> +        return;
> +    }
> +
> +    if (usbif_pipectrl(usbback_req->req.pipe)) {
> +        if (usbback_check_and_submit(usbback_req)) {

Should you at least report the errors if there are any?

> +            return;
> +        }
> +    } else {
> +        devnum = usbif_pipedevice(usbback_req->req.pipe);
> +        usbback_req->stub = usbif->addr_table[devnum];
> +
> +        if (!usbback_req->stub || !usbback_req->stub->attached) {
> +            ret = -ENODEV;
> +            goto fail_response;
> +        }
> +    }
> +
> +    QTAILQ_INSERT_TAIL(&usbback_req->stub->submit_q, usbback_req, q);
> +
> +    usbback_req->nr_buffer_segs = usbback_req->req.nr_buffer_segs;
> +    usbback_req->nr_extra_segs = usbif_pipeisoc(usbback_req->req.pipe) ?
> +                                 usbback_req->req.u.isoc.nr_frame_desc_segs : 0;
> +
> +    ret = usbback_init_packet(usbback_req);
> +    if (ret) {
> +        xen_be_printf(&usbif->xendev, 0, "invalid request\n");
> +        ret = -ESHUTDOWN;
> +        goto fail_free_urb;
> +    }
> +
> +    ret = usbback_gnttab_map(usbif, usbback_req);
> +    if (ret) {
> +        xen_be_printf(&usbif->xendev, 0, "invalid buffer\n");

Should the 'ret' be at least included?

> +        ret = -ESHUTDOWN;
> +        goto fail_free_urb;
> +    }
> +
> +    usb_handle_packet(usbback_req->stub->dev, &usbback_req->packet);
> +    if (usbback_req->packet.status != USB_RET_ASYNC) {
> +        usbback_packet_complete(&usbback_req->packet);
> +    }
> +    return;
> +
> +fail_free_urb:
> +    QTAILQ_REMOVE(&usbback_req->stub->submit_q, usbback_req, q);
> +
> +fail_response:
> +    usbback_do_response_ret(usbback_req, ret);
> +}
> +
> +static void usbback_bh(void *opaque)
> +{
> +    struct usbback_info *usbif;
> +    struct usbif_urb_back_ring *urb_ring;
> +    struct usbback_req *usbback_req;
> +    RING_IDX rc, rp;
> +    unsigned int more_to_do;
> +
> +    usbif = opaque;
> +    if (usbif->ring_error) {
> +        return;
> +    }
> +
> +    urb_ring = &usbif->urb_ring;
> +    rc = urb_ring->req_cons;
> +    rp = urb_ring->sring->req_prod;
> +    xen_rmb(); /* Ensure we see queued requests up to 'rp'. */
> +
> +    if (RING_REQUEST_PROD_OVERFLOW(urb_ring, rp)) {
> +        rc = urb_ring->rsp_prod_pvt;
> +        xen_be_printf(&usbif->xendev, 0, "domU provided bogus ring requests "
> +                      "(%#x - %#x = %u). Halting ring processing.\n",
> +                      rp, rc, rp - rc);
> +        usbif->ring_error = true;
> +        return;
> +    }
> +
> +    while (rc != rp) {
> +        if (RING_REQUEST_CONS_OVERFLOW(urb_ring, rc)) {
> +            break;
> +        }
> +        usbback_req = usbback_get_req(usbif);
> +
> +        usbback_req->req = *RING_GET_REQUEST(urb_ring, rc);
> +        usbback_req->usbif = usbif;
> +
> +        usbback_dispatch(usbback_req);
> +
> +        urb_ring->req_cons = ++rc;
> +    }
> +
> +    RING_FINAL_CHECK_FOR_REQUESTS(urb_ring, more_to_do);
> +    if (more_to_do) {
> +        qemu_bh_schedule(usbif->bh);
> +    }
> +}
> +
> +static void usbback_hotplug_notify(struct usbback_info *usbif, unsigned port)
> +{
> +    struct usbif_conn_back_ring *ring = &usbif->conn_ring;
> +    struct usbif_conn_request *req;
> +    struct usbif_conn_response *res;
> +    uint16_t id;
> +    unsigned int notify;
> +
> +    if (!usbif->conn_sring) {
> +        return;
> +    }
> +
> +    req = RING_GET_REQUEST(ring, ring->req_cons);
> +    id = req->id;
> +    ring->req_cons++;
> +    ring->sring->req_event = ring->req_cons + 1;
> +
> +    res = RING_GET_RESPONSE(ring, ring->rsp_prod_pvt);
> +    res->id = id;
> +    res->portnum = port;
> +    res->speed = usbif->ports[port - 1].speed;
> +    ring->rsp_prod_pvt++;
> +    RING_PUSH_RESPONSES_AND_CHECK_NOTIFY(ring, notify);
> +
> +    if (notify) {
> +        xen_be_send_notify(&usbif->xendev);
> +    }
> +
> +    TR_BUS("hotplug port %d speed %d\n", port, res->speed);
> +}
> +
> +static void usbback_portid_remove(struct usbback_info *usbif, unsigned port)
> +{
> +    USBPort *p;
> +
> +    if (!usbif->ports[port - 1].dev) {
> +        return;
> +    }
> +
> +    p = &(usbif->ports[port - 1].port);
> +    snprintf(p->path, sizeof(p->path), "%d", 99);
> +
> +    object_unparent(OBJECT(usbif->ports[port - 1].dev));
> +    usbif->ports[port - 1].dev = NULL;
> +    usbif->ports[port - 1].speed = USBIF_SPEED_NONE;
> +    usbif->ports[port - 1].attached = false;
> +    usbback_hotplug_notify(usbif, port);
> +
> +    TR_BUS("port %d removed\n", port);
> +}
> +
> +static void usbback_portid_add(struct usbback_info *usbif, unsigned port,
> +                               char *busid)
> +{
> +    unsigned speed;
> +    char *portname;
> +    USBPort *p;
> +    Error *local_err = NULL;
> +    QDict *qdict;
> +    QemuOpts *opts;
> +
> +    if (usbif->ports[port - 1].dev) {
> +        return;
> +    }
> +
> +    portname = strchr(busid, '-');
> +    if (!portname) {
> +        xen_be_printf(&usbif->xendev, 0, "device %s illegal specification\n",
> +                      busid);
> +        return;
> +    }
> +    portname++;
> +    p = &(usbif->ports[port - 1].port);
> +    snprintf(p->path, sizeof(p->path), "%s", portname);
> +
> +    qdict = qdict_new();
> +    qdict_put(qdict, "driver", qstring_from_str("usb-host"));
> +    qdict_put(qdict, "hostbus", qint_from_int(atoi(busid)));
> +    qdict_put(qdict, "hostport", qstring_from_str(portname));
> +    qdict_put(qdict, "xen-iso-passthrough", qbool_from_int(1));
> +    opts = qemu_opts_from_qdict(qemu_find_opts("device"), qdict, &local_err);
> +    if (local_err) {
> +        goto err;
> +    }
> +    usbif->ports[port - 1].dev = USB_DEVICE(qdev_device_add(opts));
> +    if (!usbif->ports[port - 1].dev) {
> +        goto err;
> +    }
> +    QDECREF(qdict);
> +    snprintf(p->path, sizeof(p->path), "%d", port);
> +    speed = usbif->ports[port - 1].dev->speed;

I am a bit confused as how you get the 'speed'? We are adding a device
and we don't read this?


> +    switch (speed) {
> +    case USB_SPEED_LOW:
> +        speed = USBIF_SPEED_LOW;
> +        break;
> +    case USB_SPEED_FULL:
> +        speed = USBIF_SPEED_FULL;
> +        break;
> +    case USB_SPEED_HIGH:
> +        speed = (usbif->usb_ver < USB_VER_USB20) ?
> +                USBIF_SPEED_NONE : USBIF_SPEED_HIGH;
> +        break;
> +    default:
> +        speed = USBIF_SPEED_NONE;
> +        break;
> +    }
> +    if (speed == USBIF_SPEED_NONE) {
> +        xen_be_printf(&usbif->xendev, 0, "device %s wrong speed\n", busid);
> +        object_unparent(OBJECT(usbif->ports[port - 1].dev));
> +        usbif->ports[port - 1].dev = NULL;
> +        return;
> +    }
> +    usb_device_reset(usbif->ports[port - 1].dev);
> +    usbif->ports[port - 1].speed = speed;
> +    usbif->ports[port - 1].attached = true;
> +    QTAILQ_INIT(&usbif->ports[port - 1].submit_q);
> +    usbback_hotplug_notify(usbif, port);
> +
> +    TR_BUS("port %d attached\n", port);
> +    return;
> +
> +err:
> +    QDECREF(qdict);
> +    snprintf(p->path, sizeof(p->path), "%d", 99);
> +    xen_be_printf(&usbif->xendev, 0, "device %s could not be opened\n", busid);
> +}
> +
> +static void usbback_process_port(struct usbback_info *usbif, unsigned port)
> +{
> +    char node[8];
> +    char *busid;
> +
> +    snprintf(node, sizeof(node), "port/%d", port);
> +    busid = xenstore_read_be_str(&usbif->xendev, node);
> +    if (busid == NULL) {
> +        xen_be_printf(&usbif->xendev, 0, "xenstore_read %s failed\n", node);
> +        return;
> +    }
> +
> +    /* Remove portid, if the port is not connected.  */
> +    if (strlen(busid) == 0) {
> +        usbback_portid_remove(usbif, port);
> +    } else {
> +        usbback_portid_add(usbif, port, busid);
> +    }
> +
> +    g_free(busid);
> +}
> +
> +static void usbback_disconnect(struct XenDevice *xendev)
> +{
> +    struct usbback_info *usbif;
> +    struct usbback_req *req, *tmp;
> +    unsigned int i;
> +
> +    TR_BUS("start\n");
> +
> +    usbif = container_of(xendev, struct usbback_info, xendev);
> +
> +    xen_be_unbind_evtchn(xendev);
> +
> +    if (usbif->urb_sring) {
> +        xc_gnttab_munmap(xendev->gnttabdev, usbif->urb_sring, 1);
> +        usbif->urb_sring = NULL;
> +    }
> +    if (usbif->conn_sring) {
> +        xc_gnttab_munmap(xendev->gnttabdev, usbif->conn_sring, 1);
> +        usbif->conn_sring = NULL;
> +    }
> +
> +    for (i = 0; i < usbif->num_ports; i++) {
> +        if (!usbif->ports[i].dev) {
> +            continue;
> +        }
> +        QTAILQ_FOREACH_SAFE(req, &usbif->ports[i].submit_q, q, tmp) {
> +            usbback_cancel_req(req);
> +        }
> +    }
> +
> +    TR_BUS("finished\n");
> +}
> +
> +static int usbback_connect(struct XenDevice *xendev)
> +{
> +    struct usbback_info *usbif;
> +    struct usbif_urb_sring *urb_sring;
> +    struct usbif_conn_sring *conn_sring;
> +    int urb_ring_ref;
> +    int conn_ring_ref;
> +    unsigned int i;
> +
> +    TR_BUS("start\n");
> +
> +    usbif = container_of(xendev, struct usbback_info, xendev);
> +
> +    if (xenstore_read_fe_int(xendev, "urb-ring-ref", &urb_ring_ref)) {
> +        TR("error reading urb-ring-ref\n");
> +        return -1;
> +    }
> +    if (xenstore_read_fe_int(xendev, "conn-ring-ref", &conn_ring_ref)) {
> +        TR("error reading conn-ring-ref\n");
> +        return -1;
> +    }
> +    if (xenstore_read_fe_int(xendev, "event-channel", &xendev->remote_port)) {
> +        TR("error reading event-channel\n");
> +        return -1;
> +    }
> +
> +    usbif->urb_sring = xc_gnttab_map_grant_ref(xendev->gnttabdev, xendev->dom,
> +                                               urb_ring_ref,
> +                                               PROT_READ | PROT_WRITE);
> +    usbif->conn_sring = xc_gnttab_map_grant_ref(xendev->gnttabdev, xendev->dom,
> +                                                conn_ring_ref,
> +                                                PROT_READ | PROT_WRITE);
> +    if (!usbif->urb_sring || !usbif->conn_sring) {
> +        TR("error mapping rings\n");
> +        usbback_disconnect(xendev);
> +        return -1;
> +    }
> +
> +    urb_sring = usbif->urb_sring;
> +    conn_sring = usbif->conn_sring;
> +    BACK_RING_INIT(&usbif->urb_ring, urb_sring, XC_PAGE_SIZE);
> +    BACK_RING_INIT(&usbif->conn_ring, conn_sring, XC_PAGE_SIZE);
> +
> +    xen_be_bind_evtchn(xendev);
> +
> +    xen_be_printf(xendev, 1, "urb-ring-ref %d, conn-ring-ref %d, "
> +                  "remote port %d, local port %d\n", urb_ring_ref,
> +                  conn_ring_ref, xendev->remote_port, xendev->local_port);
> +
> +    for (i = 1; i <= usbif->num_ports; i++) {
> +        if (usbif->ports[i - 1].dev) {
> +            usbback_hotplug_notify(usbif, i);
> +        }
> +    }
> +
> +    return 0;
> +}
> +
> +static void usbback_backend_changed(struct XenDevice *xendev, const char *node)
> +{
> +    struct usbback_info *usbif;
> +    unsigned int i;
> +
> +    TR_BUS("path %s\n", node);
> +
> +    usbif = container_of(xendev, struct usbback_info, xendev);
> +    for (i = 1; i <= usbif->num_ports; i++) {
> +        usbback_process_port(usbif, i);
> +    }
> +}
> +
> +static int usbback_init(struct XenDevice *xendev)
> +{
> +    struct usbback_info *usbif;
> +
> +    TR_BUS("start\n");
> +
> +    usbif = container_of(xendev, struct usbback_info, xendev);
> +
> +    if (xenstore_read_be_int(xendev, "num-ports", &usbif->num_ports) ||
> +        usbif->num_ports < 1 || usbif->num_ports > USBBACK_MAXPORTS) {

I think the compiler may be free to re-order these 'or' statements.

Could this be split in two 'if' ? One to get the value (and if it failed
then print), and then the other 'if to check for the validity of it?

> +        xen_be_printf(xendev, 0, "num-ports not readable or out of bounds\n");
> +        return -1;
> +    }
> +    if (xenstore_read_be_int(xendev, "usb-ver", &usbif->usb_ver) ||
> +        (usbif->usb_ver != USB_VER_USB11 && usbif->usb_ver != USB_VER_USB20)) {

Ditto.
> +        xen_be_printf(xendev, 0, "usb-ver not readable or out of bounds\n");
> +        return -1;
> +    }
> +
> +    usbback_backend_changed(xendev, "port");
> +
> +    TR_BUS("finished\n");
> +
> +    return 0;
> +}
> +
> +static void xen_bus_attach(USBPort *port)
> +{
> +    struct usbback_info *usbif;
> +
> +    TR_BUS("called\n");
> +    usbif = port->opaque;
> +    usbif->ports[port->index].attached = true;
> +    usbback_hotplug_notify(usbif, port->index + 1);
> +}
> +
> +static void xen_bus_detach(USBPort *port)
> +{
> +    struct usbback_info *usbif;
> +
> +    TR_BUS("called\n");
> +    usbif = port->opaque;
> +    usbif->ports[port->index].attached = false;
> +    usbback_hotplug_notify(usbif, port->index + 1);
> +}
> +
> +static void xen_bus_child_detach(USBPort *port, USBDevice *child)
> +{
> +    TR_BUS("called\n");
> +}
> +
> +static void xen_bus_complete(USBPort *port, USBPacket *packet)
> +{
> +    TR_REQ("called\n");
> +    usbback_packet_complete(packet);
> +}
> +
> +static USBPortOps xen_usb_port_ops = {
> +    .attach = xen_bus_attach,
> +    .detach = xen_bus_detach,
> +    .child_detach = xen_bus_child_detach,
> +    .complete = xen_bus_complete,
> +};
> +
> +static USBBusOps xen_usb_bus_ops = {
> +};
> +
> +static void usbback_alloc(struct XenDevice *xendev)
> +{
> +    struct usbback_info *usbif;
> +    USBPort *p;
> +    unsigned int i, max_grants;
> +
> +    usbif = container_of(xendev, struct usbback_info, xendev);
> +
> +    usb_bus_new(&usbif->bus, sizeof(usbif->bus), &xen_usb_bus_ops, xen_sysdev);
> +    for (i = 0; i < USBBACK_MAXPORTS; i++) {
> +        p = &(usbif->ports[i].port);
> +        usb_register_port(&usbif->bus, p, usbif, i, &xen_usb_port_ops,
> +                          USB_SPEED_MASK_LOW | USB_SPEED_MASK_FULL |
> +                          USB_SPEED_MASK_HIGH);
> +        snprintf(p->path, sizeof(p->path), "%d", 99);
> +    }
> +
> +    QTAILQ_INIT(&usbif->req_free_q);
> +    usbif->bh = qemu_bh_new(usbback_bh, usbif);
> +
> +    max_grants = USBIF_MAX_SEGMENTS_PER_REQUEST * USB_URB_RING_SIZE + 2;

Why the '2' ?

> +    if (xc_gnttab_set_max_grants(xendev->gnttabdev, max_grants) < 0) {
> +        xen_be_printf(xendev, 0, "xc_gnttab_set_max_grants failed: %s\n",
> +                      strerror(errno));
> +    }
> +}
> +
> +static int usbback_free(struct XenDevice *xendev)
> +{
> +    struct usbback_info *usbif;
> +    struct usbback_req *usbback_req;
> +    unsigned int i;
> +
> +    TR_BUS("start\n");
> +
> +    usbback_disconnect(xendev);
> +    usbif = container_of(xendev, struct usbback_info, xendev);
> +    for (i = 1; i <= usbif->num_ports; i++) {
> +        usbback_portid_remove(usbif, i);
> +    }

You also need:
	for (i=0; i < USBBACK_MAXPORTS; i++) {
	        p = &(usbif->ports[i].port);
		usb_unregister_port(&usbif->bus, p);
	}

I think?
> +
> +    while (!QTAILQ_EMPTY(&usbif->req_free_q)) {
> +        usbback_req = QTAILQ_FIRST(&usbif->req_free_q);
> +        QTAILQ_REMOVE(&usbif->req_free_q, usbback_req, q);
> +        g_free(usbback_req);
> +    }
> +
> +    qemu_bh_delete(usbif->bh);
> +
> +    usb_bus_release(&usbif->bus);
> +
> +    TR_BUS("finished\n");
> +
> +    return 0;
> +}
> +
> +static void usbback_event(struct XenDevice *xendev)
> +{
> +    struct usbback_info *usbif;
> +
> +    usbif = container_of(xendev, struct usbback_info, xendev);
> +    qemu_bh_schedule(usbif->bh);
> +}
> +
> +struct XenDevOps xen_usb_ops = {
> +    .size            = sizeof(struct usbback_info),
> +    .flags           = DEVOPS_FLAG_NEED_GNTDEV,
> +    .init            = usbback_init,
> +    .alloc           = usbback_alloc,
> +    .free            = usbback_free,
> +    .backend_changed = usbback_backend_changed,
> +    .initialise      = usbback_connect,
> +    .disconnect      = usbback_disconnect,
> +    .event           = usbback_event,
> +};
> diff --git a/hw/xenpv/xen_machine_pv.c b/hw/xenpv/xen_machine_pv.c
> index 57bc071..fef1e7b 100644
> --- a/hw/xenpv/xen_machine_pv.c
> +++ b/hw/xenpv/xen_machine_pv.c
> @@ -72,6 +72,9 @@ static void xen_init_pv(MachineState *machine)
>      xen_be_register("vfb", &xen_framebuffer_ops);
>      xen_be_register("qdisk", &xen_blkdev_ops);
>      xen_be_register("qnic", &xen_netdev_ops);
> +#ifdef CONFIG_USB_LIBUSB
> +    xen_be_register("qusb", &xen_usb_ops);
> +#endif
>  
>      /* configure framebuffer */
>      if (xenfb_enabled) {
> diff --git a/include/hw/xen/xen_backend.h b/include/hw/xen/xen_backend.h
> index f194aae..3d44dec 100644
> --- a/include/hw/xen/xen_backend.h
> +++ b/include/hw/xen/xen_backend.h
> @@ -4,6 +4,10 @@
>  #include "hw/xen/xen_common.h"
>  #include "sysemu/sysemu.h"
>  #include "net/net.h"
> +#ifdef CONFIG_USB_LIBUSB
> +#include <libusb.h>
> +#include "hw/usb.h"
> +#endif
>  
>  /* ------------------------------------------------------------- */
>  
> @@ -55,9 +59,6 @@ struct XenDevice {
>  
>  /* ------------------------------------------------------------- */
>  
> -#define usbback_get_packets(p) 0
> -#define usbback_set_iso_desc(p, xfer)
> -
>  /* variables */
>  extern XenXC xen_xc;
>  extern struct xs_handle *xenstore;
> @@ -101,6 +102,12 @@ extern struct XenDevOps xen_kbdmouse_ops;     /* xen_framebuffer.c */
>  extern struct XenDevOps xen_framebuffer_ops;  /* xen_framebuffer.c */
>  extern struct XenDevOps xen_blkdev_ops;       /* xen_disk.c        */
>  extern struct XenDevOps xen_netdev_ops;       /* xen_nic.c         */
> +#ifdef CONFIG_USB_LIBUSB
> +extern struct XenDevOps xen_usb_ops;          /* xen-usb.c         */
> +
> +int usbback_get_packets(USBPacket *p);
> +void usbback_set_iso_desc(USBPacket *p, struct libusb_transfer *xfer);
> +#endif
>  
>  void xen_init_display(int domid);
>  
> -- 
> 2.1.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [Qemu-devel] [Xen-devel] [Patch V1 3/3] xen: add pvUSB backend

[Juergen Gross]

Sorry, I just found time now to continue with this series.

On 27/10/15 19:54, Konrad Rzeszutek Wilk wrote:
> On Thu, Sep 03, 2015 at 12:45:13PM +0200, Juergen Gross wrote:
>> Add a backend for para-virtualized USB devices for xen domains.
>>
>> The backend is using host-libusb to forward USB requests from a
>> domain via libusb to the real device(s) passed through.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>>  hw/usb/Makefile.objs         |    4 +
>>  hw/usb/xen-usb.c             | 1120 ++++++++++++++++++++++++++++++++++++++++++
>>  hw/xenpv/xen_machine_pv.c    |    3 +
>>  include/hw/xen/xen_backend.h |   13 +-
>>  4 files changed, 1137 insertions(+), 3 deletions(-)
>>  create mode 100644 hw/usb/xen-usb.c
>>
>> diff --git a/hw/usb/Makefile.objs b/hw/usb/Makefile.objs
>> index 3fe4dff..0253184 100644
>> --- a/hw/usb/Makefile.objs
>> +++ b/hw/usb/Makefile.objs
>> @@ -36,3 +36,7 @@ common-obj-$(CONFIG_USB_REDIR) += redirect.o quirks.o
>>  
>>  # usb pass-through
>>  common-obj-y += $(patsubst %,host-%.o,$(HOST_USB))
>> +
>> +ifeq ($(CONFIG_USB_LIBUSB),y)
>> +common-obj-$(CONFIG_XEN_BACKEND) += xen-usb.o
>> +endif
>> diff --git a/hw/usb/xen-usb.c b/hw/usb/xen-usb.c
>> new file mode 100644
>> index 0000000..2570bd7
>> --- /dev/null
>> +++ b/hw/usb/xen-usb.c
>> @@ -0,0 +1,1120 @@
>> +/*
>> + *  xen paravirt usb device backend
>> + *
>> + *  (c) Juergen Gross <jgross@suse.com>
>> + *
>> + *  This program is free software; you can redistribute it and/or modify
>> + *  it under the terms of the GNU General Public License as published by
>> + *  the Free Software Foundation; under version 2 of the License.
>> + *
>> + *  This program is distributed in the hope that it will be useful,
>> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + *  GNU General Public License for more details.
>> + *
>> + *  You should have received a copy of the GNU General Public License along
>> + *  with this program; if not, see <http://www.gnu.org/licenses/>.
>> + *
>> + *  Contributions after 2012-01-13 are licensed under the terms of the
>> + *  GNU GPL, version 2 or (at your option) any later version.
>> + */
>> +
>> +#include <libusb.h>
>> +#include <sys/types.h>
>> +#include <sys/mman.h>
>> +#include <sys/time.h>
>> +
>> +#include "qemu-common.h"
>> +#include "qemu/config-file.h"
>> +#include "hw/sysbus.h"
>> +#include "hw/usb.h"
>> +#include "hw/xen/xen_backend.h"
>> +#include "monitor/qdev.h"
>> +#include "qapi/qmp/qbool.h"
>> +#include "qapi/qmp/qint.h"
>> +#include "qapi/qmp/qstring.h"
>> +#include "sys/user.h"
>> +
>> +#include <xen/io/ring.h>
>> +#include <xen/io/usbif.h>
>> +
>> +#define TR(fmt, args...)                                            \
>> +    {                                                               \
>> +        struct timeval tv;                                          \
>> +                                                                    \
>> +        gettimeofday(&tv, NULL);                                    \
>> +        fprintf(stderr, "%8ld.%06ld xen-usb(%s):" fmt, tv.tv_sec,   \
>> +                tv.tv_usec, __func__, ##args);                      \
>> +    }
>> +#define TR_REQ(fmt, args...) { if (tr_debug & 1) TR(fmt, ##args) }
>> +#define TR_BUS(fmt, args...) { if (tr_debug & 2) TR(fmt, ##args) }
>> +
>> +#define USBBACK_MAXPORTS        USBIF_PIPE_PORT_MASK
>> +#define USBBACK_DEVNAME_SIZE    32
> 
> That does not seem to be used
>> +#define USB_DEV_ADDR_SIZE       128
>> +
>> +struct usbif_ctrlrequest {
>> +    uint8_t    bRequestType;
>> +    uint8_t    bRequest;
>> +    uint16_t   wValue;
>> +    uint16_t   wIndex;
>> +    uint16_t   wLength;
>> +};
> 
> Would it make sense to mention that this is part of the ABI?

It is part of the USB hardware interface. I'll add a comment.

> And if so perhaps a pointer where this in the Xen code base?
> 
>> +
>> +struct usbif_isoc_descriptor {
>> +    uint32_t   offset;
>> +    uint32_t   length;
>> +    uint32_t   actual_length;
>> +    int32_t    status;
>> +};
> 
> Ditto?

ISOC code is remove from this patch, so I'll address this later.

> 
>> +
>> +struct usbback_info;
>> +struct usbback_req;
>> +
>> +struct usbback_stub {
>> +    USBDevice  *dev;
>> +    USBPort    port;
>> +    unsigned   speed;
> 
> unsigned int?

Okay.

>> +    bool       attached;
>> +    QTAILQ_HEAD(submit_q_head, usbback_req) submit_q;
>> +};
>> +
>> +struct usbback_req {
>> +    struct usbback_info      *usbif;
>> +    struct usbback_stub      *stub;
>> +    struct usbif_urb_request req;
>> +    USBPacket                packet;
>> +
>> +    unsigned int             nr_buffer_segs; /* # of transfer_buffer segments */
>> +    unsigned int             nr_extra_segs;  /* # of iso_frame_desc segments  */
>> +
>> +    QTAILQ_ENTRY(usbback_req) q;
>> +
>> +    void                     *buffer;
>> +    void                     *isoc_buffer;
>> +    struct libusb_transfer   *xfer;
>> +};
>> +
>> +struct usbback_info {
>> +    struct XenDevice         xendev;  /* must be first */
>> +    USBBus                   bus;
>> +    void                     *urb_sring;
>> +    void                     *conn_sring;
>> +    struct usbif_urb_back_ring urb_ring;
>> +    struct usbif_conn_back_ring conn_ring;
>> +    int                      num_ports;
>> +    int                      usb_ver;
>> +    bool                     ring_error;
>> +    QTAILQ_HEAD(req_free_q_head, usbback_req) req_free_q;
>> +    struct usbback_stub      ports[USBBACK_MAXPORTS];
>> +    struct usbback_stub      *addr_table[USB_DEV_ADDR_SIZE];
>> +    QEMUBH                   *bh;
>> +};
>> +
>> +static unsigned int tr_debug = 3;
> 
> This surely should be zero :-)

Removed with the switch to xen_be_printf()

> 
>> +
>> +static void usbback_copy_buffer(struct usbback_req *usbback_req,
>> +                                struct usbif_isoc_descriptor *iso, bool out,
>> +                                unsigned len, unsigned off)
>> +{
>> +    struct usbif_request_segment *seg;
>> +    unsigned s, offset, copy_len, copy_off;
>> +    void *addr;
>> +
>> +    offset = 0;
>> +    copy_off = iso->offset;
>> +    for (s = 0; len && s < usbback_req->nr_buffer_segs; s++) {
>> +        seg = usbback_req->req.seg + s;
>> +        if (offset + seg->length > copy_off) {
>> +            addr = usbback_req->buffer + s * PAGE_SIZE + seg->offset +
>> +                   copy_off - offset;
>> +            copy_len = len;
>> +            if (copy_len > seg->length - copy_off + offset) {
>> +                copy_len = seg->length - copy_off + offset;
>> +            }
>> +            if (out) {
>> +                memcpy(usbback_req->xfer->buffer + off, addr, copy_len);
>> +            } else {
>> +                memcpy(addr, usbback_req->xfer->buffer + off, copy_len);
>> +            }
>> +            len -= copy_len;
>> +            off += copy_len;
>> +        }
>> +        offset += usbback_req->req.seg[s].length;
>> +    }
>> +    assert(!len);
>> +}
>> +
>> +int usbback_get_packets(USBPacket *p)
> 
> static ?

ISOC: removed

> 
>> +{
>> +    struct usbback_req *usbback_req;
>> +
>> +    usbback_req = container_of(p, struct usbback_req, packet);
>> +    assert(usbif_pipeisoc(usbback_req->req.pipe));
>> +    return usbback_req->req.u.isoc.nr_frame_desc_segs;
>> +}
>> +
>> +void usbback_set_iso_desc(USBPacket *p, struct libusb_transfer *xfer)
>> +{
>> +    struct usbback_req *usbback_req;
>> +    struct usbif_isoc_descriptor *iso;
>> +    struct usbif_request_segment *seg;
>> +    unsigned i, j, np, descr, off;
>> +
>> +    usbback_req = container_of(p, struct usbback_req, packet);
>> +    assert(usbif_pipeisoc(usbback_req->req.pipe));
>> +    usbback_req->xfer = xfer;
>> +    np = usbback_req->req.u.isoc.nr_frame_desc_segs;
>> +    descr = 0;
>> +    off = 0;
>> +
>> +    for (i = 0; i < usbback_req->nr_extra_segs; i++) {
>> +        seg = usbback_req->req.seg + usbback_req->nr_buffer_segs + i;
>> +        iso = usbback_req->isoc_buffer + i * PAGE_SIZE + seg->offset;
>> +        if ((seg->length % sizeof(*iso)) ||
>> +            (seg->length / sizeof(*iso) > np - descr) ||
>> +            (iso->offset + iso->length > usbback_req->req.buffer_length)) {
>> +            xen_be_printf(&usbback_req->usbif->xendev, 0,
>> +                          "iso segment length invalid\n");
>> +            xfer->num_iso_packets = descr;
>> +            while (descr < usbback_req->req.u.isoc.nr_frame_desc_segs) {
>> +                xfer->iso_packet_desc[descr].length = 0;
>> +                xfer->iso_packet_desc[descr].actual_length = 0;
>> +                descr++;
>> +            }
>> +            return;
>> +        }
>> +        for (j = 0; j < seg->length / sizeof(*iso); j++) {
>> +            xfer->iso_packet_desc[descr].length = iso->length;
>> +            xfer->iso_packet_desc[descr].actual_length = 0;
>> +            if (!usbif_pipein(usbback_req->req.pipe)) {
>> +                usbback_copy_buffer(usbback_req, iso, true, iso->length, off);
>> +                off += iso->length;
>> +            }
>> +            iso++;
>> +            descr++;
>> +        }
>> +    }
>> +}
>> +
>> +static struct usbback_req *usbback_get_req(struct usbback_info *usbif)
>> +{
>> +    struct usbback_req *usbback_req;
>> +
>> +    if (QTAILQ_EMPTY(&usbif->req_free_q)) {
>> +        usbback_req = g_malloc0(sizeof(*usbback_req));
>> +    } else {
>> +        usbback_req = QTAILQ_FIRST(&usbif->req_free_q);
>> +        QTAILQ_REMOVE(&usbif->req_free_q, usbback_req, q);
>> +    }
>> +    return usbback_req;
>> +}
>> +
>> +static void usbback_put_req(struct usbback_req *usbback_req)
>> +{
>> +    struct usbback_info *usbif;
>> +
>> +    usbif = usbback_req->usbif;
>> +    memset(usbback_req, 0, sizeof(*usbback_req));
>> +    QTAILQ_INSERT_HEAD(&usbif->req_free_q, usbback_req, q);
>> +}
>> +
>> +static int usbback_gnttab_map(struct usbback_info *usbif,
>> +                              struct usbback_req *usbback_req)
>> +{
>> +    unsigned int nr_segs, i, prot;
>> +    uint32_t ref[USBIF_MAX_SEGMENTS_PER_REQUEST];
>> +    struct XenDevice *xendev = &usbif->xendev;
>> +    struct usbif_request_segment *seg;
>> +    void *addr;
>> +
>> +    nr_segs = usbback_req->nr_buffer_segs + usbback_req->nr_extra_segs;
>> +    if (!nr_segs) {
>> +        return 0;
>> +    }
>> +
>> +    if (nr_segs > USBIF_MAX_SEGMENTS_PER_REQUEST) {
>> +        xen_be_printf(xendev, 0, "bad number of segments in request (%d)\n",
>> +                      nr_segs);
>> +        return -EINVAL;
>> +    }
>> +
>> +    for (i = 0; i < nr_segs; i++) {
>> +        if ((unsigned)usbback_req->req.seg[i].offset +
>> +            (unsigned)usbback_req->req.seg[i].length > PAGE_SIZE) {
>> +            xen_be_printf(xendev, 0, "segment crosses page boundary\n");
>> +            return -EINVAL;
>> +        }
>> +    }
>> +
>> +    if (usbback_req->nr_buffer_segs) {
>> +        prot = PROT_READ;
>> +        if (usbif_pipein(usbback_req->req.pipe)) {
>> +                prot |= PROT_WRITE;
>> +        }
>> +        for (i = 0; i < usbback_req->nr_buffer_segs; i++) {
>> +            ref[i] = usbback_req->req.seg[i].gref;
>> +        }
>> +        usbback_req->buffer = xc_gnttab_map_domain_grant_refs(xendev->gnttabdev,
>> +            usbback_req->nr_buffer_segs, xendev->dom, ref, prot);
>> +
>> +        if (!usbback_req->buffer) {
>> +            return -ENOMEM;
>> +        }
>> +
>> +        for (i = 0; i < usbback_req->nr_buffer_segs; i++) {
>> +            seg = usbback_req->req.seg + i;
>> +            addr = usbback_req->buffer + i * PAGE_SIZE + seg->offset;
>> +            qemu_iovec_add(&usbback_req->packet.iov, addr, seg->length);
>> +        }
>> +    }
>> +
>> +    if (!usbif_pipeisoc(usbback_req->req.pipe)) {
>> +        return 0;
>> +    }
>> +
>> +    if (!usbback_req->nr_extra_segs) {
>> +        xen_be_printf(xendev, 0, "iso request without descriptor segments\n");
>> +        return -EINVAL;
> 
> Could this be moved before the xc_gnttab_map_domain_grant_refs and qemu_iovec_add
> is done? And should you unmap ->buffer?

No, this is isoc only (I'm keeping the main infrastructure for
isoc requests). And ->buffer is unmapped in usbback_do_response()
which is called in error case.

> 
>> +    }
>> +
>> +    prot = PROT_READ | PROT_WRITE;
>> +    for (i = 0; i < usbback_req->nr_extra_segs; i++) {
>> +        ref[i] = usbback_req->req.seg[i + usbback_req->req.nr_buffer_segs].gref;
>> +    }
>> +    usbback_req->isoc_buffer = xc_gnttab_map_domain_grant_refs(
>> +         xendev->gnttabdev, usbback_req->nr_extra_segs, xendev->dom, ref, prot);
>> +
>> +    if (!usbback_req->isoc_buffer) {
> 
> No unmapping of the ->buffer?

As above.

> 
>> +        return -ENOMEM;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static int usbback_init_packet(struct usbback_req *usbback_req)
>> +{
>> +    USBPacket *packet = &usbback_req->packet;
>> +    USBDevice *dev = usbback_req->stub->dev;
>> +    USBEndpoint *ep;
>> +    unsigned int pid, ep_nr;
>> +    bool sok;
>> +
>> +    qemu_iovec_init(&packet->iov, USBIF_MAX_SEGMENTS_PER_REQUEST);
>> +    pid = usbif_pipein(usbback_req->req.pipe) ? USB_TOKEN_IN : USB_TOKEN_OUT;
>> +    ep_nr = usbif_pipeendpoint(usbback_req->req.pipe);
>> +    sok = !!(usbback_req->req.transfer_flags & 1);
>> +    if (usbif_pipetype(usbback_req->req.pipe) == USBIF_PIPE_TYPE_CTRL) {
>> +        ep_nr = 0;
>> +        sok = false;
>> +    }
>> +    ep = usb_ep_get(dev, pid, ep_nr);
>> +    usb_packet_setup(packet, pid, ep, 0, 1, sok, true);
>> +
>> +    switch (usbif_pipetype(usbback_req->req.pipe)) {
>> +    case USBIF_PIPE_TYPE_ISOC:
>> +        TR_REQ("iso transfer %s: buflen: %x, %d frames\n",
>> +               (pid == USB_TOKEN_IN) ? "in" : "out",
>> +               usbback_req->req.buffer_length,
>> +               usbback_req->req.u.isoc.nr_frame_desc_segs);
>> +        break;
>> +
>> +    case USBIF_PIPE_TYPE_INT:
>> +        TR_REQ("int transfer %s: buflen: %x\n",
>> +               (pid == USB_TOKEN_IN) ? "in" : "out",
>> +               usbback_req->req.buffer_length);
>> +        break;
>> +
>> +    case USBIF_PIPE_TYPE_CTRL:
>> +        packet->parameter = *(uint64_t *)usbback_req->req.u.ctrl;
>> +        TR_REQ("ctrl parameter: %lx, buflen: %x\n", packet->parameter,
>> +               usbback_req->req.buffer_length);
>> +        break;
>> +
>> +    case USBIF_PIPE_TYPE_BULK:
>> +        TR_REQ("bulk transfer %s: buflen: %x\n",
>> +               (pid == USB_TOKEN_IN) ? "in" : "out",
>> +               usbback_req->req.buffer_length);
>> +        break;
>> +    default:
>> +        return -EINVAL;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static void usbback_do_response(struct usbback_req *usbback_req,
>> +                                int32_t status, int32_t actual_length,
>> +                                int32_t error_count)
> 
> Should the 'actual_length' and 'error_count' be unsigned int?

Same type as in the interface header. Changing this would probably
make sense, but this is out of scope for this series.

>> +{
>> +    struct usbback_info *usbif;
>> +    struct usbif_urb_response *res;
>> +    struct XenDevice *xendev;
>> +    unsigned int notify;
>> +
>> +    TR_REQ("id %d, status %d, length %d, errcnt %d\n", usbback_req->req.id,
>> +           status, actual_length, error_count);
>> +
>> +    usbif = usbback_req->usbif;
>> +    xendev = &usbif->xendev;
>> +
>> +    if (usbback_req->packet.iov.iov) {
> 
> qemu_iovec_is_zero ?

No. Would be true if qemu_iovec_add() wasn't called, but we need to call
qemu_iovec_destroy() in this case, too.

> 
>> +        qemu_iovec_destroy(&usbback_req->packet.iov);
>> +    }
>> +
>> +    if (usbback_req->buffer) {
>> +        xc_gnttab_munmap(xendev->gnttabdev, usbback_req->buffer,
>> +                         usbback_req->nr_buffer_segs);
>> +        usbback_req->buffer = NULL;
> 
> nr_buffer_segs = 0?

Not needed. Will be set _always_ in case of buffer != NULL.

> 
>> +    }
>> +
>> +    if (usbback_req->isoc_buffer) {
>> +        xc_gnttab_munmap(xendev->gnttabdev, usbback_req->isoc_buffer,
>> +                         usbback_req->nr_extra_segs);
> 
> nr_extra_segts = 0 ?
> Should 'isoc_buffer' be set to NULL as well?

Setting isoc_buffer to NULL seems to be a good idea.

> 
>> +    }
>> +
>> +    res = RING_GET_RESPONSE(&usbif->urb_ring, usbif->urb_ring.rsp_prod_pvt);
>> +    res->id = usbback_req->req.id;
>> +    res->status = status;
>> +    res->actual_length = actual_length;
>> +    res->error_count = error_count;
>> +    res->start_frame = 0;
>> +    usbif->urb_ring.rsp_prod_pvt++;
>> +    RING_PUSH_RESPONSES_AND_CHECK_NOTIFY(&usbif->urb_ring, notify);
>> +
>> +    if (notify) {
>> +        xen_be_send_notify(xendev);
>> +    }
>> +
>> +    usbback_put_req(usbback_req);
>> +}
>> +
>> +static void usbback_do_response_ret(struct usbback_req *usbback_req,
>> +                                    int32_t status)
>> +{
>> +    usbback_do_response(usbback_req, status, 0, 0);
>> +}
>> +
>> +static int32_t usbback_xlat_status(int32_t status)
>> +{
>> +    int32_t ret = -ESHUTDOWN;
>> +
>> +    switch (status) {
>> +    case USB_RET_SUCCESS:
>> +        ret = 0;
>> +        break;
>> +    case USB_RET_NODEV:
>> +        ret = -ENODEV;
>> +        break;
>> +    case USB_RET_STALL:
>> +        ret = -EPIPE;
>> +        break;
>> +    case USB_RET_BABBLE:
>> +        ret = -EOVERFLOW;
>> +        break;
>> +    case USB_RET_IOERROR:
>> +        ret = -EPROTO;
>> +        break;
>> +    }
>> +
>> +    return ret;
>> +}
>> +
>> +static void usbback_packet_complete(USBPacket *packet)
>> +{
>> +    struct usbback_req *usbback_req;
>> +    int32_t status, error_count, actual_length;
>> +    unsigned int i, j, off;
>> +    struct usbif_isoc_descriptor *iso;
>> +    struct usbif_request_segment *seg;
>> +    struct libusb_iso_packet_descriptor *libusb_iso;
>> +
>> +    error_count = 0;
>> +    actual_length = 0;
>> +    off = 0;
>> +    usbback_req = container_of(packet, struct usbback_req, packet);
>> +
>> +    QTAILQ_REMOVE(&usbback_req->stub->submit_q, usbback_req, q);
>> +
>> +    status = usbback_xlat_status(packet->status);
>> +    if (usbback_req->isoc_buffer) {
>> +        libusb_iso = usbback_req->xfer->iso_packet_desc;
>> +        j = 0;
>> +        for (i = 0; i < usbback_req->nr_extra_segs; i++) {
>> +            seg = usbback_req->req.seg + usbback_req->nr_buffer_segs + i;
>> +            iso = usbback_req->isoc_buffer + i * PAGE_SIZE + seg->offset;
>> +            for (j = 0; j < seg->length / sizeof(*iso); j++) {
>> +                iso->actual_length = libusb_iso->actual_length;
>> +                iso->status = usbback_xlat_status(libusb_iso->status);
>> +                actual_length += libusb_iso->actual_length;
>> +                error_count += iso->status ? 1 : 0;
>> +                if (usbif_pipein(usbback_req->req.pipe)) {
>> +                    usbback_copy_buffer(usbback_req, iso, false,
>> +                                        iso->actual_length, off);
>> +                    off += iso->length;
>> +                }
>> +                libusb_iso++;
>> +                iso++;
>> +            }
>> +        }
>> +    } else {
>> +        actual_length = packet->actual_length;
>> +    }
>> +
>> +    usbback_do_response(usbback_req, status, actual_length, error_count);
>> +}
>> +
>> +static void usbback_set_address(struct usbback_info *usbif,
>> +                                struct usbback_stub *stub, int cur_addr,
>> +                                int new_addr)
>> +{
>> +    if (cur_addr) {
>> +        usbif->addr_table[cur_addr] = NULL;
>> +    }
>> +    if (new_addr) {
>> +        usbif->addr_table[new_addr] = stub;
>> +    }
>> +}
>> +
>> +static bool usbback_cancel_req(struct usbback_req *usbback_req)
>> +{
>> +    bool ret = false;
>> +
>> +    if (usb_packet_is_inflight(&usbback_req->packet)) {
>> +        usb_cancel_packet(&usbback_req->packet);
>> +        ret = true;
>> +    }
>> +    return ret;
>> +}
>> +
>> +static void usbback_process_unlink_req(struct usbback_req *usbback_req)
>> +{
>> +    struct usbback_info *usbif;
>> +    struct usbback_req *unlink_req;
>> +    unsigned int id, devnum;
>> +    int ret;
> 
> Why 'ret'? You are not returning the value.

It's the return status of the request (parameter of
usbback_do_response_ret()).

> 
>> +
>> +    usbif = usbback_req->usbif;
>> +    ret = 0;
>> +    id = usbback_req->req.u.unlink.unlink_id;
>> +    TR_REQ("unlink id %d\n", id);
>> +    devnum = usbif_pipedevice(usbback_req->req.pipe);
>> +    if (unlikely(devnum == 0)) {
>> +        usbback_req->stub = usbif->ports +
>> +                            usbif_pipeportnum(usbback_req->req.pipe);
> 
> Should you check that usbif_pipeporntnum value does not exceeed USBBACK_MAXPORTS ?
> Ah, no. The macro usbif_piportnum will mask it so it will always be within
> ports[0..USBBACK_MAXPORTS].
> 
>> +        if (unlikely(!usbback_req->stub)) {
>> +            ret = -ENODEV;
>> +            goto fail_response;
>> +        }
>> +    } else {
>> +        if (unlikely(!usbif->addr_table[devnum])) {
> 
> Should we doublecheck that devnm < USB_DEV_ADDR_SIZE ?

I think it's better to do:

#define USB_DEV_ADDR_SIZE       (USBIF_PIPE_DEV_MASK + 1)

> 
>> +            ret = -ENODEV;
>> +            goto fail_response;
>> +        }
>> +        usbback_req->stub = usbif->addr_table[devnum];
>> +    }
>> +
>> +    QTAILQ_FOREACH(unlink_req, &usbback_req->stub->submit_q, q) {
>> +        if (unlink_req->req.id == id) {
>> +            if (usbback_cancel_req(unlink_req)) {
>> +                usbback_do_response_ret(unlink_req, -EPROTO);
>> +            }
>> +            break;
>> +        }
>> +    }
>> +
>> +fail_response:
>> +    usbback_do_response_ret(usbback_req, ret);
>> +}
>> +
> 
> Could you add a comment explaining what positive return values mean?

Okay.

> 
>> +static int usbback_check_and_submit(struct usbback_req *usbback_req)
>> +{
>> +    struct usbback_info *usbif;
>> +    unsigned int devnum;
>> +    struct usbback_stub *stub;
>> +    struct usbif_ctrlrequest *ctrl;
>> +    int ret;
>> +    uint16_t wValue;
>> +
>> +    usbif = usbback_req->usbif;
>> +    stub = NULL;
>> +    devnum = usbif_pipedevice(usbback_req->req.pipe);
>> +    ctrl = (struct usbif_ctrlrequest *)usbback_req->req.u.ctrl;
>> +    wValue = le16_to_cpu(ctrl->wValue);
>> +
>> +    /*
>> +     * When the device is first connected or resetted, USB device has no
>> +     * address. In this initial state, following requests are sent to device
>> +     * address (#0),
>> +     *
>> +     *  1. GET_DESCRIPTOR (with Descriptor Type is "DEVICE") is sent,
>> +     *     and OS knows what device is connected to.
>> +     *
>> +     *  2. SET_ADDRESS is sent, and then device has its address.
>> +     *
>> +     * In the next step, SET_CONFIGURATION is sent to addressed device, and
>> +     * then the device is finally ready to use.
>> +     */
>> +    if (unlikely(devnum == 0)) {
>> +        stub = usbif->ports + usbif_pipeportnum(usbback_req->req.pipe) - 1;
>> +        if (!stub->dev || !stub->attached) {
>> +            ret = -ENODEV;
>> +            goto do_response;
>> +        }
>> +
>> +        switch (ctrl->bRequest) {
>> +        case USB_REQ_GET_DESCRIPTOR:
>> +            /*
>> +             * GET_DESCRIPTOR request to device #0.
>> +             * through to normal transfer.
> 
> s/to//
>> +             */
>> +            TR_REQ("devnum 0 GET_DESCRIPTOR\n");
>> +            usbback_req->stub = stub;
>> +            return 0;
>> +        case USB_REQ_SET_ADDRESS:
>> +            /*
>> +             * SET_ADDRESS request to device #0.
>> +             * add attached device to addr_table.
>> +             */
>> +            TR_REQ("devnum 0 SET_ADDRESS\n");
>> +            usbback_set_address(usbif, stub, 0, wValue);
>> +            ret = 0;
>> +            break;
>> +        default:
>> +            ret = -EINVAL;
>> +            break;
>> +        }
>> +        goto do_response;
>> +    }
>> +
>> +    if (unlikely(!usbif->addr_table[devnum])) {
>> +            ret = -ENODEV;
>> +            goto do_response;
>> +    }
>> +    usbback_req->stub = usbif->addr_table[devnum];
>> +
>> +    /*
>> +     * Check special request
>> +     */
>> +    if (ctrl->bRequest != USB_REQ_SET_ADDRESS) {
>> +        return 0;
>> +    }
>> +
>> +    /*
>> +     * SET_ADDRESS request to addressed device.
>> +     * change addr or remove from addr_table.
>> +     */
>> +    usbback_set_address(usbif, usbback_req->stub, devnum, wValue);
>> +    ret = 0;
>> +
>> +do_response:
>> +    usbback_do_response_ret(usbback_req, ret);
>> +    return 1;
>> +}
>> +
>> +static void usbback_dispatch(struct usbback_req *usbback_req)
>> +{
>> +    int ret;
>> +    unsigned int devnum;
>> +    struct usbback_info *usbif;
>> +
>> +    TR_REQ("start req_id %d pipe %08x\n", usbback_req->req.id,
>> +           usbback_req->req.pipe);
>> +
>> +    usbif = usbback_req->usbif;
>> +
>> +    /* unlink request */
>> +    if (unlikely(usbif_pipeunlink(usbback_req->req.pipe))) {
>> +        usbback_process_unlink_req(usbback_req);
>> +        return;
>> +    }
>> +
>> +    if (usbif_pipectrl(usbback_req->req.pipe)) {
>> +        if (usbback_check_and_submit(usbback_req)) {
> 
> Should you at least report the errors if there are any?

Hmm, why? These are parameter errors which are the fault of the frontend
only. The frontend shouldn't be capable of flooding the backend's log.

> 
>> +            return;
>> +        }
>> +    } else {
>> +        devnum = usbif_pipedevice(usbback_req->req.pipe);
>> +        usbback_req->stub = usbif->addr_table[devnum];
>> +
>> +        if (!usbback_req->stub || !usbback_req->stub->attached) {
>> +            ret = -ENODEV;
>> +            goto fail_response;
>> +        }
>> +    }
>> +
>> +    QTAILQ_INSERT_TAIL(&usbback_req->stub->submit_q, usbback_req, q);
>> +
>> +    usbback_req->nr_buffer_segs = usbback_req->req.nr_buffer_segs;
>> +    usbback_req->nr_extra_segs = usbif_pipeisoc(usbback_req->req.pipe) ?
>> +                                 usbback_req->req.u.isoc.nr_frame_desc_segs : 0;
>> +
>> +    ret = usbback_init_packet(usbback_req);
>> +    if (ret) {
>> +        xen_be_printf(&usbif->xendev, 0, "invalid request\n");
>> +        ret = -ESHUTDOWN;
>> +        goto fail_free_urb;
>> +    }
>> +
>> +    ret = usbback_gnttab_map(usbif, usbback_req);
>> +    if (ret) {
>> +        xen_be_printf(&usbif->xendev, 0, "invalid buffer\n");
> 
> Should the 'ret' be at least included?

Hmm, yes.

> 
>> +        ret = -ESHUTDOWN;
>> +        goto fail_free_urb;
>> +    }
>> +
>> +    usb_handle_packet(usbback_req->stub->dev, &usbback_req->packet);
>> +    if (usbback_req->packet.status != USB_RET_ASYNC) {
>> +        usbback_packet_complete(&usbback_req->packet);
>> +    }
>> +    return;
>> +
>> +fail_free_urb:
>> +    QTAILQ_REMOVE(&usbback_req->stub->submit_q, usbback_req, q);
>> +
>> +fail_response:
>> +    usbback_do_response_ret(usbback_req, ret);
>> +}
>> +
>> +static void usbback_bh(void *opaque)
>> +{
>> +    struct usbback_info *usbif;
>> +    struct usbif_urb_back_ring *urb_ring;
>> +    struct usbback_req *usbback_req;
>> +    RING_IDX rc, rp;
>> +    unsigned int more_to_do;
>> +
>> +    usbif = opaque;
>> +    if (usbif->ring_error) {
>> +        return;
>> +    }
>> +
>> +    urb_ring = &usbif->urb_ring;
>> +    rc = urb_ring->req_cons;
>> +    rp = urb_ring->sring->req_prod;
>> +    xen_rmb(); /* Ensure we see queued requests up to 'rp'. */
>> +
>> +    if (RING_REQUEST_PROD_OVERFLOW(urb_ring, rp)) {
>> +        rc = urb_ring->rsp_prod_pvt;
>> +        xen_be_printf(&usbif->xendev, 0, "domU provided bogus ring requests "
>> +                      "(%#x - %#x = %u). Halting ring processing.\n",
>> +                      rp, rc, rp - rc);
>> +        usbif->ring_error = true;
>> +        return;
>> +    }
>> +
>> +    while (rc != rp) {
>> +        if (RING_REQUEST_CONS_OVERFLOW(urb_ring, rc)) {
>> +            break;
>> +        }
>> +        usbback_req = usbback_get_req(usbif);
>> +
>> +        usbback_req->req = *RING_GET_REQUEST(urb_ring, rc);
>> +        usbback_req->usbif = usbif;
>> +
>> +        usbback_dispatch(usbback_req);
>> +
>> +        urb_ring->req_cons = ++rc;
>> +    }
>> +
>> +    RING_FINAL_CHECK_FOR_REQUESTS(urb_ring, more_to_do);
>> +    if (more_to_do) {
>> +        qemu_bh_schedule(usbif->bh);
>> +    }
>> +}
>> +
>> +static void usbback_hotplug_notify(struct usbback_info *usbif, unsigned port)
>> +{
>> +    struct usbif_conn_back_ring *ring = &usbif->conn_ring;
>> +    struct usbif_conn_request *req;
>> +    struct usbif_conn_response *res;
>> +    uint16_t id;
>> +    unsigned int notify;
>> +
>> +    if (!usbif->conn_sring) {
>> +        return;
>> +    }
>> +
>> +    req = RING_GET_REQUEST(ring, ring->req_cons);
>> +    id = req->id;
>> +    ring->req_cons++;
>> +    ring->sring->req_event = ring->req_cons + 1;
>> +
>> +    res = RING_GET_RESPONSE(ring, ring->rsp_prod_pvt);
>> +    res->id = id;
>> +    res->portnum = port;
>> +    res->speed = usbif->ports[port - 1].speed;
>> +    ring->rsp_prod_pvt++;
>> +    RING_PUSH_RESPONSES_AND_CHECK_NOTIFY(ring, notify);
>> +
>> +    if (notify) {
>> +        xen_be_send_notify(&usbif->xendev);
>> +    }
>> +
>> +    TR_BUS("hotplug port %d speed %d\n", port, res->speed);
>> +}
>> +
>> +static void usbback_portid_remove(struct usbback_info *usbif, unsigned port)
>> +{
>> +    USBPort *p;
>> +
>> +    if (!usbif->ports[port - 1].dev) {
>> +        return;
>> +    }
>> +
>> +    p = &(usbif->ports[port - 1].port);
>> +    snprintf(p->path, sizeof(p->path), "%d", 99);
>> +
>> +    object_unparent(OBJECT(usbif->ports[port - 1].dev));
>> +    usbif->ports[port - 1].dev = NULL;
>> +    usbif->ports[port - 1].speed = USBIF_SPEED_NONE;
>> +    usbif->ports[port - 1].attached = false;
>> +    usbback_hotplug_notify(usbif, port);
>> +
>> +    TR_BUS("port %d removed\n", port);
>> +}
>> +
>> +static void usbback_portid_add(struct usbback_info *usbif, unsigned port,
>> +                               char *busid)
>> +{
>> +    unsigned speed;
>> +    char *portname;
>> +    USBPort *p;
>> +    Error *local_err = NULL;
>> +    QDict *qdict;
>> +    QemuOpts *opts;
>> +
>> +    if (usbif->ports[port - 1].dev) {
>> +        return;
>> +    }
>> +
>> +    portname = strchr(busid, '-');
>> +    if (!portname) {
>> +        xen_be_printf(&usbif->xendev, 0, "device %s illegal specification\n",
>> +                      busid);
>> +        return;
>> +    }
>> +    portname++;
>> +    p = &(usbif->ports[port - 1].port);
>> +    snprintf(p->path, sizeof(p->path), "%s", portname);
>> +
>> +    qdict = qdict_new();
>> +    qdict_put(qdict, "driver", qstring_from_str("usb-host"));
>> +    qdict_put(qdict, "hostbus", qint_from_int(atoi(busid)));
>> +    qdict_put(qdict, "hostport", qstring_from_str(portname));
>> +    qdict_put(qdict, "xen-iso-passthrough", qbool_from_int(1));
>> +    opts = qemu_opts_from_qdict(qemu_find_opts("device"), qdict, &local_err);
>> +    if (local_err) {
>> +        goto err;
>> +    }
>> +    usbif->ports[port - 1].dev = USB_DEVICE(qdev_device_add(opts));
>> +    if (!usbif->ports[port - 1].dev) {
>> +        goto err;
>> +    }
>> +    QDECREF(qdict);
>> +    snprintf(p->path, sizeof(p->path), "%d", port);
>> +    speed = usbif->ports[port - 1].dev->speed;
> 
> I am a bit confused as how you get the 'speed'? We are adding a device
> and we don't read this?

Huh? I don't understand. We exactly do this: we are adding a device
and then we read the speed to be able to report it.

> 
> 
>> +    switch (speed) {
>> +    case USB_SPEED_LOW:
>> +        speed = USBIF_SPEED_LOW;
>> +        break;
>> +    case USB_SPEED_FULL:
>> +        speed = USBIF_SPEED_FULL;
>> +        break;
>> +    case USB_SPEED_HIGH:
>> +        speed = (usbif->usb_ver < USB_VER_USB20) ?
>> +                USBIF_SPEED_NONE : USBIF_SPEED_HIGH;
>> +        break;
>> +    default:
>> +        speed = USBIF_SPEED_NONE;
>> +        break;
>> +    }
>> +    if (speed == USBIF_SPEED_NONE) {
>> +        xen_be_printf(&usbif->xendev, 0, "device %s wrong speed\n", busid);
>> +        object_unparent(OBJECT(usbif->ports[port - 1].dev));
>> +        usbif->ports[port - 1].dev = NULL;
>> +        return;
>> +    }
>> +    usb_device_reset(usbif->ports[port - 1].dev);
>> +    usbif->ports[port - 1].speed = speed;
>> +    usbif->ports[port - 1].attached = true;
>> +    QTAILQ_INIT(&usbif->ports[port - 1].submit_q);
>> +    usbback_hotplug_notify(usbif, port);
>> +
>> +    TR_BUS("port %d attached\n", port);
>> +    return;
>> +
>> +err:
>> +    QDECREF(qdict);
>> +    snprintf(p->path, sizeof(p->path), "%d", 99);
>> +    xen_be_printf(&usbif->xendev, 0, "device %s could not be opened\n", busid);
>> +}
>> +
>> +static void usbback_process_port(struct usbback_info *usbif, unsigned port)
>> +{
>> +    char node[8];
>> +    char *busid;
>> +
>> +    snprintf(node, sizeof(node), "port/%d", port);
>> +    busid = xenstore_read_be_str(&usbif->xendev, node);
>> +    if (busid == NULL) {
>> +        xen_be_printf(&usbif->xendev, 0, "xenstore_read %s failed\n", node);
>> +        return;
>> +    }
>> +
>> +    /* Remove portid, if the port is not connected.  */
>> +    if (strlen(busid) == 0) {
>> +        usbback_portid_remove(usbif, port);
>> +    } else {
>> +        usbback_portid_add(usbif, port, busid);
>> +    }
>> +
>> +    g_free(busid);
>> +}
>> +
>> +static void usbback_disconnect(struct XenDevice *xendev)
>> +{
>> +    struct usbback_info *usbif;
>> +    struct usbback_req *req, *tmp;
>> +    unsigned int i;
>> +
>> +    TR_BUS("start\n");
>> +
>> +    usbif = container_of(xendev, struct usbback_info, xendev);
>> +
>> +    xen_be_unbind_evtchn(xendev);
>> +
>> +    if (usbif->urb_sring) {
>> +        xc_gnttab_munmap(xendev->gnttabdev, usbif->urb_sring, 1);
>> +        usbif->urb_sring = NULL;
>> +    }
>> +    if (usbif->conn_sring) {
>> +        xc_gnttab_munmap(xendev->gnttabdev, usbif->conn_sring, 1);
>> +        usbif->conn_sring = NULL;
>> +    }
>> +
>> +    for (i = 0; i < usbif->num_ports; i++) {
>> +        if (!usbif->ports[i].dev) {
>> +            continue;
>> +        }
>> +        QTAILQ_FOREACH_SAFE(req, &usbif->ports[i].submit_q, q, tmp) {
>> +            usbback_cancel_req(req);
>> +        }
>> +    }
>> +
>> +    TR_BUS("finished\n");
>> +}
>> +
>> +static int usbback_connect(struct XenDevice *xendev)
>> +{
>> +    struct usbback_info *usbif;
>> +    struct usbif_urb_sring *urb_sring;
>> +    struct usbif_conn_sring *conn_sring;
>> +    int urb_ring_ref;
>> +    int conn_ring_ref;
>> +    unsigned int i;
>> +
>> +    TR_BUS("start\n");
>> +
>> +    usbif = container_of(xendev, struct usbback_info, xendev);
>> +
>> +    if (xenstore_read_fe_int(xendev, "urb-ring-ref", &urb_ring_ref)) {
>> +        TR("error reading urb-ring-ref\n");
>> +        return -1;
>> +    }
>> +    if (xenstore_read_fe_int(xendev, "conn-ring-ref", &conn_ring_ref)) {
>> +        TR("error reading conn-ring-ref\n");
>> +        return -1;
>> +    }
>> +    if (xenstore_read_fe_int(xendev, "event-channel", &xendev->remote_port)) {
>> +        TR("error reading event-channel\n");
>> +        return -1;
>> +    }
>> +
>> +    usbif->urb_sring = xc_gnttab_map_grant_ref(xendev->gnttabdev, xendev->dom,
>> +                                               urb_ring_ref,
>> +                                               PROT_READ | PROT_WRITE);
>> +    usbif->conn_sring = xc_gnttab_map_grant_ref(xendev->gnttabdev, xendev->dom,
>> +                                                conn_ring_ref,
>> +                                                PROT_READ | PROT_WRITE);
>> +    if (!usbif->urb_sring || !usbif->conn_sring) {
>> +        TR("error mapping rings\n");
>> +        usbback_disconnect(xendev);
>> +        return -1;
>> +    }
>> +
>> +    urb_sring = usbif->urb_sring;
>> +    conn_sring = usbif->conn_sring;
>> +    BACK_RING_INIT(&usbif->urb_ring, urb_sring, XC_PAGE_SIZE);
>> +    BACK_RING_INIT(&usbif->conn_ring, conn_sring, XC_PAGE_SIZE);
>> +
>> +    xen_be_bind_evtchn(xendev);
>> +
>> +    xen_be_printf(xendev, 1, "urb-ring-ref %d, conn-ring-ref %d, "
>> +                  "remote port %d, local port %d\n", urb_ring_ref,
>> +                  conn_ring_ref, xendev->remote_port, xendev->local_port);
>> +
>> +    for (i = 1; i <= usbif->num_ports; i++) {
>> +        if (usbif->ports[i - 1].dev) {
>> +            usbback_hotplug_notify(usbif, i);
>> +        }
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static void usbback_backend_changed(struct XenDevice *xendev, const char *node)
>> +{
>> +    struct usbback_info *usbif;
>> +    unsigned int i;
>> +
>> +    TR_BUS("path %s\n", node);
>> +
>> +    usbif = container_of(xendev, struct usbback_info, xendev);
>> +    for (i = 1; i <= usbif->num_ports; i++) {
>> +        usbback_process_port(usbif, i);
>> +    }
>> +}
>> +
>> +static int usbback_init(struct XenDevice *xendev)
>> +{
>> +    struct usbback_info *usbif;
>> +
>> +    TR_BUS("start\n");
>> +
>> +    usbif = container_of(xendev, struct usbback_info, xendev);
>> +
>> +    if (xenstore_read_be_int(xendev, "num-ports", &usbif->num_ports) ||
>> +        usbif->num_ports < 1 || usbif->num_ports > USBBACK_MAXPORTS) {
> 
> I think the compiler may be free to re-order these 'or' statements.

No. The C standard is very clear about this: logical or's are evaluated
from left to right. Otherwise something like: if (!buff || buff->x)
wouldn't work.

> 
> Could this be split in two 'if' ? One to get the value (and if it failed
> then print), and then the other 'if to check for the validity of it?
> 
>> +        xen_be_printf(xendev, 0, "num-ports not readable or out of bounds\n");
>> +        return -1;
>> +    }
>> +    if (xenstore_read_be_int(xendev, "usb-ver", &usbif->usb_ver) ||
>> +        (usbif->usb_ver != USB_VER_USB11 && usbif->usb_ver != USB_VER_USB20)) {
> 
> Ditto.
>> +        xen_be_printf(xendev, 0, "usb-ver not readable or out of bounds\n");
>> +        return -1;
>> +    }
>> +
>> +    usbback_backend_changed(xendev, "port");
>> +
>> +    TR_BUS("finished\n");
>> +
>> +    return 0;
>> +}
>> +
>> +static void xen_bus_attach(USBPort *port)
>> +{
>> +    struct usbback_info *usbif;
>> +
>> +    TR_BUS("called\n");
>> +    usbif = port->opaque;
>> +    usbif->ports[port->index].attached = true;
>> +    usbback_hotplug_notify(usbif, port->index + 1);
>> +}
>> +
>> +static void xen_bus_detach(USBPort *port)
>> +{
>> +    struct usbback_info *usbif;
>> +
>> +    TR_BUS("called\n");
>> +    usbif = port->opaque;
>> +    usbif->ports[port->index].attached = false;
>> +    usbback_hotplug_notify(usbif, port->index + 1);
>> +}
>> +
>> +static void xen_bus_child_detach(USBPort *port, USBDevice *child)
>> +{
>> +    TR_BUS("called\n");
>> +}
>> +
>> +static void xen_bus_complete(USBPort *port, USBPacket *packet)
>> +{
>> +    TR_REQ("called\n");
>> +    usbback_packet_complete(packet);
>> +}
>> +
>> +static USBPortOps xen_usb_port_ops = {
>> +    .attach = xen_bus_attach,
>> +    .detach = xen_bus_detach,
>> +    .child_detach = xen_bus_child_detach,
>> +    .complete = xen_bus_complete,
>> +};
>> +
>> +static USBBusOps xen_usb_bus_ops = {
>> +};
>> +
>> +static void usbback_alloc(struct XenDevice *xendev)
>> +{
>> +    struct usbback_info *usbif;
>> +    USBPort *p;
>> +    unsigned int i, max_grants;
>> +
>> +    usbif = container_of(xendev, struct usbback_info, xendev);
>> +
>> +    usb_bus_new(&usbif->bus, sizeof(usbif->bus), &xen_usb_bus_ops, xen_sysdev);
>> +    for (i = 0; i < USBBACK_MAXPORTS; i++) {
>> +        p = &(usbif->ports[i].port);
>> +        usb_register_port(&usbif->bus, p, usbif, i, &xen_usb_port_ops,
>> +                          USB_SPEED_MASK_LOW | USB_SPEED_MASK_FULL |
>> +                          USB_SPEED_MASK_HIGH);
>> +        snprintf(p->path, sizeof(p->path), "%d", 99);
>> +    }
>> +
>> +    QTAILQ_INIT(&usbif->req_free_q);
>> +    usbif->bh = qemu_bh_new(usbback_bh, usbif);
>> +
>> +    max_grants = USBIF_MAX_SEGMENTS_PER_REQUEST * USB_URB_RING_SIZE + 2;
> 
> Why the '2' ?

Connect and request rings. I'll add a comment.

> 
>> +    if (xc_gnttab_set_max_grants(xendev->gnttabdev, max_grants) < 0) {
>> +        xen_be_printf(xendev, 0, "xc_gnttab_set_max_grants failed: %s\n",
>> +                      strerror(errno));
>> +    }
>> +}
>> +
>> +static int usbback_free(struct XenDevice *xendev)
>> +{
>> +    struct usbback_info *usbif;
>> +    struct usbback_req *usbback_req;
>> +    unsigned int i;
>> +
>> +    TR_BUS("start\n");
>> +
>> +    usbback_disconnect(xendev);
>> +    usbif = container_of(xendev, struct usbback_info, xendev);
>> +    for (i = 1; i <= usbif->num_ports; i++) {
>> +        usbback_portid_remove(usbif, i);
>> +    }
> 
> You also need:
> 	for (i=0; i < USBBACK_MAXPORTS; i++) {
> 	        p = &(usbif->ports[i].port);
> 		usb_unregister_port(&usbif->bus, p);
> 	}
> 
> I think?

Hmm, you might be right.

>> +
>> +    while (!QTAILQ_EMPTY(&usbif->req_free_q)) {
>> +        usbback_req = QTAILQ_FIRST(&usbif->req_free_q);
>> +        QTAILQ_REMOVE(&usbif->req_free_q, usbback_req, q);
>> +        g_free(usbback_req);
>> +    }
>> +
>> +    qemu_bh_delete(usbif->bh);
>> +
>> +    usb_bus_release(&usbif->bus);
>> +
>> +    TR_BUS("finished\n");
>> +
>> +    return 0;
>> +}
>> +
>> +static void usbback_event(struct XenDevice *xendev)
>> +{
>> +    struct usbback_info *usbif;
>> +
>> +    usbif = container_of(xendev, struct usbback_info, xendev);
>> +    qemu_bh_schedule(usbif->bh);
>> +}
>> +
>> +struct XenDevOps xen_usb_ops = {
>> +    .size            = sizeof(struct usbback_info),
>> +    .flags           = DEVOPS_FLAG_NEED_GNTDEV,
>> +    .init            = usbback_init,
>> +    .alloc           = usbback_alloc,
>> +    .free            = usbback_free,
>> +    .backend_changed = usbback_backend_changed,
>> +    .initialise      = usbback_connect,
>> +    .disconnect      = usbback_disconnect,
>> +    .event           = usbback_event,
>> +};
>> diff --git a/hw/xenpv/xen_machine_pv.c b/hw/xenpv/xen_machine_pv.c
>> index 57bc071..fef1e7b 100644
>> --- a/hw/xenpv/xen_machine_pv.c
>> +++ b/hw/xenpv/xen_machine_pv.c
>> @@ -72,6 +72,9 @@ static void xen_init_pv(MachineState *machine)
>>      xen_be_register("vfb", &xen_framebuffer_ops);
>>      xen_be_register("qdisk", &xen_blkdev_ops);
>>      xen_be_register("qnic", &xen_netdev_ops);
>> +#ifdef CONFIG_USB_LIBUSB
>> +    xen_be_register("qusb", &xen_usb_ops);
>> +#endif
>>  
>>      /* configure framebuffer */
>>      if (xenfb_enabled) {
>> diff --git a/include/hw/xen/xen_backend.h b/include/hw/xen/xen_backend.h
>> index f194aae..3d44dec 100644
>> --- a/include/hw/xen/xen_backend.h
>> +++ b/include/hw/xen/xen_backend.h
>> @@ -4,6 +4,10 @@
>>  #include "hw/xen/xen_common.h"
>>  #include "sysemu/sysemu.h"
>>  #include "net/net.h"
>> +#ifdef CONFIG_USB_LIBUSB
>> +#include <libusb.h>
>> +#include "hw/usb.h"
>> +#endif
>>  
>>  /* ------------------------------------------------------------- */
>>  
>> @@ -55,9 +59,6 @@ struct XenDevice {
>>  
>>  /* ------------------------------------------------------------- */
>>  
>> -#define usbback_get_packets(p) 0
>> -#define usbback_set_iso_desc(p, xfer)
>> -
>>  /* variables */
>>  extern XenXC xen_xc;
>>  extern struct xs_handle *xenstore;
>> @@ -101,6 +102,12 @@ extern struct XenDevOps xen_kbdmouse_ops;     /* xen_framebuffer.c */
>>  extern struct XenDevOps xen_framebuffer_ops;  /* xen_framebuffer.c */
>>  extern struct XenDevOps xen_blkdev_ops;       /* xen_disk.c        */
>>  extern struct XenDevOps xen_netdev_ops;       /* xen_nic.c         */
>> +#ifdef CONFIG_USB_LIBUSB
>> +extern struct XenDevOps xen_usb_ops;          /* xen-usb.c         */
>> +
>> +int usbback_get_packets(USBPacket *p);
>> +void usbback_set_iso_desc(USBPacket *p, struct libusb_transfer *xfer);
>> +#endif
>>  
>>  void xen_init_display(int domid);
>>  
>> -- 
>> 2.1.4
>>
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> http://lists.xen.org/xen-devel
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>