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

Re: [Xen-devel] [xen-devel][PATCH RFC] libxl: New xl interfaces for managing USB devices

On 07/03/2014 01:56 PM, Simon Cao wrote:

 docs/man/xl.pod.1      |  39 +++++++++++++++++++++++++++++++++++++++
 tools/libxl/libxl.h     |  26 ++++++++++++++++++++++++++
 tools/libxl/libxl_types.idl |  27 +++++++++++++++++++++++++++
 tools/libxl/xl_cmdtable.c  |  33 +++++++++++++++++++++++++++++++++
 4 files changed, 125 insertions(+)

diff --git a/docs/man/xl.pod.1 b/docs/man/xl.pod.1
index 30bd4bf..db76ce2 100644
--- a/docs/man/xl.pod.1
+++ b/docs/man/xl.pod.1
@@ -1194,6 +1194,45 @@ List virtual network interfaces for a domain.
 +=head2 USB DEVICES
+=over 4
+=item B<usb-assginable-list>

Nit: typo.

I'm not really sure we need this one. ÂThe only other time we have this is for PCI devices; but that's because PCI devices have an extra step: they must first be seized by pciback, and then assigned to the domain.

There is no equivalent (AFAIK) for USB devices to this; so this would just be duplicating the functionality of lsusb. ÂThere's no "pci-list-seizable-devices" to show what devices can be passed to "pci-assignable-add"; you just have to use lspci. ÂWe should follow suit here.

I list this command here because it exists in the previous xm/xend commands. Since the devices listed by "lsusb" isn't all assignable, such as the USB hubs, "usb-assginable-list" will list all the assignable devices in Dom0 that can be attached to guest VM in a manner that is suitable to usb-attach.

OK -- well I think that's a bit incidental to the core functionality. I would put off working on this until the core functionality is working; and then I'd put it in its own patch, so we can consider whether to include it separately.

+=item B<usb-controller-create> I<usb-version> I<number_of_ports> [I<controller_type>]
+Creates a new usb virtual controller for the domain specified by I<domain-id>.
+I<usb-version> describes the version of USB controller to create, i.e., 1, 2 or 3,
+I<number_of_ports> describers the number of ports in the USB virtual controller, and
+I<controller_type> describes the type of USB controller, PVUSB or EMULATED.
+=item B<usb-controller-destroy> I<domain-id> I<controller-name>
+Destroies the virtual USB controller specified by I<controller-name> in I<domain-id>.

In keeping with the other device commands, these should be "usb-controller-attach" and "usb-controller-detach". ÂOther than that, looks good.

Since the controller is created from NULL, not just attaching a device from Dom0 to DomU, maybe "create" and "destroy" are better ?

Yeah, I understand the logic. But that's how we name network devices, for instance, which are created from nothing before being attached. In any case, you're still combining the "create" and the "attach" operation into one. Having "attach" mean "create and attach" isn't really that different from having "create" mean "create and attach".Â

Anyway, I'm pretty sure the maintainers will insist on attach / detach before accepting it (they asked me to change it when I posted it a year ago), so you might as well just do it now. :-)

+=item B<usb-attach> I<domain-id> I<hostbus.hostaddr> [I<controller-name>]
+Attaches USB device specified by I<hostbus.hostaddr> to USB controller specified by
+I<controller-name> in domain I<domain-id>.
+=item B<usb-detach> I<domain-id> ÂI<hostbus.hostaddr>
+Detaches the USB device I<hostbus.hostaddr> in domain I<domain-id>.

We want to use the same interface to be able to add other kinds of devices (tablets, mice, keyboards, maybe mass storage) in the future. I think for now this should be something like "hostdev:<hostbus.hostaddr>".

Other than that, looks good.Â
In this case, to specify a hostdev device we can use "hostdev = <hostbus.hostaddr>".

OK -- well I'm sure there will be comments on the exact syntax before the patches are finally accepted. :-)

+libxl_device_usb = Struct("device_usb", [
+ Â Â Â ("controller_name", string),
+ Â Â Â ("type", libxl_usb_type),
+ Â Â Â ("bus", uint8),
+ Â Â Â ("dev", uint8),
+ Â Â Â ])

Is there a reason you got rid of the KeyedUnion construct?

I don't know the benefit of using KeyedUnion construct. Please tell me the benefit of it.

So this idl code:

libxl_device_usb = Struct("device_usb", [
ÂÂÂÂÂÂÂ ("protocol", libxl_device_usb_protocol,
ÂÂÂÂÂÂÂ ("backend_domid",ÂÂÂ libxl_domid),
 ("backend_domname", string),
ÂÂÂÂÂÂÂ ("u", KeyedUnion(None, libxl_device_usb_type, "type",
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ [("hostdev", Struct(None, [
 ("hostaddr", integer) ]))
ÂÂÂ ])

Generated this header code:

typedef struct libxl_device_usb {
ÂÂÂ libxl_usb_protocol protocol;
ÂÂÂ libxl_domid backend_domid;
ÂÂÂ char * backend_domname;
ÂÂÂ libxl_device_usb_type type;
ÂÂÂ union {
ÂÂÂÂÂÂÂ struct {
ÂÂÂÂÂÂÂÂÂÂÂ int hostbus;
ÂÂÂÂÂÂÂÂÂÂÂ int hostaddr;
ÂÂÂÂÂÂÂ } hostdev;
ÂÂÂ } u;
} libxl_device_usb;
void libxl_device_usb_dispose(libxl_device_usb *p);
void libxl_device_usb_init(libxl_device_usb *p);
void libxl_device_usb_init_type(libxl_device_usb *p, libxl_device_usb_type type);
char *libxl_device_usb_to_json(libxl_ctx *ctx, libxl_device_usb *p);

So first of all it makes a union; so that if we eventually have, say, a mass storage type, you won't just have to toss all the possible different parameters into the struct together; it will only be as long as the longest type. Secondly, it tells the idl compiler that the value in "type" tells you what kind of union you're dealing with, so that the "usb_to_json" functions get build properly. For example, the above idl code generates the following code snippet in _libxl_types.c:libxl_device_usb_gen_json():

ÂÂÂ switch (p->type) {
ÂÂÂÂÂÂÂ s = yajl_gen_map_open(hand);
ÂÂÂÂÂÂÂ if (s != yajl_gen_status_ok)
ÂÂÂÂÂÂÂ s = yajl_gen_string(hand, (const unsigned char *)"hostbus", sizeof("hostbus")-1);
ÂÂÂÂÂÂÂ if (s != yajl_gen_status_ok)
ÂÂÂÂÂÂÂ s = yajl_gen_integer(hand, p->u.hostdev.hostbus);
ÂÂÂÂÂÂÂ if (s != yajl_gen_status_ok)
ÂÂÂÂÂÂÂ s = yajl_gen_string(hand, (const unsigned char *)"hostaddr", sizeof("hostaddr")-1);
ÂÂÂÂÂÂÂ if (s != yajl_gen_status_ok)
ÂÂÂÂÂÂÂ s = yajl_gen_integer(hand, p->u.hostdev.hostaddr);
ÂÂÂÂÂÂÂ if (s != yajl_gen_status_ok)
ÂÂÂÂÂÂÂ s = yajl_gen_map_close(hand);
ÂÂÂÂÂÂÂ if (s != yajl_gen_status_ok)
ÂÂÂÂÂÂÂ break;

So it does specific things based on what "type" is set to: if it's set to "hostdev" it outputs hostbus and hostaddr; if we added a mass storage type, and type was set to "storage", it would only output the information relevant to the mass storage device.

Cool, look forward to the code. :-)

Xen-devel mailing list



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