[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

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.


I think dev should probably be
    const libxl_device_usb *dev
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

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.


Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.