* [PATCH v2 0/2] usb, xen: add pvUSB backend
@ 2016-03-10 15:19 Juergen Gross
2016-03-10 15:19 ` [PATCH v2 1/2] xen: introduce dummy system device Juergen Gross
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Juergen Gross @ 2016-03-10 15:19 UTC (permalink / raw)
To: qemu-devel, xen-devel
Cc: Juergen Gross, konrad.wilk, kraxel, stefano.stabellini
This series adds a Xen pvUSB backend driver to qemu. USB devices
connected to the host can be passed through to a Xen guest. The
devices are specified via Xenstore. Access to the devices is done
via host-libusb.c
I've tested the backend with various USB devices (memory sticks,
keyboard, ...).
Changes in V2:
- rebased to current qemu master
- removed support for isoc requests in this series, as the optimal way to
support those isn't clear yet. Will be added in a followup series.
- several changes as requested by Stefano Stabellini and Konrad Wilk
Changes since RFC:
- open device via qdev_device_add(), making patch 1 obsolete
- modify patch 2 to use isoc passthrough via libusb instead of one
job per frame
Juergen Gross (2):
xen: introduce dummy system device
xen: add pvUSB backend
hw/usb/Makefile.objs | 4 +
hw/usb/xen-usb.c | 1021 ++++++++++++++++++++++++++++++++++++++++++
hw/xenpv/xen_machine_pv.c | 43 ++
include/hw/xen/xen_backend.h | 4 +
include/hw/xen/xen_common.h | 2 +
5 files changed, 1074 insertions(+)
create mode 100644 hw/usb/xen-usb.c
--
2.6.2
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 1/2] xen: introduce dummy system device
2016-03-10 15:19 [PATCH v2 0/2] usb, xen: add pvUSB backend Juergen Gross
@ 2016-03-10 15:19 ` Juergen Gross
2016-05-03 15:11 ` [Qemu-devel] " Anthony PERARD
2016-03-10 15:19 ` [PATCH v2 2/2] xen: add pvUSB backend Juergen Gross
2016-03-18 12:52 ` [PATCH v2 0/2] usb, " Gerd Hoffmann
2 siblings, 1 reply; 11+ messages in thread
From: Juergen Gross @ 2016-03-10 15:19 UTC (permalink / raw)
To: qemu-devel, xen-devel; +Cc: Juergen Gross, kraxel, stefano.stabellini
Introduce a new dummy system device serving as parent for virtual
buses. This will enable new pv backends to introduce virtual buses
which are removable again opposed to system buses which are meant
to stay once added.
Signed-off-by: Juergen Gross <jgross@suse.com>
---
V2: NOT changed, even if requested by Stefano Stabellini: the xen dummy
system device is needed by virtfs for Xen PV(H) guests, too
Signed-off-by: Juergen Gross <jgross@suse.com>
---
hw/xenpv/xen_machine_pv.c | 40 ++++++++++++++++++++++++++++++++++++++++
include/hw/xen/xen_backend.h | 1 +
2 files changed, 41 insertions(+)
diff --git a/hw/xenpv/xen_machine_pv.c b/hw/xenpv/xen_machine_pv.c
index fc13535..48d5bc6 100644
--- a/hw/xenpv/xen_machine_pv.c
+++ b/hw/xenpv/xen_machine_pv.c
@@ -25,10 +25,15 @@
#include "qemu/osdep.h"
#include "hw/hw.h"
#include "hw/boards.h"
+#include "hw/sysbus.h"
#include "hw/xen/xen_backend.h"
#include "xen_domainbuild.h"
#include "sysemu/block-backend.h"
+#define TYPE_XENSYSDEV "xensysdev"
+
+DeviceState *xen_sysdev;
+
static void xen_init_pv(MachineState *machine)
{
DriveInfo *dinfo;
@@ -67,6 +72,9 @@ static void xen_init_pv(MachineState *machine)
break;
}
+ xen_sysdev = qdev_create(NULL, TYPE_XENSYSDEV);
+ qdev_init_nofail(xen_sysdev);
+
xen_be_register("console", &xen_console_ops);
xen_be_register("vkbd", &xen_kbdmouse_ops);
xen_be_register("vfb", &xen_framebuffer_ops);
@@ -101,6 +109,38 @@ static void xen_init_pv(MachineState *machine)
xen_init_display(xen_domid);
}
+static int xen_sysdev_init(SysBusDevice *dev)
+{
+ return 0;
+}
+
+static Property xen_sysdev_properties[] = {
+ {/* end of property list */},
+};
+
+static void xen_sysdev_class_init(ObjectClass *klass, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(klass);
+ SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
+
+ k->init = xen_sysdev_init;
+ dc->props = xen_sysdev_properties;
+}
+
+static const TypeInfo xensysdev_info = {
+ .name = TYPE_XENSYSDEV,
+ .parent = TYPE_SYS_BUS_DEVICE,
+ .instance_size = sizeof(SysBusDevice),
+ .class_init = xen_sysdev_class_init,
+};
+
+static void xenpv_register_types(void)
+{
+ type_register_static(&xensysdev_info);
+}
+
+type_init(xenpv_register_types);
+
static void xenpv_machine_init(MachineClass *mc)
{
mc->desc = "Xen Para-virtualized PC";
diff --git a/include/hw/xen/xen_backend.h b/include/hw/xen/xen_backend.h
index c839eeb..b4b4ff0 100644
--- a/include/hw/xen/xen_backend.h
+++ b/include/hw/xen/xen_backend.h
@@ -60,6 +60,7 @@ extern xc_interface *xen_xc;
extern xenforeignmemory_handle *xen_fmem;
extern struct xs_handle *xenstore;
extern const char *xen_protocol;
+extern DeviceState *xen_sysdev;
/* xenstore helper functions */
int xenstore_write_str(const char *base, const char *node, const char *val);
--
2.6.2
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v2 2/2] xen: add pvUSB backend
2016-03-10 15:19 [PATCH v2 0/2] usb, xen: add pvUSB backend Juergen Gross
2016-03-10 15:19 ` [PATCH v2 1/2] xen: introduce dummy system device Juergen Gross
@ 2016-03-10 15:19 ` Juergen Gross
2016-05-03 15:06 ` Anthony PERARD
2016-03-18 12:52 ` [PATCH v2 0/2] usb, " Gerd Hoffmann
2 siblings, 1 reply; 11+ messages in thread
From: Juergen Gross @ 2016-03-10 15:19 UTC (permalink / raw)
To: qemu-devel, xen-devel
Cc: Juergen Gross, konrad.wilk, kraxel, stefano.stabellini
Add a backend for para-virtualized USB devices for xen domains.
The backend is using host-libusb to forward USB requests from a
domain via libusb to the real device(s) passed through.
Signed-off-by: Juergen Gross <jgross@suse.com>
---
V2: use xen_be_printf() instead of fprintf() for diagnostic prints as
requested by Stefano Stabellini
lots of small changes requested by Konrad Wilk
removed isoc request support, will be in separate patch
Signed-off-by: Juergen Gross <jgross@suse.com>
---
hw/usb/Makefile.objs | 4 +
hw/usb/xen-usb.c | 1021 ++++++++++++++++++++++++++++++++++++++++++
hw/xenpv/xen_machine_pv.c | 3 +
include/hw/xen/xen_backend.h | 3 +
include/hw/xen/xen_common.h | 2 +
5 files changed, 1033 insertions(+)
create mode 100644 hw/usb/xen-usb.c
diff --git a/hw/usb/Makefile.objs b/hw/usb/Makefile.objs
index 2717027..98b5c9d 100644
--- a/hw/usb/Makefile.objs
+++ b/hw/usb/Makefile.objs
@@ -38,3 +38,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..b12077f
--- /dev/null
+++ b/hw/usb/xen-usb.c
@@ -0,0 +1,1021 @@
+/*
+ * xen paravirt usb device backend
+ *
+ * (c) Juergen Gross <jgross@suse.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; under version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, see <http://www.gnu.org/licenses/>.
+ *
+ * Contributions after 2012-01-13 are licensed under the terms of the
+ * GNU GPL, version 2 or (at your option) any later version.
+ */
+
+#include <libusb.h>
+#include <stdio.h>
+#include <sys/types.h>
+#include <sys/mman.h>
+#include <sys/time.h>
+
+#include "qemu/osdep.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(xendev, lvl, fmt, args...) \
+ { \
+ struct timeval tv; \
+ \
+ gettimeofday(&tv, NULL); \
+ xen_be_printf(xendev, lvl, "%8ld.%06ld xen-usb(%s):" fmt, \
+ tv.tv_sec, tv.tv_usec, __func__, ##args); \
+ }
+#define TR_BUS(xendev, fmt, args...) TR(xendev, 2, fmt, ##args)
+#define TR_REQ(xendev, fmt, args...) TR(xendev, 3, fmt, ##args)
+
+#define USBBACK_MAXPORTS USBIF_PIPE_PORT_MASK
+#define USB_DEV_ADDR_SIZE (USBIF_PIPE_DEV_MASK + 1)
+
+/* USB wire protocol: structure describing control request parameter. */
+struct usbif_ctrlrequest {
+ uint8_t bRequestType;
+ uint8_t bRequest;
+ uint16_t wValue;
+ uint16_t wIndex;
+ uint16_t wLength;
+};
+
+struct usbback_info;
+struct usbback_req;
+
+struct usbback_stub {
+ USBDevice *dev;
+ USBPort port;
+ unsigned int speed;
+ bool attached;
+ QTAILQ_HEAD(submit_q_head, usbback_req) submit_q;
+};
+
+struct usbback_req {
+ struct usbback_info *usbif;
+ struct usbback_stub *stub;
+ struct usbif_urb_request req;
+ USBPacket packet;
+
+ unsigned int nr_buffer_segs; /* # of transfer_buffer segments */
+ unsigned int nr_extra_segs; /* # of iso_frame_desc segments */
+
+ QTAILQ_ENTRY(usbback_req) q;
+
+ void *buffer;
+ void *isoc_buffer;
+ struct libusb_transfer *xfer;
+};
+
+struct usbback_info {
+ struct XenDevice xendev; /* must be first */
+ USBBus bus;
+ void *urb_sring;
+ void *conn_sring;
+ struct usbif_urb_back_ring urb_ring;
+ struct usbif_conn_back_ring conn_ring;
+ int num_ports;
+ int usb_ver;
+ bool ring_error;
+ QTAILQ_HEAD(req_free_q_head, usbback_req) req_free_q;
+ struct usbback_stub ports[USBBACK_MAXPORTS];
+ struct usbback_stub *addr_table[USB_DEV_ADDR_SIZE];
+ QEMUBH *bh;
+};
+
+static 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 = xengnttab_map_domain_grant_refs(xendev->gnttabdev,
+ usbback_req->nr_buffer_segs, xendev->dom, ref, prot);
+
+ if (!usbback_req->buffer) {
+ return -ENOMEM;
+ }
+
+ for (i = 0; i < usbback_req->nr_buffer_segs; i++) {
+ seg = usbback_req->req.seg + i;
+ addr = usbback_req->buffer + i * PAGE_SIZE + seg->offset;
+ qemu_iovec_add(&usbback_req->packet.iov, addr, seg->length);
+ }
+ }
+
+ if (!usbif_pipeisoc(usbback_req->req.pipe)) {
+ return 0;
+ }
+
+ if (!usbback_req->nr_extra_segs) {
+ xen_be_printf(xendev, 0, "iso request without descriptor segments\n");
+ return -EINVAL;
+ }
+
+ prot = PROT_READ | PROT_WRITE;
+ for (i = 0; i < usbback_req->nr_extra_segs; i++) {
+ ref[i] = usbback_req->req.seg[i + usbback_req->req.nr_buffer_segs].gref;
+ }
+ usbback_req->isoc_buffer = xengnttab_map_domain_grant_refs(
+ xendev->gnttabdev, usbback_req->nr_extra_segs, xendev->dom, ref, prot);
+
+ if (!usbback_req->isoc_buffer) {
+ return -ENOMEM;
+ }
+
+ return 0;
+}
+
+static int usbback_init_packet(struct usbback_req *usbback_req)
+{
+ struct XenDevice *xendev = &usbback_req->usbif->xendev;
+ USBPacket *packet = &usbback_req->packet;
+ USBDevice *dev = usbback_req->stub->dev;
+ USBEndpoint *ep;
+ unsigned int pid, ep_nr;
+ bool sok;
+ int ret = 0;
+
+ 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(xendev, "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);
+ ret = -EINVAL; /* isoc not implemented yet */
+ break;
+
+ case USBIF_PIPE_TYPE_INT:
+ TR_REQ(xendev, "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(xendev, "ctrl parameter: %lx, buflen: %x\n", packet->parameter,
+ usbback_req->req.buffer_length);
+ break;
+
+ case USBIF_PIPE_TYPE_BULK:
+ TR_REQ(xendev, "bulk transfer %s: buflen: %x\n",
+ (pid == USB_TOKEN_IN) ? "in" : "out",
+ usbback_req->req.buffer_length);
+ break;
+ default:
+ ret = -EINVAL;
+ break;
+ }
+
+ return ret;
+}
+
+static void usbback_do_response(struct usbback_req *usbback_req, int32_t status,
+ int32_t actual_length, int32_t error_count)
+{
+ struct usbback_info *usbif;
+ struct usbif_urb_response *res;
+ struct XenDevice *xendev;
+ unsigned int notify;
+
+ usbif = usbback_req->usbif;
+ xendev = &usbif->xendev;
+
+ TR_REQ(xendev, "id %d, status %d, length %d, errcnt %d\n",
+ usbback_req->req.id, status, actual_length, error_count);
+
+ if (usbback_req->packet.iov.iov) {
+ qemu_iovec_destroy(&usbback_req->packet.iov);
+ }
+
+ if (usbback_req->buffer) {
+ xengnttab_unmap(xendev->gnttabdev, usbback_req->buffer,
+ usbback_req->nr_buffer_segs);
+ usbback_req->buffer = NULL;
+ }
+
+ if (usbback_req->isoc_buffer) {
+ xengnttab_unmap(xendev->gnttabdev, usbback_req->isoc_buffer,
+ usbback_req->nr_extra_segs);
+ usbback_req->isoc_buffer = NULL;
+ }
+
+ 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;
+
+ 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);
+ usbback_do_response(usbback_req, status, packet->actual_length, 0);
+}
+
+static void usbback_set_address(struct usbback_info *usbif,
+ struct usbback_stub *stub, int cur_addr,
+ int new_addr)
+{
+ if (cur_addr) {
+ usbif->addr_table[cur_addr] = NULL;
+ }
+ if (new_addr) {
+ usbif->addr_table[new_addr] = stub;
+ }
+}
+
+static bool usbback_cancel_req(struct usbback_req *usbback_req)
+{
+ bool ret = false;
+
+ if (usb_packet_is_inflight(&usbback_req->packet)) {
+ usb_cancel_packet(&usbback_req->packet);
+ ret = true;
+ }
+ return ret;
+}
+
+static void usbback_process_unlink_req(struct usbback_req *usbback_req)
+{
+ struct usbback_info *usbif;
+ struct usbback_req *unlink_req;
+ unsigned int id, devnum;
+ int ret;
+
+ usbif = usbback_req->usbif;
+ ret = 0;
+ id = usbback_req->req.u.unlink.unlink_id;
+ TR_REQ(&usbif->xendev, "unlink id %d\n", id);
+ devnum = usbif_pipedevice(usbback_req->req.pipe);
+ if (unlikely(devnum == 0)) {
+ usbback_req->stub = usbif->ports +
+ usbif_pipeportnum(usbback_req->req.pipe);
+ if (unlikely(!usbback_req->stub)) {
+ ret = -ENODEV;
+ goto fail_response;
+ }
+ } else {
+ if (unlikely(!usbif->addr_table[devnum])) {
+ ret = -ENODEV;
+ goto fail_response;
+ }
+ usbback_req->stub = usbif->addr_table[devnum];
+ }
+
+ QTAILQ_FOREACH(unlink_req, &usbback_req->stub->submit_q, q) {
+ if (unlink_req->req.id == id) {
+ if (usbback_cancel_req(unlink_req)) {
+ usbback_do_response_ret(unlink_req, -EPROTO);
+ }
+ break;
+ }
+ }
+
+fail_response:
+ usbback_do_response_ret(usbback_req, ret);
+}
+
+/*
+ * Checks whether a request can be handled at once or should be forwarded
+ * to the usb framework.
+ * Return value is:
+ * 0 in case of usb framework is needed
+ * >0 in case of local handling (no error)
+ * <0 parameter error
+ * The request response has been queued already if return value not 0.
+ */
+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 normal transfer.
+ */
+ TR_REQ(&usbif->xendev, "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(&usbif->xendev, "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;
+
+ usbif = usbback_req->usbif;
+
+ TR_REQ(&usbif->xendev, "start req_id %d pipe %08x\n", usbback_req->req.id,
+ usbback_req->req.pipe);
+
+ /* unlink request */
+ if (unlikely(usbif_pipeunlink(usbback_req->req.pipe))) {
+ usbback_process_unlink_req(usbback_req);
+ return;
+ }
+
+ if (usbif_pipectrl(usbback_req->req.pipe)) {
+ if (usbback_check_and_submit(usbback_req)) {
+ return;
+ }
+ } else {
+ devnum = usbif_pipedevice(usbback_req->req.pipe);
+ usbback_req->stub = usbif->addr_table[devnum];
+
+ if (!usbback_req->stub || !usbback_req->stub->attached) {
+ ret = -ENODEV;
+ goto fail_response;
+ }
+ }
+
+ QTAILQ_INSERT_TAIL(&usbback_req->stub->submit_q, usbback_req, q);
+
+ usbback_req->nr_buffer_segs = usbback_req->req.nr_buffer_segs;
+ usbback_req->nr_extra_segs = usbif_pipeisoc(usbback_req->req.pipe) ?
+ usbback_req->req.u.isoc.nr_frame_desc_segs : 0;
+
+ ret = usbback_init_packet(usbback_req);
+ if (ret) {
+ xen_be_printf(&usbif->xendev, 0, "invalid request\n");
+ ret = -ESHUTDOWN;
+ goto fail_free_urb;
+ }
+
+ ret = usbback_gnttab_map(usbif, usbback_req);
+ if (ret) {
+ xen_be_printf(&usbif->xendev, 0, "invalid buffer, ret=%d\n", ret);
+ 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(&usbif->xendev, "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(&usbif->xendev, "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));
+ 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, &local_err));
+ if (!usbif->ports[port - 1].dev) {
+ goto err;
+ }
+ QDECREF(qdict);
+ snprintf(p->path, sizeof(p->path), "%d", port);
+ speed = usbif->ports[port - 1].dev->speed;
+ switch (speed) {
+ case USB_SPEED_LOW:
+ speed = USBIF_SPEED_LOW;
+ break;
+ case USB_SPEED_FULL:
+ speed = USBIF_SPEED_FULL;
+ break;
+ case USB_SPEED_HIGH:
+ speed = (usbif->usb_ver < USB_VER_USB20) ?
+ USBIF_SPEED_NONE : USBIF_SPEED_HIGH;
+ break;
+ default:
+ speed = USBIF_SPEED_NONE;
+ break;
+ }
+ if (speed == USBIF_SPEED_NONE) {
+ xen_be_printf(&usbif->xendev, 0, "device %s wrong speed\n", busid);
+ object_unparent(OBJECT(usbif->ports[port - 1].dev));
+ usbif->ports[port - 1].dev = NULL;
+ return;
+ }
+ usb_device_reset(usbif->ports[port - 1].dev);
+ usbif->ports[port - 1].speed = speed;
+ usbif->ports[port - 1].attached = true;
+ QTAILQ_INIT(&usbif->ports[port - 1].submit_q);
+ usbback_hotplug_notify(usbif, port);
+
+ TR_BUS(&usbif->xendev, "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(xendev, "start\n");
+
+ usbif = container_of(xendev, struct usbback_info, xendev);
+
+ xen_be_unbind_evtchn(xendev);
+
+ if (usbif->urb_sring) {
+ xengnttab_unmap(xendev->gnttabdev, usbif->urb_sring, 1);
+ usbif->urb_sring = NULL;
+ }
+ if (usbif->conn_sring) {
+ xengnttab_unmap(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(xendev, "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(xendev, "start\n");
+
+ usbif = container_of(xendev, struct usbback_info, xendev);
+
+ if (xenstore_read_fe_int(xendev, "urb-ring-ref", &urb_ring_ref)) {
+ xen_be_printf(xendev, 0, "error reading urb-ring-ref\n");
+ return -1;
+ }
+ if (xenstore_read_fe_int(xendev, "conn-ring-ref", &conn_ring_ref)) {
+ xen_be_printf(xendev, 0, "error reading conn-ring-ref\n");
+ return -1;
+ }
+ if (xenstore_read_fe_int(xendev, "event-channel", &xendev->remote_port)) {
+ xen_be_printf(xendev, 0, "error reading event-channel\n");
+ return -1;
+ }
+
+ usbif->urb_sring = xengnttab_map_grant_ref(xendev->gnttabdev, xendev->dom,
+ urb_ring_ref,
+ PROT_READ | PROT_WRITE);
+ usbif->conn_sring = xengnttab_map_grant_ref(xendev->gnttabdev, xendev->dom,
+ conn_ring_ref,
+ PROT_READ | PROT_WRITE);
+ if (!usbif->urb_sring || !usbif->conn_sring) {
+ xen_be_printf(xendev, 0, "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(xendev, "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(xendev, "start\n");
+
+ usbif = container_of(xendev, struct usbback_info, xendev);
+
+ if (xenstore_read_be_int(xendev, "num-ports", &usbif->num_ports) ||
+ usbif->num_ports < 1 || usbif->num_ports > USBBACK_MAXPORTS) {
+ xen_be_printf(xendev, 0, "num-ports not readable or out of bounds\n");
+ return -1;
+ }
+ if (xenstore_read_be_int(xendev, "usb-ver", &usbif->usb_ver) ||
+ (usbif->usb_ver != USB_VER_USB11 && usbif->usb_ver != USB_VER_USB20)) {
+ xen_be_printf(xendev, 0, "usb-ver not readable or out of bounds\n");
+ return -1;
+ }
+
+ usbback_backend_changed(xendev, "port");
+
+ TR_BUS(xendev, "finished\n");
+
+ return 0;
+}
+
+static void xen_bus_attach(USBPort *port)
+{
+ struct usbback_info *usbif;
+
+ usbif = port->opaque;
+ TR_BUS(&usbif->xendev, "\n");
+ usbif->ports[port->index].attached = true;
+ usbback_hotplug_notify(usbif, port->index + 1);
+}
+
+static void xen_bus_detach(USBPort *port)
+{
+ struct usbback_info *usbif;
+
+ usbif = port->opaque;
+ TR_BUS(&usbif->xendev, "\n");
+ usbif->ports[port->index].attached = false;
+ usbback_hotplug_notify(usbif, port->index + 1);
+}
+
+static void xen_bus_child_detach(USBPort *port, USBDevice *child)
+{
+ struct usbback_info *usbif;
+
+ usbif = port->opaque;
+ TR_BUS(&usbif->xendev, "\n");
+}
+
+static void xen_bus_complete(USBPort *port, USBPacket *packet)
+{
+ struct usbback_info *usbif;
+
+ usbif = port->opaque;
+ TR_REQ(&usbif->xendev, "\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: for each request and for the rings (request and connect). */
+ max_grants = USBIF_MAX_SEGMENTS_PER_REQUEST * USB_URB_RING_SIZE + 2;
+ if (xengnttab_set_max_grants(xendev->gnttabdev, max_grants) < 0) {
+ xen_be_printf(xendev, 0, "xengnttab_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(xendev, "start\n");
+
+ usbback_disconnect(xendev);
+ usbif = container_of(xendev, struct usbback_info, xendev);
+ for (i = 1; i <= usbif->num_ports; i++) {
+ usbback_portid_remove(usbif, i);
+ }
+
+ while (!QTAILQ_EMPTY(&usbif->req_free_q)) {
+ usbback_req = QTAILQ_FIRST(&usbif->req_free_q);
+ QTAILQ_REMOVE(&usbif->req_free_q, usbback_req, q);
+ g_free(usbback_req);
+ }
+
+ qemu_bh_delete(usbif->bh);
+
+ for (i = 0; i < USBBACK_MAXPORTS; i++) {
+ usb_unregister_port(&usbif->bus, &(usbif->ports[i].port));
+ }
+
+ usb_bus_release(&usbif->bus);
+
+ TR_BUS(xendev, "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 48d5bc6..f68cf48 100644
--- a/hw/xenpv/xen_machine_pv.c
+++ b/hw/xenpv/xen_machine_pv.c
@@ -80,6 +80,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 b4b4ff0..1c3b12f 100644
--- a/include/hw/xen/xen_backend.h
+++ b/include/hw/xen/xen_backend.h
@@ -99,6 +99,9 @@ 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 */
+#endif
void xen_init_display(int domid);
diff --git a/include/hw/xen/xen_common.h b/include/hw/xen/xen_common.h
index bd65e67..d010cee 100644
--- a/include/hw/xen/xen_common.h
+++ b/include/hw/xen/xen_common.h
@@ -49,6 +49,8 @@ typedef xc_gnttab xengnttab_handle;
#define xengnttab_unmap(h, a, n) xc_gnttab_munmap(h, a, n)
#define xengnttab_map_grant_refs(h, c, d, r, p) \
xc_gnttab_map_grant_refs(h, c, d, r, p)
+#define xengnttab_map_domain_grant_refs(h, c, d, r, p) \
+ xc_gnttab_map_domain_grant_refs(h, c, d, r, p)
#define xenforeignmemory_open(l, f) xen_xc
--
2.6.2
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2 0/2] usb, xen: add pvUSB backend
2016-03-10 15:19 [PATCH v2 0/2] usb, xen: add pvUSB backend Juergen Gross
2016-03-10 15:19 ` [PATCH v2 1/2] xen: introduce dummy system device Juergen Gross
2016-03-10 15:19 ` [PATCH v2 2/2] xen: add pvUSB backend Juergen Gross
@ 2016-03-18 12:52 ` Gerd Hoffmann
2016-03-18 14:47 ` Juergen Gross
2 siblings, 1 reply; 11+ messages in thread
From: Gerd Hoffmann @ 2016-03-18 12:52 UTC (permalink / raw)
To: Juergen Gross; +Cc: xen-devel, qemu-devel, konrad.wilk, stefano.stabellini
On Do, 2016-03-10 at 16:19 +0100, Juergen Gross wrote:
> This series adds a Xen pvUSB backend driver to qemu. USB devices
> connected to the host can be passed through to a Xen guest. The
> devices are specified via Xenstore. Access to the devices is done
> via host-libusb.c
> I've tested the backend with various USB devices (memory sticks,
> keyboard, ...).
Patches look sane to me.
Have you tested both virtual and physical devices? Given how it is
written devices such as the virtual usb tablet should work just fine
too.
I can take that through the usb queue, but I'd like to see someone from
xen have a look at this too. Reviews anyone?
thanks,
Gerd
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 0/2] usb, xen: add pvUSB backend
2016-03-18 12:52 ` [PATCH v2 0/2] usb, " Gerd Hoffmann
@ 2016-03-18 14:47 ` Juergen Gross
2016-03-29 4:55 ` [Xen-devel] " Juergen Gross
0 siblings, 1 reply; 11+ messages in thread
From: Juergen Gross @ 2016-03-18 14:47 UTC (permalink / raw)
To: Gerd Hoffmann; +Cc: xen-devel, qemu-devel, stefano.stabellini
On 18/03/16 13:52, Gerd Hoffmann wrote:
> On Do, 2016-03-10 at 16:19 +0100, Juergen Gross wrote:
>> This series adds a Xen pvUSB backend driver to qemu. USB devices
>> connected to the host can be passed through to a Xen guest. The
>> devices are specified via Xenstore. Access to the devices is done
>> via host-libusb.c
>
>> I've tested the backend with various USB devices (memory sticks,
>> keyboard, ...).
>
> Patches look sane to me.
>
> Have you tested both virtual and physical devices? Given how it is
> written devices such as the virtual usb tablet should work just fine
> too.
I tested with physical devices only.
TBH, I don't think a virtual device would work, given how the to be used
device is selected (driver="usb-host", hostbus, hostport).
> I can take that through the usb queue, but I'd like to see someone from
> xen have a look at this too. Reviews anyone?
Awesome, thanks for the thumbs up!
Juergen
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Xen-devel] [PATCH v2 0/2] usb, xen: add pvUSB backend
2016-03-18 14:47 ` Juergen Gross
@ 2016-03-29 4:55 ` Juergen Gross
0 siblings, 0 replies; 11+ messages in thread
From: Juergen Gross @ 2016-03-29 4:55 UTC (permalink / raw)
To: stefano.stabellini; +Cc: xen-devel, Gerd Hoffmann, qemu-devel
On 18/03/16 15:47, Juergen Gross wrote:
> On 18/03/16 13:52, Gerd Hoffmann wrote:
>> On Do, 2016-03-10 at 16:19 +0100, Juergen Gross wrote:
>>> This series adds a Xen pvUSB backend driver to qemu. USB devices
>>> connected to the host can be passed through to a Xen guest. The
>>> devices are specified via Xenstore. Access to the devices is done
>>> via host-libusb.c
>>
>>> I've tested the backend with various USB devices (memory sticks,
>>> keyboard, ...).
>>
>> Patches look sane to me.
>>
>> Have you tested both virtual and physical devices? Given how it is
>> written devices such as the virtual usb tablet should work just fine
>> too.
>
> I tested with physical devices only.
>
> TBH, I don't think a virtual device would work, given how the to be used
> device is selected (driver="usb-host", hostbus, hostport).
>
>> I can take that through the usb queue, but I'd like to see someone from
>> xen have a look at this too. Reviews anyone?
Stefano, could you please have a look?
Juergen
>
> Awesome, thanks for the thumbs up!
>
>
> Juergen
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/2] xen: add pvUSB backend
2016-03-10 15:19 ` [PATCH v2 2/2] xen: add pvUSB backend Juergen Gross
@ 2016-05-03 15:06 ` Anthony PERARD
2016-05-04 8:25 ` Juergen Gross
0 siblings, 1 reply; 11+ messages in thread
From: Anthony PERARD @ 2016-05-03 15:06 UTC (permalink / raw)
To: Juergen Gross
Cc: xen-devel, kraxel, qemu-devel, stefano.stabellini, konrad.wilk
On Thu, Mar 10, 2016 at 04:19:30PM +0100, Juergen Gross wrote:
> Add a backend for para-virtualized USB devices for xen domains.
>
> The backend is using host-libusb to forward USB requests from a
> domain via libusb to the real device(s) passed through.
>
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
[...]
> diff --git a/hw/usb/xen-usb.c b/hw/usb/xen-usb.c
> new file mode 100644
> index 0000000..b12077f
> --- /dev/null
> +++ b/hw/usb/xen-usb.c
> @@ -0,0 +1,1021 @@
> +/*
> + * xen paravirt usb device backend
> + *
> + * (c) Juergen Gross <jgross@suse.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; under version 2 of the License.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, see <http://www.gnu.org/licenses/>.
> + *
> + * Contributions after 2012-01-13 are licensed under the terms of the
> + * GNU GPL, version 2 or (at your option) any later version.
> + */
> +
> +#include <libusb.h>
> +#include <stdio.h>
> +#include <sys/types.h>
> +#include <sys/mman.h>
> +#include <sys/time.h>
> +
> +#include "qemu/osdep.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(xendev, lvl, fmt, args...) \
> + { \
> + struct timeval tv; \
> + \
> + gettimeofday(&tv, NULL); \
> + xen_be_printf(xendev, lvl, "%8ld.%06ld xen-usb(%s):" fmt, \
> + tv.tv_sec, tv.tv_usec, __func__, ##args); \
> + }
> +#define TR_BUS(xendev, fmt, args...) TR(xendev, 2, fmt, ##args)
> +#define TR_REQ(xendev, fmt, args...) TR(xendev, 3, fmt, ##args)
> +
> +#define USBBACK_MAXPORTS USBIF_PIPE_PORT_MASK
> +#define USB_DEV_ADDR_SIZE (USBIF_PIPE_DEV_MASK + 1)
> +
> +/* USB wire protocol: structure describing control request parameter. */
> +struct usbif_ctrlrequest {
> + uint8_t bRequestType;
> + uint8_t bRequest;
> + uint16_t wValue;
> + uint16_t wIndex;
> + uint16_t wLength;
> +};
> +
> +struct usbback_info;
> +struct usbback_req;
> +
> +struct usbback_stub {
> + USBDevice *dev;
> + USBPort port;
> + unsigned int speed;
> + bool attached;
> + QTAILQ_HEAD(submit_q_head, usbback_req) submit_q;
> +};
> +
> +struct usbback_req {
> + struct usbback_info *usbif;
> + struct usbback_stub *stub;
> + struct usbif_urb_request req;
> + USBPacket packet;
> +
> + unsigned int nr_buffer_segs; /* # of transfer_buffer segments */
> + unsigned int nr_extra_segs; /* # of iso_frame_desc segments */
> +
> + QTAILQ_ENTRY(usbback_req) q;
> +
> + void *buffer;
> + void *isoc_buffer;
> + struct libusb_transfer *xfer;
> +};
> +
> +struct usbback_info {
> + struct XenDevice xendev; /* must be first */
> + USBBus bus;
> + void *urb_sring;
> + void *conn_sring;
> + struct usbif_urb_back_ring urb_ring;
> + struct usbif_conn_back_ring conn_ring;
> + int num_ports;
> + int usb_ver;
> + bool ring_error;
> + QTAILQ_HEAD(req_free_q_head, usbback_req) req_free_q;
> + struct usbback_stub ports[USBBACK_MAXPORTS];
> + struct usbback_stub *addr_table[USB_DEV_ADDR_SIZE];
> + QEMUBH *bh;
> +};
> +
> +static 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));
You could use g_new0(struct usbback_req, 1) here.
> + } 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)
Looks like this function is the only one to have both usbif and usbback_req
as parameters.
> +{
> + 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 = xengnttab_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;
> + }
The code below would never be reached since isoc request are discared in
usbback_init_packet. Is this some left over from previous version?
> + if (!usbback_req->nr_extra_segs) {
> + xen_be_printf(xendev, 0, "iso request without descriptor segments\n");
> + return -EINVAL;
> + }
> +
> + prot = PROT_READ | PROT_WRITE;
> + for (i = 0; i < usbback_req->nr_extra_segs; i++) {
> + ref[i] = usbback_req->req.seg[i + usbback_req->req.nr_buffer_segs].gref;
> + }
> + usbback_req->isoc_buffer = xengnttab_map_domain_grant_refs(
> + xendev->gnttabdev, usbback_req->nr_extra_segs, xendev->dom, ref, prot);
> +
> + if (!usbback_req->isoc_buffer) {
> + return -ENOMEM;
> + }
> +
> + return 0;
> +}
> +
> +static int usbback_init_packet(struct usbback_req *usbback_req)
> +{
> + struct XenDevice *xendev = &usbback_req->usbif->xendev;
> + USBPacket *packet = &usbback_req->packet;
> + USBDevice *dev = usbback_req->stub->dev;
> + USBEndpoint *ep;
> + unsigned int pid, ep_nr;
> + bool sok;
> + int ret = 0;
> +
> + 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);
Why 1, where this came from?
> + if (usbif_pipetype(usbback_req->req.pipe) == USBIF_PIPE_TYPE_CTRL) {
usbif_pipectrl()?
> + 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(xendev, "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);
> + ret = -EINVAL; /* isoc not implemented yet */
> + break;
> +
> + case USBIF_PIPE_TYPE_INT:
> + TR_REQ(xendev, "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(xendev, "ctrl parameter: %lx, buflen: %x\n", packet->parameter,
> + usbback_req->req.buffer_length);
> + break;
> +
> + case USBIF_PIPE_TYPE_BULK:
> + TR_REQ(xendev, "bulk transfer %s: buflen: %x\n",
> + (pid == USB_TOKEN_IN) ? "in" : "out",
> + usbback_req->req.buffer_length);
> + break;
> + default:
> + ret = -EINVAL;
> + break;
> + }
> +
> + return ret;
> +}
> +
> +static void usbback_do_response(struct usbback_req *usbback_req, int32_t status,
> + int32_t actual_length, int32_t error_count)
> +{
> + struct usbback_info *usbif;
> + struct usbif_urb_response *res;
> + struct XenDevice *xendev;
> + unsigned int notify;
> +
> + usbif = usbback_req->usbif;
> + xendev = &usbif->xendev;
> +
> + TR_REQ(xendev, "id %d, status %d, length %d, errcnt %d\n",
> + usbback_req->req.id, status, actual_length, error_count);
> +
> + if (usbback_req->packet.iov.iov) {
> + qemu_iovec_destroy(&usbback_req->packet.iov);
> + }
> +
> + if (usbback_req->buffer) {
> + xengnttab_unmap(xendev->gnttabdev, usbback_req->buffer,
> + usbback_req->nr_buffer_segs);
> + usbback_req->buffer = NULL;
> + }
> +
> + if (usbback_req->isoc_buffer) {
> + xengnttab_unmap(xendev->gnttabdev, usbback_req->isoc_buffer,
> + usbback_req->nr_extra_segs);
> + usbback_req->isoc_buffer = NULL;
> + }
> +
> + res = RING_GET_RESPONSE(&usbif->urb_ring, usbif->urb_ring.rsp_prod_pvt);
What happen if the ring is full?
> + 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)
USBPacket->status is int, so I think the parameter status should be int as
well.
> +{
> + 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;
> + }
You could return from the switch instead of using a variable.
> +
> + return ret;
> +}
> +
> +static void usbback_packet_complete(USBPacket *packet)
> +{
> + struct usbback_req *usbback_req;
> + int32_t status;
> +
> + 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);
> + usbback_do_response(usbback_req, status, packet->actual_length, 0);
> +}
> +
> +static void usbback_set_address(struct usbback_info *usbif,
> + struct usbback_stub *stub, int cur_addr,
> + int new_addr)
unsigned int for both cur_addr and new_addr?
> +{
> + if (cur_addr) {
> + usbif->addr_table[cur_addr] = NULL;
> + }
> + if (new_addr) {
> + usbif->addr_table[new_addr] = stub;
> + }
> +}
> +
> +static bool usbback_cancel_req(struct usbback_req *usbback_req)
> +{
> + bool ret = false;
> +
> + if (usb_packet_is_inflight(&usbback_req->packet)) {
> + usb_cancel_packet(&usbback_req->packet);
> + ret = true;
> + }
> + return ret;
> +}
> +
> +static void usbback_process_unlink_req(struct usbback_req *usbback_req)
> +{
> + struct usbback_info *usbif;
> + struct usbback_req *unlink_req;
> + unsigned int id, devnum;
> + int ret;
> +
> + usbif = usbback_req->usbif;
> + ret = 0;
> + id = usbback_req->req.u.unlink.unlink_id;
> + TR_REQ(&usbif->xendev, "unlink id %d\n", id);
> + devnum = usbif_pipedevice(usbback_req->req.pipe);
> + if (unlikely(devnum == 0)) {
> + usbback_req->stub = usbif->ports +
> + usbif_pipeportnum(usbback_req->req.pipe);
> + if (unlikely(!usbback_req->stub)) {
> + ret = -ENODEV;
> + goto fail_response;
> + }
> + } else {
> + if (unlikely(!usbif->addr_table[devnum])) {
> + ret = -ENODEV;
> + goto fail_response;
> + }
> + usbback_req->stub = usbif->addr_table[devnum];
> + }
> +
> + QTAILQ_FOREACH(unlink_req, &usbback_req->stub->submit_q, q) {
> + if (unlink_req->req.id == id) {
> + if (usbback_cancel_req(unlink_req)) {
> + usbback_do_response_ret(unlink_req, -EPROTO);
> + }
> + break;
> + }
> + }
> +
> +fail_response:
> + usbback_do_response_ret(usbback_req, ret);
> +}
> +
> +/*
> + * Checks whether a request can be handled at once or should be forwarded
> + * to the usb framework.
> + * Return value is:
> + * 0 in case of usb framework is needed
> + * >0 in case of local handling (no error)
> + * <0 parameter error
The function only return 1 or 0. The comment those not match the function?
> + * The request response has been queued already if return value not 0.
> + */
> +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 normal transfer.
> + */
> + TR_REQ(&usbif->xendev, "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(&usbif->xendev, "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;
> +
> + usbif = usbback_req->usbif;
> +
> + TR_REQ(&usbif->xendev, "start req_id %d pipe %08x\n", usbback_req->req.id,
> + usbback_req->req.pipe);
> +
> + /* unlink request */
> + if (unlikely(usbif_pipeunlink(usbback_req->req.pipe))) {
> + usbback_process_unlink_req(usbback_req);
> + return;
> + }
> +
> + if (usbif_pipectrl(usbback_req->req.pipe)) {
> + if (usbback_check_and_submit(usbback_req)) {
> + return;
> + }
> + } else {
> + devnum = usbif_pipedevice(usbback_req->req.pipe);
> + usbback_req->stub = usbif->addr_table[devnum];
> +
> + if (!usbback_req->stub || !usbback_req->stub->attached) {
> + ret = -ENODEV;
> + goto fail_response;
> + }
> + }
> +
> + QTAILQ_INSERT_TAIL(&usbback_req->stub->submit_q, usbback_req, q);
> +
> + usbback_req->nr_buffer_segs = usbback_req->req.nr_buffer_segs;
> + usbback_req->nr_extra_segs = usbif_pipeisoc(usbback_req->req.pipe) ?
> + usbback_req->req.u.isoc.nr_frame_desc_segs : 0;
> +
> + ret = usbback_init_packet(usbback_req);
> + if (ret) {
> + xen_be_printf(&usbif->xendev, 0, "invalid request\n");
> + ret = -ESHUTDOWN;
> + goto fail_free_urb;
> + }
> +
> + ret = usbback_gnttab_map(usbif, usbback_req);
> + if (ret) {
> + xen_be_printf(&usbif->xendev, 0, "invalid buffer, ret=%d\n", ret);
> + 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;
Maybe use atomic_read() here to avoid req_prod been read more than once.
> + 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);
Should not we check if there is something to consume here?
> + id = req->id;
> + ring->req_cons++;
> + ring->sring->req_event = ring->req_cons + 1;
> +
> + res = RING_GET_RESPONSE(ring, ring->rsp_prod_pvt);
What if the ring is already full?
> + 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(&usbif->xendev, "hotplug port %d speed %d\n", port, res->speed);
> +}
> +
Thanks,
--
Anthony PERARD
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/2] xen: introduce dummy system device
2016-03-10 15:19 ` [PATCH v2 1/2] xen: introduce dummy system device Juergen Gross
@ 2016-05-03 15:11 ` Anthony PERARD
0 siblings, 0 replies; 11+ messages in thread
From: Anthony PERARD @ 2016-05-03 15:11 UTC (permalink / raw)
To: Juergen Gross; +Cc: xen-devel, kraxel, qemu-devel, stefano.stabellini
On Thu, Mar 10, 2016 at 04:19:29PM +0100, Juergen Gross wrote:
> Introduce a new dummy system device serving as parent for virtual
> buses. This will enable new pv backends to introduce virtual buses
> which are removable again opposed to system buses which are meant
> to stay once added.
>
> Signed-off-by: Juergen Gross <jgross@suse.com>
Looks good.
Acked-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
> V2: NOT changed, even if requested by Stefano Stabellini: the xen dummy
> system device is needed by virtfs for Xen PV(H) guests, too
>
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
> hw/xenpv/xen_machine_pv.c | 40 ++++++++++++++++++++++++++++++++++++++++
> include/hw/xen/xen_backend.h | 1 +
> 2 files changed, 41 insertions(+)
>
> diff --git a/hw/xenpv/xen_machine_pv.c b/hw/xenpv/xen_machine_pv.c
> index fc13535..48d5bc6 100644
> --- a/hw/xenpv/xen_machine_pv.c
> +++ b/hw/xenpv/xen_machine_pv.c
> @@ -25,10 +25,15 @@
> #include "qemu/osdep.h"
> #include "hw/hw.h"
> #include "hw/boards.h"
> +#include "hw/sysbus.h"
> #include "hw/xen/xen_backend.h"
> #include "xen_domainbuild.h"
> #include "sysemu/block-backend.h"
>
> +#define TYPE_XENSYSDEV "xensysdev"
> +
> +DeviceState *xen_sysdev;
> +
> static void xen_init_pv(MachineState *machine)
> {
> DriveInfo *dinfo;
> @@ -67,6 +72,9 @@ static void xen_init_pv(MachineState *machine)
> break;
> }
>
> + xen_sysdev = qdev_create(NULL, TYPE_XENSYSDEV);
> + qdev_init_nofail(xen_sysdev);
> +
> xen_be_register("console", &xen_console_ops);
> xen_be_register("vkbd", &xen_kbdmouse_ops);
> xen_be_register("vfb", &xen_framebuffer_ops);
> @@ -101,6 +109,38 @@ static void xen_init_pv(MachineState *machine)
> xen_init_display(xen_domid);
> }
>
> +static int xen_sysdev_init(SysBusDevice *dev)
> +{
> + return 0;
> +}
> +
> +static Property xen_sysdev_properties[] = {
> + {/* end of property list */},
> +};
> +
> +static void xen_sysdev_class_init(ObjectClass *klass, void *data)
> +{
> + DeviceClass *dc = DEVICE_CLASS(klass);
> + SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
> +
> + k->init = xen_sysdev_init;
> + dc->props = xen_sysdev_properties;
> +}
> +
> +static const TypeInfo xensysdev_info = {
> + .name = TYPE_XENSYSDEV,
> + .parent = TYPE_SYS_BUS_DEVICE,
> + .instance_size = sizeof(SysBusDevice),
> + .class_init = xen_sysdev_class_init,
> +};
> +
> +static void xenpv_register_types(void)
> +{
> + type_register_static(&xensysdev_info);
> +}
> +
> +type_init(xenpv_register_types);
> +
> static void xenpv_machine_init(MachineClass *mc)
> {
> mc->desc = "Xen Para-virtualized PC";
> diff --git a/include/hw/xen/xen_backend.h b/include/hw/xen/xen_backend.h
> index c839eeb..b4b4ff0 100644
> --- a/include/hw/xen/xen_backend.h
> +++ b/include/hw/xen/xen_backend.h
> @@ -60,6 +60,7 @@ extern xc_interface *xen_xc;
> extern xenforeignmemory_handle *xen_fmem;
> extern struct xs_handle *xenstore;
> extern const char *xen_protocol;
> +extern DeviceState *xen_sysdev;
>
> /* xenstore helper functions */
> int xenstore_write_str(const char *base, const char *node, const char *val);
> --
> 2.6.2
>
>
--
Anthony PERARD
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/2] xen: add pvUSB backend
2016-05-03 15:06 ` Anthony PERARD
@ 2016-05-04 8:25 ` Juergen Gross
2016-05-05 10:13 ` Anthony PERARD
0 siblings, 1 reply; 11+ messages in thread
From: Juergen Gross @ 2016-05-04 8:25 UTC (permalink / raw)
To: Anthony PERARD
Cc: xen-devel, kraxel, qemu-devel, stefano.stabellini, konrad.wilk
On 03/05/16 17:06, Anthony PERARD wrote:
> On Thu, Mar 10, 2016 at 04:19:30PM +0100, Juergen Gross wrote:
>> Add a backend for para-virtualized USB devices for xen domains.
>>
>> The backend is using host-libusb to forward USB requests from a
>> domain via libusb to the real device(s) passed through.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>
> [...]
>
>> diff --git a/hw/usb/xen-usb.c b/hw/usb/xen-usb.c
>> new file mode 100644
>> index 0000000..b12077f
>> --- /dev/null
>> +++ b/hw/usb/xen-usb.c
>> +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));
>
> You could use g_new0(struct usbback_req, 1) here.
Okay.
>> + } else {
>> + usbback_req = QTAILQ_FIRST(&usbif->req_free_q);
>> + QTAILQ_REMOVE(&usbif->req_free_q, usbback_req, q);
>> + }
>> + return usbback_req;
>> +}
>> +static int usbback_gnttab_map(struct usbback_info *usbif,
>> + struct usbback_req *usbback_req)
>
> Looks like this function is the only one to have both usbif and usbback_req
> as parameters.
Will change.
>> +{
>> + 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 = xengnttab_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;
>> + }
>
> The code below would never be reached since isoc request are discared in
> usbback_init_packet. Is this some left over from previous version?
Yes, but on purpose. This is infrastructure stuff which will be needed
regardless how isoc support will be added later. I'll add a comment.
>
>> + if (!usbback_req->nr_extra_segs) {
>> + xen_be_printf(xendev, 0, "iso request without descriptor segments\n");
>> + return -EINVAL;
>> + }
>> +
>> + prot = PROT_READ | PROT_WRITE;
>> + for (i = 0; i < usbback_req->nr_extra_segs; i++) {
>> + ref[i] = usbback_req->req.seg[i + usbback_req->req.nr_buffer_segs].gref;
>> + }
>> + usbback_req->isoc_buffer = xengnttab_map_domain_grant_refs(
>> + xendev->gnttabdev, usbback_req->nr_extra_segs, xendev->dom, ref, prot);
>> +
>> + if (!usbback_req->isoc_buffer) {
>> + return -ENOMEM;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int usbback_init_packet(struct usbback_req *usbback_req)
>> +{
>> + struct XenDevice *xendev = &usbback_req->usbif->xendev;
>> + USBPacket *packet = &usbback_req->packet;
>> + USBDevice *dev = usbback_req->stub->dev;
>> + USBEndpoint *ep;
>> + unsigned int pid, ep_nr;
>> + bool sok;
>> + int ret = 0;
>> +
>> + 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);
>
> Why 1, where this came from?
Well spotted!
I'll have to define this in the pvusb interface header. This is the
URB_SHORT_NOT_OK flag from the frontend.
>
>> + if (usbif_pipetype(usbback_req->req.pipe) == USBIF_PIPE_TYPE_CTRL) {
>
> usbif_pipectrl()?
Yepp.
>
>> + 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(xendev, "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);
>> + ret = -EINVAL; /* isoc not implemented yet */
>> + break;
>> +
>> + case USBIF_PIPE_TYPE_INT:
>> + TR_REQ(xendev, "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(xendev, "ctrl parameter: %lx, buflen: %x\n", packet->parameter,
>> + usbback_req->req.buffer_length);
>> + break;
>> +
>> + case USBIF_PIPE_TYPE_BULK:
>> + TR_REQ(xendev, "bulk transfer %s: buflen: %x\n",
>> + (pid == USB_TOKEN_IN) ? "in" : "out",
>> + usbback_req->req.buffer_length);
>> + break;
>> + default:
>> + ret = -EINVAL;
>> + break;
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +static void usbback_do_response(struct usbback_req *usbback_req, int32_t status,
>> + int32_t actual_length, int32_t error_count)
>> +{
>> + struct usbback_info *usbif;
>> + struct usbif_urb_response *res;
>> + struct XenDevice *xendev;
>> + unsigned int notify;
>> +
>> + usbif = usbback_req->usbif;
>> + xendev = &usbif->xendev;
>> +
>> + TR_REQ(xendev, "id %d, status %d, length %d, errcnt %d\n",
>> + usbback_req->req.id, status, actual_length, error_count);
>> +
>> + if (usbback_req->packet.iov.iov) {
>> + qemu_iovec_destroy(&usbback_req->packet.iov);
>> + }
>> +
>> + if (usbback_req->buffer) {
>> + xengnttab_unmap(xendev->gnttabdev, usbback_req->buffer,
>> + usbback_req->nr_buffer_segs);
>> + usbback_req->buffer = NULL;
>> + }
>> +
>> + if (usbback_req->isoc_buffer) {
>> + xengnttab_unmap(xendev->gnttabdev, usbback_req->isoc_buffer,
>> + usbback_req->nr_extra_segs);
>> + usbback_req->isoc_buffer = NULL;
>> + }
>> +
>> + res = RING_GET_RESPONSE(&usbif->urb_ring, usbif->urb_ring.rsp_prod_pvt);
>
> What happen if the ring is full?
Can't happen if frontend behaves correctly (and won't hurt backend
if frontend misbehaves): RING_FULL() is required to be used on
frontend side. This will ensure there are never more than RING_SIZE()
active requests, so there will always be a response slot available.
>> + 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)
>
> USBPacket->status is int, so I think the parameter status should be int as
> well.
Okay.
>> +{
>> + 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;
>> + }
>
> You could return from the switch instead of using a variable.
Okay.
>> +
>> + return ret;
>> +}
>> +
>> +static void usbback_packet_complete(USBPacket *packet)
>> +{
>> + struct usbback_req *usbback_req;
>> + int32_t status;
>> +
>> + 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);
>> + usbback_do_response(usbback_req, status, packet->actual_length, 0);
>> +}
>> +
>> +static void usbback_set_address(struct usbback_info *usbif,
>> + struct usbback_stub *stub, int cur_addr,
>> + int new_addr)
>
> unsigned int for both cur_addr and new_addr?
Yes.
>> +{
>> + if (cur_addr) {
>> + usbif->addr_table[cur_addr] = NULL;
>> + }
>> + if (new_addr) {
>> + usbif->addr_table[new_addr] = stub;
>> + }
>> +}
>> +/*
>> + * Checks whether a request can be handled at once or should be forwarded
>> + * to the usb framework.
>> + * Return value is:
>> + * 0 in case of usb framework is needed
>> + * >0 in case of local handling (no error)
>> + * <0 parameter error
>
> The function only return 1 or 0. The comment those not match the function?
Uuh, yes. This is a leftover from a previous version.
>> + * The request response has been queued already if return value not 0.
>> + */
>> +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 normal transfer.
>> + */
>> + TR_REQ(&usbif->xendev, "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(&usbif->xendev, "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_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;
>
> Maybe use atomic_read() here to avoid req_prod been read more than once.
Hmm. This isn't done in the other backends.
TBH: what would happen if req_prod would be read multiple times? In the
worst case we would see a new request from the guest which we would have
missed without the atomic_read().
>> + 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);
>
> Should not we check if there is something to consume here?
No. We just have to dequeue a request in order to stay in sync with the
number of enqueued responses (which are events).
What we should do, however, is to check for the ring full condition here
already, as we shouldn't start with the dequeueing in this situation.
This requires introducing a hotplug notify queue which is drained in
usbback_bh().
>> + id = req->id;
>> + ring->req_cons++;
>> + ring->sring->req_event = ring->req_cons + 1;
>> +
>> + res = RING_GET_RESPONSE(ring, ring->rsp_prod_pvt);
>
> What if the ring is already full?
See above.
>> + 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(&usbif->xendev, "hotplug port %d speed %d\n", port, res->speed);
>> +}
>> +
Thanks for the review,
Juergen
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/2] xen: add pvUSB backend
2016-05-04 8:25 ` Juergen Gross
@ 2016-05-05 10:13 ` Anthony PERARD
2016-05-06 4:57 ` Juergen Gross
0 siblings, 1 reply; 11+ messages in thread
From: Anthony PERARD @ 2016-05-05 10:13 UTC (permalink / raw)
To: Juergen Gross
Cc: xen-devel, kraxel, qemu-devel, stefano.stabellini, konrad.wilk
On Wed, May 04, 2016 at 10:25:03AM +0200, Juergen Gross wrote:
> On 03/05/16 17:06, Anthony PERARD wrote:
> > On Thu, Mar 10, 2016 at 04:19:30PM +0100, Juergen Gross wrote:
> >> +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;
> >
> > Maybe use atomic_read() here to avoid req_prod been read more than once.
>
> Hmm. This isn't done in the other backends.
>
> TBH: what would happen if req_prod would be read multiple times? In the
> worst case we would see a new request from the guest which we would have
> missed without the atomic_read().
If the guest is misbehaving, it maybe could provoke QEMU to handle more
request. I'm not sure.
For this use of atomic_read, I'm mostly refering to XSA-155[1] and a
conversation[2].
[1] http://xenbits.xen.org/xsa/advisory-155.html
[2] <570CFA45.7070504@citrix.com>
http://lists.xen.org/archives/html/xen-devel/2016-04/msg01696.html
> >> + xen_rmb(); /* Ensure we see queued requests up to 'rp'. */
> >> +
--
Anthony PERARD
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/2] xen: add pvUSB backend
2016-05-05 10:13 ` Anthony PERARD
@ 2016-05-06 4:57 ` Juergen Gross
0 siblings, 0 replies; 11+ messages in thread
From: Juergen Gross @ 2016-05-06 4:57 UTC (permalink / raw)
To: Anthony PERARD
Cc: xen-devel, kraxel, qemu-devel, stefano.stabellini, konrad.wilk
On 05/05/16 12:13, Anthony PERARD wrote:
> On Wed, May 04, 2016 at 10:25:03AM +0200, Juergen Gross wrote:
>> On 03/05/16 17:06, Anthony PERARD wrote:
>>> On Thu, Mar 10, 2016 at 04:19:30PM +0100, Juergen Gross wrote:
>>>> +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;
>>>
>>> Maybe use atomic_read() here to avoid req_prod been read more than once.
>>
>> Hmm. This isn't done in the other backends.
>>
>> TBH: what would happen if req_prod would be read multiple times? In the
>> worst case we would see a new request from the guest which we would have
>> missed without the atomic_read().
>
> If the guest is misbehaving, it maybe could provoke QEMU to handle more
> request. I'm not sure.
I don't think this would add any risk to dom0. A misbehaving guest
writing arbitrary values to ->req_prod could influence qemu activity
in just the same way regardless whether atomic_read() is used on qemu
side or not. The only difference would be that with atomic_read() the
additional qemu activity would be delayed until the next invocation
of the function.
> For this use of atomic_read, I'm mostly refering to XSA-155[1] and a
> conversation[2].
The main problem with XSA-155 was modification of the request's contents
by the guest after verification by the backend happened. This is not
related to reading the producer's ring index.
I should use RING_COPY_REQUEST(), however.
Juergen
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2016-05-06 4:57 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-03-10 15:19 [PATCH v2 0/2] usb, xen: add pvUSB backend Juergen Gross
2016-03-10 15:19 ` [PATCH v2 1/2] xen: introduce dummy system device Juergen Gross
2016-05-03 15:11 ` [Qemu-devel] " Anthony PERARD
2016-03-10 15:19 ` [PATCH v2 2/2] xen: add pvUSB backend Juergen Gross
2016-05-03 15:06 ` Anthony PERARD
2016-05-04 8:25 ` Juergen Gross
2016-05-05 10:13 ` Anthony PERARD
2016-05-06 4:57 ` Juergen Gross
2016-03-18 12:52 ` [PATCH v2 0/2] usb, " Gerd Hoffmann
2016-03-18 14:47 ` Juergen Gross
2016-03-29 4:55 ` [Xen-devel] " Juergen Gross
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).