|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH V5 3/7] libxl: add pvusb API
On 08/06/2015 04:11 AM, Chun Yan Liu wrote:
> As 4.6 goes to bug fixing stage, maybe we can pick up this thread? :-)
>
> Beside to call for your precious review comments and suggestions so that we
> can
> make progress, I also want to confirm about the previous discussed two TODO
> things:
> 1) use UDEV name rule to specify usb device uniquely even across reboot. That
> got consensus. Next thing is exposing that name to some sysfs entry,
> right?
So just to be clear, my understanding of the plan was that we try to fix
up the current patch series and check it in once the 4.7 window opens;
and that having the utility library to convert other names (including
this udev-style naming) into something libxl can use would be a separate
series.
I wasn't necessarily expecting you to work on it (since it wasn't your
idea), but if you're keen, I'm sure we'd be grateful. :-)
> 2) use libusb instead of reading sysfs by ourselves. As George mentioned,
> using
> libusb is not simpler than reading sysfs; and if UDEV name is stored to
> some
> sysfs entry for us to retrieve, then we still need reading sysfs things.
> Could we
> get to a final decision?
> If these are settled down, I can update related code.
I don't think that libusb would be particularly useful for the current
pvusb code, since
1. It's already Linux-specific,
2. We need to mess around with sysfs anyway.
The same thing can't be said of the HVM path: I think it fairly likely
that the emulated pass-through will Just Work (or nearly so) on BSDs
(assuming that qemu itself works on the BSDs).
I think it we write our utility library for converting
vendorid:productid[:serialno], bus-port, &c, then it might make sense to
use libusb if it makes it more portable.
Regarding the code:
Things are looking pretty close. A couple of comments in-line:
>>>> diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
>>>> index 93bb41e..9869a21 100644
>>>> --- a/tools/libxl/libxl_device.c
>>>> +++ b/tools/libxl/libxl_device.c
>>>> @@ -676,6 +676,10 @@ void libxl__devices_destroy(libxl__egc *egc,
>>>> libxl__devices_remove_state *drs)
>>>> aodev->action = LIBXL__DEVICE_ACTION_REMOVE;
>>>> aodev->dev = dev;
>>>> aodev->force = drs->force;
>>>> + if (dev->backend_kind == LIBXL__DEVICE_KIND_VUSB) {
>>>> + libxl__initiate_device_usbctrl_remove(egc, aodev);
>>>> + continue;
>>>> + }
I take it the reason we need to special-case this is that we need to go
through and un-assign all of the devices inside the controller first?
At some point we probably want to generalize this a bit, so that usb
controllers and vscsi controllers look the same (rather than both being
special-cased).
But since this is internal, maybe we can wait for that design until we
actually have both types available.
>>>> +static int
>>>> +libxl__device_usb_assigned_list(libxl__gc *gc,
>>>> + libxl_device_usb **list, int *num)
>>>> +{
>>>> + char **domlist;
>>>> + unsigned int nd = 0, i, j;
>>>> + char *be_path;
>>>> + libxl_device_usb *usb;
>>>> +
>>>> + *list = NULL;
>>>> + *num = 0;
>>>> +
>>>> + domlist = libxl__xs_directory(gc, XBT_NULL, "/local/domain", &nd);
>>>> + be_path = GCSPRINTF("/local/domain/%d/backend/vusb",
>>>> LIBXL_TOOLSTACK_DOMID);
Hmm, so this had made me realize that I don't think we're doing quite
the right thing for non-dom0 backends.
First of all, things are a bit "interesting" for driver domain backends,
because the "namespace" of hostbus.hostaddr depends on the backend of
the virtual controller. Which wouldn't be particularly interesting,
*except* that because the USB space is so dynamic, you normally have to
query the devices to find the hostbus.hostaddr; and you can't do any
queries from dom0 at the moment (except, for example, by ssh'ing into
the other vm). So to even assign a hostbus.hostaddr device you have to
somehow learn, out-of-band, what those numbers are within the domain.
Secondly, if the backend is in another domain, then all the stuff re
assigning a usb device to usbback can't be done by libxl in the
toolstack domain either. Which means I'm pretty sure this stuff will
fail at the moment for USB driver domains trying to assign a
non-existent hostbus.hostaddr to the (possibly non-existent) dom0 usbback.
A couple of ways forward:
1. Delete backend_domid and backend_name; add them back later when we
decide to implement proper USB driver domain support
2. Leave backend_domid and backend_name, but return an error if they are
not 0 and NULL respectively.
3. If backend_domid != TOOLSTACK_DOMID, then do the xenstore stuff in
libxl, but assume that someone else has figured out what the proper
hostbus.hostaddr are for the backend domain, and also assigned that
device to the usbback inside that domain.
4. Invent some protocol for talking to the backend, allowing you to
query assignable devices and assign devices to usbback in that domain
from the toolstack domain.
#4 is probably more than we want to do at this point; and #1 I think
will cause us some hassle down the road if we ever *do* want to
implement usb driver domains.
I would personally probably go for #2, unless we have some intention of
at least smoke-testing #3; but I can certainly see the advantages of
implementing #3 even if we don't end up testing it.
>>>> +int libxl_device_usb_getinfo(libxl_ctx *ctx, libxl_device_usb *usb,
>>>> + libxl_usbinfo *usbinfo)
>>>> +{
>>>> + GC_INIT(ctx);
>>>> + char *filename;
>>>> + char *busid;
>>>> + void *buf = NULL;
>>>> + int buflen, rc;
>>>> +
>>>> + usbinfo->ctrl = usb->ctrl;
>>>> + usbinfo->port = usb->port;
>>>> +
>>>> + busid = usb_busaddr_to_busid(gc, usb->hostbus, usb->hostaddr);
If we're going to use "<ctrl,port>" for the "handle" here, then those
are the only fields we're allowed to *read* from the usb struct. You
should do something like you do in ctrlport_to_device_usb() to convert
<ctrl,port> to busid. (And I suppose then have hostbus and hostaddr in
the usbinfo struct as well.)
>>>> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
>>>> index 23f27d4..5278d03 100644
>>>> --- a/tools/libxl/libxl_types.idl
>>>> +++ b/tools/libxl/libxl_types.idl
>>>> @@ -541,6 +541,28 @@ libxl_device_pci = Struct("device_pci", [
>>>> ("seize", bool),
>>>> ])
>>>>
>>>> +libxl_usb_protocol = Enumeration("usb_protocol", [
>>>> + (0, "AUTO"),
>>>> + (1, "PV"),
>>>> + (2, "QEMU"),
>>>> + ])
>>>> +
>>>> +libxl_device_usbctrl = Struct("device_usbctrl", [
>>>> + ("protocol", libxl_usb_protocol),
Still chewing on the name here. Is "datapath" more descriptive?
Oh, wait -- actually, now that we have the usbctrl/usb dichotomy, we
could in theory use "type" here to be "PV / IOEMU" (or perhaps
"usbctrltype", as in libxl_device_nic), and then use "devtype" in
libxl_device_usb to specify "hostdev", "tablet", &c.
>>>> + ("devid", libxl_devid),
>>>> + ("version", integer),
>>>> + ("ports", integer),
>>>> + ("backend_domid", libxl_domid),
>>>> + ("backend_domname", string),
>>>> + ])
>>>> +
>>>> +libxl_device_usb = Struct("device_usb", [
>>>> + ("ctrl", libxl_devid),
>>>> + ("port", integer),
>>>> + ("hostbus", integer),
>>>> + ("hostaddr", integer),
>>>> + ])
I think we do want to plan for the future here by doing something like this:
libxl_device_usb = Struct("device_usb", [
("ctrl", libxl_devid),
("port", integer),
("u", KeyedUnion(None, libxl_device_usb_type, "devtype",
[("hostdev", Struct(None, [
("hostbus", integer),
("hostaddr", integer) ]))
]))
])
That way we can add in more device types supported by qemu later. (And
with juergen adding pvusb support to qemu, they might even be
appropriate for more than just the IOEMU path. :-)
Thanks Chunyan, and sorry for the delay!
-George
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |