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

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

>>> On 8/13/2015 at 05:09 PM, in message
<20150813090938.GI7460@xxxxxxxxxxxxxxxxxxxxx>, Wei Liu <wei.liu2@xxxxxxxxxx>
wrote: 
> On Tue, Aug 11, 2015 at 08:24:01PM -0600, Chun Yan Liu wrote: 
> >  
> >  
> > >>> 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?  
> >  
> > You mean races with other device removal (like 'vbd') ? Yes, it is  
> idempotent. 
> > Only for 'vusb' (corresponding to USB controller), before removing USB  
> controller 
> > it will first removing all USB devices under it.
h >  
>  
> No. What I mean is, the removal of usbctrl triggers removal of all 
> assigned usb devices. And then this function initiates removal of 
> assigned usb devices again. Is this a possible scenario? 

No, it's not possible. libxl__devices_destroy is used in domain destroy, it's
trying to scan each device type in xenstore and destroy them. Since USB device
is NOT presented as a separate device type but inside USB controller (which is
represented by a 'vusb' device in xenstore), so when scanning 'vusb' type, it
tries to destroy USB controller, within that it will destroy all USB devices 
under
that controller. No entry to remove USB device alone. 

Thanks,
Chunyan

>  
> > >   
> > > >                  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.  
> >  
> > It will complete ao in aodev->callback(egc, aodev) in "out:" section, here: 
> >    if (rc) aodev->callback(egc, aodev); 
> >  
>  
> I'm still confused by the way it is structured. If aodev->callback 
> completes the AO nonetheless, why don't you just call that 
> unconditionally? 
>  
> Wei. 
>  
>  
> > Thanks, 
> > Chunyan 
> >  
> > >   
> > > 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