Re: [Xen-devel] [PATCH V6 3/7] libxl: add pvusb API

["Chun Yan Liu" <cyliu@xxxxxxxx>]

Ian and George, could I have your comments?

Thanks,
Chunyan

>>> On 8/11/2015 at 07:27 PM, in message
<20150811112702.GF7460@xxxxxxxxxxxxxxxxxxxxx>, Wei Liu <wei.liu2@xxxxxxxxxx>
wrote: 
> On Mon, Aug 10, 2015 at 06:35:24PM +0800, Chunyan Liu wrote: 
> > Add pvusb APIs, including: 
> >  - attach/detach (create/destroy) virtual usb controller. 
> >  - attach/detach usb device 
> >  - list usb controller and usb devices 
> >  - some other helper functions 
> >  
> > Signed-off-by: Chunyan Liu <cyliu@xxxxxxxx> 
> > Signed-off-by: Simon Cao <caobosimon@xxxxxxxxx> 
> >  
> > --- 
> > changes: 
> >   - Address George's comments: 
> >   * Update libxl_device_usb_getinfo to read ctrl/port only and 
> >     get other information. 
> >   * Update backend path according to xenstore frontend 'xxx/backend' 
> >     entry instead of using TOOLSTACK_DOMID. 
> >   * Use 'type' to indicate qemu/pv instead of previous naming 'protocol'. 
> >   * Add USB 'devtype' union, currently only includes "hostdev" 
> >  
>  
> I will leave this to Ian and George since they had strong opinions on 
> this. 
>  
> I only skimmed this patch. Some comments below. 
>  
> [...] 
> > + 
> > +int libxl_device_usb_getinfo(libxl_ctx *ctx, uint32_t domid, 
> > +                             libxl_device_usb *usb, 
> > +                             libxl_usbinfo *usbinfo); 
> > + 
> >  /* Network Interfaces */ 
> >  int libxl_device_nic_add(libxl_ctx *ctx, uint32_t domid, libxl_device_nic  
> *nic, 
> >                           const libxl_asyncop_how *ao_how) 
> > 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; 
> > +                } 
>  
> Is there a risk that this races with individual device removal? I think 
> you get away with it because removal of individual device is idempotent? 
>  
> >                  libxl__initiate_device_remove(egc, aodev); 
> >              } 
> >          } 
> > diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h 
> > index f98f089..5be3b3a 100644 
> > --- a/tools/libxl/libxl_internal.h 
> > +++ b/tools/libxl/libxl_internal.h 
> > @@ -2553,6 +2553,14 @@ _hidden void libxl__device_vtpm_add(libxl__egc *egc, 
> >  
> uint32_t domid, 
> >                                     libxl_device_vtpm *vtpm, 
> >                                     libxl__ao_device *aodev); 
> >   
> > +_hidden void libxl__device_usbctrl_add(libxl__egc *egc, uint32_t domid, 
> > +                                       libxl_device_usbctrl *usbctrl, 
> > +                                       libxl__ao_device *aodev); 
> > + 
> > +_hidden void libxl__device_usb_add(libxl__egc *egc, uint32_t domid, 
> > +                                   libxl_device_usb *usb, 
> > +                                   libxl__ao_device *aodev); 
> > + 
> >  /* Internal function to connect a vkb device */ 
> >  _hidden int libxl__device_vkb_add(libxl__gc *gc, uint32_t domid, 
> >                                    libxl_device_vkb *vkb); 
> > @@ -2585,6 +2593,13 @@ _hidden void  
> libxl__wait_device_connection(libxl__egc*, 
> >  _hidden void libxl__initiate_device_remove(libxl__egc *egc, 
> >                                             libxl__ao_device *aodev); 
> >   
> > +_hidden int libxl__device_from_usbctrl(libxl__gc *gc, uint32_t domid, 
> [...] 
> > +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 = -1; 
> > +    char *busid = NULL; 
> > + 
> > +    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; 
> > +    } 
> > + 
> > +    /* check usb device is already assigned */ 
> > +    if (is_usb_assigned(gc, usb)) { 
> > +        LOG(ERROR, "USB device is already attached to a domain."); 
> > +        goto out; 
> > +    } 
> > + 
> > +    rc = libxl__device_usb_setdefault(gc, domid, usb, aodev->update_json); 
> > +    if (rc) goto out; 
> > + 
> > +    rc = libxl__device_usb_add_xenstore(gc, domid, usb, 
> > aodev->update_json); 
> > +    if (rc) goto out; 
> > + 
> > +    rc = usbback_dev_assign(gc, usb); 
> > +    if (rc) { 
> > +        libxl__device_usb_remove_xenstore(gc, domid, usb); 
> > +        goto out; 
> > +    } 
> > + 
> > +    libxl__ao_complete(egc, ao, 0); 
> > +    rc = 0; 
> > + 
> > +out: 
>  
> You forget to complete ao in failure path. 
>  
> But I'm not very familiar with the AO machinery, I will let Ian comment 
> on this. 
>  
> Wei. 
>  
> _______________________________________________ 
> 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