[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH v6 1/2] libxl: Introduce functions to add and remove USB devices to an HVM guest
On 24/04/13 13:38, Ian Campbell wrote:
I didn't see an implementation of libxl__device_usb_setdefault?
You mean, for the internal structure?
On Fri, 2013-04-19 at 16:59 +0100, George Dunlap wrote:
xs_write(ctx->xsh, t, libxl__sprintf(gc, "%s/vm", dom_path), vm_path,
strlen(vm_path));
rc = libxl__domain_rename(gc, *domid, 0, info->name, t);
if (rc)
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 3ba3a21..d2e00fa 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1412,6 +1412,24 @@ _hidden int libxl__qmp_save(libxl__gc *gc, int domid,
const char *filename);
/* Set dirty bitmap logging status */
_hidden int libxl__qmp_set_global_dirty_log(libxl__gc *gc, int domid, bool
enable);
_hidden int libxl__qmp_insert_cdrom(libxl__gc *gc, int domid, const
libxl_device_disk *disk);
+/* Same as normal, but "translated" */
You can use the IDL to do internal stuff too -- see
tools/libxl/libxl_types_internal.idl
What does "translated" mean? Which fields and how? Might it be better to
include a pointer to the original libxl_device_usb instead of
duplicating some/all of these fields?
I think one of the biggest thing was avoiding having to look up the
stubdomain in a bunch of different functions -- that's the dm_domid.
There's also the translation of "AUTO" protocol into PV or HVM, and
(originally) the translation of BACKEND_DEFAULT into the actual backend
domain.
If we were willing to have the library change the protocol in the
structure passed to it, then a pointer might work as well.
+typedef struct libxl__device_usb {
+ libxl_usb_protocol protocol;
+ libxl_domid target_domid;
+ libxl_domid backend_domid;
+ libxl_domid dm_domid;
The only use of this I see is:
+ usbdev->dm_domid = libxl_get_stubdom_id(ctx, domid);
...
+ if (usbdev->dm_domid != 0) {
twice, both times all in the same function so you could just use a local
variable.
A big part of that is because we don't support stubdomains yet. When we
do, there will be more complicated cases to consider.
If you remove this and pass target_domid as a parameter to various
functions (which is the convention in libxl) then the need for this
weird internal/external representation goes away.
There's also the resolution of protocol from ANY into PV or HVM; but if
we're willing to change the structure passed in by the caller, then yes,
we can get rid of this struct for now and consider re-introducing it
if/when we actually need it.
+ libxl_device_usb_type type;
+ union {
+ struct {
+ int hostbus;
+ int hostaddr;
+ } hostdev;
+ } u;
+} libxl__device_usb;
+_hidden int libxl__qmp_usb_add(libxl__gc *gc, int domid,
+ libxl__device_usb *dev);
+_hidden int libxl__qmp_usb_remove(libxl__gc *gc, int domid,
+ libxl__device_usb *dev);
/* close and free the QMP handler */
_hidden void libxl__qmp_close(libxl__qmp_handler *qmp);
/* remove the socket file, if the file has already been removed,
diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
index 644d2c0..9ad3e59 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
I'd like Anthony's feedback on the QMP bits (CCd)
@@ -42,6 +42,7 @@
#define QMP_RECEIVE_BUFFER_SIZE 4096
#define PCI_PT_QDEV_ID "pci-pt-%02x_%02x.%01x"
+#define HOST_USB_QDEV_ID "usb-hostdev-%04x.%04x"
typedef int (*qmp_callback_t)(libxl__qmp_handler *qmp,
const libxl__json_object *tree,
@@ -929,6 +930,70 @@ int libxl__qmp_insert_cdrom(libxl__gc *gc, int domid,
}
}
+static int libxl__qmp_usb_hostdev_add(libxl__gc *gc, int domid,
+ libxl__device_usb *dev)
+{
+ libxl__json_object *args = NULL;
+ char *id;
+
+ id = GCSPRINTF(HOST_USB_QDEV_ID,
+ (uint16_t)dev->u.hostdev.hostbus,
+ (uint16_t)dev->u.hostdev.hostaddr);
You could make these uint16 in the IDL if you wanted to restrict it like
that. Even without that is the cast really necessary though, given the
%x format string?
If you do use uint16_t then you probably want to use PRIx16 too.
This actually a vestigial artifact from the fact that you could
originally specify ANY in the field, which was encoded as -1. I'll just
take out now.
+
+ qmp_parameters_add_string(gc, &args, "driver", "usb-host");
+ QMP_PARAMETERS_SPRINTF(&args, "hostbus", "0x%x", dev->u.hostdev.hostbus);
+ QMP_PARAMETERS_SPRINTF(&args, "hostaddr", "0x%x", dev->u.hostdev.hostaddr);
+
+ qmp_parameters_add_string(gc, &args, "id", id);
+
+ return qmp_run_command(gc, domid, "device_add", args, NULL, NULL);
+}
+
+int libxl__qmp_usb_add(libxl__gc *gc, int domid, libxl__device_usb *usbdev)
+{
+ int rc;
+ switch (usbdev->type) {
+ case LIBXL_DEVICE_USB_TYPE_HOSTDEV:
Will this function ever handle anything other than HOSTDEV? AIUI the
only other potential type is PV which won't come anywhere near here?
You're mixing up protocol and device type. PV will only ever handle
HOSTDEV types, but qmp allows a wide range of virtual devices: tablets,
mice, keyboards, thumb drives, &c. I'd like at some point for all USB
devices specified in the config file to be able to be listed and
hot-plugged / hot-unplugged.
(same comments on the remove bits)
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index fcb1ecd..3c6a709 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -408,6 +408,28 @@ libxl_device_vtpm = Struct("device_vtpm", [
("uuid", libxl_uuid),
])
+libxl_device_usb_protocol = Enumeration("usb_protocol", [
+ (0, "AUTO"),
+ (1, "PV"),
+ (2, "DEVICEMODEL"),
+ ])
+
+libxl_device_usb_type = Enumeration("device_usb_type", [
+ (1, "HOSTDEV"),
+ ])
+
+libxl_device_usb = Struct("device_usb", [
+ ("protocol", libxl_device_usb_protocol,
+ {'init_val': 'LIBXL_USB_PROTOCOL_AUTO'}),
+ ("backend_domid", libxl_domid,
+ {'init_val': 'LIBXL_DEVICE_USB_BACKEND_DEFAULT'}),
+ ("u", KeyedUnion(None, libxl_device_usb_type, "type",
+ [("hostdev", Struct(None, [
+ ("hostbus", integer),
+ ("hostaddr", integer) ]))
+ ]))
+ ])
+
libxl_domain_config = Struct("domain_config", [
("c_info", libxl_domain_create_info),
("b_info", libxl_domain_build_info),
Do you not want to add a list of USB devices to the domain_build_info?
I don't think I can get that functionality working for 4.3. It's
definitely on my plans for the future, though.
diff --git a/tools/libxl/libxl_usb.c b/tools/libxl/libxl_usb.c
new file mode 100644
index 0000000..c320487
--- /dev/null
+++ b/tools/libxl/libxl_usb.c
@@ -0,0 +1,526 @@
+/*
+ * Copyright (C) 2009 Citrix Ltd.
+ * Author Vincent Hanquez <vincent.hanquez@xxxxxxxxxxxxx>
+ * Author Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
Probably not true?
Haha -- oops...
+/*
+ * /libxl/<domid>/usb/<devid>/{type, protocol, backend, [typeinfo]}
+ */
+#define USB_INFO_PATH "%s/usb"
This duplicates something you added in libxl_create.c I think. I nearly
suggested moving that code into this file as a libxl__domain_usb_init or
something like that.
The other option would be to make a function like
libxl__xs_libxl_path(), but that seemed a bit heavy-handed to me. I
guess libxl__domain_usb_init sounds good.
+#define USB_HOSTDEV_ID "hostdev-%04x-%04x"
+
+/*
+ * Just use plain numbers for now. Replace these with strings at some point.
+ */
+static char * hostdev_xs_path(libxl__gc *gc, uint32_t domid,
+ libxl__device_usb *usbdev)
+{
+ return GCSPRINTF(USB_INFO_PATH "/%s",
+ libxl__xs_libxl_path(gc, domid),
+ GCSPRINTF(USB_HOSTDEV_ID,
+ (uint16_t)usbdev->u.hostdev.hostbus,
+ (uint16_t)usbdev->u.hostdev.hostaddr));
+}
+
+static void hostdev_xs_append_params(libxl__gc *gc, libxl__device_usb *usbdev,
+ flexarray_t *a)
+{
+ flexarray_append_pair(a, "hostbus",
+ GCSPRINTF("%d",
+ usbdev->u.hostdev.hostbus));
+ flexarray_append_pair(a, "hostaddr",
+ GCSPRINTF("%d",
+ usbdev->u.hostdev.hostaddr));
Are the second and third lines > 80 chars in total?
You mean, "Why did you break the GSPRINTF into two lines?" Yes, because
on one line it's > 80 chars.
[...]
+static int usb_add_xenstore(libxl__gc *gc, uint32_t domid,
+ libxl__device_usb *usbdev)
+{
[...]
+ default:
+ LOG(ERROR, "Invalid device type: %d", usbdev->type);
+ return ERROR_FAIL;
+ }
+
+ LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Adding new usb device to xenstore");
You use a mixture of this and the LOG(DEBUG,...) form in this patch, we
tend to prefer the latter for new code.
Ack.
+
+ for(;;) {
+ rc = libxl__xs_transaction_start(gc, &t);
+ if (rc) goto out;
+
+ /* Helpfully, mkdir returns 0 on failure, 1 on success */
+ if (!libxl__xs_mkdir(gc, t, dev_path, noperm, ARRAY_SIZE(noperm))) {
+ rc = ERROR_FAIL;
+ goto out;
+ }
+
+ /* And libxl__xs_writev *always* returns 0 no matter what */
Strictly speaking the *current* implementation always returns 0, but if
someone changes that your code will break. But none of the other callers
check it either, so meh ;-)
In fact, I already have tried changing it, and the result is that domain
creation fails -- so whoever does change it will have a lot of cleaning
up to do anyway. :-) (This is on my to-do list for the 4.4 dev cycle.)
+ libxl__xs_writev(gc, t, dev_path,
+ libxl__xs_kvs_of_flexarray(gc, dev, dev->count));
+
+ rc = libxl__xs_transaction_commit(gc, &t);
+ if (!rc) break;
+ if (rc <0) goto out;
+ }
+
+out:
According to libxl_internal.h you need to call
libxl__xs_transaction_abort on the error path.
Good catch -- sorry I missed that.
[...]
+
+static int is_usbdev_type_hostdev_equal(libxl__device_usb *a,
+ libxl__device_usb *b)
+{
+ if (!memcmp(&a->u.hostdev, &b->u.hostdev, sizeof(a->u.hostdev)))
I'm not 100% sure it is permissible to compare structs for equality in
this way. What if the compiler decides to add some padding which doesn't
get initialised?
I'm afraid this probably means you have to compare field by field.
Ack.
+ return 1;
+ else
+ return 0;
+}
+static void usbdev_ext_to_int(libxl__device_usb *dev_int,
+ libxl_device_usb *dev_ext)
+{
+ dev_int->protocol = dev_ext->protocol;
+
+ if (dev_ext->backend_domid == LIBXL_DEVICE_USB_BACKEND_DEFAULT)
+ dev_int->backend_domid = 0;
+ else
+ dev_int->backend_domid = dev_ext->backend_domid;
+
+ dev_int->type = dev_ext->type;
+ memcpy(&dev_int->u, &dev_ext->u, sizeof(dev_ext->u));
I don't think you can be sure that these two things have been laid out
the same by the compiler.
Ack.
If you make dev_ext->backend_domid default to 0 then I'm not entirely
sure what the point of this internal/external distinction is.
+static int libxl__device_usb_add(libxl__gc *gc, uint32_t domid,
+ libxl_device_usb *dev_ext)
+{
+ libxl_ctx *ctx = libxl__gc_owner(gc);
+ libxl__device_usb *assigned, _usbdev, *usbdev;
+ int rc = ERROR_FAIL, num_assigned;
+ libxl_domain_type domtype = libxl__domain_type(gc, domid);
+
+ /* Interpret incoming */
+ usbdev = &_usbdev;
+
+ usbdev->target_domid = domid;
+ usbdev->dm_domid = libxl_get_stubdom_id(ctx, domid);
+
+ usbdev_ext_to_int(usbdev, dev_ext);
+
+ if (usbdev->protocol == LIBXL_USB_PROTOCOL_AUTO) {
+ if (domtype == LIBXL_DOMAIN_TYPE_PV) {
+ usbdev->protocol = LIBXL_USB_PROTOCOL_PV;
+ } else if (domtype == LIBXL_DOMAIN_TYPE_HVM) {
+ /* FIXME: See if we can detect PV frontend */
+ usbdev->protocol = LIBXL_USB_PROTOCOL_DEVICEMODEL;
+ }
+ }
+
+ /* Check to make sure we're doing something that's impemented */
"implemented"
+ if (usbdev->protocol != LIBXL_USB_PROTOCOL_DEVICEMODEL) {
+ rc = ERROR_FAIL;
+ LOG(ERROR, "Protocol not implemented");
+ goto out;
+ }
+
+ if (usbdev->dm_domid != 0) {
+ rc = ERROR_FAIL;
+ LOG(ERROR, "Stubdoms not yet supported");
+ goto out;
+ }
+
+ /* Double-check to make sure it's not already assigned */
+ rc = get_assigned_devices(gc, domid, &assigned, &num_assigned);
+ if (rc) {
+ LOG(ERROR, "cannot determine if device is assigned,"
+ " refusing to continue");
+ goto out;
+ }
+ if (is_usbdev_in_array(assigned, num_assigned, usbdev)) {
+ LOG(ERROR, "USB device already attached to a domain");
+ rc = ERROR_FAIL;
+ goto out;
+ }
+
+ /* Do the add */
+ if (do_usb_add(gc, domid, usbdev))
+ rc = ERROR_FAIL;
+
+out:
+ return rc;
+}
+
+int libxl_device_usb_add(libxl_ctx *ctx, uint32_t domid,
+ libxl_device_usb *usbdev,
+ const libxl_asyncop_how *ao_how)
+{
+ AO_CREATE(ctx, domid, ao_how);
+ int rc;
+ rc = libxl__device_usb_add(gc, domid, usbdev);
+ libxl__ao_complete(egc, ao, rc);
+ return AO_INPROGRESS;
+}
Please define this (and the remove) using the DEFINE_DEVICE_ADD/REMOVE
macros in libxl.c. Not least because I'm not convinced your handling of
the AO stuff is correct (or indeed present).
This will probably require the libxl__device_usb_add/remove to change to
do the AIO stuff too -- i.e. at the end:
aodev->rc = rc;
if (rc) aodev->callback(egc, aodev);
return;
These follow the template set by
libxl_pci.c:libxl_device_pci_{add,remove,destroy}(), which were chosen
specifically because they essentially do a null ASYNC, but
interface-wise allow for real async to be added in the future.
-George
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
