Re: [Xen-devel] [PATCH V13 3/5] libxl: add pvusb API
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
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 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
Re: [Xen-devel] [PATCH V13 3/5] libxl: add pvusb API
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 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 usbbac
Re: [Xen-devel] [PATCH V13 3/5] libxl: add pvusb API
>>> On 2/3/2016 at 10:33 PM, in message <56b20fcc.3010...@citrix.com>, George Dunlap 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 mac
Re: [Xen-devel] [PATCH V13 3/5] libxl: add pvusb API
>>> On 2/3/2016 at 10:38 PM, in message <56b210fa.7040...@citrix.com>, George Dunlap 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 > > 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
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 > 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
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: [Xen-devel] [PATCH V13 3/5] libxl: add pvusb API
>>> On 2/3/2016 at 12:48 AM, in message , George Dunlap wrote: > 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 &c can at least know something out of the ordinary has > happened, even if it doesn't go back to the state we started in when > the function was called. That makes sense. But do we need to differentiate failure at removing xenstore information step and error in bind/unbind step? Or, treat them as same? That affects doing it within other process (like usbctrl-detach). For the former, xl usb-list can list usb device and one can do usbdev-detach again; and if in a usbctrl-detach process, will stop the process. For the later, one cannot do usbdev-detach, and if in usbctrl-detach process, in current codes, will ignore that. Chunyan > > -George > > ___ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel > > ___ 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
>>> On 2/3/2016 at 02:11 AM, in message <22192.61775.427189.268...@mariner.uk.xensource.com>, 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. Indeed "unassign from usbback, but listed under pvusb xenstore" is a confusing state. usb-list can list it but guest can not see it. What user can do under that state is: reattempt usbdev_remove, if it succeeds, everything is cleaned up, that's the best result; but possibly it still fails (for example, in my testing, always cannot rebind to original driver), in this case, the confusing state will be lasting, and the device could not be removed, this is worse. > > But, surely usbback needs to cope with the notion that the device > might disappear. USB devices can disappear at any time. > > > It's not clear to me under what conditions 3->2 might fail, > > As an example, someone might press ^C on `xl usb-detach'. > > > or what could be done about it if it did fail. > > The most obvious reason for it failing is that something somewhere > still held onto the device. (For umounting filesystems, and detaching > block devices, this happens a lot.) So if the detach from usbback > fails would probably be possible to simply retry it. > > > Regarding 2->1, again it's not clear that there's anything libxl can > > do. Reloading the driver's module might reset it enough to pick up > > the (now unassigned) USB devices again; other than that, rebooting is > > probably the best option. > > I think re-attempting the bind may work. USB devices can be flaky. > > > It's also not clear to me, if some functions succeed in being > > reassigned and others fail, whether it's best to just try to assign as > > many as we can, or better to go back and un-assign all the ones that > > succeeded. > > Unless explicitly requested, I don't think the system should create > situations some interfaces are assigned to host drivers and some to > usbback. > > And, I'm a fan of `crash-only software': in general, if a failure > occurs, the situation should just be left as-is. The intermediate > state needs to be visible and rectifiable. > > This approach to software state machines avoids bugs where the system > gets into `impossible' states, recovery from which is impossible > because the designers didn't anticipate them. > > It would be tolerable if the recovery sometimes involves `lsusb' and > echoing things into sysfs, but it would be better if not. > > > Perhaps the best approach code-wise is to change the "goto out" on > > failure of unbind_usbintf() into a "continue". That way: > > > > 1. All interfaces which can be re-assigned are re-assigned (and work > > as much as possible) > > > > 2. All interfaces which can be unbound but not re-assigned are at > > least unbound (so that reloading the original driver might pick them > > up) > > I certainly don't mind the software trying to do as much of its task > as possible. Could I understand that this way is acceptable? That means: removing xenstore, and as much as we could (on failure of "unbind from usbback" or "bind to original driver", don't "goto out", just "continue"). Chunyan > > Ian. > > ___ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel > > ___ 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 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
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 &c can at least know something out of the ordinary has happened, even if it doesn't go back to the state we started in when the function was called. -George ___ 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
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 usbback and some potentially al
Re: [Xen-devel] [PATCH V13 3/5] libxl: add pvusb API
>>> 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
>>> 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
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
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
>>> 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, &st); > > > > This `rc' should be `r'. (CODING_STYLE.) > > > > I mentioned this in my review of v12 in the context of > > sysfs_write_intf. But there is still more than one occurrence in v13 > > of `rc' for a syscall return value. > > > > Sorry, will double check again. > > > > > > +static int sysfs_write_intf(libxl__gc *gc, const char *intf, const char > > > > > *path) > > > +{ > > > > Last time I wrote: > > > > But there is nothing usb specific about this function. Looking for > > similar code elsewhere this function seems very similar to > > libxl_pci.c:sysfs_write_bdf, but I won't ask you to try to refactor > > these two functions together. > > > > This time I have remembered that libxl_write_exactly exists, and could > > be used. Sorry for not saing this last time but I think you can > > probably just get rid of this in favour of libxl_write_exactly. If > > you think not, please say why. > > After checking the codes, yes, except for open and close fd, > libxl_write_exactly does the work. Will reuse it. > > > > > > > > +static int bind_usbintf(libxl__gc *gc, const char *intf, const char > > *drvpath) > > > +{ > > > +char *path; > > > +struct stat st; > > > + > > > +path = GCSPRINTF("%s/%s", drvpath, intf); > > > +/* if already bound, return */ > > > +if (!lstat(path, &st)) > > > +return 0; > > > +else if (errno != ENOENT) > > > +return ERR
Re: [Xen-devel] [PATCH V13 3/5] libxl: add pvusb API
>>> 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, &st); > > This `rc' should be `r'. (CODING_STYLE.) > > I mentioned this in my review of v12 in the context of > sysfs_write_intf. But there is still more than one occurrence in v13 > of `rc' for a syscall return value. > Sorry, will double check again. > > > +static int sysfs_write_intf(libxl__gc *gc, const char *intf, const char > *path) > > +{ > > Last time I wrote: > > But there is nothing usb specific about this function. Looking for > similar code elsewhere this function seems very similar to > libxl_pci.c:sysfs_write_bdf, but I won't ask you to try to refactor > these two functions together. > > This time I have remembered that libxl_write_exactly exists, and could > be used. Sorry for not saing this last time but I think you can > probably just get rid of this in favour of libxl_write_exactly. If > you think not, please say why. After checking the codes, yes, except for open and close fd, libxl_write_exactly does the work. Will reuse it. > > > > +static int bind_usbintf(libxl__gc *gc, const char *intf, const char > *drvpath) > > +{ > > +char *path; > > +struct stat st; > > + > > +path = GCSPRINTF("%s/%s", drvpath, intf); > > +/* if already bound, return */ > > +if (!lstat(path, &st)) > > +return 0; > > +else if (errno != ENOENT) > > +return ERROR_FAIL; > > This new ERROR_FAIL fails to log anything, and probably should. I > think the anomalous style leads to this mistake. You should say >r = lstat... > and adopt the pattern from the rest of libxl. Will update. Thanks, Chunyan > > > Thanks, > Ian. > > ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH V13 3/5] libxl: add pvusb API
Chunyan Liu writes ("[PATCH V13 3/5] libxl: add pvusb API"): > Add pvusb APIs, including: > - attach/detach (create/destroy) virtual usb controller. > - attach/detach usb device > - list usb controller and usb devices > - some other helper functions Thanks. This is making progress but I'm afraid we're not quite there yet. > +static int usbback_dev_unassign(libxl__gc *gc, const char *busid) > +{ ... > +/* Till here, USB device has been unbound from USBBACK and > + * removed from xenstore, usb list couldn't show it anymore, > + * so no matter removimg driver path successfully or not, > + * we will report operation success. > + */ I'm still unconvinced by this and this may mean that the code in this function is in the wrong order. Earlier we had this exchange: > > Ought this function to really report success if these calls fail ? > > I think so. Till here, the USB device has already been unbound from > usbback and removed from xenstore. usb-list cannot list it any more. The problem is that I think that if this function fails, it can leave - debris in xenstore (the usbback path) - the interface bound to the wrong driver And then there is no way for the user to get libxl to re-attempt the operation, or clean up. Am I right ? One way to avoid this kind of problem is to deal with the xenstore path last. That way the device will still appear as attached to the domain. To do this properly I think bind_usbintf may need to become idempotent even in the face of other callers racing to assign the device. I think that would mean bind_usbif it would have to know what driver to expect to find the device bound to. George, am I right here ? I have a few other comments too: > +/* get original driver path of usb interface, stored in @drvpath */ > +static int usbintf_get_drvpath(libxl__gc *gc, const char *intf, char > **drvpath) > +{ > +char *spath, *dp = NULL; > +struct stat st; > +int rc; > + > +spath = GCSPRINTF(SYSFS_USB_DEV "/%s/driver", intf); > + > +rc = lstat(spath, &st); This `rc' should be `r'. (CODING_STYLE.) I mentioned this in my review of v12 in the context of sysfs_write_intf. But there is still more than one occurrence in v13 of `rc' for a syscall return value. > +static int sysfs_write_intf(libxl__gc *gc, const char *intf, const char > *path) > +{ Last time I wrote: But there is nothing usb specific about this function. Looking for similar code elsewhere this function seems very similar to libxl_pci.c:sysfs_write_bdf, but I won't ask you to try to refactor these two functions together. This time I have remembered that libxl_write_exactly exists, and could be used. Sorry for not saing this last time but I think you can probably just get rid of this in favour of libxl_write_exactly. If you think not, please say why. > +static int bind_usbintf(libxl__gc *gc, const char *intf, const char *drvpath) > +{ > +char *path; > +struct stat st; > + > +path = GCSPRINTF("%s/%s", drvpath, intf); > +/* if already bound, return */ > +if (!lstat(path, &st)) > +return 0; > +else if (errno != ENOENT) > +return ERROR_FAIL; This new ERROR_FAIL fails to log anything, and probably should. I think the anomalous style leads to this mistake. You should say r = lstat... and adopt the pattern from the rest of libxl. Thanks, Ian. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
[Xen-devel] [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 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 = LIBXL__DEVICE_ACTION_REMOVE; aodev->callback = device_comp