Re: [Xen-devel] [PATCH V14 4/6] libxl: add pvusb API

2016-02-29 Thread Chun Yan Liu 


>>> On 2/29/2016 at 06:14 PM, in message <56d419f6.8030...@citrix.com>, George
Dunlap <george.dun...@citrix.com> wrote: 
> On 26/02/16 12:09, Ian Jackson wrote: 
> > George Dunlap writes ("Re: [Xen-devel] [PATCH V14 4/6] libxl: add pvusb  
> API"): 
> >> On Fri, Feb 19, 2016 at 10:39 AM, Chunyan Liu <cy...@suse.com> wrote: 
> >>> +  [...] 
> >> 
> >> So I see below that you're calling this before removing things from 
> >> xenstore, so that if any of these fail, the user can still call "xl 
> >> usb-remove" to retry. 
> >> 
> >> Unfortunately, since you reassign interfaces to the original driver 
> >> before all interfaces are de-assigned from usbback, you can end up in 
> >> a situation where the device is partially re-plugged into the original 
> >> drivers, partially still assigned to pciback. 
> >> 
> >> I think this whole process should be three steps: 
> >> 
> >> 1. Unassign all interfaces from usbback, stopping and returning an 
> >> error as soon as one attempt fails 
> >> 
> >> 2. Remove the pvusb xenstore nodes (stopping and returning an error if it  
> fails) 
> >> 
> >> 3. Attempt to re-assign all interfaces to the original drivers, 
> >> stopping and returning an error as soon as one attempt fails. 

I'll update codes to this order and post.

-Chunyan

> >  
> > This seems like a good plan to me. 
> >  
> > (Making 3 after 2 re-attemptable would mean that the original driver 
> > information needs to be saved in a different location in xenstore to 
> > the pvusb control nodes, but that is not a problem.) 
> >  
> >> * If one of the re-assignments fail, then the user will have to reload 
> >> the drivers, reboot, or mess around with sysfs to fix things. 
> >> *However*, this avoids a scenario where a user is completely unable to 
> >> remove a device from a domain because of a buggy driver. 
> >  
> > Right. 
> >  
> >> I realize this falls short of the "crash-only" design IanJ suggested 
> >> we try to do, but I think that in this case the work required to do 
> >> such a design would be a lot more work than the benefit it gives us. 
> >  
> > I think what you have above is indeed crash-only.  You can tell by all 
> > the "if any error occurs, stop immediately". 
> >  
> > The only wrinkle is that the obvious implementation reads the old 
> > bindings from xenstore between 1 and 2, deletes the information from 
> > xenstore in 2, and uses that information in step 3, which is cheating 
> > (and leads to the sysfs-wrangling-required state).  But that is quite 
> > easy to avoid: 
> >   - record the old driver bindings somewhere in xenstore (keyed by 
> > the physical host device, not the guest domain) 
> >   - provide a libxl call and corresponding xl command which attempts 
> > to rebind 
>  
> The re-bind information is already stored in a different location, keyed 
> by physical host device. :-) (Search for uses of USBBACK_INFO_PATH.) 
>  
> But we would, as you say, need to add a separate function/command for 
> doing clean-up (simply calling xl usb-detach on the virtual device again 
> doesn't make much sense, since the virtual devices no longer shows up in 
> xl usb-list).  Since we don't have another such command to copy, we'd be 
> inventing a new one, which means thinking very carefully about the 
> design of the interface (since we'd want future such functions to follow 
> this precedent if possible),  , so... 
>  
> > But, FAOD, I do not want to block this patch until such a thing is 
> > implemented.  I think it is sufficient to document the existence of 
> > the awkward state, and the likely workarounds. 
>  
> Great, thanks. :-) 
>  
>  -George 
>  
>  
>  
>  


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

Re: [Xen-devel] [PATCH V14 4/6] libxl: add pvusb API

