Re: [PATCH V7 3/7] libxl: add pvusb API

["Chun Yan Liu" <cyliu@suse.com>]

>>> On 10/1/2015 at 01:55 AM, in message <560C2204.9030707@citrix.com>, George
Dunlap <george.dunlap@citrix.com> wrote: 
> On 25/09/15 03:11, Chunyan Liu wrote: 
> > Add pvusb APIs, including: 
> >  - attach/detach (create/destroy) virtual usb controller. 
> >  - attach/detach usb device 
> >  - list usb controllers and usb devices 
> >  - get information of usb controller and usb device 
> >  - some other helper functions 
> >  
> > Signed-off-by: Chunyan Liu <cyliu@suse.com> 
> > Signed-off-by: Simon Cao <caobosimon@gmail.com> 
>  
> Hey Chunyan!  This looks pretty close to being ready to check in to me. 
>  
> There are four basic comments I have.  I'm keen to get this series in so 
> that we can start doing more collaborative improvement; so I'll give my 
> comments, and then talk about timing and a plan to get this in as 
> quikcly as possible at the bottom. 
>  
>  
> > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c 
> > index aea4887..1e2c63e 100644 
> > --- a/tools/libxl/libxl.c 
> > +++ b/tools/libxl/libxl.c 
> > @@ -4192,11 +4192,54 @@ DEFINE_DEVICE_REMOVE(vtpm, destroy, 1) 
> >   
> >   
> /**************************************************************************** 
> **/ 
> >   
> > +/* Macro for defining device remove/destroy functions for usbctrl */ 
> > +/* Following functions are defined: 
> > + * libxl_device_usbctrl_remove 
> > + * libxl_device_usbctrl_destroy 
> > + */ 
> > + 
> > +#define DEFINE_DEVICE_REMOVE_EXT(type, removedestroy, f)                \ 
> > +    int libxl_device_##type##_##removedestroy(libxl_ctx *ctx,           \ 
> > +        uint32_t domid, libxl_device_##type *type,                      \ 
> > +        const libxl_asyncop_how *ao_how)                                \ 
> > +    {                                                                   \ 
> > +        AO_CREATE(ctx, domid, ao_how);                                  \ 
> > +        libxl__device *device;                                          \ 
> > +        libxl__ao_device *aodev;                                        \ 
> > +        int rc;                                                         \ 
> > +                                                                        \ 
> > +        GCNEW(device);                                                  \ 
> > +        rc = libxl__device_from_##type(gc, domid, type, device);        \ 
> > +        if (rc != 0) goto out;                                          \ 
> > +                                                                        \ 
> > +        GCNEW(aodev);                                                   \ 
> > +        libxl__prepare_ao_device(ao, aodev);                            \ 
> > +        aodev->action = LIBXL__DEVICE_ACTION_REMOVE;                    \ 
> > +        aodev->dev = device;                                            \ 
> > +        aodev->callback = device_addrm_aocomplete;                      \ 
> > +        aodev->force = f;                                               \ 
> > +        libxl__initiate_device_##type##_remove(egc, aodev);             \ 
>  
> So this seems to be exactly the same as DEFINE_DEVICE_REMOVE(), except 
> that you call libxl__initiate_device_usbctrl_remove() here rather than 
> libxl__initiate_device_remove(). 
>  
> It seems like it might be better to have a separate patch renaming 
> libxl__initiate_device_remove to libxl__initiate_device_generic_remove 
> (or something like that), and then add a parameter to the definition 
> above making it so that the definitions above pass in "generic". 
>  
> > diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c 
> > index bee5ed5..935f25b 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; 
> > +                } 
> >                  libxl__initiate_device_remove(egc, aodev); 
>  
> I think this would probably be more clear if you did: 
>  
> if(dev->backend_kind == LIBXL__DEVICE_KIND_VUSB) 
>   libxl__initiate_device_usbctl_remove() 
> else 
>   libxl__initiate_device_remove() 
>  
> > @@ -3951,7 +3966,10 @@ static inline void  
> libxl__update_config_vtpm(libxl__gc *gc, 
> >  #define COMPARE_PCI(a, b) ((a)->func == (b)->func &&    \ 
> >                             (a)->bus == (b)->bus &&      \ 
> >                             (a)->dev == (b)->dev) 
> > - 
> > +#define COMPARE_USB(a, b) ((a)->u.hostdev.hostbus == (b)->u.hostdev.hostbus && \ 
> > +                           (a)->u.hostdev.hostaddr == (b)->u.hostdev.hostaddr) 
>  
> Hmm... COMPARE_USB is used in three places: 
>  
> 1. Used in libxl.c:libxl_retrieve_domain_configuration() to de-duplicate 
> JSON and xenstore configuration 
>  
> 2. Used in libxl_pvusb.c:libxl__device_usb_add_xentore() to avoid adding 
> the same device twice 
>  
> 3. Used in libxl_pvusb.c:is_usbdev_in_array() to avoid assigning the 
> same host device 
>  
> For #3, we pretty clearly want to be comparing hostbus/hostaddr.  (In 
> fact, you didn't use the COMPARE_USB macro until I suggested it in v3.) 
>  
> But for #1 and #2, is that what we want?  Should we be comparing 
> <ctrl,port> instead? 
>  
> In fact, after thinking about pvusb backends more, I think I want to 
> assert that we do want to compare <ctrl,port> instead: <crtl,port> is 
> guaranteed to be unique for a domain, but it's entirely possible to have 
> two different devices, from two different backend domains, with the same 
> bus.addr. 
>  
> > +int libxl_devid_to_device_usbctrl(libxl_ctx *ctx, 
> > +                                  uint32_t domid, 
> > +                                  int devid, 
> > +                                  libxl_device_usbctrl *usbctrl) 
> > +{ 
> > +    GC_INIT(ctx); 
> > +    libxl_device_usbctrl *usbctrls; 
> > +    int nb = 0; 
> > +    int i, rc = -1; 
> > + 
> > +    usbctrls = libxl_device_usbctrl_list(ctx, domid, &nb); 
> > +    if (!nb) goto out; 
> > + 
> > +    for (i = 0; i < nb; i++) { 
> > +        if (devid == usbctrls[i].devid) { 
> > +            *usbctrl = usbctrls[i]; 
>  
> libxl maintainers: Is this kind of copying OK? 
>  
> The analog functions for vtpm and nic both take very different 
> approaches; both call libxl_device_[type]_init() and then fill in 
> individual elements (albeit in different ways). 
>  
> > +/* 
> > + * USB add 
> > + */ 
> > +static int do_usb_add(libxl__gc *gc, uint32_t domid, 
> > +                      libxl_device_usb *usbdev, 
> > +                      libxl__ao_device *aodev) 
> > +{ 
> > +    libxl_ctx *ctx = CTX; 
> > +    libxl_usbctrlinfo usbctrlinfo; 
> > +    libxl_device_usbctrl usbctrl; 
> > +    int rc; 
> > + 
> > +    libxl_usbctrlinfo_init(&usbctrlinfo); 
> > +    usbctrl.devid = usbdev->ctrl; 
> > +    rc = libxl_device_usbctrl_getinfo(ctx, domid, &usbctrl, &usbctrlinfo); 
> > +    if (rc) 
> > +        goto out; 
> > + 
> > +    switch (usbctrlinfo.type) { 
> > +    case LIBXL_USBCTRL_TYPE_DEVICEMODEL: 
> > +        LOG(ERROR, "Not supported"); 
> > +        break; 
> > +    case LIBXL_USBCTRL_TYPE_PV: 
> > +        rc = libxl__device_usb_add_xenstore(gc, domid, usbdev, 
> > +                                            aodev->update_json); 
> > +        if (rc) goto out; 
> > + 
> > +        rc = usbback_dev_assign(gc, usbdev); 
>  
> This assumes that the usb controller is dom0; but the interface 
> explicitly allows pvusb driver domains. 
>  
> I think it would be enough here to do this check if the usbctrl device 
> is dom0.  We should then assume that a pvusb driver domain will be 
> configured with all usb devices assigned to usbback already. 
>  
> I assume that there's a feedback mechanisms for backends for situations 
> where the requested device can't be made, right?  For example, if you 
> have a network driver domain and request a non-existent bridge?  If so, 
> I think we can let the same mechanism worth for pvusb backends to say "I 
> don't have that device available". 
>  
> > +        if (rc) { 
> > +            libxl__device_usb_remove_xenstore(gc, domid, usbdev); 
> > +            goto out; 
> > +        } 
> > +        break; 
> > +    default: 
> > +        rc = ERROR_FAIL; 
> > +        goto out; 
> > +    } 
> > + 
> > +out: 
> > +    libxl_usbctrlinfo_dispose(&usbctrlinfo); 
> > +    return rc; 
> > +} 
> > + 
> > +/* AO operation to add a usb device. 
> > + * 
> > + * Generally, it does: 
> > + * 1) check if the usb device type is assignable 
> > + * 2) check if the usb device is already assigned to a domain 
> > + * 3) add 'busid' of the usb device to xenstore contoller/port/. 
> > + *    (PVUSB driver watches the xenstore changes and will detect that.) 
> > + * 4) unbind usb device from original driver and bind to usbback. 
> > + *    If usb device has many interfaces, then: 
> > + *    - unbind each interface from its original driver and bind to usbback. 
> > + *    - store the original driver to xenstore for later rebinding when 
> > + *      detaching the device. 
> > + */ 
> > +void libxl__device_usb_add(libxl__egc *egc, uint32_t domid, 
> > +                           libxl_device_usb *usb, 
> > +                           libxl__ao_device *aodev) 
> > +{ 
> > +    STATE_AO_GC(aodev->ao); 
> > +    int rc = ERROR_FAIL; 
> > +    char *busid = NULL; 
> > +    libxl_device_usb *assigned; 
> > +    int num_assigned; 
> > + 
> > +    assert(usb->u.hostdev.hostbus > 0 && usb->u.hostdev.hostaddr > 0); 
> > + 
> > +    busid = usb_busaddr_to_busid(gc, usb->u.hostdev.hostbus, 
> > +                                 usb->u.hostdev.hostaddr); 
> > +    if (!busid) { 
> > +        LOG(ERROR, "USB device doesn't exist in sysfs"); 
> > +        goto out; 
> > +    } 
> > + 
> > +    if (!is_usb_assignable(gc, usb)) { 
> > +        LOG(ERROR, "USB device is not assignable."); 
> > +        goto out; 
> > +    } 
>  
> Similar issue with pvusb driver domains: we can't do this check for 
> driver domains. 
>  
> I wonder if we should do this check instead down on the 
> usbback_assign_path(). 
>  
> If we assume that for pvusb driver domains: 
> 1. All assignable devices will be assigned to usbback 
> 2. No controllers will be assigned (since they can't be) 
> 3. pvusb can say "no such device" for unassigned devices 
> then we can safely ignore the check for pvusb driver domains 
>  
> > + 
> > +    /* check usb device is already assigned */ 
> > +    rc = get_assigned_devices(gc, &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, usb)) { 
> > +        LOG(ERROR, "USB device already attached to a domain"); 
> > +        rc = ERROR_FAIL; 
> > +        goto out; 
> > +    } 
>  
> WRT driver domains: Since we're looking inside xenstore instead of 
> sysfs, we *can* actually do these checks. 
>  
> However, the "bus.addr" space is defined by the driver domain kernel, 
> and is not unique across all driver domains: If we have one usb 
> controller assigned to domain 0, and another assigned to a driver 
> domain, two distinct devices are likely to end up with the same bus.addr. 
>  
> So to be compatible with driver domains, we'd need to have 
> get_assigned_devices() only get devices with the same backend_domid as 
> the controller we're checking. 
>  
> > +int libxl_device_usb_getinfo(libxl_ctx *ctx, uint32_t domid, 
> > +                             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; 
> > + 
> > +    if (libxl_ctrlport_to_device_usb(ctx, domid, 
> > +                                     usb->ctrl, usb->port, usb) < 0) { 
> > +        rc = ERROR_FAIL; 
> > +        goto out; 
> > +    } 
>  
> I suppose technically since usb isn't declared const that we haven't 
> promised to modify it; but none of the other device_*_getinfo() 
> functions modify the device struct; I think it would be better to follow 
> suit and use a local variable to get the information from.

Take it.
 
>  
> However... 
>  
> > + 
> > +    usbinfo->devnum = usb->u.hostdev.hostaddr; 
> > +    usbinfo->busnum = usb->u.hostdev.hostbus; 
> > + 
> > +    busid = usb_busaddr_to_busid(gc, usb->u.hostdev.hostbus, 
> > +                                 usb->u.hostdev.hostaddr); 
> > +    if (!busid) { 
> > +        rc = ERROR_FAIL; 
> > +        goto out; 
> > +    } 
> > + 
> > +    filename = GCSPRINTF(SYSFS_USB_DEV"/%s/idVendor", busid); 
> > +    if (!libxl_read_sysfs_file_contents(ctx, filename, &buf, NULL)) 
> > +        sscanf(buf, "%" SCNx16, &usbinfo->idVendor); 
> > + 
> > +    filename = GCSPRINTF(SYSFS_USB_DEV"/%s/idProduct", busid); 
> > +    if (!libxl_read_sysfs_file_contents(ctx, filename, &buf, NULL)) 
> > +        sscanf(buf, "%" SCNx16, &usbinfo->idProduct); 
> > + 
> > +    filename = GCSPRINTF(SYSFS_USB_DEV"/%s/manufacturer", busid); 
> > +    if (!libxl_read_sysfs_file_contents(ctx, filename, &buf, &buflen) && 
> > +        buflen > 0) { 
> > +        /* replace \n to \0 */ 
> > +        if (((char *)buf)[buflen - 1] == '\n') 
> > +            ((char *)buf)[buflen - 1] = '\0'; 
> > +        usbinfo->manuf = libxl__strdup(NOGC, buf); 
> > +   } 
> > + 
> > +    filename = GCSPRINTF(SYSFS_USB_DEV"/%s/product", busid); 
> > +    if (!libxl_read_sysfs_file_contents(ctx, filename, &buf, &buflen) && 
> > +        buflen > 0) { 
> > +        /* replace \n to \0 */ 
> > +        if (((char *)buf)[buflen - 1] == '\n') 
> > +            ((char *)buf)[buflen - 1] = '\0'; 
> > +        usbinfo->prod = libxl__strdup(NOGC, buf); 
> > +    } 
>  
> Basically, starting here, we have information which won't be available 
> if we're using a pvusb driver domain. 
>  
> This information is nice-to-have, but I don't think this information is 
> directly relevant to libxl or xl; the funcitonality to get this 
> information is available from other libraries like libusb.  I'm inclined 
> to say that if we want to have pvusb driver domains (and I think we do), 
> we should just get rid of this information.

For command 'xl usb-list', those information should be reported to user.
Do you mean we could call libusb to get thoes information directly in
xl toolstack and get rid of this function?

I think we can keep the function, since every other device type has the
function XXX_getinfo. But here we could check backend_domid, for
backend=dom0, doing above work; for backend!=dom0 (e.g. pvusb
driver domain, no matter how, it should also be able to let users getting
those information. Can add code  in future.)
 
>  
> > diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl 
> > index 9f6ec00..4844f18 100644 
> > --- a/tools/libxl/libxl_types.idl 
> > +++ b/tools/libxl/libxl_types.idl 
> > @@ -595,6 +595,35 @@ libxl_device_rdm = Struct("device_rdm", [ 
> >      ("policy", libxl_rdm_reserve_policy), 
> >      ]) 
> >   
> > +libxl_usbctrl_type = Enumeration("usbctrl_type", [ 
> > +    (0, "AUTO"), 
> > +    (1, "PV"), 
> > +    (2, "DEVICEMODEL"), 
> > +    ]) 
> > + 
> > +libxl_usbdev_type = Enumeration("usbdev_type", [ 
> > +    (1, "hostdev"), 
> > +    ]) 
> > + 
> > +libxl_device_usbctrl = Struct("device_usbctrl", [ 
> > +    ("type", libxl_usbctrl_type), 
> > +    ("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), 
> > +    ("u", KeyedUnion(None, libxl_usbdev_type, "devtype", 
> > +           [("hostdev", Struct(None, [ 
> > +                 ("hostbus",   uint8), 
> > +                 ("hostaddr",  uint8)])), 
> > +           ])), 
> > +    ]) 
> > + 
> >  libxl_device_dtdev = Struct("device_dtdev", [ 
> >      ("path", string), 
> >      ]) 
> > @@ -627,6 +656,8 @@ libxl_domain_config = Struct("domain_config", [ 
> >      ("pcidevs", Array(libxl_device_pci, "num_pcidevs")), 
> >      ("rdms", Array(libxl_device_rdm, "num_rdms")), 
> >      ("dtdevs", Array(libxl_device_dtdev, "num_dtdevs")), 
> > +    ("usbctrls", Array(libxl_device_usbctrl, "num_usbctrls")), 
> > +    ("usbs", Array(libxl_device_usb, "num_usbs")), 
> >      ("vfbs", Array(libxl_device_vfb, "num_vfbs")), 
> >      ("vkbs", Array(libxl_device_vkb, "num_vkbs")), 
> >      ("vtpms", Array(libxl_device_vtpm, "num_vtpms")), 
> > @@ -676,6 +707,32 @@ libxl_vtpminfo = Struct("vtpminfo", [ 
> >      ("uuid", libxl_uuid), 
> >      ], dir=DIR_OUT) 
> >   
> > +libxl_usbctrlinfo = Struct("usbctrlinfo", [ 
> > +    ("type", libxl_usbctrl_type), 
> > +    ("devid", libxl_devid), 
> > +    ("version", integer), 
> > +    ("ports", integer), 
> > +    ("backend", string), 
> > +    ("backend_id", uint32), 
> > +    ("frontend", string), 
> > +    ("frontend_id", uint32), 
> > +    ("state", integer), 
> > +    ("evtch", integer), 
> > +    ("ref_urb", integer), 
> > +    ("ref_conn", integer), 
> > +    ], dir=DIR_OUT) 
> > + 
> > +libxl_usbinfo = Struct("usbinfo", [ 
> > +    ("ctrl", libxl_devid), 
> > +    ("port", integer), 
> > +    ("busnum", uint8), 
> > +    ("devnum", uint8), 
> > +    ("idVendor", uint16), 
> > +    ("idProduct", uint16), 
> > +    ("prod", string), 
> > +    ("manuf", string), 
> > +    ], dir=DIR_OUT) 
>  
> With the exception of the stuff in libxl_usbinfo we can't get if the 
> backend isn't dom0, the interface looks good to me now. 
>  
> OK, so I've covered four things: 
>  
> 1. The DEFINE_DEVICE_REMOVE_EXT macro.  I think I'd prefer checking in 
> things are as it is, and then have a follow-up patch to refactor 
> everything into a single DEFINE_DEVICE_REMOVE macro as described above. 
>  
> 2. COMPARE_USB.  I think this needs to be fixed; but this should be a 
> fairly simple change. 
>  
> 3. The conditional in libxl__device_destroy, the struct copy in 
> libxl_devid_to_device_usbctrl, and the modification of the device in 
> libxl_device_usb_getinfo are minor adjustments that can be discussed and 
> fixed. 
>  
> 4. The pvusb driver domain != dom0 comments.  This is somewhat secondary 
> to your primary goal; but at the moment the code will certainly not 
> work.  We have a couple of options: 
> a: Wait until we've fixed the driver domain functionality 
> b: Check the code in right now, with the driver domain fuctionality 
> explicitly disabled; that is, it will return an error if 
> backend_domain!=0.  Get a promise to implement the driver domain 
> functionality properly for 4.7 (on the threat of removing the feature).

I'll check the codes and add backend_domain check in related functions.

Thanks,
Chunyan  
 
> c: Take driver domain support out of the interface for now; send 
> follow-up patches to implement it properly. 
> d. Take driver domain support out of the interface and leave it that 
> way, for simplicity. 
>  
> I'd probably go with b or c. 
>  
> Thoughts? 
>  
>  -George 
>  
>