Re: [PATCH RFC] libxl: Introduce functions to add and remove USB devices to an HVM guest

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

On 09/04/13 17:30, Ian Jackson wrote:
> George Dunlap writes ("Re: [Xen-devel] [PATCH RFC] libxl: Introduce functions to add and remove USB devices to an HVM guest"):
>> The reason for specifying STUBDOM I guess is that STUBDOM is really a
>> special case, where it's really both a PVUSB  and a DEVICEMODEL; so in
>> theory you could specify a backend_domid.
> I think in principle you could specify a backend domid for a non-stub
> dm too.

How is that supposed to work?

>> The question, I guess, is whether we should assume that the caller can
>> be trusted to know whether the VM in question is using a stub domain or
>> not.  It's certainly possible to make the interface only specify PVUSB
>> or DEVICEMODEL, and to make it (someday) so that calling DEVICEMODEL for
>> a stubdom will set up PVUSB for the stubdom, then tell the device model
>> to make the device available via emulation all automatically.
> libxl can tell whether the guest is using a stub-dm.

Yes, but the question is about all the extra random plumbing that libxl 
would be doing, and the extra codepaths that will be in use, and whether 
doing all that automatically is really a good interface or not.

For example, most distro kernels (apparently) have buggy PVUSB 
back-ends; possibly stubdoms have buggy PVUSB front-ends.  Now suppose 
that a user is using HVM domains without stubdoms, and is used to 
(without thinking) just adding in USB devices via qemu.  And then 
suppose that he decides he wants security / scalability / whatever, and 
implements stubdoms.  But he doesn't realize the implications; so the 
next time he happens to pass in a device, it suddenly starts using the 
buggy PVUSB path, and hilarity ensues. Perhaps someday we even unify the 
config file and the hot-plug USB stuff, like it's unified for pci 
devices; and the user just forgets that he has devices passed through 
specified, and hilarity ensues without his even taking an active step to 
cause it.

I agree that we should in general honor the principle of "Ask what the 
caller/user is trying to do, not the technical details of how to do 
it".  But I think we should also honor the "Principle of least 
surprise", and I'm afraid that we may not be able to honor both fully in 
this case.  The question is which one can we fudge while causing the 
least damage.

>> But since we're not implementing stubdom at the moment anyway, we can
>> probably just take out STUBDOM entirely and discuss whether to add a new
>> protocol when we actually decide to implement it.
> It affects the documentation now, I thinki.

Can we not just say that only HVM domains without stub domains are 
supported?  And then later either change the documentation to say, "stub 
domains are supported automatically" or to say "stub domains require a 
different protocol" (whichever we end up deciding)?

>
>>> What are the semantics if multiple host devices match *dev ?
>>> How is the id returned ?
>> I think in general if it matches multiple devices then it should fail.
>> That's what qemu will do, so at the moment that is implemented by
>> default for us.
> This should be documented.

Ack.

>
>>> I think dev should probably be
>>>     const libxl_device_usb *dev
>> Sure.
> You didn't ask my question about ids.

I looked at the other interfaces, and they seem to just use a 
re-specification of the same parameters to specify the same device. (IOW 
I took out the whole "device handle" thing.)

One thing we could do is just get rid of the "vendorid:productid" thing 
entirely, at least for now.  hostbus.hostaddr is unique, it's just 
potentially a bit less convenient if the topology tends to change across 
reboots / sessions (e.g., plugging in your device into a different 
port).  That would make it much more like pci devices, which are 
specified uniquely with domain:bus:device.function.

We could then consider adding "vendorid:productid" as a 
properly-supported interface for either PVUSB or DEVICEMODEL at some 
point in the future -- i.e., have libxl look it up, check that it's 
unique, and translate it into hostbus.hostaddr.

>
>>> This is a lot of enum-handling machinery.  Don't we have something
>>> similar already which could be pressed into service ?  If not then
>>> this should be in a separate file.
> ...
>>> Perhaps this should be generated by the idl compiler ?\
>> I'm not going to be able to make that happen by the feature freeze this
>> Friday.
>>
>> Perhaps just writing the raw numeric values for now?
> If it's just needed for xenstore for the toolstack's own consumption
> then that's fine, as when you upgrade the toolstack you need to
> upgrade xen and thus migrate away all your domains.  Alternatively I
> think the IDL compiler produces long names which might be useable too.

OK, I'll take a look.

>
>>> I stopped reading here because of the wrap damage I'm afraid.
>>> (Reproduced it by adding hard CRs where my MUA wraps it, so you can
>>> see what it looks like.)
>> Well the main purpose of this mail was to see if this could be
>> implemented by Friday, or if it would need to wait until 4.4.  We've
>> uncovered a couple of things which if required would mean waiting --
>> what do you think?  I'm fine with waiting -- that's why I sent the
>> e-mail, so I could stop early if there was little chance of success. :-)
> I don't think my questions need to be answered in a way that means we
> have to wait, so long as we have a route to improving things later.
>
> Ian.