2016-02-29 Thread George Dunlap 
On 26/02/16 12:09, Ian Jackson wrote:
> George Dunlap writes ("Re: [Xen-devel] [PATCH V14 4/6] libxl: add pvusb API"):
>> On Fri, Feb 19, 2016 at 10:39 AM, Chunyan Liu <cy...@suse.com> wrote:
>>> +  [...]
>>
>> So I see below that you're calling this before removing things from
>> xenstore, so that if any of these fail, the user can still call "xl
>> usb-remove" to retry.
>>
>> Unfortunately, since you reassign interfaces to the original driver
>> before all interfaces are de-assigned from usbback, you can end up in
>> a situation where the device is partially re-plugged into the original
>> drivers, partially still assigned to pciback.
>>
>> I think this whole process should be three steps:
>>
>> 1. Unassign all interfaces from usbback, stopping and returning an
>> error as soon as one attempt fails
>>
>> 2. Remove the pvusb xenstore nodes (stopping and returning an error if it 
>> fails)
>>
>> 3. Attempt to re-assign all interfaces to the original drivers,
>> stopping and returning an error as soon as one attempt fails.
> 
> This seems like a good plan to me.
> 
> (Making 3 after 2 re-attemptable would mean that the original driver
> information needs to be saved in a different location in xenstore to
> the pvusb control nodes, but that is not a problem.)
> 
>> * If one of the re-assignments fail, then the user will have to reload
>> the drivers, reboot, or mess around with sysfs to fix things.
>> *However*, this avoids a scenario where a user is completely unable to
>> remove a device from a domain because of a buggy driver.
> 
> Right.
> 
>> I realize this falls short of the "crash-only" design IanJ suggested
>> we try to do, but I think that in this case the work required to do
>> such a design would be a lot more work than the benefit it gives us.
> 
> I think what you have above is indeed crash-only.  You can tell by all
> the "if any error occurs, stop immediately".
> 
> The only wrinkle is that the obvious implementation reads the old
> bindings from xenstore between 1 and 2, deletes the information from
> xenstore in 2, and uses that information in step 3, which is cheating
> (and leads to the sysfs-wrangling-required state).  But that is quite
> easy to avoid:
>   - record the old driver bindings somewhere in xenstore (keyed by
> the physical host device, not the guest domain)
>   - provide a libxl call and corresponding xl command which attempts
> to rebind

The re-bind information is already stored in a different location, keyed
by physical host device. :-) (Search for uses of USBBACK_INFO_PATH.)

But we would, as you say, need to add a separate function/command for
doing clean-up (simply calling xl usb-detach on the virtual device again
doesn't make much sense, since the virtual devices no longer shows up in
xl usb-list).  Since we don't have another such command to copy, we'd be
inventing a new one, which means thinking very carefully about the
design of the interface (since we'd want future such functions to follow
this precedent if possible),  , so...

> But, FAOD, I do not want to block this patch until such a thing is
> implemented.  I think it is sufficient to document the existence of
> the awkward state, and the likely workarounds.

Great, thanks. :-)

 -George



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

Re: [Xen-devel] [PATCH V14 4/6] libxl: add pvusb API

2016-02-26 Thread Ian Jackson 
George Dunlap writes ("Re: [Xen-devel] [PATCH V14 4/6] libxl: add pvusb API"):
> On Fri, Feb 19, 2016 at 10:39 AM, Chunyan Liu <cy...@suse.com> wrote:
> > +  [...]
> 
> So I see below that you're calling this before removing things from
> xenstore, so that if any of these fail, the user can still call "xl
> usb-remove" to retry.
> 
> Unfortunately, since you reassign interfaces to the original driver
> before all interfaces are de-assigned from usbback, you can end up in
> a situation where the device is partially re-plugged into the original
> drivers, partially still assigned to pciback.
> 
> I think this whole process should be three steps:
> 
> 1. Unassign all interfaces from usbback, stopping and returning an
> error as soon as one attempt fails
> 
> 2. Remove the pvusb xenstore nodes (stopping and returning an error if it 
> fails)
> 
> 3. Attempt to re-assign all interfaces to the original drivers,
> stopping and returning an error as soon as one attempt fails.

