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

2016-03-03 Thread Chun Yan Liu 


>>> On 3/3/2016 at 05:27 PM, in message
, George
Dunlap  wrote: 
> On Thu, Mar 3, 2016 at 2:59 AM, Chun Yan Liu  wrote: 
> On 3/3/2016 at 02:46 AM, in message <56d7350f.7010...@citrix.com>, George 
> > Dunlap  wrote: 
> >> Sorry, just looked through the rest of the series, and there's one more 
> >> thing. 
> >> 
> >> Neither here nor in the man page do we explain what to do if something 
> >> goes wrong with the detach.  I think the best thing to do is probably to 
> >> make the logged error messages more helpful. 
> >> 
> >> What about something like this: 
> >> 
> >> * On failure to unbind: "Error removing device from guest.  Try running 
> >> usbdev-detach again." 
> >> 
> >> * On failure to rebind: "USB device removed from guest, but couldn't 
> >> re-bind to domain 0.  Try removing and re-inserting the USB device or 
> >> reloading the driver modules." 
> > Here, user could first try 'echo xxx > sysfs_driver_path/bind", so maybe: 
> > "Try binding USB device to original driver by echoing the device to 
> > [driver_path]/bind, or removing and re-inserting the USB device, or 
> > reloading the driver modules." 
>  
> Yes, I had thought about the idea of giving the user specific commands 
> to retry.  The question is how much we can expect the user to do to 
> recover the state.  At some point "reloading the driver module" was 
> seen as something reasonable for a reasonably advanced user, while 
> "messing around with sysfs" was seen to be something too technical. 
> But as you say, giving them the exact command to cut-and-paste might 
> be somewhat more reasonable. 
>  
> I'm still inclined to say that we should just stick with module 
> reloading and removing and re-inserting the device, but I wouldn't 
> insist on it. 

I see. Just use what you suggested. Will update and repost.

Thanks,
Chunyan

>  
> If we do print this kind of help message, then we should make sure to 
> print a more complete message that includes cut-and-paste commands for 
> *all* remaining unbound interfaces. 
>  
>  -George 
>  
> ___ 
> Xen-devel mailing list 
> Xen-devel@lists.xen.org 
> http://lists.xen.org/xen-devel 
>  



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

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

2016-03-03 Thread Chun Yan Liu 


