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

[George Dunlap <george.dunlap@citrix.com>]

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

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

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