* [PATCH V6 0/7] xen pvusb toolstack work
@ 2015-08-10 10:35 Chunyan Liu
2015-08-10 10:35 ` [PATCH V6 1/7] libxl: export some functions for pvusb use Chunyan Liu
` (6 more replies)
0 siblings, 7 replies; 41+ messages in thread
From: Chunyan Liu @ 2015-08-10 10:35 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 V5:
* Address George's comments on libxl API and Ian's comments on
libxl_read_sysfs_file_content
V5 is here:
http://lists.xen.org/archives/html/xen-devel/2015-06/msg04052.html
V4 is here:
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.
Chunyan Liu (7):
libxl: export some functions for pvusb use
libxl_read_file_contents: add new entry to read sysfs file
libxl: add pvusb API
libxl: add libxl_device_usb_assignable_list API
xl: add pvusb commands
xl: add usb-assignable-list command
domcreate: support pvusb in configuration file
docs/man/xl.cfg.pod.5 | 75 ++
docs/man/xl.pod.1 | 40 +
tools/libxl/Makefile | 2 +-
tools/libxl/libxl.c | 57 +-
tools/libxl/libxl.h | 68 ++
tools/libxl/libxl_create.c | 73 +-
tools/libxl/libxl_device.c | 8 +
tools/libxl/libxl_internal.h | 33 +-
tools/libxl/libxl_osdeps.h | 13 +
tools/libxl/libxl_pvusb.c | 1373 ++++++++++++++++++++++++++++++++++
tools/libxl/libxl_types.idl | 59 ++
tools/libxl/libxl_types_internal.idl | 1 +
tools/libxl/libxl_utils.c | 67 +-
tools/libxl/libxl_utils.h | 5 +
tools/libxl/xl.h | 6 +
tools/libxl/xl_cmdimpl.c | 369 ++++++++-
tools/libxl/xl_cmdtable.c | 29 +
17 files changed, 2259 insertions(+), 19 deletions(-)
create mode 100644 tools/libxl/libxl_pvusb.c
--
2.1.4
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH V6 1/7] libxl: export some functions for pvusb use
2015-08-10 10:35 [PATCH V6 0/7] xen pvusb toolstack work Chunyan Liu
@ 2015-08-10 10:35 ` Chunyan Liu
2015-08-11 11:26 ` Wei Liu
2015-08-10 10:35 ` [PATCH V6 2/7] libxl_read_file_contents: add new entry to read sysfs file Chunyan Liu
` (5 subsequent siblings)
6 siblings, 1 reply; 41+ messages in thread
From: Chunyan Liu @ 2015-08-10 10:35 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>
---
tools/libxl/libxl.c | 4 ++--
tools/libxl/libxl_internal.h | 3 +++
2 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 083f099..006e8da 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1995,7 +1995,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;
@@ -2014,7 +2014,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,
+int libxl__resolve_domid(libxl__gc *gc, const char *name,
uint32_t *domid)
{
if (!name)
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 6ea6c83..6013628 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1152,6 +1152,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] 41+ messages in thread
* [PATCH V6 2/7] libxl_read_file_contents: add new entry to read sysfs file
2015-08-10 10:35 [PATCH V6 0/7] xen pvusb toolstack work Chunyan Liu
2015-08-10 10:35 ` [PATCH V6 1/7] libxl: export some functions for pvusb use Chunyan Liu
@ 2015-08-10 10:35 ` Chunyan Liu
2015-08-11 11:26 ` Wei Liu
2015-08-10 10:35 ` [PATCH V6 3/7] libxl: add pvusb API Chunyan Liu
` (4 subsequent siblings)
6 siblings, 1 reply; 41+ messages in thread
From: Chunyan Liu @ 2015-08-10 10:35 UTC (permalink / raw)
To: xen-devel
Cc: jgross, wei.liu2, ian.campbell, george.dunlap, Ian.Jackson,
Chunyan Liu, jfehlig
Sysfs file has size=4096 but actual file content is less than that.
Current libxl_read_file_contents will treat it as error when file size
and actual file content differs, so reading sysfs file content with
this function always fails.
Add a new entry 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>
---
Changes:
- read one more byte to check bigger size problem.
tools/libxl/libxl_internal.h | 2 ++
tools/libxl/libxl_utils.c | 51 ++++++++++++++++++++++++++++++++++----------
2 files changed, 42 insertions(+), 11 deletions(-)
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 6013628..f98f089 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -4001,6 +4001,8 @@ void libxl__bitmap_copy_best_effort(libxl__gc *gc, libxl_bitmap *dptr,
int libxl__count_physical_sockets(libxl__gc *gc, int *sockets);
#endif
+_hidden int libxl_read_sysfs_file_contents(libxl_ctx *ctx, const char *filename,
+ void **data_r, int *datalen_r);
/*
* Local variables:
diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c
index bfc9699..9234efb 100644
--- a/tools/libxl/libxl_utils.c
+++ b/tools/libxl/libxl_utils.c
@@ -322,8 +322,10 @@ out:
return rc;
}
-int libxl_read_file_contents(libxl_ctx *ctx, const char *filename,
- void **data_r, int *datalen_r) {
+static int libxl_read_file_contents_core(libxl_ctx *ctx, const char *filename,
+ void **data_r, int *datalen_r,
+ bool tolerate_shrinking_file)
+{
GC_INIT(ctx);
FILE *f = 0;
uint8_t *data = 0;
@@ -359,20 +361,34 @@ int libxl_read_file_contents(libxl_ctx *ctx, const char *filename,
datalen = stab.st_size;
if (stab.st_size && data_r) {
- data = malloc(datalen);
+ data = malloc(datalen + 1);
if (!data) goto xe;
- rs = fread(data, 1, datalen, f);
- if (rs != datalen) {
- if (ferror(f))
+ rs = fread(data, 1, datalen + 1, f);
+ if (rs > datalen) {
+ LOG(ERROR, "%s increased size while we were reading it",
+ filename);
+ goto xe;
+ }
+
+ if (rs < datalen) {
+ if (ferror(f)) {
LOGE(ERROR, "failed to read %s", filename);
- else if (feof(f))
- LOG(ERROR, "%s changed size while we were reading it",
- filename);
- else
+ goto xe;
+ } else if (feof(f)) {
+ if (tolerate_shrinking_file) {
+ datalen = rs;
+ } else {
+ LOG(ERROR, "%s shrunk size while we were reading it",
+ filename);
+ goto xe;
+ }
+ } else {
abort();
- goto xe;
+ }
}
+
+ data = realloc(data, datalen);
}
if (fclose(f)) {
@@ -396,6 +412,19 @@ int libxl_read_file_contents(libxl_ctx *ctx, const char *filename,
return e;
}
+int libxl_read_file_contents(libxl_ctx *ctx, const char *filename,
+ void **data_r, int *datalen_r)
+{
+ return libxl_read_file_contents_core(ctx, filename, data_r, datalen_r, 0);
+}
+
+int libxl_read_sysfs_file_contents(libxl_ctx *ctx, const char *filename,
+ void **data_r, int *datalen_r)
+{
+ return libxl_read_file_contents_core(ctx, filename, data_r, datalen_r, 1);
+}
+
+
#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] 41+ messages in thread
* [PATCH V6 3/7] libxl: add pvusb API
2015-08-10 10:35 [PATCH V6 0/7] xen pvusb toolstack work Chunyan Liu
2015-08-10 10:35 ` [PATCH V6 1/7] libxl: export some functions for pvusb use Chunyan Liu
2015-08-10 10:35 ` [PATCH V6 2/7] libxl_read_file_contents: add new entry to read sysfs file Chunyan Liu
@ 2015-08-10 10:35 ` Chunyan Liu
2015-08-11 11:27 ` Wei Liu
2015-09-08 14:17 ` Ian Campbell
2015-08-10 10:35 ` [PATCH V6 4/7] libxl: add libxl_device_usb_assignable_list API Chunyan Liu
` (3 subsequent siblings)
6 siblings, 2 replies; 41+ messages in thread
From: Chunyan Liu @ 2015-08-10 10:35 UTC (permalink / raw)
To: xen-devel
Cc: jgross, wei.liu2, ian.campbell, george.dunlap, Ian.Jackson,
Chunyan Liu, 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>
---
changes:
- Address George's comments:
* Update libxl_device_usb_getinfo to read ctrl/port only and
get other information.
* Update backend path according to xenstore frontend 'xxx/backend'
entry instead of using TOOLSTACK_DOMID.
* Use 'type' to indicate qemu/pv instead of previous naming 'protocol'.
* Add USB 'devtype' union, currently only includes "hostdev"
tools/libxl/Makefile | 2 +-
tools/libxl/libxl.c | 53 ++
tools/libxl/libxl.h | 65 ++
tools/libxl/libxl_device.c | 4 +
tools/libxl/libxl_internal.h | 20 +-
tools/libxl/libxl_osdeps.h | 13 +
tools/libxl/libxl_pvusb.c | 1320 ++++++++++++++++++++++++++++++++++
tools/libxl/libxl_types.idl | 59 ++
tools/libxl/libxl_types_internal.idl | 1 +
tools/libxl/libxl_utils.c | 16 +
tools/libxl/libxl_utils.h | 5 +
11 files changed, 1556 insertions(+), 2 deletions(-)
create mode 100644 tools/libxl/libxl_pvusb.c
diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
index 9036076..cdb50fe 100644
--- a/tools/libxl/Makefile
+++ b/tools/libxl/Makefile
@@ -103,7 +103,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 006e8da..35843a8 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -4179,11 +4179,54 @@ DEFINE_DEVICE_REMOVE(vtpm, destroy, 1)
/******************************************************************************/
+/* Macro for defining device remove/destroy functions for usbctrl */
+/* Following functions are defined:
+ * libxl_device_usbctrl_remove
+ * libxl_device_usbctrl_destroy
+ */
+
+#define DEFINE_DEVICE_REMOVE_EXT(type, removedestroy, f) \
+ int libxl_device_##type##_##removedestroy(libxl_ctx *ctx, \
+ uint32_t domid, libxl_device_##type *type, \
+ const libxl_asyncop_how *ao_how) \
+ { \
+ AO_CREATE(ctx, domid, ao_how); \
+ libxl__device *device; \
+ libxl__ao_device *aodev; \
+ int rc; \
+ \
+ GCNEW(device); \
+ rc = libxl__device_from_##type(gc, domid, type, device); \
+ if (rc != 0) goto out; \
+ \
+ GCNEW(aodev); \
+ libxl__prepare_ao_device(ao, aodev); \
+ aodev->action = LIBXL__DEVICE_ACTION_REMOVE; \
+ aodev->dev = device; \
+ aodev->callback = device_addrm_aocomplete; \
+ aodev->force = f; \
+ libxl__initiate_device_##type##_remove(egc, aodev); \
+ \
+ out: \
+ if (rc) return AO_CREATE_FAIL(rc); \
+ return AO_INPROGRESS; \
+ }
+
+
+DEFINE_DEVICE_REMOVE_EXT(usbctrl, remove, 0)
+DEFINE_DEVICE_REMOVE_EXT(usbctrl, destroy, 1)
+
+#undef DEFINE_DEVICE_REMOVE_EXT
+
+/******************************************************************************/
+
/* Macro for defining device addition functions in a compact way */
/* The following functions are defined:
* libxl_device_disk_add
* libxl_device_nic_add
* libxl_device_vtpm_add
+ * libxl_device_usbctrl_add
+ * libxl_device_usb_add
*/
#define DEFINE_DEVICE_ADD(type) \
@@ -4215,6 +4258,12 @@ DEFINE_DEVICE_ADD(nic)
/* vtpm */
DEFINE_DEVICE_ADD(vtpm)
+/* usbctrl */
+DEFINE_DEVICE_ADD(usbctrl)
+
+/* usb */
+DEFINE_DEVICE_ADD(usb)
+
#undef DEFINE_DEVICE_ADD
/******************************************************************************/
@@ -6671,6 +6720,10 @@ int libxl_retrieve_domain_configuration(libxl_ctx *ctx, uint32_t domid,
MERGE(pci, pcidevs, COMPARE_PCI, {});
+ MERGE(usbctrl, usbctrls, COMPARE_USBCTRL, {});
+
+ MERGE(usb, usbs, 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 5f9047c..05b6331 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -123,6 +123,23 @@
#define LIBXL_HAVE_DOMAIN_NODEAFFINITY 1
/*
+ * LIBXL_HAVE_PVUSB indicates the functions for doing hot-plug of
+ * USB devices through pvusb.
+ *
+ * With this functionality, 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 either 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 either 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.
+ *
+ */
+#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
@@ -1389,6 +1406,54 @@ 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 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_usb_add(libxl_ctx *ctx, uint32_t domid, libxl_device_usb *usb,
+ const libxl_asyncop_how *ao_how)
+ LIBXL_EXTERNAL_CALLERS_ONLY;
+
+int libxl_device_usb_remove(libxl_ctx *ctx, uint32_t domid, libxl_device_usb *usb,
+ const libxl_asyncop_how *ao_how)
+ LIBXL_EXTERNAL_CALLERS_ONLY;
+
+libxl_device_usb *
+libxl_device_usb_list(libxl_ctx *ctx, uint32_t domid, int *num);
+
+libxl_device_usb *
+libxl_device_usb_list_per_usbctrl(libxl_ctx *ctx, uint32_t domid,
+ libxl_devid usbctrl, int *num);
+
+void libxl_device_usb_list_free(libxl_device_usb *list, int nr);
+
+int libxl_device_usb_getinfo(libxl_ctx *ctx, uint32_t domid,
+ libxl_device_usb *usb,
+ libxl_usbinfo *usbinfo);
+
/* 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 bee5ed5..935f25b 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -676,6 +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;
+ if (dev->backend_kind == LIBXL__DEVICE_KIND_VUSB) {
+ libxl__initiate_device_usbctrl_remove(egc, aodev);
+ continue;
+ }
libxl__initiate_device_remove(egc, aodev);
}
}
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index f98f089..5be3b3a 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -2553,6 +2553,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_usb_add(libxl__egc *egc, uint32_t domid,
+ libxl_device_usb *usb,
+ 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);
@@ -2585,6 +2593,13 @@ _hidden void libxl__wait_device_connection(libxl__egc*,
_hidden void libxl__initiate_device_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
* be passed to the hotplug script for the requested device.
@@ -3937,7 +3952,10 @@ 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)->u.hostdev.hostbus == (b)->u.hostdev.hostbus && \
+ (a)->u.hostdev.hostaddr == (b)->u.hostdev.hostaddr)
+#define COMPARE_USBCTRL(a, b) ((a)->devid == (b)->devid)
+
/* DEVICE_ADD
*
* Add a device in libxl_domain_config structure
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..d4c4c03
--- /dev/null
+++ b/tools/libxl/libxl_pvusb.c
@@ -0,0 +1,1320 @@
+/*
+ * 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"
+
+#define USBBACK_INFO_PATH "/libxl/usbback"
+
+#define USBHUB_CLASS_CODE 9
+
+/* Utility to read backend xenstore keys */
+#define READ_BACKEND(tgc, subpath) \
+ libxl__xs_read(tgc, XBT_NULL, GCSPRINTF("%s/" subpath, be_path))
+
+/* Utility to read frontend xenstore keys */
+#define READ_FRONTEND(tgc, subpath) \
+ libxl__xs_read(tgc, XBT_NULL, GCSPRINTF("%s/" subpath, fe_path))
+
+static int libxl__device_usbctrl_setdefault(libxl__gc *gc, uint32_t domid,
+ libxl_device_usbctrl *usbctrl)
+{
+ int rc;
+
+ if (!usbctrl->version)
+ usbctrl->version = 2;
+
+ if (!usbctrl->ports)
+ usbctrl->ports = 8;
+
+ if (usbctrl->type == LIBXL_USBCTRL_TYPE_AUTO)
+ usbctrl->type = LIBXL_USBCTRL_TYPE_PV;
+
+ 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", "1");
+ 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", "1");
+
+ 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.
+ */
+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;
+ }
+ }
+
+ 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);
+
+ rc = 0;
+
+out:
+ aodev->rc = rc;
+ if (rc) aodev->callback(egc, aodev);
+ return;
+}
+
+static int
+libxl__device_usb_list_per_usbctrl(libxl__gc *gc, uint32_t domid,
+ libxl_devid usbctrl,
+ libxl_device_usb **usbs, int *num);
+
+static int
+libxl__device_usb_remove(libxl__gc *gc, uint32_t domid, libxl_device_usb *usb);
+
+/* 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
+ */
+void libxl__initiate_device_usbctrl_remove(libxl__egc *egc,
+ libxl__ao_device *aodev)
+{
+ STATE_AO_GC(aodev->ao);
+ libxl_device_usb *usbs = NULL;
+ int numusb = 0;
+ int i, rc;
+ uint32_t domid = ao->domid;
+ int usbctrl_devid = aodev->dev->devid;
+
+ /* Remove usb devices first */
+ rc = libxl__device_usb_list_per_usbctrl(gc, domid, usbctrl_devid,
+ &usbs, &numusb);
+ if (rc) goto out;
+
+ for (i = 0; i < numusb; i++) {
+ if (libxl__device_usb_remove(gc, domid, &usbs[i])) {
+ LOG(ERROR, "libxl__device_usb_remove failed");
+ rc = ERROR_FAIL;
+ goto out;
+ }
+ }
+
+ libxl_device_usb_list_free(usbs, numusb);
+
+ /* Remove usbctrl */
+ return libxl__initiate_device_remove(egc, aodev);
+
+out:
+ libxl_device_usb_list_free(usbs, numusb);
+ aodev->rc = rc;
+ if (rc) aodev->callback(egc, aodev);
+ return;
+}
+
+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 **dir = NULL;
+ unsigned int ndirs = 0;
+
+ *num = 0;
+
+ path = GCSPRINTF("%s/device/vusb",
+ libxl__xs_get_dompath(gc, domid));
+ dir = libxl__xs_directory(gc, XBT_NULL, path, &ndirs);
+
+ if (dir && ndirs) {
+ usbctrls = libxl__zalloc(NOGC, sizeof(*usbctrls) * ndirs);
+ libxl_device_usbctrl *usbctrl;
+ libxl_device_usbctrl *end = usbctrls + ndirs;
+ for (usbctrl = usbctrls; usbctrl < end; usbctrl++, dir++, (*num)++) {
+ const char *tmp, *be_path;
+ const char *fe_path = GCSPRINTF("%s/%s", path, *dir);
+
+ libxl_device_usbctrl_init(usbctrl);
+ usbctrl->devid = atoi(*dir);
+
+ be_path = READ_FRONTEND(gc, "backend");
+ if (!be_path) goto outerr;
+
+ tmp = READ_FRONTEND(gc, "backend-id");
+ if (!tmp) goto outerr;
+ usbctrl->backend_domid = atoi(tmp);
+
+ tmp = READ_BACKEND(gc, "usb-ver");
+ if (!tmp) goto outerr;
+ usbctrl->version = atoi(tmp);
+
+ tmp = READ_BACKEND(gc, "num-ports");
+ if (!tmp) goto outerr;
+ usbctrl->ports = atoi(tmp);
+
+ tmp = READ_BACKEND(gc, "type");
+ if (!tmp) goto outerr;
+ libxl_usbctrl_type_from_string(tmp, &usbctrl->type);
+ }
+ }
+
+ goto out;
+
+outerr:
+ LOG(ERROR, "Unable to list USB Controllers");
+ libxl_device_usbctrl_list_free(usbctrls, *num);
+ *num = 0;
+ usbctrls = NULL;
+
+out:
+ GC_FREE;
+ return usbctrls;
+}
+
+int libxl_device_usbctrl_getinfo(libxl_ctx *ctx, uint32_t domid,
+ libxl_device_usbctrl *usbctrl,
+ libxl_usbctrlinfo *usbctrlinfo)
+{
+ GC_INIT(ctx);
+ char *dompath;
+ const char *fe_path, *be_path, *tmp;
+ int rc = 0;
+
+ usbctrlinfo->devid = usbctrl->devid;
+
+ dompath = libxl__xs_get_dompath(gc, domid);
+ fe_path = GCSPRINTF("%s/device/vusb/%d", dompath, usbctrl->devid);
+ be_path = READ_FRONTEND(gc, "backend");
+ if (!be_path) {
+ rc = ERROR_FAIL;
+ goto out;
+ }
+ usbctrlinfo->backend = libxl__strdup(NOGC, be_path);
+
+ tmp = READ_FRONTEND(gc, "backend-id");
+ usbctrlinfo->backend_id = tmp ? strtoul(tmp, NULL, 10) : -1;
+
+ tmp = READ_FRONTEND(gc, "state");
+ usbctrlinfo->state = tmp ? strtoul(tmp, NULL, 10) : -1;
+
+ tmp = READ_FRONTEND(gc, "event-channel");
+ usbctrlinfo->evtch = tmp ? strtoul(tmp, NULL, 10) : -1;
+
+ tmp = READ_FRONTEND(gc, "urb-ring-ref");
+ usbctrlinfo->ref_urb = tmp ? strtoul(tmp, NULL, 10) : -1;
+
+ tmp = READ_FRONTEND(gc, "conn-ring-ref");
+ usbctrlinfo->ref_conn = tmp ? strtoul(tmp, NULL, 10) : -1;
+
+ tmp = READ_BACKEND(gc, "frontend");
+ usbctrlinfo->frontend = libxl__strdup(NOGC, tmp);
+
+ tmp = READ_BACKEND(gc, "frontend-id");
+ usbctrlinfo->frontend_id = tmp ? strtoul(tmp, NULL, 10) : -1;
+
+ tmp = READ_BACKEND(gc, "num-ports");
+ usbctrlinfo->ports = tmp ? strtoul(tmp, NULL, 10) : -1;
+
+ tmp = READ_BACKEND(gc, "usb-ver");
+ usbctrlinfo->version = tmp ? strtoul(tmp, NULL, 10) : -1;
+
+ tmp = READ_BACKEND(gc, "type");
+ libxl_usbctrl_type_from_string(tmp, &usbctrlinfo->type);
+
+out:
+ GC_FREE;
+ return rc;
+}
+
+int libxl_devid_to_device_usbctrl(libxl_ctx *ctx,
+ uint32_t domid,
+ int devid,
+ libxl_device_usbctrl *usbctrl)
+{
+ GC_INIT(ctx);
+ libxl_device_usbctrl *usbctrls;
+ int nb = 0;
+ int i, rc = -1;
+
+ usbctrls = libxl_device_usbctrl_list(ctx, domid, &nb);
+ if (!nb) goto out;
+
+ libxl_device_usbctrl_init(usbctrl);
+ for (i = 0; i < nb; i++) {
+ if (devid == usbctrls[i].devid) {
+ *usbctrl = usbctrls[i];
+ rc = 0;
+ break;
+ }
+ }
+
+ libxl_device_usbctrl_list_free(usbctrls, nb);
+
+out:
+ GC_FREE;
+ return rc;
+}
+
+static char *usb_busaddr_to_busid(libxl__gc *gc, int bus, int addr)
+{
+ libxl_ctx *ctx = CTX;
+ struct dirent *de;
+ DIR *dir;
+ char *busid = NULL;
+
+ assert(bus > 0 && addr > 0);
+
+ if (!(dir = opendir(SYSFS_USB_DEV)))
+ return NULL;
+
+ while ((de = readdir(dir))) {
+ char *filename;
+ void *buf;
+ int busnum = -1;
+ int devnum = -1;
+
+ if (!de->d_name)
+ continue;
+
+ filename = GCSPRINTF(SYSFS_USB_DEV"/%s/devnum", de->d_name);
+ if (!libxl_read_sysfs_file_contents(ctx, filename, &buf, NULL))
+ sscanf(buf, "%d", &devnum);
+
+ filename = GCSPRINTF(SYSFS_USB_DEV"/%s/busnum", de->d_name);
+ if (!libxl_read_sysfs_file_contents(ctx, filename, &buf, NULL))
+ sscanf(buf, "%d", &busnum);
+
+ if (bus == busnum && addr == devnum) {
+ busid = libxl__strdup(NOGC, de->d_name);
+ break;
+ }
+ }
+
+ closedir(dir);
+ return busid;
+}
+
+static void usb_busaddr_from_busid(libxl__gc *gc, char *busid,
+ int *bus, int *addr)
+{
+ libxl_ctx *ctx = CTX;
+ char *filename;
+ void *buf;
+
+ assert(busid);
+
+ filename = GCSPRINTF(SYSFS_USB_DEV"/%s/busnum", busid);
+ if (!libxl_read_sysfs_file_contents(ctx, filename, &buf, NULL))
+ sscanf(buf, "%d", bus);
+
+ filename = GCSPRINTF(SYSFS_USB_DEV"/%s/devnum", busid);
+ if (!libxl_read_sysfs_file_contents(ctx, filename, &buf, NULL))
+ sscanf(buf, "%d", addr);
+}
+
+static int
+libxl__device_usb_assigned_list(libxl__gc *gc,
+ libxl_device_usb **list, int *num)
+{
+ char **domlist;
+ unsigned int nd = 0, i, j;
+ libxl_device_usb *usb;
+
+ *list = NULL;
+ *num = 0;
+
+ domlist = libxl__xs_directory(gc, XBT_NULL, "/local/domain", &nd);
+ for (i = 0; i < nd; i++) {
+ char *path, **ctrl_list;
+ unsigned int nc = 0;
+
+ path = GCSPRINTF("/local/domain/%s/device/vusb", domlist[i]);
+ ctrl_list = libxl__xs_directory(gc, XBT_NULL, path, &nc);
+
+ for (j = 0; j < nc; j++) {
+ char *be_path, *num_ports;
+
+ be_path = libxl__xs_read(gc, XBT_NULL,
+ GCSPRINTF("%s/%s/backend", path, ctrl_list[j]));
+ num_ports = READ_BACKEND(gc, "num-ports");
+ if (num_ports) {
+ int nport = atoi(num_ports), k;
+ char *devpath, *busid;
+
+ for (k = 0; k < nport; k++) {
+ devpath = GCSPRINTF("%s/port/%d", be_path, k + 1);
+ busid = libxl__xs_read(gc, XBT_NULL, devpath);
+ /* If there is USB device attached, add it to list */
+ if (busid && strcmp(busid, "")) {
+ GCREALLOC_ARRAY(*list, *num + 1);
+ usb = *list + *num;
+ usb->ctrl = atoi(ctrl_list[j]);
+ usb->port = k + 1;
+ usb_busaddr_from_busid(gc, busid,
+ &usb->u.hostdev.hostbus,
+ &usb->u.hostdev.hostaddr);
+ (*num)++;
+ }
+ }
+ }
+ }
+ }
+
+ return 0;
+}
+
+static bool is_usb_in_array(libxl_device_usb *usbs, int num,
+ libxl_device_usb *usb)
+{
+ int i;
+
+ for (i = 0; i < num; i++) {
+ if (COMPARE_USB(&usbs[i], usb))
+ return true;
+ }
+
+ return false;
+}
+
+/* check if USB device is already assigned to a domain */
+static bool is_usb_assigned(libxl__gc *gc, libxl_device_usb *usb)
+{
+ libxl_device_usb *usbs;
+ int rc, num;
+
+ rc = libxl__device_usb_assigned_list(gc, &usbs, &num);
+ if (rc) {
+ LOG(ERROR, "Fail to get assigned usb list");
+ return true;
+ }
+
+ return is_usb_in_array(usbs, num, usb);
+}
+
+/* check if USB device type is assignable */
+static bool is_usb_assignable(libxl__gc *gc, libxl_device_usb *usb)
+{
+ libxl_ctx *ctx = CTX;
+ int classcode;
+ char *filename;
+ void *buf = NULL;
+ char *busid = NULL;
+
+ assert(usb->u.hostdev.hostbus > 0 && usb->u.hostdev.hostaddr > 0);
+ busid = usb_busaddr_to_busid(gc, usb->u.hostdev.hostbus, usb->u.hostdev.hostaddr);
+
+ filename = GCSPRINTF(SYSFS_USB_DEV"/%s/bDeviceClass", busid);
+ if (libxl_read_sysfs_file_contents(ctx, filename, &buf, NULL))
+ return false;
+
+ sscanf(buf, "%d", &classcode);
+ return classcode != USBHUB_CLASS_CODE;
+}
+
+/* get usb devices under certain usb controller */
+static int
+libxl__device_usb_list_per_usbctrl(libxl__gc *gc, uint32_t domid,
+ libxl_devid usbctrl,
+ libxl_device_usb **usbs, int *num)
+{
+ char *fe_path, *be_path, *num_devs;
+ int n, i;
+
+ *usbs = NULL;
+ *num = 0;
+
+ fe_path = GCSPRINTF("%s/device/vusb/%d",
+ libxl__xs_get_dompath(gc, domid), usbctrl);
+ if (!fe_path)
+ return -1;
+
+ be_path = READ_FRONTEND(gc, "backend");
+ if (!be_path)
+ return -1;
+
+ num_devs = READ_BACKEND(gc, "num-ports");
+ if (!num_devs)
+ return 0;
+
+ n = atoi(num_devs);
+ *usbs = libxl__calloc(NOGC, n, sizeof(libxl_device_usb));
+
+ for (i = 0; i < n; i++) {
+ char *busid;
+ libxl_device_usb *usb = NULL;
+
+ busid = libxl__xs_read(gc, XBT_NULL,
+ GCSPRINTF("%s/port/%d", be_path, i + 1));
+ if (busid && strcmp(busid, "")) {
+ usb = *usbs + *num;
+ usb->ctrl = usbctrl;
+ usb->port = i + 1;
+ usb_busaddr_from_busid(gc, busid,
+ &usb->u.hostdev.hostbus,
+ &usb->u.hostdev.hostaddr);
+ (*num)++;
+ }
+ }
+
+ return 0;
+}
+
+/* get all usb devices of the domain */
+libxl_device_usb *
+libxl_device_usb_list(libxl_ctx *ctx, uint32_t domid, int *num)
+{
+ GC_INIT(ctx);
+ char *path;
+ char **usbctrls;
+ unsigned int nc = 0;
+ int i, j;
+ libxl_device_usb *usbs = NULL;
+
+ *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 nd = 0;
+ libxl_device_usb *tmp = NULL;
+ libxl__device_usb_list_per_usbctrl(gc, domid,
+ atoi(usbctrls[i]), &tmp, &nd);
+ if (!nd) continue;
+
+ usbs = libxl__realloc(NOGC, usbs, sizeof(*usbs) * (*num + nd));
+ for (j = 0; j < nd; j++) {
+ usbs[*num] = tmp[j];
+ (*num)++;
+ }
+ libxl_device_usb_list_free(tmp, nd);
+ }
+
+ GC_FREE;
+ return usbs;
+}
+
+libxl_device_usb *
+libxl_device_usb_list_per_usbctrl(libxl_ctx *ctx, uint32_t domid,
+ libxl_devid usbctrl, int *num)
+{
+ GC_INIT(ctx);
+ libxl_device_usb *usbs = NULL;
+
+ libxl__device_usb_list_per_usbctrl(gc, domid, usbctrl, &usbs, num);
+
+ GC_FREE;
+ return usbs;
+}
+
+/* find first unused controller:port and give that to usb device */
+static int
+libxl__device_usb_set_default_usbctrl(libxl__gc *gc, uint32_t domid,
+ libxl_device_usb *usb)
+{
+ libxl_ctx *ctx = CTX;
+ libxl_device_usbctrl *usbctrls = NULL;
+ int numctrl = 0;
+ int i, j, rc = -1;
+
+ usbctrls = libxl_device_usbctrl_list(ctx, domid, &numctrl);
+ if (!numctrl)
+ goto out;
+
+ for (i = 0; i < numctrl; i++) {
+ for (j = 0; j < usbctrls[i].ports; j++) {
+ 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);
+ tmp = libxl__xs_read(gc, XBT_NULL, path);
+ if (tmp && !strcmp(tmp, "")) {
+ usb->ctrl = usbctrls[i].devid;
+ usb->port = j + 1;
+ rc = 0;
+ break;
+ }
+ }
+ }
+
+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_usb_setdefault(libxl__gc *gc, uint32_t domid,
+ libxl_device_usb *usb,
+ bool update_json)
+{
+ int rc = -1;
+
+ if (!usb->devtype)
+ usb->devtype = LIBXL_USBDEV_TYPE_HOSTDEV;
+
+ if (usb->ctrl == -1) {
+ if (usb->port) {
+ LOG(ERROR, "USB controller must be specified if you specify port");
+ return ERROR_INVAL;
+ }
+
+ rc = libxl__device_usb_set_default_usbctrl(gc, domid, usb);
+ /* 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) {
+ goto out;
+ }
+ }
+
+ rc = libxl__device_usbctrl_add_xenstore(gc, domid, usbctrl,
+ update_json);
+ if (rc) goto out;
+
+ usb->ctrl = usbctrl->devid;
+ usb->port = 1;
+ }
+ } else if (!usb->port) {
+ /* Valid port starts from 1. Choose port for us. */
+ int i, ports;
+ char *fe_path, *be_path, *tmp;
+
+ fe_path = GCSPRINTF("%s/device/vusb/%d",
+ libxl__xs_get_dompath(gc, domid), usb->ctrl);
+ be_path = READ_FRONTEND(gc, "backend");
+ tmp = READ_BACKEND(gc, "num-ports");
+ ports = tmp ? atoi(tmp) : 0;
+
+ for (i = 0; i < ports; i++) {
+ tmp = libxl__xs_read(gc, XBT_NULL,
+ GCSPRINTF("%s/port/%d", be_path, i + 1));
+ if (tmp && !strcmp(tmp, "")) {
+ usb->port = i + 1;
+ break;
+ }
+ }
+
+ if (!usb->port) {
+ LOG(ERROR, "No available port under specified controller");
+ goto out;
+ }
+ } else {
+ char *fe_path, *be_path, *tmp;
+
+ fe_path = GCSPRINTF("%s/device/vusb/%d",
+ libxl__xs_get_dompath(gc, domid), usb->ctrl);
+ be_path = READ_FRONTEND(gc, "backend");
+ tmp = libxl__xs_read(gc, XBT_NULL,
+ GCSPRINTF("%s/port/%d", be_path, usb->port));
+ if (!tmp || strcmp(tmp, "")) {
+ LOG(ERROR, "The controller port isn't available");
+ 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_usb_add_xenstore(libxl__gc *gc, uint32_t domid,
+ libxl_device_usb *usb,
+ bool update_json)
+{
+ char *be_path;
+ char *busid;
+ int rc;
+ xs_transaction_t t = XBT_NULL;
+ libxl_domain_config d_config;
+ libxl_device_usb usb_saved;
+ libxl__domain_userdata_lock *lock = NULL;
+
+ libxl_domain_config_init(&d_config);
+ libxl_device_usb_init(&usb_saved);
+ libxl_device_usb_copy(CTX, &usb_saved, usb);
+
+ busid = usb_busaddr_to_busid(gc, usb->u.hostdev.hostbus,
+ usb->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(usb, usbs, domid, &usb_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, usb->ctrl, usb->port);
+
+ LOG(DEBUG, "Adding new usb device to xenstore");
+ if (libxl__xs_write_checked(gc, t, be_path, busid))
+ 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_usb_dispose(&usb_saved);
+ libxl_domain_config_dispose(&d_config);
+ return rc;
+}
+
+static int libxl__device_usb_remove_xenstore(libxl__gc *gc, uint32_t domid,
+ libxl_device_usb *usb)
+{
+ char *be_path;
+
+ be_path = GCSPRINTF("%s/backend/vusb/%d/%d/port/%d",
+ libxl__xs_get_dompath(gc, LIBXL_TOOLSTACK_DOMID),
+ domid, usb->ctrl, usb->port);
+ LOG(DEBUG, "Removing USB device from xenstore");
+ if (libxl__xs_write_checked(gc,XBT_NULL, be_path, ""))
+ return ERROR_FAIL;
+
+ return 0;
+}
+
+/* bind/unbind usb device interface */
+static int unbind_usb_intf(libxl__gc *gc, char *intf, char **drvpath)
+{
+ char *path, *spath, *dp = NULL;
+ int fd = -1;
+ int rc = 0;
+ struct stat st;
+
+ spath = GCSPRINTF(SYSFS_USB_DEV"/%s/driver", intf);
+ if (!lstat(spath, &st)) {
+ /* Find the canonical path to the driver. */
+ dp = libxl__zalloc(gc, PATH_MAX);
+ dp = realpath(spath, dp);
+
+ path = GCSPRINTF("%s/unbind", spath);
+ fd = open(path, O_WRONLY);
+ if (fd < 0)
+ return ERROR_FAIL;
+ rc = write(fd, intf, strlen(intf));
+ close(fd);
+ if (rc < 0)
+ return ERROR_FAIL;
+ }
+
+ if (drvpath)
+ *drvpath = dp;
+
+ return 0;
+}
+
+static int bind_usb_intf(libxl__gc *gc, char *intf, char *drvpath)
+{
+ char *path;
+ struct stat st;
+ int fd, rc = 0;
+
+ path = GCSPRINTF("%s/%s", drvpath, intf);
+ rc = lstat(path, &st);
+ /* already bind, return */
+ if (rc == 0)
+ return 0;
+
+ path = GCSPRINTF("%s/bind", drvpath);
+ fd = open(path, O_WRONLY);
+ if (fd < 0)
+ return ERROR_FAIL;
+
+ rc = write(fd, intf, strlen(intf));
+ close(fd);
+ if (rc < 0)
+ return ERROR_FAIL;
+
+ return 0;
+}
+
+/* Is usb interface bound to usbback? */
+static int usb_intf_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 usb_get_all_interfaces(libxl__gc *gc, libxl_device_usb *usb,
+ char ***intfs, int *num)
+{
+ DIR *dir;
+ struct dirent *entry;
+ char *buf;
+ char *busid;
+ int rc = 0;
+
+ *intfs = NULL;
+ *num = 0;
+
+ busid = usb_busaddr_to_busid(gc, usb->u.hostdev.hostbus,
+ usb->u.hostdev.hostaddr);
+ if (!busid) {
+ rc = ERROR_FAIL;
+ goto out;
+ }
+
+ buf = GCSPRINTF("%s:", busid);
+
+ if (!(dir = opendir(SYSFS_USB_DEV))) {
+ rc = ERROR_FAIL;
+ goto out;
+ }
+
+ while ((entry = readdir(dir)) != NULL) {
+ if (!strncmp(entry->d_name, buf, strlen(buf))) {
+ GCREALLOC_ARRAY(*intfs, *num + 1);
+ if (*intfs == NULL) {
+ rc = ERROR_FAIL;
+ goto out;
+ }
+ (*intfs)[*num] = libxl__strdup(gc, entry->d_name);
+ (*num)++;
+ }
+ }
+
+ closedir(dir);
+
+out:
+ 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(char *busid)
+{
+ char *str = strdup(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, libxl_device_usb *usb)
+{
+ char **intfs = NULL;
+ char *path;
+ int num = 0, i;
+ int rc = 0;
+ char *busid;
+ char *usb_encode = NULL;
+
+ if (usb_get_all_interfaces(gc, usb, &intfs, &num) < 0)
+ return ERROR_FAIL;
+
+ busid = usb_busaddr_to_busid(gc, usb->u.hostdev.hostbus,
+ usb->u.hostdev.hostaddr);
+ usb_encode = usb_interface_xenstore_encode(busid);
+
+ for (i = 0; i < num; i++) {
+ char *intf = intfs[i];
+ char *drvpath = NULL;
+
+ /* check if the USB interface is already bound to "usbbcak" */
+ if (usb_intf_is_assigned(gc, intf) > 0) {
+ /* unbind interface from usbback driver */
+ if (unbind_usb_intf(gc, intf, NULL) < 0) {
+ rc = ERROR_FAIL;
+ goto out;
+ }
+ }
+
+ /* bind interface to its originial driver */
+ drvpath = libxl__xs_read(gc, XBT_NULL,
+ GCSPRINTF(USBBACK_INFO_PATH"/%s/%s/driver_path",
+ usb_encode, usb_interface_xenstore_encode(intf)));
+ if (drvpath && bind_usb_intf(gc, intf, drvpath))
+ LOGE(WARN, "Couldn't bind %s to %s", intf, drvpath);
+ }
+
+ /* finally, remove xs driver path */
+ path = GCSPRINTF(USBBACK_INFO_PATH"/%s", usb_encode);
+ libxl__xs_rm_checked(gc, XBT_NULL, path);
+
+out:
+ free(usb_encode);
+ 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, libxl_device_usb *usb)
+{
+ char **intfs = NULL;
+ int num = 0, i;
+ int rc = 0;
+ char *busid;
+ char *usb_encode = NULL;
+
+ if (usb_get_all_interfaces(gc, usb, &intfs, &num) < 0)
+ return ERROR_FAIL;
+
+ busid = usb_busaddr_to_busid(gc, usb->u.hostdev.hostbus,
+ usb->u.hostdev.hostaddr);
+ usb_encode = usb_interface_xenstore_encode(busid);
+
+ for (i = 0; i < num; i++) {
+ char *intf = intfs[i];
+ char *path = NULL;
+ char *drvpath = NULL;
+
+ /* already assigned to usbback */
+ if (usb_intf_is_assigned(gc, intf) > 0)
+ continue;
+
+ /* unbind interface from original driver */
+ if (unbind_usb_intf(gc, intf, &drvpath) < 0) {
+ rc = ERROR_FAIL;
+ goto out_rebind;
+ }
+
+ if (drvpath) {
+ /* write driver path to xenstore for later rebinding */
+ path = GCSPRINTF(USBBACK_INFO_PATH"/%s/%s/driver_path",
+ usb_encode, usb_interface_xenstore_encode(intf));
+ if (libxl__xs_write_checked(gc, XBT_NULL, path, drvpath) < 0) {
+ LOG(WARN, "Write of %s to node %s failed", drvpath, path);
+ }
+ }
+
+ /* bind interface to usbback */
+ if (bind_usb_intf(gc, intf, SYSFS_USBBACK_DRIVER) < 0) {
+ LOGE(ERROR, "Couldn't bind %s to %s", intf, SYSFS_USBBACK_DRIVER);
+ rc = ERROR_FAIL;
+ goto out_rebind;
+ }
+ }
+
+ goto out;
+
+out_rebind:
+ /* some interfaces might be bound to usbback, unbind it then and
+ * rebind to its original driver
+ */
+ usbback_dev_unassign(gc, usb);
+out:
+ free(usb_encode);
+ 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.
+ */
+void libxl__device_usb_add(libxl__egc *egc, uint32_t domid,
+ libxl_device_usb *usb,
+ libxl__ao_device *aodev)
+{
+ STATE_AO_GC(aodev->ao);
+ int rc = -1;
+ char *busid = NULL;
+
+ assert(usb->u.hostdev.hostbus > 0 && usb->u.hostdev.hostaddr > 0);
+
+ busid = usb_busaddr_to_busid(gc, usb->u.hostdev.hostbus,
+ usb->u.hostdev.hostaddr);
+ if (!busid) {
+ LOG(ERROR, "USB device doesn't exist in sysfs");
+ goto out;
+ }
+
+ if (!is_usb_assignable(gc, usb)) {
+ LOG(ERROR, "USB device is not assignable.");
+ goto out;
+ }
+
+ /* check usb device is already assigned */
+ if (is_usb_assigned(gc, usb)) {
+ LOG(ERROR, "USB device is already attached to a domain.");
+ goto out;
+ }
+
+ rc = libxl__device_usb_setdefault(gc, domid, usb, aodev->update_json);
+ if (rc) goto out;
+
+ rc = libxl__device_usb_add_xenstore(gc, domid, usb, aodev->update_json);
+ if (rc) goto out;
+
+ rc = usbback_dev_assign(gc, usb);
+ if (rc) {
+ libxl__device_usb_remove_xenstore(gc, domid, usb);
+ goto out;
+ }
+
+ libxl__ao_complete(egc, ao, 0);
+ rc = 0;
+
+out:
+ aodev->rc = rc;
+ if (rc) aodev->callback(egc, aodev);
+ return;
+}
+
+/* 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_usb_remove(libxl__gc *gc, uint32_t domid,
+ libxl_device_usb *usb)
+{
+ if (libxl__device_usb_remove_xenstore(gc, domid, usb))
+ return -1;
+
+ usbback_dev_unassign(gc, usb);
+
+ return 0;
+}
+
+int libxl_device_usb_remove(libxl_ctx *ctx, uint32_t domid,
+ libxl_device_usb *usb,
+ const libxl_asyncop_how *ao_how)
+
+{
+ AO_CREATE(ctx, domid, ao_how);
+ int rc;
+
+ rc = libxl__device_usb_remove(gc, domid, usb);
+
+ libxl__ao_complete(egc, ao, rc);
+ return AO_INPROGRESS;
+}
+
+int libxl_ctrlport_to_device_usb(libxl_ctx *ctx,
+ uint32_t domid,
+ int ctrl,
+ int port,
+ libxl_device_usb *usb)
+{
+ GC_INIT(ctx);
+ char *dompath, *be_path, *busid;
+ int rc = ERROR_FAIL;
+
+ dompath = libxl__xs_get_dompath(gc, domid);
+ if (!dompath)
+ goto out;
+
+ be_path = libxl__xs_read(gc, XBT_NULL,
+ GCSPRINTF("%s/device/vusb/%d/backend", dompath, ctrl));
+ if (!be_path)
+ goto out;
+
+ busid = libxl__xs_read(gc, XBT_NULL,
+ GCSPRINTF("%s/port/%d", be_path, port));
+ if (busid && strcmp(busid, "")) {
+ usb->ctrl = ctrl;
+ usb->port = port;
+ usb_busaddr_from_busid(gc, busid, &usb->u.hostdev.hostbus,
+ &usb->u.hostdev.hostaddr);
+ rc = 0;
+ }
+
+out:
+ GC_FREE;
+ return rc;
+}
+
+int libxl_device_usb_getinfo(libxl_ctx *ctx, uint32_t domid,
+ libxl_device_usb *usb,
+ libxl_usbinfo *usbinfo)
+{
+ GC_INIT(ctx);
+ char *filename;
+ char *busid;
+ void *buf = NULL;
+ int buflen, rc;
+
+ usbinfo->ctrl = usb->ctrl;
+ usbinfo->port = usb->port;
+
+ if (libxl_ctrlport_to_device_usb(ctx, domid,
+ usb->ctrl, usb->port, usb) < 0) {
+ rc = ERROR_FAIL;
+ goto out;
+ }
+
+ usbinfo->devnum = usb->u.hostdev.hostaddr;
+ usbinfo->busnum = usb->u.hostdev.hostbus;
+
+ busid = usb_busaddr_to_busid(gc, usb->u.hostdev.hostbus,
+ usb->u.hostdev.hostaddr);
+ if (!busid) {
+ rc = ERROR_FAIL;
+ goto out;
+ }
+
+ filename = GCSPRINTF(SYSFS_USB_DEV"/%s/idVendor", busid);
+ if (!libxl_read_sysfs_file_contents(ctx, filename, &buf, NULL))
+ sscanf(buf, "%x", &usbinfo->idVendor);
+
+ filename = GCSPRINTF(SYSFS_USB_DEV"/%s/idProduct", busid);
+ if (!libxl_read_sysfs_file_contents(ctx, filename, &buf, NULL))
+ sscanf(buf, "%x", &usbinfo->idProduct);
+
+ filename = GCSPRINTF(SYSFS_USB_DEV"/%s/manufacturer", busid);
+ if (!libxl_read_sysfs_file_contents(ctx, filename, &buf, &buflen) &&
+ buflen > 0) {
+ /* replace \n to \0 */
+ if (((char *)buf)[buflen - 1] == '\n')
+ ((char *)buf)[buflen - 1] = '\0';
+ usbinfo->manuf = libxl__strdup(NOGC, buf);
+ }
+
+ filename = GCSPRINTF(SYSFS_USB_DEV"/%s/product", busid);
+ if (!libxl_read_sysfs_file_contents(ctx, filename, &buf, &buflen) &&
+ buflen > 0) {
+ /* replace \n to \0 */
+ if (((char *)buf)[buflen - 1] == '\n')
+ ((char *)buf)[buflen - 1] = '\0';
+ usbinfo->prod = libxl__strdup(NOGC, buf);
+ }
+
+ rc = 0;
+
+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 ef346e7..ef10484 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -594,6 +594,37 @@ libxl_device_rdm = Struct("device_rdm", [
("policy", libxl_rdm_reserve_policy),
])
+libxl_usbctrl_type = Enumeration("usbctrl_type", [
+ (0, "AUTO"),
+ (1, "PV"),
+ (2, "QEMU"),
+ ])
+
+libxl_usbdev_type = Enumeration("usbdev_type", [
+ (0, "invalid"),
+ (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_usb = Struct("device_usb", [
+ ("ctrl", libxl_devid),
+ ("port", integer),
+ ("u", KeyedUnion(None, libxl_usbdev_type, "devtype",
+ [("hostdev", Struct(None, [
+ ("hostbus", integer),
+ ("hostaddr", integer)])),
+ ("invalid", None),
+ ])),
+ ])
+
libxl_device_dtdev = Struct("device_dtdev", [
("path", string),
])
@@ -626,6 +657,8 @@ libxl_domain_config = Struct("domain_config", [
("pcidevs", Array(libxl_device_pci, "num_pcidevs")),
("rdms", Array(libxl_device_rdm, "num_rdms")),
("dtdevs", Array(libxl_device_dtdev, "num_dtdevs")),
+ ("usbctrls", Array(libxl_device_usbctrl, "num_usbctrls")),
+ ("usbs", Array(libxl_device_usb, "num_usbs")),
("vfbs", Array(libxl_device_vfb, "num_vfbs")),
("vkbs", Array(libxl_device_vkb, "num_vkbs")),
("vtpms", Array(libxl_device_vtpm, "num_vtpms")),
@@ -674,6 +707,32 @@ 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_usbinfo = Struct("usbinfo", [
+ ("ctrl", libxl_devid),
+ ("port", integer),
+ ("busnum", integer),
+ ("devnum", integer),
+ ("idVendor", integer),
+ ("idProduct", integer),
+ ("prod", string),
+ ("manuf", string),
+ ], 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 9234efb..b196203 100644
--- a/tools/libxl/libxl_utils.c
+++ b/tools/libxl/libxl_utils.c
@@ -1241,6 +1241,22 @@ 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_usb_list_free(libxl_device_usb* list, int nr)
+{
+ int i;
+ for (i = 0; i < nr; i++)
+ libxl_device_usb_dispose(&list[i]);
+ free(list);
+}
+
/*
* Local variables:
* mode: C
diff --git a/tools/libxl/libxl_utils.h b/tools/libxl/libxl_utils.h
index 1e5ca8a..ef049e3 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_usb(libxl_ctx *ctx, uint32_t domid,
+ int ctrl, int port,
+ libxl_device_usb *usb);
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] 41+ messages in thread
* [PATCH V6 4/7] libxl: add libxl_device_usb_assignable_list API
2015-08-10 10:35 [PATCH V6 0/7] xen pvusb toolstack work Chunyan Liu
` (2 preceding siblings ...)
2015-08-10 10:35 ` [PATCH V6 3/7] libxl: add pvusb API Chunyan Liu
@ 2015-08-10 10:35 ` Chunyan Liu
2015-08-10 10:35 ` [PATCH V6 5/7] xl: add pvusb commands Chunyan Liu
` (2 subsequent siblings)
6 siblings, 0 replies; 41+ messages in thread
From: Chunyan Liu @ 2015-08-10 10:35 UTC (permalink / raw)
To: xen-devel
Cc: jgross, wei.liu2, ian.campbell, george.dunlap, Ian.Jackson,
Chunyan Liu, jfehlig
Add API for listing assignable USB devices info.
Assignable USB device means the USB device type is assignable and
it's not assigned to any guest yet.
Signed-off-by: Chunyan Liu <cyliu@suse.com>
---
This could be squashed with previous patch. Split because there is
some dispute on this. If this is acceptable, could be squashed,
otherwise could be removed.
tools/libxl/libxl.h | 3 +++
tools/libxl/libxl_pvusb.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 56 insertions(+)
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 05b6331..d1360ce 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -1433,6 +1433,9 @@ int libxl_device_usbctrl_getinfo(libxl_ctx *ctx, uint32_t domid,
libxl_usbctrlinfo *usbctrlinfo);
/* USB Devices */
+libxl_device_usb *
+libxl_device_usb_assignable_list(libxl_ctx *ctx, int *num);
+
int libxl_device_usb_add(libxl_ctx *ctx, uint32_t domid, libxl_device_usb *usb,
const libxl_asyncop_how *ao_how)
LIBXL_EXTERNAL_CALLERS_ONLY;
diff --git a/tools/libxl/libxl_pvusb.c b/tools/libxl/libxl_pvusb.c
index d4c4c03..e56fa07 100644
--- a/tools/libxl/libxl_pvusb.c
+++ b/tools/libxl/libxl_pvusb.c
@@ -552,6 +552,59 @@ static bool is_usb_assignable(libxl__gc *gc, libxl_device_usb *usb)
return classcode != USBHUB_CLASS_CODE;
}
+libxl_device_usb *
+libxl_device_usb_assignable_list(libxl_ctx *ctx, int *num)
+{
+ GC_INIT(ctx);
+ libxl_device_usb *usbs = NULL;
+ libxl_device_usb *assigned;
+ int num_assigned;
+ struct dirent *de;
+ DIR *dir;
+
+ *num = 0;
+
+ if (libxl__device_usb_assigned_list(gc, &assigned, &num_assigned) < 0)
+ goto out;
+
+ if (!(dir = opendir(SYSFS_USB_DEV)))
+ goto out;
+
+ while ((de = readdir(dir))) {
+ libxl_device_usb *usb;
+ int bus = -1, addr = -1;
+
+ if (!de->d_name)
+ continue;
+
+ usb_busaddr_from_busid(gc, de->d_name, &bus, &addr);
+ if (bus < 1 || addr < 1)
+ continue;
+
+ GCNEW(usb);
+ usb->u.hostdev.hostbus = bus;
+ usb->u.hostdev.hostaddr = addr;
+
+ if (!is_usb_assignable(gc, usb))
+ continue;
+
+ if (is_usb_in_array(assigned, num_assigned, usb))
+ continue;
+
+ usbs = libxl__realloc(NOGC, usbs, sizeof(*usbs) * (*num + 1));
+ libxl_device_usb_init(usbs + *num);
+ usbs[*num].u.hostdev.hostbus = bus;
+ usbs[*num].u.hostdev.hostaddr = addr;
+ (*num)++;
+ }
+
+ closedir(dir);
+
+out:
+ GC_FREE;
+ return usbs;
+}
+
/* get usb devices under certain usb controller */
static int
libxl__device_usb_list_per_usbctrl(libxl__gc *gc, uint32_t domid,
--
2.1.4
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH V6 5/7] xl: add pvusb commands
2015-08-10 10:35 [PATCH V6 0/7] xen pvusb toolstack work Chunyan Liu
` (3 preceding siblings ...)
2015-08-10 10:35 ` [PATCH V6 4/7] libxl: add libxl_device_usb_assignable_list API Chunyan Liu
@ 2015-08-10 10:35 ` Chunyan Liu
2015-08-10 10:35 ` [PATCH V6 6/7] xl: add usb-assignable-list command Chunyan Liu
2015-08-10 10:35 ` [PATCH V6 7/7] domcreate: support pvusb in configuration file Chunyan Liu
6 siblings, 0 replies; 41+ messages in thread
From: Chunyan Liu @ 2015-08-10 10:35 UTC (permalink / raw)
To: xen-devel
Cc: jgross, wei.liu2, ian.campbell, george.dunlap, Ian.Jackson,
Chunyan Liu, jfehlig, Simon Cao
Add pvusb commands: usb-ctrl-attach, usb-ctrl-detach, usb-list,
usb-attach and usb-detach.
To attach a usb device to guest through pvusb, one could follow
following example:
#xl usb-ctrl-attach test_vm version=1 num_ports=8
#xl usb-list test_vm
will show the usb controllers and port usage under the domain.
#xl usb-attach test_vm 1.6
will find the first usable controller:port, and attach usb
device whose bus address is 1.6 (busnum is 1, devnum is 6)
to it. One could also specify which <controller> and which <port>.
#xl usb-detach test_vm 0 1
will detach USB device under controller 0 port 1.
#xl usb-ctrl-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>
---
docs/man/xl.pod.1 | 40 ++++++++
tools/libxl/xl.h | 5 +
tools/libxl/xl_cmdimpl.c | 230 ++++++++++++++++++++++++++++++++++++++++++++++
tools/libxl/xl_cmdtable.c | 25 +++++
4 files changed, 300 insertions(+)
diff --git a/docs/man/xl.pod.1 b/docs/man/xl.pod.1
index f22c3f3..4c92c78 100644
--- a/docs/man/xl.pod.1
+++ b/docs/man/xl.pod.1
@@ -1345,6 +1345,46 @@ List pass-through pci devices for a domain.
=back
+=head1 USB PASS-THROUGH
+
+=over 4
+
+=item B<usb-ctrl-attach> I<domain-id> I[<type=val>] [I<version=val>] [I<ports=number>]
+
+Create a new USB controller for the specified domain.
+B<type=val> is the usb controller type, currently only support 'pv'.
+B<version=val> is the usb controller version, could be 1 (USB1.1) or 2 (USB2.0).
+B<ports=number> is the total ports of the usb controller.
+By default, it will create a USB2.0 controller with 8 ports.
+
+=item B<usb-ctrl-detach> I<domain-id> I<devid>
+
+Destroy a USB controller from the specified domain.
+B<devid> is devid of the USB controller.
+
+If B<-f> is specified, B<xl> is going to forcefully remove the device even
+without guest's collaboration.
+
+=item B<usb-attach> I<domain-id> I<bus.addr> [I<controller=devid> [I<port=number>]]
+
+Hot-plug a new pass-through USB device to the specified domain.
+B<bus.addr> is the busnum.devnum of the physical USB device to pass-through.
+B<controller=devid> B<port=number> is the USB controller:port to hotplug the
+USB device to. By default, it will find the first available controller:port
+and use it; if there is no controller, it will create one.
+
+=item B<usb-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 13bccba..e136fdf 100644
--- a/tools/libxl/xl.h
+++ b/tools/libxl/xl.h
@@ -85,6 +85,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_usbattach(int argc, char **argv);
+int main_usbdetach(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 499a05c..3e4d93a 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -3322,6 +3322,236 @@ int main_cd_insert(int argc, char **argv)
return 0;
}
+int main_usbctrl_attach(int argc, char **argv)
+{
+ uint32_t domid;
+ int opt, rc = 1;
+ char *oparg;
+ libxl_device_usbctrl usbctrl;
+
+ SWITCH_FOREACH_OPT(opt, "", NULL, "usb-ctrl-attach", 1) {
+ /* No options */
+ }
+
+ domid = find_domain(argv[optind++]);
+
+ libxl_device_usbctrl_init(&usbctrl);
+
+ while (argc > optind) {
+ if (MATCH_OPTION("type", argv[optind], oparg)) {
+ if (!strcmp(oparg, "pv")) {
+ usbctrl.type = LIBXL_USBCTRL_TYPE_PV;
+ } else {
+ fprintf(stderr, "unsupported type `%s'\n", oparg);
+ goto out;
+ }
+ } else if (MATCH_OPTION("version", argv[optind], oparg)) {
+ usbctrl.version = atoi(oparg);
+ if (usbctrl.version != 1 && usbctrl.version != 2) {
+ fprintf(stderr, "unsupported version `%s'\n", oparg);
+ goto out;
+ }
+ } else if (MATCH_OPTION("ports", argv[optind], oparg)) {
+ usbctrl.ports = atoi(oparg);
+ if (usbctrl.ports < 1 || usbctrl.ports > 31) {
+ fprintf(stderr, "unsupported ports `%s'\n", oparg);
+ goto out;
+ }
+ } else {
+ fprintf(stderr, "unrecognized argument `%s'\n", argv[optind]);
+ goto out;
+ }
+ optind++;
+ }
+
+ rc = libxl_device_usbctrl_add(ctx, domid, &usbctrl, 0);
+ if (rc)
+ fprintf(stderr, "libxl_device_usbctrl_add failed.\n");
+
+out:
+ 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, "usb-ctrl-detach", 2) {
+ /* No options */
+ }
+
+ domid = find_domain(argv[optind]);
+ devid = atoi(argv[optind+1]);
+ 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");
+
+ libxl_device_usbctrl_dispose(&usbctrl);
+ return rc;
+
+}
+
+int main_usbattach(int argc, char **argv)
+{
+ uint32_t domid;
+ char *devname, *p;
+ int opt, rc = 1;
+ char *oparg;
+ libxl_device_usb usb;
+
+ SWITCH_FOREACH_OPT(opt, "", NULL, "usb-attach", 2) {
+ /* No options */
+ }
+
+ libxl_device_usb_init(&usb);
+
+ domid = find_domain(argv[optind++]);
+ devname = argv[optind++];
+ p = strchr(devname, '.');
+ if (p) {
+ usb.u.hostdev.hostbus = strtoul(devname, NULL, 0);
+ usb.u.hostdev.hostaddr = strtoul(p + 1, NULL, 0);
+ }
+
+ if (usb.u.hostdev.hostbus < 1 || usb.u.hostdev.hostaddr < 1) {
+ fprintf(stderr, "Invalid usb device.\n");
+ goto out;
+ }
+
+ while (argc > optind) {
+ if (MATCH_OPTION("controller", argv[optind], oparg)) {
+ usb.ctrl = atoi(oparg);
+ } else if (MATCH_OPTION("port", argv[optind], oparg)) {
+ usb.port = atoi(oparg);
+ } else {
+ fprintf(stderr, "unrecognized argument `%s'\n", argv[optind]);
+ goto out;
+ }
+ optind++;
+ }
+
+ rc = libxl_device_usb_add(ctx, domid, &usb, 0);
+ if (rc)
+ fprintf(stderr, "libxl_device_usb_add failed.\n");
+
+out:
+ libxl_device_usb_dispose(&usb);
+ return rc;
+}
+
+int main_usbdetach(int argc, char **argv)
+{
+ uint32_t domid;
+ int ctrl, port;
+ int opt, rc = 1;
+ libxl_device_usb usb;
+
+ SWITCH_FOREACH_OPT(opt, "", NULL, "usb-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");
+ goto out;
+ }
+
+ libxl_device_usb_init(&usb);
+ if (libxl_ctrlport_to_device_usb(ctx, domid, ctrl, port, &usb)) {
+ fprintf(stderr, "Unknown device at controller %d port %d.\n",
+ ctrl, port);
+ goto out;
+ }
+
+ rc = libxl_device_usb_remove(ctx, domid, &usb, 0);
+ if (rc)
+ fprintf(stderr, "libxl_device_usb_remove failed.\n");
+
+out:
+ libxl_device_usb_dispose(&usb);
+ 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 %-30s\n",
+ "Devid", "Type", "BE", "state", "usb-ver", "ports", "BE-path");
+
+ libxl_usbctrlinfo_init(&usbctrlinfo);
+
+ if (!libxl_device_usbctrl_getinfo(ctx, domid,
+ &usbctrls[i], &usbctrlinfo)) {
+ printf("%-6d %-6s %-3d %-5d %-7d %-5d %-30s\n",
+ usbctrlinfo.devid,
+ libxl_usbctrl_type_to_string(usbctrlinfo.type),
+ usbctrlinfo.backend_id, usbctrlinfo.state,
+ usbctrlinfo.version, usbctrlinfo.ports,
+ usbctrlinfo.backend);
+
+ for (j = 1; j <= usbctrlinfo.ports; j++) {
+ libxl_device_usb usb;
+ libxl_usbinfo usbinfo;
+
+ libxl_device_usb_init(&usb);
+ libxl_usbinfo_init(&usbinfo);
+
+ printf(" Port %d:", j);
+
+ usb.ctrl = usbctrlinfo.devid;
+ usb.port = j;
+ if (!libxl_device_usb_getinfo(ctx, domid, &usb, &usbinfo)) {
+ printf(" Bus %03x Device %03x: ID %04x:%04x %s %s\n",
+ usbinfo.busnum, usbinfo.devnum,
+ usbinfo.idVendor, usbinfo.idProduct,
+ usbinfo.manuf ?: "", usbinfo.prod ?: "");
+ } else {
+ printf("\n");
+ }
+ libxl_usbinfo_dispose(&usbinfo);
+ libxl_device_usb_dispose(&usb);
+ }
+ }
+
+ 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 0071f12..46f276e 100644
--- a/tools/libxl/xl_cmdtable.c
+++ b/tools/libxl/xl_cmdtable.c
@@ -551,6 +551,31 @@ struct cmd_spec cmd_table[] = {
},
#endif
+ { "usb-ctrl-attach",
+ &main_usbctrl_attach, 0, 1,
+ "Create a virtual USB controller for a domain",
+ "<Domain> [type=pv] [version=<version>] [ports=<number>]",
+ },
+ { "usb-ctrl-detach",
+ &main_usbctrl_detach, 0, 1,
+ "Remove the virtual USB controller specified by <DevId> for a domain",
+ "<Domain> <DevId>",
+ },
+ { "usb-attach",
+ &main_usbattach, 0, 1,
+ "Attach a USB device to a domain",
+ "<Domain> <bus.addr> [controller=<DevId> [port=<port>]]",
+ },
+ { "usb-detach",
+ &main_usbdetach, 0, 1,
+ "Detach a USB device from a domain",
+ "<Domain> <controller> <port>",
+ },
+ { "usb-list",
+ &main_usblist, 0, 0,
+ "List information about USB devices for a domain",
+ "<Domain>",
+ },
};
int cmdtable_len = sizeof(cmd_table)/sizeof(struct cmd_spec);
--
2.1.4
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH V6 6/7] xl: add usb-assignable-list command
2015-08-10 10:35 [PATCH V6 0/7] xen pvusb toolstack work Chunyan Liu
` (4 preceding siblings ...)
2015-08-10 10:35 ` [PATCH V6 5/7] xl: add pvusb commands Chunyan Liu
@ 2015-08-10 10:35 ` Chunyan Liu
2015-08-10 10:35 ` [PATCH V6 7/7] domcreate: support pvusb in configuration file Chunyan Liu
6 siblings, 0 replies; 41+ messages in thread
From: Chunyan Liu @ 2015-08-10 10:35 UTC (permalink / raw)
To: xen-devel
Cc: jgross, wei.liu2, ian.campbell, george.dunlap, Ian.Jackson,
Chunyan Liu, jfehlig
Add xl usb-assignable-list command to list assignable USB devices.
Assignable USB device means the USB device type is assignable and
it's not assigned to any guest yet.
Signed-off-by: Chunyan Liu <cyliu@suse.com>
---
Same as "libxl: add libxl_device_usb_assignable_list API" patch,
this patch could be sqaushed to previous one. Split because of
some dispute. Could be squashed if acceptable, otherwise could
be removed.
tools/libxl/xl.h | 1 +
tools/libxl/xl_cmdimpl.c | 27 +++++++++++++++++++++++++++
tools/libxl/xl_cmdtable.c | 4 ++++
3 files changed, 32 insertions(+)
diff --git a/tools/libxl/xl.h b/tools/libxl/xl.h
index e136fdf..e579ecc 100644
--- a/tools/libxl/xl.h
+++ b/tools/libxl/xl.h
@@ -85,6 +85,7 @@ 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_usbassignable_list(int argc, char **argv);
int main_usbctrl_attach(int argc, char **argv);
int main_usbctrl_detach(int argc, char **argv);
int main_usbattach(int argc, char **argv);
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 3e4d93a..e33871c 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -3322,6 +3322,33 @@ int main_cd_insert(int argc, char **argv)
return 0;
}
+static void usb_assignable_list(void)
+{
+ libxl_device_usb *usbs;
+ int num, i;
+
+ usbs = libxl_device_usb_assignable_list(ctx, &num);
+
+ for (i = 0; i < num; i++) {
+ printf("%d.%d\n", usbs[i].u.hostdev.hostbus,
+ usbs[i].u.hostdev.hostaddr);
+ }
+
+ libxl_device_usb_list_free(usbs, num);
+}
+
+int main_usbassignable_list(int argc, char **argv)
+{
+ int opt;
+
+ SWITCH_FOREACH_OPT(opt, "", NULL, "usb-assignable-list", 0) {
+ /* No options */
+ }
+
+ usb_assignable_list();
+ return 0;
+}
+
int main_usbctrl_attach(int argc, char **argv)
{
uint32_t domid;
diff --git a/tools/libxl/xl_cmdtable.c b/tools/libxl/xl_cmdtable.c
index 46f276e..ba51331 100644
--- a/tools/libxl/xl_cmdtable.c
+++ b/tools/libxl/xl_cmdtable.c
@@ -576,6 +576,10 @@ struct cmd_spec cmd_table[] = {
"List information about USB devices for a domain",
"<Domain>",
},
+ { "usb-assignable-list",
+ &main_usbassignable_list, 0, 0,
+ "List all assignable USB devices",
+ },
};
int cmdtable_len = sizeof(cmd_table)/sizeof(struct cmd_spec);
--
2.1.4
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH V6 7/7] domcreate: support pvusb in configuration file
2015-08-10 10:35 [PATCH V6 0/7] xen pvusb toolstack work Chunyan Liu
` (5 preceding siblings ...)
2015-08-10 10:35 ` [PATCH V6 6/7] xl: add usb-assignable-list command Chunyan Liu
@ 2015-08-10 10:35 ` Chunyan Liu
2015-08-11 11:27 ` Wei Liu
6 siblings, 1 reply; 41+ messages in thread
From: Chunyan Liu @ 2015-08-10 10:35 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=['2.1,controller=0,port=1', ]
Signed-off-by: Chunyan Liu <cyliu@suse.com>
Signed-off-by: Simon Cao <caobosimon@gmail.com>
---
docs/man/xl.cfg.pod.5 | 75 +++++++++++++++++++++++++++++
tools/libxl/libxl_create.c | 73 ++++++++++++++++++++++++++--
tools/libxl/libxl_device.c | 4 ++
tools/libxl/libxl_internal.h | 8 ++++
tools/libxl/xl_cmdimpl.c | 112 ++++++++++++++++++++++++++++++++++++++++++-
5 files changed, 268 insertions(+), 4 deletions(-)
diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
index 80e51bb..45f3ff3 100644
--- a/docs/man/xl.cfg.pod.5
+++ b/docs/man/xl.cfg.pod.5
@@ -709,6 +709,81 @@ 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 protocol to implement USB controller, could be "pv" (indicates
+PVUSB) or "qemu" (indicates QEMU emulated). Currently only "pv" is supported.
+
+=item B<version=VERSION>
+
+Specifies version of the USB controller, could be 1 (USB1.1) or 2 (USB2.0).
+Default is 2 (USB2.0).
+
+=item B<ports=PORTS>
+
+Specifies port number of the USB controller. Default is 8.
+
+Each USB controller will have an index starting from 0. On the same
+controller, each port will have an index starting from 1.
+
+E.g.
+usbctrl=["version=1,ports=4", "version=2,ports=8",]
+The first controller has:
+controller index = 0, and port 1,2,3,4.
+The second controller has:
+controller index = 1, and port 1,2,3,4,5,6,7,8.
+
+=back
+
+=back
+
+=item B<usbdev=[ "USB_SPEC_STRING", "USB_SPEC_STRING", ... ]>
+
+Specifies the host USB devices to passthrough to this guest. Each
+B<USB_SPEC_STRING> has the form C<bus.addr,KEY=VALUE,KEY=VALUE,...> where:
+
+=over 4
+
+=item B<bus.addr>
+
+Identifies the busnum.devnum of the USB device from the host perspective.
+This is the same scheme as used in the output of C<lsusb> for the device in
+question.
+
+=item B<KEY=VALUE>
+
+Possible B<KEY>s are:
+
+=over 4
+
+=item B<controller=CONTROLLER>
+
+Specifies USB controller index, to which controller the USB device is attached.
+
+=item B<port=PORT>
+
+Specifies USB port index, to which port the USB device is attached. B<port=PORT>
+is valid only when B<controller=CONTROLLER> is specified. Without
+B<controller=CONTROLLER>, it will find the first available USB controller:port
+and use it. If there is no controller at all, it will create one.
+
+=back
+
+=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 2348ffc..2988991 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -729,6 +729,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_usbs(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,
@@ -1385,13 +1389,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:
@@ -1399,6 +1403,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_usbs;
+ libxl__add_usbctrls(egc, ao, domid, d_config, &dcs->multidev);
+ libxl__multidev_prepared(egc, &dcs->multidev, 0);
+ return;
+ }
+
+ domcreate_attach_usbs(egc, multidev, 0);
+ return;
+
+error_out:
+ assert(ret);
+ domcreate_complete(egc, dcs, ret);
+}
+
+
+static void domcreate_attach_usbs(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_usbs > 0) {
+ /* Attach usbctrls */
+ libxl__multidev_begin(ao, &dcs->multidev);
+ dcs->multidev.callback = domcreate_attach_pci;
+ libxl__add_usbs(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)
{
@@ -1412,7 +1479,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 935f25b..92d5d10 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(usb)
#undef DEFINE_DEVICES_ADD
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 5be3b3a..c23741a 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -3283,6 +3283,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_usbs(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 e33871c..632d32f 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1226,6 +1226,79 @@ static void parse_vnuma_config(const XLU_Config *config,
free(vcpu_parsed);
}
+static void parse_usbctrl_config(libxl_device_usbctrl *usbctrl,
+ const char *buf)
+{
+ char *buf2 = strdup(buf);
+ char *p, *p2;
+
+ p = strtok(buf2, ",");
+ if (!p)
+ goto out;
+ do {
+ while (*p == ' ')
+ p++;
+ if ((p2 = strchr(p, '=')) == NULL)
+ break;
+ *p2 = '\0';
+ if (!strcmp(p, "type")) {
+ if (!strcmp(p2 + 1, "pv")) {
+ usbctrl->type = LIBXL_USBCTRL_TYPE_PV;
+ } else {
+ fprintf(stderr,
+ "Unsupported USB controller type '%s'\n",
+ p2 + 1);
+ exit(1);
+ }
+ } else if (!strcmp(p, "version")){
+ usbctrl->version = atoi(p2 + 1);
+ } else if (!strcmp(p, "ports")){
+ usbctrl->ports = atoi(p2 + 1);
+ } else {
+ fprintf(stderr, "Unknown string `%s' in usb spec\n", p);
+ exit(1);
+ }
+ } while ((p = strtok(NULL, ",")) != NULL);
+
+out:
+ free(buf2);
+}
+
+static void parse_usb_config(libxl_device_usb *usb, const char *buf)
+{
+ char *buf2 = strdup(buf);
+ char *p, *p2;
+
+ p = strtok(buf2, ",");
+ if (!p)
+ goto out;
+ do {
+ while(*p == ' ')
+ ++p;
+ if ((p2 = strchr(p, '=')) == NULL) {
+ char *busaddr = p;
+ p = strchr(busaddr, '.');
+ if (p) {
+ usb->u.hostdev.hostbus = strtoul(busaddr, NULL, 0);
+ usb->u.hostdev.hostaddr = strtoul(p + 1, NULL, 0);
+ }
+ continue;
+ }
+ *p2 = '\0';
+ if (!strcmp(p, "controller")) {
+ usb->ctrl = atoi(p2 + 1);
+ } else if (!strcmp(p, "port")) {
+ usb->port = atoi(p2 + 1);
+ } else {
+ fprintf(stderr, "Unknown string `%s' in usb spec\n", p);
+ exit(1);
+ }
+ } while ((p = strtok(NULL, ",")) != NULL);
+
+out:
+ free(buf2);
+}
+
static void parse_config_data(const char *config_source,
const char *config_data,
int config_len,
@@ -1234,7 +1307,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, *usbs;
XLU_ConfigList *channels, *ioports, *irqs, *iomem, *viridian, *dtdevs;
int num_ioports, num_irqs, num_iomem, num_cpus, num_viridian;
int pci_power_mgmt = 0;
@@ -2042,6 +2116,42 @@ 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;
+
+ d_config->usbctrls =
+ (libxl_device_usbctrl *)realloc(d_config->usbctrls,
+ sizeof(libxl_device_usbctrl) * (d_config->num_usbctrls + 1));
+ usbctrl = d_config->usbctrls + d_config->num_usbctrls;
+ libxl_device_usbctrl_init(usbctrl);
+
+ parse_usbctrl_config(usbctrl, buf);
+
+ d_config->num_usbctrls++;
+ }
+ }
+
+ if (!xlu_cfg_get_list(config, "usbdev", &usbs, 0, 0)) {
+ d_config->num_usbs = 0;
+ d_config->usbs = NULL;
+ while ((buf = xlu_cfg_get_listitem(usbs, d_config->num_usbs)) != NULL) {
+ libxl_device_usb *usb;
+
+ d_config->usbs = (libxl_device_usb *)realloc(d_config->usbs,
+ sizeof(libxl_device_usb) * (d_config->num_usbs + 1));
+ usb = d_config->usbs + d_config->num_usbs;
+ libxl_device_usb_init(usb);
+
+ parse_usb_config(usb, buf);
+
+ d_config->num_usbs++;
+ }
+ }
+
switch (xlu_cfg_get_list(config, "cpuid", &cpuids, 0, 1)) {
case 0:
{
--
2.1.4
^ permalink raw reply related [flat|nested] 41+ messages in thread
* Re: [PATCH V6 1/7] libxl: export some functions for pvusb use
2015-08-10 10:35 ` [PATCH V6 1/7] libxl: export some functions for pvusb use Chunyan Liu
@ 2015-08-11 11:26 ` Wei Liu
0 siblings, 0 replies; 41+ messages in thread
From: Wei Liu @ 2015-08-11 11:26 UTC (permalink / raw)
To: Chunyan Liu
Cc: jgross, wei.liu2, ian.campbell, george.dunlap, Ian.Jackson,
xen-devel, jfehlig, Simon Cao
On Mon, Aug 10, 2015 at 06:35:22PM +0800, Chunyan Liu wrote:
> Signed-off-by: Chunyan Liu <cyliu@suse.com>
> Signed-off-by: Simon Cao <caobosimon@gmail.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
>
> ---
> tools/libxl/libxl.c | 4 ++--
> tools/libxl/libxl_internal.h | 3 +++
> 2 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 083f099..006e8da 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -1995,7 +1995,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;
> @@ -2014,7 +2014,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,
> +int libxl__resolve_domid(libxl__gc *gc, const char *name,
> uint32_t *domid)
Nit: please adjust indentation.
> {
> if (!name)
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index 6ea6c83..6013628 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -1152,6 +1152,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 [flat|nested] 41+ messages in thread
* Re: [PATCH V6 2/7] libxl_read_file_contents: add new entry to read sysfs file
2015-08-10 10:35 ` [PATCH V6 2/7] libxl_read_file_contents: add new entry to read sysfs file Chunyan Liu
@ 2015-08-11 11:26 ` Wei Liu
2015-08-12 2:37 ` Chun Yan Liu
0 siblings, 1 reply; 41+ messages in thread
From: Wei Liu @ 2015-08-11 11:26 UTC (permalink / raw)
To: Chunyan Liu
Cc: jgross, wei.liu2, ian.campbell, george.dunlap, Ian.Jackson,
xen-devel, jfehlig
On Mon, Aug 10, 2015 at 06:35:23PM +0800, Chunyan Liu wrote:
> Sysfs file has size=4096 but actual file content is less than that.
> Current libxl_read_file_contents will treat it as error when file size
> and actual file content differs, so reading sysfs file content with
> this function always fails.
>
> Add a new entry 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>
>
> ---
> Changes:
> - read one more byte to check bigger size problem.
>
> tools/libxl/libxl_internal.h | 2 ++
> tools/libxl/libxl_utils.c | 51 ++++++++++++++++++++++++++++++++++----------
> 2 files changed, 42 insertions(+), 11 deletions(-)
>
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index 6013628..f98f089 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -4001,6 +4001,8 @@ void libxl__bitmap_copy_best_effort(libxl__gc *gc, libxl_bitmap *dptr,
>
> int libxl__count_physical_sockets(libxl__gc *gc, int *sockets);
> #endif
> +_hidden int libxl_read_sysfs_file_contents(libxl_ctx *ctx, const char *filename,
> + void **data_r, int *datalen_r);
Indentation looks wrong.
>
> /*
> * Local variables:
> diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c
> index bfc9699..9234efb 100644
> --- a/tools/libxl/libxl_utils.c
> +++ b/tools/libxl/libxl_utils.c
> @@ -322,8 +322,10 @@ out:
> return rc;
> }
>
> -int libxl_read_file_contents(libxl_ctx *ctx, const char *filename,
> - void **data_r, int *datalen_r) {
> +static int libxl_read_file_contents_core(libxl_ctx *ctx, const char *filename,
> + void **data_r, int *datalen_r,
> + bool tolerate_shrinking_file)
> +{
> GC_INIT(ctx);
> FILE *f = 0;
> uint8_t *data = 0;
> @@ -359,20 +361,34 @@ int libxl_read_file_contents(libxl_ctx *ctx, const char *filename,
> datalen = stab.st_size;
>
> if (stab.st_size && data_r) {
> - data = malloc(datalen);
> + data = malloc(datalen + 1);
> if (!data) goto xe;
>
> - rs = fread(data, 1, datalen, f);
> - if (rs != datalen) {
> - if (ferror(f))
> + rs = fread(data, 1, datalen + 1, f);
> + if (rs > datalen) {
> + LOG(ERROR, "%s increased size while we were reading it",
> + filename);
> + goto xe;
> + }
> +
> + if (rs < datalen) {
> + if (ferror(f)) {
> LOGE(ERROR, "failed to read %s", filename);
> - else if (feof(f))
> - LOG(ERROR, "%s changed size while we were reading it",
> - filename);
> - else
> + goto xe;
> + } else if (feof(f)) {
> + if (tolerate_shrinking_file) {
> + datalen = rs;
> + } else {
> + LOG(ERROR, "%s shrunk size while we were reading it",
> + filename);
> + goto xe;
> + }
> + } else {
> abort();
> - goto xe;
> + }
This is a bit bikeshedding, but you can leave "goto xe" out of two `if'
to reduce patch size.
> }
> +
> + data = realloc(data, datalen);
Should check return value of realloc.
The logic of this function reflects what has been discussed so far.
Wei.
> }
>
> if (fclose(f)) {
> @@ -396,6 +412,19 @@ int libxl_read_file_contents(libxl_ctx *ctx, const char *filename,
> return e;
> }
>
> +int libxl_read_file_contents(libxl_ctx *ctx, const char *filename,
> + void **data_r, int *datalen_r)
> +{
> + return libxl_read_file_contents_core(ctx, filename, data_r, datalen_r, 0);
> +}
> +
> +int libxl_read_sysfs_file_contents(libxl_ctx *ctx, const char *filename,
> + void **data_r, int *datalen_r)
> +{
> + return libxl_read_file_contents_core(ctx, filename, data_r, datalen_r, 1);
> +}
> +
> +
> #define READ_WRITE_EXACTLY(rw, zero_is_eof, constdata) \
> \
> int libxl_##rw##_exactly(libxl_ctx *ctx, int fd, \
> --
> 2.1.4
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH V6 3/7] libxl: add pvusb API
2015-08-10 10:35 ` [PATCH V6 3/7] libxl: add pvusb API Chunyan Liu
@ 2015-08-11 11:27 ` Wei Liu
2015-08-12 2:24 ` Chun Yan Liu
2015-08-31 6:10 ` Chun Yan Liu
2015-09-08 14:17 ` Ian Campbell
1 sibling, 2 replies; 41+ messages in thread
From: Wei Liu @ 2015-08-11 11:27 UTC (permalink / raw)
To: Chunyan Liu
Cc: jgross, wei.liu2, ian.campbell, george.dunlap, Ian.Jackson,
xen-devel, jfehlig, Simon Cao
On Mon, Aug 10, 2015 at 06:35:24PM +0800, Chunyan Liu wrote:
> 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>
>
> ---
> changes:
> - Address George's comments:
> * Update libxl_device_usb_getinfo to read ctrl/port only and
> get other information.
> * Update backend path according to xenstore frontend 'xxx/backend'
> entry instead of using TOOLSTACK_DOMID.
> * Use 'type' to indicate qemu/pv instead of previous naming 'protocol'.
> * Add USB 'devtype' union, currently only includes "hostdev"
>
I will leave this to Ian and George since they had strong opinions on
this.
I only skimmed this patch. Some comments below.
[...]
> +
> +int libxl_device_usb_getinfo(libxl_ctx *ctx, uint32_t domid,
> + libxl_device_usb *usb,
> + libxl_usbinfo *usbinfo);
> +
> /* 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 bee5ed5..935f25b 100644
> --- a/tools/libxl/libxl_device.c
> +++ b/tools/libxl/libxl_device.c
> @@ -676,6 +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;
> + if (dev->backend_kind == LIBXL__DEVICE_KIND_VUSB) {
> + libxl__initiate_device_usbctrl_remove(egc, aodev);
> + continue;
> + }
Is there a risk that this races with individual device removal? I think
you get away with it because removal of individual device is idempotent?
> libxl__initiate_device_remove(egc, aodev);
> }
> }
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index f98f089..5be3b3a 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -2553,6 +2553,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_usb_add(libxl__egc *egc, uint32_t domid,
> + libxl_device_usb *usb,
> + 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);
> @@ -2585,6 +2593,13 @@ _hidden void libxl__wait_device_connection(libxl__egc*,
> _hidden void libxl__initiate_device_remove(libxl__egc *egc,
> libxl__ao_device *aodev);
>
> +_hidden int libxl__device_from_usbctrl(libxl__gc *gc, uint32_t domid,
[...]
> +void libxl__device_usb_add(libxl__egc *egc, uint32_t domid,
> + libxl_device_usb *usb,
> + libxl__ao_device *aodev)
> +{
> + STATE_AO_GC(aodev->ao);
> + int rc = -1;
> + char *busid = NULL;
> +
> + assert(usb->u.hostdev.hostbus > 0 && usb->u.hostdev.hostaddr > 0);
> +
> + busid = usb_busaddr_to_busid(gc, usb->u.hostdev.hostbus,
> + usb->u.hostdev.hostaddr);
> + if (!busid) {
> + LOG(ERROR, "USB device doesn't exist in sysfs");
> + goto out;
> + }
> +
> + if (!is_usb_assignable(gc, usb)) {
> + LOG(ERROR, "USB device is not assignable.");
> + goto out;
> + }
> +
> + /* check usb device is already assigned */
> + if (is_usb_assigned(gc, usb)) {
> + LOG(ERROR, "USB device is already attached to a domain.");
> + goto out;
> + }
> +
> + rc = libxl__device_usb_setdefault(gc, domid, usb, aodev->update_json);
> + if (rc) goto out;
> +
> + rc = libxl__device_usb_add_xenstore(gc, domid, usb, aodev->update_json);
> + if (rc) goto out;
> +
> + rc = usbback_dev_assign(gc, usb);
> + if (rc) {
> + libxl__device_usb_remove_xenstore(gc, domid, usb);
> + goto out;
> + }
> +
> + libxl__ao_complete(egc, ao, 0);
> + rc = 0;
> +
> +out:
You forget to complete ao in failure path.
But I'm not very familiar with the AO machinery, I will let Ian comment
on this.
Wei.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH V6 7/7] domcreate: support pvusb in configuration file
2015-08-10 10:35 ` [PATCH V6 7/7] domcreate: support pvusb in configuration file Chunyan Liu
@ 2015-08-11 11:27 ` Wei Liu
0 siblings, 0 replies; 41+ messages in thread
From: Wei Liu @ 2015-08-11 11:27 UTC (permalink / raw)
To: Chunyan Liu
Cc: jgross, wei.liu2, ian.campbell, george.dunlap, Ian.Jackson,
xen-devel, jfehlig, Simon Cao
On Mon, Aug 10, 2015 at 06:35:28PM +0800, Chunyan Liu wrote:
> 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=['2.1,controller=0,port=1', ]
>
> Signed-off-by: Chunyan Liu <cyliu@suse.com>
> Signed-off-by: Simon Cao <caobosimon@gmail.com>
> ---
[...]
> }
>
> + 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;
> +
> + d_config->usbctrls =
> + (libxl_device_usbctrl *)realloc(d_config->usbctrls,
> + sizeof(libxl_device_usbctrl) * (d_config->num_usbctrls + 1));
> + usbctrl = d_config->usbctrls + d_config->num_usbctrls;
> + libxl_device_usbctrl_init(usbctrl);
> +
Use ARRAY_EXTEND_INIT macro.
> + parse_usbctrl_config(usbctrl, buf);
> +
> + d_config->num_usbctrls++;
> + }
> + }
> +
> + if (!xlu_cfg_get_list(config, "usbdev", &usbs, 0, 0)) {
> + d_config->num_usbs = 0;
> + d_config->usbs = NULL;
> + while ((buf = xlu_cfg_get_listitem(usbs, d_config->num_usbs)) != NULL) {
> + libxl_device_usb *usb;
> +
> + d_config->usbs = (libxl_device_usb *)realloc(d_config->usbs,
> + sizeof(libxl_device_usb) * (d_config->num_usbs + 1));
> + usb = d_config->usbs + d_config->num_usbs;
> + libxl_device_usb_init(usb);
> +
Ditto.
Wei.
> + parse_usb_config(usb, buf);
> +
> + d_config->num_usbs++;
> + }
> + }
> +
> switch (xlu_cfg_get_list(config, "cpuid", &cpuids, 0, 1)) {
> case 0:
> {
> --
> 2.1.4
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH V6 3/7] libxl: add pvusb API
2015-08-11 11:27 ` Wei Liu
@ 2015-08-12 2:24 ` Chun Yan Liu
2015-08-13 9:09 ` Wei Liu
2015-08-31 6:10 ` Chun Yan Liu
1 sibling, 1 reply; 41+ messages in thread
From: Chun Yan Liu @ 2015-08-12 2:24 UTC (permalink / raw)
To: wei.liu2
Cc: Juergen Gross, ian.campbell, george.dunlap, Ian.Jackson,
xen-devel, Jim Fehlig, Simon Cao
>>> On 8/11/2015 at 07:27 PM, in message
<20150811112702.GF7460@zion.uk.xensource.com>, Wei Liu <wei.liu2@citrix.com>
wrote:
> On Mon, Aug 10, 2015 at 06:35:24PM +0800, Chunyan Liu wrote:
> > 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>
> >
> > ---
> > changes:
> > - Address George's comments:
> > * Update libxl_device_usb_getinfo to read ctrl/port only and
> > get other information.
> > * Update backend path according to xenstore frontend 'xxx/backend'
> > entry instead of using TOOLSTACK_DOMID.
> > * Use 'type' to indicate qemu/pv instead of previous naming 'protocol'.
> > * Add USB 'devtype' union, currently only includes "hostdev"
> >
>
> I will leave this to Ian and George since they had strong opinions on
> this.
>
> I only skimmed this patch. Some comments below.
>
> [...]
> > +
> > +int libxl_device_usb_getinfo(libxl_ctx *ctx, uint32_t domid,
> > + libxl_device_usb *usb,
> > + libxl_usbinfo *usbinfo);
> > +
> > /* 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 bee5ed5..935f25b 100644
> > --- a/tools/libxl/libxl_device.c
> > +++ b/tools/libxl/libxl_device.c
> > @@ -676,6 +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;
> > + if (dev->backend_kind == LIBXL__DEVICE_KIND_VUSB) {
> > + libxl__initiate_device_usbctrl_remove(egc, aodev);
> > + continue;
> > + }
>
> Is there a risk that this races with individual device removal? I think
> you get away with it because removal of individual device is idempotent?
You mean races with other device removal (like 'vbd') ? Yes, it is idempotent.
Only for 'vusb' (corresponding to USB controller), before removing USB controller
it will first removing all USB devices under it.
>
> > libxl__initiate_device_remove(egc, aodev);
> > }
> > }
> > diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> > index f98f089..5be3b3a 100644
> > --- a/tools/libxl/libxl_internal.h
> > +++ b/tools/libxl/libxl_internal.h
> > @@ -2553,6 +2553,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_usb_add(libxl__egc *egc, uint32_t domid,
> > + libxl_device_usb *usb,
> > + 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);
> > @@ -2585,6 +2593,13 @@ _hidden void
> libxl__wait_device_connection(libxl__egc*,
> > _hidden void libxl__initiate_device_remove(libxl__egc *egc,
> > libxl__ao_device *aodev);
> >
> > +_hidden int libxl__device_from_usbctrl(libxl__gc *gc, uint32_t domid,
> [...]
> > +void libxl__device_usb_add(libxl__egc *egc, uint32_t domid,
> > + libxl_device_usb *usb,
> > + libxl__ao_device *aodev)
> > +{
> > + STATE_AO_GC(aodev->ao);
> > + int rc = -1;
> > + char *busid = NULL;
> > +
> > + assert(usb->u.hostdev.hostbus > 0 && usb->u.hostdev.hostaddr > 0);
> > +
> > + busid = usb_busaddr_to_busid(gc, usb->u.hostdev.hostbus,
> > + usb->u.hostdev.hostaddr);
> > + if (!busid) {
> > + LOG(ERROR, "USB device doesn't exist in sysfs");
> > + goto out;
> > + }
> > +
> > + if (!is_usb_assignable(gc, usb)) {
> > + LOG(ERROR, "USB device is not assignable.");
> > + goto out;
> > + }
> > +
> > + /* check usb device is already assigned */
> > + if (is_usb_assigned(gc, usb)) {
> > + LOG(ERROR, "USB device is already attached to a domain.");
> > + goto out;
> > + }
> > +
> > + rc = libxl__device_usb_setdefault(gc, domid, usb, aodev->update_json);
> > + if (rc) goto out;
> > +
> > + rc = libxl__device_usb_add_xenstore(gc, domid, usb, aodev->update_json);
> > + if (rc) goto out;
> > +
> > + rc = usbback_dev_assign(gc, usb);
> > + if (rc) {
> > + libxl__device_usb_remove_xenstore(gc, domid, usb);
> > + goto out;
> > + }
> > +
> > + libxl__ao_complete(egc, ao, 0);
> > + rc = 0;
> > +
> > +out:
>
> You forget to complete ao in failure path.
It will complete ao in aodev->callback(egc, aodev) in "out:" section, here:
if (rc) aodev->callback(egc, aodev);
Thanks,
Chunyan
>
> But I'm not very familiar with the AO machinery, I will let Ian comment
> on this.
>
> Wei.
>
>
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH V6 2/7] libxl_read_file_contents: add new entry to read sysfs file
2015-08-11 11:26 ` Wei Liu
@ 2015-08-12 2:37 ` Chun Yan Liu
2015-08-13 9:11 ` Wei Liu
0 siblings, 1 reply; 41+ messages in thread
From: Chun Yan Liu @ 2015-08-12 2:37 UTC (permalink / raw)
To: wei.liu2
Cc: Juergen Gross, ian.campbell, george.dunlap, Ian.Jackson,
xen-devel, Jim Fehlig
>>> On 8/11/2015 at 07:26 PM, in message
<20150811112655.GE7460@zion.uk.xensource.com>, Wei Liu <wei.liu2@citrix.com>
wrote:
> On Mon, Aug 10, 2015 at 06:35:23PM +0800, Chunyan Liu wrote:
> > Sysfs file has size=4096 but actual file content is less than that.
> > Current libxl_read_file_contents will treat it as error when file size
> > and actual file content differs, so reading sysfs file content with
> > this function always fails.
> >
> > Add a new entry 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>
> >
> > ---
> > Changes:
> > - read one more byte to check bigger size problem.
> >
> > tools/libxl/libxl_internal.h | 2 ++
> > tools/libxl/libxl_utils.c | 51 ++++++++++++++++++++++++++++++++++----------
> > 2 files changed, 42 insertions(+), 11 deletions(-)
> >
> > diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> > index 6013628..f98f089 100644
> > --- a/tools/libxl/libxl_internal.h
> > +++ b/tools/libxl/libxl_internal.h
> > @@ -4001,6 +4001,8 @@ void libxl__bitmap_copy_best_effort(libxl__gc *gc,
> libxl_bitmap *dptr,
> >
> > int libxl__count_physical_sockets(libxl__gc *gc, int *sockets);
> > #endif
> > +_hidden int libxl_read_sysfs_file_contents(libxl_ctx *ctx, const char
> *filename,
> > + void **data_r, int *datalen_r);
>
> Indentation looks wrong.
>
> >
> > /*
> > * Local variables:
> > diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c
> > index bfc9699..9234efb 100644
> > --- a/tools/libxl/libxl_utils.c
> > +++ b/tools/libxl/libxl_utils.c
> > @@ -322,8 +322,10 @@ out:
> > return rc;
> > }
> >
> > -int libxl_read_file_contents(libxl_ctx *ctx, const char *filename,
> > - void **data_r, int *datalen_r) {
> > +static int libxl_read_file_contents_core(libxl_ctx *ctx, const char
> *filename,
> > + void **data_r, int *datalen_r,
> > + bool tolerate_shrinking_file)
> > +{
> > GC_INIT(ctx);
> > FILE *f = 0;
> > uint8_t *data = 0;
> > @@ -359,20 +361,34 @@ int libxl_read_file_contents(libxl_ctx *ctx, const
> char *filename,
> > datalen = stab.st_size;
> >
> > if (stab.st_size && data_r) {
> > - data = malloc(datalen);
> > + data = malloc(datalen + 1);
> > if (!data) goto xe;
> >
> > - rs = fread(data, 1, datalen, f);
> > - if (rs != datalen) {
> > - if (ferror(f))
> > + rs = fread(data, 1, datalen + 1, f);
> > + if (rs > datalen) {
> > + LOG(ERROR, "%s increased size while we were reading it",
> > + filename);
> > + goto xe;
> > + }
> > +
> > + if (rs < datalen) {
> > + if (ferror(f)) {
> > LOGE(ERROR, "failed to read %s", filename);
> > - else if (feof(f))
> > - LOG(ERROR, "%s changed size while we were reading it",
> > - filename);
> > - else
> > + goto xe;
> > + } else if (feof(f)) {
> > + if (tolerate_shrinking_file) {
> > + datalen = rs;
> > + } else {
> > + LOG(ERROR, "%s shrunk size while we were reading it",
> > + filename);
> > + goto xe;
> > + }
> > + } else {
> > abort();
> > - goto xe;
> > + }
>
> This is a bit bikeshedding, but you can leave "goto xe" out of two `if'
> to reduce patch size.
I guess you mean if (ferror(f)) and if (feof(f)) ? We can't leave 'goto xe' outside,
since in if (feof(f)) && if (tolerate_shrinking_file), it's not error but an expected
result in sysfs case.
> > }
> > +
> > + data = realloc(data, datalen);
>
> Should check return value of realloc.
Will add a check:
if (!data) goto xe;
Thanks,
Chunyan
>
> The logic of this function reflects what has been discussed so far.
>
> Wei.
>
> > }
> >
> > if (fclose(f)) {
> > @@ -396,6 +412,19 @@ int libxl_read_file_contents(libxl_ctx *ctx, const char
> *filename,
> > return e;
> > }
> >
> > +int libxl_read_file_contents(libxl_ctx *ctx, const char *filename,
> > + void **data_r, int *datalen_r)
> > +{
> > + return libxl_read_file_contents_core(ctx, filename, data_r, datalen_r,
> 0);
> > +}
> > +
> > +int libxl_read_sysfs_file_contents(libxl_ctx *ctx, const char *filename,
> > + void **data_r, int *datalen_r)
> > +{
> > + return libxl_read_file_contents_core(ctx, filename, data_r, datalen_r,
> 1);
> > +}
> > +
> > +
> > #define READ_WRITE_EXACTLY(rw, zero_is_eof, constdata)
> \
> >
> \
> > int libxl_##rw##_exactly(libxl_ctx *ctx, int fd, \
> > --
> > 2.1.4
>
>
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH V6 3/7] libxl: add pvusb API
2015-08-12 2:24 ` Chun Yan Liu
@ 2015-08-13 9:09 ` Wei Liu
2015-08-14 1:49 ` Chun Yan Liu
2015-08-18 2:31 ` Chun Yan Liu
0 siblings, 2 replies; 41+ messages in thread
From: Wei Liu @ 2015-08-13 9:09 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 Tue, Aug 11, 2015 at 08:24:01PM -0600, Chun Yan Liu wrote:
>
>
> >>> On 8/11/2015 at 07:27 PM, in message
> <20150811112702.GF7460@zion.uk.xensource.com>, Wei Liu <wei.liu2@citrix.com>
> wrote:
> > On Mon, Aug 10, 2015 at 06:35:24PM +0800, Chunyan Liu wrote:
> > > 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>
> > >
> > > ---
> > > changes:
> > > - Address George's comments:
> > > * Update libxl_device_usb_getinfo to read ctrl/port only and
> > > get other information.
> > > * Update backend path according to xenstore frontend 'xxx/backend'
> > > entry instead of using TOOLSTACK_DOMID.
> > > * Use 'type' to indicate qemu/pv instead of previous naming 'protocol'.
> > > * Add USB 'devtype' union, currently only includes "hostdev"
> > >
> >
> > I will leave this to Ian and George since they had strong opinions on
> > this.
> >
> > I only skimmed this patch. Some comments below.
> >
> > [...]
> > > +
> > > +int libxl_device_usb_getinfo(libxl_ctx *ctx, uint32_t domid,
> > > + libxl_device_usb *usb,
> > > + libxl_usbinfo *usbinfo);
> > > +
> > > /* 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 bee5ed5..935f25b 100644
> > > --- a/tools/libxl/libxl_device.c
> > > +++ b/tools/libxl/libxl_device.c
> > > @@ -676,6 +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;
> > > + if (dev->backend_kind == LIBXL__DEVICE_KIND_VUSB) {
> > > + libxl__initiate_device_usbctrl_remove(egc, aodev);
> > > + continue;
> > > + }
> >
> > Is there a risk that this races with individual device removal? I think
> > you get away with it because removal of individual device is idempotent?
>
> You mean races with other device removal (like 'vbd') ? Yes, it is idempotent.
> Only for 'vusb' (corresponding to USB controller), before removing USB controller
> it will first removing all USB devices under it.
>
No. What I mean is, the removal of usbctrl triggers removal of all
assigned usb devices. And then this function initiates removal of
assigned usb devices again. Is this a possible scenario?
> >
> > > libxl__initiate_device_remove(egc, aodev);
> > > }
> > > }
> > > diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> > > index f98f089..5be3b3a 100644
> > > --- a/tools/libxl/libxl_internal.h
> > > +++ b/tools/libxl/libxl_internal.h
> > > @@ -2553,6 +2553,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_usb_add(libxl__egc *egc, uint32_t domid,
> > > + libxl_device_usb *usb,
> > > + 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);
> > > @@ -2585,6 +2593,13 @@ _hidden void
> > libxl__wait_device_connection(libxl__egc*,
> > > _hidden void libxl__initiate_device_remove(libxl__egc *egc,
> > > libxl__ao_device *aodev);
> > >
> > > +_hidden int libxl__device_from_usbctrl(libxl__gc *gc, uint32_t domid,
> > [...]
> > > +void libxl__device_usb_add(libxl__egc *egc, uint32_t domid,
> > > + libxl_device_usb *usb,
> > > + libxl__ao_device *aodev)
> > > +{
> > > + STATE_AO_GC(aodev->ao);
> > > + int rc = -1;
> > > + char *busid = NULL;
> > > +
> > > + assert(usb->u.hostdev.hostbus > 0 && usb->u.hostdev.hostaddr > 0);
> > > +
> > > + busid = usb_busaddr_to_busid(gc, usb->u.hostdev.hostbus,
> > > + usb->u.hostdev.hostaddr);
> > > + if (!busid) {
> > > + LOG(ERROR, "USB device doesn't exist in sysfs");
> > > + goto out;
> > > + }
> > > +
> > > + if (!is_usb_assignable(gc, usb)) {
> > > + LOG(ERROR, "USB device is not assignable.");
> > > + goto out;
> > > + }
> > > +
> > > + /* check usb device is already assigned */
> > > + if (is_usb_assigned(gc, usb)) {
> > > + LOG(ERROR, "USB device is already attached to a domain.");
> > > + goto out;
> > > + }
> > > +
> > > + rc = libxl__device_usb_setdefault(gc, domid, usb, aodev->update_json);
> > > + if (rc) goto out;
> > > +
> > > + rc = libxl__device_usb_add_xenstore(gc, domid, usb, aodev->update_json);
> > > + if (rc) goto out;
> > > +
> > > + rc = usbback_dev_assign(gc, usb);
> > > + if (rc) {
> > > + libxl__device_usb_remove_xenstore(gc, domid, usb);
> > > + goto out;
> > > + }
> > > +
> > > + libxl__ao_complete(egc, ao, 0);
> > > + rc = 0;
> > > +
> > > +out:
> >
> > You forget to complete ao in failure path.
>
> It will complete ao in aodev->callback(egc, aodev) in "out:" section, here:
> if (rc) aodev->callback(egc, aodev);
>
I'm still confused by the way it is structured. If aodev->callback
completes the AO nonetheless, why don't you just call that
unconditionally?
Wei.
> Thanks,
> Chunyan
>
> >
> > But I'm not very familiar with the AO machinery, I will let Ian comment
> > on this.
> >
> > Wei.
> >
> >
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH V6 2/7] libxl_read_file_contents: add new entry to read sysfs file
2015-08-12 2:37 ` Chun Yan Liu
@ 2015-08-13 9:11 ` Wei Liu
0 siblings, 0 replies; 41+ messages in thread
From: Wei Liu @ 2015-08-13 9:11 UTC (permalink / raw)
To: Chun Yan Liu
Cc: Juergen Gross, wei.liu2, ian.campbell, george.dunlap, Ian.Jackson,
xen-devel, Jim Fehlig
On Tue, Aug 11, 2015 at 08:37:09PM -0600, Chun Yan Liu wrote:
[...]
> > > +
> > > + if (rs < datalen) {
> > > + if (ferror(f)) {
> > > LOGE(ERROR, "failed to read %s", filename);
> > > - else if (feof(f))
> > > - LOG(ERROR, "%s changed size while we were reading it",
> > > - filename);
> > > - else
> > > + goto xe;
> > > + } else if (feof(f)) {
> > > + if (tolerate_shrinking_file) {
> > > + datalen = rs;
> > > + } else {
> > > + LOG(ERROR, "%s shrunk size while we were reading it",
> > > + filename);
> > > + goto xe;
> > > + }
> > > + } else {
> > > abort();
> > > - goto xe;
> > > + }
> >
> > This is a bit bikeshedding, but you can leave "goto xe" out of two `if'
> > to reduce patch size.
>
> I guess you mean if (ferror(f)) and if (feof(f)) ? We can't leave 'goto xe' outside,
> since in if (feof(f)) && if (tolerate_shrinking_file), it's not error but an expected
> result in sysfs case.
>
Oh, right. I missed that tolerate_shrinking_file check. Sorry for the
noise.
Wei.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH V6 3/7] libxl: add pvusb API
2015-08-13 9:09 ` Wei Liu
@ 2015-08-14 1:49 ` Chun Yan Liu
2015-08-18 2:31 ` Chun Yan Liu
1 sibling, 0 replies; 41+ messages in thread
From: Chun Yan Liu @ 2015-08-14 1:49 UTC (permalink / raw)
To: wei.liu2
Cc: Juergen Gross, ian.campbell, george.dunlap, Ian.Jackson,
xen-devel, Jim Fehlig, Simon Cao
>>> On 8/13/2015 at 05:09 PM, in message
<20150813090938.GI7460@zion.uk.xensource.com>, Wei Liu <wei.liu2@citrix.com>
wrote:
> On Tue, Aug 11, 2015 at 08:24:01PM -0600, Chun Yan Liu wrote:
> >
> >
> > >>> On 8/11/2015 at 07:27 PM, in message
> > <20150811112702.GF7460@zion.uk.xensource.com>, Wei Liu <wei.liu2@citrix.com>
> > wrote:
> > > On Mon, Aug 10, 2015 at 06:35:24PM +0800, Chunyan Liu wrote:
> > > > 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>
> > > >
> > > > ---
> > > > changes:
> > > > - Address George's comments:
> > > > * Update libxl_device_usb_getinfo to read ctrl/port only and
> > > > get other information.
> > > > * Update backend path according to xenstore frontend 'xxx/backend'
> > > > entry instead of using TOOLSTACK_DOMID.
> > > > * Use 'type' to indicate qemu/pv instead of previous naming 'protocol'.
>
> > > > * Add USB 'devtype' union, currently only includes "hostdev"
> > > >
> > >
> > > I will leave this to Ian and George since they had strong opinions on
> > > this.
> > >
> > > I only skimmed this patch. Some comments below.
> > >
> > > [...]
> > > > +
> > > > +int libxl_device_usb_getinfo(libxl_ctx *ctx, uint32_t domid,
> > > > + libxl_device_usb *usb,
> > > > + libxl_usbinfo *usbinfo);
> > > > +
> > > > /* 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 bee5ed5..935f25b 100644
> > > > --- a/tools/libxl/libxl_device.c
> > > > +++ b/tools/libxl/libxl_device.c
> > > > @@ -676,6 +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;
> > > > + if (dev->backend_kind == LIBXL__DEVICE_KIND_VUSB) {
> > > > + libxl__initiate_device_usbctrl_remove(egc, aodev);
> > > > + continue;
> > > > + }
> > >
> > > Is there a risk that this races with individual device removal? I think
> > > you get away with it because removal of individual device is idempotent?
> >
> > You mean races with other device removal (like 'vbd') ? Yes, it is
> idempotent.
> > Only for 'vusb' (corresponding to USB controller), before removing USB
> controller
> > it will first removing all USB devices under it.
h >
>
> No. What I mean is, the removal of usbctrl triggers removal of all
> assigned usb devices. And then this function initiates removal of
> assigned usb devices again. Is this a possible scenario?
No, it's not possible. libxl__devices_destroy is used in domain destroy, it's
trying to scan each device type in xenstore and destroy them. Since USB device
is NOT presented as a separate device type but inside USB controller (which is
represented by a 'vusb' device in xenstore), so when scanning 'vusb' type, it
tries to destroy USB controller, within that it will destroy all USB devices under
that controller. No entry to remove USB device alone.
Thanks,
Chunyan
>
> > >
> > > > libxl__initiate_device_remove(egc, aodev);
> > > > }
> > > > }
> > > > diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> > > > index f98f089..5be3b3a 100644
> > > > --- a/tools/libxl/libxl_internal.h
> > > > +++ b/tools/libxl/libxl_internal.h
> > > > @@ -2553,6 +2553,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_usb_add(libxl__egc *egc, uint32_t domid,
> > > > + libxl_device_usb *usb,
> > > > + 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);
> > > > @@ -2585,6 +2593,13 @@ _hidden void
> > > libxl__wait_device_connection(libxl__egc*,
> > > > _hidden void libxl__initiate_device_remove(libxl__egc *egc,
> > > > libxl__ao_device *aodev);
> > > >
> > > > +_hidden int libxl__device_from_usbctrl(libxl__gc *gc, uint32_t domid,
> > > [...]
> > > > +void libxl__device_usb_add(libxl__egc *egc, uint32_t domid,
> > > > + libxl_device_usb *usb,
> > > > + libxl__ao_device *aodev)
> > > > +{
> > > > + STATE_AO_GC(aodev->ao);
> > > > + int rc = -1;
> > > > + char *busid = NULL;
> > > > +
> > > > + assert(usb->u.hostdev.hostbus > 0 && usb->u.hostdev.hostaddr > 0);
> > > > +
> > > > + busid = usb_busaddr_to_busid(gc, usb->u.hostdev.hostbus,
> > > > + usb->u.hostdev.hostaddr);
> > > > + if (!busid) {
> > > > + LOG(ERROR, "USB device doesn't exist in sysfs");
> > > > + goto out;
> > > > + }
> > > > +
> > > > + if (!is_usb_assignable(gc, usb)) {
> > > > + LOG(ERROR, "USB device is not assignable.");
> > > > + goto out;
> > > > + }
> > > > +
> > > > + /* check usb device is already assigned */
> > > > + if (is_usb_assigned(gc, usb)) {
> > > > + LOG(ERROR, "USB device is already attached to a domain.");
> > > > + goto out;
> > > > + }
> > > > +
> > > > + rc = libxl__device_usb_setdefault(gc, domid, usb, aodev->update_json);
>
> > > > + if (rc) goto out;
> > > > +
> > > > + rc = libxl__device_usb_add_xenstore(gc, domid, usb,
> aodev->update_json);
> > > > + if (rc) goto out;
> > > > +
> > > > + rc = usbback_dev_assign(gc, usb);
> > > > + if (rc) {
> > > > + libxl__device_usb_remove_xenstore(gc, domid, usb);
> > > > + goto out;
> > > > + }
> > > > +
> > > > + libxl__ao_complete(egc, ao, 0);
> > > > + rc = 0;
> > > > +
> > > > +out:
> > >
> > > You forget to complete ao in failure path.
> >
> > It will complete ao in aodev->callback(egc, aodev) in "out:" section, here:
> > if (rc) aodev->callback(egc, aodev);
> >
>
> I'm still confused by the way it is structured. If aodev->callback
> completes the AO nonetheless, why don't you just call that
> unconditionally?
>
> Wei.
>
>
> > Thanks,
> > Chunyan
> >
> > >
> > > But I'm not very familiar with the AO machinery, I will let Ian comment
> > > on this.
> > >
> > > Wei.
> > >
> > >
>
>
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH V6 3/7] libxl: add pvusb API
2015-08-13 9:09 ` Wei Liu
2015-08-14 1:49 ` Chun Yan Liu
@ 2015-08-18 2:31 ` Chun Yan Liu
1 sibling, 0 replies; 41+ messages in thread
From: Chun Yan Liu @ 2015-08-18 2:31 UTC (permalink / raw)
To: wei.liu2
Cc: Juergen Gross, ian.campbell, george.dunlap, Ian.Jackson,
xen-devel, Jim Fehlig, Simon Cao
>>> On 8/13/2015 at 05:09 PM, in message
<20150813090938.GI7460@zion.uk.xensource.com>, Wei Liu <wei.liu2@citrix.com>
wrote:
> On Tue, Aug 11, 2015 at 08:24:01PM -0600, Chun Yan Liu wrote:
> >
> >
> > >>> On 8/11/2015 at 07:27 PM, in message
> > <20150811112702.GF7460@zion.uk.xensource.com>, Wei Liu <wei.liu2@citrix.com>
> > wrote:
> > > On Mon, Aug 10, 2015 at 06:35:24PM +0800, Chunyan Liu wrote:
> > > > 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>
> > > >
> > > > ---
> > > > changes:
> > > > - Address George's comments:
> > > > * Update libxl_device_usb_getinfo to read ctrl/port only and
> > > > get other information.
> > > > * Update backend path according to xenstore frontend 'xxx/backend'
> > > > entry instead of using TOOLSTACK_DOMID.
> > > > * Use 'type' to indicate qemu/pv instead of previous naming 'protocol'.
>
> > > > * Add USB 'devtype' union, currently only includes "hostdev"
> > > >
> > >
> > > I will leave this to Ian and George since they had strong opinions on
> > > this.
> > >
> > > I only skimmed this patch. Some comments below.
> > >
> > > [...]
> > > > +
> > > > +int libxl_device_usb_getinfo(libxl_ctx *ctx, uint32_t domid,
> > > > + libxl_device_usb *usb,
> > > > + libxl_usbinfo *usbinfo);
> > > > +
> > > > /* 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 bee5ed5..935f25b 100644
> > > > --- a/tools/libxl/libxl_device.c
> > > > +++ b/tools/libxl/libxl_device.c
> > > > @@ -676,6 +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;
> > > > + if (dev->backend_kind == LIBXL__DEVICE_KIND_VUSB) {
> > > > + libxl__initiate_device_usbctrl_remove(egc, aodev);
> > > > + continue;
> > > > + }
> > >
> > > Is there a risk that this races with individual device removal? I think
> > > you get away with it because removal of individual device is idempotent?
> >
> > You mean races with other device removal (like 'vbd') ? Yes, it is
> idempotent.
> > Only for 'vusb' (corresponding to USB controller), before removing USB
> controller
> > it will first removing all USB devices under it.
> >
>
> No. What I mean is, the removal of usbctrl triggers removal of all
> assigned usb devices. And then this function initiates removal of
> assigned usb devices again. Is this a possible scenario?
>
> > >
> > > > libxl__initiate_device_remove(egc, aodev);
> > > > }
> > > > }
> > > > diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> > > > index f98f089..5be3b3a 100644
> > > > --- a/tools/libxl/libxl_internal.h
> > > > +++ b/tools/libxl/libxl_internal.h
> > > > @@ -2553,6 +2553,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_usb_add(libxl__egc *egc, uint32_t domid,
> > > > + libxl_device_usb *usb,
> > > > + 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);
> > > > @@ -2585,6 +2593,13 @@ _hidden void
> > > libxl__wait_device_connection(libxl__egc*,
> > > > _hidden void libxl__initiate_device_remove(libxl__egc *egc,
> > > > libxl__ao_device *aodev);
> > > >
> > > > +_hidden int libxl__device_from_usbctrl(libxl__gc *gc, uint32_t domid,
> > > [...]
> > > > +void libxl__device_usb_add(libxl__egc *egc, uint32_t domid,
> > > > + libxl_device_usb *usb,
> > > > + libxl__ao_device *aodev)
> > > > +{
> > > > + STATE_AO_GC(aodev->ao);
> > > > + int rc = -1;
> > > > + char *busid = NULL;
> > > > +
> > > > + assert(usb->u.hostdev.hostbus > 0 && usb->u.hostdev.hostaddr > 0);
> > > > +
> > > > + busid = usb_busaddr_to_busid(gc, usb->u.hostdev.hostbus,
> > > > + usb->u.hostdev.hostaddr);
> > > > + if (!busid) {
> > > > + LOG(ERROR, "USB device doesn't exist in sysfs");
> > > > + goto out;
> > > > + }
> > > > +
> > > > + if (!is_usb_assignable(gc, usb)) {
> > > > + LOG(ERROR, "USB device is not assignable.");
> > > > + goto out;
> > > > + }
> > > > +
> > > > + /* check usb device is already assigned */
> > > > + if (is_usb_assigned(gc, usb)) {
> > > > + LOG(ERROR, "USB device is already attached to a domain.");
> > > > + goto out;
> > > > + }
> > > > +
> > > > + rc = libxl__device_usb_setdefault(gc, domid, usb, aodev->update_json);
>
> > > > + if (rc) goto out;
> > > > +
> > > > + rc = libxl__device_usb_add_xenstore(gc, domid, usb,
> aodev->update_json);
> > > > + if (rc) goto out;
> > > > +
> > > > + rc = usbback_dev_assign(gc, usb);
> > > > + if (rc) {
> > > > + libxl__device_usb_remove_xenstore(gc, domid, usb);
> > > > + goto out;
> > > > + }
> > > > +
> > > > + libxl__ao_complete(egc, ao, 0);
> > > > + rc = 0;
> > > > +
> > > > +out:
> > >
> > > You forget to complete ao in failure path.
> >
> > It will complete ao in aodev->callback(egc, aodev) in "out:" section, here:
> > if (rc) aodev->callback(egc, aodev);
> >
>
> I'm still confused by the way it is structured. If aodev->callback
> completes the AO nonetheless, why don't you just call that
> unconditionally?
In general case, it won't call libxl__ao_complete directly. In correct path,
it will call libxl__wait_device_connection (it will wait for front/backend driver
status change and deal with hotplug script, and then call callback function
'addrmcompelte' or in some path call libxl__ao_compelete to complete the ao);
in error path, it will call aodev->callback function 'addrmcomplete' to complete
the ao.
Here, we try to follow the general routine, so keep the error path handling;
but for correct path, since there is no need to wait for device, so we explicitly
call libxl__ao_compelte to complete ao. That may keep the function easier to
read? (since it keeps the same framework as others.) I see the pending scsi patch
series when adding scsi device does in the same way.
Thanks,
Chunyan
>
> Wei.
>
>
> > Thanks,
> > Chunyan
> >
> > >
> > > But I'm not very familiar with the AO machinery, I will let Ian comment
> > > on this.
> > >
> > > Wei.
> > >
> > >
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>
>
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH V6 3/7] libxl: add pvusb API
2015-08-11 11:27 ` Wei Liu
2015-08-12 2:24 ` Chun Yan Liu
@ 2015-08-31 6:10 ` Chun Yan Liu
1 sibling, 0 replies; 41+ messages in thread
From: Chun Yan Liu @ 2015-08-31 6:10 UTC (permalink / raw)
To: wei.liu2
Cc: Juergen Gross, ian.campbell, george.dunlap, Ian.Jackson,
xen-devel, Jim Fehlig, Simon Cao
Ian and George, could I have your comments?
Thanks,
Chunyan
>>> On 8/11/2015 at 07:27 PM, in message
<20150811112702.GF7460@zion.uk.xensource.com>, Wei Liu <wei.liu2@citrix.com>
wrote:
> On Mon, Aug 10, 2015 at 06:35:24PM +0800, Chunyan Liu wrote:
> > 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>
> >
> > ---
> > changes:
> > - Address George's comments:
> > * Update libxl_device_usb_getinfo to read ctrl/port only and
> > get other information.
> > * Update backend path according to xenstore frontend 'xxx/backend'
> > entry instead of using TOOLSTACK_DOMID.
> > * Use 'type' to indicate qemu/pv instead of previous naming 'protocol'.
> > * Add USB 'devtype' union, currently only includes "hostdev"
> >
>
> I will leave this to Ian and George since they had strong opinions on
> this.
>
> I only skimmed this patch. Some comments below.
>
> [...]
> > +
> > +int libxl_device_usb_getinfo(libxl_ctx *ctx, uint32_t domid,
> > + libxl_device_usb *usb,
> > + libxl_usbinfo *usbinfo);
> > +
> > /* 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 bee5ed5..935f25b 100644
> > --- a/tools/libxl/libxl_device.c
> > +++ b/tools/libxl/libxl_device.c
> > @@ -676,6 +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;
> > + if (dev->backend_kind == LIBXL__DEVICE_KIND_VUSB) {
> > + libxl__initiate_device_usbctrl_remove(egc, aodev);
> > + continue;
> > + }
>
> Is there a risk that this races with individual device removal? I think
> you get away with it because removal of individual device is idempotent?
>
> > libxl__initiate_device_remove(egc, aodev);
> > }
> > }
> > diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> > index f98f089..5be3b3a 100644
> > --- a/tools/libxl/libxl_internal.h
> > +++ b/tools/libxl/libxl_internal.h
> > @@ -2553,6 +2553,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_usb_add(libxl__egc *egc, uint32_t domid,
> > + libxl_device_usb *usb,
> > + 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);
> > @@ -2585,6 +2593,13 @@ _hidden void
> libxl__wait_device_connection(libxl__egc*,
> > _hidden void libxl__initiate_device_remove(libxl__egc *egc,
> > libxl__ao_device *aodev);
> >
> > +_hidden int libxl__device_from_usbctrl(libxl__gc *gc, uint32_t domid,
> [...]
> > +void libxl__device_usb_add(libxl__egc *egc, uint32_t domid,
> > + libxl_device_usb *usb,
> > + libxl__ao_device *aodev)
> > +{
> > + STATE_AO_GC(aodev->ao);
> > + int rc = -1;
> > + char *busid = NULL;
> > +
> > + assert(usb->u.hostdev.hostbus > 0 && usb->u.hostdev.hostaddr > 0);
> > +
> > + busid = usb_busaddr_to_busid(gc, usb->u.hostdev.hostbus,
> > + usb->u.hostdev.hostaddr);
> > + if (!busid) {
> > + LOG(ERROR, "USB device doesn't exist in sysfs");
> > + goto out;
> > + }
> > +
> > + if (!is_usb_assignable(gc, usb)) {
> > + LOG(ERROR, "USB device is not assignable.");
> > + goto out;
> > + }
> > +
> > + /* check usb device is already assigned */
> > + if (is_usb_assigned(gc, usb)) {
> > + LOG(ERROR, "USB device is already attached to a domain.");
> > + goto out;
> > + }
> > +
> > + rc = libxl__device_usb_setdefault(gc, domid, usb, aodev->update_json);
> > + if (rc) goto out;
> > +
> > + rc = libxl__device_usb_add_xenstore(gc, domid, usb, aodev->update_json);
> > + if (rc) goto out;
> > +
> > + rc = usbback_dev_assign(gc, usb);
> > + if (rc) {
> > + libxl__device_usb_remove_xenstore(gc, domid, usb);
> > + goto out;
> > + }
> > +
> > + libxl__ao_complete(egc, ao, 0);
> > + rc = 0;
> > +
> > +out:
>
> You forget to complete ao in failure path.
>
> But I'm not very familiar with the AO machinery, I will let Ian comment
> on this.
>
> Wei.
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>
>
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH V6 3/7] libxl: add pvusb API
2015-08-10 10:35 ` [PATCH V6 3/7] libxl: add pvusb API Chunyan Liu
2015-08-11 11:27 ` Wei Liu
@ 2015-09-08 14:17 ` Ian Campbell
2015-09-08 16:52 ` George Dunlap
2015-09-11 5:42 ` Chun Yan Liu
1 sibling, 2 replies; 41+ messages in thread
From: Ian Campbell @ 2015-09-08 14:17 UTC (permalink / raw)
To: Chunyan Liu, xen-devel
Cc: jgross, wei.liu2, george.dunlap, Ian.Jackson, jfehlig, Simon Cao
On Mon, 2015-08-10 at 18:35 +0800, Chunyan Liu wrote:
Sorry for the delay, between 4.6 freeze crunch, conference and vacation
I've been a bit swamped.
I'm just going to comment on the APIs (mainly public libxl.h and .idl) in
this pass.
> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index 5f9047c..05b6331 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -123,6 +123,23 @@
> #define LIBXL_HAVE_DOMAIN_NODEAFFINITY 1
>
> /*
> + * LIBXL_HAVE_PVUSB indicates the functions for doing hot-plug of
And cold-plug, no?
> + * USB devices through pvusb.
> + *
> + * With this functionality, 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 either 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 either 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.
I think this API documentation belongs alongside the API declarations (i.e
the prototypes) rather than hidden away next to the feature flag.
> + *
> + */
> +#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
> @@ -1389,6 +1406,54 @@ 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 Controllers*/
>
[....]
Seem fine.
> +
> +/* USB Devices */
> +int libxl_device_usb_add(libxl_ctx *ctx, uint32_t domid, libxl_device_usb *usb,
> + const libxl_asyncop_how *ao_how)
> + LIBXL_EXTERNAL_CALLERS_ONLY;
> +
> +int libxl_device_usb_remove(libxl_ctx *ctx, uint32_t domid, libxl_device_usb *usb,
> + const libxl_asyncop_how *ao_how)
> + LIBXL_EXTERNAL_CALLERS_ONLY;
> +
> +libxl_device_usb *
> +libxl_device_usb_list(libxl_ctx *ctx, uint32_t domid, int *num);
> +
> +libxl_device_usb *
> +libxl_device_usb_list_per_usbctrl(libxl_ctx *ctx, uint32_t domid,
> + libxl_devid usbctrl, int *num);
I'd probably say "..._for_usbctrl" or "..._by_usbctrl", but that's just
nitpicking.
> +
> +void libxl_device_usb_list_free(libxl_device_usb *list, int nr);
> +
> +int libxl_device_usb_getinfo(libxl_ctx *ctx, uint32_t domid,
> + libxl_device_usb *usb,
> + libxl_usbinfo *usbinfo);
> +
> /* 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_types.idl b/tools/libxl/libxl_types.idl
> index ef346e7..ef10484 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -594,6 +594,37 @@ libxl_device_rdm = Struct("device_rdm", [
> ("policy", libxl_rdm_reserve_policy),
> ])
>
> +libxl_usbctrl_type = Enumeration("usbctrl_type", [
> + (0, "AUTO"),
What are the proposed semantics of using LIBXL_USBCTRL_TYPE_AUTO?
> + (1, "PV"),
> + (2, "QEMU"),
Is "QEMU" what we want here, as opposed to, say, "EMU" (similar to NICs)?
I think we probably don't want to go as fine grained as "XHCI" and "EHCI"
etc, do we? I see we have a version field below, is it intended that there
be some way to select between e.g. UHCI and OHCI (which IIRC are different
USB 1.0 controllers).
Maybe these questions should all be left aside for when QMEU support is
actually added (AFAICT this field is just a placeholder)? In fact I glanced
at the code and was surprised to find nothing checking for
LIBXL_USBCTRL_TYPE at all, did I miss something?
I think the two choices are:
We can decide quickly and easily what the option(s) other than PV should be
here and you include it in the IDL, but you would then need to check
usbctrl->type == PV at various points, not silently treat all options as
PV.
Or this becomes a long conversation in which case I think your best bet
would be to leave the enum with just the PV (and maybe AUTO) entries and
leave the decision on the name for the emulated option to the series which
implements that.
> + ])
> +
> +libxl_usbdev_type = Enumeration("usbdev_type", [
> + (0, "invalid"),
> + (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_usb = Struct("device_usb", [
> + ("ctrl", libxl_devid),
> + ("port", integer),
> + ("u", KeyedUnion(None, libxl_usbdev_type, "devtype",
> + [("hostdev", Struct(None, [
> + ("hostbus", integer),
> + ("hostaddr", integer)])),
> + ("invalid", None),
AIUI this is what was agreed to, i.e. an enum with only one real option, in
order to leave a space for new devtypes without major API overhaul.
Please can you confirm that hostbus and hostaddr are both flat integer
namespaces (i.e. there is no structure to the bits within either, they are
just a number).
Do these fields have any particular size requirements arising from e.g. the
USB spec or from possible dom0 implementations?
If they have a well defined fixed size from a USB spec then maybe we could
use the appropriate fixed size types?
> + ])),
> + ])
> +
> libxl_device_dtdev = Struct("device_dtdev", [
> ("path", string),
> ])
> @@ -626,6 +657,8 @@ libxl_domain_config = Struct("domain_config", [
> ("pcidevs", Array(libxl_device_pci, "num_pcidevs")),
> ("rdms", Array(libxl_device_rdm, "num_rdms")),
> ("dtdevs", Array(libxl_device_dtdev, "num_dtdevs")),
> + ("usbctrls", Array(libxl_device_usbctrl, "num_usbctrls")),
> + ("usbs", Array(libxl_device_usb, "num_usbs")),
> ("vfbs", Array(libxl_device_vfb, "num_vfbs")),
> ("vkbs", Array(libxl_device_vkb, "num_vkbs")),
> ("vtpms", Array(libxl_device_vtpm, "num_vtpms")),
> @@ -674,6 +707,32 @@ 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_usbinfo = Struct("usbinfo", [
> + ("ctrl", libxl_devid),
> + ("port", integer),
> + ("busnum", integer),
> + ("devnum", integer),
> + ("idVendor", integer),
> + ("idProduct", integer),
I think id* are 16 bits? uint16 might be better then.
> + ("prod", string),
> + ("manuf", string),
> + ], dir=DIR_OUT)
> +
> libxl_vcpuinfo = Struct("vcpuinfo", [
> ("vcpuid", uint32),
> ("cpu", uint32),
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH V6 3/7] libxl: add pvusb API
2015-09-08 14:17 ` Ian Campbell
@ 2015-09-08 16:52 ` George Dunlap
2015-09-09 7:38 ` Chun Yan Liu
` (2 more replies)
2015-09-11 5:42 ` Chun Yan Liu
1 sibling, 3 replies; 41+ messages in thread
From: George Dunlap @ 2015-09-08 16:52 UTC (permalink / raw)
To: Ian Campbell, Chunyan Liu, xen-devel
Cc: jgross, wei.liu2, george.dunlap, Ian.Jackson, jfehlig, Simon Cao
On 09/08/2015 03:17 PM, Ian Campbell wrote:
> On Mon, 2015-08-10 at 18:35 +0800, Chunyan Liu wrote:
>
> Sorry for the delay, between 4.6 freeze crunch, conference and vacation
> I've been a bit swamped.
>
> I'm just going to comment on the APIs (mainly public libxl.h and .idl) in
> this pass.
>
>> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
>> index 5f9047c..05b6331 100644
>> --- a/tools/libxl/libxl.h
>> +++ b/tools/libxl/libxl.h
>> @@ -123,6 +123,23 @@
>> #define LIBXL_HAVE_DOMAIN_NODEAFFINITY 1
>>
>> /*
>> + * LIBXL_HAVE_PVUSB indicates the functions for doing hot-plug of
>
> And cold-plug, no?
So you should probably say something like "indicates functions for
plugging in USB devices through pvusb -- both hotplug and at domain
creation time."
>> +libxl_usbctrl_type = Enumeration("usbctrl_type", [
>> + (0, "AUTO"),
>
> What are the proposed semantics of using LIBXL_USBCTRL_TYPE_AUTO?
Generally "DTRT". Meaning:
1. If your domain has no devicemodel, use PV.
2. If your device has a devicemodel, and no PV drivers have peen
detected, use the devicemodel.
3. If your device has a devicemodel, but PV drivers have been detected,
use PV.
At the moment we don't have a way to check for PV drivers, so this just
collapses down to "PV for domains without a DM and DM for domains with a
DM."
>
>> + (1, "PV"),
>> + (2, "QEMU"),
>
> Is "QEMU" what we want here, as opposed to, say, "EMU" (similar to NICs)?
I had this as "DEVICEMODEL", since what we mean is that we want the
device model to provide access (and in theory in the future we may use a
different device model). But "EMU" works for me too.
> I think we probably don't want to go as fine grained as "XHCI" and "EHCI"
> etc, do we? I see we have a version field below, is it intended that there
> be some way to select between e.g. UHCI and OHCI (which IIRC are different
> USB 1.0 controllers).
>
> Maybe these questions should all be left aside for when QMEU support is
> actually added (AFAICT this field is just a placeholder)? In fact I glanced
> at the code and was surprised to find nothing checking for
> LIBXL_USBCTRL_TYPE at all, did I miss something?
>
> I think the two choices are:
>
> We can decide quickly and easily what the option(s) other than PV should be
> here and you include it in the IDL, but you would then need to check
> usbctrl->type == PV at various points, not silently treat all options as
> PV.
>
> Or this becomes a long conversation in which case I think your best bet
> would be to leave the enum with just the PV (and maybe AUTO) entries and
> leave the decision on the name for the emulated option to the series which
> implements that.
I think the idea was to simply offer 1, 2, and 3 as options, and for the
devicemodel version, choose a suitable controller (or set of
controllers) for each option; similar to what usbversion= does now.
>
>> + ])
>> +
>> +libxl_usbdev_type = Enumeration("usbdev_type", [
>> + (0, "invalid"),
>> + (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_usb = Struct("device_usb", [
>> + ("ctrl", libxl_devid),
>> + ("port", integer),
>> + ("u", KeyedUnion(None, libxl_usbdev_type, "devtype",
>> + [("hostdev", Struct(None, [
>> + ("hostbus", integer),
>> + ("hostaddr", integer)])),
>> + ("invalid", None),
>
> AIUI this is what was agreed to, i.e. an enum with only one real option, in
> order to leave a space for new devtypes without major API overhaul.
>
> Please can you confirm that hostbus and hostaddr are both flat integer
> namespaces (i.e. there is no structure to the bits within either, they are
> just a number).
I can confirm this.
-George
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH V6 3/7] libxl: add pvusb API
2015-09-08 16:52 ` George Dunlap
@ 2015-09-09 7:38 ` Chun Yan Liu
2015-09-17 8:19 ` Chun Yan Liu
2015-09-17 8:20 ` Chun Yan Liu
2 siblings, 0 replies; 41+ messages in thread
From: Chun Yan Liu @ 2015-09-09 7:38 UTC (permalink / raw)
To: George Dunlap, Ian Campbell, xen-devel
Cc: Juergen Gross, wei.liu2, george.dunlap, Ian.Jackson, Jim Fehlig,
Simon Cao
>>> On 9/9/2015 at 12:52 AM, in message <55EF1244.107@citrix.com>, George Dunlap
<george.dunlap@citrix.com> wrote:
> On 09/08/2015 03:17 PM, Ian Campbell wrote:
> > On Mon, 2015-08-10 at 18:35 +0800, Chunyan Liu wrote:
> >
> > Sorry for the delay, between 4.6 freeze crunch, conference and vacation
> > I've been a bit swamped.
> >
> > I'm just going to comment on the APIs (mainly public libxl.h and .idl) in
> > this pass.
> >
> >> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> >> index 5f9047c..05b6331 100644
> >> --- a/tools/libxl/libxl.h
> >> +++ b/tools/libxl/libxl.h
> >> @@ -123,6 +123,23 @@
> >> #define LIBXL_HAVE_DOMAIN_NODEAFFINITY 1
> >>
> >> /*
> >> + * LIBXL_HAVE_PVUSB indicates the functions for doing hot-plug of
> >
> > And cold-plug, no?
>
> So you should probably say something like "indicates functions for
> plugging in USB devices through pvusb -- both hotplug and at domain
> creation time."
Thanks. Will clarify.
>
> >> +libxl_usbctrl_type = Enumeration("usbctrl_type", [
> >> + (0, "AUTO"),
> >
> > What are the proposed semantics of using LIBXL_USBCTRL_TYPE_AUTO?
>
> Generally "DTRT". Meaning:
> 1. If your domain has no devicemodel, use PV.
> 2. If your device has a devicemodel, and no PV drivers have peen
> detected, use the devicemodel.
> 3. If your device has a devicemodel, but PV drivers have been detected,
> use PV.
>
> At the moment we don't have a way to check for PV drivers, so this just
> collapses down to "PV for domains without a DM and DM for domains with a
> DM."
Better to be: by default, PV for PV guest and DM for HVM guest.
Thanks,
Chunyan
>
> >
> >> + (1, "PV"),
> >> + (2, "QEMU"),
> >
> > Is "QEMU" what we want here, as opposed to, say, "EMU" (similar to NICs)?
>
> I had this as "DEVICEMODEL", since what we mean is that we want the
> device model to provide access (and in theory in the future we may use a
> different device model). But "EMU" works for me too.
>
> > I think we probably don't want to go as fine grained as "XHCI" and "EHCI"
> > etc, do we? I see we have a version field below, is it intended that there
> > be some way to select between e.g. UHCI and OHCI (which IIRC are different
> > USB 1.0 controllers).
> >
> > Maybe these questions should all be left aside for when QMEU support is
> > actually added (AFAICT this field is just a placeholder)? In fact I glanced
> > at the code and was surprised to find nothing checking for
> > LIBXL_USBCTRL_TYPE at all, did I miss something?
> >
> > I think the two choices are:
> >
> > We can decide quickly and easily what the option(s) other than PV should be
> > here and you include it in the IDL, but you would then need to check
> > usbctrl->type == PV at various points, not silently treat all options as
> > PV.
> >
> > Or this becomes a long conversation in which case I think your best bet
> > would be to leave the enum with just the PV (and maybe AUTO) entries and
> > leave the decision on the name for the emulated option to the series which
> > implements that.
>
> I think the idea was to simply offer 1, 2, and 3 as options, and for the
> devicemodel version, choose a suitable controller (or set of
> controllers) for each option; similar to what usbversion= does now.
>
> >
> >> + ])
> >> +
> >> +libxl_usbdev_type = Enumeration("usbdev_type", [
> >> + (0, "invalid"),
> >> + (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_usb = Struct("device_usb", [
> >> + ("ctrl", libxl_devid),
> >> + ("port", integer),
> >> + ("u", KeyedUnion(None, libxl_usbdev_type, "devtype",
> >> + [("hostdev", Struct(None, [
> >> + ("hostbus", integer),
> >> + ("hostaddr", integer)])),
> >> + ("invalid", None),
> >
> > AIUI this is what was agreed to, i.e. an enum with only one real option, in
> > order to leave a space for new devtypes without major API overhaul.
> >
> > Please can you confirm that hostbus and hostaddr are both flat integer
> > namespaces (i.e. there is no structure to the bits within either, they are
> > just a number).
>
> I can confirm this.
>
> -George
>
>
>
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH V6 3/7] libxl: add pvusb API
2015-09-08 14:17 ` Ian Campbell
2015-09-08 16:52 ` George Dunlap
@ 2015-09-11 5:42 ` Chun Yan Liu
2015-09-11 13:26 ` Ian Campbell
1 sibling, 1 reply; 41+ messages in thread
From: Chun Yan Liu @ 2015-09-11 5:42 UTC (permalink / raw)
To: Ian Campbell, xen-devel
Cc: Juergen Gross, wei.liu2, george.dunlap, Ian.Jackson, Jim Fehlig,
Simon Cao
>>> On 9/8/2015 at 10:17 PM, in message <1441721852.24450.120.camel@citrix.com>,
Ian Campbell <ian.campbell@citrix.com> wrote:
> On Mon, 2015-08-10 at 18:35 +0800, Chunyan Liu wrote:
>
> Sorry for the delay, between 4.6 freeze crunch, conference and vacation
> I've been a bit swamped.
>
> I'm just going to comment on the APIs (mainly public libxl.h and .idl) in
> this pass.
>
> > diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> > index 5f9047c..05b6331 100644
> > --- a/tools/libxl/libxl.h
> > +++ b/tools/libxl/libxl.h
> > @@ -123,6 +123,23 @@
> > #define LIBXL_HAVE_DOMAIN_NODEAFFINITY 1
> >
> > /*
> > + * LIBXL_HAVE_PVUSB indicates the functions for doing hot-plug of
>
> And cold-plug, no?
>
> > + * USB devices through pvusb.
> > + *
> > + * With this functionality, 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 either 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 either 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.
>
> I think this API documentation belongs alongside the API declarations (i.e
> the prototypes) rather than hidden away next to the feature flag.
>
> > + *
> > + */
> > +#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
> > @@ -1389,6 +1406,54 @@ 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 Controllers*/
> >
> [....]
>
> Seem fine.
>
> > +
> > +/* USB Devices */
> > +int libxl_device_usb_add(libxl_ctx *ctx, uint32_t domid, libxl_device_usb
> *usb,
> > + const libxl_asyncop_how *ao_how)
> > + LIBXL_EXTERNAL_CALLERS_ONLY;
> > +
> > +int libxl_device_usb_remove(libxl_ctx *ctx, uint32_t domid,
> libxl_device_usb *usb,
> > + const libxl_asyncop_how *ao_how)
> > + LIBXL_EXTERNAL_CALLERS_ONLY;
> > +
> > +libxl_device_usb *
> > +libxl_device_usb_list(libxl_ctx *ctx, uint32_t domid, int *num);
> > +
> > +libxl_device_usb *
> > +libxl_device_usb_list_per_usbctrl(libxl_ctx *ctx, uint32_t domid,
> > + libxl_devid usbctrl, int *num);
>
> I'd probably say "..._for_usbctrl" or "..._by_usbctrl", but that's just
> nitpicking.
>
> > +
> > +void libxl_device_usb_list_free(libxl_device_usb *list, int nr);
> > +
> > +int libxl_device_usb_getinfo(libxl_ctx *ctx, uint32_t domid,
> > + libxl_device_usb *usb,
> > + libxl_usbinfo *usbinfo);
> > +
> > /* 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_types.idl b/tools/libxl/libxl_types.idl
> > index ef346e7..ef10484 100644
> > --- a/tools/libxl/libxl_types.idl
> > +++ b/tools/libxl/libxl_types.idl
> > @@ -594,6 +594,37 @@ libxl_device_rdm = Struct("device_rdm", [
> > ("policy", libxl_rdm_reserve_policy),
> > ])
> >
> > +libxl_usbctrl_type = Enumeration("usbctrl_type", [
> > + (0, "AUTO"),
>
> What are the proposed semantics of using LIBXL_USBCTRL_TYPE_AUTO?
>
> > + (1, "PV"),
> > + (2, "QEMU"),
>
> Is "QEMU" what we want here, as opposed to, say, "EMU" (similar to NICs)?
>
> I think we probably don't want to go as fine grained as "XHCI" and "EHCI"
> etc, do we? I see we have a version field below, is it intended that there
> be some way to select between e.g. UHCI and OHCI (which IIRC are different
> USB 1.0 controllers).
>
> Maybe these questions should all be left aside for when QMEU support is
> actually added (AFAICT this field is just a placeholder)? In fact I glanced
> at the code and was surprised to find nothing checking for
> LIBXL_USBCTRL_TYPE at all, did I miss something?
>
> I think the two choices are:
>
> We can decide quickly and easily what the option(s) other than PV should be
> here and you include it in the IDL, but you would then need to check
> usbctrl->type == PV at various points, not silently treat all options as
> PV.
>
> Or this becomes a long conversation in which case I think your best bet
> would be to leave the enum with just the PV (and maybe AUTO) entries and
> leave the decision on the name for the emulated option to the series which
> implements that.
>
> > + ])
> > +
> > +libxl_usbdev_type = Enumeration("usbdev_type", [
> > + (0, "invalid"),
> > + (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_usb = Struct("device_usb", [
> > + ("ctrl", libxl_devid),
> > + ("port", integer),
> > + ("u", KeyedUnion(None, libxl_usbdev_type, "devtype",
> > + [("hostdev", Struct(None, [
> > + ("hostbus", integer),
> > + ("hostaddr", integer)])),
> > + ("invalid", None),
>
> AIUI this is what was agreed to, i.e. an enum with only one real option, in
> order to leave a space for new devtypes without major API overhaul.
>
> Please can you confirm that hostbus and hostaddr are both flat integer
> namespaces (i.e. there is no structure to the bits within either, they are
> just a number).
>
> Do these fields have any particular size requirements arising from e.g. the
> USB spec or from possible dom0 implementations?
>
> If they have a well defined fixed size from a USB spec then maybe we could
> use the appropriate fixed size types?
Didn't see the size limitation. In Linux kernel code, busnum and devnum (here
'hostbus, hostaddr') are both 'int' type. And idProduct and idVendor are 'u16'.
- Chunyan
>
> > + ])),
> > + ])
> > +
> > libxl_device_dtdev = Struct("device_dtdev", [
> > ("path", string),
> > ])
> > @@ -626,6 +657,8 @@ libxl_domain_config = Struct("domain_config", [
> > ("pcidevs", Array(libxl_device_pci, "num_pcidevs")),
> > ("rdms", Array(libxl_device_rdm, "num_rdms")),
> > ("dtdevs", Array(libxl_device_dtdev, "num_dtdevs")),
> > + ("usbctrls", Array(libxl_device_usbctrl, "num_usbctrls")),
> > + ("usbs", Array(libxl_device_usb, "num_usbs")),
> > ("vfbs", Array(libxl_device_vfb, "num_vfbs")),
> > ("vkbs", Array(libxl_device_vkb, "num_vkbs")),
> > ("vtpms", Array(libxl_device_vtpm, "num_vtpms")),
> > @@ -674,6 +707,32 @@ 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_usbinfo = Struct("usbinfo", [
> > + ("ctrl", libxl_devid),
> > + ("port", integer),
> > + ("busnum", integer),
> > + ("devnum", integer),
> > + ("idVendor", integer),
> > + ("idProduct", integer),
>
> I think id* are 16 bits? uint16 might be better then.
>
> > + ("prod", string),
> > + ("manuf", string),
> > + ], dir=DIR_OUT)
> > +
> > libxl_vcpuinfo = Struct("vcpuinfo", [
> > ("vcpuid", uint32),
> > ("cpu", uint32),
>
>
>
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH V6 3/7] libxl: add pvusb API
2015-09-11 5:42 ` Chun Yan Liu
@ 2015-09-11 13:26 ` Ian Campbell
2015-09-11 13:55 ` Juergen Gross
2015-09-15 8:14 ` Chun Yan Liu
0 siblings, 2 replies; 41+ messages in thread
From: Ian Campbell @ 2015-09-11 13:26 UTC (permalink / raw)
To: Chun Yan Liu, xen-devel
Cc: Juergen Gross, wei.liu2, george.dunlap, Ian.Jackson, Jim Fehlig,
Simon Cao
On Thu, 2015-09-10 at 23:42 -0600, Chun Yan Liu wrote:
>
> > Do these fields have any particular size requirements arising from e.g. the
> > USB spec or from possible dom0 implementations?
> >
> > If they have a well defined fixed size from a USB spec then maybe we
> > could
> > use the appropriate fixed size types?
>
> Di> dn't see the size limitation. In Linux kernel code, busnum and devnum (here
> 'hostbus, hostaddr') are both 'int' type.
Is that a Linux-specific implementation detail or a fundamental property of
USB? We should be designing the interface around Linux implementation
details. It seems like something in the USB spec ought to define precisely
the number of bits in both a bus number and a device address within that
bus.
Note also that integer in the libxl IDL is signed 24 bits.
> And idProduct and idVendor are 'u16'.
That's a USB spec thing, I think, so int16 in the IDL seems appropriate.
Ian.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH V6 3/7] libxl: add pvusb API
2015-09-11 13:26 ` Ian Campbell
@ 2015-09-11 13:55 ` Juergen Gross
2015-09-11 14:09 ` Ian Campbell
2015-09-15 8:14 ` Chun Yan Liu
1 sibling, 1 reply; 41+ messages in thread
From: Juergen Gross @ 2015-09-11 13:55 UTC (permalink / raw)
To: Ian Campbell, Chun Yan Liu, xen-devel
Cc: george.dunlap, Ian.Jackson, Jim Fehlig, Simon Cao, wei.liu2
On 09/11/2015 03:26 PM, Ian Campbell wrote:
> On Thu, 2015-09-10 at 23:42 -0600, Chun Yan Liu wrote:
>>
>>> Do these fields have any particular size requirements arising from e.g. the
>>> USB spec or from possible dom0 implementations?
>>>
>>> If they have a well defined fixed size from a USB spec then maybe we
>>> could
>>> use the appropriate fixed size types?
>>
>> Di> dn't see the size limitation. In Linux kernel code, busnum and devnum (here
>> 'hostbus, hostaddr') are both 'int' type.
>
> Is that a Linux-specific implementation detail or a fundamental property of
> USB? We should be designing the interface around Linux implementation
> details. It seems like something in the USB spec ought to define precisely
> the number of bits in both a bus number and a device address within that
> bus.
The USB spec is only about _the_ bus. How many buses a host can
operate and how they are numbered is outside the USB spec.
Devices are addressed via their ports in the USB protocol. devnum
is a unique index for a device on the bus, the USB protocol equivalent
is a list of ports of:
- 1 member in case of direct attached devices
- multiple members in case of hubs between bus and device
>
> Note also that integer in the libxl IDL is signed 24 bits.
>
>> And idProduct and idVendor are 'u16'.
>
> That's a USB spec thing, I think, so int16 in the IDL seems appropriate.
Correct.
Juergen
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH V6 3/7] libxl: add pvusb API
2015-09-11 13:55 ` Juergen Gross
@ 2015-09-11 14:09 ` Ian Campbell
2015-09-11 14:18 ` Juergen Gross
0 siblings, 1 reply; 41+ messages in thread
From: Ian Campbell @ 2015-09-11 14:09 UTC (permalink / raw)
To: Juergen Gross, Chun Yan Liu, xen-devel
Cc: george.dunlap, Ian.Jackson, Jim Fehlig, Simon Cao, wei.liu2
On Fri, 2015-09-11 at 15:55 +0200, Juergen Gross wrote:
> On 09/11/2015 03:26 PM, Ian Campbell wrote:
> > On Thu, 2015-09-10 at 23:42 -0600, Chun Yan Liu wrote:
> > >
> > > > Do these fields have any particular size requirements arising from
> > > > e.g. the
> > > > USB spec or from possible dom0 implementations?
> > > >
> > > > If they have a well defined fixed size from a USB spec then maybe
> > > > we
> > > > could
> > > > use the appropriate fixed size types?
> > >
> > > Di> dn't see the size limitation. In Linux kernel code, busnum and
> > > devnum (here
> > > 'hostbus, hostaddr') are both 'int' type.
> >
> > Is that a Linux-specific implementation detail or a fundamental
> > property of
> > USB? We should be designing the interface around Linux implementation
> > details. It seems like something in the USB spec ought to define
> > precisely
> > the number of bits in both a bus number and a device address within
> > that
> > bus.
>
> The USB spec is only about _the_ bus. How many buses a host can
> operate and how they are numbered is outside the USB spec.
>
> Devices are addressed via their ports in the USB protocol. devnum
> is a unique index for a device on the bus, the USB protocol equivalent
> is a list of ports of:
> - 1 member in case of direct attached devices
> - multiple members in case of hubs between bus and device
Thanks for the info. So an "address" in the USB protocol is actually a
"path" and "hostbus" is an implementation dependent shorthand for all but
the last link in that path.
What is the size of each element in the chain, that would seem to be the
correct size of "hostaddr".
Ian.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH V6 3/7] libxl: add pvusb API
2015-09-11 14:09 ` Ian Campbell
@ 2015-09-11 14:18 ` Juergen Gross
2015-09-11 14:41 ` Ian Campbell
0 siblings, 1 reply; 41+ messages in thread
From: Juergen Gross @ 2015-09-11 14:18 UTC (permalink / raw)
To: Ian Campbell, Chun Yan Liu, xen-devel
Cc: george.dunlap, Ian.Jackson, Jim Fehlig, Simon Cao, wei.liu2
On 09/11/2015 04:09 PM, Ian Campbell wrote:
> On Fri, 2015-09-11 at 15:55 +0200, Juergen Gross wrote:
>> On 09/11/2015 03:26 PM, Ian Campbell wrote:
>>> On Thu, 2015-09-10 at 23:42 -0600, Chun Yan Liu wrote:
>>>>
>>>>> Do these fields have any particular size requirements arising from
>>>>> e.g. the
>>>>> USB spec or from possible dom0 implementations?
>>>>>
>>>>> If they have a well defined fixed size from a USB spec then maybe
>>>>> we
>>>>> could
>>>>> use the appropriate fixed size types?
>>>>
>>>> Di> dn't see the size limitation. In Linux kernel code, busnum and
>>>> devnum (here
>>>> 'hostbus, hostaddr') are both 'int' type.
>>>
>>> Is that a Linux-specific implementation detail or a fundamental
>>> property of
>>> USB? We should be designing the interface around Linux implementation
>>> details. It seems like something in the USB spec ought to define
>>> precisely
>>> the number of bits in both a bus number and a device address within
>>> that
>>> bus.
>>
>> The USB spec is only about _the_ bus. How many buses a host can
>> operate and how they are numbered is outside the USB spec.
>>
>> Devices are addressed via their ports in the USB protocol. devnum
>> is a unique index for a device on the bus, the USB protocol equivalent
>> is a list of ports of:
>> - 1 member in case of direct attached devices
>> - multiple members in case of hubs between bus and device
>
> Thanks for the info. So an "address" in the USB protocol is actually a
> "path" and "hostbus" is an implementation dependent shorthand for all but
> the last link in that path.
I'm not sure in which direction you are looking. "address" is a path.
A path is normally a list of ports starting at the host and walking
through all hubs until you reach the device. The "bus" is the root
of that path. So the number of buses the host knows of is the number
of USB host adapters without any hub.
> What is the size of each element in the chain, that would seem to be the
> correct size of "hostaddr".
One bus can have up to 31 ports. In theory I think up to 7 cascaded
hubs are possible, but I don't think the resulting theoretical maximum
of about 1 trillion devices on a bus is to be considered. :-)
Juergen
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH V6 3/7] libxl: add pvusb API
2015-09-11 14:18 ` Juergen Gross
@ 2015-09-11 14:41 ` Ian Campbell
2015-09-11 15:42 ` Ian Jackson
2015-09-14 3:48 ` Juergen Gross
0 siblings, 2 replies; 41+ messages in thread
From: Ian Campbell @ 2015-09-11 14:41 UTC (permalink / raw)
To: Juergen Gross, Chun Yan Liu, xen-devel
Cc: george.dunlap, Ian.Jackson, Jim Fehlig, Simon Cao, wei.liu2
On Fri, 2015-09-11 at 16:18 +0200, Juergen Gross wrote:
> On 09/11/2015 04:09 PM, Ian Campbell wrote:
> > On Fri, 2015-09-11 at 15:55 +0200, Juergen Gross wrote:
> > > On 09/11/2015 03:26 PM, Ian Campbell wrote:
> > > > On Thu, 2015-09-10 at 23:42 -0600, Chun Yan Liu wrote:
> > > > >
> > > > > > Do these fields have any particular size requirements arising
> > > > > > from
> > > > > > e.g. the
> > > > > > USB spec or from possible dom0 implementations?
> > > > > >
> > > > > > If they have a well defined fixed size from a USB spec then
> > > > > > maybe
> > > > > > we
> > > > > > could
> > > > > > use the appropriate fixed size types?
> > > > >
> > > > > Di> dn't see the size limitation. In Linux kernel code, busnum
> > > > > and
> > > > > devnum (here
> > > > > 'hostbus, hostaddr') are both 'int' type.
> > > >
> > > > Is that a Linux-specific implementation detail or a fundamental
> > > > property of
> > > > USB? We should be designing the interface around Linux
> > > > implementation
> > > > details. It seems like something in the USB spec ought to define
> > > > precisely
> > > > the number of bits in both a bus number and a device address within
> > > > that
> > > > bus.
> > >
> > > The USB spec is only about _the_ bus. How many buses a host can
> > > operate and how they are numbered is outside the USB spec.
> > >
> > > Devices are addressed via their ports in the USB protocol. devnum
> > > is a unique index for a device on the bus, the USB protocol
> > > equivalent
> > > is a list of ports of:
> > > - 1 member in case of direct attached devices
> > > - multiple members in case of hubs between bus and device
> >
> > Thanks for the info. So an "address" in the USB protocol is actually a
> > "path" and "hostbus" is an implementation dependent shorthand for all
> > but
> > the last link in that path.
>
> I'm not sure in which direction you are looking. "address" is a path.
> A path is normally a list of ports starting at the host and walking
> through all hubs until you reach the device. The "bus" is the root
> of that path. So the number of buses the host knows of is the number
> of USB host adapters without any hub.
OK, I thought I understood but the above suggests not.
In USB speak, the address is a list of port numbers, which you follow from
the host bus which is the root.
In Linux speak a "bus" is actually each hub along that path.
Let me try a worked example and see if I've got it right. Lets take this
topology:
ROOT0
|-PORT0 ----+--HUB1
|-PORT1-, |-PORT0 -- DEVICE A
| `-PORT1 -- DEVICE B
|
`--HUB2
|-PORT0 -- DEVICE C
`-PORT1 -- HUB3
|-PORT0 -- DEVICE D
`-PORT1 -x
ROOT1 -- ... other stuff
In the USB protocol there are two buses corresponding to ROOT0 and ROOT1.
So in the protocol the address of DEVICE D on the bus associated with ROOT0
is [1,1,0], that is PORT1 on ROOT0 => PORT1 on HUB2 => PORT0 on HUB3.
DEVICE A is [0,0] on the bus associated with ROOT0, similarly.
In the Linux numbering scheme each ROOTn or HUBn is given a bus number,
somewhat arbitrarily (although I'm sure there is a scheme by which they
allocated). So perhaps:
ROOT0==BUS1
HUB1==BUS2
HUB2==BUS2
HUB3==BUS4
ROOT1==BUS42
And in this scheme the address is hostbus+hostaddr, so DEVICE D is [3,0],
that is hostbus==3==HUB3, and port 0. And DEVICE A is [2,0]
Is that right?
> One bus can have up to 31 ports.
So the answer is that hostaddr can be 5 bits?
> In theory I think up to 7 cascaded
> hubs are possible, but I don't think the resulting theoretical maximum
> of about 1 trillion devices on a bus is to be considered. :-)
And this suggests that in principal a Linux hostbus could be 5*7 bits == 35
bits, maybe. Or at least that any USB address can be encoded in that many
bits.
Ian.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH V6 3/7] libxl: add pvusb API
2015-09-11 14:41 ` Ian Campbell
@ 2015-09-11 15:42 ` Ian Jackson
2015-09-14 3:48 ` Juergen Gross
1 sibling, 0 replies; 41+ messages in thread
From: Ian Jackson @ 2015-09-11 15:42 UTC (permalink / raw)
To: Ian Campbell
Cc: Juergen Gross, wei.liu2, george.dunlap, Ian.Jackson, Chun Yan Liu,
xen-devel, Jim Fehlig, Simon Cao
Most of what you say is right, I think, but:
Ian Campbell writes ("Re: [Xen-devel] [PATCH V6 3/7] libxl: add pvusb API"):
> On Fri, 2015-09-11 at 16:18 +0200, Juergen Gross wrote:
> > In theory I think up to 7 cascaded
> > hubs are possible, but I don't think the resulting theoretical maximum
> > of about 1 trillion devices on a bus is to be considered. :-)
>
> And this suggests that in principal a Linux hostbus could be 5*7 bits == 35
> bits, maybe. Or at least that any USB address can be encoded in that many
> bits.
No. The total possible number of `buses' in that sense
per root is:
1 root
+ 31 in theory a hub plugged into each of 31 ports
+ 31^2 in theory a hub plugged into each port
...
+ 31^6
You could plug hubs into the last layer but the maximum depth is 7 so
you wouldn't be able to plug any devices into that last hub.
And of course the number of root hubs is not really limited by
anything meaningful.
Ian.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH V6 3/7] libxl: add pvusb API
2015-09-11 14:41 ` Ian Campbell
2015-09-11 15:42 ` Ian Jackson
@ 2015-09-14 3:48 ` Juergen Gross
2015-09-14 10:36 ` George Dunlap
1 sibling, 1 reply; 41+ messages in thread
From: Juergen Gross @ 2015-09-14 3:48 UTC (permalink / raw)
To: Ian Campbell, Chun Yan Liu, xen-devel
Cc: george.dunlap, Ian.Jackson, Jim Fehlig, Simon Cao, wei.liu2
On 09/11/2015 04:41 PM, Ian Campbell wrote:
> On Fri, 2015-09-11 at 16:18 +0200, Juergen Gross wrote:
>> On 09/11/2015 04:09 PM, Ian Campbell wrote:
>>> On Fri, 2015-09-11 at 15:55 +0200, Juergen Gross wrote:
>>>> On 09/11/2015 03:26 PM, Ian Campbell wrote:
>>>>> On Thu, 2015-09-10 at 23:42 -0600, Chun Yan Liu wrote:
>>>>>>
>>>>>>> Do these fields have any particular size requirements arising
>>>>>>> from
>>>>>>> e.g. the
>>>>>>> USB spec or from possible dom0 implementations?
>>>>>>>
>>>>>>> If they have a well defined fixed size from a USB spec then
>>>>>>> maybe
>>>>>>> we
>>>>>>> could
>>>>>>> use the appropriate fixed size types?
>>>>>>
>>>>>> Di> dn't see the size limitation. In Linux kernel code, busnum
>>>>>> and
>>>>>> devnum (here
>>>>>> 'hostbus, hostaddr') are both 'int' type.
>>>>>
>>>>> Is that a Linux-specific implementation detail or a fundamental
>>>>> property of
>>>>> USB? We should be designing the interface around Linux
>>>>> implementation
>>>>> details. It seems like something in the USB spec ought to define
>>>>> precisely
>>>>> the number of bits in both a bus number and a device address within
>>>>> that
>>>>> bus.
>>>>
>>>> The USB spec is only about _the_ bus. How many buses a host can
>>>> operate and how they are numbered is outside the USB spec.
>>>>
>>>> Devices are addressed via their ports in the USB protocol. devnum
>>>> is a unique index for a device on the bus, the USB protocol
>>>> equivalent
>>>> is a list of ports of:
>>>> - 1 member in case of direct attached devices
>>>> - multiple members in case of hubs between bus and device
>>>
>>> Thanks for the info. So an "address" in the USB protocol is actually a
>>> "path" and "hostbus" is an implementation dependent shorthand for all
>>> but
>>> the last link in that path.
>>
>> I'm not sure in which direction you are looking. "address" is a path.
>> A path is normally a list of ports starting at the host and walking
>> through all hubs until you reach the device. The "bus" is the root
>> of that path. So the number of buses the host knows of is the number
>> of USB host adapters without any hub.
>
> OK, I thought I understood but the above suggests not.
>
> In USB speak, the address is a list of port numbers, which you follow from
> the host bus which is the root.
>
> In Linux speak a "bus" is actually each hub along that path.
No. Each hub is just a port which happens to have more ports behind it.
> Let me try a worked example and see if I've got it right. Lets take this
> topology:
>
> ROOT0
> |-PORT0 ----+--HUB1
> |-PORT1-, |-PORT0 -- DEVICE A
> | `-PORT1 -- DEVICE B
> |
> `--HUB2
> |-PORT0 -- DEVICE C
> `-PORT1 -- HUB3
> |-PORT0 -- DEVICE D
> `-PORT1 -x
>
> ROOT1 -- ... other stuff
>
> In the USB protocol there are two buses corresponding to ROOT0 and ROOT1.
>
> So in the protocol the address of DEVICE D on the bus associated with ROOT0
> is [1,1,0], that is PORT1 on ROOT0 => PORT1 on HUB2 => PORT0 on HUB3.
>
> DEVICE A is [0,0] on the bus associated with ROOT0, similarly.
>
> In the Linux numbering scheme each ROOTn or HUBn is given a bus number,
> somewhat arbitrarily (although I'm sure there is a scheme by which they
> allocated). So perhaps:
>
> ROOT0==BUS1
Correct.
> HUB1==BUS2
No, Just Bus1-Port0 or Bus1:Devnum1
> HUB2==BUS2
Bus1-Port1 or Bus1:Devnum2
> HUB3==BUS4
Bus1-Port1.Port1 or Bus1:Devnum3
> ROOT1==BUS42
Bus2
> And in this scheme the address is hostbus+hostaddr, so DEVICE D is [3,0],
> that is hostbus==3==HUB3, and port 0. And DEVICE A is [2,0]
Device D: Bus1-Port1.Port1.Port0 or e.g. Bus1:Devnum4
Device A: Bus1-Port0.Port0 or e.g. Bus1:Devnum5
> Is that right?
>
>> One bus can have up to 31 ports.
>
> So the answer is that hostaddr can be 5 bits?
5*8 (7 hubs and a device at the last level) == 40, that's the 1 trillion
I suggested before. Things are a little bit more complicated. A devnum
in a bus is never assigned twice. So when you plug in a device, it might
get devnum 6. Unplug it and replug it again will lead to devnum 7.
>> In theory I think up to 7 cascaded
>> hubs are possible, but I don't think the resulting theoretical maximum
>> of about 1 trillion devices on a bus is to be considered. :-)
>
> And this suggests that in principal a Linux hostbus could be 5*7 bits == 35
> bits, maybe. Or at least that any USB address can be encoded in that many
> bits.
Busnum can grow to arbitrary values. A new USB bus detected will get a
new bus number. Removing it and adding it again will again use a new
number.
Juergen
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH V6 3/7] libxl: add pvusb API
2015-09-14 3:48 ` Juergen Gross
@ 2015-09-14 10:36 ` George Dunlap
2015-09-14 10:53 ` Juergen Gross
0 siblings, 1 reply; 41+ messages in thread
From: George Dunlap @ 2015-09-14 10:36 UTC (permalink / raw)
To: Juergen Gross
Cc: Wei Liu, Ian Campbell, Ian Jackson, Chun Yan Liu,
xen-devel@lists.xen.org, Jim Fehlig, Simon Cao
On Mon, Sep 14, 2015 at 4:48 AM, Juergen Gross <jgross@suse.com> wrote:
> On 09/11/2015 04:41 PM, Ian Campbell wrote:
>>
>> On Fri, 2015-09-11 at 16:18 +0200, Juergen Gross wrote:
>>>
>>> On 09/11/2015 04:09 PM, Ian Campbell wrote:
>>>>
>>>> On Fri, 2015-09-11 at 15:55 +0200, Juergen Gross wrote:
>>>>>
>>>>> On 09/11/2015 03:26 PM, Ian Campbell wrote:
>>>>>>
>>>>>> On Thu, 2015-09-10 at 23:42 -0600, Chun Yan Liu wrote:
>>>>>>>
>>>>>>>
>>>>>>>> Do these fields have any particular size requirements arising
>>>>>>>> from
>>>>>>>> e.g. the
>>>>>>>> USB spec or from possible dom0 implementations?
>>>>>>>>
>>>>>>>> If they have a well defined fixed size from a USB spec then
>>>>>>>> maybe
>>>>>>>> we
>>>>>>>> could
>>>>>>>> use the appropriate fixed size types?
>>>>>>>
>>>>>>>
>>>>>>> Di> dn't see the size limitation. In Linux kernel code, busnum
>>>>>>> and
>>>>>>> devnum (here
>>>>>>> 'hostbus, hostaddr') are both 'int' type.
>>>>>>
>>>>>>
>>>>>> Is that a Linux-specific implementation detail or a fundamental
>>>>>> property of
>>>>>> USB? We should be designing the interface around Linux
>>>>>> implementation
>>>>>> details. It seems like something in the USB spec ought to define
>>>>>> precisely
>>>>>> the number of bits in both a bus number and a device address within
>>>>>> that
>>>>>> bus.
>>>>>
>>>>>
>>>>> The USB spec is only about _the_ bus. How many buses a host can
>>>>> operate and how they are numbered is outside the USB spec.
>>>>>
>>>>> Devices are addressed via their ports in the USB protocol. devnum
>>>>> is a unique index for a device on the bus, the USB protocol
>>>>> equivalent
>>>>> is a list of ports of:
>>>>> - 1 member in case of direct attached devices
>>>>> - multiple members in case of hubs between bus and device
>>>>
>>>>
>>>> Thanks for the info. So an "address" in the USB protocol is actually a
>>>> "path" and "hostbus" is an implementation dependent shorthand for all
>>>> but
>>>> the last link in that path.
>>>
>>>
>>> I'm not sure in which direction you are looking. "address" is a path.
>>> A path is normally a list of ports starting at the host and walking
>>> through all hubs until you reach the device. The "bus" is the root
>>> of that path. So the number of buses the host knows of is the number
>>> of USB host adapters without any hub.
>>
>>
>> OK, I thought I understood but the above suggests not.
>>
>> In USB speak, the address is a list of port numbers, which you follow from
>> the host bus which is the root.
>>
>> In Linux speak a "bus" is actually each hub along that path.
>
>
> No. Each hub is just a port which happens to have more ports behind it.
>
>> Let me try a worked example and see if I've got it right. Lets take this
>> topology:
>>
>> ROOT0
>> |-PORT0 ----+--HUB1
>> |-PORT1-, |-PORT0 -- DEVICE A
>> | `-PORT1 -- DEVICE B
>> |
>> `--HUB2
>> |-PORT0 -- DEVICE C
>> `-PORT1 -- HUB3
>> |-PORT0 -- DEVICE D
>> `-PORT1 -x
>>
>> ROOT1 -- ... other stuff
>>
>> In the USB protocol there are two buses corresponding to ROOT0 and ROOT1.
>>
>> So in the protocol the address of DEVICE D on the bus associated with
>> ROOT0
>> is [1,1,0], that is PORT1 on ROOT0 => PORT1 on HUB2 => PORT0 on HUB3.
>>
>> DEVICE A is [0,0] on the bus associated with ROOT0, similarly.
>>
>> In the Linux numbering scheme each ROOTn or HUBn is given a bus number,
>> somewhat arbitrarily (although I'm sure there is a scheme by which they
>> allocated). So perhaps:
>>
>> ROOT0==BUS1
>
>
> Correct.
>
>> HUB1==BUS2
>
>
> No, Just Bus1-Port0 or Bus1:Devnum1
>
>> HUB2==BUS2
>
>
> Bus1-Port1 or Bus1:Devnum2
>
>> HUB3==BUS4
>
>
> Bus1-Port1.Port1 or Bus1:Devnum3
>
>> ROOT1==BUS42
>
>
> Bus2
>
>> And in this scheme the address is hostbus+hostaddr, so DEVICE D is [3,0],
>> that is hostbus==3==HUB3, and port 0. And DEVICE A is [2,0]
>
>
> Device D: Bus1-Port1.Port1.Port0 or e.g. Bus1:Devnum4
> Device A: Bus1-Port0.Port0 or e.g. Bus1:Devnum5
>
>> Is that right?
>>
>>> One bus can have up to 31 ports.
>>
>>
>> So the answer is that hostaddr can be 5 bits?
>
>
> 5*8 (7 hubs and a device at the last level) == 40, that's the 1 trillion
> I suggested before. Things are a little bit more complicated. A devnum
> in a bus is never assigned twice. So when you plug in a device, it might
> get devnum 6. Unplug it and replug it again will lead to devnum 7.
>
>>> In theory I think up to 7 cascaded
>>> hubs are possible, but I don't think the resulting theoretical maximum
>>> of about 1 trillion devices on a bus is to be considered. :-)
>>
>>
>> And this suggests that in principal a Linux hostbus could be 5*7 bits ==
>> 35
>> bits, maybe. Or at least that any USB address can be encoded in that many
>> bits.
>
>
> Busnum can grow to arbitrary values. A new USB bus detected will get a
> new bus number. Removing it and adding it again will again use a new
> number.
FWIW libusb seems to define these as uint8:
http://libusb.org/static/api-1.0/group__dev.html#gaf2718609d50c8ded2704e4051b3d2925
(I *think* that "bus number" and "device address" correspond to busnum
and devnum...)
Anyone want to look into the Linux source code to find out how big it
will allow busnum / devnum to grow?
-George
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH V6 3/7] libxl: add pvusb API
2015-09-14 10:36 ` George Dunlap
@ 2015-09-14 10:53 ` Juergen Gross
2015-09-14 11:12 ` Ian Jackson
0 siblings, 1 reply; 41+ messages in thread
From: Juergen Gross @ 2015-09-14 10:53 UTC (permalink / raw)
To: George Dunlap
Cc: Wei Liu, Ian Campbell, Ian Jackson, Chun Yan Liu,
xen-devel@lists.xen.org, Jim Fehlig, Simon Cao
On 09/14/2015 12:36 PM, George Dunlap wrote:
> On Mon, Sep 14, 2015 at 4:48 AM, Juergen Gross <jgross@suse.com> wrote:
>> On 09/11/2015 04:41 PM, Ian Campbell wrote:
>>>
>>> On Fri, 2015-09-11 at 16:18 +0200, Juergen Gross wrote:
>>>>
>>>> On 09/11/2015 04:09 PM, Ian Campbell wrote:
>>>>>
>>>>> On Fri, 2015-09-11 at 15:55 +0200, Juergen Gross wrote:
>>>>>>
>>>>>> On 09/11/2015 03:26 PM, Ian Campbell wrote:
>>>>>>>
>>>>>>> On Thu, 2015-09-10 at 23:42 -0600, Chun Yan Liu wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>> Do these fields have any particular size requirements arising
>>>>>>>>> from
>>>>>>>>> e.g. the
>>>>>>>>> USB spec or from possible dom0 implementations?
>>>>>>>>>
>>>>>>>>> If they have a well defined fixed size from a USB spec then
>>>>>>>>> maybe
>>>>>>>>> we
>>>>>>>>> could
>>>>>>>>> use the appropriate fixed size types?
>>>>>>>>
>>>>>>>>
>>>>>>>> Di> dn't see the size limitation. In Linux kernel code, busnum
>>>>>>>> and
>>>>>>>> devnum (here
>>>>>>>> 'hostbus, hostaddr') are both 'int' type.
>>>>>>>
>>>>>>>
>>>>>>> Is that a Linux-specific implementation detail or a fundamental
>>>>>>> property of
>>>>>>> USB? We should be designing the interface around Linux
>>>>>>> implementation
>>>>>>> details. It seems like something in the USB spec ought to define
>>>>>>> precisely
>>>>>>> the number of bits in both a bus number and a device address within
>>>>>>> that
>>>>>>> bus.
>>>>>>
>>>>>>
>>>>>> The USB spec is only about _the_ bus. How many buses a host can
>>>>>> operate and how they are numbered is outside the USB spec.
>>>>>>
>>>>>> Devices are addressed via their ports in the USB protocol. devnum
>>>>>> is a unique index for a device on the bus, the USB protocol
>>>>>> equivalent
>>>>>> is a list of ports of:
>>>>>> - 1 member in case of direct attached devices
>>>>>> - multiple members in case of hubs between bus and device
>>>>>
>>>>>
>>>>> Thanks for the info. So an "address" in the USB protocol is actually a
>>>>> "path" and "hostbus" is an implementation dependent shorthand for all
>>>>> but
>>>>> the last link in that path.
>>>>
>>>>
>>>> I'm not sure in which direction you are looking. "address" is a path.
>>>> A path is normally a list of ports starting at the host and walking
>>>> through all hubs until you reach the device. The "bus" is the root
>>>> of that path. So the number of buses the host knows of is the number
>>>> of USB host adapters without any hub.
>>>
>>>
>>> OK, I thought I understood but the above suggests not.
>>>
>>> In USB speak, the address is a list of port numbers, which you follow from
>>> the host bus which is the root.
>>>
>>> In Linux speak a "bus" is actually each hub along that path.
>>
>>
>> No. Each hub is just a port which happens to have more ports behind it.
>>
>>> Let me try a worked example and see if I've got it right. Lets take this
>>> topology:
>>>
>>> ROOT0
>>> |-PORT0 ----+--HUB1
>>> |-PORT1-, |-PORT0 -- DEVICE A
>>> | `-PORT1 -- DEVICE B
>>> |
>>> `--HUB2
>>> |-PORT0 -- DEVICE C
>>> `-PORT1 -- HUB3
>>> |-PORT0 -- DEVICE D
>>> `-PORT1 -x
>>>
>>> ROOT1 -- ... other stuff
>>>
>>> In the USB protocol there are two buses corresponding to ROOT0 and ROOT1.
>>>
>>> So in the protocol the address of DEVICE D on the bus associated with
>>> ROOT0
>>> is [1,1,0], that is PORT1 on ROOT0 => PORT1 on HUB2 => PORT0 on HUB3.
>>>
>>> DEVICE A is [0,0] on the bus associated with ROOT0, similarly.
>>>
>>> In the Linux numbering scheme each ROOTn or HUBn is given a bus number,
>>> somewhat arbitrarily (although I'm sure there is a scheme by which they
>>> allocated). So perhaps:
>>>
>>> ROOT0==BUS1
>>
>>
>> Correct.
>>
>>> HUB1==BUS2
>>
>>
>> No, Just Bus1-Port0 or Bus1:Devnum1
>>
>>> HUB2==BUS2
>>
>>
>> Bus1-Port1 or Bus1:Devnum2
>>
>>> HUB3==BUS4
>>
>>
>> Bus1-Port1.Port1 or Bus1:Devnum3
>>
>>> ROOT1==BUS42
>>
>>
>> Bus2
>>
>>> And in this scheme the address is hostbus+hostaddr, so DEVICE D is [3,0],
>>> that is hostbus==3==HUB3, and port 0. And DEVICE A is [2,0]
>>
>>
>> Device D: Bus1-Port1.Port1.Port0 or e.g. Bus1:Devnum4
>> Device A: Bus1-Port0.Port0 or e.g. Bus1:Devnum5
>>
>>> Is that right?
>>>
>>>> One bus can have up to 31 ports.
>>>
>>>
>>> So the answer is that hostaddr can be 5 bits?
>>
>>
>> 5*8 (7 hubs and a device at the last level) == 40, that's the 1 trillion
>> I suggested before. Things are a little bit more complicated. A devnum
>> in a bus is never assigned twice. So when you plug in a device, it might
>> get devnum 6. Unplug it and replug it again will lead to devnum 7.
>>
>>>> In theory I think up to 7 cascaded
>>>> hubs are possible, but I don't think the resulting theoretical maximum
>>>> of about 1 trillion devices on a bus is to be considered. :-)
>>>
>>>
>>> And this suggests that in principal a Linux hostbus could be 5*7 bits ==
>>> 35
>>> bits, maybe. Or at least that any USB address can be encoded in that many
>>> bits.
>>
>>
>> Busnum can grow to arbitrary values. A new USB bus detected will get a
>> new bus number. Removing it and adding it again will again use a new
>> number.
>
> FWIW libusb seems to define these as uint8:
>
> http://libusb.org/static/api-1.0/group__dev.html#gaf2718609d50c8ded2704e4051b3d2925
>
> (I *think* that "bus number" and "device address" correspond to busnum
> and devnum...)
>
> Anyone want to look into the Linux source code to find out how big it
> will allow busnum / devnum to grow?
drivers/usb/core/hcd.c is using a bitmap to find the next bus number
currently not in use. It's size is USB_MAXBUS which in turn has the
value 64.
choose_devnum() in drivers/usb/core/hub.c is doing a similar job for
device numbers. Here the highest number supported is 127.
Juergen
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH V6 3/7] libxl: add pvusb API
2015-09-14 10:53 ` Juergen Gross
@ 2015-09-14 11:12 ` Ian Jackson
2015-09-14 11:23 ` Juergen Gross
2015-09-14 14:03 ` George Dunlap
0 siblings, 2 replies; 41+ messages in thread
From: Ian Jackson @ 2015-09-14 11:12 UTC (permalink / raw)
To: Juergen Gross
Cc: Wei Liu, Ian Campbell, George Dunlap, Chun Yan Liu,
xen-devel@lists.xen.org, Jim Fehlig, Simon Cao
Juergen Gross writes ("Re: [Xen-devel] [PATCH V6 3/7] libxl: add pvusb API"):
> On 09/14/2015 12:36 PM, George Dunlap wrote:
> > Anyone want to look into the Linux source code to find out how big it
> > will allow busnum / devnum to grow?
>
> drivers/usb/core/hcd.c is using a bitmap to find the next bus number
> currently not in use. It's size is USB_MAXBUS which in turn has the
> value 64.
>
> choose_devnum() in drivers/usb/core/hub.c is doing a similar job for
> device numbers. Here the highest number supported is 127.
We are defining an API, which shouldn't involve this kind of
implementation-grobbling.
At an API level, it seems that this Linux busnum is not documented to
have any particular number or behaviour or range or anything. We
should use the biggest type we can use conveniently.
Do we need to worry that some bus might have 2^24 unplugs/plugs
(perhaps in some kind of software emulation) and that we need to use a
type which can hold a uint32_t or maybe even a uint64_t ?
Ian.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH V6 3/7] libxl: add pvusb API
2015-09-14 11:12 ` Ian Jackson
@ 2015-09-14 11:23 ` Juergen Gross
2015-09-14 14:03 ` George Dunlap
1 sibling, 0 replies; 41+ messages in thread
From: Juergen Gross @ 2015-09-14 11:23 UTC (permalink / raw)
To: Ian Jackson
Cc: Wei Liu, Ian Campbell, George Dunlap, Chun Yan Liu,
xen-devel@lists.xen.org, Jim Fehlig, Simon Cao
On 09/14/2015 01:12 PM, Ian Jackson wrote:
> Juergen Gross writes ("Re: [Xen-devel] [PATCH V6 3/7] libxl: add pvusb API"):
>> On 09/14/2015 12:36 PM, George Dunlap wrote:
>>> Anyone want to look into the Linux source code to find out how big it
>>> will allow busnum / devnum to grow?
>>
>> drivers/usb/core/hcd.c is using a bitmap to find the next bus number
>> currently not in use. It's size is USB_MAXBUS which in turn has the
>> value 64.
>>
>> choose_devnum() in drivers/usb/core/hub.c is doing a similar job for
>> device numbers. Here the highest number supported is 127.
>
> We are defining an API, which shouldn't involve this kind of
> implementation-grobbling.
>
> At an API level, it seems that this Linux busnum is not documented to
> have any particular number or behaviour or range or anything. We
> should use the biggest type we can use conveniently.
Agreed.
> Do we need to worry that some bus might have 2^24 unplugs/plugs
> (perhaps in some kind of software emulation) and that we need to use a
> type which can hold a uint32_t or maybe even a uint64_t ?
uint128_t ? ;-)
I think 24 bits should be more than enough. Nobody will accept such huge
numbers without any need: they are to be used by users.
Juergen
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH V6 3/7] libxl: add pvusb API
2015-09-14 11:12 ` Ian Jackson
2015-09-14 11:23 ` Juergen Gross
@ 2015-09-14 14:03 ` George Dunlap
2015-09-17 8:24 ` Chun Yan Liu
1 sibling, 1 reply; 41+ messages in thread
From: George Dunlap @ 2015-09-14 14:03 UTC (permalink / raw)
To: Ian Jackson
Cc: Juergen Gross, Wei Liu, Ian Campbell, Chun Yan Liu,
xen-devel@lists.xen.org, Jim Fehlig, Simon Cao
On Mon, Sep 14, 2015 at 12:12 PM, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:
> Juergen Gross writes ("Re: [Xen-devel] [PATCH V6 3/7] libxl: add pvusb API"):
>> On 09/14/2015 12:36 PM, George Dunlap wrote:
>> > Anyone want to look into the Linux source code to find out how big it
>> > will allow busnum / devnum to grow?
>>
>> drivers/usb/core/hcd.c is using a bitmap to find the next bus number
>> currently not in use. It's size is USB_MAXBUS which in turn has the
>> value 64.
>>
>> choose_devnum() in drivers/usb/core/hub.c is doing a similar job for
>> device numbers. Here the highest number supported is 127.
>
> We are defining an API, which shouldn't involve this kind of
> implementation-grobbling.
>
> At an API level, it seems that this Linux busnum is not documented to
> have any particular number or behaviour or range or anything. We
> should use the biggest type we can use conveniently
>
> Do we need to worry that some bus might have 2^24 unplugs/plugs
> (perhaps in some kind of software emulation) and that we need to use a
> type which can hold a uint32_t or maybe even a uint64_t ?
libusb is already a published API that supports uint8, or up to 255.
Following their lead seems like a reasonable thing to do. If ever
that number goes above 255, basically every Linux program that touches
a USB device will need to be recompiled with a new version of libusb.
Is there any reason for Linux to go above 255? Things I can think of:
1. Users have more than 255 devices plugged into the same bus.
2. A security / confusion issue due to devnum reuse when users plug
and unplug devices hundreds of times.
Both of these seem pretty unlikely.
I would personally go with uint8, but int16 or int32 certainly won't hurt.
-George
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH V6 3/7] libxl: add pvusb API
2015-09-11 13:26 ` Ian Campbell
2015-09-11 13:55 ` Juergen Gross
@ 2015-09-15 8:14 ` Chun Yan Liu
1 sibling, 0 replies; 41+ messages in thread
From: Chun Yan Liu @ 2015-09-15 8:14 UTC (permalink / raw)
To: Ian Campbell, xen-devel
Cc: Juergen Gross, wei.liu2, george.dunlap, Ian.Jackson, Jim Fehlig,
Simon Cao
>>> On 9/11/2015 at 09:26 PM, in message <1441978018.3549.33.camel@citrix.com>, Ian
Campbell <ian.campbell@citrix.com> wrote:
> On Thu, 2015-09-10 at 23:42 -0600, Chun Yan Liu wrote:
> >
> > > Do these fields have any particular size requirements arising from e.g.
> the
> > > USB spec or from possible dom0 implementations?
> > >
> > > If they have a well defined fixed size from a USB spec then maybe we
> > > could
> > > use the appropriate fixed size types?
> >
> > Di> dn't see the size limitation. In Linux kernel code, busnum and devnum
> (here
> > 'hostbus, hostaddr') are both 'int' type.
>
> Is that a Linux-specific implementation detail or a fundamental property of
> USB? We should be designing the interface around Linux implementation
> details. It seems like something in the USB spec ought to define precisely
> the number of bits in both a bus number and a device address within that
> bus.
Have a look at USB 2.0 Spec, it has some description on Device Address: a seven-bit
value representing the address of the debvice on USB. (up to 127 devices). So
int8 is appropriate.
No description to Bus Num.
-Chunyan
>
> > And idProduct and idVendor are 'u16'.
>
> That's a USB spec thing, I think, so int16 in the IDL seems appropriate.
>
> Ian.
>
>
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH V6 3/7] libxl: add pvusb API
2015-09-08 16:52 ` George Dunlap
2015-09-09 7:38 ` Chun Yan Liu
@ 2015-09-17 8:19 ` Chun Yan Liu
2015-09-17 9:54 ` George Dunlap
2015-09-17 8:20 ` Chun Yan Liu
2 siblings, 1 reply; 41+ messages in thread
From: Chun Yan Liu @ 2015-09-17 8:19 UTC (permalink / raw)
To: George Dunlap, Ian Campbell, xen-devel
Cc: Juergen Gross, wei.liu2, george.dunlap, Ian.Jackson, Jim Fehlig,
Simon Cao
>>> On 9/9/2015 at 12:52 AM, in message <55EF1244.107@citrix.com>, George Dunlap
<george.dunlap@citrix.com> wrote:
> On 09/08/2015 03:17 PM, Ian Campbell wrote:
> > On Mon, 2015-08-10 at 18:35 +0800, Chunyan Liu wrote:
> >
> > Sorry for the delay, between 4.6 freeze crunch, conference and vacation
> > I've been a bit swamped.
> >
> > I'm just going to comment on the APIs (mainly public libxl.h and .idl) in
> > this pass.
> >
> >> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> >> index 5f9047c..05b6331 100644
> >> --- a/tools/libxl/libxl.h
> >> +++ b/tools/libxl/libxl.h
> >> @@ -123,6 +123,23 @@
> >> #define LIBXL_HAVE_DOMAIN_NODEAFFINITY 1
> >>
> >> /*
> >> + * LIBXL_HAVE_PVUSB indicates the functions for doing hot-plug of
> >
> > And cold-plug, no?
>
> So you should probably say something like "indicates functions for
> plugging in USB devices through pvusb -- both hotplug and at domain
> creation time."
>
> >> +libxl_usbctrl_type = Enumeration("usbctrl_type", [
> >> + (0, "AUTO"),
> >
> > What are the proposed semantics of using LIBXL_USBCTRL_TYPE_AUTO?
>
> Generally "DTRT". Meaning:
> 1. If your domain has no devicemodel, use PV.
> 2. If your device has a devicemodel, and no PV drivers have peen
> detected, use the devicemodel.
> 3. If your device has a devicemodel, but PV drivers have been detected,
> use PV.
>
> At the moment we don't have a way to check for PV drivers, so this just
> collapses down to "PV for domains without a DM and DM for domains with a
> DM."
>
> >
> >> + (1, "PV"),
> >> + (2, "QEMU"),
> >
> > Is "QEMU" what we want here, as opposed to, say, "EMU" (similar to NICs)?
>
> I had this as "DEVICEMODEL", since what we mean is that we want the
> device model to provide access (and in theory in the future we may use a
> different device model). But "EMU" works for me too.
>
> > I think we probably don't want to go as fine grained as "XHCI" and "EHCI"
> > etc, do we? I see we have a version field below, is it intended that there
> > be some way to select between e.g. UHCI and OHCI (which IIRC are different
> > USB 1.0 controllers).
> >
> > Maybe these questions should all be left aside for when QMEU support is
> > actually added (AFAICT this field is just a placeholder)? In fact I glanced
> > at the code and was surprised to find nothing checking for
> > LIBXL_USBCTRL_TYPE at all, did I miss something?
> >
> > I think the two choices are:
> >
> > We can decide quickly and easily what the option(s) other than PV should be
> > here and you include it in the IDL, but you would then need to check
> > usbctrl->type == PV at various points, not silently treat all options as
> > PV.
> >
> > Or this becomes a long conversation in which case I think your best bet
> > would be to leave the enum with just the PV (and maybe AUTO) entries and
> > leave the decision on the name for the emulated option to the series which
> > implements that.
>
> I think the idea was to simply offer 1, 2, and 3 as options, and for the
> devicemodel version, choose a suitable controller (or set of
> controllers) for each option; similar to what usbversion= does now.
Hi, George,
I'm still confused about the expected look concerning PV/EMU type handling in
this patch series.
In earlier version, we tried to extract common things in libxl_usb.c and put
pvusb specific thing in libxl_pvusb.c, prefixed with pvusb_xxx. As you
suggested, we can leave that when EMU USB patch series added.
Now, about how to handle PV/EMU type in this patch series, I can think of 3
ways:
1. We define the enumeration (contains PV/AUTO only, user interface only allows
'pv' or 'not specified', so we handle everything in 'pv' way without further
check. Leave check and other adjusting things when EMU USB patch series added.
2. We check domain type and set proper type if not specified (i.e. 'pv' for PV
guest, 'emu' for HVM guest). In add/remove function, check if type='emu', report
'not supported' directly; otherwise, continue do following things. When EMU USB
patch serires added, need to extract common things and adjust the check place.
3. Same as 2, but extract common things, only in PV/EMU USB specific part, check
type, if type='emu', report 'not supported'; otherwise, do pvusb work. When
adding EMU USB patch series, only need to add EMU USB specific things in the
type='emu' branch.
Which one is expected? Or none?
- Chunyan
>
> >
> >> + ])
> >> +
> >> +libxl_usbdev_type = Enumeration("usbdev_type", [
> >> + (0, "invalid"),
> >> + (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_usb = Struct("device_usb", [
> >> + ("ctrl", libxl_devid),
> >> + ("port", integer),
> >> + ("u", KeyedUnion(None, libxl_usbdev_type, "devtype",
> >> + [("hostdev", Struct(None, [
> >> + ("hostbus", integer),
> >> + ("hostaddr", integer)])),
> >> + ("invalid", None),
> >
> > AIUI this is what was agreed to, i.e. an enum with only one real option, in
> > order to leave a space for new devtypes without major API overhaul.
> >
> > Please can you confirm that hostbus and hostaddr are both flat integer
> > namespaces (i.e. there is no structure to the bits within either, they are
> > just a number).
>
> I can confirm this.
>
> -George
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>
>
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH V6 3/7] libxl: add pvusb API
2015-09-08 16:52 ` George Dunlap
2015-09-09 7:38 ` Chun Yan Liu
2015-09-17 8:19 ` Chun Yan Liu
@ 2015-09-17 8:20 ` Chun Yan Liu
2 siblings, 0 replies; 41+ messages in thread
From: Chun Yan Liu @ 2015-09-17 8:20 UTC (permalink / raw)
To: George Dunlap, Ian Campbell, xen-devel
Cc: Juergen Gross, wei.liu2, george.dunlap, Ian.Jackson, Jim Fehlig,
Simon Cao
>>> On 9/9/2015 at 12:52 AM, in message <55EF1244.107@citrix.com>, George Dunlap
<george.dunlap@citrix.com> wrote:
> On 09/08/2015 03:17 PM, Ian Campbell wrote:
> > On Mon, 2015-08-10 at 18:35 +0800, Chunyan Liu wrote:
> >
> > Sorry for the delay, between 4.6 freeze crunch, conference and vacation
> > I've been a bit swamped.
> >
> > I'm just going to comment on the APIs (mainly public libxl.h and .idl) in
> > this pass.
> >
> >> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> >> index 5f9047c..05b6331 100644
> >> --- a/tools/libxl/libxl.h
> >> +++ b/tools/libxl/libxl.h
> >> @@ -123,6 +123,23 @@
> >> #define LIBXL_HAVE_DOMAIN_NODEAFFINITY 1
> >>
> >> /*
> >> + * LIBXL_HAVE_PVUSB indicates the functions for doing hot-plug of
> >
> > And cold-plug, no?
>
> So you should probably say something like "indicates functions for
> plugging in USB devices through pvusb -- both hotplug and at domain
> creation time."
>
> >> +libxl_usbctrl_type = Enumeration("usbctrl_type", [
> >> + (0, "AUTO"),
> >
> > What are the proposed semantics of using LIBXL_USBCTRL_TYPE_AUTO?
>
> Generally "DTRT". Meaning:
> 1. If your domain has no devicemodel, use PV.
> 2. If your device has a devicemodel, and no PV drivers have peen
> detected, use the devicemodel.
> 3. If your device has a devicemodel, but PV drivers have been detected,
> use PV.
>
> At the moment we don't have a way to check for PV drivers, so this just
> collapses down to "PV for domains without a DM and DM for domains with a
> DM."
>
> >
> >> + (1, "PV"),
> >> + (2, "QEMU"),
> >
> > Is "QEMU" what we want here, as opposed to, say, "EMU" (similar to NICs)?
>
> I had this as "DEVICEMODEL", since what we mean is that we want the
> device model to provide access (and in theory in the future we may use a
> different device model). But "EMU" works for me too.
>
> > I think we probably don't want to go as fine grained as "XHCI" and "EHCI"
> > etc, do we? I see we have a version field below, is it intended that there
> > be some way to select between e.g. UHCI and OHCI (which IIRC are different
> > USB 1.0 controllers).
> >
> > Maybe these questions should all be left aside for when QMEU support is
> > actually added (AFAICT this field is just a placeholder)? In fact I glanced
> > at the code and was surprised to find nothing checking for
> > LIBXL_USBCTRL_TYPE at all, did I miss something?
> >
> > I think the two choices are:
> >
> > We can decide quickly and easily what the option(s) other than PV should be
> > here and you include it in the IDL, but you would then need to check
> > usbctrl->type == PV at various points, not silently treat all options as
> > PV.
> >
> > Or this becomes a long conversation in which case I think your best bet
> > would be to leave the enum with just the PV (and maybe AUTO) entries and
> > leave the decision on the name for the emulated option to the series which
> > implements that.
>
> I think the idea was to simply offer 1, 2, and 3 as options, and for the
> devicemodel version, choose a suitable controller (or set of
> controllers) for each option; similar to what usbversion= does now.
Hi, George,
I'm still confused about the expected look concerning PV/EMU type handling in
this patch series.
In earlier version, we tried to extract common things in libxl_usb.c and put
pvusb specific thing in libxl_pvusb.c, prefixed with pvusb_xxx. As you
suggested, we can leave that when EMU USB patch series added.
Now, about how to handle PV/EMU type in this patch series, I can think of 3
ways:
1. We define the enumeration (contains PV/AUTO only, user interface only allows
'pv' or 'not specified', so we handle everything in 'pv' way without further
check. Leave check and other adjusting things when EMU USB patch series added.
2. We check domain type and set proper type if not specified (i.e. 'pv' for PV
guest, 'emu' for HVM guest). In add/remove function, check if type='emu', report
'not supported' directly; otherwise, continue do following things. When EMU USB
patch serires added, need to extract common things and adjust the check place.
3. Same as 2, but extract common things, only in PV/EMU USB specific part, check
type, if type='emu', report 'not supported'; otherwise, do pvusb work. When
adding EMU USB patch series, only need to add EMU USB specific things in the
type='emu' branch.
Which one is expected? Or none?
- Chunyan
>
> >
> >> + ])
> >> +
> >> +libxl_usbdev_type = Enumeration("usbdev_type", [
> >> + (0, "invalid"),
> >> + (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_usb = Struct("device_usb", [
> >> + ("ctrl", libxl_devid),
> >> + ("port", integer),
> >> + ("u", KeyedUnion(None, libxl_usbdev_type, "devtype",
> >> + [("hostdev", Struct(None, [
> >> + ("hostbus", integer),
> >> + ("hostaddr", integer)])),
> >> + ("invalid", None),
> >
> > AIUI this is what was agreed to, i.e. an enum with only one real option, in
> > order to leave a space for new devtypes without major API overhaul.
> >
> > Please can you confirm that hostbus and hostaddr are both flat integer
> > namespaces (i.e. there is no structure to the bits within either, they are
> > just a number).
>
> I can confirm this.
>
> -George
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>
>
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH V6 3/7] libxl: add pvusb API
2015-09-14 14:03 ` George Dunlap
@ 2015-09-17 8:24 ` Chun Yan Liu
0 siblings, 0 replies; 41+ messages in thread
From: Chun Yan Liu @ 2015-09-17 8:24 UTC (permalink / raw)
To: George Dunlap, Ian Jackson
Cc: Juergen Gross, Wei Liu, Ian Campbell, xen-devel@lists.xen.org,
Jim Fehlig, Simon Cao
>>> On 9/14/2015 at 10:03 PM, in message
<CAFLBxZayaqTeJiB3RfG8qHxjCzQy8BBTe0Hxj+FT6ABSLf+Ntg@mail.gmail.com>, George
Dunlap <George.Dunlap@eu.citrix.com> wrote:
> On Mon, Sep 14, 2015 at 12:12 PM, Ian Jackson <Ian.Jackson@eu.citrix.com>
> wrote:
> > Juergen Gross writes ("Re: [Xen-devel] [PATCH V6 3/7] libxl: add pvusb
> API"):
> >> On 09/14/2015 12:36 PM, George Dunlap wrote:
> >> > Anyone want to look into the Linux source code to find out how big it
> >> > will allow busnum / devnum to grow?
> >>
> >> drivers/usb/core/hcd.c is using a bitmap to find the next bus number
> >> currently not in use. It's size is USB_MAXBUS which in turn has the
> >> value 64.
> >>
> >> choose_devnum() in drivers/usb/core/hub.c is doing a similar job for
> >> device numbers. Here the highest number supported is 127.
> >
> > We are defining an API, which shouldn't involve this kind of
> > implementation-grobbling.
> >
> > At an API level, it seems that this Linux busnum is not documented to
> > have any particular number or behaviour or range or anything. We
> > should use the biggest type we can use conveniently
> >
> > Do we need to worry that some bus might have 2^24 unplugs/plugs
> > (perhaps in some kind of software emulation) and that we need to use a
> > type which can hold a uint32_t or maybe even a uint64_t ?
>
> libusb is already a published API that supports uint8, or up to 255.
> Following their lead seems like a reasonable thing to do. If ever
> that number goes above 255, basically every Linux program that touches
> a USB device will need to be recompiled with a new version of libusb.
>
> Is there any reason for Linux to go above 255? Things I can think of:
>
> 1. Users have more than 255 devices plugged into the same bus.
>
> 2. A security / confusion issue due to devnum reuse when users plug
> and unplug devices hundreds of times.
>
> Both of these seem pretty unlikely.
>
> I would personally go with uint8, but int16 or int32 certainly won't hurt.
So can we agree to use uint8 for hostbus and hostaddr as libusb does?
>
> -George
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>
>
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH V6 3/7] libxl: add pvusb API
2015-09-17 8:19 ` Chun Yan Liu
@ 2015-09-17 9:54 ` George Dunlap
2015-09-29 17:19 ` Wei Liu
0 siblings, 1 reply; 41+ messages in thread
From: George Dunlap @ 2015-09-17 9:54 UTC (permalink / raw)
To: Chun Yan Liu, Ian Campbell, xen-devel
Cc: Juergen Gross, wei.liu2, george.dunlap, Ian.Jackson, Jim Fehlig,
Simon Cao
On 09/17/2015 09:19 AM, Chun Yan Liu wrote:
>
>
>>>> On 9/9/2015 at 12:52 AM, in message <55EF1244.107@citrix.com>, George Dunlap
> <george.dunlap@citrix.com> wrote:
>> On 09/08/2015 03:17 PM, Ian Campbell wrote:
>>> On Mon, 2015-08-10 at 18:35 +0800, Chunyan Liu wrote:
>>>
>>> Sorry for the delay, between 4.6 freeze crunch, conference and vacation
>>> I've been a bit swamped.
>>>
>>> I'm just going to comment on the APIs (mainly public libxl.h and .idl) in
>>> this pass.
>>>
>>>> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
>>>> index 5f9047c..05b6331 100644
>>>> --- a/tools/libxl/libxl.h
>>>> +++ b/tools/libxl/libxl.h
>>>> @@ -123,6 +123,23 @@
>>>> #define LIBXL_HAVE_DOMAIN_NODEAFFINITY 1
>>>>
>>>> /*
>>>> + * LIBXL_HAVE_PVUSB indicates the functions for doing hot-plug of
>>>
>>> And cold-plug, no?
>>
>> So you should probably say something like "indicates functions for
>> plugging in USB devices through pvusb -- both hotplug and at domain
>> creation time."
>>
>>>> +libxl_usbctrl_type = Enumeration("usbctrl_type", [
>>>> + (0, "AUTO"),
>>>
>>> What are the proposed semantics of using LIBXL_USBCTRL_TYPE_AUTO?
>>
>> Generally "DTRT". Meaning:
>> 1. If your domain has no devicemodel, use PV.
>> 2. If your device has a devicemodel, and no PV drivers have peen
>> detected, use the devicemodel.
>> 3. If your device has a devicemodel, but PV drivers have been detected,
>> use PV.
>>
>> At the moment we don't have a way to check for PV drivers, so this just
>> collapses down to "PV for domains without a DM and DM for domains with a
>> DM."
>>
>>>
>>>> + (1, "PV"),
>>>> + (2, "QEMU"),
>>>
>>> Is "QEMU" what we want here, as opposed to, say, "EMU" (similar to NICs)?
>>
>> I had this as "DEVICEMODEL", since what we mean is that we want the
>> device model to provide access (and in theory in the future we may use a
>> different device model). But "EMU" works for me too.
>>
>>> I think we probably don't want to go as fine grained as "XHCI" and "EHCI"
>>> etc, do we? I see we have a version field below, is it intended that there
>>> be some way to select between e.g. UHCI and OHCI (which IIRC are different
>>> USB 1.0 controllers).
>>>
>>> Maybe these questions should all be left aside for when QMEU support is
>>> actually added (AFAICT this field is just a placeholder)? In fact I glanced
>>> at the code and was surprised to find nothing checking for
>>> LIBXL_USBCTRL_TYPE at all, did I miss something?
>>>
>>> I think the two choices are:
>>>
>>> We can decide quickly and easily what the option(s) other than PV should be
>>> here and you include it in the IDL, but you would then need to check
>>> usbctrl->type == PV at various points, not silently treat all options as
>>> PV.
>>>
>>> Or this becomes a long conversation in which case I think your best bet
>>> would be to leave the enum with just the PV (and maybe AUTO) entries and
>>> leave the decision on the name for the emulated option to the series which
>>> implements that.
>>
>> I think the idea was to simply offer 1, 2, and 3 as options, and for the
>> devicemodel version, choose a suitable controller (or set of
>> controllers) for each option; similar to what usbversion= does now.
>
>
> Hi, George,
>
> I'm still confused about the expected look concerning PV/EMU type handling in
> this patch series.
>
> In earlier version, we tried to extract common things in libxl_usb.c and put
> pvusb specific thing in libxl_pvusb.c, prefixed with pvusb_xxx. As you
> suggested, we can leave that when EMU USB patch series added.
>
> Now, about how to handle PV/EMU type in this patch series, I can think of 3
> ways:
>
> 1. We define the enumeration (contains PV/AUTO only, user interface only allows
> 'pv' or 'not specified', so we handle everything in 'pv' way without further
> check. Leave check and other adjusting things when EMU USB patch series added.
>
> 2. We check domain type and set proper type if not specified (i.e. 'pv' for PV
> guest, 'emu' for HVM guest). In add/remove function, check if type='emu', report
> 'not supported' directly; otherwise, continue do following things. When EMU USB
> patch serires added, need to extract common things and adjust the check place.
>
> 3. Same as 2, but extract common things, only in PV/EMU USB specific part, check
> type, if type='emu', report 'not supported'; otherwise, do pvusb work. When
> adding EMU USB patch series, only need to add EMU USB specific things in the
> type='emu' branch.
>
> Which one is expected? Or none?
So there are two questions here, first WRT the code, the second WRT the
interface.
WRT the code, *normally* the first person to submit the code gets to
have it easy, and the second person has to do all the work of
refactoring. So you would be completely within your rights to simply
submit "libxl_usb.c", and make me refactor that into libxl_pvusb.c and
whatever else (probably the qemu stuff would go in libxl_qmp.c).
Earlier I asked you as a favor to put things in libxl_pvusb.c, and you
were kind enough to do so -- so thank you. Just having things roughly
where they might end up eventually has already been a big help. I'll
have to move some of the code around pretty much no matter what you do.
So I don't think it's worth making any more effort wrt the code itself.
WRT the interface -- if we do a release with PV defined, but not
EMU/DEVICEMODEL, then when we add that option, we'll have to add Yet
Another LIBL_HAS_BLAH. I would personally like too avoid that.
As it happens, if you were to check this in now at the beginning of the
cycle, it's very likely I could get the EMU side in before the release.
So it's *probably* OK to just write AUTO and PV.
I would personally prefer to play it safe and give all three interface
elements (AUTO, PV, and EMU/DEVICEMODEL), and return ENOTSUPP (or
whatever) for DEVICEMODEL until it's implemented. But that's really a
policy decision for the maintianers at this point.
-George
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH V6 3/7] libxl: add pvusb API
2015-09-17 9:54 ` George Dunlap
@ 2015-09-29 17:19 ` Wei Liu
0 siblings, 0 replies; 41+ messages in thread
From: Wei Liu @ 2015-09-29 17:19 UTC (permalink / raw)
To: George Dunlap
Cc: Juergen Gross, wei.liu2, Ian Campbell, george.dunlap, Ian.Jackson,
Chun Yan Liu, xen-devel, Jim Fehlig, Simon Cao
On Thu, Sep 17, 2015 at 10:54:23AM +0100, George Dunlap wrote:
[...]
> > Hi, George,
> >
> > I'm still confused about the expected look concerning PV/EMU type handling in
> > this patch series.
> >
> > In earlier version, we tried to extract common things in libxl_usb.c and put
> > pvusb specific thing in libxl_pvusb.c, prefixed with pvusb_xxx. As you
> > suggested, we can leave that when EMU USB patch series added.
> >
> > Now, about how to handle PV/EMU type in this patch series, I can think of 3
> > ways:
> >
> > 1. We define the enumeration (contains PV/AUTO only, user interface only allows
> > 'pv' or 'not specified', so we handle everything in 'pv' way without further
> > check. Leave check and other adjusting things when EMU USB patch series added.
> >
> > 2. We check domain type and set proper type if not specified (i.e. 'pv' for PV
> > guest, 'emu' for HVM guest). In add/remove function, check if type='emu', report
> > 'not supported' directly; otherwise, continue do following things. When EMU USB
> > patch serires added, need to extract common things and adjust the check place.
> >
> > 3. Same as 2, but extract common things, only in PV/EMU USB specific part, check
> > type, if type='emu', report 'not supported'; otherwise, do pvusb work. When
> > adding EMU USB patch series, only need to add EMU USB specific things in the
> > type='emu' branch.
> >
> > Which one is expected? Or none?
>
> So there are two questions here, first WRT the code, the second WRT the
> interface.
>
> WRT the code, *normally* the first person to submit the code gets to
> have it easy, and the second person has to do all the work of
> refactoring. So you would be completely within your rights to simply
> submit "libxl_usb.c", and make me refactor that into libxl_pvusb.c and
> whatever else (probably the qemu stuff would go in libxl_qmp.c).
>
> Earlier I asked you as a favor to put things in libxl_pvusb.c, and you
> were kind enough to do so -- so thank you. Just having things roughly
> where they might end up eventually has already been a big help. I'll
> have to move some of the code around pretty much no matter what you do.
> So I don't think it's worth making any more effort wrt the code itself.
>
> WRT the interface -- if we do a release with PV defined, but not
> EMU/DEVICEMODEL, then when we add that option, we'll have to add Yet
> Another LIBL_HAS_BLAH. I would personally like too avoid that.
>
(Note that I didn't go through all emails)
I think adding yet another LIBXL_HAS_BLAH wouldn't be a problem. Chunyan
will have to add one anyway.
> As it happens, if you were to check this in now at the beginning of the
> cycle, it's very likely I could get the EMU side in before the release.
> So it's *probably* OK to just write AUTO and PV.
>
Again, extending the interface shouldn't be a problem -- we do that all
the time. So IMHO having AUTO and PV only is OK.
> I would personally prefer to play it safe and give all three interface
> elements (AUTO, PV, and EMU/DEVICEMODEL), and return ENOTSUPP (or
> whatever) for DEVICEMODEL until it's implemented. But that's really a
> policy decision for the maintianers at this point.
>
This works for me too. I don't really see a problem in choosing one way
or another TBH.
Wei.
> -George
^ permalink raw reply [flat|nested] 41+ messages in thread
end of thread, other threads:[~2015-09-29 17:19 UTC | newest]
Thread overview: 41+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-08-10 10:35 [PATCH V6 0/7] xen pvusb toolstack work Chunyan Liu
2015-08-10 10:35 ` [PATCH V6 1/7] libxl: export some functions for pvusb use Chunyan Liu
2015-08-11 11:26 ` Wei Liu
2015-08-10 10:35 ` [PATCH V6 2/7] libxl_read_file_contents: add new entry to read sysfs file Chunyan Liu
2015-08-11 11:26 ` Wei Liu
2015-08-12 2:37 ` Chun Yan Liu
2015-08-13 9:11 ` Wei Liu
2015-08-10 10:35 ` [PATCH V6 3/7] libxl: add pvusb API Chunyan Liu
2015-08-11 11:27 ` Wei Liu
2015-08-12 2:24 ` Chun Yan Liu
2015-08-13 9:09 ` Wei Liu
2015-08-14 1:49 ` Chun Yan Liu
2015-08-18 2:31 ` Chun Yan Liu
2015-08-31 6:10 ` Chun Yan Liu
2015-09-08 14:17 ` Ian Campbell
2015-09-08 16:52 ` George Dunlap
2015-09-09 7:38 ` Chun Yan Liu
2015-09-17 8:19 ` Chun Yan Liu
2015-09-17 9:54 ` George Dunlap
2015-09-29 17:19 ` Wei Liu
2015-09-17 8:20 ` Chun Yan Liu
2015-09-11 5:42 ` Chun Yan Liu
2015-09-11 13:26 ` Ian Campbell
2015-09-11 13:55 ` Juergen Gross
2015-09-11 14:09 ` Ian Campbell
2015-09-11 14:18 ` Juergen Gross
2015-09-11 14:41 ` Ian Campbell
2015-09-11 15:42 ` Ian Jackson
2015-09-14 3:48 ` Juergen Gross
2015-09-14 10:36 ` George Dunlap
2015-09-14 10:53 ` Juergen Gross
2015-09-14 11:12 ` Ian Jackson
2015-09-14 11:23 ` Juergen Gross
2015-09-14 14:03 ` George Dunlap
2015-09-17 8:24 ` Chun Yan Liu
2015-09-15 8:14 ` Chun Yan Liu
2015-08-10 10:35 ` [PATCH V6 4/7] libxl: add libxl_device_usb_assignable_list API Chunyan Liu
2015-08-10 10:35 ` [PATCH V6 5/7] xl: add pvusb commands Chunyan Liu
2015-08-10 10:35 ` [PATCH V6 6/7] xl: add usb-assignable-list command Chunyan Liu
2015-08-10 10:35 ` [PATCH V6 7/7] domcreate: support pvusb in configuration file Chunyan Liu
2015-08-11 11:27 ` Wei 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.