>>> On 3/3/2016 at 05:20 PM, in message
, George
Dunlap  wrote: 
> On Thu, Mar 3, 2016 at 3:11 AM, Chun Yan Liu  wrote: 
> On 3/3/2016 at 02:32 AM, in message <56d731b1.60...@citrix.com>, George 
> Dunlap 
> >  wrote: 
> >> On 01/03/16 08:09, Chunyan Liu wrote: 
> >> > +static int usbdev_rebind(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; 
> >> > + 
> >> > +/* 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; 
> >> > +} 
> >> > +} 
> >> > +} 
> >> > + 
> >> > +path = GCSPRINTF(USBBACK_INFO_PATH "/%s", usbdev_encode); 
> >> > +rc = libxl__xs_rm_checked(gc, XBT_NULL, path); 
> >> > + 
> >> > +out: 
> >> 
> >> So it looks like if one of the re-binds fails, then it stops where it is 
> >> and leaves the USBBACK re-bind info in xenstore.  In that case it's not 
> >> clear to me how that information would ever be removed. 
> >> 
> >> I think until such time as we have a command to re-attempt the re-bind, 
> >>  if there's an error in the actual rebind, it should just break out of 
> >> the for loop, and remove the re-bind nodes, and document a way to let 
> >> the user try to clean things up. 
> > 
> > Just according to last time discussion about how to handle the rebind 
> > failure, seems Ian preferred to add a xl command to deal with rebind 
> > in future, based on that need, I think driver_path info should be kept 
> > in xenstore then. Without that need, I agree it's best to remove 
> > xenstore nodes. So, keep or remove? 
>  
> Well when we have such a command, then yes, we'll need to keep the 
> nodes for it to use and delete.  But at the moment, we have no such 
> command, so these nodes will just sit around cluttering up the libxl 
> xenstore space, and nothing will delete them (unless a user manually 
> runs xenstore-rm). 
>  
> So I think for now we should delete them on failure; and at some point 
> later, when someone implements a recovery command, then they should 
> change this code to not delete the xenstore nodes (and also change the 
> log messages to point to that command). 

OK. Got it. Will update.

-  Chunyan

>  
>  -George 
>  
>  


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

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

2016-03-03 Thread George Dunlap 
On Thu, Mar 3, 2016 at 2:59 AM, Chun Yan Liu  wrote:
 On 3/3/2016 at 02:46 AM, in message <56d7350f.7010...@citrix.com>, George
> Dunlap  wrote:
>> Sorry, just looked through the rest of the series, and there's one more
>> thing.
>>
>> Neither here nor in the man page do we explain what to do if something
>> goes wrong with the detach.  I think the best thing to do is probably to
>> make the logged error messages more helpful.
>>
>> What about something like this:
>>
>> * On failure to unbind: "Error removing device from guest.  Try running
>> usbdev-detach again."
>>
>> * On failure to rebind: "USB device removed from guest, but couldn't
>> re-bind to domain 0.  Try removing and re-inserting the USB device or
>> reloading the driver modules."
> Here, user could first try 'echo xxx > sysfs_driver_path/bind", so maybe:
> "Try binding USB device to original driver by echoing the device to
> [driver_path]/bind, or removing and re-inserting the USB device, or
> reloading the driver modules."

Yes, I had thought about the idea of giving the user specific commands
to retry.  The question is how much we can expect the user to do to
recover the state.  At some point "reloading the driver module" was
seen as something reasonable for a reasonably advanced user, while
"messing around with sysfs" was seen to be something too technical.
But as you say, giving them the exact command to cut-and-paste might
be somewhat more reasonable.

I'm still inclined to say that we should just stick with module
reloading and removing and re-inserting the device, but I wouldn't
insist on it.

If we do print this kind of help message, then we should make sure to
print a more complete message that includes cut-and-paste commands for
*all* remaining unbound interfaces.

 -George

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

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

2016-03-03 Thread George Dunlap 
On Thu, Mar 3, 2016 at 3:11 AM, Chun Yan Liu  wrote:
 On 3/3/2016 at 02:32 AM, in message <56d731b1.60...@citrix.com>, George 
 Dunlap
>  wrote:
>> On 01/03/16 08:09, Chunyan Liu wrote:
>> > +static int usbdev_rebind(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;
>> > +
>> > +/* 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;
>> > +}
>> > +}
>> > +}
>> > +
>> > +path = GCSPRINTF(USBBACK_INFO_PATH "/%s", usbdev_encode);
>> > +rc = libxl__xs_rm_checked(gc, XBT_NULL, path);
>> > +
>> > +out:
>>
>> So it looks like if one of the re-binds fails, then it stops where it is
>> and leaves the USBBACK re-bind info in xenstore.  In that case it's not
>> clear to me how that information would ever be removed.
>>
>> I think until such time as we have a command to re-attempt the re-bind,
>>  if there's an error in the actual rebind, it should just break out of
>> the for loop, and remove the re-bind nodes, and document a way to let
>> the user try to clean things up.
>
> Just according to last time discussion about how to handle the rebind
> failure, seems Ian preferred to add a xl command to deal with rebind
> in future, based on that need, I think driver_path info should be kept
> in xenstore then. Without that need, I agree it's best to remove
> xenstore nodes. So, keep or remove?

Well when we have such a command, then yes, we'll need to keep the
nodes for it to use and delete.  But at the moment, we have no such
command, so these nodes will just sit around cluttering up the libxl
xenstore space, and nothing will delete them (unless a user manually
runs xenstore-rm).

So I think for now we should delete them on failure; and at some point
later, when someone implements a recovery command, then they should
change this code to not delete the xenstore nodes (and also change the
log messages to point to that command).

 -George

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

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

2016-03-02 Thread Chun Yan Liu 


>>> On 3/3/2016 at 02:32 AM, in message <56d731b1.60...@citrix.com>, George 
>>> Dunlap
 wrote: 
> On 01/03/16 08:09, 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: 
> >   reorder usbdev_remove to following 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. 
>  
> Thanks, Chunyan!  One minor comment about these changes... 
>  
> > +static int usbdev_rebind(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; 
> > + 
> > +/* 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; 
> > +} 
> > +} 
> > +} 
> > + 
> > +path = GCSPRINTF(USBBACK_INFO_PATH "/%s", usbdev_encode); 
> > +rc = libxl__xs_rm_checked(gc, XBT_NULL, path); 
> > + 
> > +out: 
>  
> So it looks like if one of the re-binds fails, then it stops where it is 
> and leaves the USBBACK re-bind info in xenstore.  In that case it's not 
> clear to me how that information would ever be removed. 
>  
> I think until such time as we have a command to re-attempt the re-bind, 
>  if there's an error in the actual rebind, it should just break out of 
> the for loop, and remove the re-bind nodes, and document a way to let 
> the user try to clean things up. 

Just according to last time discussion about how to handle the rebind
failure, seems Ian preferred to add a xl command to deal with rebind
in future, based on that need, I think driver_path info should be kept
in xenstore then. Without that need, I agree it's best to remove
xenstore nodes. So, keep or remove?

[Post last time Ian's idea]
[start]
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.
[end]

>  
> > +static int do_usbdev_remove(libxl__gc *gc, uint32_t domid, 
> > +libxl_device_usbdev *usbdev) 
> > +{ 
> > +int rc; 
> > +char *busid; 
> > +libxl_device_usbctrl usbctrl; 
> > +libxl_usbctrlinfo usbctrlinfo; 
> > + 
> > +libxl_device_usbctrl_init(); 
> > +libxl_usbctrlinfo_init(); 
> > +usbctrl.devid = usbdev->ctrl; 
> > + 
> > +rc = libxl_device_usbctrl_getinfo(CTX, domid, , ); 
> > +if (rc) goto out; 
> > + 
> > +switch (usbctrlinfo.type) { 
> > +case LIBXL_USBCTRL_TYPE_PV: 
> > +busid = usbdev_busid_from_ctrlport(gc, domid, usbdev); 
> > +if (!busid) { 
> > +rc = ERROR_FAIL; 
> > +goto out; 
> > +} 
> > + 
> > +rc = usbback_dev_unassign(gc, busid); 
> > +if (rc) goto out; 
> > + 
> > +rc = libxl__device_usbdev_remove_xenstore(gc, domid, usbdev); 
> > +if (rc) goto out; 
> > + 
> > +rc = usbdev_rebind(gc, busid); 
> > +if (rc) goto out; 
>  
> I think we need a comment here saying why we're doing things

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

2016-03-02 Thread Chun Yan Liu 


>>> On 3/3/2016 at 02:46 AM, in message <56d7350f.7010...@citrix.com>, George
Dunlap  wrote: 
> On 02/03/16 18:32, George Dunlap wrote: 
> > On 01/03/16 08:09, 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: 
> >>   reorder usbdev_remove to following 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. 
> >  
> > Thanks, Chunyan!  One minor comment about these changes... 
> >  
> >> +static int usbdev_rebind(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; 
> >> + 
> >> +/* 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; 
> >> +} 
> >> +} 
> >> +} 
> >> + 
> >> +path = GCSPRINTF(USBBACK_INFO_PATH "/%s", usbdev_encode); 
> >> +rc = libxl__xs_rm_checked(gc, XBT_NULL, path); 
> >> + 
> >> +out: 
> >  
> > So it looks like if one of the re-binds fails, then it stops where it is 
> > and leaves the USBBACK re-bind info in xenstore.  In that case it's not 
> > clear to me how that information would ever be removed. 
> >  
> > I think until such time as we have a command to re-attempt the re-bind, 
> >  if there's an error in the actual rebind, it should just break out of 
> > the for loop, and remove the re-bind nodes, and document a way to let 
> > the user try to clean things up. 
> >  
> >> +static int do_usbdev_remove(libxl__gc *gc, uint32_t domid, 
> >> +libxl_device_usbdev *usbdev) 
> >> +{ 
> >> +int rc; 
> >> +char *busid; 
> >> +libxl_device_usbctrl usbctrl; 
> >> +libxl_usbctrlinfo usbctrlinfo; 
> >> + 
> >> +libxl_device_usbctrl_init(); 
> >> +libxl_usbctrlinfo_init(); 
> >> +usbctrl.devid = usbdev->ctrl; 
> >> + 
> >> +rc = libxl_device_usbctrl_getinfo(CTX, domid, , 
> >> ); 
> >> +if (rc) goto out; 
> >> + 
> >> +switch (usbctrlinfo.type) { 
> >> +case LIBXL_USBCTRL_TYPE_PV: 
> >> +busid = usbdev_busid_from_ctrlport(gc, domid, usbdev); 
> >> +if (!busid) { 
> >> +rc = ERROR_FAIL; 
> >> +goto out; 
> >> +} 
> >> + 
> >> +rc = usbback_dev_unassign(gc, busid); 
> >> +if (rc) goto out; 
> >> + 
> >> +rc = libxl__device_usbdev_remove_xenstore(gc, domid, usbdev); 
> >> +if (rc) goto out; 
> >> + 
> >> +rc = usbdev_rebind(gc, busid); 
> >> +if (rc) goto out; 
> >  
> > I think we need a comment here saying why we're doing things in this 
> > order.  Maybe: 
> >  
> > "Things are done in this order to balance simplicity with robustness in 
> > the case of failure: 
> > * We unbind all interfaces before rebinding any interfaces, so that we 
> > never get into a situation where some interfaces are assigned to usbback 
> > and some are assigned to the original drivers. 
> > * We also unbind the interfaces before removing the pvusb xenstore 
> > nodes, so that if the unbind fails in the middle, the device still shows 
> > up in xl usb-list, and the user can re-try removing it." 
>  
> Sorry, just looked through the rest of the series, and there's one more 
> thing. 
>  
> Neither here nor in the man page do we explain what to do if something 
> goes wrong with the detach.  I think the best thing to do is probably to 
> make the logged error messages more helpful. 
>  
> What about

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

2016-03-02 Thread George Dunlap 
On 02/03/16 18:32, George Dunlap wrote:
> On 01/03/16 08:09, 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:
>>   reorder usbdev_remove to following 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.
> 
> Thanks, Chunyan!  One minor comment about these changes...
> 
>> +static int usbdev_rebind(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;
>> +
>> +/* 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;
>> +}
>> +}
>> +}
>> +
>> +path = GCSPRINTF(USBBACK_INFO_PATH "/%s", usbdev_encode);
>> +rc = libxl__xs_rm_checked(gc, XBT_NULL, path);
>> +
>> +out:
> 
> So it looks like if one of the re-binds fails, then it stops where it is
> and leaves the USBBACK re-bind info in xenstore.  In that case it's not
> clear to me how that information would ever be removed.
> 
> I think until such time as we have a command to re-attempt the re-bind,
>  if there's an error in the actual rebind, it should just break out of
> the for loop, and remove the re-bind nodes, and document a way to let
> the user try to clean things up.
> 
>> +static int do_usbdev_remove(libxl__gc *gc, uint32_t domid,
>> +libxl_device_usbdev *usbdev)
>> +{
>> +int rc;
>> +char *busid;
>> +libxl_device_usbctrl usbctrl;
>> +libxl_usbctrlinfo usbctrlinfo;
>> +
>> +libxl_device_usbctrl_init();
>> +libxl_usbctrlinfo_init();
>> +usbctrl.devid = usbdev->ctrl;
>> +
>> +rc = libxl_device_usbctrl_getinfo(CTX, domid, , );
>> +if (rc) goto out;
>> +
>> +switch (usbctrlinfo.type) {
>> +case LIBXL_USBCTRL_TYPE_PV:
>> +busid = usbdev_busid_from_ctrlport(gc, domid, usbdev);
>> +if (!busid) {
>> +rc = ERROR_FAIL;
>> +goto out;
>> +}
>> +
>> +rc = usbback_dev_unassign(gc, busid);
>> +if (rc) goto out;
>> +
>> +rc = libxl__device_usbdev_remove_xenstore(gc, domid, usbdev);
>> +if (rc) goto out;
>> +
>> +rc = usbdev_rebind(gc, busid);
>> +if (rc) goto out;
> 
> I think we need a comment here saying why we're doing things in this
> order.  Maybe:
> 
> "Things are done in this order to balance simplicity with robustness in
> the case of failure:
> * We unbind all interfaces before rebinding any interfaces, so that we
> never get into a situation where some interfaces are assigned to usbback
> and some are assigned to the original drivers.
> * We also unbind the interfaces before removing the pvusb xenstore
> nodes, so that if the unbind fails in the middle, the device still shows
> up in xl usb-list, and the user can re-try removing it."

Sorry, just looked through the rest of the series, and there's one more
thing.

Neither here nor in the man page do we explain what to do if something
goes wrong with the detach.  I think the best thing to do is probably to
make the logged error messages more helpful.

What about something like this:

* On failure to unbind: "Error removing device from guest.  Try running
usbdev-detach again."

* On failure to rebind: "USB device removed from guest, but couldn't
re-bind to domain 0.  Try removing and re-inserting the USB device or
reloading the driver modules."

What do you think?

 -George

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

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

2016-03-02 Thread George Dunlap 
On 01/03/16 08:09, 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:
>   reorder usbdev_remove to following 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.

Thanks, Chunyan!  One minor comment about these changes...

> +static int usbdev_rebind(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;
> +
> +/* 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;
> +}
> +}
> +}
> +
> +path = GCSPRINTF(USBBACK_INFO_PATH "/%s", usbdev_encode);
> +rc = libxl__xs_rm_checked(gc, XBT_NULL, path);
> +
> +out:

So it looks like if one of the re-binds fails, then it stops where it is
and leaves the USBBACK re-bind info in xenstore.  In that case it's not
clear to me how that information would ever be removed.

I think until such time as we have a command to re-attempt the re-bind,
 if there's an error in the actual rebind, it should just break out of
the for loop, and remove the re-bind nodes, and document a way to let
the user try to clean things up.

> +static int do_usbdev_remove(libxl__gc *gc, uint32_t domid,
> +libxl_device_usbdev *usbdev)
> +{
> +int rc;
> +char *busid;
> +libxl_device_usbctrl usbctrl;
> +libxl_usbctrlinfo usbctrlinfo;
> +
> +libxl_device_usbctrl_init();
> +libxl_usbctrlinfo_init();
> +usbctrl.devid = usbdev->ctrl;
> +
> +rc = libxl_device_usbctrl_getinfo(CTX, domid, , );
> +if (rc) goto out;
> +
> +switch (usbctrlinfo.type) {
> +case LIBXL_USBCTRL_TYPE_PV:
> +busid = usbdev_busid_from_ctrlport(gc, domid, usbdev);
> +if (!busid) {
> +rc = ERROR_FAIL;
> +goto out;
> +}
> +
> +rc = usbback_dev_unassign(gc, busid);
> +if (rc) goto out;
> +
> +rc = libxl__device_usbdev_remove_xenstore(gc, domid, usbdev);
> +if (rc) goto out;
> +
> +rc = usbdev_rebind(gc, busid);
> +if (rc) goto out;

I think we need a comment here saying why we're doing things in this
order.  Maybe:

"Things are done in this order to balance simplicity with robustness in
the case of failure:
* We unbind all interfaces before rebinding any interfaces, so that we
never get into a situation where some interfaces are assigned to usbback
and some are assigned to the original drivers.
* We also unbind the interfaces before removing the pvusb xenstore
nodes, so that if the unbind fails in the middle, the device still shows
up in xl usb-list, and the user can re-try removing it."

Other than that, I gave this patche a moderately thorough review again
today, and I think everything else looks good to me.

 -George


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

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

2016-03-01 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:
  reorder usbdev_remove to following 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.

 tools/libxl/Makefile |3 +-
 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| 1596 ++
 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, 1798 insertions(+), 2 deletions(-)
 create mode 100644 tools/libxl/libxl_pvusb.c

diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
index 789a12e..8fa7b87 100644
--- a/tools/libxl/Makefile
+++ b/tools/libxl/Makefile
@@ -105,7 +105,8 @@ 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_dom_save.o $(LIBXL_OBJS-y)
+   libxl_dom_suspend.o libxl_dom_save.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 2ab5ad3..1e68688 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -4102,6 +4102,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,   \
@@ -4159,6 +4161,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. */
@@ -4174,6 +4180,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) \
@@ -4205,6 +4213,12 @@ DEFINE_DEVICE_ADD(nic)
 /* vtpm */
 DEFINE_DEVICE_ADD(vtpm)
 
+/* usbctrl */
+DEFINE_DEVICE_ADD(usbctrl)
+
+/* usb */
+DEFINE_DEVICE_ADD(usbdev)
+
 #undef DEFINE_DEVICE_ADD
 
 
/**/
@@ -6750,6 +6764,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 0859383..5cc3ce3 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
@@ -1536,6 +1542,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.