|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH RFC V2 3/5] libxl: add pvusb API
On Mon, Jan 19, 2015 at 8:28 AM, Chunyan Liu <cyliu@xxxxxxxx> wrote:
> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index 0a123f1..2e89244 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> +int libxl_intf_to_device_usb(libxl_ctx *ctx, uint32_t domid,
> + char *intf, libxl_device_usb *usb)
> + LIBXL_EXTERNAL_CALLERS_ONLY;
I guess this function will go away?
> diff --git a/tools/libxl/libxl_usb.c b/tools/libxl/libxl_usb.c
> new file mode 100644
> index 0000000..830a846
> --- /dev/null
> +++ b/tools/libxl/libxl_usb.c
> @@ -0,0 +1,1277 @@
> +static int libxl__usbport_add_xenstore(libxl__gc *gc,
> + xs_transaction_t tran,
> + uint32_t domid,
> + libxl_device_usbctrl *usbctrl)
Would it be too much to ask to have all the pvusb-specific stuff in a
separate file -- say, "libxl_pvusb.c" or something? That would make
it a lot easier when I add in the HVM USB stuff.
(Just to be clear, I'm asking as a favor -- it's policy that the first
mover gets to have it easier, and people who want to come and add
something later are the ones who have to do the refactoring.)
> +{
> + char *path;
> + int i;
> +
> + path = GCSPRINTF("%s/backend/vusb/%d/%d/port",
> + libxl__xs_get_dompath(gc, 0), domid, usbctrl->devid);
> +
> + libxl__xs_mkdir(gc, tran, path, NULL, 0);
> +
> + for (i = 1; i <= usbctrl->num_ports; i++) {
> + if (libxl__xs_write_checked(gc, tran, GCSPRINTF("%s/%d", path, i),
> ""))
> + return ERROR_FAIL;
> + }
> +
> + return 0;
> +}
> +
> +static int libxl__usbctrl_add_xenstore(libxl__gc *gc, uint32_t domid,
> + libxl_device_usbctrl *usbctrl)
> +{
> + libxl_ctx *ctx = libxl__gc_owner(gc);
> + flexarray_t *front;
> + flexarray_t *back;
> + libxl__device *device;
> + xs_transaction_t tran;
> + int rc = 0;
> +
> + GCNEW(device);
> + rc = libxl__device_from_usbctrl(gc, domid, usbctrl, device);
> + if (rc) goto out;
> +
> + front = flexarray_make(gc, 4, 1);
> + back = flexarray_make(gc, 12, 1);
> +
> + flexarray_append(back, "frontend-id");
> + flexarray_append(back, libxl__sprintf(gc, "%d", domid));
> + flexarray_append(back, "online");
> + flexarray_append(back, "1");
> + flexarray_append(back, "state");
> + flexarray_append(back, libxl__sprintf(gc, "%d", 1));
> + flexarray_append(back, "usb-ver");
> + flexarray_append(back, libxl__sprintf(gc, "%d", usbctrl->usb_version));
> + flexarray_append(back, "num-ports");
> + flexarray_append(back, libxl__sprintf(gc, "%d", usbctrl->num_ports));
So how much of this is pvusb-specific, and how much would be shared
between DEVICEMODEL? Because this bit looks specifically like the
stuff used to set up the pvusb connection...
> + flexarray_append(back, "type");
> + switch(usbctrl->type) {
> + case LIBXL_USBCTRL_TYPE_PV:{
> + flexarray_append(back, "PVUSB");
> + break;
> + }
> + case LIBXL_USBCTRL_TYPE_DEVICEMODEL: {
> + flexarray_append(back, "IOEMU");
> + break;
> + }
> + default:
> + /* not supported */
> + rc = ERROR_FAIL;
> + goto out;
> + }
...but this looks like it's trying to be shared between PVUSB and
DEVICEMODEL. I'm pretty sure this isn't going to work long-run,
because if we were to write all this stuff for a devicemodel, wouldn't
the pvusb back-end take this as setting up a new pvusb port?
Also, you don't seem to be storing or retreiving usbctrl->name here --
if we want that to be part of the interface we need to use it. (I
think we wanted that so that it could default to something like pv-0,
emul-1, &c).
In general, I don't think libxl should be storing stuff in the pvusb
front/back xenstore directories not related to that protocol. We
should store extraneous information in a libxl-specific directory.
You can see an example of the kind of think I'm talking about in the
HVM USB patch I submitted last year (see usb_add_xenstore()):
http://lists.xen.org/archives/html/xen-devel/2014-06/msg00086.html
Alternately -- at the moment, the only extraneous information we've
got is the name of the controller; if you wanted to propose that we
get rid of the name field, then there wouldn't be any extra
information to store.
> +
> + flexarray_append(front, "backend-id");
> + flexarray_append(front, libxl__sprintf(gc, "%d",
> usbctrl->backend_domid));
> + flexarray_append(front, "state");
> + flexarray_append(front, libxl__sprintf(gc, "%d", 1));
> +
> +retry_transaction:
> + tran = xs_transaction_start(ctx->xsh);
> +
> + libxl__device_generic_add(gc, tran, device,
> + libxl__xs_kvs_of_flexarray(gc, back,
> back->count),
> + libxl__xs_kvs_of_flexarray(gc, front,
> front->count),
> + NULL);
> + libxl__usbport_add_xenstore(gc, tran, domid, usbctrl);
> +
> + if (!xs_transaction_end(ctx->xsh, tran, 0)) {
> + if (errno == EAGAIN)
> + goto retry_transaction;
> + else {
> + rc = ERROR_FAIL;
> + goto out;
> + }
> + }
> +
> +out:
> + return rc;
> +}
> +
> +static int libxl__device_usbctrl_add(libxl__gc *gc, uint32_t domid,
> + libxl_device_usbctrl *usbctrl)
> +{
> + int rc = 0;
> +
> + rc = libxl__device_usbctrl_setdefault(gc, domid, usbctrl);
> + if(rc) goto out;
> +
> + if (usbctrl->devid == -1) {
This also needs to have a symbolic name; something with "AUTO" in it.
> + if ((usbctrl->devid = libxl__device_nextid(gc, domid, "vusb")) < 0) {
> + rc = ERROR_FAIL;
> + goto out;
> + }
> + }
> +
> + if (libxl__usbctrl_add_xenstore(gc, domid, usbctrl) < 0){
> + rc = ERROR_FAIL;
> + goto out;
> + }
> +
> +out:
> + return rc;
> +}
> +
> +/* Following functions are to get assignable usb devices */
<snip>
> +libxl_device_usb *
> +libxl_device_usb_assignable_list(libxl_ctx *ctx, int *num)
I'm a bit ambivalent about this one. For people using xl, "lsusb"
should be just fine. Is anyone building a toolstack on top of libxl
directly going to need this functionality? Would libvirt use this
functionality, for instance, or would it use libusb? Or just leave
that to the caller?
> +static int libxl__device_usb_setdefault(libxl__gc *gc, uint32_t domid,
> + libxl_device_usb *usb)
> +{
> + char *be_path, *tmp;
> +
> + if (usb->ctrl == -1) {
> + int ret = libxl__device_usb_set_default_usbctrl(gc, domid, usb);
> + /* If no existing ctrl to host this usb device, setup a new one */
> + if (ret) {
> + libxl_device_usbctrl usbctrl;
> + libxl_device_usbctrl_init(&usbctrl);
> + libxl__device_usbctrl_add(gc, domid, &usbctrl);
What happens if this fails?
-George
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |