Re: [Xen-devel] [PATCH V13 3/5] libxl: add pvusb API

[Ian Jackson]

George Dunlap writes ("Re: [Xen-devel] [PATCH V13 3/5] libxl: add pvusb API"):
> There's a difference between "making it intelligent" and "not making it
> broken". :-)  Given that users can potentially cause a number of these
> things to fail just by pressing Ctrl-C, we need to at least make sure
> that we don't get into a completely wedged state that the user can't do
> anything to fix.  That requires some careful thought.

Right.  There is a useful design principle which can help simplify and
clarify: "crash-only software".[1][2]

The idea is that when an error occurs, it is usually best to simply
stop and leave the system in whatever intermediate state.  The next
run of the software is responsible for discovering, interpreting and
if necessary rectifying the situation.

In general these recovery paths are necessary anyway.  So the
on-error-cleanup doesn't help.


In the context of that series, I think that means:

We observe that there is an ordering of possible states
  attached to dom0 driver
  unattached
  attached to usbback
  assigned to a guest

When reconfiguring a device, it will move through these states in
order (forwards or backwards, normally).

NB that I don't discuss here what information may be recorded in
xenstore and the VM configuration at each stage: so in fact there are
various micro-states in between the major states shown above (eg,
"attached to usbback and in process of attaching to geust").


To make a coherent design, we need to:

Arrange that each of these states can be seen by the user somehow.
(Eg in output from list-assignable.)

Arrange that each intermediate state can be recovered to at least one
endpoint state by use of some appropriate command(s).

Arrange that the recording of metadata in xenstore and the domain json
config occurs in the right order to support the above (ie, that the
micro-states are right).  After an approach has been chosen, to show
that the design is correct, it is probably easiest to explicitly
enumerate all the micro-states.


Ideally I would like to see this aspect of the design covered in a doc
comment (or maybe commit messages), in some form or other.


I hope this is of some help.

Thanks,
Ian.


[1] https://www.usenix.org/events/hotos03/tech/full_papers/candea/candea.pdf
[2] https://en.wikipedia.org/wiki/Crash-only_software

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

Re: [Xen-devel] [PATCH V13 3/5] libxl: add pvusb API

[George Dunlap]