This seems like a good plan to me.

(Making 3 after 2 re-attemptable would mean that the original driver
information needs to be saved in a different location in xenstore to
the pvusb control nodes, but that is not a problem.)

> * If one of the re-assignments fail, then the user will have to reload
> the drivers, reboot, or mess around with sysfs to fix things.
> *However*, this avoids a scenario where a user is completely unable to
> remove a device from a domain because of a buggy driver.

Right.

> I realize this falls short of the "crash-only" design IanJ suggested
> we try to do, but I think that in this case the work required to do
> such a design would be a lot more work than the benefit it gives us.

I think what you have above is indeed crash-only.  You can tell by all
the "if any error occurs, stop immediately".

The only wrinkle is that the obvious implementation reads the old
bindings from xenstore between 1 and 2, deletes the information from
xenstore in 2, and uses that information in step 3, which is cheating
(and leads to the sysfs-wrangling-required state).  But that is quite
easy to avoid:
  - record the old driver bindings somewhere in xenstore (keyed by
the physical host device, not the guest domain)
  - provide a libxl call and corresponding xl command which attempts
to rebind

But, FAOD, I do not want to block this patch until such a thing is
implemented.  I think it is sufficient to document the existence of
the awkward state, and the likely workarounds.

Ian.

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

Re: [Xen-devel] [PATCH V14 4/6] libxl: add pvusb API

2016-02-25 Thread Chun Yan Liu 


>>> On 2/24/2016 at 01:10 AM, in message
, George
Dunlap  wrote: 
> On Fri, Feb 19, 2016 at 10:39 AM, 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: Simon Cao  
> > Signed-off-by: George Dunlap  
> > Signed-off-by: Chunyan Liu  
> > --- 
> > changes: 
> >   Address Olaf's comments: 
> >   * move DEFINE_DEVICE_REMOVE changes to a separate patch 
> >   Address Ian's comments: 
> >   * adjust order of removing xenstore and bind/unbind driver in usb_remove. 
> >   * reuse libxl_write_exactly in usbintf_bind/unbind 
> >   * several coding style fix 
>  
> [snip] 
>  
> > +/* Unbind USB device from "usbback" driver. 
> > + * 
> > + * If there are many interfaces under USB device, check each interface, 
> > + * unbind from "usbback" driver and rebind to its original driver. 
> > + */ 
> > +static int usbback_dev_unassign(libxl__gc *gc, const char *busid) 
> > +{ 
> > +char **intfs = NULL; 
> > +char *usbdev_encode = NULL; 
> > +char *path = NULL; 
> > +int i, num = 0; 
> > +int rc; 
> > + 
> > +rc = usbdev_get_all_interfaces(gc, busid, , ); 
> > +if (rc) goto out; 
> > + 
> > +usbdev_encode = usb_interface_xenstore_encode(gc, busid); 
> > + 
> > +for (i = 0; i < num; i++) { 
> > +char *intf = intfs[i]; 
> > +char *usbintf_encode = NULL; 
> > +const char *drvpath; 
> > + 
> > +/* check if the USB interface is already bound to "usbback" */ 
> > +if (usbintf_is_assigned(gc, intf) > 0) { 
> > +/* unbind interface from usbback driver */ 
> > +rc = unbind_usbintf(gc, intf); 
> > +if (rc) { 
> > +LOGE(ERROR, "Couldn't unbind %s from usbback", intf); 
> > +goto out; 
> > +} 
> > +} 
> > + 
> > +/* rebind USB interface to its originial driver */ 
> > +usbintf_encode = usb_interface_xenstore_encode(gc, intf); 
> > +path = GCSPRINTF(USBBACK_INFO_PATH "/%s/%s/driver_path", 
> > + usbdev_encode, usbintf_encode); 
> > +rc = libxl__xs_read_checked(gc, XBT_NULL, path, ); 
> > +if (rc) goto out; 
> > + 
> > +if (drvpath) { 
> > +rc = bind_usbintf(gc, intf, drvpath); 
> > +if (rc) { 
> > +LOGE(ERROR, "Couldn't rebind %s to %s", intf, drvpath); 
> > +goto out; 
> > +} 
> > +} 
> > +} 
>  
> So I see below that you're calling this before removing things from 
> xenstore, so that if any of these fail, the user can still call "xl 
> usb-remove" to retry. 
>  
> Unfortunately, since you reassign interfaces to the original driver 
> before all interfaces are de-assigned from usbback, you can end up in 
> a situation where the device is partially re-plugged into the original 
> drivers, partially still assigned to pciback. 
>  
> I think this whole process should be three steps: 
>  
> 1. Unassign all interfaces from usbback, stopping and returning an 
> error as soon as one attempt fails 
>  
> 2. Remove the pvusb xenstore nodes (stopping and returning an error if it  
> fails) 
>  
> 3. Attempt to re-assign all interfaces to the original drivers, 
> stopping and returning an error as soon as one attempt fails. 
>  
> And in the case of #3, the log message should give a suggestion as to 
> what might help; for instance, "Couldn't rebind USB device to driver 
> [driver name].  Reloading the module may help."  (I think you should 
> be able to get the driver name from the path, right?) 

