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

["Chun Yan Liu" <cyliu@xxxxxxxx>]

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

I'll update codes to this order and post.

-Chunyan

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


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