Re: [PATCH V3 4/6] xl: add pvusb commands

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

On 05/21/2015 04:35 AM, Juergen Gross wrote:
> On 05/20/2015 05:25 PM, George Dunlap wrote:
>> On 05/20/2015 03:55 PM, Juergen Gross wrote:
>>> On 05/20/2015 04:41 PM, George Dunlap wrote:
>>>> On Wed, May 20, 2015 at 3:33 PM, Juergen Gross <jgross@suse.com> wrote:
>>>>> On 05/20/2015 04:20 PM, George Dunlap wrote:
>>>>>>
>>>>>> On Mon, Apr 20, 2015 at 9:12 AM, Juergen Gross <jgross@suse.com>
>>>>>> wrote:
>>>>>>>
>>>>>>> On 04/19/2015 05:50 AM, Chunyan Liu wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> Add pvusb commands: usb-ctrl-attach, usb-ctrl-detach, usb-list,
>>>>>>>> usb-attach and usb-detach.
>>>>>>>>
>>>>>>>> To attach a usb device to guest through pvusb, one could follow
>>>>>>>> following example:
>>>>>>>>
>>>>>>>>      #xl usb-ctrl-attach test_vm version=1 num_ports=8
>>>>>>>>
>>>>>>>>      #xl usb-list test_vm
>>>>>>>>      will show the usb controllers and port usage under the domain.
>>>>>>>>
>>>>>>>>      #xl usb-attach test_vm 1.6
>>>>>>>>      will find the first usable controller:port, and attach usb
>>>>>>>>      device whose bus address is 1.6 (busnum is 1, devnum is 6)
>>>>>>>>      to it. One could also specify which <controller> and which
>>>>>>>> <port>.
>>>>>>>>
>>>>>>>>      #xl usb-detach test_vm 1.6
>>>>>>>>
>>>>>>>>      #xl usb-ctrl-detach test_vm dev_id
>>>>>>>>      will destroy the controller with specified dev_id. Dev_id
>>>>>>>>      can be traced in usb-list info.
>>>>>>>>
>>>>>>>> Signed-off-by: Chunyan Liu <cyliu@suse.com>
>>>>>>>> Signed-off-by: Simon Cao <caobosimon@gmail.com>
>>>>>>>> ---
>>>>>>>> Changes to v2:
>>>>>>>>       * use bus.addr as user interface instead of busid in
>>>>>>>> usb-attach|detach
>>>>>>>>       * remove usb-assignable-list interface
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Why? While lsusb in combination with xl usb-list for each domain
>>>>>>> will
>>>>>>> give the same information, having to iterate through all domains
>>>>>>> can be
>>>>>>> quite annoying.
>>>>>>>
>>>>>>> An alternative would be to accept omitting the domain for xl
>>>>>>> usb-list
>>>>>>> and list all domains with assigned usb devices in this case.
>>>>>>
>>>>>>
>>>>>> I don't understand what information it is that you want.  Do you want
>>>>>> a list of devices *not already assigned* to domains?
>>>>>
>>>>>
>>>>> Yes.
>>>>
>>>> ...and why do you need that, instead of just remembering what you'd
>>>> assigned to whom?
>>>>
>>>> We don't really have the equivalent for pci either.  That is, if a
>>>> device shows up in "lspci" but not in "pci-assignable-list", that may
>>>> be either because 1) I hasn't yet been assigned to pciback (and this
>>>> is available to be assigned to a domain), or 2) because it's already
>>>> been assigned to a domain.  Someone new coming to the system would
>>>> need to check all VMs to see which devices hadn't yet been assigned.
>>>
>>> So this is a problem of pci-assignable-list, which isn't present for
>>> USB devices. Any USB device not already assigned to a VM would be listed
>>> as before with "xm usb-assignable-list".
>>>
>>> Additionally all systems support hotplug of USB devices - the list of
>>> available USB-devices can change rather often. If you unplug e.g. a
>>> memory stick which has been assigned to a VM and stick it in again it
>>> might show up under a different address and has to be reassigned.
>>> Remembering having it already assigned won't help in this case as much
>>> as a simple command to list the devices ready for assignment.
>>
>> OK. :-)  Yes, I can see that having USB devices disappear and appear as
>> a different bus:host would make such a command particularly useful.
>>
>> But then we have the problem that "assignable" means something different
>> in each case.  In pci-assignable-list, it means "Devices which have been
>> assigned to pciback but not yet been attached to a domain".  In your
>> suggested command, it means "Devices which have not yet been assigned
>> either to pvusbback or to a domain."
> 
> For USB devices there isn't such an action as "assign it to pvusbback".
> You just assign a USB device to a domain. One command, no steps to
> prepare that action (besides the possibility to define a USB controller
> first).

You can do the same thing (now) for pci devices, by setting "seize=1" in
the device spec (which will cause xl to first assign them to pciback,
then to the guest, just as the usb code does now).

> 
>> When I introduced pci-assignable-* I didn't realize that "assignable"
>> was already in one of the xm commands with a different meaning.  I
>> introduced it because until that point, neither xm nor xl had a way of
>> attaching a device to pciback.
>>
>> So we basically have three options:
>> 1. Keep both names the same
>> 2. Rename usb-assignable-list to something else (usb-available-list?)
>> 3. Rename pci-assignable-* to something else
>>
>> #1 would be the most backwards-compatible.  But I think it's really bad
>> going forward, since you have two commands that look like they should do
>> similar things that don't.
> 
> Actually they do. :-)

Actually, they don't. :-)

>From your description, "xm usb-assignable-list" would list *all* USB
devices on the system which were available to be assigned to a guest.

"xl pci-assignable-list" does *not* list all PCI devices on a system
which are available to be assigned.  It lists the ones which can be
assigned without the "seize=1" parameter -- the ones you've already done
something with.  It won't tell you about the other devices on the system
which have not yet been assigned to pciback.

Yes, from a pedantic perspective, both will tell you on which devices
you can run "X-attach" without any extra arguments.  But from a
practical perspective, "xm usb-assignable-list" tells you something
practical about the state of devices on the whole system; and "xl
pci-assignable-list" tells you a technical quirk about devices are in a
half-way state between not being assigned and actually being assigned.

(I'm about 45% of the opinion that we should make pci behave like the
proposed USB patches -- i.e., make seize=1 the default, and just
deprecate the whole "pci-assignable" state altogether.  My only
hesitation is that it removes one level of safety check against doing
something like unplugging the host's main disk controller or something.)

 -George