Yes, that's right.

>  
> A couple of properties this gives us: 
>  
> * If the un-assign partially succeeds, the user can re-try the unassign 
>  
> * If one of the re-assignments fail, then the user will have to reload 
> the drivers, reboot, or mess around with sysfs to fix things. 
> *However*, this avoids a scenario where a user is completely unable to 
> remove a device from a domain because of a buggy driver. 

To guest or host, this situation that some interface is bound to pciback
but some is to original driver, is indeed a mess. But to toolstack, it can
still have chance to redo 'xl usbdev-remove' to retry the left work (unassign
interfaces those bound to pciback, and reassign to original interface).

>  
> What do you think? 

The 3 steps above are indeed more reasonable. Codes need to be
reorganized (previously doing unassingn_form_pciback and rebind to
original driver in a same cycle for each interface, now will be split into
two separate processes: walk through each interface to do
unassign_from_pciback, and walk through each interface to rebind to
original

[Xen-devel] [PATCH V14 4/6] libxl: add pvusb API

2016-02-19 Thread Chunyan Liu 
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: Simon Cao 
Signed-off-by: George Dunlap 
Signed-off-by: Chunyan Liu 
---
changes:
  Address Olaf's comments:
  * move DEFINE_DEVICE_REMOVE changes to a separate patch
  Address Ian's comments:
  * adjust order of removing xenstore and bind/unbind driver in usb_remove.
  * reuse libxl_write_exactly in usbintf_bind/unbind
  * several coding style fix

 tools/libxl/Makefile |2 +-
 tools/libxl/libxl.c  |   18 +
 tools/libxl/libxl.h  |   77 ++
 tools/libxl/libxl_device.c   |5 +-
 tools/libxl/libxl_internal.h |   18 +
 tools/libxl/libxl_osdeps.h   |   13 +
 tools/libxl/libxl_pvusb.c| 1567 ++
 tools/libxl/libxl_types.idl  |   46 +
 tools/libxl/libxl_types_internal.idl |1 +
 tools/libxl/libxl_utils.c|   18 +
 tools/libxl/libxl_utils.h|5 +
 11 files changed, 1768 insertions(+), 2 deletions(-)
 create mode 100644 tools/libxl/libxl_pvusb.c

diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
index 620720e..459fc51 100644
--- a/tools/libxl/Makefile
+++ b/tools/libxl/Makefile
@@ -105,7 +105,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 f385134..aa879d2 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -4167,6 +4167,8 @@ out:
  * libxl_device_vkb_destroy
  * libxl_device_vfb_remove
  * libxl_device_vfb_destroy
+ * libxl_device_usbctrl_remove
+ * libxl_device_usbctrl_destroy
  */
 #define DEFINE_DEVICE_REMOVE_EXT(type, remtype, removedestroy, f)\
 int libxl_device_##type##_##removedestroy(libxl_ctx *ctx,   \
@@ -4224,6 +4226,10 @@ DEFINE_DEVICE_REMOVE(vfb, destroy, 1)
 DEFINE_DEVICE_REMOVE(vtpm, remove, 0)
 DEFINE_DEVICE_REMOVE(vtpm, destroy, 1)
 
+/* usbctrl */
+DEFINE_DEVICE_REMOVE_CUSTOM(usbctrl, remove, 0)
+DEFINE_DEVICE_REMOVE_CUSTOM(usbctrl, destroy, 1)
+
 /* channel/console hotunplug is not implemented. There are 2 possibilities:
  * 1. add support for secondary consoles to xenconsoled
  * 2. dynamically add/remove qemu chardevs via qmp messages. */
@@ -4239,6 +4245,8 @@ DEFINE_DEVICE_REMOVE(vtpm, destroy, 1)
  * libxl_device_disk_add
  * libxl_device_nic_add
  * libxl_device_vtpm_add
+ * libxl_device_usbctrl_add
+ * libxl_device_usbdev_add
  */
 
 #define DEFINE_DEVICE_ADD(type) \
@@ -4270,6 +4278,12 @@ DEFINE_DEVICE_ADD(nic)
 /* vtpm */
 DEFINE_DEVICE_ADD(vtpm)
 
+/* usbctrl */
+DEFINE_DEVICE_ADD(usbctrl)
+
+/* usb */
+DEFINE_DEVICE_ADD(usbdev)
+
 #undef DEFINE_DEVICE_ADD
 
 
/**/
@@ -6815,6 +6829,10 @@ int libxl_retrieve_domain_configuration(libxl_ctx *ctx, 
uint32_t domid,
 
 MERGE(pci, pcidevs, COMPARE_PCI, {});
 
+MERGE(usbctrl, usbctrls, COMPARE_USBCTRL, {});
+
+MERGE(usbdev, usbdevs, COMPARE_USB, {});
+
 /* Take care of removable device. We maintain invariant in the
  * insert / remove operation so that:
  * 1. if xenstore is "empty" while JSON is not, the result
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index ea7a4b8..e72d001 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -123,6 +123,12 @@
 #define LIBXL_HAVE_DOMAIN_NODEAFFINITY 1
 
 /*
+ * LIBXL_HAVE_PVUSB indicates functions for plugging in USB devices
+ * through pvusb -- both hotplug and at domain creation time..
+ */
+#define LIBXL_HAVE_PVUSB 1
+
+/*
  * LIBXL_HAVE_BUILDINFO_HVM_VENDOR_DEVICE indicates that the
  * libxl_vendor_device field is present in the hvm sections of
  * libxl_domain_build_info. This field tells libxl which
@@ -1517,6 +1523,77 @@ int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, 
libxl_device_disk *disk,
const libxl_asyncop_how *ao_how)
LIBXL_EXTERNAL_CALLERS_ONLY;
 
+/*
+ * USB
+ *
+ * For each device removed or added, one of these protocols is available:
+ * - PV (i.e., PVUSB)
+ * - DEVICEMODEL (i.e, qemu)
+ *
+ * PV is available for either PV or HVM domains.  DEVICEMODEL is only
+ * available for HVM domains.  The caller can additionally specify
+ * "AUTO", in which case the library will try to determine the best
+ * protocol