* [PATCH V13 0/5] xen pvusb toolstack work
@ 2016-01-19 8:39 Chunyan Liu
2016-01-19 8:39 ` [PATCH V13 1/5] libxl: export some functions for pvusb use Chunyan Liu
` (4 more replies)
0 siblings, 5 replies; 42+ messages in thread
From: Chunyan Liu @ 2016-01-19 8:39 UTC (permalink / raw)
To: xen-devel
Cc: jgross, wei.liu2, ian.campbell, george.dunlap, Ian.Jackson,
Chunyan Liu, jfehlig
This patch series is to add pvusb toolstack work, supporting hot add|remove
USB device to|from guest and specify USB device in domain configuration file.
Changes to V12:
* update error handlings in libxl pvusb API (patch 3/5)
* adjust order of patch 4/5 and 5/5, and in 5/5, reuse documentation
generated in 4/5.
V12:
http://lists.xen.org/archives/html/xen-devel/2015-12/msg02697.html
V11:
http://lists.xen.org/archives/html/xen-devel/2015-12/msg01626.html
V10:
http://lists.xen.org/archives/html/xen-devel/2015-12/msg01172.html
V9:
http://lists.xen.org/archives/html/xen-devel/2015-11/msg02744.html
V8:
http://lists.xen.org/archives/html/xen-devel/2015-10/msg02178.html
V7:
http://lists.xen.org/archives/html/xen-devel/2015-09/msg03115.html
V6:
http://lists.xen.org/archives/html/xen-devel/2015-08/msg00750.html
V5:
http://lists.xen.org/archives/html/xen-devel/2015-06/msg04052.html
V4:
http://lists.xenproject.org/archives/html/xen-devel/2015-06/msg01327.html
Related Discussion Threads:
http://www.redhat.com/archives/libvir-list/2014-June/msg00038.html
http://lists.xen.org/archives/html/xen-devel/2014-06/msg00086.html
<<< pvusb work introduction >>>
1. Overview
There are two general methods for passing through individual host
devices to a guest. The first is via an emulated USB device
controller; the second is PVUSB.
Additionally, there are two ways to add USB devices to a guest: via
the config file at domain creation time, and via hot-plug while the VM
is running.
* Emulated USB
In emulated USB, the device model (qemu) presents an emulated USB
controller to the guest. The device model process then grabs control
of the device from domain 0 and and passes the USB commands between
the guest OS and the host USB device.
This method is only available to HVM domains, and is not available for
domains running with device model stubdomains.
* PVUSB
PVUSB uses a paravirtialized front-end/back-end interface, similar to
the traditional Xen PV network and disk protocols. In order to use
PVUSB, you need usbfront in your guest OS, and usbback in dom0 (or
your USB driver domain).
2. Specifying a host USB device
QEMU qmp commands allows USB devices to be specified either by their
bus address (in the form bus.device) or their device tag (in the form
vendorid:deviceid).
Each way of specifying has its advantages:
Specifying by device tag will always get the same device,
regardless of where the device ends up in the USB bus topology.
However, if there are two identical devices, it will not allow you to
specify which one.
Specifying by bus address will always allow you to choose a
specific device, even if you have duplicates. However, the bus address
may change depending on which port you plugged the device into, and
possibly also after a reboot.
To avoid duplication of vendorid:deviceid, we'll use bus address to
specify host USB device in xl toolstack.
You can use lsusb to list the USB devices on the system:
Bus 001 Device 003: ID 0424:2514 Standard Microsystems Corp. USB 2.0
Hub
Bus 003 Device 002: ID f617:0905
Bus 001 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub
Bus 001 Device 004: ID 0424:2640 Standard Microsystems Corp. USB 2.0
Hub
Bus 001 Device 005: ID 0424:4060 Standard Microsystems Corp. Ultra
Fast Media Reader
Bus 001 Device 006: ID 046d:c016 Logitech, Inc. Optical Wheel Mouse
To pass through the Logitec mouse, for instance, you could specify
1.6 (remove leading zeroes).
Note: USB hubs can not be assigned to guest.
3. PVUSB toolstack
* Specify USB device in xl config file
You can just specify usb devices, like:
usbdev=['1.6']
Then it will create a USB controller automatically and attach the USB
device to the first available USB controller:port.
or, you can explicitly specify usb controllers and usb devices, like:
usbctrl=['verison=1, ports=4', 'version=2, ports=8', ]
usbdev=['1.6, controller=0, port=1']
Then it will create two USB controllers as you specified.
And if controller and port are specified in usb config, then it will
attach the USB device to that controller:port. About the controller
and port value:
Each USB controller has a index (or called devid) based on 0. The 1st
controller has index 0, the 2nd controller has index 1, ...
Under controller, each port has a port number based on 1. In above
configuration, the 1st controller will have port 1,2,3,4.
* Hot-Plug USB device
To attach a USB device, you should first create a USB controller.
e.g.
xl usb-ctrl-attach domain [version=1|2] [ports=value]
By default, it will create a USB2.0 controller with 8 ports.
Then you could attach a USB device.
e.g.
xl usb-attach domain 1.6 [controller=index port=number]
By default, it will find the 1st available controller:port to attach
the USB device.
You could view USB device status of the domain by usb-list.
e.g.
xl usb-list domain
It will list USB controllers and USB devices under each controller.
You could detach a USB device with usb-detach command.
e.g.
xl usb-detach domain 1.6
You can also remove the whole USB controller by usb-ctrl-detach
command.
e.g.
xl usb-ctrl-detach domain 0
It will remove the USB controller with index 0 and all USB devices
under it.
4. PVUSB Libxl implementation
* usb-ctrl-attach
To create a usb controller, we need:
1) generate usb controler related information
2) write usb controller frontend/backend info to xenstore
PVUSB frontend and backend driver will probe xenstore paths and build
connection between frontend and backend.
* usb-ctrl-detach
To remove a usb controller, we need:
1) check if the usb controller exists or not
2) remove all usb devices under controller
3) remove usb controller info from xenstore
* usb-attach
To attach a usb device, we need:
1) check if the usb device type is assignable
2) check if the usb device is already assigned to a domain
3) add 'busid' of the usb device to xenstore contoller/port/.
PVUSB driver watches the xenstore changes and detects that,
and needs to use 'busid' to do following work.
4) unbind usb device from original driver and bind to usbback.
If usb device has many interfaces, then:
- unbind each interface from its original driver and bind to usbback.
- store the original driver to xenstore for later rebinding when
detaching the device.
* usb-detach
To detach a usb device, we need:
1) check if the usb device is assigned to the domain
2) remove the usb device from xenstore controller/port.
3) unbind usb device from usbback and rebind to its original driver.
If usb device has many interfaces, do it to each interface.
* usb-list
List all USB controllers and USB devices under each controller.
5. PVUSB xenstore information
PVUSB xenstore information includes three parts: frontend, backend
and /libxl part.
A USB controller is corresponding to a "vusb" device in xenstore.
Adding a USB controller will add a new "vusb" device, removing a
USB controller will delete the related "vusb" device.
Following is an example xenstore values of a USB controller.
Backend:
backend = ""
vusb = ""
1 = ""
0 = ""
frontend = "/local/domain/1/device/vusb/0"
frontend-id = "1"
online = "1"
state = "4"
type = "pv"
usb-ver = "1"
num-ports = "4"
port = ""
1 = ""
2 = ""
3 = ""
4 = ""
Frontend:
device = ""
vusb = ""
0 = ""
backend = "/local/domain/0/backend/vusb/1/0"
backend-id = "0"
state = "4"
urb-ring-ref = "348"
conn-ring-ref = "346"
event-channel = "20"
Adding a USB device won't create a new "vusb" device, but only write
the USB device busid to one port of USB controller.
For example, attaching a USB device (busid is 2-1.6) to above USB
controller port 1, it only need write 2-1.6 to port 1 of this USB
controller:
Backend:
backend = ""
vusb = ""
1 = ""
0 = ""
frontend = "/local/domain/1/device/vusb/0"
frontend-id = "1"
online = "1"
state = "4"
type = "pv"
usb-ver = "1"
num-ports = "4"
port = ""
1 = "2-1.6"
2 = ""
3 = ""
4 = ""
Frontend doesn't change.
Since assign a host USB device to guest, we'll unbind USB interfaces
from their original drivers and bind them to usbback. After detaching
this USB device from guest, one would hope the USB interfaces could
be rebind to their original drivers, so there should some place to
get the original driver info. To support that, when attaching a USB
device to guest, we'll save the original driver info in xenstore too,
the place is /libxl/usbback, for example:
libxl = ""
1 = ""
dm-version = "qemu_xen"
usbback = ""
3-11 = ""
3-11@1_0 = ""
driver_path = "/sys/bus/usb/drivers/btusb"
In this example, USB device (busid is 3-11, /sys/bus/usb/devices/3-11).
It has interface 3-11:1.0, whose original dirver is btusb.
Since xenstore doesn't allow ':' and '.' in a key, so we encode the
interface by changing ':' to '@' and changing '.' to '_'.
When detaching the USB device from guest, we can rebind 3-11:1.0 to
btusb driver.
*** BLURB HERE ***
Chunyan Liu (5):
libxl: export some functions for pvusb use
libxl_utils: add internal function to read sysfs file contents
libxl: add pvusb API
domcreate: support pvusb in configuration file
xl: add pvusb commands
docs/man/xl.cfg.pod.5 | 84 ++
docs/man/xl.pod.1 | 35 +
tools/libxl/Makefile | 2 +-
tools/libxl/libxl.c | 39 +-
tools/libxl/libxl.h | 77 ++
tools/libxl/libxl_create.c | 73 +-
tools/libxl/libxl_device.c | 17 +-
tools/libxl/libxl_internal.h | 37 +-
tools/libxl/libxl_osdeps.h | 13 +
tools/libxl/libxl_pvusb.c | 1565 ++++++++++++++++++++++++++++++++++
tools/libxl/libxl_types.idl | 46 +
tools/libxl/libxl_types_internal.idl | 1 +
tools/libxl/libxl_utils.c | 92 ++
tools/libxl/libxl_utils.h | 5 +
tools/libxl/xl.h | 5 +
tools/libxl/xl_cmdimpl.c | 297 ++++++-
tools/libxl/xl_cmdtable.c | 25 +
17 files changed, 2393 insertions(+), 20 deletions(-)
create mode 100644 tools/libxl/libxl_pvusb.c
--
2.1.4
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH V13 1/5] libxl: export some functions for pvusb use
2016-01-19 8:39 [PATCH V13 0/5] xen pvusb toolstack work Chunyan Liu
@ 2016-01-19 8:39 ` Chunyan Liu
2016-01-19 8:39 ` [PATCH V13 2/5] libxl_utils: add internal function to read sysfs file contents Chunyan Liu
` (3 subsequent siblings)
4 siblings, 0 replies; 42+ messages in thread
From: Chunyan Liu @ 2016-01-19 8:39 UTC (permalink / raw)
To: xen-devel
Cc: jgross, wei.liu2, ian.campbell, george.dunlap, Ian.Jackson,
Chunyan Liu, jfehlig, Simon Cao
Signed-off-by: Chunyan Liu <cyliu@suse.com>
Signed-off-by: Simon Cao <caobosimon@gmail.com>
Reviewed-by: Wei Liu <wei.liu2@citrix.com>
Acked-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
tools/libxl/libxl.c | 5 ++---
tools/libxl/libxl_internal.h | 3 +++
2 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 00d9ec4..43d5709 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -2036,7 +2036,7 @@ out:
}
/* common function to get next device id */
-static int libxl__device_nextid(libxl__gc *gc, uint32_t domid, char *device)
+int libxl__device_nextid(libxl__gc *gc, uint32_t domid, char *device)
{
char *dompath, **l;
unsigned int nb;
@@ -2055,8 +2055,7 @@ static int libxl__device_nextid(libxl__gc *gc, uint32_t domid, char *device)
return nextid;
}
-static int libxl__resolve_domid(libxl__gc *gc, const char *name,
- uint32_t *domid)
+int libxl__resolve_domid(libxl__gc *gc, const char *name, uint32_t *domid)
{
if (!name)
return 0;
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 4c01a82..9e94835 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1176,6 +1176,9 @@ _hidden int libxl__init_console_from_channel(libxl__gc *gc,
libxl__device_console *console,
int dev_num,
libxl_device_channel *channel);
+_hidden int libxl__device_nextid(libxl__gc *gc, uint32_t domid, char *device);
+_hidden int libxl__resolve_domid(libxl__gc *gc, const char *name,
+ uint32_t *domid);
/*
* For each aggregate type which can be used as an input we provide:
--
2.1.4
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH V13 2/5] libxl_utils: add internal function to read sysfs file contents
2016-01-19 8:39 [PATCH V13 0/5] xen pvusb toolstack work Chunyan Liu
2016-01-19 8:39 ` [PATCH V13 1/5] libxl: export some functions for pvusb use Chunyan Liu
@ 2016-01-19 8:39 ` Chunyan Liu
2016-01-19 8:39 ` [PATCH V13 3/5] libxl: add pvusb API Chunyan Liu
` (2 subsequent siblings)
4 siblings, 0 replies; 42+ messages in thread
From: Chunyan Liu @ 2016-01-19 8:39 UTC (permalink / raw)
To: xen-devel
Cc: jgross, wei.liu2, ian.campbell, george.dunlap, Ian.Jackson,
Chunyan Liu, jfehlig
Add a new function libxl_read_sysfs_file_contents to handle sysfs file
specially. It would be used in later pvusb work.
Signed-off-by: Chunyan Liu <cyliu@suse.com>
Acked-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
tools/libxl/libxl_internal.h | 4 +++
tools/libxl/libxl_utils.c | 74 ++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 78 insertions(+)
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 9e94835..d1eb18f 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -4039,6 +4039,10 @@ void libxl__bitmap_copy_best_effort(libxl__gc *gc, libxl_bitmap *dptr,
int libxl__count_physical_sockets(libxl__gc *gc, int *sockets);
+_hidden int libxl__read_sysfs_file_contents(libxl__gc *gc,
+ const char *filename,
+ void **data_r,
+ int *datalen_r);
#define LIBXL_QEMU_USER_PREFIX "xen-qemuuser"
#define LIBXL_QEMU_USER_BASE LIBXL_QEMU_USER_PREFIX"-domid"
diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c
index e42422a..e64f301 100644
--- a/tools/libxl/libxl_utils.c
+++ b/tools/libxl/libxl_utils.c
@@ -396,6 +396,80 @@ int libxl_read_file_contents(libxl_ctx *ctx, const char *filename,
return e;
}
+int libxl__read_sysfs_file_contents(libxl__gc *gc, const char *filename,
+ void **data_r, int *datalen_r)
+{
+ FILE *f = 0;
+ uint8_t *data = 0;
+ int datalen = 0;
+ int e;
+ struct stat stab;
+ ssize_t rs;
+
+ f = fopen(filename, "r");
+ if (!f) {
+ if (errno == ENOENT) return ENOENT;
+ LOGE(ERROR, "failed to open %s", filename);
+ goto xe;
+ }
+
+ if (fstat(fileno(f), &stab)) {
+ LOGE(ERROR, "failed to fstat %s", filename);
+ goto xe;
+ }
+
+ if (!S_ISREG(stab.st_mode)) {
+ LOGE(ERROR, "%s is not a plain file", filename);
+ errno = ENOTTY;
+ goto xe;
+ }
+
+ if (stab.st_size > INT_MAX) {
+ LOG(ERROR, "file %s is far too large", filename);
+ errno = EFBIG;
+ goto xe;
+ }
+
+ datalen = stab.st_size;
+
+ if (stab.st_size && data_r) {
+ data = libxl__malloc(gc, datalen);
+
+ /* For sysfs file, datalen is always PAGE_SIZE. 'read'
+ * will return the number of bytes of the actual content,
+ * rs <= datalen is expected.
+ */
+ rs = fread(data, 1, datalen, f);
+ if (rs < datalen) {
+ if (ferror(f)) {
+ LOGE(ERROR, "failed to read %s", filename);
+ goto xe;
+ }
+
+ datalen = rs;
+ data = libxl__realloc(gc, data, datalen);
+ }
+ }
+
+ if (fclose(f)) {
+ f = 0;
+ LOGE(ERROR, "failed to close %s", filename);
+ goto xe;
+ }
+
+ if (data_r) *data_r = data;
+ if (datalen_r) *datalen_r = datalen;
+
+ return 0;
+
+ xe:
+ e = errno;
+ assert(e != ENOENT);
+ if (f) fclose(f);
+ return e;
+}
+
+
#define READ_WRITE_EXACTLY(rw, zero_is_eof, constdata) \
\
int libxl_##rw##_exactly(libxl_ctx *ctx, int fd, \
--
2.1.4
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH V13 3/5] libxl: add pvusb API
2016-01-19 8:39 [PATCH V13 0/5] xen pvusb toolstack work Chunyan Liu
2016-01-19 8:39 ` [PATCH V13 1/5] libxl: export some functions for pvusb use Chunyan Liu
2016-01-19 8:39 ` [PATCH V13 2/5] libxl_utils: add internal function to read sysfs file contents Chunyan Liu
@ 2016-01-19 8:39 ` Chunyan Liu
2016-01-19 15:48 ` Ian Jackson
2016-01-26 16:12 ` Olaf Hering
2016-01-19 8:39 ` [PATCH V13 4/5] domcreate: support pvusb in configuration file Chunyan Liu
2016-01-19 8:39 ` [PATCH V13 5/5] xl: add pvusb commands Chunyan Liu
4 siblings, 2 replies; 42+ messages in thread
From: Chunyan Liu @ 2016-01-19 8:39 UTC (permalink / raw)
To: xen-devel
Cc: jgross, wei.liu2, ian.campbell, george.dunlap, Ian.Jackson,
Chunyan Liu, George Dunlap, jfehlig, Simon Cao
Add pvusb APIs, including:
- attach/detach (create/destroy) virtual usb controller.
- attach/detach usb device
- list usb controller and usb devices
- some other helper functions
Signed-off-by: Chunyan Liu <cyliu@suse.com>
Signed-off-by: Simon Cao <caobosimon@gmail.com>
Signed-off-by: George Dunlap <george.dunlap@citrix.com>
---
changes:
* address error handlings
tools/libxl/Makefile | 2 +-
tools/libxl/libxl.c | 34 +-
tools/libxl/libxl.h | 77 ++
tools/libxl/libxl_device.c | 13 +-
tools/libxl/libxl_internal.h | 22 +-
tools/libxl/libxl_osdeps.h | 13 +
tools/libxl/libxl_pvusb.c | 1567 ++++++++++++++++++++++++++++++++++
tools/libxl/libxl_types.idl | 46 +
tools/libxl/libxl_types_internal.idl | 1 +
tools/libxl/libxl_utils.c | 18 +
tools/libxl/libxl_utils.h | 5 +
11 files changed, 1785 insertions(+), 13 deletions(-)
create mode 100644 tools/libxl/libxl_pvusb.c
diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
index 2abae0c..e25ffa6 100644
--- a/tools/libxl/Makefile
+++ b/tools/libxl/Makefile
@@ -104,7 +104,7 @@ LIBXL_OBJS = flexarray.o libxl.o libxl_create.o libxl_dm.o libxl_pci.o \
libxl_stream_read.o libxl_stream_write.o \
libxl_save_callout.o _libxl_save_msgs_callout.o \
libxl_qmp.o libxl_event.o libxl_fork.o \
- libxl_dom_suspend.o $(LIBXL_OBJS-y)
+ libxl_dom_suspend.o libxl_pvusb.o $(LIBXL_OBJS-y)
LIBXL_OBJS += libxl_genid.o
LIBXL_OBJS += _libxl_types.o libxl_flask.o _libxl_types_internal.o
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 43d5709..920c135 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -3204,7 +3204,7 @@ void libxl__device_disk_local_initiate_detach(libxl__egc *egc,
aodev->dev = device;
aodev->callback = local_device_detach_cb;
aodev->force = 0;
- libxl__initiate_device_remove(egc, aodev);
+ libxl__initiate_device_generic_remove(egc, aodev);
return;
}
@@ -4172,8 +4172,10 @@ out:
* libxl_device_vkb_destroy
* libxl_device_vfb_remove
* libxl_device_vfb_destroy
+ * libxl_device_usbctrl_remove
+ * libxl_device_usbctrl_destroy
*/
-#define DEFINE_DEVICE_REMOVE(type, removedestroy, f) \
+#define DEFINE_DEVICE_REMOVE_EXT(type, remtype, removedestroy, f) \
int libxl_device_##type##_##removedestroy(libxl_ctx *ctx, \
uint32_t domid, libxl_device_##type *type, \
const libxl_asyncop_how *ao_how) \
@@ -4193,13 +4195,19 @@ out:
aodev->dev = device; \
aodev->callback = device_addrm_aocomplete; \
aodev->force = f; \
- libxl__initiate_device_remove(egc, aodev); \
+ libxl__initiate_device_##remtype##_remove(egc, aodev); \
\
out: \
- if (rc) return AO_CREATE_FAIL(rc); \
+ if (rc) return AO_CREATE_FAIL(rc); \
return AO_INPROGRESS; \
}
+#define DEFINE_DEVICE_REMOVE(type, removedestroy, f) \
+ DEFINE_DEVICE_REMOVE_EXT(type, generic, removedestroy, f)
+
+#define DEFINE_DEVICE_REMOVE_CUSTOM(type, removedestroy, f) \
+ DEFINE_DEVICE_REMOVE_EXT(type, type, removedestroy, f)
+
/* Define all remove/destroy functions and undef the macro */
/* disk */
@@ -4223,6 +4231,10 @@ DEFINE_DEVICE_REMOVE(vfb, destroy, 1)
DEFINE_DEVICE_REMOVE(vtpm, remove, 0)
DEFINE_DEVICE_REMOVE(vtpm, destroy, 1)
+/* usbctrl */
+DEFINE_DEVICE_REMOVE_CUSTOM(usbctrl, remove, 0)
+DEFINE_DEVICE_REMOVE_CUSTOM(usbctrl, destroy, 1)
+
/* channel/console hotunplug is not implemented. There are 2 possibilities:
* 1. add support for secondary consoles to xenconsoled
* 2. dynamically add/remove qemu chardevs via qmp messages. */
@@ -4236,6 +4248,8 @@ DEFINE_DEVICE_REMOVE(vtpm, destroy, 1)
* libxl_device_disk_add
* libxl_device_nic_add
* libxl_device_vtpm_add
+ * libxl_device_usbctrl_add
+ * libxl_device_usbdev_add
*/
#define DEFINE_DEVICE_ADD(type) \
@@ -4267,6 +4281,12 @@ DEFINE_DEVICE_ADD(nic)
/* vtpm */
DEFINE_DEVICE_ADD(vtpm)
+/* usbctrl */
+DEFINE_DEVICE_ADD(usbctrl)
+
+/* usb */
+DEFINE_DEVICE_ADD(usbdev)
+
#undef DEFINE_DEVICE_ADD
/******************************************************************************/
@@ -4432,7 +4452,7 @@ static int remove_device(libxl__egc *egc, libxl__ao *ao,
aodev->dev = dev;
aodev->action = LIBXL__DEVICE_ACTION_REMOVE;
aodev->callback = device_complete;
- libxl__initiate_device_remove(egc, aodev);
+ libxl__initiate_device_generic_remove(egc, aodev);
break;
case LIBXL__DEVICE_KIND_QDISK:
if (--dguest->num_qdisks == 0) {
@@ -6807,6 +6827,10 @@ int libxl_retrieve_domain_configuration(libxl_ctx *ctx, uint32_t domid,
MERGE(pci, pcidevs, COMPARE_PCI, {});
+ MERGE(usbctrl, usbctrls, COMPARE_USBCTRL, {});
+
+ MERGE(usbdev, usbdevs, COMPARE_USB, {});
+
/* Take care of removable device. We maintain invariant in the
* insert / remove operation so that:
* 1. if xenstore is "empty" while JSON is not, the result
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 0e347b9..c708cc8 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -123,6 +123,12 @@
#define LIBXL_HAVE_DOMAIN_NODEAFFINITY 1
/*
+ * LIBXL_HAVE_PVUSB indicates functions for plugging in USB devices
+ * through pvusb -- both hotplug and at domain creation time..
+ */
+#define LIBXL_HAVE_PVUSB 1
+
+/*
* LIBXL_HAVE_BUILDINFO_HVM_VENDOR_DEVICE indicates that the
* libxl_vendor_device field is present in the hvm sections of
* libxl_domain_build_info. This field tells libxl which
@@ -1503,6 +1509,77 @@ int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk,
const libxl_asyncop_how *ao_how)
LIBXL_EXTERNAL_CALLERS_ONLY;
+/*
+ * USB
+ *
+ * For each device removed or added, one of these protocols is available:
+ * - PV (i.e., PVUSB)
+ * - DEVICEMODEL (i.e, qemu)
+ *
+ * PV is available for either PV or HVM domains. DEVICEMODEL is only
+ * available for HVM domains. The caller can additionally specify
+ * "AUTO", in which case the library will try to determine the best
+ * protocol automatically.
+ *
+ * At the moment, the only protocol implemented is PV.
+ *
+ * One can add/remove USB controllers to/from guest, and attach/detach USB
+ * devices to/from USB controllers.
+ *
+ * To add USB controllers and USB devices, one can adding USB controllers
+ * first and then attaching USB devices to some USB controller, or adding
+ * USB devices to guest directly, it will automatically create a USB
+ * controller for USB devices to attach.
+ *
+ * To remove USB controllers or USB devices, one can remove USB devices
+ * under USB controller one by one and then remove USB controller, or
+ * remove USB controller directly, it will remove all USB devices under
+ * it automatically.
+ *
+ */
+/* USB Controllers*/
+int libxl_device_usbctrl_add(libxl_ctx *ctx, uint32_t domid,
+ libxl_device_usbctrl *usbctrl,
+ const libxl_asyncop_how *ao_how)
+ LIBXL_EXTERNAL_CALLERS_ONLY;
+
+int libxl_device_usbctrl_remove(libxl_ctx *ctx, uint32_t domid,
+ libxl_device_usbctrl *usbctrl,
+ const libxl_asyncop_how *ao_how)
+ LIBXL_EXTERNAL_CALLERS_ONLY;
+
+int libxl_device_usbctrl_destroy(libxl_ctx *ctx, uint32_t domid,
+ libxl_device_usbctrl *usbctrl,
+ const libxl_asyncop_how *ao_how)
+ LIBXL_EXTERNAL_CALLERS_ONLY;
+
+libxl_device_usbctrl *libxl_device_usbctrl_list(libxl_ctx *ctx,
+ uint32_t domid, int *num);
+
+void libxl_device_usbctrl_list_free(libxl_device_usbctrl *list, int nr);
+
+
+int libxl_device_usbctrl_getinfo(libxl_ctx *ctx, uint32_t domid,
+ libxl_device_usbctrl *usbctrl,
+ libxl_usbctrlinfo *usbctrlinfo);
+
+/* USB Devices */
+
+int libxl_device_usbdev_add(libxl_ctx *ctx, uint32_t domid,
+ libxl_device_usbdev *usbdev,
+ const libxl_asyncop_how *ao_how)
+ LIBXL_EXTERNAL_CALLERS_ONLY;
+
+int libxl_device_usbdev_remove(libxl_ctx *ctx, uint32_t domid,
+ libxl_device_usbdev *usbdev,
+ const libxl_asyncop_how *ao_how)
+ LIBXL_EXTERNAL_CALLERS_ONLY;
+
+libxl_device_usbdev *
+libxl_device_usbdev_list(libxl_ctx *ctx, uint32_t domid, int *num);
+
+void libxl_device_usbdev_list_free(libxl_device_usbdev *list, int nr);
+
/* Network Interfaces */
int libxl_device_nic_add(libxl_ctx *ctx, uint32_t domid, libxl_device_nic *nic,
const libxl_asyncop_how *ao_how)
diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index 8bb5e93..b7a6a13 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -676,7 +676,10 @@ void libxl__devices_destroy(libxl__egc *egc, libxl__devices_remove_state *drs)
aodev->action = LIBXL__DEVICE_ACTION_REMOVE;
aodev->dev = dev;
aodev->force = drs->force;
- libxl__initiate_device_remove(egc, aodev);
+ if (dev->backend_kind == LIBXL__DEVICE_KIND_VUSB)
+ libxl__initiate_device_usbctrl_remove(egc, aodev);
+ else
+ libxl__initiate_device_generic_remove(egc, aodev);
}
}
}
@@ -775,8 +778,8 @@ out:
return;
}
-void libxl__initiate_device_remove(libxl__egc *egc,
- libxl__ao_device *aodev)
+void libxl__initiate_device_generic_remove(libxl__egc *egc,
+ libxl__ao_device *aodev)
{
STATE_AO_GC(aodev->ao);
xs_transaction_t t = 0;
@@ -806,7 +809,7 @@ void libxl__initiate_device_remove(libxl__egc *egc,
(info.paused || info.dying || info.shutdown)) {
/*
* TODO: 4.2 Bodge due to QEMU, see comment on top of
- * libxl__initiate_device_remove in libxl_internal.h
+ * libxl__initiate_device_generic_remove in libxl_internal.h
*/
rc = libxl__ev_time_register_rel(ao, &aodev->timeout,
device_qemu_timeout,
@@ -942,7 +945,7 @@ static void device_backend_callback(libxl__egc *egc, libxl__ev_devstate *ds,
!aodev->force) {
LOG(DEBUG, "Timeout reached, initiating forced remove");
aodev->force = 1;
- libxl__initiate_device_remove(egc, aodev);
+ libxl__initiate_device_generic_remove(egc, aodev);
return;
}
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index d1eb18f..0ccad9a 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -2583,6 +2583,14 @@ _hidden void libxl__device_vtpm_add(libxl__egc *egc, uint32_t domid,
libxl_device_vtpm *vtpm,
libxl__ao_device *aodev);
+_hidden void libxl__device_usbctrl_add(libxl__egc *egc, uint32_t domid,
+ libxl_device_usbctrl *usbctrl,
+ libxl__ao_device *aodev);
+
+_hidden void libxl__device_usbdev_add(libxl__egc *egc, uint32_t domid,
+ libxl_device_usbdev *usbdev,
+ libxl__ao_device *aodev);
+
/* Internal function to connect a vkb device */
_hidden int libxl__device_vkb_add(libxl__gc *gc, uint32_t domid,
libxl_device_vkb *vkb);
@@ -2612,8 +2620,15 @@ _hidden void libxl__wait_device_connection(libxl__egc*,
*
* Once finished, aodev->callback will be executed.
*/
-_hidden void libxl__initiate_device_remove(libxl__egc *egc,
- libxl__ao_device *aodev);
+_hidden void libxl__initiate_device_generic_remove(libxl__egc *egc,
+ libxl__ao_device *aodev);
+
+_hidden int libxl__device_from_usbctrl(libxl__gc *gc, uint32_t domid,
+ libxl_device_usbctrl *usbctrl,
+ libxl__device *device);
+
+_hidden void libxl__initiate_device_usbctrl_remove(libxl__egc *egc,
+ libxl__ao_device *aodev);
/*
* libxl__get_hotplug_script_info returns the args and env that should
@@ -3975,6 +3990,9 @@ static inline void libxl__update_config_vtpm(libxl__gc *gc,
#define COMPARE_PCI(a, b) ((a)->func == (b)->func && \
(a)->bus == (b)->bus && \
(a)->dev == (b)->dev)
+#define COMPARE_USB(a, b) ((a)->ctrl == (b)->ctrl && \
+ (a)->port == (b)->port)
+#define COMPARE_USBCTRL(a, b) ((a)->devid == (b)->devid)
/* DEVICE_ADD
*
diff --git a/tools/libxl/libxl_osdeps.h b/tools/libxl/libxl_osdeps.h
index d9661c9..802c762 100644
--- a/tools/libxl/libxl_osdeps.h
+++ b/tools/libxl/libxl_osdeps.h
@@ -24,6 +24,8 @@
#define _GNU_SOURCE
#if defined(__NetBSD__)
+#define SYSFS_USB_DEV "/sys/bus/usb/devices"
+#define SYSFS_USBBACK_DRIVER "/kern/xen/usb"
#define SYSFS_PCI_DEV "/sys/bus/pci/devices"
#define SYSFS_PCIBACK_DRIVER "/kern/xen/pci"
#define NETBACK_NIC_NAME "xvif%ui%d"
@@ -31,6 +33,8 @@
#elif defined(__OpenBSD__)
#include <util.h>
#elif defined(__linux__)
+#define SYSFS_USB_DEV "/sys/bus/usb/devices"
+#define SYSFS_USBBACK_DRIVER "/sys/bus/usb/drivers/usbback"
#define SYSFS_PCI_DEV "/sys/bus/pci/devices"
#define SYSFS_PCIBACK_DRIVER "/sys/bus/pci/drivers/pciback"
#define NETBACK_NIC_NAME "vif%u.%d"
@@ -38,6 +42,8 @@
#elif defined(__sun__)
#include <stropts.h>
#elif defined(__FreeBSD__)
+#define SYSFS_USB_DEV "/dev/null"
+#define SYSFS_USBBACK_DRIVER "/dev/null"
#define SYSFS_PCI_DEV "/dev/null"
#define SYSFS_PCIBACK_DRIVER "/dev/null"
#define NETBACK_NIC_NAME "xnb%u.%d"
@@ -45,6 +51,13 @@
#include <sys/endian.h>
#endif
+#ifndef SYSFS_USBBACK_DRIVER
+#error define SYSFS_USBBACK_DRIVER for your platform
+#endif
+#ifndef SYSFS_USB_DEV
+#error define SYSFS_USB_DEV for your platform
+#endif
+
#ifndef SYSFS_PCIBACK_DRIVER
#error define SYSFS_PCIBACK_DRIVER for your platform
#endif
diff --git a/tools/libxl/libxl_pvusb.c b/tools/libxl/libxl_pvusb.c
new file mode 100644
index 0000000..1869611
--- /dev/null
+++ b/tools/libxl/libxl_pvusb.c
@@ -0,0 +1,1567 @@
+/*
+ * Copyright (C) 2015 SUSE LINUX Products GmbH, Nuernberg, Germany.
+ * Author Chunyan Liu <cyliu@suse.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU Lesser General Public License as published
+ * by the Free Software Foundation; version 2.1 only. with the special
+ * exception on linking described in file 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 Lesser General Public License for more details.
+ */
+
+#include "libxl_osdeps.h" /* must come before any other headers */
+
+#include "libxl_internal.h"
+#include <inttypes.h>
+
+#define USBBACK_INFO_PATH "/libxl/usbback"
+
+#define USBHUB_CLASS_CODE 9
+
+static int libxl__device_usbctrl_setdefault(libxl__gc *gc, uint32_t domid,
+ libxl_device_usbctrl *usbctrl)
+{
+ int rc;
+ libxl_domain_type domtype = libxl__domain_type(gc, domid);
+
+ if (!usbctrl->version)
+ usbctrl->version = 2;
+
+ if (!usbctrl->ports)
+ usbctrl->ports = 8;
+
+ if (usbctrl->type == LIBXL_USBCTRL_TYPE_AUTO) {
+ if (domtype == LIBXL_DOMAIN_TYPE_PV) {
+ usbctrl->type = LIBXL_USBCTRL_TYPE_PV;
+ } else if (domtype == LIBXL_DOMAIN_TYPE_HVM) {
+ /* FIXME: See if we can detect PV frontend */
+ usbctrl->type = LIBXL_USBCTRL_TYPE_DEVICEMODEL;
+ }
+ }
+
+ rc = libxl__resolve_domid(gc, usbctrl->backend_domname,
+ &usbctrl->backend_domid);
+ return rc;
+}
+
+int libxl__device_from_usbctrl(libxl__gc *gc, uint32_t domid,
+ libxl_device_usbctrl *usbctrl,
+ libxl__device *device)
+{
+ device->backend_devid = usbctrl->devid;
+ device->backend_domid = usbctrl->backend_domid;
+ device->backend_kind = LIBXL__DEVICE_KIND_VUSB;
+ device->devid = usbctrl->devid;
+ device->domid = domid;
+ device->kind = LIBXL__DEVICE_KIND_VUSB;
+
+ return 0;
+}
+
+/* Add usbctrl information to xenstore.
+ *
+ * Adding a usb controller will add a new 'vusb' device in xenstore, and
+ * add corresponding frontend, backend information to it. According to
+ * "update_json", decide wether to update json config file.
+ */
+static int libxl__device_usbctrl_add_xenstore(libxl__gc *gc, uint32_t domid,
+ libxl_device_usbctrl *usbctrl,
+ bool update_json)
+{
+ libxl__device *device;
+ flexarray_t *front;
+ flexarray_t *back;
+ xs_transaction_t t = XBT_NULL;
+ int i, rc;
+ libxl_domain_config d_config;
+ libxl_device_usbctrl usbctrl_saved;
+ libxl__domain_userdata_lock *lock = NULL;
+
+ libxl_domain_config_init(&d_config);
+ libxl_device_usbctrl_init(&usbctrl_saved);
+ libxl_device_usbctrl_copy(CTX, &usbctrl_saved, usbctrl);
+
+ GCNEW(device);
+ rc = libxl__device_from_usbctrl(gc, domid, usbctrl, device);
+ if (rc) goto out;
+
+ front = flexarray_make(gc, 4, 1);
+ back = flexarray_make(gc, 12, 1);
+
+ flexarray_append_pair(back, "frontend-id", GCSPRINTF("%d", domid));
+ flexarray_append_pair(back, "online", "1");
+ flexarray_append_pair(back, "state",
+ GCSPRINTF("%d", XenbusStateInitialising));
+ flexarray_append_pair(back, "type",
+ (char *)libxl_usbctrl_type_to_string(usbctrl->type));
+ flexarray_append_pair(back, "usb-ver", GCSPRINTF("%d", usbctrl->version));
+ flexarray_append_pair(back, "num-ports", GCSPRINTF("%d", usbctrl->ports));
+ flexarray_append_pair(back, "port", "");
+ for (i = 0; i < usbctrl->ports; i++)
+ flexarray_append_pair(back, GCSPRINTF("port/%d", i + 1), "");
+
+ flexarray_append_pair(front, "backend-id",
+ GCSPRINTF("%d", usbctrl->backend_domid));
+ flexarray_append_pair(front, "state",
+ GCSPRINTF("%d", XenbusStateInitialising));
+
+ if (update_json) {
+ lock = libxl__lock_domain_userdata(gc, domid);
+ if (!lock) {
+ rc = ERROR_LOCK_FAIL;
+ goto out;
+ }
+
+ rc = libxl__get_domain_configuration(gc, domid, &d_config);
+ if (rc) goto out;
+
+ DEVICE_ADD(usbctrl, usbctrls, domid, &usbctrl_saved,
+ COMPARE_USBCTRL, &d_config);
+ }
+
+ for (;;) {
+ rc = libxl__xs_transaction_start(gc, &t);
+ if (rc) goto out;
+
+ rc = libxl__device_exists(gc, t, device);
+ if (rc < 0) goto out;
+ if (rc == 1) {
+ /* already exists in xenstore */
+ LOG(ERROR, "device already exists in xenstore");
+ rc = ERROR_DEVICE_EXISTS;
+ goto out;
+ }
+
+ if (update_json) {
+ rc = libxl__set_domain_configuration(gc, domid, &d_config);
+ if (rc) goto out;
+ }
+
+ libxl__device_generic_add(gc, t, device,
+ libxl__xs_kvs_of_flexarray(gc, back, back->count),
+ libxl__xs_kvs_of_flexarray(gc, front, front->count),
+ NULL);
+
+ rc = libxl__xs_transaction_commit(gc, &t);
+ if (!rc) break;
+ if (rc < 0) goto out;
+ }
+
+out:
+ libxl__xs_transaction_abort(gc, &t);
+ if (lock) libxl__unlock_domain_userdata(lock);
+ libxl_device_usbctrl_dispose(&usbctrl_saved);
+ libxl_domain_config_dispose(&d_config);
+ return rc;
+}
+
+/* AO operation to add a usb controller.
+ *
+ * Generally, it does:
+ * 1) fill in necessary usb controler information with default value
+ * 2) write usb controller frontend/backend info to xenstore, update json
+ * config file if necessary.
+ * 3) wait for device connection. PVUSB frontend and backend driver will
+ * probe xenstore paths and build connection between frontend and backend.
+ *
+ * Before calling this function, aodev should be properly filled:
+ * aodev->ao, aodev->callback, aodev->update_json, ...
+ */
+void libxl__device_usbctrl_add(libxl__egc *egc, uint32_t domid,
+ libxl_device_usbctrl *usbctrl,
+ libxl__ao_device *aodev)
+{
+ STATE_AO_GC(aodev->ao);
+ libxl__device *device;
+ int rc;
+
+ rc = libxl__device_usbctrl_setdefault(gc, domid, usbctrl);
+ if (rc < 0) goto out;
+
+ if (usbctrl->devid == -1) {
+ usbctrl->devid = libxl__device_nextid(gc, domid, "vusb");
+ if (usbctrl->devid < 0) {
+ rc = ERROR_FAIL;
+ goto out;
+ }
+ }
+
+ if (usbctrl->type != LIBXL_USBCTRL_TYPE_PV) {
+ LOG(ERROR, "Unsupported USB controller type");
+ rc = ERROR_FAIL;
+ goto out;
+ }
+
+ rc = libxl__device_usbctrl_add_xenstore(gc, domid, usbctrl,
+ aodev->update_json);
+ if (rc) goto out;
+
+ GCNEW(device);
+ rc = libxl__device_from_usbctrl(gc, domid, usbctrl, device);
+ if (rc) goto out;
+
+ aodev->dev = device;
+ aodev->action = LIBXL__DEVICE_ACTION_ADD;
+ libxl__wait_device_connection(egc, aodev);
+ return;
+
+out:
+ aodev->rc = rc;
+ aodev->callback(egc, aodev);
+ return;
+}
+
+static int libxl__device_usbdev_list_for_usbctrl(libxl__gc *gc, uint32_t domid,
+ libxl_devid usbctrl,
+ libxl_device_usbdev **usbdevs,
+ int *num);
+
+static int libxl__device_usbdev_remove(libxl__gc *gc, uint32_t domid,
+ libxl_device_usbdev *usbdev);
+
+/* AO function to remove a usb controller.
+ *
+ * Generally, it does:
+ * 1) check if the usb controller exists or not
+ * 2) remove all usb devices under controller
+ * 3) remove usb controller information from xenstore
+ *
+ * Before calling this function, aodev should be properly filled:
+ * aodev->ao, aodev->dev, aodev->callback, ...
+ */
+void libxl__initiate_device_usbctrl_remove(libxl__egc *egc,
+ libxl__ao_device *aodev)
+{
+ STATE_AO_GC(aodev->ao);
+ libxl_device_usbdev *usbdevs = NULL;
+ int num_usbdev = 0;
+ int i, rc;
+ uint32_t domid = ao->domid;
+ int usbctrl_devid = aodev->dev->devid;
+ libxl_device_usbctrl usbctrl;
+ libxl_usbctrlinfo usbctrlinfo;
+
+ libxl_device_usbctrl_init(&usbctrl);
+ libxl_usbctrlinfo_init(&usbctrlinfo);
+ usbctrl.devid = usbctrl_devid;
+
+ rc = libxl_device_usbctrl_getinfo(CTX, domid, &usbctrl, &usbctrlinfo);
+ if (rc) goto out;
+
+ if (usbctrlinfo.type != LIBXL_USBCTRL_TYPE_PV) {
+ LOG(ERROR, "Unsupported USB controller type");
+ rc = ERROR_FAIL;
+ goto out;
+ }
+
+ /* Remove usb devices first */
+ rc = libxl__device_usbdev_list_for_usbctrl(gc, domid, usbctrl_devid,
+ &usbdevs, &num_usbdev);
+ if (rc) goto out;
+
+ for (i = 0; i < num_usbdev; i++) {
+ if (libxl__device_usbdev_remove(gc, domid, &usbdevs[i])) {
+ LOG(ERROR, "libxl__device_usbdev_remove failed: controller %d, "
+ "port %d", usbdevs[i].ctrl, usbdevs[i].port);
+ rc = ERROR_FAIL;
+ goto out;
+ }
+ }
+
+ libxl_device_usbctrl_dispose(&usbctrl);
+ libxl_usbctrlinfo_dispose(&usbctrlinfo);
+
+ /* Remove usbctrl */
+ libxl__initiate_device_generic_remove(egc, aodev);
+ return;
+
+out:
+ libxl_device_usbctrl_dispose(&usbctrl);
+ libxl_usbctrlinfo_dispose(&usbctrlinfo);
+ aodev->rc = rc;
+ aodev->callback(egc, aodev);
+ return;
+}
+
+static const char *vusb_be_from_xs_fe(libxl__gc *gc, const char *fe_path,
+ uint32_t tgt_domid)
+{
+ const char *be_path;
+ int r;
+ uint32_t be_domid, fe_domid;
+
+ r = libxl__xs_read_checked(gc, XBT_NULL, GCSPRINTF("%s/backend", fe_path),
+ &be_path);
+ if (r || !be_path) return NULL;
+
+ /* Check to see that it has the proper form, and that fe_domid ==
+ * target domid */
+ r = sscanf(be_path, "/local/domain/%d/backend/vusb/%d",
+ &be_domid, &fe_domid);
+
+ if (r != 2 || fe_domid != tgt_domid) {
+ LOG(ERROR, "Malformed backend, refusing to use");
+ return NULL;
+ }
+
+ return be_path;
+}
+
+libxl_device_usbctrl *
+libxl_device_usbctrl_list(libxl_ctx *ctx, uint32_t domid, int *num)
+{
+ GC_INIT(ctx);
+ libxl_device_usbctrl *usbctrls = NULL;
+ char *path = NULL;
+ char **entry = NULL;
+ unsigned int nentries = 0;
+
+ *num = 0;
+
+ path = GCSPRINTF("%s/device/vusb",
+ libxl__xs_get_dompath(gc, domid));
+ entry = libxl__xs_directory(gc, XBT_NULL, path, &nentries);
+
+ if (entry && nentries) {
+ usbctrls = libxl__zalloc(NOGC, sizeof(*usbctrls) * nentries);
+ libxl_device_usbctrl *usbctrl;
+ libxl_device_usbctrl *end = usbctrls + nentries;
+ for (usbctrl = usbctrls;
+ usbctrl < end;
+ usbctrl++, entry++, (*num)++) {
+ const char *tmp, *be_path, *fe_path;
+ int ret;
+
+ libxl_device_usbctrl_init(usbctrl);
+ usbctrl->devid = atoi(*entry);
+
+#define READ_SUBPATH(path, subpath) ({ \
+ ret = libxl__xs_read_checked(gc, XBT_NULL, \
+ GCSPRINTF("%s/" subpath, path), \
+ &tmp); \
+ if (ret) goto out; \
+ (char *)tmp; \
+ })
+
+#define READ_SUBPATH_INT(path, subpath) ({ \
+ ret = libxl__xs_read_checked(gc, XBT_NULL, \
+ GCSPRINTF("%s/" subpath, path), \
+ &tmp); \
+ if (ret) goto out; \
+ tmp ? atoi(tmp) : -1; \
+ })
+
+ fe_path = GCSPRINTF("%s/%s", path, *entry);
+ be_path = vusb_be_from_xs_fe(gc, fe_path, domid);
+ if (!be_path) goto out;
+ usbctrl->backend_domid = READ_SUBPATH_INT(fe_path, "backend-id");
+ usbctrl->version = READ_SUBPATH_INT(be_path, "usb-ver");
+ usbctrl->ports = READ_SUBPATH_INT(be_path, "num-ports");
+ libxl_usbctrl_type_from_string(READ_SUBPATH(be_path, "type"),
+ &usbctrl->type);
+
+#undef READ_SUBPATH
+#undef READ_SUBPATH_INT
+ }
+ }
+
+ GC_FREE;
+ return usbctrls;
+
+out:
+ LOG(ERROR, "Unable to list USB Controllers");
+ libxl_device_usbctrl_list_free(usbctrls, *num);
+ GC_FREE;
+ *num = 0;
+ return NULL;
+}
+
+int libxl_device_usbctrl_getinfo(libxl_ctx *ctx, uint32_t domid,
+ libxl_device_usbctrl *usbctrl,
+ libxl_usbctrlinfo *usbctrlinfo)
+{
+ GC_INIT(ctx);
+ const char *dompath, *fe_path, *be_path, *tmp;
+ int rc;
+
+ usbctrlinfo->devid = usbctrl->devid;
+
+#define READ_SUBPATH(path, subpath) ({ \
+ rc = libxl__xs_read_checked(gc, XBT_NULL, \
+ GCSPRINTF("%s/" subpath, path), \
+ &tmp); \
+ if (rc) goto out; \
+ (char *)tmp; \
+ })
+
+#define READ_SUBPATH_INT(path, subpath) ({ \
+ rc = libxl__xs_read_checked(gc, XBT_NULL, \
+ GCSPRINTF("%s/" subpath, path), \
+ &tmp); \
+ if (rc) goto out; \
+ tmp ? atoi(tmp) : -1; \
+ })
+
+ dompath = libxl__xs_get_dompath(gc, domid);
+ fe_path = GCSPRINTF("%s/device/vusb/%d", dompath, usbctrl->devid);
+ be_path = READ_SUBPATH(fe_path, "backend");
+ usbctrlinfo->backend = libxl__strdup(NOGC, be_path);
+ usbctrlinfo->backend_id = READ_SUBPATH_INT(fe_path, "backend-id");
+ usbctrlinfo->state = READ_SUBPATH_INT(fe_path, "state");
+ usbctrlinfo->evtch = READ_SUBPATH_INT(fe_path, "event-channel");
+ usbctrlinfo->ref_urb = READ_SUBPATH_INT(fe_path, "urb-ring-ref");
+ usbctrlinfo->ref_conn = READ_SUBPATH_INT(fe_path, "urb-ring-ref");
+ tmp = READ_SUBPATH(be_path, "frontend");
+ usbctrlinfo->frontend = libxl__strdup(NOGC, tmp);
+ usbctrlinfo->frontend_id = READ_SUBPATH_INT(be_path, "frontend-id");
+ usbctrlinfo->ports = READ_SUBPATH_INT(be_path, "num-ports");
+ usbctrlinfo->version = READ_SUBPATH_INT(be_path, "usb-ver");;
+ tmp = READ_SUBPATH(be_path, "type");
+ libxl_usbctrl_type_from_string(tmp, &usbctrlinfo->type);
+
+#undef READ_SUBPATH
+#undef READ_SUBPATH_INT
+
+ rc = 0;
+
+out:
+ GC_FREE;
+ return rc;
+}
+
+int libxl_devid_to_device_usbctrl(libxl_ctx *ctx,
+ uint32_t domid,
+ int devid,
+ libxl_device_usbctrl *usbctrl)
+{
+ libxl_device_usbctrl *usbctrls;
+ int nb = 0;
+ int i, rc;
+
+ usbctrls = libxl_device_usbctrl_list(ctx, domid, &nb);
+ if (!usbctrls) return ERROR_FAIL;
+
+ rc = ERROR_FAIL;
+ for (i = 0; i < nb; i++) {
+ if (devid == usbctrls[i].devid) {
+ libxl_device_usbctrl_copy(ctx, usbctrl, &usbctrls[i]);
+ rc = 0;
+ break;
+ }
+ }
+
+ libxl_device_usbctrl_list_free(usbctrls, nb);
+ return rc;
+}
+
+static void *zalloc_dirent(libxl__gc *gc, const char *dirpath)
+{
+ size_t need = offsetof(struct dirent, d_name) +
+ pathconf(dirpath, _PC_NAME_MAX) + 1;
+
+ return libxl__zalloc(gc, need);
+}
+
+static char *usbdev_busaddr_to_busid(libxl__gc *gc, int bus, int addr)
+{
+ DIR *dir;
+ char *busid = NULL;
+ struct dirent *de_buf;
+ struct dirent *de;
+
+ /* invalid hostbus or hostaddr */
+ if (bus < 1 || addr < 1)
+ return NULL;
+
+ dir = opendir(SYSFS_USB_DEV);
+ if (!dir) {
+ LOGE(ERROR, "opendir failed: '%s'", SYSFS_USB_DEV);
+ return NULL;
+ }
+
+ de_buf = zalloc_dirent(gc, SYSFS_USB_DEV);
+
+ for (;;) {
+ char *filename;
+ void *buf;
+ int busnum = -1;
+ int devnum = -1;
+
+ int r = readdir_r(dir, de_buf, &de);
+ if (r) {
+ LOGE(ERROR, "failed to readdir %s", SYSFS_USB_DEV);
+ break;
+ }
+ if (!de)
+ break;
+
+ if (!strcmp(de->d_name, ".") ||
+ !strcmp(de->d_name, ".."))
+ continue;
+
+ filename = GCSPRINTF(SYSFS_USB_DEV "/%s/devnum", de->d_name);
+ if (!libxl__read_sysfs_file_contents(gc, filename, &buf, NULL))
+ devnum = atoi(buf);
+
+ filename = GCSPRINTF(SYSFS_USB_DEV "/%s/busnum", de->d_name);
+ if (!libxl__read_sysfs_file_contents(gc, filename, &buf, NULL))
+ busnum = atoi(buf);
+
+ if (bus == busnum && addr == devnum) {
+ busid = libxl__strdup(gc, de->d_name);
+ break;
+ }
+ }
+
+ closedir(dir);
+ return busid;
+}
+
+static int usbdev_busaddr_from_busid(libxl__gc *gc, const char *busid,
+ uint8_t *bus, uint8_t *addr)
+{
+ char *filename;
+ void *buf;
+
+ filename = GCSPRINTF(SYSFS_USB_DEV "/%s/busnum", busid);
+ if (!libxl__read_sysfs_file_contents(gc, filename, &buf, NULL))
+ *bus = atoi(buf);
+ else
+ return ERROR_FAIL;
+
+ filename = GCSPRINTF(SYSFS_USB_DEV "/%s/devnum", busid);
+ if (!libxl__read_sysfs_file_contents(gc, filename, &buf, NULL))
+ *addr = atoi(buf);
+ else
+ return ERROR_FAIL;
+
+ return 0;
+}
+
+static int get_assigned_devices(libxl__gc *gc,
+ libxl_device_usbdev **list, int *num)
+{
+ char **domlist;
+ unsigned int ndom = 0;
+ int i, j, k;
+ int rc;
+
+ *list = NULL;
+ *num = 0;
+
+ domlist = libxl__xs_directory(gc, XBT_NULL, "/local/domain", &ndom);
+ for (i = 0; i < ndom; i++) {
+ char *path;
+ char **usbctrls;
+ unsigned int nc = 0;
+ uint32_t domid = atoi(domlist[i]);
+
+ path = GCSPRINTF("%s/device/vusb", libxl__xs_get_dompath(gc, domid));
+ usbctrls = libxl__xs_directory(gc, XBT_NULL, path, &nc);
+
+ for (j = 0; j < nc; j++) {
+ libxl_device_usbdev *tmp = NULL;
+ int nd = 0;
+
+ rc = libxl__device_usbdev_list_for_usbctrl(gc, domid,
+ atoi(usbctrls[j]),
+ &tmp, &nd);
+ if (rc) goto out;
+
+ if (!nd) continue;
+
+ GCREALLOC_ARRAY(*list, *num + nd);
+ for (k = 0; k < nd; k++) {
+ libxl_device_usbdev_copy(CTX, *list + *num, tmp + k);
+ (*num)++;
+ }
+ }
+ }
+
+ return 0;
+
+out:
+ LOG(ERROR, "fail to get assigned devices");
+ return rc;
+}
+
+static bool is_usbdev_in_array(libxl_device_usbdev *usbdevs, int num,
+ libxl_device_usbdev *usbdev)
+{
+ int i;
+
+ for (i = 0; i < num; i++) {
+ if (usbdevs[i].u.hostdev.hostbus == usbdev->u.hostdev.hostbus &&
+ usbdevs[i].u.hostdev.hostaddr == usbdev->u.hostdev.hostaddr)
+ return true;
+ }
+
+ return false;
+}
+
+/* check if USB device type is assignable */
+static bool is_usbdev_assignable(libxl__gc *gc, libxl_device_usbdev *usbdev)
+{
+ int classcode;
+ char *filename;
+ void *buf = NULL;
+ char *busid = NULL;
+
+ busid = usbdev_busaddr_to_busid(gc, usbdev->u.hostdev.hostbus,
+ usbdev->u.hostdev.hostaddr);
+ if (!busid) return false;
+
+ filename = GCSPRINTF(SYSFS_USB_DEV "/%s/bDeviceClass", busid);
+ if (libxl__read_sysfs_file_contents(gc, filename, &buf, NULL))
+ return false;
+
+ classcode = atoi(buf);
+ return classcode != USBHUB_CLASS_CODE;
+}
+
+/* get usb devices under certain usb controller */
+static int
+libxl__device_usbdev_list_for_usbctrl(libxl__gc *gc,
+ uint32_t domid,
+ libxl_devid usbctrl,
+ libxl_device_usbdev **usbdevs,
+ int *num)
+{
+ const char *fe_path, *be_path, *num_devs;
+ int n, i, rc;
+
+ *usbdevs = NULL;
+ *num = 0;
+
+ fe_path = GCSPRINTF("%s/device/vusb/%d",
+ libxl__xs_get_dompath(gc, domid), usbctrl);
+
+ be_path = vusb_be_from_xs_fe(gc, fe_path, domid);
+ if (!be_path) {
+ rc = ERROR_FAIL;
+ goto out;
+ }
+
+ rc = libxl__xs_read_checked(gc, XBT_NULL,
+ GCSPRINTF("%s/num-ports", be_path),
+ &num_devs);
+ if (rc) goto out;
+
+ n = num_devs ? atoi(num_devs) : 0;
+
+ for (i = 0; i < n; i++) {
+ const char *busid;
+ libxl_device_usbdev *usbdev;
+
+ rc = libxl__xs_read_checked(gc, XBT_NULL,
+ GCSPRINTF("%s/port/%d", be_path, i + 1),
+ &busid);
+ if (rc) goto out;
+
+ if (busid && strcmp(busid, "")) {
+ GCREALLOC_ARRAY(*usbdevs, *num + 1);
+ usbdev = *usbdevs + *num;
+ (*num)++;
+ libxl_device_usbdev_init(usbdev);
+ usbdev->ctrl = usbctrl;
+ usbdev->port = i + 1;
+ usbdev->type = LIBXL_USBDEV_TYPE_HOSTDEV;
+ rc = usbdev_busaddr_from_busid(gc, busid,
+ &usbdev->u.hostdev.hostbus,
+ &usbdev->u.hostdev.hostaddr);
+ if (rc) goto out;
+ }
+ }
+
+ rc = 0;
+
+out:
+ return rc;
+}
+
+/* get all usb devices of the domain */
+libxl_device_usbdev *
+libxl_device_usbdev_list(libxl_ctx *ctx, uint32_t domid, int *num)
+{
+ GC_INIT(ctx);
+ libxl_device_usbdev *usbdevs = NULL;
+ const char *path;
+ char **usbctrls;
+ unsigned int nc = 0;
+ int i, j;
+
+ *num = 0;
+
+ path = GCSPRINTF("%s/device/vusb",
+ libxl__xs_get_dompath(gc, domid));
+ usbctrls = libxl__xs_directory(gc, XBT_NULL, path, &nc);
+
+ for (i = 0; i < nc; i++) {
+ int r, nd = 0;
+ libxl_device_usbdev *tmp = NULL;
+
+ r = libxl__device_usbdev_list_for_usbctrl(gc, domid,
+ atoi(usbctrls[i]),
+ &tmp, &nd);
+ if (!r || !nd) continue;
+
+ usbdevs = libxl__realloc(NOGC, usbdevs,
+ sizeof(*usbdevs) * (*num + nd));
+ for (j = 0; j < nd; j++) {
+ libxl_device_usbdev_copy(ctx, usbdevs + *num, tmp + j);
+ (*num)++;
+ }
+ }
+
+ GC_FREE;
+ return usbdevs;
+}
+
+/* find first unused controller:port and give that to usb device */
+static int
+libxl__device_usbdev_set_default_usbctrl(libxl__gc *gc, uint32_t domid,
+ libxl_device_usbdev *usbdev)
+{
+ libxl_device_usbctrl *usbctrls = NULL;
+ int numctrl = 0;
+ int i, j, rc;
+
+ usbctrls = libxl_device_usbctrl_list(CTX, domid, &numctrl);
+ if (!numctrl || !usbctrls) {
+ rc = ERROR_FAIL;
+ goto out;
+ }
+
+ for (i = 0; i < numctrl; i++) {
+ for (j = 0; j < usbctrls[i].ports; j++) {
+ const char *path, *tmp;
+
+ path = GCSPRINTF("%s/backend/vusb/%d/%d/port/%d",
+ libxl__xs_get_dompath(gc, LIBXL_TOOLSTACK_DOMID),
+ domid, usbctrls[i].devid, j + 1);
+ rc = libxl__xs_read_checked(gc, XBT_NULL, path, &tmp);
+ if (rc) goto out;
+
+ if (tmp && !strcmp(tmp, "")) {
+ usbdev->ctrl = usbctrls[i].devid;
+ usbdev->port = j + 1;
+ rc = 0;
+ goto out;
+ }
+ }
+ }
+
+ /* no available controller:port */
+ rc = ERROR_FAIL;
+
+out:
+ libxl_device_usbctrl_list_free(usbctrls, numctrl);
+ return rc;
+}
+
+/* Fill in usb information with default value.
+ *
+ * Generally, it does:
+ * 1) if "controller" is not specified:
+ * - if "port" is not specified, try to find an available controller:port,
+ * if found, use that; otherwise, create a new controller, use this
+ * controller and its first port
+ * - if "port" is specified, report error.
+ * 2) if "controller" is specified, but port is not specified:
+ * try to find an available port under this controller, if found, use
+ * that, otherwise, report error.
+ * 3) if both "controller" and "port" are specified:
+ * check the controller:port is available, if not, report error.
+ */
+static int libxl__device_usbdev_setdefault(libxl__gc *gc,
+ uint32_t domid,
+ libxl_device_usbdev *usbdev,
+ bool update_json)
+{
+ int rc;
+
+ if (!usbdev->type)
+ usbdev->type = LIBXL_USBDEV_TYPE_HOSTDEV;
+
+ if (usbdev->ctrl == -1) {
+ if (usbdev->port) {
+ LOG(ERROR, "USB controller must be specified if you specify port");
+ return ERROR_INVAL;
+ }
+
+ rc = libxl__device_usbdev_set_default_usbctrl(gc, domid, usbdev);
+ /* If no existing controller to host this usb device, add a new one */
+ if (rc) {
+ libxl_device_usbctrl *usbctrl;
+
+ GCNEW(usbctrl);
+ libxl_device_usbctrl_init(usbctrl);
+ rc = libxl__device_usbctrl_setdefault(gc, domid, usbctrl);
+ if (rc < 0) goto out;
+
+ if (usbctrl->devid == -1) {
+ usbctrl->devid = libxl__device_nextid(gc, domid, "vusb");
+ if (usbctrl->devid < 0) {
+ rc = ERROR_FAIL;
+ goto out;
+ }
+ }
+
+ rc = libxl__device_usbctrl_add_xenstore(gc, domid, usbctrl,
+ update_json);
+ if (rc) goto out;
+
+ usbdev->ctrl = usbctrl->devid;
+ usbdev->port = 1;
+ }
+ } else {
+ /* A controller was specified; look it up */
+ const char *fe_path, *be_path, *tmp;
+
+ fe_path = GCSPRINTF("%s/device/vusb/%d",
+ libxl__xs_get_dompath(gc, domid),
+ usbdev->ctrl);
+
+ be_path = vusb_be_from_xs_fe(gc, fe_path, domid);
+ if (!be_path) {
+ rc = ERROR_FAIL;
+ goto out;
+ }
+
+ if (usbdev->port) {
+ /* A specific port was requested; see if it's available */
+ rc = libxl__xs_read_checked(gc, XBT_NULL,
+ GCSPRINTF("%s/port/%d",
+ be_path, usbdev->port),
+ &tmp);
+ if (rc) goto out;
+
+ if (tmp && strcmp(tmp, "")) {
+ LOG(ERROR, "The controller port isn't available");
+ rc = ERROR_FAIL;
+ goto out;
+ }
+ } else {
+ /* No port was requested. Choose free port. */
+ int i, ports;
+
+ rc = libxl__xs_read_checked(gc, XBT_NULL,
+ GCSPRINTF("%s/num-ports", be_path), &tmp);
+ if (rc) goto out;
+
+ ports = tmp ? atoi(tmp) : 0;
+
+ for (i = 0; i < ports; i++) {
+ rc = libxl__xs_read_checked(gc, XBT_NULL,
+ GCSPRINTF("%s/port/%d", be_path, i + 1),
+ &tmp);
+ if (rc) goto out;
+
+ if (tmp && !strcmp(tmp, "")) {
+ usbdev->port = i + 1;
+ break;
+ }
+ }
+
+ if (!usbdev->port) {
+ LOG(ERROR, "No available port under specified controller");
+ rc = ERROR_FAIL;
+ goto out;
+ }
+ }
+ }
+
+ rc = 0;
+
+out:
+ return rc;
+}
+
+/* Add usb information to xenstore
+ *
+ * Adding a usb device won't create new 'vusb' device, but only write
+ * the device busid to the controller:port in xenstore.
+ */
+static int libxl__device_usbdev_add_xenstore(libxl__gc *gc, uint32_t domid,
+ libxl_device_usbdev *usbdev,
+ bool update_json)
+{
+ char *be_path, *busid;
+ int rc;
+ xs_transaction_t t = XBT_NULL;
+ libxl_domain_config d_config;
+ libxl_device_usbdev usbdev_saved;
+ libxl__domain_userdata_lock *lock = NULL;
+
+ libxl_domain_config_init(&d_config);
+ libxl_device_usbdev_init(&usbdev_saved);
+ libxl_device_usbdev_copy(CTX, &usbdev_saved, usbdev);
+
+ busid = usbdev_busaddr_to_busid(gc, usbdev->u.hostdev.hostbus,
+ usbdev->u.hostdev.hostaddr);
+ if (!busid) {
+ LOG(DEBUG, "Fail to get busid of usb device");
+ goto out;
+ }
+
+ if (update_json) {
+ lock = libxl__lock_domain_userdata(gc, domid);
+ if (!lock) {
+ rc = ERROR_LOCK_FAIL;
+ goto out;
+ }
+
+ rc = libxl__get_domain_configuration(gc, domid, &d_config);
+ if (rc) goto out;
+
+ DEVICE_ADD(usbdev, usbdevs, domid, &usbdev_saved,
+ COMPARE_USB, &d_config);
+ }
+
+ for (;;) {
+ rc = libxl__xs_transaction_start(gc, &t);
+ if (rc) goto out;
+
+ if (update_json) {
+ rc = libxl__set_domain_configuration(gc, domid, &d_config);
+ if (rc) goto out;
+ }
+
+ be_path = GCSPRINTF("%s/backend/vusb/%d/%d/port/%d",
+ libxl__xs_get_dompath(gc, LIBXL_TOOLSTACK_DOMID),
+ domid, usbdev->ctrl, usbdev->port);
+
+ LOG(DEBUG, "Adding usb device %s to xenstore: controller %d, port %d",
+ busid, usbdev->ctrl, usbdev->port);
+
+ rc = libxl__xs_write_checked(gc, t, be_path, busid);
+ if (rc) goto out;
+
+ rc = libxl__xs_transaction_commit(gc, &t);
+ if (!rc) break;
+ if (rc < 0) goto out;
+ }
+
+ rc = 0;
+
+out:
+ if (lock) libxl__unlock_domain_userdata(lock);
+ libxl_device_usbdev_dispose(&usbdev_saved);
+ libxl_domain_config_dispose(&d_config);
+ return rc;
+}
+
+static int libxl__device_usbdev_remove_xenstore(libxl__gc *gc, uint32_t domid,
+ libxl_device_usbdev *usbdev)
+{
+ char *be_path;
+
+ be_path = GCSPRINTF("%s/backend/vusb/%d/%d/port/%d",
+ libxl__xs_get_dompath(gc, LIBXL_TOOLSTACK_DOMID),
+ domid, usbdev->ctrl, usbdev->port);
+
+ LOG(DEBUG, "Removing usb device from xenstore: controller %d, port %d",
+ usbdev->ctrl, usbdev->port);
+
+ return libxl__xs_write_checked(gc, XBT_NULL, be_path, "");
+}
+
+static char *usbdev_busid_from_ctrlport(libxl__gc *gc, uint32_t domid,
+ libxl_device_usbdev *usbdev)
+{
+ return libxl__xs_read(gc, XBT_NULL,
+ GCSPRINTF("%s/backend/vusb/%d/%d/port/%d",
+ libxl__xs_get_dompath(gc, LIBXL_TOOLSTACK_DOMID),
+ domid, usbdev->ctrl, usbdev->port));
+}
+
+/* get original driver path of usb interface, stored in @drvpath */
+static int usbintf_get_drvpath(libxl__gc *gc, const char *intf, char **drvpath)
+{
+ char *spath, *dp = NULL;
+ struct stat st;
+ int rc;
+
+ spath = GCSPRINTF(SYSFS_USB_DEV "/%s/driver", intf);
+
+ rc = lstat(spath, &st);
+ if (rc == 0) {
+ /* Find the canonical path to the driver. */
+ dp = libxl__zalloc(gc, PATH_MAX);
+ dp = realpath(spath, dp);
+ if (!dp) {
+ LOGE(ERROR, "get realpath failed: '%s'", spath);
+ return ERROR_FAIL;
+ }
+ } else if (errno != ENOENT) {
+ LOGE(ERROR, "lstat failed: '%s'", spath);
+ return ERROR_FAIL;
+ }
+
+ *drvpath = dp;
+
+ return 0;
+}
+
+static int sysfs_write_intf(libxl__gc *gc, const char *intf, const char *path)
+{
+ int fd = -1;
+ int r, rc;
+
+ fd = open(path, O_WRONLY);
+ if (fd < 0) {
+ LOGE(ERROR, "open file failed: '%s'", path);
+ return ERROR_FAIL;
+ }
+
+ r = write(fd, intf, strlen(intf));
+
+ if (r < 0) {
+ LOGE(ERROR, "write '%s' to '%s' failed", intf, path);
+ rc = ERROR_FAIL;
+ goto out;
+ }
+
+ if (r != strlen(intf)) {
+ LOG(ERROR, "write '%s' to '%s' failed: incorrect write count",
+ intf, path);
+ rc = ERROR_FAIL;
+ goto out;
+ }
+
+ rc = 0;
+
+out:
+ if (fd >= 0) close(fd);
+ return rc;
+}
+
+static int unbind_usbintf(libxl__gc *gc, const char *intf)
+{
+ char *path;
+
+ path = GCSPRINTF(SYSFS_USB_DEV "/%s/driver/unbind", intf);
+ return sysfs_write_intf(gc, intf, path);
+}
+
+static int bind_usbintf(libxl__gc *gc, const char *intf, const char *drvpath)
+{
+ char *path;
+ struct stat st;
+
+ path = GCSPRINTF("%s/%s", drvpath, intf);
+ /* if already bound, return */
+ if (!lstat(path, &st))
+ return 0;
+ else if (errno != ENOENT)
+ return ERROR_FAIL;
+
+ path = GCSPRINTF("%s/bind", drvpath);
+ return sysfs_write_intf(gc, intf, path);
+}
+
+/* Is usb interface bound to usbback? */
+static int usbintf_is_assigned(libxl__gc *gc, char *intf)
+{
+ char *spath;
+ int rc;
+ struct stat st;
+
+ spath = GCSPRINTF(SYSFS_USBBACK_DRIVER "/%s", intf);
+ rc = lstat(spath, &st);
+
+ if (rc == 0)
+ return 1;
+ if (rc < 0 && errno == ENOENT)
+ return 0;
+ LOGE(ERROR, "Accessing %s", spath);
+ return -1;
+}
+
+static int usbdev_get_all_interfaces(libxl__gc *gc, const char *busid,
+ char ***intfs, int *num)
+{
+ DIR *dir;
+ char *buf;
+ struct dirent *de_buf;
+ struct dirent *de;
+ int rc;
+
+ *intfs = NULL;
+ *num = 0;
+
+ buf = GCSPRINTF("%s:", busid);
+
+ dir = opendir(SYSFS_USB_DEV);
+ if (!dir) {
+ LOGE(ERROR, "opendir failed: '%s'", SYSFS_USB_DEV);
+ return ERROR_FAIL;
+ }
+
+ de_buf = zalloc_dirent(gc, SYSFS_USB_DEV);
+
+ for (;;) {
+ int r = readdir_r(dir, de_buf, &de);
+
+ if (r) {
+ LOGE(ERROR, "failed to readdir %s", SYSFS_USB_DEV);
+ rc = ERROR_FAIL;
+ goto out;
+ }
+ if (!de)
+ break;
+
+ if (!strcmp(de->d_name, ".") ||
+ !strcmp(de->d_name, ".."))
+ continue;
+
+ if (!strncmp(de->d_name, buf, strlen(buf))) {
+ GCREALLOC_ARRAY(*intfs, *num + 1);
+ (*intfs)[*num] = libxl__strdup(gc, de->d_name);
+ (*num)++;
+ }
+ }
+
+ rc = 0;
+
+out:
+ closedir(dir);
+ return rc;
+}
+
+/* Encode usb interface so that it could be written to xenstore as a key.
+ *
+ * Since xenstore key cannot include '.' or ':', we'll change '.' to '_',
+ * change ':' to '@'. For example, 3-1:2.1 will be encoded to 3-1@2_1.
+ * This will be used to save original driver of USB device to xenstore.
+ */
+static char *usb_interface_xenstore_encode(libxl__gc *gc, const char *busid)
+{
+ char *str = libxl__strdup(gc, busid);
+ int i, len = strlen(str);
+
+ for (i = 0; i < len; i++) {
+ if (str[i] == '.') str[i] = '_';
+ if (str[i] == ':') str[i] = '@';
+ }
+ return str;
+}
+
+/* Unbind USB device from "usbback" driver.
+ *
+ * If there are many interfaces under USB device, check each interface,
+ * unbind from "usbback" driver and rebind to its original driver.
+ */
+static int usbback_dev_unassign(libxl__gc *gc, const char *busid)
+{
+ char **intfs = NULL;
+ char *usbdev_encode = NULL;
+ char *path = NULL;
+ int i, num = 0;
+ int rc;
+
+ rc = usbdev_get_all_interfaces(gc, busid, &intfs, &num);
+ if (rc) goto out;
+
+ usbdev_encode = usb_interface_xenstore_encode(gc, busid);
+
+ for (i = 0; i < num; i++) {
+ char *intf = intfs[i];
+ char *usbintf_encode = NULL;
+ const char *drvpath;
+
+ /* check if the USB interface is already bound to "usbback" */
+ if (usbintf_is_assigned(gc, intf) > 0) {
+ /* unbind interface from usbback driver */
+ rc = unbind_usbintf(gc, intf);
+ if (rc) goto out;
+ }
+
+ /* try to rebind USB interface to its originial driver.
+ * If rebinding failed, export warning so that user can
+ * handle it later.
+ */
+ usbintf_encode = usb_interface_xenstore_encode(gc, intf);
+ path = GCSPRINTF(USBBACK_INFO_PATH "/%s/%s/driver_path",
+ usbdev_encode, usbintf_encode);
+ rc = libxl__xs_read_checked(gc, XBT_NULL, path, &drvpath);
+ if (rc) continue;
+
+ if (drvpath && bind_usbintf(gc, intf, drvpath))
+ LOGE(WARN, "Couldn't rebind %s to %s", intf, drvpath);
+ }
+
+ /* finally, remove xenstore driver path. */
+ path = GCSPRINTF(USBBACK_INFO_PATH "/%s", usbdev_encode);
+ rc = libxl__xs_rm_checked(gc, XBT_NULL, path);
+ if (rc)
+ LOG(WARN, "Failed to remove driver path");
+
+ /* Till here, USB device has been unbound from USBBACK and
+ * removed from xenstore, usb list couldn't show it anymore,
+ * so no matter removimg driver path successfully or not,
+ * we will report operation success.
+ */
+
+ rc = 0;
+
+out:
+ return rc;
+}
+
+/* Bind USB device to "usbback" driver.
+ *
+ * If there are many interfaces under USB device, check each interface,
+ * unbind from original driver and bind to "usbback" driver.
+ */
+static int usbback_dev_assign(libxl__gc *gc, const char *busid)
+{
+ char **intfs = NULL;
+ int num = 0, i;
+ int rc;
+ char *usbdev_encode = NULL;
+
+ rc = usbdev_get_all_interfaces(gc, busid, &intfs, &num);
+ if (rc) return rc;
+
+ usbdev_encode = usb_interface_xenstore_encode(gc, busid);
+
+ for (i = 0; i < num; i++) {
+ char *intf = intfs[i];
+ char *drvpath = NULL;
+
+ /* already assigned to usbback */
+ if (usbintf_is_assigned(gc, intf) > 0)
+ continue;
+
+ rc = usbintf_get_drvpath(gc, intf, &drvpath);
+ if (rc) goto out;
+
+ if (drvpath) {
+ /* write driver path to xenstore for later rebinding */
+ char *usbintf_encode = NULL;
+ char *path;
+
+ usbintf_encode = usb_interface_xenstore_encode(gc, intf);
+ path = GCSPRINTF(USBBACK_INFO_PATH "/%s/%s/driver_path",
+ usbdev_encode, usbintf_encode);
+ rc = libxl__xs_write_checked(gc, XBT_NULL, path, drvpath);
+ if (rc) goto out;
+
+ /* unbind interface from original driver */
+ rc = unbind_usbintf(gc, intf);
+ if (rc) goto out;
+ }
+
+ /* bind interface to usbback */
+ rc = bind_usbintf(gc, intf, SYSFS_USBBACK_DRIVER);
+ if (rc) {
+ LOG(ERROR, "Couldn't bind %s to %s", intf, SYSFS_USBBACK_DRIVER);
+ goto out;
+ }
+ }
+
+ return 0;
+
+out:
+ /* some interfaces might be bound to usbback, unbind it and
+ * rebind it to its original driver
+ */
+ usbback_dev_unassign(gc, busid);
+ return rc;
+}
+
+static int do_usbdev_add(libxl__gc *gc, uint32_t domid,
+ libxl_device_usbdev *usbdev,
+ bool update_json)
+{
+ int rc;
+ char *busid;
+ libxl_device_usbctrl usbctrl;
+ libxl_usbctrlinfo usbctrlinfo;
+
+ libxl_device_usbctrl_init(&usbctrl);
+ libxl_usbctrlinfo_init(&usbctrlinfo);
+ usbctrl.devid = usbdev->ctrl;
+
+ rc = libxl_device_usbctrl_getinfo(CTX, domid, &usbctrl, &usbctrlinfo);
+ if (rc) goto out;
+
+ switch (usbctrlinfo.type) {
+ case LIBXL_USBCTRL_TYPE_PV:
+ busid = usbdev_busaddr_to_busid(gc, usbdev->u.hostdev.hostbus,
+ usbdev->u.hostdev.hostaddr);
+ if (!busid) {
+ rc = ERROR_FAIL;
+ goto out;
+ }
+
+ rc = libxl__device_usbdev_add_xenstore(gc, domid, usbdev, update_json);
+ if (rc) goto out;
+
+ rc = usbback_dev_assign(gc, busid);
+ if (rc) {
+ libxl__device_usbdev_remove_xenstore(gc, domid, usbdev);
+ goto out;
+ }
+ break;
+ case LIBXL_USBCTRL_TYPE_DEVICEMODEL:
+ default:
+ LOG(ERROR, "Unsupported usb controller type");
+ rc = ERROR_FAIL;
+ goto out;
+ }
+
+ rc = 0;
+
+out:
+ libxl_device_usbctrl_dispose(&usbctrl);
+ libxl_usbctrlinfo_dispose(&usbctrlinfo);
+ return rc;
+}
+
+/* AO operation to add a usb device.
+ *
+ * Generally, it does:
+ * 1) check if the usb device type is assignable
+ * 2) check if the usb device is already assigned to a domain
+ * 3) add 'busid' of the usb device to xenstore contoller/port/.
+ * (PVUSB driver watches the xenstore changes and will detect that.)
+ * 4) unbind usb device from original driver and bind to usbback.
+ * If usb device has many interfaces, then:
+ * - unbind each interface from its original driver and bind to usbback.
+ * - store the original driver to xenstore for later rebinding when
+ * detaching the device.
+ *
+ * Before calling this function, aodev should be properly filled:
+ * aodev->ao, aodev->callback, aodev->update_json, ...
+ */
+void libxl__device_usbdev_add(libxl__egc *egc, uint32_t domid,
+ libxl_device_usbdev *usbdev,
+ libxl__ao_device *aodev)
+{
+ STATE_AO_GC(aodev->ao);
+ int rc;
+ libxl_device_usbdev *assigned;
+ int num_assigned;
+ libxl_device_usbctrl usbctrl;
+ libxl_usbctrlinfo usbctrlinfo;
+
+ libxl_device_usbctrl_init(&usbctrl);
+ libxl_usbctrlinfo_init(&usbctrlinfo);
+
+ /* Currently only support adding USB device from Dom0 backend.
+ * So, if USB controller is specified, check its backend domain,
+ * if it's not Dom0, report error.
+ */
+ if (usbdev->ctrl != -1) {
+ usbctrl.devid = usbdev->ctrl;
+ rc = libxl_device_usbctrl_getinfo(CTX, domid, &usbctrl, &usbctrlinfo);
+ if (rc) goto out;
+
+ if (usbctrlinfo.backend_id != LIBXL_TOOLSTACK_DOMID) {
+ LOG(ERROR, "Don't support adding USB device from non-Dom0 backend");
+ rc = ERROR_INVAL;
+ goto out;
+ }
+ }
+
+ /* check usb device is assignable type */
+ if (!is_usbdev_assignable(gc, usbdev)) {
+ LOG(ERROR, "USB device is not assignable.");
+ rc = ERROR_FAIL;
+ goto out;
+ }
+
+ /* check usb device is already assigned */
+ rc = get_assigned_devices(gc, &assigned, &num_assigned);
+ if (rc) {
+ LOG(ERROR, "cannot determine if device is assigned,"
+ " refusing to continue");
+ goto out;
+ }
+
+ if (is_usbdev_in_array(assigned, num_assigned, usbdev)) {
+ LOG(ERROR, "USB device already attached to a domain");
+ rc = ERROR_INVAL;
+ goto out;
+ }
+
+ /* fill default values, e.g, if usbdev->ctrl and usbdev->port
+ * not specified, choose available controller:port and fill in. */
+ rc = libxl__device_usbdev_setdefault(gc, domid, usbdev,
+ aodev->update_json);
+ if (rc) goto out;
+
+ /* do actual adding usb device operation */
+ rc = do_usbdev_add(gc, domid, usbdev, aodev->update_json);
+
+out:
+ libxl_device_usbctrl_dispose(&usbctrl);
+ libxl_usbctrlinfo_dispose(&usbctrlinfo);
+ aodev->rc = rc;
+ aodev->callback(egc, aodev);
+ return;
+}
+
+static int do_usbdev_remove(libxl__gc *gc, uint32_t domid,
+ libxl_device_usbdev *usbdev)
+{
+ int rc;
+ char *busid;
+ libxl_device_usbctrl usbctrl;
+ libxl_usbctrlinfo usbctrlinfo;
+
+ libxl_device_usbctrl_init(&usbctrl);
+ libxl_usbctrlinfo_init(&usbctrlinfo);
+ usbctrl.devid = usbdev->ctrl;
+
+ rc = libxl_device_usbctrl_getinfo(CTX, domid, &usbctrl, &usbctrlinfo);
+ if (rc) goto out;
+
+ switch (usbctrlinfo.type) {
+ case LIBXL_USBCTRL_TYPE_PV:
+ busid = usbdev_busid_from_ctrlport(gc, domid, usbdev);
+ if (!busid) {
+ rc = ERROR_FAIL;
+ goto out;
+ }
+
+ rc = libxl__device_usbdev_remove_xenstore(gc, domid, usbdev);
+ if (rc) goto out;
+
+ rc = usbback_dev_unassign(gc, busid);
+ if (rc) {
+ /* Till here, usb device information is already removed
+ * from xenstore, usb list couldn't list it any more.
+ * If unassign usb device from usbback failed, export
+ * warning only so that user could handle driver status
+ * later.
+ */
+ LOG(WARN, "unbind usb device from usbback and rebind to its "
+ "original driver failed");
+ }
+ break;
+ case LIBXL_USBCTRL_TYPE_DEVICEMODEL:
+ default:
+ LOG(ERROR, "Unsupported usb controller type");
+ rc = ERROR_FAIL;
+ goto out;
+ }
+
+ rc = 0;
+
+out:
+ libxl_device_usbctrl_dispose(&usbctrl);
+ libxl_usbctrlinfo_dispose(&usbctrlinfo);
+ return rc;
+}
+
+/* Operation to remove usb device.
+ *
+ * Generally, it does:
+ * 1) check if the usb device is assigned to the domain
+ * 2) remove the usb device from xenstore controller/port.
+ * 3) unbind usb device from usbback and rebind to its original driver.
+ * If usb device has many interfaces, do it to each interface.
+ */
+static int libxl__device_usbdev_remove(libxl__gc *gc, uint32_t domid,
+ libxl_device_usbdev *usbdev)
+{
+ libxl_usbctrlinfo usbctrlinfo;
+ libxl_device_usbctrl usbctrl;
+ int rc;
+
+ if (usbdev->ctrl < 0 || usbdev->port < 1) {
+ LOG(ERROR, "Invalid USB device");
+ return ERROR_FAIL;
+ }
+
+ libxl_device_usbctrl_init(&usbctrl);
+ libxl_usbctrlinfo_init(&usbctrlinfo);
+ usbctrl.devid = usbdev->ctrl;
+
+ rc = libxl_device_usbctrl_getinfo(CTX, domid, &usbctrl, &usbctrlinfo);
+ if (rc) goto out;
+
+ if (usbctrlinfo.backend_id != LIBXL_TOOLSTACK_DOMID) {
+ LOG(ERROR, "Don't support removing USB device from non-Dom0 backend");
+ rc = ERROR_INVAL;
+ goto out;
+ }
+
+ /* do actual removing usb device operation */
+ rc = do_usbdev_remove(gc, domid, usbdev);
+
+out:
+ libxl_device_usbctrl_dispose(&usbctrl);
+ libxl_usbctrlinfo_dispose(&usbctrlinfo);
+ return rc;
+}
+
+int libxl_device_usbdev_remove(libxl_ctx *ctx, uint32_t domid,
+ libxl_device_usbdev *usbdev,
+ const libxl_asyncop_how *ao_how)
+
+{
+ AO_CREATE(ctx, domid, ao_how);
+ int rc;
+
+ rc = libxl__device_usbdev_remove(gc, domid, usbdev);
+
+ libxl__ao_complete(egc, ao, rc);
+ return AO_INPROGRESS;
+}
+
+int libxl_ctrlport_to_device_usbdev(libxl_ctx *ctx,
+ uint32_t domid,
+ int ctrl,
+ int port,
+ libxl_device_usbdev *usbdev)
+{
+ GC_INIT(ctx);
+ const char *dompath, *fe_path, *be_path, *busid;
+ int rc;
+
+ dompath = libxl__xs_get_dompath(gc, domid);
+
+ fe_path = GCSPRINTF("%s/device/vusb/%d", dompath, ctrl);
+
+ be_path = vusb_be_from_xs_fe(gc, fe_path, domid);
+ if (!be_path) {
+ rc = ERROR_FAIL;
+ goto out;
+ }
+
+ rc = libxl__xs_read_checked(gc, XBT_NULL,
+ GCSPRINTF("%s/port/%d", be_path, port),
+ &busid);
+ if (rc) goto out;
+
+ if (!busid || !strcmp(busid, "")) {
+ rc = ERROR_FAIL;
+ goto out;
+ }
+
+ usbdev->ctrl = ctrl;
+ usbdev->port = port;
+ usbdev->type = LIBXL_USBDEV_TYPE_HOSTDEV;
+ rc = usbdev_busaddr_from_busid(gc, busid,
+ &usbdev->u.hostdev.hostbus,
+ &usbdev->u.hostdev.hostaddr);
+
+out:
+ GC_FREE;
+ return rc;
+}
+/*
+ * Local variables:
+ * mode: C
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 9658356..befee94 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -606,6 +606,35 @@ libxl_device_rdm = Struct("device_rdm", [
("policy", libxl_rdm_reserve_policy),
])
+libxl_usbctrl_type = Enumeration("usbctrl_type", [
+ (0, "AUTO"),
+ (1, "PV"),
+ (2, "DEVICEMODEL"),
+ ])
+
+libxl_usbdev_type = Enumeration("usbdev_type", [
+ (1, "hostdev"),
+ ])
+
+libxl_device_usbctrl = Struct("device_usbctrl", [
+ ("type", libxl_usbctrl_type),
+ ("devid", libxl_devid),
+ ("version", integer),
+ ("ports", integer),
+ ("backend_domid", libxl_domid),
+ ("backend_domname", string),
+ ])
+
+libxl_device_usbdev = Struct("device_usbdev", [
+ ("ctrl", libxl_devid),
+ ("port", integer),
+ ("u", KeyedUnion(None, libxl_usbdev_type, "type",
+ [("hostdev", Struct(None, [
+ ("hostbus", uint8),
+ ("hostaddr", uint8)])),
+ ])),
+ ])
+
libxl_device_dtdev = Struct("device_dtdev", [
("path", string),
])
@@ -644,6 +673,8 @@ libxl_domain_config = Struct("domain_config", [
# a channel manifests as a console with a name,
# see docs/misc/channels.txt
("channels", Array(libxl_device_channel, "num_channels")),
+ ("usbctrls", Array(libxl_device_usbctrl, "num_usbctrls")),
+ ("usbdevs", Array(libxl_device_usbdev, "num_usbdevs")),
("on_poweroff", libxl_action_on_shutdown),
("on_reboot", libxl_action_on_shutdown),
@@ -687,6 +718,21 @@ libxl_vtpminfo = Struct("vtpminfo", [
("uuid", libxl_uuid),
], dir=DIR_OUT)
+libxl_usbctrlinfo = Struct("usbctrlinfo", [
+ ("type", libxl_usbctrl_type),
+ ("devid", libxl_devid),
+ ("version", integer),
+ ("ports", integer),
+ ("backend", string),
+ ("backend_id", uint32),
+ ("frontend", string),
+ ("frontend_id", uint32),
+ ("state", integer),
+ ("evtch", integer),
+ ("ref_urb", integer),
+ ("ref_conn", integer),
+ ], dir=DIR_OUT)
+
libxl_vcpuinfo = Struct("vcpuinfo", [
("vcpuid", uint32),
("cpu", uint32),
diff --git a/tools/libxl/libxl_types_internal.idl b/tools/libxl/libxl_types_internal.idl
index 5e55685..696f5f8 100644
--- a/tools/libxl/libxl_types_internal.idl
+++ b/tools/libxl/libxl_types_internal.idl
@@ -22,6 +22,7 @@ libxl__device_kind = Enumeration("device_kind", [
(6, "VKBD"),
(7, "CONSOLE"),
(8, "VTPM"),
+ (9, "VUSB"),
])
libxl__console_backend = Enumeration("console_backend", [
diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c
index e64f301..94dac4e 100644
--- a/tools/libxl/libxl_utils.c
+++ b/tools/libxl/libxl_utils.c
@@ -1311,6 +1311,24 @@ int libxl__random_bytes(libxl__gc *gc, uint8_t *buf, size_t len)
return ret;
}
+void libxl_device_usbctrl_list_free(libxl_device_usbctrl *list, int nr)
+{
+ int i;
+
+ for (i = 0; i < nr; i++)
+ libxl_device_usbctrl_dispose(&list[i]);
+ free(list);
+}
+
+void libxl_device_usbdev_list_free(libxl_device_usbdev *list, int nr)
+{
+ int i;
+
+ for (i = 0; i < nr; i++)
+ libxl_device_usbdev_dispose(&list[i]);
+ free(list);
+}
+
/*
* Local variables:
* mode: C
diff --git a/tools/libxl/libxl_utils.h b/tools/libxl/libxl_utils.h
index 339ebdf..4495417 100644
--- a/tools/libxl/libxl_utils.h
+++ b/tools/libxl/libxl_utils.h
@@ -76,6 +76,11 @@ int libxl_uuid_to_device_vtpm(libxl_ctx *ctx, uint32_t domid,
libxl_uuid *uuid, libxl_device_vtpm *vtpm);
int libxl_devid_to_device_vtpm(libxl_ctx *ctx, uint32_t domid,
int devid, libxl_device_vtpm *vtpm);
+int libxl_devid_to_device_usbctrl(libxl_ctx *ctx, uint32_t domid,
+ int devid, libxl_device_usbctrl *usbctrl);
+int libxl_ctrlport_to_device_usbdev(libxl_ctx *ctx, uint32_t domid,
+ int ctrl, int port,
+ libxl_device_usbdev *usbdev);
int libxl_bitmap_alloc(libxl_ctx *ctx, libxl_bitmap *bitmap, int n_bits);
/* Allocated bimap is from malloc, libxl_bitmap_dispose() to be
--
2.1.4
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH V13 4/5] domcreate: support pvusb in configuration file
2016-01-19 8:39 [PATCH V13 0/5] xen pvusb toolstack work Chunyan Liu
` (2 preceding siblings ...)
2016-01-19 8:39 ` [PATCH V13 3/5] libxl: add pvusb API Chunyan Liu
@ 2016-01-19 8:39 ` Chunyan Liu
2016-01-19 18:08 ` Ian Jackson
2016-01-19 8:39 ` [PATCH V13 5/5] xl: add pvusb commands Chunyan Liu
4 siblings, 1 reply; 42+ messages in thread
From: Chunyan Liu @ 2016-01-19 8:39 UTC (permalink / raw)
To: xen-devel
Cc: jgross, wei.liu2, ian.campbell, george.dunlap, Ian.Jackson,
Chunyan Liu, jfehlig, Simon Cao
Add code to support pvusb in domain config file. One could specify
usbctrl and usb in domain's configuration file and create domain,
then usb controllers will be created and usb device would be attached
to guest automatically.
One could specify usb controllers and usb devices in config file
like this:
usbctrl=['version=2,ports=4', 'version=1, ports=4', ]
usbdev=['hostbus=2, hostaddr=1, controller=0,port=1', ]
Signed-off-by: Chunyan Liu <cyliu@suse.com>
Signed-off-by: Simon Cao <caobosimon@gmail.com>
Reviewed-by: George Dunlap <george.dunlap@citrix.com>
---
Changes:
* adjust patch order of this patch and next patch, so that
next patch can use docs generated in this patch.
docs/man/xl.cfg.pod.5 | 84 +++++++++++++++++++++++++++++++++
tools/libxl/libxl_create.c | 73 +++++++++++++++++++++++++++--
tools/libxl/libxl_device.c | 4 ++
tools/libxl/libxl_internal.h | 8 ++++
tools/libxl/xl_cmdimpl.c | 107 ++++++++++++++++++++++++++++++++++++++++++-
5 files changed, 272 insertions(+), 4 deletions(-)
diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
index 8899f75..99ef9ca 100644
--- a/docs/man/xl.cfg.pod.5
+++ b/docs/man/xl.cfg.pod.5
@@ -722,6 +722,90 @@ Note this may be overridden by rdm_policy option in PCI device configuration.
=back
+=item B<usbctrl=[ "USBCTRL_SPEC_STRING", "USBCTRL_SPEC_STRING", ... ]>
+
+Specifies the USB controllers created for this guest. Each
+B<USB_SPEC_STRING> has the form C<KEY=VALUE,KEY=VALUE,...> where:
+
+=over 4
+
+=item B<KEY=VALUE>
+
+Possible B<KEY>s are:
+
+=over 4
+
+=item B<type=TYPE>
+
+Specifies the usb controller type. Currently only 'pv' and 'auto'
+are supported.
+
+=item B<version=VERSION>
+
+Specifies the usb controller version. Possible values include
+1 (USB1.1) and 2 (USB2.0). Default is 2 (USB2.0).
+
+=item B<ports=PORTS>
+
+Specifies the total ports of the usb controller. The maximum
+number is 31. Default is 8.
+
+USB controler ids start from 0. In line with the USB spec, however,
+ports on a controller start from 1.
+
+E.g.
+usbctrl=["version=1,ports=4", "version=2,ports=8",]
+The first controller has:
+controller id = 0, and port 1,2,3,4.
+The second controller has:
+controller id = 1, and port 1,2,3,4,5,6,7,8.
+
+=back
+
+=back
+
+=item B<usbdev=[ "USB_SPEC_STRING", "USB_SPEC_STRING", ... ]>
+
+Specifies the USB devices to be attached to the guest at boot. Each
+B<USB_SPEC_STRING> has the form C<KEY=VALUE,KEY=VALUE,...> where:
+
+=over 4
+
+=item B<KEY=VALUE>
+
+Possible B<KEY>s are:
+
+=over 4
+
+=item B<devtype=hostdev>
+
+Specifies USB device type. Currently only support 'hostdev'.
+
+=item B<hostbus=busnum>
+
+Specifies busnum of the USB device from the host perspective.
+
+=item B<hostaddr=devnum>
+
+Specifies devnum of the USB device from the host perspective.
+
+=item B<controller=CONTROLLER>
+
+Specifies USB controller id, to which controller the USB device is attached.
+
+=item B<port=PORT>
+
+Specifies USB port, to which port the USB device is attached. B<port=PORT>
+is valid only when B<controller=CONTROLLER> is specified.
+
+=back
+
+If no controller is specified, an available controller:port combination
+will be used. If there are no available controller:port options,
+a new controller will be created.
+
+=back
+
=item B<pci=[ "PCI_SPEC_STRING", "PCI_SPEC_STRING", ... ]>
Specifies the host PCI devices to passthrough to this guest. Each B<PCI_SPEC_STRING>
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 261816a..7c87c34 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -733,6 +733,10 @@ static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *aodevs,
static void domcreate_attach_vtpms(libxl__egc *egc, libxl__multidev *multidev,
int ret);
+static void domcreate_attach_usbctrls(libxl__egc *egc,
+ libxl__multidev *multidev, int ret);
+static void domcreate_attach_usbdevs(libxl__egc *egc, libxl__multidev *multidev,
+ int ret);
static void domcreate_attach_pci(libxl__egc *egc, libxl__multidev *aodevs,
int ret);
static void domcreate_attach_dtdev(libxl__egc *egc,
@@ -1398,13 +1402,13 @@ static void domcreate_attach_vtpms(libxl__egc *egc,
if (d_config->num_vtpms > 0) {
/* Attach vtpms */
libxl__multidev_begin(ao, &dcs->multidev);
- dcs->multidev.callback = domcreate_attach_pci;
+ dcs->multidev.callback = domcreate_attach_usbctrls;
libxl__add_vtpms(egc, ao, domid, d_config, &dcs->multidev);
libxl__multidev_prepared(egc, &dcs->multidev, 0);
return;
}
- domcreate_attach_pci(egc, multidev, 0);
+ domcreate_attach_usbctrls(egc, multidev, 0);
return;
error_out:
@@ -1412,6 +1416,69 @@ error_out:
domcreate_complete(egc, dcs, ret);
}
+static void domcreate_attach_usbctrls(libxl__egc *egc,
+ libxl__multidev *multidev, int ret)
+{
+ libxl__domain_create_state *dcs = CONTAINER_OF(multidev, *dcs, multidev);
+ STATE_AO_GC(dcs->ao);
+ int domid = dcs->guest_domid;
+
+ libxl_domain_config *const d_config = dcs->guest_config;
+
+ if (ret) {
+ LOG(ERROR, "unable to add vtpm devices");
+ goto error_out;
+ }
+
+ if (d_config->num_usbctrls > 0) {
+ /* Attach usbctrls */
+ libxl__multidev_begin(ao, &dcs->multidev);
+ dcs->multidev.callback = domcreate_attach_usbdevs;
+ libxl__add_usbctrls(egc, ao, domid, d_config, &dcs->multidev);
+ libxl__multidev_prepared(egc, &dcs->multidev, 0);
+ return;
+ }
+
+ domcreate_attach_usbdevs(egc, multidev, 0);
+ return;
+
+error_out:
+ assert(ret);
+ domcreate_complete(egc, dcs, ret);
+}
+
+
+static void domcreate_attach_usbdevs(libxl__egc *egc, libxl__multidev *multidev,
+ int ret)
+{
+ libxl__domain_create_state *dcs = CONTAINER_OF(multidev, *dcs, multidev);
+ STATE_AO_GC(dcs->ao);
+ int domid = dcs->guest_domid;
+
+ libxl_domain_config *const d_config = dcs->guest_config;
+
+ if (ret) {
+ LOG(ERROR, "unable to add usbctrl devices");
+ goto error_out;
+ }
+
+ if (d_config->num_usbdevs > 0) {
+ /* Attach usbctrls */
+ libxl__multidev_begin(ao, &dcs->multidev);
+ dcs->multidev.callback = domcreate_attach_pci;
+ libxl__add_usbdevs(egc, ao, domid, d_config, &dcs->multidev);
+ libxl__multidev_prepared(egc, &dcs->multidev, 0);
+ return;
+ }
+
+ domcreate_attach_pci(egc, multidev, 0);
+ return;
+
+error_out:
+ assert(ret);
+ domcreate_complete(egc, dcs, ret);
+}
+
static void domcreate_attach_pci(libxl__egc *egc, libxl__multidev *multidev,
int ret)
{
@@ -1424,7 +1491,7 @@ static void domcreate_attach_pci(libxl__egc *egc, libxl__multidev *multidev,
libxl_domain_config *const d_config = dcs->guest_config;
if (ret) {
- LOG(ERROR, "unable to add vtpm devices");
+ LOG(ERROR, "unable to add usb devices");
goto error_out;
}
diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index b7a6a13..4ced9b6 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -544,6 +544,8 @@ void libxl__multidev_prepared(libxl__egc *egc,
* libxl__add_disks
* libxl__add_nics
* libxl__add_vtpms
+ * libxl__add_usbctrls
+ * libxl__add_usbs
*/
#define DEFINE_DEVICES_ADD(type) \
@@ -563,6 +565,8 @@ void libxl__multidev_prepared(libxl__egc *egc,
DEFINE_DEVICES_ADD(disk)
DEFINE_DEVICES_ADD(nic)
DEFINE_DEVICES_ADD(vtpm)
+DEFINE_DEVICES_ADD(usbctrl)
+DEFINE_DEVICES_ADD(usbdev)
#undef DEFINE_DEVICES_ADD
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 0ccad9a..64925b1 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -3319,6 +3319,14 @@ _hidden void libxl__add_vtpms(libxl__egc *egc, libxl__ao *ao, uint32_t domid,
libxl_domain_config *d_config,
libxl__multidev *multidev);
+_hidden void libxl__add_usbctrls(libxl__egc *egc, libxl__ao *ao,
+ uint32_t domid, libxl_domain_config *d_config,
+ libxl__multidev *multidev);
+
+_hidden void libxl__add_usbdevs(libxl__egc *egc, libxl__ao *ao,
+ uint32_t domid, libxl_domain_config *d_config,
+ libxl__multidev *multidev);
+
/*----- device model creation -----*/
/* First layer; wraps libxl__spawn_spawn. */
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index f9933cb..d0715e6 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1255,6 +1255,58 @@ static void parse_vnuma_config(const XLU_Config *config,
free(vcpu_parsed);
}
+/* Parses usbctrl data and adds info into usbctrl
+ * Returns 1 if the input token does not match one of the keys
+ * or parsed values are not correct. Successful parse returns 0 */
+static int parse_usbctrl_config(libxl_device_usbctrl *usbctrl, char *token)
+{
+ char *oparg;
+
+ if (MATCH_OPTION("type", token, oparg)) {
+ if (libxl_usbctrl_type_from_string(oparg, &usbctrl->type)) {
+ fprintf(stderr, "Invalid usb controller type '%s'\n", oparg);
+ return 1;
+ }
+ } else if (MATCH_OPTION("version", token, oparg)) {
+ usbctrl->version = atoi(oparg);
+ } else if (MATCH_OPTION("ports", token, oparg)) {
+ usbctrl->ports = atoi(oparg);
+ } else {
+ fprintf(stderr, "Unknown string `%s' in usbctrl spec\n", token);
+ return 1;
+ }
+
+ return 0;
+}
+
+/* Parses usbdev data and adds info into usbdev
+ * Returns 1 if the input token does not match one of the keys
+ * or parsed values are not correct. Successful parse returns 0 */
+static int parse_usbdev_config(libxl_device_usbdev *usbdev, char *token)
+{
+ char *oparg;
+
+ if (MATCH_OPTION("type", token, oparg)) {
+ if (libxl_usbdev_type_from_string(oparg, &usbdev->type)) {
+ fprintf(stderr, "Invalid usb device type: %s\n", optarg);
+ return 1;
+ }
+ } else if (MATCH_OPTION("hostbus", token, oparg)) {
+ usbdev->u.hostdev.hostbus = strtoul(oparg, NULL, 0);
+ } else if (MATCH_OPTION("hostaddr", token, oparg)) {
+ usbdev->u.hostdev.hostaddr = strtoul(oparg, NULL, 0);
+ } else if (MATCH_OPTION("controller", token, oparg)) {
+ usbdev->ctrl = atoi(oparg);
+ } else if (MATCH_OPTION("port", token, oparg)) {
+ usbdev->port = atoi(oparg);
+ } else {
+ fprintf(stderr, "Unknown string `%s' in usbdev spec\n", token);
+ return 1;
+ }
+
+ return 0;
+}
+
static void parse_config_data(const char *config_source,
const char *config_data,
int config_len,
@@ -1263,7 +1315,8 @@ static void parse_config_data(const char *config_source,
const char *buf;
long l, vcpus = 0;
XLU_Config *config;
- XLU_ConfigList *cpus, *vbds, *nics, *pcis, *cvfbs, *cpuids, *vtpms;
+ XLU_ConfigList *cpus, *vbds, *nics, *pcis, *cvfbs, *cpuids, *vtpms,
+ *usbctrls, *usbdevs;
XLU_ConfigList *channels, *ioports, *irqs, *iomem, *viridian, *dtdevs;
int num_ioports, num_irqs, num_iomem, num_cpus, num_viridian;
int pci_power_mgmt = 0;
@@ -2080,6 +2133,58 @@ skip_vfb:
}
}
+ if (!xlu_cfg_get_list(config, "usbctrl", &usbctrls, 0, 0)) {
+ d_config->num_usbctrls = 0;
+ d_config->usbctrls = NULL;
+ while ((buf = xlu_cfg_get_listitem(usbctrls, d_config->num_usbctrls))
+ != NULL) {
+ libxl_device_usbctrl *usbctrl;
+ char *buf2 = strdup(buf);
+ char *p;
+
+ usbctrl = ARRAY_EXTEND_INIT(d_config->usbctrls,
+ d_config->num_usbctrls,
+ libxl_device_usbctrl_init);
+ p = strtok(buf2, ",");
+ if (!p)
+ goto skip_usbctrl;
+ do {
+ while (*p == ' ')
+ p++;
+ if (parse_usbctrl_config(usbctrl, p))
+ exit(1);
+ } while ((p = strtok(NULL, ",")) != NULL);
+skip_usbctrl:
+ free(buf2);
+ }
+ }
+
+ if (!xlu_cfg_get_list(config, "usbdev", &usbdevs, 0, 0)) {
+ d_config->num_usbdevs = 0;
+ d_config->usbdevs = NULL;
+ while ((buf = xlu_cfg_get_listitem(usbdevs, d_config->num_usbdevs))
+ != NULL) {
+ libxl_device_usbdev *usbdev;
+ char *buf2 = strdup(buf);
+ char *p;
+
+ usbdev = ARRAY_EXTEND_INIT_NODEVID(d_config->usbdevs,
+ d_config->num_usbdevs,
+ libxl_device_usbdev_init);
+ p = strtok(buf2, ",");
+ if (!p)
+ goto skip_usbdev;
+ do {
+ while (*p == ' ')
+ p++;
+ if (parse_usbdev_config(usbdev, p))
+ exit(1);
+ } while ((p = strtok(NULL, ",")) != NULL);
+skip_usbdev:
+ free(buf2);
+ }
+ }
+
switch (xlu_cfg_get_list(config, "cpuid", &cpuids, 0, 1)) {
case 0:
{
--
2.1.4
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH V13 5/5] xl: add pvusb commands
2016-01-19 8:39 [PATCH V13 0/5] xen pvusb toolstack work Chunyan Liu
` (3 preceding siblings ...)
2016-01-19 8:39 ` [PATCH V13 4/5] domcreate: support pvusb in configuration file Chunyan Liu
@ 2016-01-19 8:39 ` Chunyan Liu
2016-01-19 18:11 ` Ian Jackson
2016-02-16 16:56 ` Olaf Hering
4 siblings, 2 replies; 42+ messages in thread
From: Chunyan Liu @ 2016-01-19 8:39 UTC (permalink / raw)
To: xen-devel
Cc: jgross, wei.liu2, ian.campbell, george.dunlap, Ian.Jackson,
Chunyan Liu, jfehlig, Simon Cao
Add pvusb commands: usbctrl-attach, usbctrl-detach, usb-list,
usbdev-attach and usbdev-detach.
To attach a usb device to guest through pvusb, one could follow
following example:
#xl usbctrl-attach test_vm version=1 ports=8
#xl usb-list test_vm
will show the usb controllers and port usage under the domain.
#xl usbdev-attach test_vm hostbus=1 hostaddr=2
will find the first usable controller:port, and attach usb
device whose busnum is 1 and devnum is 6.
One could also specify which <controller> and which <port>.
#xl usbdev-detach test_vm 0 1
will detach USB device under controller 0 port 1.
#xl usbctrl-detach test_vm dev_id
will destroy the controller with specified dev_id. Dev_id
can be traced in usb-list info.
Signed-off-by: Chunyan Liu <cyliu@suse.com>
Signed-off-by: Simon Cao <caobosimon@gmail.com>
Reviewed-by: George Dunlap <george.dunlap@citrix.com>
---
changes:
* update docs to reuse documentation in xl.cfg
* remove backend patch information from usb-list
docs/man/xl.pod.1 | 35 +++++++++
tools/libxl/xl.h | 5 ++
tools/libxl/xl_cmdimpl.c | 190 ++++++++++++++++++++++++++++++++++++++++++++++
tools/libxl/xl_cmdtable.c | 25 ++++++
4 files changed, 255 insertions(+)
diff --git a/docs/man/xl.pod.1 b/docs/man/xl.pod.1
index 4279c7c..7e0a380 100644
--- a/docs/man/xl.pod.1
+++ b/docs/man/xl.pod.1
@@ -1345,6 +1345,41 @@ List pass-through pci devices for a domain.
=back
+=head1 USB PASS-THROUGH
+
+=over 4
+
+=item B<usbctrl-attach> I<domain-id> I<usbctrl-device>
+
+Create a new USB controller in the domain specified by I<domain-id>,
+I<usbctrl-device> describes the device to attach, using the same format
+as the B<usbctrl> string in the domain config file. See L<xl.cfg> for
+more information.
+
+=item B<usbctrl-detach> I<domain-id> I<devid>
+
+Destroy a USB controller from the specified domain.
+B<devid> is devid of the USB controller.
+
+=item B<usbdev-attach> I<domain-id> I<usbdev-device>
+
+Hot-plug a new pass-through USB device to the domain specified by
+I<domain-id>, I<usbdev-device> describes the device to attach, using
+the same format as the B<usbdev> string in the domain config file.
+See L<xl.cfg> for more information.
+
+=item B<usbdev-detach> I<domain-id> I<controller=devid> I<port=number>
+
+Hot-unplug a previously assigned USB device from a domain.
+B<controller=devid> and B<port=number> is USB controller:port in guest
+where the USB device is attached to.
+
+=item B<usb-list> I<domain-id>
+
+List pass-through usb devices for a domain.
+
+=back
+
=head1 TMEM
=over 4
diff --git a/tools/libxl/xl.h b/tools/libxl/xl.h
index bdab125..309627a 100644
--- a/tools/libxl/xl.h
+++ b/tools/libxl/xl.h
@@ -92,6 +92,11 @@ int main_blockdetach(int argc, char **argv);
int main_vtpmattach(int argc, char **argv);
int main_vtpmlist(int argc, char **argv);
int main_vtpmdetach(int argc, char **argv);
+int main_usbctrl_attach(int argc, char **argv);
+int main_usbctrl_detach(int argc, char **argv);
+int main_usbdev_attach(int argc, char **argv);
+int main_usbdev_detach(int argc, char **argv);
+int main_usblist(int argc, char **argv);
int main_uptime(int argc, char **argv);
int main_claims(int argc, char **argv);
int main_tmem_list(int argc, char **argv);
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index d0715e6..a968e46 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -3507,6 +3507,196 @@ int main_cd_insert(int argc, char **argv)
return 0;
}
+int main_usbctrl_attach(int argc, char **argv)
+{
+ uint32_t domid;
+ int opt, rc = 0;
+ libxl_device_usbctrl usbctrl;
+
+ SWITCH_FOREACH_OPT(opt, "", NULL, "usbctrl-attach", 1) {
+ /* No options */
+ }
+
+ domid = find_domain(argv[optind++]);
+
+ libxl_device_usbctrl_init(&usbctrl);
+
+ for (argv += optind, argc -= optind; argc > 0; ++argv, --argc) {
+ if (parse_usbctrl_config(&usbctrl, *argv))
+ return 1;
+ }
+
+ rc = libxl_device_usbctrl_add(ctx, domid, &usbctrl, 0);
+ if (rc) {
+ fprintf(stderr, "libxl_device_usbctrl_add failed.\n");
+ rc = 1;
+ }
+
+ libxl_device_usbctrl_dispose(&usbctrl);
+ return rc;
+}
+
+int main_usbctrl_detach(int argc, char **argv)
+{
+ uint32_t domid;
+ int opt, devid, rc;
+ libxl_device_usbctrl usbctrl;
+
+ SWITCH_FOREACH_OPT(opt, "", NULL, "usbctrl-detach", 2) {
+ /* No options */
+ }
+
+ domid = find_domain(argv[optind]);
+ devid = atoi(argv[optind+1]);
+
+ libxl_device_usbctrl_init(&usbctrl);
+ if (libxl_devid_to_device_usbctrl(ctx, domid, devid, &usbctrl)) {
+ fprintf(stderr, "Unknown device %s.\n", argv[optind+1]);
+ return 1;
+ }
+
+ rc = libxl_device_usbctrl_remove(ctx, domid, &usbctrl, 0);
+ if (rc) {
+ fprintf(stderr, "libxl_device_usbctrl_remove failed.\n");
+ rc = 1;
+ }
+
+ libxl_device_usbctrl_dispose(&usbctrl);
+ return rc;
+
+}
+
+int main_usbdev_attach(int argc, char **argv)
+{
+ uint32_t domid;
+ int opt, rc;
+ libxl_device_usbdev usbdev;
+
+ SWITCH_FOREACH_OPT(opt, "", NULL, "usbdev-attach", 2) {
+ /* No options */
+ }
+
+ libxl_device_usbdev_init(&usbdev);
+
+ domid = find_domain(argv[optind++]);
+
+ for (argv += optind, argc -= optind; argc > 0; ++argv, --argc) {
+ if (parse_usbdev_config(&usbdev, *argv))
+ return 1;
+ }
+
+ rc = libxl_device_usbdev_add(ctx, domid, &usbdev, 0);
+ if (rc) {
+ fprintf(stderr, "libxl_device_usbdev_add failed.\n");
+ rc = 1;
+ }
+
+ libxl_device_usbdev_dispose(&usbdev);
+ return rc;
+}
+
+int main_usbdev_detach(int argc, char **argv)
+{
+ uint32_t domid;
+ int ctrl, port;
+ int opt, rc = 1;
+ libxl_device_usbdev usbdev;
+
+ SWITCH_FOREACH_OPT(opt, "", NULL, "usbdev-detach", 3) {
+ /* No options */
+ }
+
+ domid = find_domain(argv[optind]);
+ ctrl = atoi(argv[optind+1]);
+ port = atoi(argv[optind+2]);
+
+ if (argc - optind > 3) {
+ fprintf(stderr, "Invalid arguments.\n");
+ return 1;
+ }
+
+ libxl_device_usbdev_init(&usbdev);
+ if (libxl_ctrlport_to_device_usbdev(ctx, domid, ctrl, port, &usbdev)) {
+ fprintf(stderr, "Unknown device at controller %d port %d.\n",
+ ctrl, port);
+ return 1;
+ }
+
+ rc = libxl_device_usbdev_remove(ctx, domid, &usbdev, 0);
+ if (rc) {
+ fprintf(stderr, "libxl_device_usbdev_remove failed.\n");
+ rc = 1;
+ }
+
+ libxl_device_usbdev_dispose(&usbdev);
+ return rc;
+}
+
+int main_usblist(int argc, char **argv)
+{
+ uint32_t domid;
+ libxl_device_usbctrl *usbctrls;
+ libxl_usbctrlinfo usbctrlinfo;
+ int numctrl, i, j, opt;
+
+ SWITCH_FOREACH_OPT(opt, "", NULL, "usb-list", 1) {
+ /* No options */
+ }
+
+ domid = find_domain(argv[optind++]);
+
+ if (argc > optind) {
+ fprintf(stderr, "Invalid arguments.\n");
+ exit(-1);
+ }
+
+ usbctrls = libxl_device_usbctrl_list(ctx, domid, &numctrl);
+ if (!usbctrls) {
+ return 0;
+ }
+
+ for (i = 0; i < numctrl; ++i) {
+ printf("%-6s %-6s %-3s %-5s %-7s %-5s\n",
+ "Devid", "Type", "BE", "state", "usb-ver", "ports");
+
+ libxl_usbctrlinfo_init(&usbctrlinfo);
+
+ if (!libxl_device_usbctrl_getinfo(ctx, domid,
+ &usbctrls[i], &usbctrlinfo)) {
+ printf("%-6d %-6s %-3d %-5d %-7d %-5d\n",
+ usbctrlinfo.devid,
+ libxl_usbctrl_type_to_string(usbctrlinfo.type),
+ usbctrlinfo.backend_id, usbctrlinfo.state,
+ usbctrlinfo.version, usbctrlinfo.ports);
+
+ for (j = 1; j <= usbctrlinfo.ports; j++) {
+ libxl_device_usbdev usbdev;
+
+ libxl_device_usbdev_init(&usbdev);
+
+ printf(" Port %d:", j);
+
+ if (!libxl_ctrlport_to_device_usbdev(ctx, domid,
+ usbctrlinfo.devid,
+ j, &usbdev)) {
+ printf(" Bus %03x Device %03x\n",
+ usbdev.u.hostdev.hostbus,
+ usbdev.u.hostdev.hostaddr);
+ } else {
+ printf("\n");
+ }
+
+ libxl_device_usbdev_dispose(&usbdev);
+ }
+ }
+
+ libxl_usbctrlinfo_dispose(&usbctrlinfo);
+ }
+
+ libxl_device_usbctrl_list_free(usbctrls, numctrl);
+ return 0;
+}
+
int main_console(int argc, char **argv)
{
uint32_t domid;
diff --git a/tools/libxl/xl_cmdtable.c b/tools/libxl/xl_cmdtable.c
index fdc1ac6..b14b881 100644
--- a/tools/libxl/xl_cmdtable.c
+++ b/tools/libxl/xl_cmdtable.c
@@ -553,6 +553,31 @@ struct cmd_spec cmd_table[] = {
},
#endif
+ { "usbctrl-attach",
+ &main_usbctrl_attach, 0, 1,
+ "Create a virtual USB controller for a domain",
+ "<Domain> [type=pv] [version=<version>] [ports=<number>]",
+ },
+ { "usbctrl-detach",
+ &main_usbctrl_detach, 0, 1,
+ "Remove the virtual USB controller specified by <DevId> for a domain",
+ "<Domain> <DevId>",
+ },
+ { "usbdev-attach",
+ &main_usbdev_attach, 0, 1,
+ "Attach a USB device to a domain",
+ "<Domain> hostbus=<busnum> hostaddr=<devnum> [controller=<DevId> [port=<port>]]",
+ },
+ { "usbdev-detach",
+ &main_usbdev_detach, 0, 1,
+ "Detach a USB device from a domain",
+ "<Domain> <controller> <port>",
+ },
+ { "usb-list",
+ &main_usblist, 0, 0,
+ "List information about all USB controllers and devices for a domain",
+ "<Domain>",
+ },
};
int cmdtable_len = sizeof(cmd_table)/sizeof(struct cmd_spec);
--
2.1.4
^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [PATCH V13 3/5] libxl: add pvusb API
2016-01-19 8:39 ` [PATCH V13 3/5] libxl: add pvusb API Chunyan Liu
@ 2016-01-19 15:48 ` Ian Jackson
2016-02-02 16:26 ` George Dunlap
2016-02-02 16:48 ` George Dunlap
2016-01-26 16:12 ` Olaf Hering
1 sibling, 2 replies; 42+ messages in thread
From: Ian Jackson @ 2016-01-19 15:48 UTC (permalink / raw)
To: Chunyan Liu
Cc: jgross, wei.liu2, ian.campbell, george.dunlap, George Dunlap,
xen-devel, jfehlig, Simon Cao
Chunyan Liu writes ("[PATCH V13 3/5] libxl: add pvusb API"):
> Add pvusb APIs, including:
> - attach/detach (create/destroy) virtual usb controller.
> - attach/detach usb device
> - list usb controller and usb devices
> - some other helper functions
Thanks. This is making progress but I'm afraid we're not quite there
yet.
> +static int usbback_dev_unassign(libxl__gc *gc, const char *busid)
> +{
...
> + /* Till here, USB device has been unbound from USBBACK and
> + * removed from xenstore, usb list couldn't show it anymore,
> + * so no matter removimg driver path successfully or not,
> + * we will report operation success.
> + */
I'm still unconvinced by this and this may mean that the code in this
function is in the wrong order. Earlier we had this exchange:
> > Ought this function to really report success if these calls fail ?
>
> I think so. Till here, the USB device has already been unbound from
> usbback and removed from xenstore. usb-list cannot list it any more.
The problem is that I think that if this function fails, it can leave
- debris in xenstore (the usbback path)
- the interface bound to the wrong driver
And then there is no way for the user to get libxl to re-attempt the
operation, or clean up. Am I right ?
One way to avoid this kind of problem is to deal with the xenstore
path last. That way the device will still appear as attached to the
domain.
To do this properly I think bind_usbintf may need to become idempotent
even in the face of other callers racing to assign the device. I
think that would mean bind_usbif it would have to know what driver to
expect to find the device bound to.
George, am I right here ?
I have a few other comments too:
> +/* get original driver path of usb interface, stored in @drvpath */
> +static int usbintf_get_drvpath(libxl__gc *gc, const char *intf, char **drvpath)
> +{
> + char *spath, *dp = NULL;
> + struct stat st;
> + int rc;
> +
> + spath = GCSPRINTF(SYSFS_USB_DEV "/%s/driver", intf);
> +
> + rc = lstat(spath, &st);
This `rc' should be `r'. (CODING_STYLE.)
I mentioned this in my review of v12 in the context of
sysfs_write_intf. But there is still more than one occurrence in v13
of `rc' for a syscall return value.
> +static int sysfs_write_intf(libxl__gc *gc, const char *intf, const char *path)
> +{
Last time I wrote:
But there is nothing usb specific about this function. Looking for
similar code elsewhere this function seems very similar to
libxl_pci.c:sysfs_write_bdf, but I won't ask you to try to refactor
these two functions together.
This time I have remembered that libxl_write_exactly exists, and could
be used. Sorry for not saing this last time but I think you can
probably just get rid of this in favour of libxl_write_exactly. If
you think not, please say why.
> +static int bind_usbintf(libxl__gc *gc, const char *intf, const char *drvpath)
> +{
> + char *path;
> + struct stat st;
> +
> + path = GCSPRINTF("%s/%s", drvpath, intf);
> + /* if already bound, return */
> + if (!lstat(path, &st))
> + return 0;
> + else if (errno != ENOENT)
> + return ERROR_FAIL;
This new ERROR_FAIL fails to log anything, and probably should. I
think the anomalous style leads to this mistake. You should say
r = lstat...
and adopt the pattern from the rest of libxl.
Thanks,
Ian.
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH V13 4/5] domcreate: support pvusb in configuration file
2016-01-19 8:39 ` [PATCH V13 4/5] domcreate: support pvusb in configuration file Chunyan Liu
@ 2016-01-19 18:08 ` Ian Jackson
0 siblings, 0 replies; 42+ messages in thread
From: Ian Jackson @ 2016-01-19 18:08 UTC (permalink / raw)
To: Chunyan Liu
Cc: jgross, wei.liu2, ian.campbell, george.dunlap, xen-devel,
jfehlig, Simon Cao
Chunyan Liu writes ("[PATCH V13 4/5] domcreate: support pvusb in configuration file"):
> Add code to support pvusb in domain config file. One could specify
> usbctrl and usb in domain's configuration file and create domain,
> then usb controllers will be created and usb device would be attached
> to guest automatically.
>
> One could specify usb controllers and usb devices in config file
> like this:
> usbctrl=['version=2,ports=4', 'version=1, ports=4', ]
> usbdev=['hostbus=2, hostaddr=1, controller=0,port=1', ]
>
> Signed-off-by: Chunyan Liu <cyliu@suse.com>
> Signed-off-by: Simon Cao <caobosimon@gmail.com>
> Reviewed-by: George Dunlap <george.dunlap@citrix.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH V13 5/5] xl: add pvusb commands
2016-01-19 8:39 ` [PATCH V13 5/5] xl: add pvusb commands Chunyan Liu
@ 2016-01-19 18:11 ` Ian Jackson
2016-01-20 3:10 ` Jim Fehlig
2016-02-16 16:56 ` Olaf Hering
1 sibling, 1 reply; 42+ messages in thread
From: Ian Jackson @ 2016-01-19 18:11 UTC (permalink / raw)
To: Chunyan Liu
Cc: jgross, wei.liu2, ian.campbell, george.dunlap, xen-devel,
jfehlig, Simon Cao
Chunyan Liu writes ("[PATCH V13 5/5] xl: add pvusb commands"):
> Add pvusb commands: usbctrl-attach, usbctrl-detach, usb-list,
> usbdev-attach and usbdev-detach.
Thanks for swapping this with the other patch. It is better now.
> +=item B<usbctrl-attach> I<domain-id> I<usbctrl-device>
However, I think you need to explictly state that the user may (and
indeed, must) pass multiple settings as separate arguments. AFAICT
the parser here doesn't do the ,-splitting.
> +I<usbctrl-device> describes the device to attach, using the same format
> +as the B<usbctrl> string in the domain config file. See L<xl.cfg> for
> +more information.
And this, therefore, is not quite true.
To be clear, I think that you should fix the documentation to match
the code.
Thanks,
Ian.
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH V13 5/5] xl: add pvusb commands
2016-01-19 18:11 ` Ian Jackson
@ 2016-01-20 3:10 ` Jim Fehlig
2016-01-21 12:12 ` Wei Liu
2016-02-16 17:53 ` Ian Jackson
0 siblings, 2 replies; 42+ messages in thread
From: Jim Fehlig @ 2016-01-20 3:10 UTC (permalink / raw)
To: Ian Jackson, Chunyan Liu
Cc: jgross, wei.liu2, ian.campbell, george.dunlap, xen-devel, Simon Cao
On 01/19/2016 11:11 AM, Ian Jackson wrote:
> Chunyan Liu writes ("[PATCH V13 5/5] xl: add pvusb commands"):
>> Add pvusb commands: usbctrl-attach, usbctrl-detach, usb-list,
>> usbdev-attach and usbdev-detach.
> Thanks for swapping this with the other patch. It is better now.
>
>> +=item B<usbctrl-attach> I<domain-id> I<usbctrl-device>
> However, I think you need to explictly state that the user may (and
> indeed, must) pass multiple settings as separate arguments. AFAICT
> the parser here doesn't do the ,-splitting.
I just noticed this is the case with network devices as well. E.g.
#xl network-attach hvm-domU mac=00:16:3e:xx:yy:zz,bridge=br0
libxl: error: libxl_device.c:1095:device_hotplug_child_death_cb: script: Could
not find bridge device xenbr0
main_networkattach() in tools/libxl/xl_cmdimpl.c doesn't split on the ',', so
everything after mac=00:16:3e:xx:yy:zz is ignored. I'd need advice on how to fix
this though. Based on xl-network-configuration doc and Xen tool's long history
of network-attach supporting that syntax, I'd say main_networkattach() should be
changed to split on ','. I could also change the docs. Do tools maintainers have
a preference, or alternative option?
Regards,
Jim
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH V13 5/5] xl: add pvusb commands
2016-01-20 3:10 ` Jim Fehlig
@ 2016-01-21 12:12 ` Wei Liu
2016-01-21 12:21 ` Ian Campbell
2016-02-16 17:53 ` Ian Jackson
1 sibling, 1 reply; 42+ messages in thread
From: Wei Liu @ 2016-01-21 12:12 UTC (permalink / raw)
To: Jim Fehlig
Cc: jgross, wei.liu2, ian.campbell, george.dunlap, Ian Jackson,
Chunyan Liu, xen-devel, Simon Cao
On Tue, Jan 19, 2016 at 08:10:07PM -0700, Jim Fehlig wrote:
> On 01/19/2016 11:11 AM, Ian Jackson wrote:
> > Chunyan Liu writes ("[PATCH V13 5/5] xl: add pvusb commands"):
> >> Add pvusb commands: usbctrl-attach, usbctrl-detach, usb-list,
> >> usbdev-attach and usbdev-detach.
> > Thanks for swapping this with the other patch. It is better now.
> >
> >> +=item B<usbctrl-attach> I<domain-id> I<usbctrl-device>
> > However, I think you need to explictly state that the user may (and
> > indeed, must) pass multiple settings as separate arguments. AFAICT
> > the parser here doesn't do the ,-splitting.
>
> I just noticed this is the case with network devices as well. E.g.
>
> #xl network-attach hvm-domU mac=00:16:3e:xx:yy:zz,bridge=br0
> libxl: error: libxl_device.c:1095:device_hotplug_child_death_cb: script: Could
> not find bridge device xenbr0
>
> main_networkattach() in tools/libxl/xl_cmdimpl.c doesn't split on the ',', so
> everything after mac=00:16:3e:xx:yy:zz is ignored. I'd need advice on how to fix
> this though. Based on xl-network-configuration doc and Xen tool's long history
> of network-attach supporting that syntax, I'd say main_networkattach() should be
> changed to split on ','. I could also change the docs. Do tools maintainers have
> a preference, or alternative option?
>
It is splitting on " " at the moment which is not very desirable.
I think this is a bug. I've added it to my list and will look into
fixing it.
Wei.
> Regards,
> Jim
>
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH V13 5/5] xl: add pvusb commands
2016-01-21 12:12 ` Wei Liu
@ 2016-01-21 12:21 ` Ian Campbell
2016-01-21 12:29 ` Wei Liu
0 siblings, 1 reply; 42+ messages in thread
From: Ian Campbell @ 2016-01-21 12:21 UTC (permalink / raw)
To: Wei Liu, Jim Fehlig
Cc: jgross, george.dunlap, Ian Jackson, Chunyan Liu, xen-devel, Simon Cao
On Thu, 2016-01-21 at 12:12 +0000, Wei Liu wrote:
> On Tue, Jan 19, 2016 at 08:10:07PM -0700, Jim Fehlig wrote:
> > On 01/19/2016 11:11 AM, Ian Jackson wrote:
> > > Chunyan Liu writes ("[PATCH V13 5/5] xl: add pvusb commands"):
> > > > Add pvusb commands: usbctrl-attach, usbctrl-detach, usb-list,
> > > > usbdev-attach and usbdev-detach.
> > > Thanks for swapping this with the other patch. It is better now.
> > >
> > > > +=item B<usbctrl-attach> I<domain-id> I<usbctrl-device>
> > > However, I think you need to explictly state that the user may (and
> > > indeed, must) pass multiple settings as separate arguments. AFAICT
> > > the parser here doesn't do the ,-splitting.
> >
> > I just noticed this is the case with network devices as well. E.g.
> >
> > #xl network-attach hvm-domU mac=00:16:3e:xx:yy:zz,bridge=br0
> > libxl: error: libxl_device.c:1095:device_hotplug_child_death_cb:
> > script: Could
> > not find bridge device xenbr0
> >
> > main_networkattach() in tools/libxl/xl_cmdimpl.c doesn't split on the
> > ',', so
> > everything after mac=00:16:3e:xx:yy:zz is ignored. I'd need advice on
> > how to fix
> > this though. Based on xl-network-configuration doc and Xen tool's long
> > history
> > of network-attach supporting that syntax, I'd say main_networkattach()
> > should be
> > changed to split on ','. I could also change the docs. Do tools
> > maintainers have
> > a preference, or alternative option?
> >
>
> It is splitting on " " at the moment which is not very desirable.
>
> I think this is a bug. I've added it to my list and will look into
> fixing it.
If you are feeling particularly highly motivated then xl_cmdimpl.c contains
quite a lot of adhoc parsing of various comma a space separated lists
(often dupped for a given instance into cmdline vs cfg file, which leads to
unintentional behavioural differences like above) which could usefully be
consolidated into a series of helpers.
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH V13 5/5] xl: add pvusb commands
2016-01-21 12:21 ` Ian Campbell
@ 2016-01-21 12:29 ` Wei Liu
2016-01-21 17:37 ` Ian Jackson
0 siblings, 1 reply; 42+ messages in thread
From: Wei Liu @ 2016-01-21 12:29 UTC (permalink / raw)
To: Ian Campbell
Cc: jgross, Wei Liu, george.dunlap, Ian Jackson, Chunyan Liu,
xen-devel, Jim Fehlig, Simon Cao
On Thu, Jan 21, 2016 at 12:21:11PM +0000, Ian Campbell wrote:
> On Thu, 2016-01-21 at 12:12 +0000, Wei Liu wrote:
> > On Tue, Jan 19, 2016 at 08:10:07PM -0700, Jim Fehlig wrote:
> > > On 01/19/2016 11:11 AM, Ian Jackson wrote:
> > > > Chunyan Liu writes ("[PATCH V13 5/5] xl: add pvusb commands"):
> > > > > Add pvusb commands: usbctrl-attach, usbctrl-detach, usb-list,
> > > > > usbdev-attach and usbdev-detach.
> > > > Thanks for swapping this with the other patch. It is better now.
> > > >
> > > > > +=item B<usbctrl-attach> I<domain-id> I<usbctrl-device>
> > > > However, I think you need to explictly state that the user may (and
> > > > indeed, must) pass multiple settings as separate arguments. AFAICT
> > > > the parser here doesn't do the ,-splitting.
> > >
> > > I just noticed this is the case with network devices as well. E.g.
> > >
> > > #xl network-attach hvm-domU mac=00:16:3e:xx:yy:zz,bridge=br0
> > > libxl: error: libxl_device.c:1095:device_hotplug_child_death_cb:
> > > script: Could
> > > not find bridge device xenbr0
> > >
> > > main_networkattach() in tools/libxl/xl_cmdimpl.c doesn't split on the
> > > ',', so
> > > everything after mac=00:16:3e:xx:yy:zz is ignored. I'd need advice on
> > > how to fix
> > > this though. Based on xl-network-configuration doc and Xen tool's long
> > > history
> > > of network-attach supporting that syntax, I'd say main_networkattach()
> > > should be
> > > changed to split on ','. I could also change the docs. Do tools
> > > maintainers have
> > > a preference, or alternative option?
> > >
> >
> > It is splitting on " " at the moment which is not very desirable.
> >
> > I think this is a bug. I've added it to my list and will look into
> > fixing it.
>
> If you are feeling particularly highly motivated then xl_cmdimpl.c contains
> quite a lot of adhoc parsing of various comma a space separated lists
> (often dupped for a given instance into cmdline vs cfg file, which leads to
> unintentional behavioural differences like above) which could usefully be
> consolidated into a series of helpers.
>
Or rewrite every adhoc parser with bison/flex or Ocaml / Haskell parsec.
Jokes aside. I will see what I can do. Consolidating them into helper
functions is a good start.
Wei.
> Ian.
>
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH V13 5/5] xl: add pvusb commands
2016-01-21 12:29 ` Wei Liu
@ 2016-01-21 17:37 ` Ian Jackson
0 siblings, 0 replies; 42+ messages in thread
From: Ian Jackson @ 2016-01-21 17:37 UTC (permalink / raw)
To: Wei Liu
Cc: jgross, Ian Campbell, george.dunlap, Chunyan Liu, xen-devel,
Jim Fehlig, Simon Cao
Wei Liu writes ("Re: [Xen-devel] [PATCH V13 5/5] xl: add pvusb commands"):
> Or rewrite every adhoc parser with bison/flex or Ocaml / Haskell parsec.
I endorse this product and/or service.
> Jokes aside. I will see what I can do. Consolidating them into helper
> functions is a good start.
Yes :-).
Ian.
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH V13 3/5] libxl: add pvusb API
2016-01-19 8:39 ` [PATCH V13 3/5] libxl: add pvusb API Chunyan Liu
2016-01-19 15:48 ` Ian Jackson
@ 2016-01-26 16:12 ` Olaf Hering
2016-01-27 2:29 ` Chun Yan Liu
1 sibling, 1 reply; 42+ messages in thread
From: Olaf Hering @ 2016-01-26 16:12 UTC (permalink / raw)
To: Chunyan Liu
Cc: jgross, wei.liu2, ian.campbell, george.dunlap, Ian.Jackson,
George Dunlap, xen-devel, jfehlig, Simon Cao
On Tue, Jan 19, Chunyan Liu wrote:
> +++ b/tools/libxl/libxl.c
> @@ -3204,7 +3204,7 @@ void libxl__device_disk_local_initiate_detach(libxl__egc *egc,
> aodev->dev = device;
> aodev->callback = local_device_detach_cb;
> aodev->force = 0;
> - libxl__initiate_device_remove(egc, aodev);
> + libxl__initiate_device_generic_remove(egc, aodev);
> return;
> }
>
> @@ -4172,8 +4172,10 @@ out:
> * libxl_device_vkb_destroy
> * libxl_device_vfb_remove
> * libxl_device_vfb_destroy
> + * libxl_device_usbctrl_remove
> + * libxl_device_usbctrl_destroy
This should be moved down to DEFINE_DEVICE_REMOVE_CUSTOM.
> */
> -#define DEFINE_DEVICE_REMOVE(type, removedestroy, f) \
> +#define DEFINE_DEVICE_REMOVE_EXT(type, remtype, removedestroy, f) \
> int libxl_device_##type##_##removedestroy(libxl_ctx *ctx, \
> uint32_t domid, libxl_device_##type *type, \
> const libxl_asyncop_how *ao_how) \
> @@ -4193,13 +4195,19 @@ out:
> aodev->dev = device; \
> aodev->callback = device_addrm_aocomplete; \
> aodev->force = f; \
> - libxl__initiate_device_remove(egc, aodev); \
> + libxl__initiate_device_##remtype##_remove(egc, aodev); \
> \
> out: \
> - if (rc) return AO_CREATE_FAIL(rc); \
> + if (rc) return AO_CREATE_FAIL(rc); \
> return AO_INPROGRESS; \
> }
>
> +#define DEFINE_DEVICE_REMOVE(type, removedestroy, f) \
> + DEFINE_DEVICE_REMOVE_EXT(type, generic, removedestroy, f)
> +
> +#define DEFINE_DEVICE_REMOVE_CUSTOM(type, removedestroy, f) \
> + DEFINE_DEVICE_REMOVE_EXT(type, type, removedestroy, f)
> +
> /* Define all remove/destroy functions and undef the macro */
>
> /* disk */
If this is the way to move forward, please split this out into a
separate change which can be applied to staging independent of any pvusb
changes.
I think the patch needs also the #undef.
> @@ -4223,6 +4231,10 @@ DEFINE_DEVICE_REMOVE(vfb, destroy, 1)
> DEFINE_DEVICE_REMOVE(vtpm, remove, 0)
> DEFINE_DEVICE_REMOVE(vtpm, destroy, 1)
>
> +/* usbctrl */
> +DEFINE_DEVICE_REMOVE_CUSTOM(usbctrl, remove, 0)
> +DEFINE_DEVICE_REMOVE_CUSTOM(usbctrl, destroy, 1)
> +
> /* channel/console hotunplug is not implemented. There are 2 possibilities:
> * 1. add support for secondary consoles to xenconsoled
> * 2. dynamically add/remove qemu chardevs via qmp messages. */
A comment should mention both libxl_device_usbctrl_remove/destroy and
libxl__initiate_device_usbctrl_remove/destroy.
Olaf
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH V13 3/5] libxl: add pvusb API
2016-01-26 16:12 ` Olaf Hering
@ 2016-01-27 2:29 ` Chun Yan Liu
0 siblings, 0 replies; 42+ messages in thread
From: Chun Yan Liu @ 2016-01-27 2:29 UTC (permalink / raw)
To: Olaf Hering
Cc: Juergen Gross, wei.liu2, ian.campbell, george.dunlap,
Ian.Jackson, George Dunlap, xen-devel, Jim Fehlig, Simon Cao
>>> On 1/27/2016 at 12:12 AM, in message <20160126161253.GA9905@gmail.com>, Olaf
Hering <olaf@aepfle.de> wrote:
> On Tue, Jan 19, Chunyan Liu wrote:
>
> > +++ b/tools/libxl/libxl.c
> > @@ -3204,7 +3204,7 @@ void
> libxl__device_disk_local_initiate_detach(libxl__egc *egc,
> > aodev->dev = device;
> > aodev->callback = local_device_detach_cb;
> > aodev->force = 0;
> > - libxl__initiate_device_remove(egc, aodev);
> > + libxl__initiate_device_generic_remove(egc, aodev);
> > return;
> > }
> >
> > @@ -4172,8 +4172,10 @@ out:
> > * libxl_device_vkb_destroy
> > * libxl_device_vfb_remove
> > * libxl_device_vfb_destroy
> > + * libxl_device_usbctrl_remove
> > + * libxl_device_usbctrl_destroy
>
> This should be moved down to DEFINE_DEVICE_REMOVE_CUSTOM.
>
> > */
> > -#define DEFINE_DEVICE_REMOVE(type, removedestroy, f) \
> > +#define DEFINE_DEVICE_REMOVE_EXT(type, remtype, removedestroy, f) \
> > int libxl_device_##type##_##removedestroy(libxl_ctx *ctx, \
> > uint32_t domid, libxl_device_##type *type, \
> > const libxl_asyncop_how *ao_how) \
> > @@ -4193,13 +4195,19 @@ out:
> > aodev->dev = device; \
> > aodev->callback = device_addrm_aocomplete; \
> > aodev->force = f; \
> > - libxl__initiate_device_remove(egc, aodev); \
> > + libxl__initiate_device_##remtype##_remove(egc, aodev); \
> > \
> > out: \
> > - if (rc) return AO_CREATE_FAIL(rc);
> \
> > + if (rc) return AO_CREATE_FAIL(rc); \
> > return AO_INPROGRESS; \
> > }
> >
> > +#define DEFINE_DEVICE_REMOVE(type, removedestroy, f) \
> > + DEFINE_DEVICE_REMOVE_EXT(type, generic, removedestroy, f)
> > +
> > +#define DEFINE_DEVICE_REMOVE_CUSTOM(type, removedestroy, f) \
> > + DEFINE_DEVICE_REMOVE_EXT(type, type, removedestroy, f)
> > +
> > /* Define all remove/destroy functions and undef the macro */
> >
> > /* disk */
>
>
> If this is the way to move forward, please split this out into a
> separate change which can be applied to staging independent of any pvusb
> changes.
>
> I think the patch needs also the #undef.
>
Got you. Will update.
>
> > @@ -4223,6 +4231,10 @@ DEFINE_DEVICE_REMOVE(vfb, destroy, 1)
> > DEFINE_DEVICE_REMOVE(vtpm, remove, 0)
> > DEFINE_DEVICE_REMOVE(vtpm, destroy, 1)
> >
> > +/* usbctrl */
> > +DEFINE_DEVICE_REMOVE_CUSTOM(usbctrl, remove, 0)
> > +DEFINE_DEVICE_REMOVE_CUSTOM(usbctrl, destroy, 1)
> > +
> > /* channel/console hotunplug is not implemented. There are 2
> possibilities:
> > * 1. add support for secondary consoles to xenconsoled
> > * 2. dynamically add/remove qemu chardevs via qmp messages. */
>
> A comment should mention both libxl_device_usbctrl_remove/destroy and
> libxl__initiate_device_usbctrl_remove/destroy.
Will add comments.
Thanks,
Chunyan
>
> Olaf
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>
>
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH V13 3/5] libxl: add pvusb API
2016-01-19 15:48 ` Ian Jackson
@ 2016-02-02 16:26 ` George Dunlap
2016-02-02 18:11 ` Ian Jackson
2016-02-02 16:48 ` George Dunlap
1 sibling, 1 reply; 42+ messages in thread
From: George Dunlap @ 2016-02-02 16:26 UTC (permalink / raw)
To: Ian Jackson
Cc: Jürgen Groß,
Wei Liu, Ian Campbell, George Dunlap, xen-devel, Jim Fehlig,
Simon Cao, Chunyan Liu
On Tue, Jan 19, 2016 at 3:48 PM, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:
> Chunyan Liu writes ("[PATCH V13 3/5] libxl: add pvusb API"):
>> +static int usbback_dev_unassign(libxl__gc *gc, const char *busid)
>> +{
> ...
>> + /* Till here, USB device has been unbound from USBBACK and
>> + * removed from xenstore, usb list couldn't show it anymore,
>> + * so no matter removimg driver path successfully or not,
>> + * we will report operation success.
>> + */
>
> I'm still unconvinced by this and this may mean that the code in this
> function is in the wrong order. Earlier we had this exchange:
>
>> > Ought this function to really report success if these calls fail ?
>>
>> I think so. Till here, the USB device has already been unbound from
>> usbback and removed from xenstore. usb-list cannot list it any more.
>
> The problem is that I think that if this function fails, it can leave
> - debris in xenstore (the usbback path)
> - the interface bound to the wrong driver
> And then there is no way for the user to get libxl to re-attempt the
> operation, or clean up. Am I right ?
>
> One way to avoid this kind of problem is to deal with the xenstore
> path last. That way the device will still appear as attached to the
> domain.
>
> To do this properly I think bind_usbintf may need to become idempotent
> even in the face of other callers racing to assign the device. I
> think that would mean bind_usbif it would have to know what driver to
> expect to find the device bound to.
>
> George, am I right here ?
There are effectively four states a device can be in, from the
'assignment' point of view:
1. Assigned to the normal Linux device driver(s) for the USB device
2. Assigned to no driver
3. Assigned to usbback, but not yet assigned to any guest
4. Assigned to a guest
Regardless of the state, the USB device is visible to lsusb, and
always exists in the usb controller's device heirarchy, regardless of
what driver it's assigned to (if any).
As far as sysfs and lspci goes, pci is an exact analog.
For usb, libxl really only has two states: assigned (state 4) and
unassigned (states 1 or 2). libxl__device_usbdev_add() will take
devices from states 1 or 2 and move them into state 3, then state 4.
libxl__device_usbdev_remove() will take devices from state 4, through
state 3, and then to state 2 (and then possibly state 1, if it has the
appropriate information). usbdev_list will only list things in state
4.
Additionally, each USB "device" has one or more "interfaces". To
assign a "device" to usbback in the sysfs case means assigning all of
the "interfaces". The code seems to assume that different interfaces
from the same device can be assigned to different drivers.
For comparison, the libxl pci pass-through code has three states: not
assignable (states 1 or 2), assignable (state 3), and assigned (state
4). libxl__device_pci_assignable_add moves it from 1 or 2 to state 3;
libxl_device_pci_add moves it from state 3 to 4 (unless the "seize"
flag is set, in which case it will call
libxl__device_pci_assignable_add if necessary).
libxl_device_pci_assignable_list lists things in state 3;
libxl_device_pci_list list things in state 4.
pci devices also have analogs called "functions"; but libxl treats
each of these separately (i.e., you actually assign a "function" to a
VM, not a "device"). If a device has several functions which all need
to be assigned, it's up to the user to assign each one individually.
So for removal, we have to consider what to do with a failure at each
of the steps.
* 4 -> 3. This is just removing it from xenstore. The xenstore
removal is transactional, so (I think) a failure here should mean that
it's still in state 4; it should still show up in usb_list, and the
user could try removing again if she wants.
* 3 -> 2. At this point, what the code actually does is try to unbind
each interface from usbback; if any of the calls to unbind_usbintf()
fail, it will give up (leaving any remaining functions bound to
usbback).
* 2 -> 1. If we stored a driver path in xenstore (under the /libxl
node, not under the pvusb node), we then try to rebind each interface
to that driver. If this binding fails, we simply print a warning and
continue (i.e., we attempt to re-bind with all interfaces, even if one
of them fails).
Regarding Ian's comments:
Since "assigned to the guest" and "listed under the pvusb xenstore
node" are the same thing, is it even *possible* to (safely) unassign
the device from usbback before removing the xenstore nodes?
It's not clear to me under what conditions 3->2 might fail, or what
could be done about it if it did fail. Perhaps removing the usbback
module? Failing that, rebooting the system is the only recovery
option I can think of. Certainly trying to assign it back to the
guest (i.e., move it back to the previous legal state of '4') isn't a
good idea.
I do agree, however, that stopping in the middle here (leaving some
interfaces assigned to usbback and some potentially already
re-assigned to the original driver) is a bad; as is leaving the original
driver for the interface in xenstore.
Regarding 2->1, again it's not clear that there's anything libxl can
do. Reloading the driver's module might reset it enough to pick up
the (now unassigned) USB devices again; other than that, rebooting is
probably the best option.
It's also not clear to me, if some functions succeed in being
reassigned and others fail, whether it's best to just try to assign as
many as we can, or better to go back and un-assign all the ones that
succeeded.
Perhaps the best approach code-wise is to change the "goto out" on
failure of unbind_usbintf() into a "continue". That way:
1. All interfaces which can be re-assigned are re-assigned (and work
as much as possible)
2. All interfaces which can be unbound but not re-assigned are at
least unbound (so that reloading the original driver might pick them
up)
3. The xenstore paths storing the original driver information are
cleaned up.
And we should of course include some useful error messages to give the
user a hint as to what they can do to fix things; perhaps:
"ERROR: Unexpected failure, device now in wedged state (partially
assigned to usbback), don't know how to recover. Try unloading and
reloading the usbback module, or rebooting the system."
"ERROR: Unexpected failure re-assigning device, don't know how to
recover. Try unloading and reloading $MODULE, or rebooting the
system."
Thoughts?
-George
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH V13 3/5] libxl: add pvusb API
2016-01-19 15:48 ` Ian Jackson
2016-02-02 16:26 ` George Dunlap
@ 2016-02-02 16:48 ` George Dunlap
2016-02-03 8:25 ` Chun Yan Liu
1 sibling, 1 reply; 42+ messages in thread
From: George Dunlap @ 2016-02-02 16:48 UTC (permalink / raw)
To: Ian Jackson
Cc: Jürgen Groß,
Wei Liu, Ian Campbell, George Dunlap, xen-devel, Jim Fehlig,
Simon Cao, Chunyan Liu
On Tue, Jan 19, 2016 at 3:48 PM, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:
> Chunyan Liu writes ("[PATCH V13 3/5] libxl: add pvusb API"):
>> > Ought this function to really report success if these calls fail ?
>>
>> I think so. Till here, the USB device has already been unbound from
>> usbback and removed from xenstore. usb-list cannot list it any more.
Also, I think we probably should return some sort of error, so that
scripts &c can at least know something out of the ordinary has
happened, even if it doesn't go back to the state we started in when
the function was called.
-George
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH V13 3/5] libxl: add pvusb API
2016-02-02 16:26 ` George Dunlap
@ 2016-02-02 18:11 ` Ian Jackson
2016-02-03 7:34 ` Chun Yan Liu
2016-02-03 14:33 ` George Dunlap
0 siblings, 2 replies; 42+ messages in thread
From: Ian Jackson @ 2016-02-02 18:11 UTC (permalink / raw)
To: George Dunlap
Cc: Juergen Gross, Wei Liu, Ian Campbell, George Dunlap, xen-devel,
Jim Fehlig, Simon Cao, Chunyan Liu
George Dunlap writes ("Re: [Xen-devel] [PATCH V13 3/5] libxl: add pvusb API"):
> There are effectively four states a device can be in, from the
> 'assignment' point of view:
>
> 1. Assigned to the normal Linux device driver(s) for the USB device
>
> 2. Assigned to no driver
>
> 3. Assigned to usbback, but not yet assigned to any guest
>
> 4. Assigned to a guest
Thanks for your clear explanation (of which I will snip much).
> Additionally, each USB "device" has one or more "interfaces". To
> assign a "device" to usbback in the sysfs case means assigning all of
> the "interfaces". The code seems to assume that different interfaces
> from the same device can be assigned to different drivers.
It is indeed the case that in principle a single USB device with
multiple interfaces can be assigned to multiple different drivers.
> Regarding Ian's comments:
>
> Since "assigned to the guest" and "listed under the pvusb xenstore
> node" are the same thing, is it even *possible* to (safely) unassign
> the device from usbback before removing the xenstore nodes?
It might be possible to remove some of the xenstore nodes but leave
others present, so that usbback detaches, but enough information
remains for libxl to know that Xen still `owns' the device.
But, surely usbback needs to cope with the notion that the device
might disappear. USB devices can disappear at any time.
> It's not clear to me under what conditions 3->2 might fail,
As an example, someone might press ^C on `xl usb-detach'.
> or what could be done about it if it did fail.
The most obvious reason for it failing is that something somewhere
still held onto the device. (For umounting filesystems, and detaching
block devices, this happens a lot.) So if the detach from usbback
fails would probably be possible to simply retry it.
> Regarding 2->1, again it's not clear that there's anything libxl can
> do. Reloading the driver's module might reset it enough to pick up
> the (now unassigned) USB devices again; other than that, rebooting is
> probably the best option.
I think re-attempting the bind may work. USB devices can be flaky.
> It's also not clear to me, if some functions succeed in being
> reassigned and others fail, whether it's best to just try to assign as
> many as we can, or better to go back and un-assign all the ones that
> succeeded.
Unless explicitly requested, I don't think the system should create
situations some interfaces are assigned to host drivers and some to
usbback.
And, I'm a fan of `crash-only software': in general, if a failure
occurs, the situation should just be left as-is. The intermediate
state needs to be visible and rectifiable.
This approach to software state machines avoids bugs where the system
gets into `impossible' states, recovery from which is impossible
because the designers didn't anticipate them.
It would be tolerable if the recovery sometimes involves `lsusb' and
echoing things into sysfs, but it would be better if not.
> Perhaps the best approach code-wise is to change the "goto out" on
> failure of unbind_usbintf() into a "continue". That way:
>
> 1. All interfaces which can be re-assigned are re-assigned (and work
> as much as possible)
>
> 2. All interfaces which can be unbound but not re-assigned are at
> least unbound (so that reloading the original driver might pick them
> up)
I certainly don't mind the software trying to do as much of its task
as possible.
Ian.
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH V13 3/5] libxl: add pvusb API
2016-02-02 18:11 ` Ian Jackson
@ 2016-02-03 7:34 ` Chun Yan Liu
2016-02-03 14:38 ` George Dunlap
2016-02-03 14:33 ` George Dunlap
1 sibling, 1 reply; 42+ messages in thread
From: Chun Yan Liu @ 2016-02-03 7:34 UTC (permalink / raw)
To: George Dunlap, Ian Jackson
Cc: Juergen Gross, Wei Liu, Ian Campbell, George Dunlap, xen-devel,
Jim Fehlig, Simon Cao
>>> On 2/3/2016 at 02:11 AM, in message
<22192.61775.427189.268007@mariner.uk.xensource.com>, Ian Jackson
<Ian.Jackson@eu.citrix.com> wrote:
> George Dunlap writes ("Re: [Xen-devel] [PATCH V13 3/5] libxl: add pvusb API"):
> > There are effectively four states a device can be in, from the
> > 'assignment' point of view:
> >
> > 1. Assigned to the normal Linux device driver(s) for the USB device
> >
> > 2. Assigned to no driver
> >
> > 3. Assigned to usbback, but not yet assigned to any guest
> >
> > 4. Assigned to a guest
>
> Thanks for your clear explanation (of which I will snip much).
>
> > Additionally, each USB "device" has one or more "interfaces". To
> > assign a "device" to usbback in the sysfs case means assigning all of
> > the "interfaces". The code seems to assume that different interfaces
> > from the same device can be assigned to different drivers.
>
> It is indeed the case that in principle a single USB device with
> multiple interfaces can be assigned to multiple different drivers.
>
> > Regarding Ian's comments:
> >
> > Since "assigned to the guest" and "listed under the pvusb xenstore
> > node" are the same thing, is it even *possible* to (safely) unassign
> > the device from usbback before removing the xenstore nodes?
>
> It might be possible to remove some of the xenstore nodes but leave
> others present, so that usbback detaches, but enough information
> remains for libxl to know that Xen still `owns' the device.
Indeed "unassign from usbback, but listed under pvusb xenstore" is
a confusing state. usb-list can list it but guest can not see it.
What user can do under that state is: reattempt usbdev_remove, if it
succeeds, everything is cleaned up, that's the best result; but
possibly it still fails (for example, in my testing, always cannot
rebind to original driver), in this case, the confusing state will
be lasting, and the device could not be removed, this is worse.
>
> But, surely usbback needs to cope with the notion that the device
> might disappear. USB devices can disappear at any time.
>
> > It's not clear to me under what conditions 3->2 might fail,
>
> As an example, someone might press ^C on `xl usb-detach'.
>
> > or what could be done about it if it did fail.
>
> The most obvious reason for it failing is that something somewhere
> still held onto the device. (For umounting filesystems, and detaching
> block devices, this happens a lot.) So if the detach from usbback
> fails would probably be possible to simply retry it.
>
> > Regarding 2->1, again it's not clear that there's anything libxl can
> > do. Reloading the driver's module might reset it enough to pick up
> > the (now unassigned) USB devices again; other than that, rebooting is
> > probably the best option.
>
> I think re-attempting the bind may work. USB devices can be flaky.
>
> > It's also not clear to me, if some functions succeed in being
> > reassigned and others fail, whether it's best to just try to assign as
> > many as we can, or better to go back and un-assign all the ones that
> > succeeded.
>
> Unless explicitly requested, I don't think the system should create
> situations some interfaces are assigned to host drivers and some to
> usbback.
>
> And, I'm a fan of `crash-only software': in general, if a failure
> occurs, the situation should just be left as-is. The intermediate
> state needs to be visible and rectifiable.
>
> This approach to software state machines avoids bugs where the system
> gets into `impossible' states, recovery from which is impossible
> because the designers didn't anticipate them.
>
> It would be tolerable if the recovery sometimes involves `lsusb' and
> echoing things into sysfs, but it would be better if not.
>
> > Perhaps the best approach code-wise is to change the "goto out" on
> > failure of unbind_usbintf() into a "continue". That way:
> >
> > 1. All interfaces which can be re-assigned are re-assigned (and work
> > as much as possible)
> >
> > 2. All interfaces which can be unbound but not re-assigned are at
> > least unbound (so that reloading the original driver might pick them
> > up)
>
> I certainly don't mind the software trying to do as much of its task
> as possible.
Could I understand that this way is acceptable? That means: removing
xenstore, and as much as we could (on failure of "unbind from usbback"
or "bind to original driver", don't "goto out", just "continue").
Chunyan
>
> Ian.
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>
>
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH V13 3/5] libxl: add pvusb API
2016-02-02 16:48 ` George Dunlap
@ 2016-02-03 8:25 ` Chun Yan Liu
0 siblings, 0 replies; 42+ messages in thread
From: Chun Yan Liu @ 2016-02-03 8:25 UTC (permalink / raw)
To: George Dunlap, Ian Jackson
Cc: Juergen Gross, Wei Liu, Ian Campbell, George Dunlap, xen-devel,
Jim Fehlig, Simon Cao
>>> On 2/3/2016 at 12:48 AM, in message
<CAFLBxZY38HfouhD4eS63+HieQ8dfsza=rY7r_CNGhknmVLowJg@mail.gmail.com>, George
Dunlap <George.Dunlap@eu.citrix.com> wrote:
> On Tue, Jan 19, 2016 at 3:48 PM, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:
> > Chunyan Liu writes ("[PATCH V13 3/5] libxl: add pvusb API"):
> >> > Ought this function to really report success if these calls fail ?
> >>
> >> I think so. Till here, the USB device has already been unbound from
> >> usbback and removed from xenstore. usb-list cannot list it any more.
>
> Also, I think we probably should return some sort of error, so that
> scripts &c can at least know something out of the ordinary has
> happened, even if it doesn't go back to the state we started in when
> the function was called.
That makes sense. But do we need to differentiate failure at removing
xenstore information step and error in bind/unbind step? Or, treat
them as same? That affects doing it within other process (like
usbctrl-detach). For the former, xl usb-list can list usb device and
one can do usbdev-detach again; and if in a usbctrl-detach process,
will stop the process. For the later, one cannot do usbdev-detach,
and if in usbctrl-detach process, in current codes, will ignore that.
Chunyan
>
> -George
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>
>
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH V13 3/5] libxl: add pvusb API
2016-02-02 18:11 ` Ian Jackson
2016-02-03 7:34 ` Chun Yan Liu
@ 2016-02-03 14:33 ` George Dunlap
2016-02-04 1:53 ` Chun Yan Liu
[not found] ` <56B31FAB02000066000A6596@suse.com>
1 sibling, 2 replies; 42+ messages in thread
From: George Dunlap @ 2016-02-03 14:33 UTC (permalink / raw)
To: Ian Jackson, George Dunlap
Cc: Juergen Gross, Wei Liu, Ian Campbell, Chunyan Liu, xen-devel,
Jim Fehlig, Simon Cao
On 02/02/16 18:11, Ian Jackson wrote:
> George Dunlap writes ("Re: [Xen-devel] [PATCH V13 3/5] libxl: add pvusb API"):
>> There are effectively four states a device can be in, from the
>> 'assignment' point of view:
>>
>> 1. Assigned to the normal Linux device driver(s) for the USB device
>>
>> 2. Assigned to no driver
>>
>> 3. Assigned to usbback, but not yet assigned to any guest
>>
>> 4. Assigned to a guest
>
> Thanks for your clear explanation (of which I will snip much).
>
>> Additionally, each USB "device" has one or more "interfaces". To
>> assign a "device" to usbback in the sysfs case means assigning all of
>> the "interfaces". The code seems to assume that different interfaces
>> from the same device can be assigned to different drivers.
>
> It is indeed the case that in principle a single USB device with
> multiple interfaces can be assigned to multiple different drivers.
>
>> Regarding Ian's comments:
>>
>> Since "assigned to the guest" and "listed under the pvusb xenstore
>> node" are the same thing, is it even *possible* to (safely) unassign
>> the device from usbback before removing the xenstore nodes?
>
> It might be possible to remove some of the xenstore nodes but leave
> others present, so that usbback detaches, but enough information
> remains for libxl to know that Xen still `owns' the device.
>
> But, surely usbback needs to cope with the notion that the device
> might disappear. USB devices can disappear at any time.
That's true. But if the USB device has actually disappeared, the device
is in fact "safe" from usbback. I think I might consider state 2 safe
to go to, but I definitely wouldn't consider state 1 safe with the
xenstore nodes still in place.
>
>> It's not clear to me under what conditions 3->2 might fail,
>
> As an example, someone might press ^C on `xl usb-detach'.
*grumble*
>> or what could be done about it if it did fail.
>
> The most obvious reason for it failing is that something somewhere
> still held onto the device. (For umounting filesystems, and detaching
> block devices, this happens a lot.) So if the detach from usbback
> fails would probably be possible to simply retry it.
I'm not sure what this means in the context of moving from 3 (assigned
to usbback) to 2 (unassigned). usbback will definitely not use the
device to mount something in dom0; and I'm pretty sure has no visibility
as to whether it's being used as a filesystem in the domU.
(Moving from 1 -> 2 of course would be subject to this sort of thing,
but that's a different issue.)
>> Regarding 2->1, again it's not clear that there's anything libxl can
>> do. Reloading the driver's module might reset it enough to pick up
>> the (now unassigned) USB devices again; other than that, rebooting is
>> probably the best option.
>
> I think re-attempting the bind may work. USB devices can be flaky.
>
>> It's also not clear to me, if some functions succeed in being
>> reassigned and others fail, whether it's best to just try to assign as
>> many as we can, or better to go back and un-assign all the ones that
>> succeeded.
>
> Unless explicitly requested, I don't think the system should create
> situations some interfaces are assigned to host drivers and some to
> usbback.
I was talking here about whether it would be better to have some
interfaces assigned to the original drivers and some interfaces left
unassigned, or to try to leave all interfaces unassigned if any of the
assignments fail.
I agree, it would be better to try to avoid the possibility of having
some interfaces bound to usbback and some interfaces bound to the
original drivers.
> And, I'm a fan of `crash-only software': in general, if a failure
> occurs, the situation should just be left as-is. The intermediate
> state needs to be visible and rectifiable.
>
> This approach to software state machines avoids bugs where the system
> gets into `impossible' states, recovery from which is impossible
> because the designers didn't anticipate them.
>
> It would be tolerable if the recovery sometimes involves `lsusb' and
> echoing things into sysfs, but it would be better if not.
Right -- so what about this. When removing a USB device:
* First modify the pvusb xenstore nodes such that 1) it's safe to
attempt removing the interfaces from usbback, but 2) they still show up
in usb-list. (This may be a noop.)
* Attempt to remove all interfaces from usbback; if any of these fail,
stop and report an error. Possible recovery options:
- Re-try the usb_remove command
- Re-load usbback (obviously disruptive to other VMs)
- Reboot the host
- Manually try unbinding with sysfs
* Remove the remaining pvusb xenstore nodes. If this fails, stop and
report the error. Possible recovery options:
- Re-try the usb_remove command
= REBIND OPTION 1: Do the best we can without extra commands
* Attempt to re-bind the interfaces to the original drivers, as recorded
in the libxl usb xenstore nodes. If any of these fail, report an error
but continue to try the rest of the interfaces, and remove the xenstore
nodes containing information about the original drivers. Possible
recovery options for the user:
- Reload the original drivers
- Reboot the host
- Manually try rebinding with sysfs
= REBIND OPTION 2: Include a recovery command, usb-retry-rebind
* Attempt to re-bind the interfaces to the original drivers, as recorded
in the libxl usb xenstore nodes. If any of these fail, stop immediately
and report an error; do not remove the xenstore nodes containing the
original drivers of any interfaces that failed to rebind. Possible
recovery options for the user:
- Run 'xl usbdev-retry-rebind', which will just execute this step again
- Unload and reload original host drivers
- Reboot the host
Both of these:
1. Will avoid the state of some interfaces bound to usbback, some
rebound to the original drivers
2. Give the user a convenient way to re-try unbinding from usbback it failed
3. Give the user out-of-xl ways to attempt to recover the state other
than messing around with sysfs.
Rebind option 2 will give the user a convenient way to retry rebinding
to the original driver via xl if that step failed.
I'm inclined to suggest rebind option #1 for now, just to keep things
simple.
Thoughts?
Chunyan, would the first half of that (removing from usbback before
removing the pvusb xenstore nodes) actually work?
-George
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH V13 3/5] libxl: add pvusb API
2016-02-03 7:34 ` Chun Yan Liu
@ 2016-02-03 14:38 ` George Dunlap
2016-02-04 1:44 ` Chun Yan Liu
0 siblings, 1 reply; 42+ messages in thread
From: George Dunlap @ 2016-02-03 14:38 UTC (permalink / raw)
To: Chun Yan Liu, George Dunlap, Ian Jackson
Cc: Juergen Gross, Wei Liu, Ian Campbell, xen-devel, Jim Fehlig, Simon Cao
On 03/02/16 07:34, Chun Yan Liu wrote:
>
>
>>>> On 2/3/2016 at 02:11 AM, in message
> <22192.61775.427189.268007@mariner.uk.xensource.com>, Ian Jackson
> <Ian.Jackson@eu.citrix.com> wrote:
>> George Dunlap writes ("Re: [Xen-devel] [PATCH V13 3/5] libxl: add pvusb API"):
>>> There are effectively four states a device can be in, from the
>>> 'assignment' point of view:
>>>
>>> 1. Assigned to the normal Linux device driver(s) for the USB device
>>>
>>> 2. Assigned to no driver
>>>
>>> 3. Assigned to usbback, but not yet assigned to any guest
>>>
>>> 4. Assigned to a guest
>>
>> Thanks for your clear explanation (of which I will snip much).
>>
>>> Additionally, each USB "device" has one or more "interfaces". To
>>> assign a "device" to usbback in the sysfs case means assigning all of
>>> the "interfaces". The code seems to assume that different interfaces
>>> from the same device can be assigned to different drivers.
>>
>> It is indeed the case that in principle a single USB device with
>> multiple interfaces can be assigned to multiple different drivers.
>>
>>> Regarding Ian's comments:
>>>
>>> Since "assigned to the guest" and "listed under the pvusb xenstore
>>> node" are the same thing, is it even *possible* to (safely) unassign
>>> the device from usbback before removing the xenstore nodes?
>>
>> It might be possible to remove some of the xenstore nodes but leave
>> others present, so that usbback detaches, but enough information
>> remains for libxl to know that Xen still `owns' the device.
>
> Indeed "unassign from usbback, but listed under pvusb xenstore" is
> a confusing state. usb-list can list it but guest can not see it.
> What user can do under that state is: reattempt usbdev_remove, if it
> succeeds, everything is cleaned up, that's the best result; but
> possibly it still fails (for example, in my testing, always cannot
> rebind to original driver), in this case, the confusing state will
> be lasting, and the device could not be removed, this is worse.
As I said in my other mail, I think removing the pvusb nodes should be
done once it's successfully un-bound from usbback, *even if* the re-bind
to the original driver fails. (That is, once it reaches state 2,
usb-list should no longer list it.)
>>> Perhaps the best approach code-wise is to change the "goto out" on
>>> failure of unbind_usbintf() into a "continue". That way:
>>>
>>> 1. All interfaces which can be re-assigned are re-assigned (and work
>>> as much as possible)
>>>
>>> 2. All interfaces which can be unbound but not re-assigned are at
>>> least unbound (so that reloading the original driver might pick them
>>> up)
>>
>> I certainly don't mind the software trying to do as much of its task
>> as possible.
>
> Could I understand that this way is acceptable? That means: removing
> xenstore, and as much as we could (on failure of "unbind from usbback"
> or "bind to original driver", don't "goto out", just "continue").
I think part of it depends on what is *possible*. If it's possible to
safely unbind the device from usbback while retaining its place in the
pvusb-related xenstore nodes, then I think we should (so that the user
can re-try removing it). If it's not possible, then of course we have
to remove the pvusb xenstore nodes first, and then we'll just have to
deal as gracefully as possible with failure unbinding from usbback.
-George
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH V13 3/5] libxl: add pvusb API
2016-02-03 14:38 ` George Dunlap
@ 2016-02-04 1:44 ` Chun Yan Liu
0 siblings, 0 replies; 42+ messages in thread
From: Chun Yan Liu @ 2016-02-04 1:44 UTC (permalink / raw)
To: George Dunlap, Ian Jackson
Cc: Juergen Gross, Wei Liu, Ian Campbell, xen-devel, Jim Fehlig, Simon Cao
>>> On 2/3/2016 at 10:38 PM, in message <56B210FA.7040803@citrix.com>, George
Dunlap <george.dunlap@citrix.com> wrote:
> On 03/02/16 07:34, Chun Yan Liu wrote:
> >
> >
> >>>> On 2/3/2016 at 02:11 AM, in message
> > <22192.61775.427189.268007@mariner.uk.xensource.com>, Ian Jackson
> > <Ian.Jackson@eu.citrix.com> wrote:
> >> George Dunlap writes ("Re: [Xen-devel] [PATCH V13 3/5] libxl: add pvusb
> API"):
> >>> There are effectively four states a device can be in, from the
> >>> 'assignment' point of view:
> >>>
> >>> 1. Assigned to the normal Linux device driver(s) for the USB device
> >>>
> >>> 2. Assigned to no driver
> >>>
> >>> 3. Assigned to usbback, but not yet assigned to any guest
> >>>
> >>> 4. Assigned to a guest
> >>
> >> Thanks for your clear explanation (of which I will snip much).
> >>
> >>> Additionally, each USB "device" has one or more "interfaces". To
> >>> assign a "device" to usbback in the sysfs case means assigning all of
> >>> the "interfaces". The code seems to assume that different interfaces
> >>> from the same device can be assigned to different drivers.
> >>
> >> It is indeed the case that in principle a single USB device with
> >> multiple interfaces can be assigned to multiple different drivers.
> >>
> >>> Regarding Ian's comments:
> >>>
> >>> Since "assigned to the guest" and "listed under the pvusb xenstore
> >>> node" are the same thing, is it even *possible* to (safely) unassign
> >>> the device from usbback before removing the xenstore nodes?
> >>
> >> It might be possible to remove some of the xenstore nodes but leave
> >> others present, so that usbback detaches, but enough information
> >> remains for libxl to know that Xen still `owns' the device.
> >
> > Indeed "unassign from usbback, but listed under pvusb xenstore" is
> > a confusing state. usb-list can list it but guest can not see it.
> > What user can do under that state is: reattempt usbdev_remove, if it
> > succeeds, everything is cleaned up, that's the best result; but
> > possibly it still fails (for example, in my testing, always cannot
> > rebind to original driver), in this case, the confusing state will
> > be lasting, and the device could not be removed, this is worse.
>
> As I said in my other mail, I think removing the pvusb nodes should be
> done once it's successfully un-bound from usbback, *even if* the re-bind
> to the original driver fails. (That is, once it reaches state 2,
> usb-list should no longer list it.)
>
> >>> Perhaps the best approach code-wise is to change the "goto out" on
> >>> failure of unbind_usbintf() into a "continue". That way:
> >>>
> >>> 1. All interfaces which can be re-assigned are re-assigned (and work
> >>> as much as possible)
> >>>
> >>> 2. All interfaces which can be unbound but not re-assigned are at
> >>> least unbound (so that reloading the original driver might pick them
> >>> up)
> >>
> >> I certainly don't mind the software trying to do as much of its task
> >> as possible.
> >
> > Could I understand that this way is acceptable? That means: removing
> > xenstore, and as much as we could (on failure of "unbind from usbback"
> > or "bind to original driver", don't "goto out", just "continue").
>
> I think part of it depends on what is *possible*. If it's possible to
> safely unbind the device from usbback while retaining its place in the
> pvusb-related xenstore nodes, then I think we should (so that the user
Juergen, I think you are the person who can answer the question best.
Can you confirm that?
-Chunyan
> can re-try removing it). If it's not possible, then of course we have
> to remove the pvusb xenstore nodes first, and then we'll just have to
> deal as gracefully as possible with failure unbinding from usbback.
>
> -George
>
>
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH V13 3/5] libxl: add pvusb API
2016-02-03 14:33 ` George Dunlap
@ 2016-02-04 1:53 ` Chun Yan Liu
[not found] ` <56B31FAB02000066000A6596@suse.com>
1 sibling, 0 replies; 42+ messages in thread
From: Chun Yan Liu @ 2016-02-04 1:53 UTC (permalink / raw)
To: George Dunlap, George Dunlap, Ian Jackson
Cc: Juergen Gross, Wei Liu, Ian Campbell, xen-devel, Jim Fehlig, Simon Cao
>>> On 2/3/2016 at 10:33 PM, in message <56B20FCC.3010308@citrix.com>, George
Dunlap <george.dunlap@citrix.com> wrote:
> On 02/02/16 18:11, Ian Jackson wrote:
> > George Dunlap writes ("Re: [Xen-devel] [PATCH V13 3/5] libxl: add pvusb
> API"):
> >> There are effectively four states a device can be in, from the
> >> 'assignment' point of view:
> >>
> >> 1. Assigned to the normal Linux device driver(s) for the USB device
> >>
> >> 2. Assigned to no driver
> >>
> >> 3. Assigned to usbback, but not yet assigned to any guest
> >>
> >> 4. Assigned to a guest
> >
> > Thanks for your clear explanation (of which I will snip much).
> >
> >> Additionally, each USB "device" has one or more "interfaces". To
> >> assign a "device" to usbback in the sysfs case means assigning all of
> >> the "interfaces". The code seems to assume that different interfaces
> >> from the same device can be assigned to different drivers.
> >
> > It is indeed the case that in principle a single USB device with
> > multiple interfaces can be assigned to multiple different drivers.
> >
> >> Regarding Ian's comments:
> >>
> >> Since "assigned to the guest" and "listed under the pvusb xenstore
> >> node" are the same thing, is it even *possible* to (safely) unassign
> >> the device from usbback before removing the xenstore nodes?
> >
> > It might be possible to remove some of the xenstore nodes but leave
> > others present, so that usbback detaches, but enough information
> > remains for libxl to know that Xen still `owns' the device.
> >
> > But, surely usbback needs to cope with the notion that the device
> > might disappear. USB devices can disappear at any time.
>
> That's true. But if the USB device has actually disappeared, the device
> is in fact "safe" from usbback. I think I might consider state 2 safe
> to go to, but I definitely wouldn't consider state 1 safe with the
> xenstore nodes still in place.
>
> >
> >> It's not clear to me under what conditions 3->2 might fail,
> >
> > As an example, someone might press ^C on `xl usb-detach'.
>
> *grumble*
>
> >> or what could be done about it if it did fail.
> >
> > The most obvious reason for it failing is that something somewhere
> > still held onto the device. (For umounting filesystems, and detaching
> > block devices, this happens a lot.) So if the detach from usbback
> > fails would probably be possible to simply retry it.
>
> I'm not sure what this means in the context of moving from 3 (assigned
> to usbback) to 2 (unassigned). usbback will definitely not use the
> device to mount something in dom0; and I'm pretty sure has no visibility
> as to whether it's being used as a filesystem in the domU.
>
> (Moving from 1 -> 2 of course would be subject to this sort of thing,
> but that's a different issue.)
>
> >> Regarding 2->1, again it's not clear that there's anything libxl can
> >> do. Reloading the driver's module might reset it enough to pick up
> >> the (now unassigned) USB devices again; other than that, rebooting is
> >> probably the best option.
> >
> > I think re-attempting the bind may work. USB devices can be flaky.
> >
> >> It's also not clear to me, if some functions succeed in being
> >> reassigned and others fail, whether it's best to just try to assign as
> >> many as we can, or better to go back and un-assign all the ones that
> >> succeeded.
> >
> > Unless explicitly requested, I don't think the system should create
> > situations some interfaces are assigned to host drivers and some to
> > usbback.
>
> I was talking here about whether it would be better to have some
> interfaces assigned to the original drivers and some interfaces left
> unassigned, or to try to leave all interfaces unassigned if any of the
> assignments fail.
>
> I agree, it would be better to try to avoid the possibility of having
> some interfaces bound to usbback and some interfaces bound to the
> original drivers.
>
> > And, I'm a fan of `crash-only software': in general, if a failure
> > occurs, the situation should just be left as-is. The intermediate
> > state needs to be visible and rectifiable.
> >
> > This approach to software state machines avoids bugs where the system
> > gets into `impossible' states, recovery from which is impossible
> > because the designers didn't anticipate them.
> >
> > It would be tolerable if the recovery sometimes involves `lsusb' and
> > echoing things into sysfs, but it would be better if not.
>
> Right -- so what about this. When removing a USB device:
>
> * First modify the pvusb xenstore nodes such that 1) it's safe to
> attempt removing the interfaces from usbback, but 2) they still show up
> in usb-list. (This may be a noop.)
>
> * Attempt to remove all interfaces from usbback; if any of these fail,
> stop and report an error. Possible recovery options:
> - Re-try the usb_remove command
> - Re-load usbback (obviously disruptive to other VMs)
> - Reboot the host
> - Manually try unbinding with sysfs
>
> * Remove the remaining pvusb xenstore nodes. If this fails, stop and
> report the error. Possible recovery options:
> - Re-try the usb_remove command
>
> = REBIND OPTION 1: Do the best we can without extra commands
>
> * Attempt to re-bind the interfaces to the original drivers, as recorded
> in the libxl usb xenstore nodes. If any of these fail, report an error
> but continue to try the rest of the interfaces, and remove the xenstore
> nodes containing information about the original drivers. Possible
> recovery options for the user:
> - Reload the original drivers
> - Reboot the host
> - Manually try rebinding with sysfs
>
> = REBIND OPTION 2: Include a recovery command, usb-retry-rebind
>
> * Attempt to re-bind the interfaces to the original drivers, as recorded
> in the libxl usb xenstore nodes. If any of these fail, stop immediately
> and report an error; do not remove the xenstore nodes containing the
> original drivers of any interfaces that failed to rebind. Possible
> recovery options for the user:
> - Run 'xl usbdev-retry-rebind', which will just execute this step again
> - Unload and reload original host drivers
> - Reboot the host
>
> Both of these:
>
> 1. Will avoid the state of some interfaces bound to usbback, some
> rebound to the original drivers
>
> 2. Give the user a convenient way to re-try unbinding from usbback it failed
>
> 3. Give the user out-of-xl ways to attempt to recover the state other
> than messing around with sysfs.
>
> Rebind option 2 will give the user a convenient way to retry rebinding
> to the original driver via xl if that step failed.
>
> I'm inclined to suggest rebind option #1 for now, just to keep things
> simple.
>
> Thoughts?
>
> Chunyan, would the first half of that (removing from usbback before
> removing the pvusb xenstore nodes) actually work?
>From testing, yes, it works. But I am not sure if it is safe though. Juergen
should be very clear about that according to usbback internal codes.
Juergen, could you help to confirm that?
Thanks,
Chunyan
>
> -George
>
>
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH V13 3/5] libxl: add pvusb API
[not found] ` <56B31FAB02000066000A6596@suse.com>
@ 2016-02-04 14:39 ` Juergen Gross
2016-02-08 14:07 ` George Dunlap
0 siblings, 1 reply; 42+ messages in thread
From: Juergen Gross @ 2016-02-04 14:39 UTC (permalink / raw)
To: Chun Yan Liu, George Dunlap, George Dunlap, Ian Jackson
Cc: Jim Fehlig, Simon Cao, Wei Liu, Ian Campbell, xen-devel
On 04/02/16 02:53, Chun Yan Liu wrote:
>
>
>>>> On 2/3/2016 at 10:33 PM, in message <56B20FCC.3010308@citrix.com>, George
> Dunlap <george.dunlap@citrix.com> wrote:
>> On 02/02/16 18:11, Ian Jackson wrote:
>>> George Dunlap writes ("Re: [Xen-devel] [PATCH V13 3/5] libxl: add pvusb
>> API"):
>>>> There are effectively four states a device can be in, from the
>>>> 'assignment' point of view:
>>>>
>>>> 1. Assigned to the normal Linux device driver(s) for the USB device
>>>>
>>>> 2. Assigned to no driver
>>>>
>>>> 3. Assigned to usbback, but not yet assigned to any guest
>>>>
>>>> 4. Assigned to a guest
>>>
>>> Thanks for your clear explanation (of which I will snip much).
>>>
>>>> Additionally, each USB "device" has one or more "interfaces". To
>>>> assign a "device" to usbback in the sysfs case means assigning all of
>>>> the "interfaces". The code seems to assume that different interfaces
>>>> from the same device can be assigned to different drivers.
>>>
>>> It is indeed the case that in principle a single USB device with
>>> multiple interfaces can be assigned to multiple different drivers.
>>>
>>>> Regarding Ian's comments:
>>>>
>>>> Since "assigned to the guest" and "listed under the pvusb xenstore
>>>> node" are the same thing, is it even *possible* to (safely) unassign
>>>> the device from usbback before removing the xenstore nodes?
>>>
>>> It might be possible to remove some of the xenstore nodes but leave
>>> others present, so that usbback detaches, but enough information
>>> remains for libxl to know that Xen still `owns' the device.
>>>
>>> But, surely usbback needs to cope with the notion that the device
>>> might disappear. USB devices can disappear at any time.
>>
>> That's true. But if the USB device has actually disappeared, the device
>> is in fact "safe" from usbback. I think I might consider state 2 safe
>> to go to, but I definitely wouldn't consider state 1 safe with the
>> xenstore nodes still in place.
>>
>>>
>>>> It's not clear to me under what conditions 3->2 might fail,
>>>
>>> As an example, someone might press ^C on `xl usb-detach'.
>>
>> *grumble*
>>
>>>> or what could be done about it if it did fail.
>>>
>>> The most obvious reason for it failing is that something somewhere
>>> still held onto the device. (For umounting filesystems, and detaching
>>> block devices, this happens a lot.) So if the detach from usbback
>>> fails would probably be possible to simply retry it.
>>
>> I'm not sure what this means in the context of moving from 3 (assigned
>> to usbback) to 2 (unassigned). usbback will definitely not use the
>> device to mount something in dom0; and I'm pretty sure has no visibility
>> as to whether it's being used as a filesystem in the domU.
>>
>> (Moving from 1 -> 2 of course would be subject to this sort of thing,
>> but that's a different issue.)
>>
>>>> Regarding 2->1, again it's not clear that there's anything libxl can
>>>> do. Reloading the driver's module might reset it enough to pick up
>>>> the (now unassigned) USB devices again; other than that, rebooting is
>>>> probably the best option.
>>>
>>> I think re-attempting the bind may work. USB devices can be flaky.
>>>
>>>> It's also not clear to me, if some functions succeed in being
>>>> reassigned and others fail, whether it's best to just try to assign as
>>>> many as we can, or better to go back and un-assign all the ones that
>>>> succeeded.
>>>
>>> Unless explicitly requested, I don't think the system should create
>>> situations some interfaces are assigned to host drivers and some to
>>> usbback.
>>
>> I was talking here about whether it would be better to have some
>> interfaces assigned to the original drivers and some interfaces left
>> unassigned, or to try to leave all interfaces unassigned if any of the
>> assignments fail.
>>
>> I agree, it would be better to try to avoid the possibility of having
>> some interfaces bound to usbback and some interfaces bound to the
>> original drivers.
>>
>>> And, I'm a fan of `crash-only software': in general, if a failure
>>> occurs, the situation should just be left as-is. The intermediate
>>> state needs to be visible and rectifiable.
>>>
>>> This approach to software state machines avoids bugs where the system
>>> gets into `impossible' states, recovery from which is impossible
>>> because the designers didn't anticipate them.
>>>
>>> It would be tolerable if the recovery sometimes involves `lsusb' and
>>> echoing things into sysfs, but it would be better if not.
>>
>> Right -- so what about this. When removing a USB device:
>>
>> * First modify the pvusb xenstore nodes such that 1) it's safe to
>> attempt removing the interfaces from usbback, but 2) they still show up
>> in usb-list. (This may be a noop.)
>>
>> * Attempt to remove all interfaces from usbback; if any of these fail,
>> stop and report an error. Possible recovery options:
>> - Re-try the usb_remove command
>> - Re-load usbback (obviously disruptive to other VMs)
>> - Reboot the host
>> - Manually try unbinding with sysfs
>>
>> * Remove the remaining pvusb xenstore nodes. If this fails, stop and
>> report the error. Possible recovery options:
>> - Re-try the usb_remove command
>>
>> = REBIND OPTION 1: Do the best we can without extra commands
>>
>> * Attempt to re-bind the interfaces to the original drivers, as recorded
>> in the libxl usb xenstore nodes. If any of these fail, report an error
>> but continue to try the rest of the interfaces, and remove the xenstore
>> nodes containing information about the original drivers. Possible
>> recovery options for the user:
>> - Reload the original drivers
>> - Reboot the host
>> - Manually try rebinding with sysfs
>>
>> = REBIND OPTION 2: Include a recovery command, usb-retry-rebind
>>
>> * Attempt to re-bind the interfaces to the original drivers, as recorded
>> in the libxl usb xenstore nodes. If any of these fail, stop immediately
>> and report an error; do not remove the xenstore nodes containing the
>> original drivers of any interfaces that failed to rebind. Possible
>> recovery options for the user:
>> - Run 'xl usbdev-retry-rebind', which will just execute this step again
>> - Unload and reload original host drivers
>> - Reboot the host
>>
>> Both of these:
>>
>> 1. Will avoid the state of some interfaces bound to usbback, some
>> rebound to the original drivers
>>
>> 2. Give the user a convenient way to re-try unbinding from usbback it failed
>>
>> 3. Give the user out-of-xl ways to attempt to recover the state other
>> than messing around with sysfs.
>>
>> Rebind option 2 will give the user a convenient way to retry rebinding
>> to the original driver via xl if that step failed.
>>
>> I'm inclined to suggest rebind option #1 for now, just to keep things
>> simple.
>>
>> Thoughts?
>>
>> Chunyan, would the first half of that (removing from usbback before
>> removing the pvusb xenstore nodes) actually work?
>
> From testing, yes, it works. But I am not sure if it is safe though. Juergen
> should be very clear about that according to usbback internal codes.
>
> Juergen, could you help to confirm that?
I think it should work.
TBH, I don't think the error handling of driver binding/unbinding should
be made too intelligent. In the long run I hope to have a qemu-based
pvUSB backend. And in this case driver binding/unbinding will be handled
completely by qemu/libusb.
Whether we want to keep the kernel based pvUSB-backend support in xl
at the end should be discussed later. I'll either add the qemu based
variant or I'll replace the kernel variant with the qemu one as soon
as the qemu backend has been accepted.
Juergen
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH V13 3/5] libxl: add pvusb API
2016-02-04 14:39 ` Juergen Gross
@ 2016-02-08 14:07 ` George Dunlap
2016-02-08 14:32 ` Ian Jackson
0 siblings, 1 reply; 42+ messages in thread
From: George Dunlap @ 2016-02-08 14:07 UTC (permalink / raw)
To: Juergen Gross, Chun Yan Liu, George Dunlap, Ian Jackson
Cc: Jim Fehlig, Simon Cao, Wei Liu, Ian Campbell, xen-devel
On 04/02/16 14:39, Juergen Gross wrote:
> On 04/02/16 02:53, Chun Yan Liu wrote:
>>
>>
>>>>> On 2/3/2016 at 10:33 PM, in message <56B20FCC.3010308@citrix.com>, George
>> Dunlap <george.dunlap@citrix.com> wrote:
>>> On 02/02/16 18:11, Ian Jackson wrote:
>>>> George Dunlap writes ("Re: [Xen-devel] [PATCH V13 3/5] libxl: add pvusb
>>> API"):
>>>>> There are effectively four states a device can be in, from the
>>>>> 'assignment' point of view:
>>>>>
>>>>> 1. Assigned to the normal Linux device driver(s) for the USB device
>>>>>
>>>>> 2. Assigned to no driver
>>>>>
>>>>> 3. Assigned to usbback, but not yet assigned to any guest
>>>>>
>>>>> 4. Assigned to a guest
>>>>
>>>> Thanks for your clear explanation (of which I will snip much).
>>>>
>>>>> Additionally, each USB "device" has one or more "interfaces". To
>>>>> assign a "device" to usbback in the sysfs case means assigning all of
>>>>> the "interfaces". The code seems to assume that different interfaces
>>>>> from the same device can be assigned to different drivers.
>>>>
>>>> It is indeed the case that in principle a single USB device with
>>>> multiple interfaces can be assigned to multiple different drivers.
>>>>
>>>>> Regarding Ian's comments:
>>>>>
>>>>> Since "assigned to the guest" and "listed under the pvusb xenstore
>>>>> node" are the same thing, is it even *possible* to (safely) unassign
>>>>> the device from usbback before removing the xenstore nodes?
>>>>
>>>> It might be possible to remove some of the xenstore nodes but leave
>>>> others present, so that usbback detaches, but enough information
>>>> remains for libxl to know that Xen still `owns' the device.
>>>>
>>>> But, surely usbback needs to cope with the notion that the device
>>>> might disappear. USB devices can disappear at any time.
>>>
>>> That's true. But if the USB device has actually disappeared, the device
>>> is in fact "safe" from usbback. I think I might consider state 2 safe
>>> to go to, but I definitely wouldn't consider state 1 safe with the
>>> xenstore nodes still in place.
>>>
>>>>
>>>>> It's not clear to me under what conditions 3->2 might fail,
>>>>
>>>> As an example, someone might press ^C on `xl usb-detach'.
>>>
>>> *grumble*
>>>
>>>>> or what could be done about it if it did fail.
>>>>
>>>> The most obvious reason for it failing is that something somewhere
>>>> still held onto the device. (For umounting filesystems, and detaching
>>>> block devices, this happens a lot.) So if the detach from usbback
>>>> fails would probably be possible to simply retry it.
>>>
>>> I'm not sure what this means in the context of moving from 3 (assigned
>>> to usbback) to 2 (unassigned). usbback will definitely not use the
>>> device to mount something in dom0; and I'm pretty sure has no visibility
>>> as to whether it's being used as a filesystem in the domU.
>>>
>>> (Moving from 1 -> 2 of course would be subject to this sort of thing,
>>> but that's a different issue.)
>>>
>>>>> Regarding 2->1, again it's not clear that there's anything libxl can
>>>>> do. Reloading the driver's module might reset it enough to pick up
>>>>> the (now unassigned) USB devices again; other than that, rebooting is
>>>>> probably the best option.
>>>>
>>>> I think re-attempting the bind may work. USB devices can be flaky.
>>>>
>>>>> It's also not clear to me, if some functions succeed in being
>>>>> reassigned and others fail, whether it's best to just try to assign as
>>>>> many as we can, or better to go back and un-assign all the ones that
>>>>> succeeded.
>>>>
>>>> Unless explicitly requested, I don't think the system should create
>>>> situations some interfaces are assigned to host drivers and some to
>>>> usbback.
>>>
>>> I was talking here about whether it would be better to have some
>>> interfaces assigned to the original drivers and some interfaces left
>>> unassigned, or to try to leave all interfaces unassigned if any of the
>>> assignments fail.
>>>
>>> I agree, it would be better to try to avoid the possibility of having
>>> some interfaces bound to usbback and some interfaces bound to the
>>> original drivers.
>>>
>>>> And, I'm a fan of `crash-only software': in general, if a failure
>>>> occurs, the situation should just be left as-is. The intermediate
>>>> state needs to be visible and rectifiable.
>>>>
>>>> This approach to software state machines avoids bugs where the system
>>>> gets into `impossible' states, recovery from which is impossible
>>>> because the designers didn't anticipate them.
>>>>
>>>> It would be tolerable if the recovery sometimes involves `lsusb' and
>>>> echoing things into sysfs, but it would be better if not.
>>>
>>> Right -- so what about this. When removing a USB device:
>>>
>>> * First modify the pvusb xenstore nodes such that 1) it's safe to
>>> attempt removing the interfaces from usbback, but 2) they still show up
>>> in usb-list. (This may be a noop.)
>>>
>>> * Attempt to remove all interfaces from usbback; if any of these fail,
>>> stop and report an error. Possible recovery options:
>>> - Re-try the usb_remove command
>>> - Re-load usbback (obviously disruptive to other VMs)
>>> - Reboot the host
>>> - Manually try unbinding with sysfs
>>>
>>> * Remove the remaining pvusb xenstore nodes. If this fails, stop and
>>> report the error. Possible recovery options:
>>> - Re-try the usb_remove command
>>>
>>> = REBIND OPTION 1: Do the best we can without extra commands
>>>
>>> * Attempt to re-bind the interfaces to the original drivers, as recorded
>>> in the libxl usb xenstore nodes. If any of these fail, report an error
>>> but continue to try the rest of the interfaces, and remove the xenstore
>>> nodes containing information about the original drivers. Possible
>>> recovery options for the user:
>>> - Reload the original drivers
>>> - Reboot the host
>>> - Manually try rebinding with sysfs
>>>
>>> = REBIND OPTION 2: Include a recovery command, usb-retry-rebind
>>>
>>> * Attempt to re-bind the interfaces to the original drivers, as recorded
>>> in the libxl usb xenstore nodes. If any of these fail, stop immediately
>>> and report an error; do not remove the xenstore nodes containing the
>>> original drivers of any interfaces that failed to rebind. Possible
>>> recovery options for the user:
>>> - Run 'xl usbdev-retry-rebind', which will just execute this step again
>>> - Unload and reload original host drivers
>>> - Reboot the host
>>>
>>> Both of these:
>>>
>>> 1. Will avoid the state of some interfaces bound to usbback, some
>>> rebound to the original drivers
>>>
>>> 2. Give the user a convenient way to re-try unbinding from usbback it failed
>>>
>>> 3. Give the user out-of-xl ways to attempt to recover the state other
>>> than messing around with sysfs.
>>>
>>> Rebind option 2 will give the user a convenient way to retry rebinding
>>> to the original driver via xl if that step failed.
>>>
>>> I'm inclined to suggest rebind option #1 for now, just to keep things
>>> simple.
>>>
>>> Thoughts?
>>>
>>> Chunyan, would the first half of that (removing from usbback before
>>> removing the pvusb xenstore nodes) actually work?
>>
>> From testing, yes, it works. But I am not sure if it is safe though. Juergen
>> should be very clear about that according to usbback internal codes.
>>
>> Juergen, could you help to confirm that?
>
> I think it should work.
>
> TBH, I don't think the error handling of driver binding/unbinding should
> be made too intelligent. In the long run I hope to have a qemu-based
> pvUSB backend. And in this case driver binding/unbinding will be handled
> completely by qemu/libusb.
>
> Whether we want to keep the kernel based pvUSB-backend support in xl
> at the end should be discussed later. I'll either add the qemu based
> variant or I'll replace the kernel variant with the qemu one as soon
> as the qemu backend has been accepted.
There's a difference between "making it intelligent" and "not making it
broken". :-) Given that users can potentially cause a number of these
things to fail just by pressing Ctrl-C, we need to at least make sure
that we don't get into a completely wedged state that the user can't do
anything to fix. That requires some careful thought.
-George
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH V13 3/5] libxl: add pvusb API
2016-02-08 14:07 ` George Dunlap
@ 2016-02-08 14:32 ` Ian Jackson
0 siblings, 0 replies; 42+ messages in thread
From: Ian Jackson @ 2016-02-08 14:32 UTC (permalink / raw)
To: George Dunlap
Cc: Juergen Gross, Wei Liu, Ian Campbell, George Dunlap,
Chun Yan Liu, xen-devel, Jim Fehlig, Simon Cao
George Dunlap writes ("Re: [Xen-devel] [PATCH V13 3/5] libxl: add pvusb API"):
> There's a difference between "making it intelligent" and "not making it
> broken". :-) Given that users can potentially cause a number of these
> things to fail just by pressing Ctrl-C, we need to at least make sure
> that we don't get into a completely wedged state that the user can't do
> anything to fix. That requires some careful thought.
Right. There is a useful design principle which can help simplify and
clarify: "crash-only software".[1][2]
The idea is that when an error occurs, it is usually best to simply
stop and leave the system in whatever intermediate state. The next
run of the software is responsible for discovering, interpreting and
if necessary rectifying the situation.
In general these recovery paths are necessary anyway. So the
on-error-cleanup doesn't help.
In the context of that series, I think that means:
We observe that there is an ordering of possible states
attached to dom0 driver
unattached
attached to usbback
assigned to a guest
When reconfiguring a device, it will move through these states in
order (forwards or backwards, normally).
NB that I don't discuss here what information may be recorded in
xenstore and the VM configuration at each stage: so in fact there are
various micro-states in between the major states shown above (eg,
"attached to usbback and in process of attaching to geust").
To make a coherent design, we need to:
Arrange that each of these states can be seen by the user somehow.
(Eg in output from list-assignable.)
Arrange that each intermediate state can be recovered to at least one
endpoint state by use of some appropriate command(s).
Arrange that the recording of metadata in xenstore and the domain json
config occurs in the right order to support the above (ie, that the
micro-states are right). After an approach has been chosen, to show
that the design is correct, it is probably easiest to explicitly
enumerate all the micro-states.
Ideally I would like to see this aspect of the design covered in a doc
comment (or maybe commit messages), in some form or other.
I hope this is of some help.
Thanks,
Ian.
[1] https://www.usenix.org/events/hotos03/tech/full_papers/candea/candea.pdf
[2] https://en.wikipedia.org/wiki/Crash-only_software
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH V13 5/5] xl: add pvusb commands
2016-01-19 8:39 ` [PATCH V13 5/5] xl: add pvusb commands Chunyan Liu
2016-01-19 18:11 ` Ian Jackson
@ 2016-02-16 16:56 ` Olaf Hering
2016-02-17 6:14 ` Chun Yan Liu
2016-02-19 10:43 ` Chun Yan Liu
1 sibling, 2 replies; 42+ messages in thread
From: Olaf Hering @ 2016-02-16 16:56 UTC (permalink / raw)
To: Chunyan Liu
Cc: jgross, wei.liu2, ian.campbell, george.dunlap, Ian.Jackson,
xen-devel, jfehlig, Simon Cao
On Tue, Jan 19, Chunyan Liu wrote:
> #xl usbctrl-attach test_vm version=1 ports=8
> #xl usbdev-attach test_vm hostbus=1 hostaddr=2
I think this does not handle the -N knob of xl. Other commands check the
global dryrun_only variable.
Olaf
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH V13 5/5] xl: add pvusb commands
2016-01-20 3:10 ` Jim Fehlig
2016-01-21 12:12 ` Wei Liu
@ 2016-02-16 17:53 ` Ian Jackson
2016-02-17 11:43 ` Wei Liu
1 sibling, 1 reply; 42+ messages in thread
From: Ian Jackson @ 2016-02-16 17:53 UTC (permalink / raw)
To: Jim Fehlig
Cc: jgross, wei.liu2, ian.campbell, george.dunlap, Chunyan Liu,
xen-devel, Simon Cao
Jim Fehlig writes ("Re: [Xen-devel] [PATCH V13 5/5] xl: add pvusb commands"):
> I just noticed this is the case with network devices as well. E.g.
>
> #xl network-attach hvm-domU mac=00:16:3e:xx:yy:zz,bridge=br0
> libxl: error: libxl_device.c:1095:device_hotplug_child_death_cb: script: Could
> not find bridge device xenbr0
This is clearly a bug.
> main_networkattach() in tools/libxl/xl_cmdimpl.c doesn't split on
> the ',', so everything after mac=00:16:3e:xx:yy:zz is ignored.
That's quite silly, isn't it. Looking at the code it would also accept
mac=00:16:3e:aa:bb:cc:THIS:SHOULD:NOT:BE:HERE
The bug seems to be that the ad-hoc strtoul-based mac address parser
in xl_cmdimpl.c:parse_nic_config doesn't notice garbage after its
option.
> I'd need advice on how to fix this though. Based on
> xl-network-configuration doc and Xen tool's long history of
> network-attach supporting that syntax, I'd say main_networkattach()
> should be changed to split on ','. I could also change the docs. Do
> tools maintainers have a preference, or alternative option?
I don't have a strong opinion, but I would lean towards insisting that
on command lines each setting should be in its own argument.
Ian.
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH V13 5/5] xl: add pvusb commands
2016-02-16 16:56 ` Olaf Hering
@ 2016-02-17 6:14 ` Chun Yan Liu
2016-02-19 10:43 ` Chun Yan Liu
1 sibling, 0 replies; 42+ messages in thread
From: Chun Yan Liu @ 2016-02-17 6:14 UTC (permalink / raw)
To: Olaf Hering
Cc: Juergen Gross, wei.liu2, ian.campbell, george.dunlap,
Ian.Jackson, xen-devel, Jim Fehlig, Simon Cao
n
>>> On 2/17/2016 at 12:56 AM, in message <20160216165608.GA21669@gmail.com>, Olaf
Hering <olaf@aepfle.de> wrote:
> On Tue, Jan 19, Chunyan Liu wrote:
>
> > #xl usbctrl-attach test_vm version=1 ports=8
>
> > #xl usbdev-attach test_vm hostbus=1 hostaddr=2
>
> I think this does not handle the -N knob of xl. Other commands check the
> global dryrun_only variable.
Thanks. I'll add.
>
> Olaf
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>
>
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH V13 5/5] xl: add pvusb commands
2016-02-16 17:53 ` Ian Jackson
@ 2016-02-17 11:43 ` Wei Liu
2016-02-17 12:17 ` Ian Jackson
0 siblings, 1 reply; 42+ messages in thread
From: Wei Liu @ 2016-02-17 11:43 UTC (permalink / raw)
To: Ian Jackson
Cc: jgross, wei.liu2, ian.campbell, george.dunlap, Chunyan Liu,
xen-devel, Jim Fehlig, Simon Cao
On Tue, Feb 16, 2016 at 05:53:18PM +0000, Ian Jackson wrote:
> Jim Fehlig writes ("Re: [Xen-devel] [PATCH V13 5/5] xl: add pvusb commands"):
> > I just noticed this is the case with network devices as well. E.g.
> >
> > #xl network-attach hvm-domU mac=00:16:3e:xx:yy:zz,bridge=br0
> > libxl: error: libxl_device.c:1095:device_hotplug_child_death_cb: script: Could
> > not find bridge device xenbr0
>
> This is clearly a bug.
>
> > main_networkattach() in tools/libxl/xl_cmdimpl.c doesn't split on
> > the ',', so everything after mac=00:16:3e:xx:yy:zz is ignored.
>
> That's quite silly, isn't it. Looking at the code it would also accept
> mac=00:16:3e:aa:bb:cc:THIS:SHOULD:NOT:BE:HERE
>
> The bug seems to be that the ad-hoc strtoul-based mac address parser
> in xl_cmdimpl.c:parse_nic_config doesn't notice garbage after its
> option.
>
> > I'd need advice on how to fix this though. Based on
> > xl-network-configuration doc and Xen tool's long history of
> > network-attach supporting that syntax, I'd say main_networkattach()
> > should be changed to split on ','. I could also change the docs. Do
> > tools maintainers have a preference, or alternative option?
>
> I don't have a strong opinion, but I would lean towards insisting that
> on command lines each setting should be in its own argument.
>
We should support both IMHO. Libxlu's disk spec parser is able to do
that with parse_disk_config_multistring.
Wei.
> Ian.
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH V13 5/5] xl: add pvusb commands
2016-02-17 11:43 ` Wei Liu
@ 2016-02-17 12:17 ` Ian Jackson
0 siblings, 0 replies; 42+ messages in thread
From: Ian Jackson @ 2016-02-17 12:17 UTC (permalink / raw)
To: Wei Liu
Cc: jgross, ian.campbell, george.dunlap, Chunyan Liu, xen-devel,
Jim Fehlig, Simon Cao
Wei Liu writes ("Re: [Xen-devel] [PATCH V13 5/5] xl: add pvusb commands"):
> On Tue, Feb 16, 2016 at 05:53:18PM +0000, Ian Jackson wrote:
> > I don't have a strong opinion, but I would lean towards insisting that
> > on command lines each setting should be in its own argument.
>
> We should support both IMHO. Libxlu's disk spec parser is able to do
> that with parse_disk_config_multistring.
Fine by me. As I say I don't have a strong opinion, so we should do
what Wei wants as he does :-).
Ian.
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH V13 5/5] xl: add pvusb commands
2016-02-16 16:56 ` Olaf Hering
2016-02-17 6:14 ` Chun Yan Liu
@ 2016-02-19 10:43 ` Chun Yan Liu
2016-02-19 10:52 ` Olaf Hering
1 sibling, 1 reply; 42+ messages in thread
From: Chun Yan Liu @ 2016-02-19 10:43 UTC (permalink / raw)
To: Olaf Hering
Cc: Juergen Gross, wei.liu2, ian.campbell, george.dunlap,
Ian.Jackson, xen-devel, Jim Fehlig, Simon Cao
>>> On 2/17/2016 at 12:56 AM, in message <20160216165608.GA21669@gmail.com>, Olaf
Hering <olaf@aepfle.de> wrote:
> On Tue, Jan 19, Chunyan Liu wrote:
>
> > #xl usbctrl-attach test_vm version=1 ports=8
>
> > #xl usbdev-attach test_vm hostbus=1 hostaddr=2
>
> I think this does not handle the -N knob of xl. Other commands check the
> global dryrun_only variable.
Just sent V14. I tried to add dryrun_only codes but finally gave up. For usbdev-attach,
since it will automatically and dynamically create usb controller in some cases, it's hard
to print dryrun-only information.
Chunyan
>
> Olaf
>
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH V13 5/5] xl: add pvusb commands
2016-02-19 10:43 ` Chun Yan Liu
@ 2016-02-19 10:52 ` Olaf Hering
2016-02-19 12:07 ` Olaf Hering
0 siblings, 1 reply; 42+ messages in thread
From: Olaf Hering @ 2016-02-19 10:52 UTC (permalink / raw)
To: Chun Yan Liu
Cc: Juergen Gross, wei.liu2, ian.campbell, george.dunlap,
Ian.Jackson, xen-devel, Jim Fehlig, Simon Cao
On Fri, Feb 19, Chun Yan Liu wrote:
>
>
> >>> On 2/17/2016 at 12:56 AM, in message <20160216165608.GA21669@gmail.com>, Olaf
> Hering <olaf@aepfle.de> wrote:
> > On Tue, Jan 19, Chunyan Liu wrote:
> >
> > > #xl usbctrl-attach test_vm version=1 ports=8
> >
> > > #xl usbdev-attach test_vm hostbus=1 hostaddr=2
> >
> > I think this does not handle the -N knob of xl. Other commands check the
> > global dryrun_only variable.
>
> Just sent V14. I tried to add dryrun_only codes but finally gave up. For usbdev-attach,
> since it will automatically and dynamically create usb controller in some cases, it's hard
> to print dryrun-only information.
Not sure how to handle it, perhaps exit when xl -N is called?
Olaf
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH V13 5/5] xl: add pvusb commands
2016-02-19 10:52 ` Olaf Hering
@ 2016-02-19 12:07 ` Olaf Hering
2016-02-24 12:03 ` Wei Liu
0 siblings, 1 reply; 42+ messages in thread
From: Olaf Hering @ 2016-02-19 12:07 UTC (permalink / raw)
To: wei.liu2, ian.campbell, george.dunlap, Ian.Jackson
Cc: Juergen Gross, Jim Fehlig, Simon Cao, Chun Yan Liu, xen-devel
On Fri, Feb 19, Olaf Hering wrote:
> Not sure how to handle it, perhaps exit when xl -N is called?
Also the interface is 'xl -N' is not clearly defined. What is it
supposed to do with the newly introduced ctrl types? Should it display
the json just for the dev, just for the ctrl+dev or the entire ctrl with
all existing devs + the new dev?
Who is the consumer of the json output?
xl(1) says just "-N Dry run: do not actually execute the command."
I'm asking this mainly in the context of main_vscsiattach, which right
now dumps the enitre ctrl with all existing devs + the new dev.
Olaf
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH V13 5/5] xl: add pvusb commands
2016-02-19 12:07 ` Olaf Hering
@ 2016-02-24 12:03 ` Wei Liu
2016-02-24 12:52 ` Olaf Hering
0 siblings, 1 reply; 42+ messages in thread
From: Wei Liu @ 2016-02-24 12:03 UTC (permalink / raw)
To: Olaf Hering
Cc: Juergen Gross, wei.liu2, ian.campbell, george.dunlap,
Ian.Jackson, Chun Yan Liu, xen-devel, Jim Fehlig, Simon Cao
On Fri, Feb 19, 2016 at 01:07:08PM +0100, Olaf Hering wrote:
> On Fri, Feb 19, Olaf Hering wrote:
>
> > Not sure how to handle it, perhaps exit when xl -N is called?
>
> Also the interface is 'xl -N' is not clearly defined. What is it
> supposed to do with the newly introduced ctrl types? Should it display
> the json just for the dev, just for the ctrl+dev or the entire ctrl with
> all existing devs + the new dev?
> Who is the consumer of the json output?
> xl(1) says just "-N Dry run: do not actually execute the command."
>
> I'm asking this mainly in the context of main_vscsiattach, which right
> now dumps the enitre ctrl with all existing devs + the new dev.
>
I just turn everything into JSON and print it out?
Say, if you only add a controller, you just print the ctrl JSON. If you
add a controller and a bunch of devices, you print all of them. Does
this sound plausible?
I tend to think dryrun output is only used by administrator to eye-ball
the resulting structure. I'm not sure if it is meant to be stable at
all.
Maybe Ian has a second opinion on this.
Wei.
> Olaf
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH V13 5/5] xl: add pvusb commands
2016-02-24 12:03 ` Wei Liu
@ 2016-02-24 12:52 ` Olaf Hering
0 siblings, 0 replies; 42+ messages in thread
From: Olaf Hering @ 2016-02-24 12:52 UTC (permalink / raw)
To: Wei Liu
Cc: Juergen Gross, ian.campbell, george.dunlap, Ian.Jackson,
Chun Yan Liu, xen-devel, Jim Fehlig, Simon Cao
On Wed, Feb 24, Wei Liu wrote:
> Say, if you only add a controller, you just print the ctrl JSON. If you
> add a controller and a bunch of devices, you print all of them. Does
> this sound plausible?
Yes.
Olaf
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH V13 3/5] libxl: add pvusb API
[not found] <56B0843302000066000A4C90@relay2.provo.novell.com>
@ 2016-02-02 2:36 ` Chun Yan Liu
0 siblings, 0 replies; 42+ messages in thread
From: Chun Yan Liu @ 2016-02-02 2:36 UTC (permalink / raw)
To: George.Dunlap
Cc: Juergen Gross, wei.liu2, ian.campbell, Ian.Jackson,
george.dunlap, xen-devel, Jim Fehlig, caobosimon
>>> On 1/27/2016 at 12:21 AM, in message
<CAFLBxZaTWuViBUjH663tR-GWfwRGCWSNKoRecik9+Q6TJ3HQmA@mail.gmail.com>, George
Dunlap <George.Dunlap@eu.citrix.com> wrote:
> On Tue, Jan 26, 2016 at 7:43 AM, Chun Yan Liu <cyliu@suse.com> wrote:
> >
> >
> >>>> On 1/20/2016 at 12:56 PM, in message
> > <569F83F5020000660009E6AC@relay2.provo.novell.com>, "Chun Yan Liu"
> > <cyliu@suse.com> wrote:
> >
> >>
> >>>>> On 1/19/2016 at 11:48 PM, in message
> >> <22174.23240.402164.635740@mariner.uk.xensource.com>, Ian Jackson
> >> <Ian.Jackson@eu.citrix.com> wrote:
> >> > Chunyan Liu writes ("[PATCH V13 3/5] libxl: add pvusb API"):
> >> > > Add pvusb APIs, including:
> >> > > - attach/detach (create/destroy) virtual usb controller.
> >> > > - attach/detach usb device
> >> > > - list usb controller and usb devices
> >> > > - some other helper functions
> >> >
> >> >
> >> > Thanks. This is making progress but I'm afraid we're not quite there
> >> > yet.
> >> >
> >> >
> >> > > +static int usbback_dev_unassign(libxl__gc *gc, const char *busid)
> >> > > +{
> >> > ...
> >> > > + /* Till here, USB device has been unbound from USBBACK and
> >> > > + * removed from xenstore, usb list couldn't show it anymore,
> >> > > + * so no matter removimg driver path successfully or not,
> >> > > + * we will report operation success.
> >> > > + */
> >> >
> >> > I'm still unconvinced by this and this may mean that the code in this
> >> > function is in the wrong order. Earlier we had this exchange:
> >> >
> >> > > > Ought this function to really report success if these calls fail ?
> >> > >
> >> > > I think so. Till here, the USB device has already been unbound from
> >> > > usbback and removed from xenstore. usb-list cannot list it any more.
> >> >
> >> > The problem is that I think that if this function fails, it can leave
> >> > - debris in xenstore (the usbback path)
> >> Yes, it's true.
> >>
> >> > - the interface bound to the wrong driver
> >> No, it won't be bound to 'wrong' driver, only maybe not bound to any driver
> >> (Already unbound from usbback, but failed to rebound to its original
> >> driver).
> >> In this case, we would report warning: failed to rebind to driver xxx.
> >>
> >> > And then there is no way for the user to get libxl to re-attempt the
> >> > operation, or clean up. Am I right ?
> >>
> >> Yes. No way to re-attempt usbdev-detach or cleanup driver path in
> >> xenstore. But won't affect next time usbdev-attach the same device.
> >>
> >> >
> >> > One way to avoid this kind of problem is to deal with the xenstore
> >> > path last. That way the device will still appear as attached to the
> >> > domain.
> >>
> >> I'm afraid if the side effect is acceptable? In my testing, some USB
> >> bluetooth
> >> device always fails to rebind to 'btusb' driver after it's unbound
> >> from 'usbback'. In this case, we can't detach it from the domain then.
George & Ian, may I have your thoughts? Except for above case, I think dealing
with xenstore at last place is indeed better.
PS: I'll take vocation for half month, so hope to make any progress before that :-)
Thanks,
Chunyan
> >
> > Ian J., any opinion on this? If it's still thought to be better, I'll
> update patch.
>
> I think Ian may be waiting for me to reply and express an opinion; but
> unfortunately that will have to wait until next week. :-(
>
> -George
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>
>
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH V13 3/5] libxl: add pvusb API
2016-01-26 7:43 ` Chun Yan Liu
@ 2016-01-26 16:21 ` George Dunlap
0 siblings, 0 replies; 42+ messages in thread
From: George Dunlap @ 2016-01-26 16:21 UTC (permalink / raw)
To: Chun Yan Liu
Cc: Juergen Gross, Wei Liu, Ian Campbell, Ian Jackson, George Dunlap,
xen-devel, Jim Fehlig, Simon Cao
On Tue, Jan 26, 2016 at 7:43 AM, Chun Yan Liu <cyliu@suse.com> wrote:
>
>
>>>> On 1/20/2016 at 12:56 PM, in message
> <569F83F5020000660009E6AC@relay2.provo.novell.com>, "Chun Yan Liu"
> <cyliu@suse.com> wrote:
>
>>
>>>>> On 1/19/2016 at 11:48 PM, in message
>> <22174.23240.402164.635740@mariner.uk.xensource.com>, Ian Jackson
>> <Ian.Jackson@eu.citrix.com> wrote:
>> > Chunyan Liu writes ("[PATCH V13 3/5] libxl: add pvusb API"):
>> > > Add pvusb APIs, including:
>> > > - attach/detach (create/destroy) virtual usb controller.
>> > > - attach/detach usb device
>> > > - list usb controller and usb devices
>> > > - some other helper functions
>> >
>> >
>> > Thanks. This is making progress but I'm afraid we're not quite there
>> > yet.
>> >
>> >
>> > > +static int usbback_dev_unassign(libxl__gc *gc, const char *busid)
>> > > +{
>> > ...
>> > > + /* Till here, USB device has been unbound from USBBACK and
>> > > + * removed from xenstore, usb list couldn't show it anymore,
>> > > + * so no matter removimg driver path successfully or not,
>> > > + * we will report operation success.
>> > > + */
>> >
>> > I'm still unconvinced by this and this may mean that the code in this
>> > function is in the wrong order. Earlier we had this exchange:
>> >
>> > > > Ought this function to really report success if these calls fail ?
>> > >
>> > > I think so. Till here, the USB device has already been unbound from
>> > > usbback and removed from xenstore. usb-list cannot list it any more.
>> >
>> > The problem is that I think that if this function fails, it can leave
>> > - debris in xenstore (the usbback path)
>> Yes, it's true.
>>
>> > - the interface bound to the wrong driver
>> No, it won't be bound to 'wrong' driver, only maybe not bound to any driver
>> (Already unbound from usbback, but failed to rebound to its original
>> driver).
>> In this case, we would report warning: failed to rebind to driver xxx.
>>
>> > And then there is no way for the user to get libxl to re-attempt the
>> > operation, or clean up. Am I right ?
>>
>> Yes. No way to re-attempt usbdev-detach or cleanup driver path in
>> xenstore. But won't affect next time usbdev-attach the same device.
>>
>> >
>> > One way to avoid this kind of problem is to deal with the xenstore
>> > path last. That way the device will still appear as attached to the
>> > domain.
>>
>> I'm afraid if the side effect is acceptable? In my testing, some USB
>> bluetooth
>> device always fails to rebind to 'btusb' driver after it's unbound
>> from 'usbback'. In this case, we can't detach it from the domain then.
>
> Ian J., any opinion on this? If it's still thought to be better, I'll update patch.
I think Ian may be waiting for me to reply and express an opinion; but
unfortunately that will have to wait until next week. :-(
-George
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH V13 3/5] libxl: add pvusb API
2016-01-20 4:56 ` [PATCH V13 3/5] libxl: add pvusb API Chun Yan Liu
@ 2016-01-26 7:43 ` Chun Yan Liu
2016-01-26 16:21 ` George Dunlap
0 siblings, 1 reply; 42+ messages in thread
From: Chun Yan Liu @ 2016-01-26 7:43 UTC (permalink / raw)
To: Ian.Jackson, Chun Yan Liu
Cc: Juergen Gross, wei.liu2, ian.campbell, george.dunlap,
george.dunlap, xen-devel, Jim Fehlig, caobosimon
>>> On 1/20/2016 at 12:56 PM, in message
<569F83F5020000660009E6AC@relay2.provo.novell.com>, "Chun Yan Liu"
<cyliu@suse.com> wrote:
>
>>>> On 1/19/2016 at 11:48 PM, in message
> <22174.23240.402164.635740@mariner.uk.xensource.com>, Ian Jackson
> <Ian.Jackson@eu.citrix.com> wrote:
> > Chunyan Liu writes ("[PATCH V13 3/5] libxl: add pvusb API"):
> > > Add pvusb APIs, including:
> > > - attach/detach (create/destroy) virtual usb controller.
> > > - attach/detach usb device
> > > - list usb controller and usb devices
> > > - some other helper functions
> >
> >
> > Thanks. This is making progress but I'm afraid we're not quite there
> > yet.
> >
> >
> > > +static int usbback_dev_unassign(libxl__gc *gc, const char *busid)
> > > +{
> > ...
> > > + /* Till here, USB device has been unbound from USBBACK and
> > > + * removed from xenstore, usb list couldn't show it anymore,
> > > + * so no matter removimg driver path successfully or not,
> > > + * we will report operation success.
> > > + */
> >
> > I'm still unconvinced by this and this may mean that the code in this
> > function is in the wrong order. Earlier we had this exchange:
> >
> > > > Ought this function to really report success if these calls fail ?
> > >
> > > I think so. Till here, the USB device has already been unbound from
> > > usbback and removed from xenstore. usb-list cannot list it any more.
> >
> > The problem is that I think that if this function fails, it can leave
> > - debris in xenstore (the usbback path)
> Yes, it's true.
>
> > - the interface bound to the wrong driver
> No, it won't be bound to 'wrong' driver, only maybe not bound to any driver
> (Already unbound from usbback, but failed to rebound to its original
> driver).
> In this case, we would report warning: failed to rebind to driver xxx.
>
> > And then there is no way for the user to get libxl to re-attempt the
> > operation, or clean up. Am I right ?
>
> Yes. No way to re-attempt usbdev-detach or cleanup driver path in
> xenstore. But won't affect next time usbdev-attach the same device.
>
> >
> > One way to avoid this kind of problem is to deal with the xenstore
> > path last. That way the device will still appear as attached to the
> > domain.
>
> I'm afraid if the side effect is acceptable? In my testing, some USB
> bluetooth
> device always fails to rebind to 'btusb' driver after it's unbound
> from 'usbback'. In this case, we can't detach it from the domain then.
Ian J., any opinion on this? If it's still thought to be better, I'll update patch.
>
> >
> > To do this properly I think bind_usbintf may need to become idempotent
> > even in the face of other callers racing to assign the device. I
> > think that would mean bind_usbif it would have to know what driver to
> > expect to find the device bound to.
bind_usbintf already has parameter to indicate which driver to be bound to.
- Chunyan
> >
> > George, am I right here ?
> >
> >
> > I have a few other comments too:
> >
> > > +/* get original driver path of usb interface, stored in @drvpath */
> > > +static int usbintf_get_drvpath(libxl__gc *gc, const char *intf, char
> > **drvpath)
> > > +{
> > > + char *spath, *dp = NULL;
> > > + struct stat st;
> > > + int rc;
> > > +
> > > + spath = GCSPRINTF(SYSFS_USB_DEV "/%s/driver", intf);
> > > +
> > > + rc = lstat(spath, &st);
> >
> > This `rc' should be `r'. (CODING_STYLE.)
> >
> > I mentioned this in my review of v12 in the context of
> > sysfs_write_intf. But there is still more than one occurrence in v13
> > of `rc' for a syscall return value.
> >
>
> Sorry, will double check again.
>
> >
> > > +static int sysfs_write_intf(libxl__gc *gc, const char *intf, const char
> > *path)
> > > +{
> >
> > Last time I wrote:
> >
> > But there is nothing usb specific about this function. Looking for
> > similar code elsewhere this function seems very similar to
> > libxl_pci.c:sysfs_write_bdf, but I won't ask you to try to refactor
> > these two functions together.
> >
> > This time I have remembered that libxl_write_exactly exists, and could
> > be used. Sorry for not saing this last time but I think you can
> > probably just get rid of this in favour of libxl_write_exactly. If
> > you think not, please say why.
>
> After checking the codes, yes, except for open and close fd,
> libxl_write_exactly does the work. Will reuse it.
>
> >
> >
> > > +static int bind_usbintf(libxl__gc *gc, const char *intf, const char
> > *drvpath)
> > > +{
> > > + char *path;
> > > + struct stat st;
> > > +
> > > + path = GCSPRINTF("%s/%s", drvpath, intf);
> > > + /* if already bound, return */
> > > + if (!lstat(path, &st))
> > > + return 0;
> > > + else if (errno != ENOENT)
> > > + return ERROR_FAIL;
> >
> > This new ERROR_FAIL fails to log anything, and probably should. I
> > think the anomalous style leads to this mistake. You should say
> > r = lstat...
> > and adopt the pattern from the rest of libxl.
>
> Will update.
>
> Thanks,
> Chunyan
>
> >
> >
> > Thanks,
> > Ian.
> >
> >
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>
>
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH V13 3/5] libxl: add pvusb API
[not found] <569F8391020000660009E667@relay2.provo.novell.com>
@ 2016-01-20 4:56 ` Chun Yan Liu
2016-01-26 7:43 ` Chun Yan Liu
0 siblings, 1 reply; 42+ messages in thread
From: Chun Yan Liu @ 2016-01-20 4:56 UTC (permalink / raw)
To: Ian.Jackson
Cc: Juergen Gross, wei.liu2, ian.campbell, george.dunlap,
george.dunlap, xen-devel, Jim Fehlig, caobosimon
>>> On 1/19/2016 at 11:48 PM, in message
<22174.23240.402164.635740@mariner.uk.xensource.com>, Ian Jackson
<Ian.Jackson@eu.citrix.com> wrote:
> Chunyan Liu writes ("[PATCH V13 3/5] libxl: add pvusb API"):
> > Add pvusb APIs, including:
> > - attach/detach (create/destroy) virtual usb controller.
> > - attach/detach usb device
> > - list usb controller and usb devices
> > - some other helper functions
>
>
> Thanks. This is making progress but I'm afraid we're not quite there
> yet.
>
>
> > +static int usbback_dev_unassign(libxl__gc *gc, const char *busid)
> > +{
> ...
> > + /* Till here, USB device has been unbound from USBBACK and
> > + * removed from xenstore, usb list couldn't show it anymore,
> > + * so no matter removimg driver path successfully or not,
> > + * we will report operation success.
> > + */
>
> I'm still unconvinced by this and this may mean that the code in this
> function is in the wrong order. Earlier we had this exchange:
>
> > > Ought this function to really report success if these calls fail ?
> >
> > I think so. Till here, the USB device has already been unbound from
> > usbback and removed from xenstore. usb-list cannot list it any more.
>
> The problem is that I think that if this function fails, it can leave
> - debris in xenstore (the usbback path)
Yes, it's true.
> - the interface bound to the wrong driver
No, it won't be bound to 'wrong' driver, only maybe not bound to any driver
(Already unbound from usbback, but failed to rebound to its original driver).
In this case, we would report warning: failed to rebind to driver xxx.
> And then there is no way for the user to get libxl to re-attempt the
> operation, or clean up. Am I right ?
Yes. No way to re-attempt usbdev-detach or cleanup driver path in
xenstore. But won't affect next time usbdev-attach the same device.
>
> One way to avoid this kind of problem is to deal with the xenstore
> path last. That way the device will still appear as attached to the
> domain.
I'm afraid if the side effect is acceptable. In my testing, some USB bluetooth
device always fails to rebind to 'btusb' driver after it's unbound
from 'usbback'. In this case, we can't detach it from the domain then.
>
> To do this properly I think bind_usbintf may need to become idempotent
> even in the face of other callers racing to assign the device. I
> think that would mean bind_usbif it would have to know what driver to
> expect to find the device bound to.
>
> George, am I right here ?
>
>
> I have a few other comments too:
>
> > +/* get original driver path of usb interface, stored in @drvpath */
> > +static int usbintf_get_drvpath(libxl__gc *gc, const char *intf, char
> **drvpath)
> > +{
> > + char *spath, *dp = NULL;
> > + struct stat st;
> > + int rc;
> > +
> > + spath = GCSPRINTF(SYSFS_USB_DEV "/%s/driver", intf);
> > +
> > + rc = lstat(spath, &st);
>
> This `rc' should be `r'. (CODING_STYLE.)
>
> I mentioned this in my review of v12 in the context of
> sysfs_write_intf. But there is still more than one occurrence in v13
> of `rc' for a syscall return value.
>
Sorry, will double check again.
>
> > +static int sysfs_write_intf(libxl__gc *gc, const char *intf, const char
> *path)
> > +{
>
> Last time I wrote:
>
> But there is nothing usb specific about this function. Looking for
> similar code elsewhere this function seems very similar to
> libxl_pci.c:sysfs_write_bdf, but I won't ask you to try to refactor
> these two functions together.
>
> This time I have remembered that libxl_write_exactly exists, and could
> be used. Sorry for not saing this last time but I think you can
> probably just get rid of this in favour of libxl_write_exactly. If
> you think not, please say why.
After checking the codes, yes, except for open and close fd,
libxl_write_exactly does the work. Will reuse it.
>
>
> > +static int bind_usbintf(libxl__gc *gc, const char *intf, const char
> *drvpath)
> > +{
> > + char *path;
> > + struct stat st;
> > +
> > + path = GCSPRINTF("%s/%s", drvpath, intf);
> > + /* if already bound, return */
> > + if (!lstat(path, &st))
> > + return 0;
> > + else if (errno != ENOENT)
> > + return ERROR_FAIL;
>
> This new ERROR_FAIL fails to log anything, and probably should. I
> think the anomalous style leads to this mistake. You should say
> r = lstat...
> and adopt the pattern from the rest of libxl.
Will update.
Thanks,
Chunyan
>
>
> Thanks,
> Ian.
>
>
^ permalink raw reply [flat|nested] 42+ messages in thread
end of thread, other threads:[~2016-02-24 12:52 UTC | newest]
Thread overview: 42+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-01-19 8:39 [PATCH V13 0/5] xen pvusb toolstack work Chunyan Liu
2016-01-19 8:39 ` [PATCH V13 1/5] libxl: export some functions for pvusb use Chunyan Liu
2016-01-19 8:39 ` [PATCH V13 2/5] libxl_utils: add internal function to read sysfs file contents Chunyan Liu
2016-01-19 8:39 ` [PATCH V13 3/5] libxl: add pvusb API Chunyan Liu
2016-01-19 15:48 ` Ian Jackson
2016-02-02 16:26 ` George Dunlap
2016-02-02 18:11 ` Ian Jackson
2016-02-03 7:34 ` Chun Yan Liu
2016-02-03 14:38 ` George Dunlap
2016-02-04 1:44 ` Chun Yan Liu
2016-02-03 14:33 ` George Dunlap
2016-02-04 1:53 ` Chun Yan Liu
[not found] ` <56B31FAB02000066000A6596@suse.com>
2016-02-04 14:39 ` Juergen Gross
2016-02-08 14:07 ` George Dunlap
2016-02-08 14:32 ` Ian Jackson
2016-02-02 16:48 ` George Dunlap
2016-02-03 8:25 ` Chun Yan Liu
2016-01-26 16:12 ` Olaf Hering
2016-01-27 2:29 ` Chun Yan Liu
2016-01-19 8:39 ` [PATCH V13 4/5] domcreate: support pvusb in configuration file Chunyan Liu
2016-01-19 18:08 ` Ian Jackson
2016-01-19 8:39 ` [PATCH V13 5/5] xl: add pvusb commands Chunyan Liu
2016-01-19 18:11 ` Ian Jackson
2016-01-20 3:10 ` Jim Fehlig
2016-01-21 12:12 ` Wei Liu
2016-01-21 12:21 ` Ian Campbell
2016-01-21 12:29 ` Wei Liu
2016-01-21 17:37 ` Ian Jackson
2016-02-16 17:53 ` Ian Jackson
2016-02-17 11:43 ` Wei Liu
2016-02-17 12:17 ` Ian Jackson
2016-02-16 16:56 ` Olaf Hering
2016-02-17 6:14 ` Chun Yan Liu
2016-02-19 10:43 ` Chun Yan Liu
2016-02-19 10:52 ` Olaf Hering
2016-02-19 12:07 ` Olaf Hering
2016-02-24 12:03 ` Wei Liu
2016-02-24 12:52 ` Olaf Hering
[not found] <569F8391020000660009E667@relay2.provo.novell.com>
2016-01-20 4:56 ` [PATCH V13 3/5] libxl: add pvusb API Chun Yan Liu
2016-01-26 7:43 ` Chun Yan Liu
2016-01-26 16:21 ` George Dunlap
[not found] <56B0843302000066000A4C90@relay2.provo.novell.com>
2016-02-02 2:36 ` Chun Yan Liu
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.