Re: [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 
> ---
>  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(>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(>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  1/* 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,

Re: [Xen-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

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-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



___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-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


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-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



___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-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



___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-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 
---
 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(>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(>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  1/* 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