|
[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 3/7/2015 at 12:50 AM, in message
<CAFLBxZZfzL2F4qnuWGbPzA8v1fFGvvkBGn+gCO6CeEkvBf6hHA@xxxxxxxxxxxxxx>, George
Dunlap <George.Dunlap@xxxxxxxxxxxxx> wrote:
> 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?
With using bus:addr instead of sysfs interface, this will be removed.
>
> > 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.
Sure.
>
> (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...
To share with qemu emulated usb controller? I think pvusb backend driver will
probe things under backend/vusb/* and setup connection with pvusb frontend
driver, qemu emulated usb controller should not be placed there at all maybe.
>
> > + 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?
Yes, I think you are right. This place should only allow pvusb type.
Qemu emulated usb controller should not be stored here.
>
> 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).
First I wonder usbctrl->name is really needed. If just for interface, we can
use devid (index), it could be 0, 1, and with usb-list one knows info like:
0 is pv, 1 is emulated. But if it helps a lot with a 'name', surely we can
add :)
>
> 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?
This exists mainly because the proposed interface before using usb sysfs
interface rather than bus:addr, user has no much idea about which sysfs
interface is related to which usb device. Now if we decide to use bus:addr,
I agree this API could be removed. Just use lsusb. (But user should have this
knowledge in mind: a HUB could not be assigned to guest).
>
>
> > +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?
Currently it will check usb port existence later, so if this fails, errors will
be
reported in that step. But I think I'd better check the return value here to
report
error earlier. Thank you.
- Chunyan
>
> -George
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxx
> http://lists.xen.org/xen-devel
>
>
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |