|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH V5 3/7] libxl: add pvusb API
>>> On 8/7/2015 at 01:21 AM, in message <55C39796.8000500@xxxxxxxxxx>, George
Dunlap <george.dunlap@xxxxxxxxxx> wrote:
> 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.
For non-dom0 backends, here 'be_path' should be replaced with the right backend.
>
> 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.
I think you are right. As for the network driver domain, when specifying vif,
we also need to know the bridge name in that driver domain, then we can use:
vif = [ 'bridge=xenbr0, mac=00:16:3E:0d:13:00, model=e1000, backend=domnet' ]
>
> 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.
Yes. I think so. Should be done in driver domain then.
> 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.
Maybe I'm wrong but as I understand about driver domain (refer to network
driver domain),
all usb devices should be managed by usb driver domain, so with driver domain,
here
the hostbus.hostaddr should be the number within driver domain, and it should
assign
the device to driver domain's usbback, rather than Dom0's. This part of work
should
be done in driver domain. Code structure is quite different.
From this prespective, maybe we can add a check: if backend != Dom0, bypass the
unbind/bind work, only do xenstore stuff, leave unbind/bind work to the specific
backend to do. And currently in the entry we can return error if backend !=
Dom0,
as you suggested in following #2.
How do you think?
- Chunyan
>
> 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 |