From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60184) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zr9OO-0007wp-Ra for qemu-devel@nongnu.org; Tue, 27 Oct 2015 14:54:48 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zr9OK-0000hb-Fy for qemu-devel@nongnu.org; Tue, 27 Oct 2015 14:54:44 -0400 Received: from aserp1040.oracle.com ([141.146.126.69]:42514) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zr9OK-0000hV-5F for qemu-devel@nongnu.org; Tue, 27 Oct 2015 14:54:40 -0400 Date: Tue, 27 Oct 2015 14:54:32 -0400 From: Konrad Rzeszutek Wilk Message-ID: <20151027185432.GB13211@l.oracle.com> References: <1441277113-30693-1-git-send-email-jgross@suse.com> <1441277113-30693-4-git-send-email-jgross@suse.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1441277113-30693-4-git-send-email-jgross@suse.com> Subject: Re: [Qemu-devel] [Xen-devel] [Patch V1 3/3] xen: add pvUSB backend List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Juergen Gross Cc: qemu-devel@nongnu.org, xen-devel@lists.xensource.com, kraxel@redhat.com, stefano.stabellini@eu.citrix.com 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 0000000..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_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 > +#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 From mboxrd@z Thu Jan 1 00:00:00 1970 From: Konrad Rzeszutek Wilk Subject: Re: [Xen-devel] [Patch V1 3/3] xen: add pvUSB backend Date: Tue, 27 Oct 2015 14:54:32 -0400 Message-ID: <20151027185432.GB13211@l.oracle.com> References: <1441277113-30693-1-git-send-email-jgross@suse.com> <1441277113-30693-4-git-send-email-jgross@suse.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: <1441277113-30693-4-git-send-email-jgross@suse.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+gceq-qemu-devel=gmane.org@nongnu.org Sender: qemu-devel-bounces+gceq-qemu-devel=gmane.org@nongnu.org To: Juergen Gross Cc: qemu-devel@nongnu.org, xen-devel@lists.xensource.com, kraxel@redhat.com, stefano.stabellini@eu.citrix.com List-Id: xen-devel@lists.xenproject.org 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 0000000..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_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 > +#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