Re: [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 
>> ---
>>  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 000..2570bd7
>> --- /dev/null
>> +++ b/hw/usb/xen-usb.c
>> @@ -0,0 +1,1120 @@
>> +/*
>> + *  xen paravirt usb device backend
>> + *
>> + *  (c) Juergen Gross 
>> + *
>> + *  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 .
>> + *
>> + *  Contributions after 2012-01-13 are licensed under the terms of the
>> + *  GNU GPL, version 2 or (at your option) any later version.
>> + */
>> +
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +
>> +#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 
>> +#include 
>> +
>> +#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_MAXPORTSUSBIF_PIPE_PORT_MASK
>> +#define USBBACK_DEVNAME_SIZE32
> 
> That does not seem to be used
>> +#define USB_DEV_ADDR_SIZE   128
>> +
>> +struct usbif_ctrlrequest {
>> +uint8_tbRequestType;
>> +uint8_tbRequest;
>> +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_tstatus;
>> +};
> 
> 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;
>> +USBPortport;
>> +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;
>> +USBPacketpacket;
>> +
>> +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

Re: [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 
> ---
>  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 000..2570bd7
> --- /dev/null
> +++ b/hw/usb/xen-usb.c
> @@ -0,0 +1,1120 @@
> +/*
> + *  xen paravirt usb device backend
> + *
> + *  (c) Juergen Gross 
> + *
> + *  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 .
> + *
> + *  Contributions after 2012-01-13 are licensed under the terms of the
> + *  GNU GPL, version 2 or (at your option) any later version.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#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 
> +#include 
> +
> +#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_MAXPORTSUSBIF_PIPE_PORT_MASK
> +#define USBBACK_DEVNAME_SIZE32

That does not seem to be used
> +#define USB_DEV_ADDR_SIZE   128
> +
> +struct usbif_ctrlrequest {
> +uint8_tbRequestType;
> +uint8_tbRequest;
> +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_tstatus;
> +};

Ditto?

> +
> +struct usbback_info;
> +struct usbback_req;
> +
> +struct usbback_stub {
> +USBDevice  *dev;
> +USBPortport;
> +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;
> +USBPacketpacket;
> +
> +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_r

Re: [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 
> > 
> > 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 000..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?

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

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


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 000..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


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

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

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 000..2570bd7
> --- /dev/null
> +++ b/hw/usb/xen-usb.c
> @@ -0,0 +1,1120 @@
> +/*
> + *  xen paravirt usb device backend
> + *
> + *  (c) Juergen Gross 
> + *
> + *  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 .
> + *
> + *  Contributions after 2012-01-13 are licensed under the terms of the
> + *  GNU GPL, version 2 or (at your option) any later version.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#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 
> +#include 
> +
> +#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_MAXPORTSUSBIF_PIPE_PORT_MASK
> +#define USBBACK_DEVNAME_SIZE32
> +#define USB_DEV_ADDR_SIZE   128
> +
> +struct usbif_ctrlrequest {
> +uint8_tbRequestType;
> +uint8_tbRequest;
> +uint16_t   wValue;
> +uint16_t   wIndex;
> +uint16_t   wLength;
> +};
> +
> +struct usbif_isoc_descriptor {
> +uint32_t   offset;
> +uint32_t   length;
> +uint32_t   actual_length;
> +int32_tstatus;
> +};
> +
> +struct usbback_info;
> +struct usbback_req;
> +
> +struct usbback_stub {
> +USBDevice  *dev;
> +USBPortport;
> +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;
> +USBPacketpacket;
> +
> +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