On 04/02/16 14:39, Juergen Gross wrote:
> On 04/02/16 02:53, Chun Yan Liu wrote:
>>
>>
>>>>> On 2/3/2016 at 10:33 PM, in message <56b20fcc.3010...@citrix.com>, George
>> Dunlap <george.dun...@citrix.com> wrote: 
>>> On 02/02/16 18:11, Ian Jackson wrote: 
>>>> George Dunlap writes ("Re: [Xen-devel] [PATCH V13 3/5] libxl: add pvusb  
>>> API"): 
>>>>> There are effectively four states a device can be in, from the 
>>>>> 'assignment' point of view: 
>>>>>
>>>>> 1. Assigned to the normal Linux device driver(s) for the USB device 
>>>>>
>>>>> 2. Assigned to no driver 
>>>>>
>>>>> 3. Assigned to usbback, but not yet assigned to any guest 
>>>>>
>>>>> 4. Assigned to a guest 
>>>>  
>>>> Thanks for your clear explanation (of which I will snip much). 
>>>>  
>>>>> Additionally, each USB "device" has one or more "interfaces".  To 
>>>>> assign a "device" to usbback in the sysfs case means assigning all of 
>>>>> the "interfaces".  The code seems to assume that different interfaces 
>>>>> from the same device can be assigned to different drivers. 
>>>>  
>>>> It is indeed the case that in principle a single USB device with 
>>>> multiple interfaces can be assigned to multiple different drivers. 
>>>>  
>>>>> Regarding Ian's comments: 
>>>>>
>>>>> Since "assigned to the guest" and "listed under the pvusb xenstore 
>>>>> node" are the same thing, is it even *possible* to (safely) unassign 
>>>>> the device from usbback before removing the xenstore nodes? 
>>>>  
>>>> It might be possible to remove some of the xenstore nodes but leave 
>>>> others present, so that usbback detaches, but enough information 
>>>> remains for libxl to know that Xen still `owns' the device. 
>>>>  
>>>> But, surely usbback needs to cope with the notion that the device 
>>>> might disappear.  USB devices can disappear at any time. 
>>>  
>>> That's true. But if the USB device has actually disappeared, the device 
>>> is in fact "safe" from usbback.  I think I might consider state 2 safe 
>>> to go to, but I definitely wouldn't consider state 1 safe with the 
>>> xenstore nodes still in place. 
>>>  
>>>>  
>>>>> It's not clear to me under what conditions 3->2 might fail, 
>>>>  
>>>> As an example, someone might press ^C on `xl usb-detach'. 
>>>  
>>> *grumble* 
>>>  
>>>>> or what could be done about it if it did fail. 
>>>>  
>>>> The most obvious reason for it failing is that something somewhere 
>>>> still held onto the device.  (For umounting filesystems, and detaching 
>>>> block devices, this happens a lot.)  So if the detach from usbback 
>>>> fails would probably be possible to simply retry it. 
>>>  
>>> I'm not sure what this means in the context of moving from 3 (assigned 
>>> to usbback) to 2 (unassigned).  usbback will definitely not use the 
>>> device to mount something in dom0; and I'm pretty sure has no visibility 
>>> as to whether it's being used as a filesystem in the domU. 
>>>  
>>> (Moving from 1 -> 2 of course would be subject to this sort of thing, 
>>> but that's a different issue.) 
>>>  
>>>>> Regarding 2->1, again it's not clear that there's anything libxl can 
>>>>> do.  Reloading the driver's module might reset it enough to pick up 
>>>>> the (now unassigned) USB devices again; other than that, rebooting is 
>>>>> probably the best option. 
>>>>  
>>>> I think re-attempting the bind may work.  USB devices can be flaky. 
>>>>  
>>>>> It's also not clear to me, if some functions succeed in being 
>>>>> reassigned and others fail, whether it's best to just try to assign as 
>>>>> many as we can, or better to go back and un-assign all the ones that 
>>>>> succeeded. 
>>>>  
>>>> Unless explicitly requested, I don't think the system should create 
>>>> situations some interfaces are assigned to host drivers and some to 
>>>> usbback. 
>>>  
>>> I was talking here about whether it would be better to ha

Re: [Xen-devel] [PATCH V13 3/5] libxl: add pvusb API

[Juergen Gross]

On 04/02/16 02:53, Chun Yan Liu wrote:
> 
> 
>>>> On 2/3/2016 at 10:33 PM, in message <56b20fcc.3010...@citrix.com>, George
> Dunlap <george.dun...@citrix.com> wrote: 
>> On 02/02/16 18:11, Ian Jackson wrote: 
>>> George Dunlap writes ("Re: [Xen-devel] [PATCH V13 3/5] libxl: add pvusb  
>> API"): 
>>>> There are effectively four states a device can be in, from the 
>>>> 'assignment' point of view: 
>>>>
>>>> 1. Assigned to the normal Linux device driver(s) for the USB device 
>>>>
>>>> 2. Assigned to no driver 
>>>>
>>>> 3. Assigned to usbback, but not yet assigned to any guest 
>>>>
>>>> 4. Assigned to a guest 
>>>  
>>> Thanks for your clear explanation (of which I will snip much). 
>>>  
>>>> Additionally, each USB "device" has one or more "interfaces".  To 
>>>> assign a "device" to usbback in the sysfs case means assigning all of 
>>>> the "interfaces".  The code seems to assume that different interfaces 
>>>> from the same device can be assigned to different drivers. 
>>>  
>>> It is indeed the case that in principle a single USB device with 
>>> multiple interfaces can be assigned to multiple different drivers. 
>>>  
>>>> Regarding Ian's comments: 
>>>>
>>>> Since "assigned to the guest" and "listed under the pvusb xenstore 
>>>> node" are the same thing, is it even *possible* to (safely) unassign 
>>>> the device from usbback before removing the xenstore nodes? 
>>>  
>>> It might be possible to remove some of the xenstore nodes but leave 
>>> others present, so that usbback detaches, but enough information 
>>> remains for libxl to know that Xen still `owns' the device. 
>>>  
>>> But, surely usbback needs to cope with the notion that the device 
>>> might disappear.  USB devices can disappear at any time. 
>>  
>> That's true. But if the USB device has actually disappeared, the device 
>> is in fact "safe" from usbback.  I think I might consider state 2 safe 
>> to go to, but I definitely wouldn't consider state 1 safe with the 
>> xenstore nodes still in place. 
>>  
>>>  
>>>> It's not clear to me under what conditions 3->2 might fail, 
>>>  
>>> As an example, someone might press ^C on `xl usb-detach'. 
>>  
>> *grumble* 
>>  
>>>> or what could be done about it if it did fail. 
>>>  
>>> The most obvious reason for it failing is that something somewhere 
>>> still held onto the device.  (For umounting filesystems, and detaching 
>>> block devices, this happens a lot.)  So if the detach from usbback 
>>> fails would probably be possible to simply retry it. 
>>  
>> I'm not sure what this means in the context of moving from 3 (assigned 
>> to usbback) to 2 (unassigned).  usbback will definitely not use the 
>> device to mount something in dom0; and I'm pretty sure has no visibility 
>> as to whether it's being used as a filesystem in the domU. 
>>  
>> (Moving from 1 -> 2 of course would be subject to this sort of thing, 
>> but that's a different issue.) 
>>  
>>>> Regarding 2->1, again it's not clear that there's anything libxl can 
>>>> do.  Reloading the driver's module might reset it enough to pick up 
>>>> the (now unassigned) USB devices again; other than that, rebooting is 
>>>> probably the best option. 
>>>  
>>> I think re-attempting the bind may work.  USB devices can be flaky. 
>>>  
>>>> It's also not clear to me, if some functions succeed in being 
>>>> reassigned and others fail, whether it's best to just try to assign as 
>>>> many as we can, or better to go back and un-assign all the ones that 
>>>> succeeded. 
>>>  
>>> Unless explicitly requested, I don't think the system should create 
>>> situations some interfaces are assigned to host drivers and some to 
>>> usbback. 
>>  
>> I was talking here about whether it would be better to have some 
>> interfaces assigned to the original drivers and some interfaces left 
>> unassigned, or to try to leave all interfaces unassigned if any of the 
>> assignments fail. 
>>  
>> I agree, it would be better to try to avoid the possibility of having 
>> some interfaces bound to usbback and some interfaces bound to the 
>> original dr

Re: [Xen-devel] [PATCH V13 3/5] libxl: add pvusb API

[George Dunlap]

On 03/02/16 07:34, Chun Yan Liu wrote:
> 
> 
>>>> On 2/3/2016 at 02:11 AM, in message
> <22192.61775.427189.268...@mariner.uk.xensource.com>, Ian Jackson
> <ian.jack...@eu.citrix.com> wrote: 
>> George Dunlap writes ("Re: [Xen-devel] [PATCH V13 3/5] libxl: add pvusb 
>> API"): 
>>> There are effectively four states a device can be in, from the 
>>> 'assignment' point of view: 
>>>  
>>> 1. Assigned to the normal Linux device driver(s) for the USB device 
>>>  
>>> 2. Assigned to no driver 
>>>  
>>> 3. Assigned to usbback, but not yet assigned to any guest 
>>>  
>>> 4. Assigned to a guest 
>>  
>> Thanks for your clear explanation (of which I will snip much). 
>>  
>>> Additionally, each USB "device" has one or more "interfaces".  To 
>>> assign a "device" to usbback in the sysfs case means assigning all of 
>>> the "interfaces".  The code seems to assume that different interfaces 
>>> from the same device can be assigned to different drivers. 
>>  
>> It is indeed the case that in principle a single USB device with 
>> multiple interfaces can be assigned to multiple different drivers. 
>>  
>>> Regarding Ian's comments: 
>>>  
>>> Since "assigned to the guest" and "listed under the pvusb xenstore 
>>> node" are the same thing, is it even *possible* to (safely) unassign 
>>> the device from usbback before removing the xenstore nodes? 
>>  
>> It might be possible to remove some of the xenstore nodes but leave 
>> others present, so that usbback detaches, but enough information 
>> remains for libxl to know that Xen still `owns' the device. 
> 
> Indeed "unassign from usbback, but listed under pvusb xenstore" is
> a confusing state. usb-list can list it but guest can not see it. 
> What user can do under that state is: reattempt usbdev_remove, if it 
> succeeds, everything is cleaned up, that's the best result; but 
> possibly it still fails (for example, in my testing, always cannot 
> rebind to original driver), in this case, the confusing state will 
> be lasting, and the device could not be removed, this is worse.

As I said in my other mail, I think removing the pvusb nodes should be
done once it's successfully un-bound from usbback, *even if* the re-bind
to the original driver fails.  (That is, once it reaches state 2,
usb-list should no longer list it.)

>>> Perhaps the best approach code-wise is to change the "goto out" on 
>>> failure of unbind_usbintf() into a "continue".  That way: 
>>>  
>>> 1. All interfaces which can be re-assigned are re-assigned (and work 
>>> as much as possible) 
>>>  
>>> 2. All interfaces which can be unbound but not re-assigned are at 
>>> least unbound (so that reloading the original driver might pick them 
>>> up) 
>>  
>> I certainly don't mind the software trying to do as much of its task 
>> as possible.
> 
> Could I understand that this way is acceptable? That means: removing 
> xenstore, and as much as we could (on failure of "unbind from usbback"
> or "bind to original driver", don't "goto out", just "continue").

I think part of it depends on what is *possible*.  If it's possible to
safely unbind the device from usbback while retaining its place in the
pvusb-related xenstore nodes, then I think we should (so that the user
can re-try removing it).  If it's not possible, then of course we have
to remove the pvusb xenstore nodes first, and then we'll just have to
deal as gracefully as possible with failure unbinding from usbback.

 -George

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

Re: [Xen-devel] [PATCH V13 3/5] libxl: add pvusb API

[George Dunlap]

On 02/02/16 18:11, Ian Jackson wrote:
> George Dunlap writes ("Re: [Xen-devel] [PATCH V13 3/5] libxl: add pvusb API"):
>> There are effectively four states a device can be in, from the
>> 'assignment' point of view:
>>
>> 1. Assigned to the normal Linux device driver(s) for the USB device
>>
>> 2. Assigned to no driver
>>
>> 3. Assigned to usbback, but not yet assigned to any guest
>>
>> 4. Assigned to a guest
> 
> Thanks for your clear explanation (of which I will snip much).
> 
>> Additionally, each USB "device" has one or more "interfaces".  To
>> assign a "device" to usbback in the sysfs case means assigning all of
>> the "interfaces".  The code seems to assume that different interfaces
>> from the same device can be assigned to different drivers.
> 
> It is indeed the case that in principle a single USB device with
> multiple interfaces can be assigned to multiple different drivers.
> 
>> Regarding Ian's comments:
>>
>> Since "assigned to the guest" and "listed under the pvusb xenstore
>> node" are the same thing, is it even *possible* to (safely) unassign
>> the device from usbback before removing the xenstore nodes?
> 
> It might be possible to remove some of the xenstore nodes but leave
> others present, so that usbback detaches, but enough information
> remains for libxl to know that Xen still `owns' the device.
> 
> But, surely usbback needs to cope with the notion that the device
> might disappear.  USB devices can disappear at any time.

That's true. But if the USB device has actually disappeared, the device
is in fact "safe" from usbback.  I think I might consider state 2 safe
to go to, but I definitely wouldn't consider state 1 safe with the
xenstore nodes still in place.

> 
>> It's not clear to me under what conditions 3->2 might fail,
> 
> As an example, someone might press ^C on `xl usb-detach'.

*grumble*

>> or what could be done about it if it did fail.
> 
> The most obvious reason for it failing is that something somewhere
> still held onto the device.  (For umounting filesystems, and detaching
> block devices, this happens a lot.)  So if the detach from usbback
> fails would probably be possible to simply retry it.

I'm not sure what this means in the context of moving from 3 (assigned
to usbback) to 2 (unassigned).  usbback will definitely not use the
device to mount something in dom0; and I'm pretty sure has no visibility
as to whether it's being used as a filesystem in the domU.

(Moving from 1 -> 2 of course would be subject to this sort of thing,
but that's a different issue.)

>> Regarding 2->1, again it's not clear that there's anything libxl can
>> do.  Reloading the driver's module might reset it enough to pick up
>> the (now unassigned) USB devices again; other than that, rebooting is
>> probably the best option.
> 
> I think re-attempting the bind may work.  USB devices can be flaky.
> 
>> It's also not clear to me, if some functions succeed in being
>> reassigned and others fail, whether it's best to just try to assign as
>> many as we can, or better to go back and un-assign all the ones that
>> succeeded.
> 
> Unless explicitly requested, I don't think the system should create
> situations some interfaces are assigned to host drivers and some to
> usbback.

I was talking here about whether it would be better to have some
interfaces assigned to the original drivers and some interfaces left
unassigned, or to try to leave all interfaces unassigned if any of the
assignments fail.

I agree, it would be better to try to avoid the possibility of having
some interfaces bound to usbback and some interfaces bound to the
original drivers.

> And, I'm a fan of `crash-only software': in general, if a failure
> occurs, the situation should just be left as-is.  The intermediate
> state needs to be visible and rectifiable.
> 
> This approach to software state machines avoids bugs where the system
> gets into `impossible' states, recovery from which is impossible
> because the designers didn't anticipate them.
> 
> It would be tolerable if the recovery sometimes involves `lsusb' and
> echoing things into sysfs, but it would be better if not.

Right -- so what about this.  When removing a USB device:

* First modify the pvusb xenstore nodes such that 1) it's safe to
attempt removing the interfaces from usbback, but 2) they still show up
in usb-list.  (This may be a noop.)

* Attempt to remove all interfaces from usbback; if any of these fail,
stop and report an error.  Possible recovery options:
 - Re-try the usb_remove command
 - Re-load usbback (obviously disruptive to other VMs)
 - Reboot the host
 - Manually

Re: [Xen-devel] [PATCH V13 3/5] libxl: add pvusb API

[Chun Yan Liu]

>>> On 2/3/2016 at 10:38 PM, in message <56b210fa.7040...@citrix.com>, George
Dunlap <george.dun...@citrix.com> wrote: 
> On 03/02/16 07:34, Chun Yan Liu wrote: 
> >  
> >  
> >>>> On 2/3/2016 at 02:11 AM, in message 
> > <22192.61775.427189.268...@mariner.uk.xensource.com>, Ian Jackson 
> > <ian.jack...@eu.citrix.com> wrote:  
> >> George Dunlap writes ("Re: [Xen-devel] [PATCH V13 3/5] libxl: add pvusb  
> API"):  
> >>> There are effectively four states a device can be in, from the  
> >>> 'assignment' point of view:  
> >>>   
> >>> 1. Assigned to the normal Linux device driver(s) for the USB device  
> >>>   
> >>> 2. Assigned to no driver  
> >>>   
> >>> 3. Assigned to usbback, but not yet assigned to any guest  
> >>>   
> >>> 4. Assigned to a guest  
> >>   
> >> Thanks for your clear explanation (of which I will snip much).  
> >>   
> >>> Additionally, each USB "device" has one or more "interfaces".  To  
> >>> assign a "device" to usbback in the sysfs case means assigning all of  
> >>> the "interfaces".  The code seems to assume that different interfaces  
> >>> from the same device can be assigned to different drivers.  
> >>   
> >> It is indeed the case that in principle a single USB device with  
> >> multiple interfaces can be assigned to multiple different drivers.  
> >>   
> >>> Regarding Ian's comments:  
> >>>   
> >>> Since "assigned to the guest" and "listed under the pvusb xenstore  
> >>> node" are the same thing, is it even *possible* to (safely) unassign  
> >>> the device from usbback before removing the xenstore nodes?  
> >>   
> >> It might be possible to remove some of the xenstore nodes but leave  
> >> others present, so that usbback detaches, but enough information  
> >> remains for libxl to know that Xen still `owns' the device.  
> >  
> > Indeed "unassign from usbback, but listed under pvusb xenstore" is 
> > a confusing state. usb-list can list it but guest can not see it.  
> > What user can do under that state is: reattempt usbdev_remove, if it  
> > succeeds, everything is cleaned up, that's the best result; but  
> > possibly it still fails (for example, in my testing, always cannot  
> > rebind to original driver), in this case, the confusing state will  
> > be lasting, and the device could not be removed, this is worse. 
>  
> As I said in my other mail, I think removing the pvusb nodes should be 
> done once it's successfully un-bound from usbback, *even if* the re-bind 
> to the original driver fails.  (That is, once it reaches state 2, 
> usb-list should no longer list it.) 
>  
> >>> Perhaps the best approach code-wise is to change the "goto out" on  
> >>> failure of unbind_usbintf() into a "continue".  That way:  
> >>>   
> >>> 1. All interfaces which can be re-assigned are re-assigned (and work  
> >>> as much as possible)  
> >>>   
> >>> 2. All interfaces which can be unbound but not re-assigned are at  
> >>> least unbound (so that reloading the original driver might pick them  
> >>> up)  
> >>   
> >> I certainly don't mind the software trying to do as much of its task  
> >> as possible. 
> >  
> > Could I understand that this way is acceptable? That means: removing  
> > xenstore, and as much as we could (on failure of "unbind from usbback" 
> > or "bind to original driver", don't "goto out", just "continue"). 
>  
> I think part of it depends on what is *possible*.  If it's possible to 
> safely unbind the device from usbback while retaining its place in the 
> pvusb-related xenstore nodes, then I think we should (so that the user

Juergen, I think you are the person who can answer the question best.
Can you confirm that?

-Chunyan

> can re-try removing it).  If it's not possible, then of course we have 
> to remove the pvusb xenstore nodes first, and then we'll just have to 
> deal as gracefully as possible with failure unbinding from usbback. 
>  
>  -George 
>  
>  


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

Re: [Xen-devel] [PATCH V13 3/5] libxl: add pvusb API

[Chun Yan Liu]

>>> On 2/3/2016 at 10:33 PM, in message <56b20fcc.3010...@citrix.com>, George
Dunlap <george.dun...@citrix.com> wrote: 
> On 02/02/16 18:11, Ian Jackson wrote: 
> > George Dunlap writes ("Re: [Xen-devel] [PATCH V13 3/5] libxl: add pvusb  
> API"): 
> >> There are effectively four states a device can be in, from the 
> >> 'assignment' point of view: 
> >> 
> >> 1. Assigned to the normal Linux device driver(s) for the USB device 
> >> 
> >> 2. Assigned to no driver 
> >> 
> >> 3. Assigned to usbback, but not yet assigned to any guest 
> >> 
> >> 4. Assigned to a guest 
> >  
> > Thanks for your clear explanation (of which I will snip much). 
> >  
> >> Additionally, each USB "device" has one or more "interfaces".  To 
> >> assign a "device" to usbback in the sysfs case means assigning all of 
> >> the "interfaces".  The code seems to assume that different interfaces 
> >> from the same device can be assigned to different drivers. 
> >  
> > It is indeed the case that in principle a single USB device with 
> > multiple interfaces can be assigned to multiple different drivers. 
> >  
> >> Regarding Ian's comments: 
> >> 
> >> Since "assigned to the guest" and "listed under the pvusb xenstore 
> >> node" are the same thing, is it even *possible* to (safely) unassign 
> >> the device from usbback before removing the xenstore nodes? 
> >  
> > It might be possible to remove some of the xenstore nodes but leave 
> > others present, so that usbback detaches, but enough information 
> > remains for libxl to know that Xen still `owns' the device. 
> >  
> > But, surely usbback needs to cope with the notion that the device 
> > might disappear.  USB devices can disappear at any time. 
>  
> That's true. But if the USB device has actually disappeared, the device 
> is in fact "safe" from usbback.  I think I might consider state 2 safe 
> to go to, but I definitely wouldn't consider state 1 safe with the 
> xenstore nodes still in place. 
>  
> >  
> >> It's not clear to me under what conditions 3->2 might fail, 
> >  
> > As an example, someone might press ^C on `xl usb-detach'. 
>  
> *grumble* 
>  
> >> or what could be done about it if it did fail. 
> >  
> > The most obvious reason for it failing is that something somewhere 
> > still held onto the device.  (For umounting filesystems, and detaching 
> > block devices, this happens a lot.)  So if the detach from usbback 
> > fails would probably be possible to simply retry it. 
>  
> I'm not sure what this means in the context of moving from 3 (assigned 
> to usbback) to 2 (unassigned).  usbback will definitely not use the 
> device to mount something in dom0; and I'm pretty sure has no visibility 
> as to whether it's being used as a filesystem in the domU. 
>  
> (Moving from 1 -> 2 of course would be subject to this sort of thing, 
> but that's a different issue.) 
>  
> >> Regarding 2->1, again it's not clear that there's anything libxl can 
> >> do.  Reloading the driver's module might reset it enough to pick up 
> >> the (now unassigned) USB devices again; other than that, rebooting is 
> >> probably the best option. 
> >  
> > I think re-attempting the bind may work.  USB devices can be flaky. 
> >  
> >> It's also not clear to me, if some functions succeed in being 
> >> reassigned and others fail, whether it's best to just try to assign as 
> >> many as we can, or better to go back and un-assign all the ones that 
> >> succeeded. 
> >  
> > Unless explicitly requested, I don't think the system should create 
> > situations some interfaces are assigned to host drivers and some to 
> > usbback. 
>  
> I was talking here about whether it would be better to have some 
> interfaces assigned to the original drivers and some interfaces left 
> unassigned, or to try to leave all interfaces unassigned if any of the 
> assignments fail. 
>  
> I agree, it would be better to try to avoid the possibility of having 
> some interfaces bound to usbback and some interfaces bound to the 
> original drivers. 
>  
> > And, I'm a fan of `crash-only software': in general, if a failure 
> > occurs, the situation should just be left as-is.  The intermediate 
> > state needs to be visible and rectifiable. 
> >  
> > This approach to software state machines avoids bugs where the system 
> > gets into `impossi

Re: [Xen-devel] [PATCH V13 3/5] libxl: add pvusb API

[Ian Jackson]

George Dunlap writes ("Re: [Xen-devel] [PATCH V13 3/5] libxl: add pvusb API"):
> There are effectively four states a device can be in, from the
> 'assignment' point of view:
> 
> 1. Assigned to the normal Linux device driver(s) for the USB device
> 
> 2. Assigned to no driver
> 
> 3. Assigned to usbback, but not yet assigned to any guest
> 
> 4. Assigned to a guest

Thanks for your clear explanation (of which I will snip much).

> Additionally, each USB "device" has one or more "interfaces".  To
> assign a "device" to usbback in the sysfs case means assigning all of
> the "interfaces".  The code seems to assume that different interfaces
> from the same device can be assigned to different drivers.

It is indeed the case that in principle a single USB device with
multiple interfaces can be assigned to multiple different drivers.

> Regarding Ian's comments:
> 
> Since "assigned to the guest" and "listed under the pvusb xenstore
> node" are the same thing, is it even *possible* to (safely) unassign
> the device from usbback before removing the xenstore nodes?

It might be possible to remove some of the xenstore nodes but leave
others present, so that usbback detaches, but enough information
remains for libxl to know that Xen still `owns' the device.

But, surely usbback needs to cope with the notion that the device
might disappear.  USB devices can disappear at any time.

> It's not clear to me under what conditions 3->2 might fail,

As an example, someone might press ^C on `xl usb-detach'.

> or what could be done about it if it did fail.

The most obvious reason for it failing is that something somewhere
still held onto the device.  (For umounting filesystems, and detaching
block devices, this happens a lot.)  So if the detach from usbback
fails would probably be possible to simply retry it.

> Regarding 2->1, again it's not clear that there's anything libxl can
> do.  Reloading the driver's module might reset it enough to pick up
> the (now unassigned) USB devices again; other than that, rebooting is
> probably the best option.

I think re-attempting the bind may work.  USB devices can be flaky.

> It's also not clear to me, if some functions succeed in being
> reassigned and others fail, whether it's best to just try to assign as
> many as we can, or better to go back and un-assign all the ones that
> succeeded.

Unless explicitly requested, I don't think the system should create
situations some interfaces are assigned to host drivers and some to
usbback.

And, I'm a fan of `crash-only software': in general, if a failure
occurs, the situation should just be left as-is.  The intermediate
state needs to be visible and rectifiable.

This approach to software state machines avoids bugs where the system
gets into `impossible' states, recovery from which is impossible
because the designers didn't anticipate them.

It would be tolerable if the recovery sometimes involves `lsusb' and
echoing things into sysfs, but it would be better if not.

> Perhaps the best approach code-wise is to change the "goto out" on
> failure of unbind_usbintf() into a "continue".  That way:
> 
> 1. All interfaces which can be re-assigned are re-assigned (and work
> as much as possible)
> 
> 2. All interfaces which can be unbound but not re-assigned are at
> least unbound (so that reloading the original driver might pick them
> up)

I certainly don't mind the software trying to do as much of its task
as possible.

Ian.

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

Re: [Xen-devel] [PATCH V13 3/5] libxl: add pvusb API

[George Dunlap]

On Tue, Jan 19, 2016 at 3:48 PM, Ian Jackson  wrote:
> Chunyan Liu writes ("[PATCH V13 3/5] libxl: add pvusb API"):
>> > Ought this function to really report success if these calls fail ?
>>
>> I think so. Till here, the USB device has already been unbound from
>> usbback and removed from xenstore. usb-list cannot list it any more.

Also, I think we probably should return some sort of error, so that
scripts  can at least know something out of the ordinary has
happened, even if it doesn't go back to the state we started in when
the function was called.

 -George

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

Re: [Xen-devel] [PATCH V13 3/5] libxl: add pvusb API

[George Dunlap]

On Tue, Jan 19, 2016 at 3:48 PM, Ian Jackson  wrote:
> Chunyan Liu writes ("[PATCH V13 3/5] libxl: add pvusb API"):
>> +static int usbback_dev_unassign(libxl__gc *gc, const char *busid)
>> +{
> ...
>> +/* Till here, USB device has been unbound from USBBACK and
>> + * removed from xenstore, usb list couldn't show it anymore,
>> + * so no matter removimg driver path successfully or not,
>> + * we will report operation success.
>> + */
>
> I'm still unconvinced by this and this may mean that the code in this
> function is in the wrong order.  Earlier we had this exchange:
>
>> > Ought this function to really report success if these calls fail ?
>>
>> I think so. Till here, the USB device has already been unbound from
>> usbback and removed from xenstore. usb-list cannot list it any more.
>
> The problem is that I think that if this function fails, it can leave
>  - debris in xenstore (the usbback path)
>  - the interface bound to the wrong driver
> And then there is no way for the user to get libxl to re-attempt the
> operation, or clean up.  Am I right ?
>
> One way to avoid this kind of problem is to deal with the xenstore
> path last.  That way the device will still appear as attached to the
> domain.
>
> To do this properly I think bind_usbintf may need to become idempotent
> even in the face of other callers racing to assign the device.  I
> think that would mean bind_usbif it would have to know what driver to
> expect to find the device bound to.
>
> George, am I right here ?

There are effectively four states a device can be in, from the
'assignment' point of view:

1. Assigned to the normal Linux device driver(s) for the USB device

2. Assigned to no driver

3. Assigned to usbback, but not yet assigned to any guest

4. Assigned to a guest

Regardless of the state, the USB device is visible to lsusb, and
always exists in the usb controller's device heirarchy, regardless of
what driver it's assigned to (if any).

As far as sysfs and lspci goes, pci is an exact analog.

For usb, libxl really only has two states: assigned (state 4) and
unassigned (states 1 or 2).  libxl__device_usbdev_add() will take
devices from states 1 or 2 and move them into state 3, then state 4.
libxl__device_usbdev_remove() will take devices from state 4, through
state 3, and then to state 2 (and then possibly state 1, if it has the
appropriate information).  usbdev_list will only list things in state
4.

Additionally, each USB "device" has one or more "interfaces".  To
assign a "device" to usbback in the sysfs case means assigning all of
the "interfaces".  The code seems to assume that different interfaces
from the same device can be assigned to different drivers.

For comparison, the libxl pci pass-through code has three states: not
assignable (states 1 or 2), assignable (state 3), and assigned (state
4).  libxl__device_pci_assignable_add moves it from 1 or 2 to state 3;
libxl_device_pci_add moves it from state 3 to 4 (unless the "seize"
flag is set, in which case it will call
libxl__device_pci_assignable_add if necessary).
libxl_device_pci_assignable_list lists things in state 3;
libxl_device_pci_list list things in state 4.

pci devices also have analogs called "functions"; but libxl treats
each of these separately (i.e., you actually assign a "function" to a
VM, not a "device").  If a device has several functions which all need
to be assigned, it's up to the user to assign each one individually.

So for removal, we have to consider what to do with a failure at each
of the steps.

* 4 -> 3.  This is just removing it from xenstore.  The xenstore
removal is transactional, so (I think) a failure here should mean that
it's still in state 4; it should still show up in usb_list, and the
user could try removing again if she wants.

* 3 -> 2. At this point, what the code actually does is try to unbind
each interface from usbback; if any of the calls to unbind_usbintf()
fail, it will give up (leaving any remaining functions bound to
usbback).

* 2 -> 1.  If we stored a driver path in xenstore (under the /libxl
node, not under the pvusb node), we then try to rebind each interface
to that driver.  If this binding fails, we simply print a warning and
continue (i.e., we attempt to re-bind with all interfaces, even if one
of them fails).

Regarding Ian's comments:

Since "assigned to the guest" and "listed under the pvusb xenstore
node" are the same thing, is it even *possible* to (safely) unassign
the device from usbback before removing the xenstore nodes?

It's not clear to me under what conditions 3->2 might fail, or what
could be done about it if it did fail.  Perhaps removing the usbback
module?  Failing that, rebooting the system is the only recovery
option I can think of.  Certainly trying to assign it back to the
guest (i.e., move it back to the previous legal state of '4') isn't a
good idea.

I do agree, however, that stopping in the middle here (leaving some
interfaces assigned to 

Re: [Xen-devel] [PATCH V13 3/5] libxl: add pvusb API

[Chun Yan Liu]

>>> On 1/27/2016 at 12:21 AM, in message
, George
Dunlap  wrote: 
> On Tue, Jan 26, 2016 at 7:43 AM, Chun Yan Liu  wrote: 
> > 
> > 
>  On 1/20/2016 at 12:56 PM, in message 
> > <569f83f502660009e...@relay2.provo.novell.com>, "Chun Yan Liu" 
> >  wrote: 
> > 
> >> 
> > On 1/19/2016 at 11:48 PM, in message 
> >> <22174.23240.402164.635...@mariner.uk.xensource.com>, Ian Jackson 
> >>  wrote: 
> >> > Chunyan Liu writes ("[PATCH V13 3/5] libxl: add pvusb API"): 
> >> > > Add pvusb APIs, including: 
> >> > >  - attach/detach (create/destroy) virtual usb controller. 
> >> > >  - attach/detach usb device 
> >> > >  - list usb controller and usb devices 
> >> > >  - some other helper functions 
> >> > 
> >> > 
> >> > Thanks.  This is making progress but I'm afraid we're not quite there 
> >> > yet. 
> >> > 
> >> > 
> >> > > +static int usbback_dev_unassign(libxl__gc *gc, const char *busid) 
> >> > > +{ 
> >> > ... 
> >> > > +/* Till here, USB device has been unbound from USBBACK and 
> >> > > + * removed from xenstore, usb list couldn't show it anymore, 
> >> > > + * so no matter removimg driver path successfully or not, 
> >> > > + * we will report operation success. 
> >> > > + */ 
> >> > 
> >> > I'm still unconvinced by this and this may mean that the code in this 
> >> > function is in the wrong order.  Earlier we had this exchange: 
> >> > 
> >> > > > Ought this function to really report success if these calls fail ? 
> >> > > 
> >> > > I think so. Till here, the USB device has already been unbound from 
> >> > > usbback and removed from xenstore. usb-list cannot list it any more. 
> >> > 
> >> > The problem is that I think that if this function fails, it can leave 
> >> >  - debris in xenstore (the usbback path) 
> >> Yes, it's true. 
> >> 
> >> >  - the interface bound to the wrong driver 
> >> No, it won't be bound to 'wrong' driver, only maybe not bound to any 
> >> driver 
> >> (Already unbound from usbback, but failed to rebound to its original 
> >> driver). 
> >> In this case, we would report warning: failed to rebind to driver xxx. 
> >> 
> >> > And then there is no way for the user to get libxl to re-attempt the 
> >> > operation, or clean up.  Am I right ? 
> >> 
> >> Yes. No way to re-attempt usbdev-detach or cleanup driver path in 
> >> xenstore. But won't affect next time usbdev-attach the same device. 
> >> 
> >> > 
> >> > One way to avoid this kind of problem is to deal with the xenstore 
> >> > path last.  That way the device will still appear as attached to the 
> >> > domain. 
> >> 
> >> I'm afraid if the side effect is acceptable? In my testing, some USB
> >> bluetooth 
> >> device always fails to rebind to 'btusb' driver after it's unbound 
> >> from 'usbback'. In this case, we can't detach it from the domain then. 

George & Ian, may I have your thoughts? Except for above case, I think dealing
with xenstore at last place is indeed better.
PS: I'll take vocation for half month, so hope to make any progress before that 
:-)

Thanks,
Chunyan

> > 
> > Ian J., any opinion on this? If it's still thought to be better, I'll  
> update patch. 
>  
> I think Ian may be waiting for me to reply and express an opinion; but 
> unfortunately that will have to wait until next week. :-( 
>  
>  -George 
>  
> ___ 
> Xen-devel mailing list 
> Xen-devel@lists.xen.org 
> http://lists.xen.org/xen-devel 
>  
>  



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

Re: [Xen-devel] [PATCH V13 3/5] libxl: add pvusb API

[Olaf Hering]

On Tue, Jan 19, Chunyan Liu wrote:

> +++ b/tools/libxl/libxl.c
> @@ -3204,7 +3204,7 @@ void 
> libxl__device_disk_local_initiate_detach(libxl__egc *egc,
>  aodev->dev = device;
>  aodev->callback = local_device_detach_cb;
>  aodev->force = 0;
> -libxl__initiate_device_remove(egc, aodev);
> +libxl__initiate_device_generic_remove(egc, aodev);
>  return;
>  }
>  
> @@ -4172,8 +4172,10 @@ out:
>   * libxl_device_vkb_destroy
>   * libxl_device_vfb_remove
>   * libxl_device_vfb_destroy
> + * libxl_device_usbctrl_remove
> + * libxl_device_usbctrl_destroy

This should be moved down to DEFINE_DEVICE_REMOVE_CUSTOM.

>   */
> -#define DEFINE_DEVICE_REMOVE(type, removedestroy, f)\
> +#define DEFINE_DEVICE_REMOVE_EXT(type, remtype, removedestroy, f)\
>  int libxl_device_##type##_##removedestroy(libxl_ctx *ctx,   \
>  uint32_t domid, libxl_device_##type *type,  \
>  const libxl_asyncop_how *ao_how)\
> @@ -4193,13 +4195,19 @@ out:
>  aodev->dev = device;\
>  aodev->callback = device_addrm_aocomplete;  \
>  aodev->force = f;   \
> -libxl__initiate_device_remove(egc, aodev);  \
> +libxl__initiate_device_##remtype##_remove(egc, aodev);  \
>  \
>  out:\
> -if (rc) return AO_CREATE_FAIL(rc);   
>  \
> +if (rc) return AO_CREATE_FAIL(rc);  \
>  return AO_INPROGRESS;   \
>  }
>  
> +#define DEFINE_DEVICE_REMOVE(type, removedestroy, f) \
> +DEFINE_DEVICE_REMOVE_EXT(type, generic, removedestroy, f)
> +
> +#define DEFINE_DEVICE_REMOVE_CUSTOM(type, removedestroy, f)  \
> +DEFINE_DEVICE_REMOVE_EXT(type, type, removedestroy, f)
> +
>  /* Define all remove/destroy functions and undef the macro */
>  
>  /* disk */


If this is the way to move forward, please split this out into a
separate change which can be applied to staging independent of any pvusb
changes.

I think the patch needs also the #undef.


> @@ -4223,6 +4231,10 @@ DEFINE_DEVICE_REMOVE(vfb, destroy, 1)
>  DEFINE_DEVICE_REMOVE(vtpm, remove, 0)
>  DEFINE_DEVICE_REMOVE(vtpm, destroy, 1)
>  
> +/* usbctrl */
> +DEFINE_DEVICE_REMOVE_CUSTOM(usbctrl, remove, 0)
> +DEFINE_DEVICE_REMOVE_CUSTOM(usbctrl, destroy, 1)
> +
>  /* channel/console hotunplug is not implemented. There are 2 possibilities:
>   * 1. add support for secondary consoles to xenconsoled
>   * 2. dynamically add/remove qemu chardevs via qmp messages. */

A comment should mention both libxl_device_usbctrl_remove/destroy and
libxl__initiate_device_usbctrl_remove/destroy.

Olaf

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

Re: [Xen-devel] [PATCH V13 3/5] libxl: add pvusb API

[George Dunlap]

On Tue, Jan 26, 2016 at 7:43 AM, Chun Yan Liu  wrote:
>
>
 On 1/20/2016 at 12:56 PM, in message
> <569f83f502660009e...@relay2.provo.novell.com>, "Chun Yan Liu"
>  wrote:
>
>>
> On 1/19/2016 at 11:48 PM, in message
>> <22174.23240.402164.635...@mariner.uk.xensource.com>, Ian Jackson
>>  wrote:
>> > Chunyan Liu writes ("[PATCH V13 3/5] libxl: add pvusb API"):
>> > > Add pvusb APIs, including:
>> > >  - attach/detach (create/destroy) virtual usb controller.
>> > >  - attach/detach usb device
>> > >  - list usb controller and usb devices
>> > >  - some other helper functions
>> >
>> >
>> > Thanks.  This is making progress but I'm afraid we're not quite there
>> > yet.
>> >
>> >
>> > > +static int usbback_dev_unassign(libxl__gc *gc, const char *busid)
>> > > +{
>> > ...
>> > > +/* Till here, USB device has been unbound from USBBACK and
>> > > + * removed from xenstore, usb list couldn't show it anymore,
>> > > + * so no matter removimg driver path successfully or not,
>> > > + * we will report operation success.
>> > > + */
>> >
>> > I'm still unconvinced by this and this may mean that the code in this
>> > function is in the wrong order.  Earlier we had this exchange:
>> >
>> > > > Ought this function to really report success if these calls fail ?
>> > >
>> > > I think so. Till here, the USB device has already been unbound from
>> > > usbback and removed from xenstore. usb-list cannot list it any more.
>> >
>> > The problem is that I think that if this function fails, it can leave
>> >  - debris in xenstore (the usbback path)
>> Yes, it's true.
>>
>> >  - the interface bound to the wrong driver
>> No, it won't be bound to 'wrong' driver, only maybe not bound to any driver
>> (Already unbound from usbback, but failed to rebound to its original
>> driver).
>> In this case, we would report warning: failed to rebind to driver xxx.
>>
>> > And then there is no way for the user to get libxl to re-attempt the
>> > operation, or clean up.  Am I right ?
>>
>> Yes. No way to re-attempt usbdev-detach or cleanup driver path in
>> xenstore. But won't affect next time usbdev-attach the same device.
>>
>> >
>> > One way to avoid this kind of problem is to deal with the xenstore
>> > path last.  That way the device will still appear as attached to the
>> > domain.
>>
>> I'm afraid if the side effect is acceptable? In my testing, some USB
>> bluetooth
>> device always fails to rebind to 'btusb' driver after it's unbound
>> from 'usbback'. In this case, we can't detach it from the domain then.
>
> Ian J., any opinion on this? If it's still thought to be better, I'll update 
> patch.

I think Ian may be waiting for me to reply and express an opinion; but
unfortunately that will have to wait until next week. :-(

 -George

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

Re: [Xen-devel] [PATCH V13 3/5] libxl: add pvusb API

[Chun Yan Liu]

>>> On 1/27/2016 at 12:12 AM, in message <20160126161253.ga9...@gmail.com>, Olaf
Hering  wrote: 
> On Tue, Jan 19, Chunyan Liu wrote: 
>  
> > +++ b/tools/libxl/libxl.c 
> > @@ -3204,7 +3204,7 @@ void  
> libxl__device_disk_local_initiate_detach(libxl__egc *egc, 
> >  aodev->dev = device; 
> >  aodev->callback = local_device_detach_cb; 
> >  aodev->force = 0; 
> > -libxl__initiate_device_remove(egc, aodev); 
> > +libxl__initiate_device_generic_remove(egc, aodev); 
> >  return; 
> >  } 
> >   
> > @@ -4172,8 +4172,10 @@ out: 
> >   * libxl_device_vkb_destroy 
> >   * libxl_device_vfb_remove 
> >   * libxl_device_vfb_destroy 
> > + * libxl_device_usbctrl_remove 
> > + * libxl_device_usbctrl_destroy 
>  
> This should be moved down to DEFINE_DEVICE_REMOVE_CUSTOM. 
>  
> >   */ 
> > -#define DEFINE_DEVICE_REMOVE(type, removedestroy, f)\ 
> > +#define DEFINE_DEVICE_REMOVE_EXT(type, remtype, removedestroy, f)\ 
> >  int libxl_device_##type##_##removedestroy(libxl_ctx *ctx,   \ 
> >  uint32_t domid, libxl_device_##type *type,  \ 
> >  const libxl_asyncop_how *ao_how)\ 
> > @@ -4193,13 +4195,19 @@ out: 
> >  aodev->dev = device;\ 
> >  aodev->callback = device_addrm_aocomplete;  \ 
> >  aodev->force = f;   \ 
> > -libxl__initiate_device_remove(egc, aodev);  \ 
> > +libxl__initiate_device_##remtype##_remove(egc, aodev);  \ 
> >  \ 
> >  out:\ 
> > -if (rc) return AO_CREATE_FAIL(rc); 
> >   
>   \ 
> > +if (rc) return AO_CREATE_FAIL(rc);  \ 
> >  return AO_INPROGRESS;   \ 
> >  } 
> >   
> > +#define DEFINE_DEVICE_REMOVE(type, removedestroy, f) \ 
> > +DEFINE_DEVICE_REMOVE_EXT(type, generic, removedestroy, f) 
> > + 
> > +#define DEFINE_DEVICE_REMOVE_CUSTOM(type, removedestroy, f)  \ 
> > +DEFINE_DEVICE_REMOVE_EXT(type, type, removedestroy, f) 
> > + 
> >  /* Define all remove/destroy functions and undef the macro */ 
> >   
> >  /* disk */ 
>  
>  
> If this is the way to move forward, please split this out into a 
> separate change which can be applied to staging independent of any pvusb 
> changes. 
>  
> I think the patch needs also the #undef. 
>  

Got you. Will update.

>  
> > @@ -4223,6 +4231,10 @@ DEFINE_DEVICE_REMOVE(vfb, destroy, 1) 
> >  DEFINE_DEVICE_REMOVE(vtpm, remove, 0) 
> >  DEFINE_DEVICE_REMOVE(vtpm, destroy, 1) 
> >   
> > +/* usbctrl */ 
> > +DEFINE_DEVICE_REMOVE_CUSTOM(usbctrl, remove, 0) 
> > +DEFINE_DEVICE_REMOVE_CUSTOM(usbctrl, destroy, 1) 
> > + 
> >  /* channel/console hotunplug is not implemented. There are 2  
> possibilities: 
> >   * 1. add support for secondary consoles to xenconsoled 
> >   * 2. dynamically add/remove qemu chardevs via qmp messages. */ 
>  
> A comment should mention both libxl_device_usbctrl_remove/destroy and 
> libxl__initiate_device_usbctrl_remove/destroy. 

Will add comments.

Thanks,
Chunyan

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


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

Re: [Xen-devel] [PATCH V13 3/5] libxl: add pvusb API

[Chun Yan Liu]

>>> On 1/20/2016 at 12:56 PM, in message
<569f83f502660009e...@relay2.provo.novell.com>, "Chun Yan Liu"
 wrote: 

>  
 On 1/19/2016 at 11:48 PM, in message 
> <22174.23240.402164.635...@mariner.uk.xensource.com>, Ian Jackson 
>  wrote:  
> > Chunyan Liu writes ("[PATCH V13 3/5] libxl: add pvusb API"):  
> > > Add pvusb APIs, including:  
> > >  - attach/detach (create/destroy) virtual usb controller.  
> > >  - attach/detach usb device  
> > >  - list usb controller and usb devices  
> > >  - some other helper functions  
> >   
> >   
> > Thanks.  This is making progress but I'm afraid we're not quite there  
> > yet.  
> >   
> >   
> > > +static int usbback_dev_unassign(libxl__gc *gc, const char *busid)  
> > > +{  
> > ...  
> > > +/* Till here, USB device has been unbound from USBBACK and  
> > > + * removed from xenstore, usb list couldn't show it anymore,  
> > > + * so no matter removimg driver path successfully or not,  
> > > + * we will report operation success.  
> > > + */  
> >   
> > I'm still unconvinced by this and this may mean that the code in this  
> > function is in the wrong order.  Earlier we had this exchange:  
> >   
> > > > Ought this function to really report success if these calls fail ?  
> > >   
> > > I think so. Till here, the USB device has already been unbound from  
> > > usbback and removed from xenstore. usb-list cannot list it any more.  
> >   
> > The problem is that I think that if this function fails, it can leave  
> >  - debris in xenstore (the usbback path)  
> Yes, it's true. 
>  
> >  - the interface bound to the wrong driver 
> No, it won't be bound to 'wrong' driver, only maybe not bound to any driver 
> (Already unbound from usbback, but failed to rebound to its original  
> driver). 
> In this case, we would report warning: failed to rebind to driver xxx.  
>  
> > And then there is no way for the user to get libxl to re-attempt the  
> > operation, or clean up.  Am I right ? 
>  
> Yes. No way to re-attempt usbdev-detach or cleanup driver path in 
> xenstore. But won't affect next time usbdev-attach the same device. 
>   
> >   
> > One way to avoid this kind of problem is to deal with the xenstore  
> > path last.  That way the device will still appear as attached to the  
> > domain.  
>  
> I'm afraid if the side effect is acceptable? In my testing, some USB  
> bluetooth 
> device always fails to rebind to 'btusb' driver after it's unbound 
> from 'usbback'. In this case, we can't detach it from the domain then.  

Ian J., any opinion on this? If it's still thought to be better, I'll update 
patch.

>  
> >   
> > To do this properly I think bind_usbintf may need to become idempotent  
> > even in the face of other callers racing to assign the device.  I  
> > think that would mean bind_usbif it would have to know what driver to  
> > expect to find the device bound to. 

bind_usbintf already has parameter to indicate which driver to be bound to.

- Chunyan
> >   
> > George, am I right here ?  
> >   
> >   
> > I have a few other comments too:  
> >   
> > > +/* get original driver path of usb interface, stored in @drvpath */  
> > > +static int usbintf_get_drvpath(libxl__gc *gc, const char *intf, char   
> > **drvpath)  
> > > +{  
> > > +char *spath, *dp = NULL;  
> > > +struct stat st;  
> > > +int rc;  
> > > +  
> > > +spath = GCSPRINTF(SYSFS_USB_DEV "/%s/driver", intf);  
> > > +  
> > > +rc = lstat(spath, );  
> >   
> > This `rc' should be `r'.  (CODING_STYLE.)  
> >   
> > I mentioned this in my review of v12 in the context of  
> > sysfs_write_intf.  But there is still more than one occurrence in v13  
> > of `rc' for a syscall return value.  
> >   
>  
> Sorry, will double check again. 
>  
> >   
> > > +static int sysfs_write_intf(libxl__gc *gc, const char *intf, const char  
> > >  
> > *path)  
> > > +{  
> >   
> > Last time I wrote:  
> >   
> >   But there is nothing usb specific about this function.  Looking for  
> >   similar code elsewhere this function seems very similar to  
> >   libxl_pci.c:sysfs_write_bdf, but I won't ask you to try to refactor  
> >   these two functions together.  
> >   
> > This time I have remembered that libxl_write_exactly exists, and could  
> > be used.  Sorry for not saing this last time but I think you can  
> > probably just get rid of this in favour of libxl_write_exactly.  If  
> > you think not, please say why.  
>  
> After checking the codes, yes, except for open and close fd, 
> libxl_write_exactly does the work. Will reuse it. 
>  
> >   
> >   
> > > +static int bind_usbintf(libxl__gc *gc, const char *intf, const char   
> > *drvpath)  
> > > +{  
> > > +char *path;  
> > > +struct stat st;  
> > > +  
> > > +path = GCSPRINTF("%s/%s", drvpath, intf);  
> > > +/* if already bound, return */  
> > > +if (!lstat(path, ))  
> > > +return 0;  
> > > +else if (errno 

[Xen-devel] [PATCH V13 3/5] libxl: add pvusb API

[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: Chunyan Liu 
Signed-off-by: Simon Cao 
Signed-off-by: George Dunlap 
---
changes:
* address error handlings

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

diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
index 2abae0c..e25ffa6 100644
--- a/tools/libxl/Makefile
+++ b/tools/libxl/Makefile
@@ -104,7 +104,7 @@ LIBXL_OBJS = flexarray.o libxl.o libxl_create.o libxl_dm.o 
libxl_pci.o \
libxl_stream_read.o libxl_stream_write.o \
libxl_save_callout.o _libxl_save_msgs_callout.o \
libxl_qmp.o libxl_event.o libxl_fork.o \
-   libxl_dom_suspend.o $(LIBXL_OBJS-y)
+   libxl_dom_suspend.o libxl_pvusb.o $(LIBXL_OBJS-y)
 LIBXL_OBJS += libxl_genid.o
 LIBXL_OBJS += _libxl_types.o libxl_flask.o _libxl_types_internal.o
 
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 43d5709..920c135 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -3204,7 +3204,7 @@ void libxl__device_disk_local_initiate_detach(libxl__egc 
*egc,
 aodev->dev = device;
 aodev->callback = local_device_detach_cb;
 aodev->force = 0;
-libxl__initiate_device_remove(egc, aodev);
+libxl__initiate_device_generic_remove(egc, aodev);
 return;
 }
 
@@ -4172,8 +4172,10 @@ out:
  * libxl_device_vkb_destroy
  * libxl_device_vfb_remove
  * libxl_device_vfb_destroy
+ * libxl_device_usbctrl_remove
+ * libxl_device_usbctrl_destroy
  */
-#define DEFINE_DEVICE_REMOVE(type, removedestroy, f)\
+#define DEFINE_DEVICE_REMOVE_EXT(type, remtype, removedestroy, f)\
 int libxl_device_##type##_##removedestroy(libxl_ctx *ctx,   \
 uint32_t domid, libxl_device_##type *type,  \
 const libxl_asyncop_how *ao_how)\
@@ -4193,13 +4195,19 @@ out:
 aodev->dev = device;\
 aodev->callback = device_addrm_aocomplete;  \
 aodev->force = f;   \
-libxl__initiate_device_remove(egc, aodev);  \
+libxl__initiate_device_##remtype##_remove(egc, aodev);  \
 \
 out:\
-if (rc) return AO_CREATE_FAIL(rc);\
+if (rc) return AO_CREATE_FAIL(rc);  \
 return AO_INPROGRESS;   \
 }
 
+#define DEFINE_DEVICE_REMOVE(type, removedestroy, f) \
+DEFINE_DEVICE_REMOVE_EXT(type, generic, removedestroy, f)
+
+#define DEFINE_DEVICE_REMOVE_CUSTOM(type, removedestroy, f)  \
+DEFINE_DEVICE_REMOVE_EXT(type, type, removedestroy, f)
+
 /* Define all remove/destroy functions and undef the macro */
 
 /* disk */
@@ -4223,6 +4231,10 @@ DEFINE_DEVICE_REMOVE(vfb, destroy, 1)
 DEFINE_DEVICE_REMOVE(vtpm, remove, 0)
 DEFINE_DEVICE_REMOVE(vtpm, destroy, 1)
 
+/* usbctrl */
+DEFINE_DEVICE_REMOVE_CUSTOM(usbctrl, remove, 0)
+DEFINE_DEVICE_REMOVE_CUSTOM(usbctrl, destroy, 1)
+
 /* channel/console hotunplug is not implemented. There are 2 possibilities:
  * 1. add support for secondary consoles to xenconsoled
  * 2. dynamically add/remove qemu chardevs via qmp messages. */
@@ -4236,6 +4248,8 @@ DEFINE_DEVICE_REMOVE(vtpm, destroy, 1)
  * libxl_device_disk_add
  * libxl_device_nic_add
  * libxl_device_vtpm_add
+ * libxl_device_usbctrl_add
+ * libxl_device_usbdev_add
  */
 
 #define DEFINE_DEVICE_ADD(type) \
@@ -4267,6 +4281,12 @@ DEFINE_DEVICE_ADD(nic)
 /* vtpm */
 DEFINE_DEVICE_ADD(vtpm)
 
+/* usbctrl */
+DEFINE_DEVICE_ADD(usbctrl)
+
+/* usb */
+DEFINE_DEVICE_ADD(usbdev)
+
 #undef DEFINE_DEVICE_ADD
 
 
/**/
@@ -4432,7 +4452,7 @@ static int remove_device(libxl__egc *egc, libxl__ao *ao,
 aodev->dev = dev;
 aodev->action = 

Re: [Xen-devel] [PATCH V13 3/5] libxl: add pvusb API

[Chun Yan Liu]

>>> On 1/19/2016 at 11:48 PM, in message
<22174.23240.402164.635...@mariner.uk.xensource.com>, Ian Jackson
 wrote: 
> Chunyan Liu writes ("[PATCH V13 3/5] libxl: add pvusb API"): 
> > Add pvusb APIs, including: 
> >  - attach/detach (create/destroy) virtual usb controller. 
> >  - attach/detach usb device 
> >  - list usb controller and usb devices 
> >  - some other helper functions 
>  
>  
> Thanks.  This is making progress but I'm afraid we're not quite there 
> yet. 
>  
>  
> > +static int usbback_dev_unassign(libxl__gc *gc, const char *busid) 
> > +{ 
> ... 
> > +/* Till here, USB device has been unbound from USBBACK and 
> > + * removed from xenstore, usb list couldn't show it anymore, 
> > + * so no matter removimg driver path successfully or not, 
> > + * we will report operation success. 
> > + */ 
>  
> I'm still unconvinced by this and this may mean that the code in this 
> function is in the wrong order.  Earlier we had this exchange: 
>  
> > > Ought this function to really report success if these calls fail ? 
> >  
> > I think so. Till here, the USB device has already been unbound from 
> > usbback and removed from xenstore. usb-list cannot list it any more. 
>  
> The problem is that I think that if this function fails, it can leave 
>  - debris in xenstore (the usbback path) 
Yes, it's true.

>  - the interface bound to the wrong driver
No, it won't be bound to 'wrong' driver, only maybe not bound to any driver
(Already unbound from usbback, but failed to rebound to its original driver).
In this case, we would report warning: failed to rebind to driver xxx. 

> And then there is no way for the user to get libxl to re-attempt the 
> operation, or clean up.  Am I right ?

Yes. No way to re-attempt usbdev-detach or cleanup driver path in
xenstore. But won't affect next time usbdev-attach the same device.
 
>  
> One way to avoid this kind of problem is to deal with the xenstore 
> path last.  That way the device will still appear as attached to the 
> domain. 

I'm afraid if the side effect is acceptable. In my testing, some USB bluetooth
device always fails to rebind to 'btusb' driver after it's unbound
from 'usbback'. In this case, we can't detach it from the domain then. 

>  
> To do this properly I think bind_usbintf may need to become idempotent 
> even in the face of other callers racing to assign the device.  I 
> think that would mean bind_usbif it would have to know what driver to 
> expect to find the device bound to. 
>  
> George, am I right here ? 
>  
>  
> I have a few other comments too: 
>  
> > +/* get original driver path of usb interface, stored in @drvpath */ 
> > +static int usbintf_get_drvpath(libxl__gc *gc, const char *intf, char  
> **drvpath) 
> > +{ 
> > +char *spath, *dp = NULL; 
> > +struct stat st; 
> > +int rc; 
> > + 
> > +spath = GCSPRINTF(SYSFS_USB_DEV "/%s/driver", intf); 
> > + 
> > +rc = lstat(spath, ); 
>  
> This `rc' should be `r'.  (CODING_STYLE.) 
>  
> I mentioned this in my review of v12 in the context of 
> sysfs_write_intf.  But there is still more than one occurrence in v13 
> of `rc' for a syscall return value. 
>  

Sorry, will double check again.

>  
> > +static int sysfs_write_intf(libxl__gc *gc, const char *intf, const char  
> *path) 
> > +{ 
>  
> Last time I wrote: 
>  
>   But there is nothing usb specific about this function.  Looking for 
>   similar code elsewhere this function seems very similar to 
>   libxl_pci.c:sysfs_write_bdf, but I won't ask you to try to refactor 
>   these two functions together. 
>  
> This time I have remembered that libxl_write_exactly exists, and could 
> be used.  Sorry for not saing this last time but I think you can 
> probably just get rid of this in favour of libxl_write_exactly.  If 
> you think not, please say why. 

After checking the codes, yes, except for open and close fd,
libxl_write_exactly does the work. Will reuse it.

>  
>  
> > +static int bind_usbintf(libxl__gc *gc, const char *intf, const char  
> *drvpath) 
> > +{ 
> > +char *path; 
> > +struct stat st; 
> > + 
> > +path = GCSPRINTF("%s/%s", drvpath, intf); 
> > +/* if already bound, return */ 
> > +if (!lstat(path, )) 
> > +return 0; 
> > +else if (errno != ENOENT) 
> > +return ERROR_FAIL; 
>  
> This new ERROR_FAIL fails to log anything, and probably should.  I 
> think the anomalous style leads to this mistake.  You should say 
>r = lstat... 
> and adopt the pattern from the rest of libxl. 

Will update.

Thanks,
Chunyan

>  
>  
> Thanks, 
> Ian. 
>  
>  



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

Re: [Xen-devel] [PATCH V13 3/5] libxl: add pvusb API

[Ian Jackson]

Chunyan Liu writes ("[PATCH V13 3/5] libxl: add pvusb API"):
> Add pvusb APIs, including:
>  - attach/detach (create/destroy) virtual usb controller.
>  - attach/detach usb device
>  - list usb controller and usb devices
>  - some other helper functions


Thanks.  This is making progress but I'm afraid we're not quite there
yet.


> +static int usbback_dev_unassign(libxl__gc *gc, const char *busid)
> +{
...
> +/* Till here, USB device has been unbound from USBBACK and
> + * removed from xenstore, usb list couldn't show it anymore,
> + * so no matter removimg driver path successfully or not,
> + * we will report operation success.
> + */

I'm still unconvinced by this and this may mean that the code in this
function is in the wrong order.  Earlier we had this exchange:

> > Ought this function to really report success if these calls fail ?
> 
> I think so. Till here, the USB device has already been unbound from
> usbback and removed from xenstore. usb-list cannot list it any more.

The problem is that I think that if this function fails, it can leave
 - debris in xenstore (the usbback path)
 - the interface bound to the wrong driver
And then there is no way for the user to get libxl to re-attempt the
operation, or clean up.  Am I right ?

One way to avoid this kind of problem is to deal with the xenstore
path last.  That way the device will still appear as attached to the
domain.

To do this properly I think bind_usbintf may need to become idempotent
even in the face of other callers racing to assign the device.  I
think that would mean bind_usbif it would have to know what driver to
expect to find the device bound to.

George, am I right here ?


I have a few other comments too:

> +/* get original driver path of usb interface, stored in @drvpath */
> +static int usbintf_get_drvpath(libxl__gc *gc, const char *intf, char 
> **drvpath)
> +{
> +char *spath, *dp = NULL;
> +struct stat st;
> +int rc;
> +
> +spath = GCSPRINTF(SYSFS_USB_DEV "/%s/driver", intf);
> +
> +rc = lstat(spath, );

This `rc' should be `r'.  (CODING_STYLE.)

I mentioned this in my review of v12 in the context of
sysfs_write_intf.  But there is still more than one occurrence in v13
of `rc' for a syscall return value.


> +static int sysfs_write_intf(libxl__gc *gc, const char *intf, const char 
> *path)
> +{

Last time I wrote:

  But there is nothing usb specific about this function.  Looking for
  similar code elsewhere this function seems very similar to
  libxl_pci.c:sysfs_write_bdf, but I won't ask you to try to refactor
  these two functions together.

This time I have remembered that libxl_write_exactly exists, and could
be used.  Sorry for not saing this last time but I think you can
probably just get rid of this in favour of libxl_write_exactly.  If
you think not, please say why.


> +static int bind_usbintf(libxl__gc *gc, const char *intf, const char *drvpath)
> +{
> +char *path;
> +struct stat st;
> +
> +path = GCSPRINTF("%s/%s", drvpath, intf);
> +/* if already bound, return */
> +if (!lstat(path, ))
> +return 0;
> +else if (errno != ENOENT)
> +return ERROR_FAIL;

This new ERROR_FAIL fails to log anything, and probably should.  I
think the anomalous style leads to this mistake.  You should say
   r = lstat...
and adopt the pattern from the rest of libxl.


Thanks,
Ian.

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