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

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

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@xxxxxxxx>
> ---
>  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@xxxxxxxx>
> + *
> + *  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@xxxxxxxxxxxxx
> http://lists.xen.org/xen-devel

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