[libvirt] [PATCH 2/2] libxl: support hotplug USB host device

[Chun Yan Liu]

>>> On 5/13/2016 at 07:20 AM, in message <57350FA5.3070307 at suse.com>, Jim Fehlig
<jfehlig at suse.com> wrote: 
> Chunyan Liu wrote: 
> > Support hot attach/detach a USB host device to guest. 
> > Curretnly libxl only supports xen PV guest, and only 
>  
> s/Curretnly/Currently/ 
>  
> > supports specifying USB host device by 'bus number' 
> > and 'device number'. 
> >  
> > Signed-off-by: Chunyan Liu <cyliu at suse.com> 
> > --- 
> >  src/libxl/libxl_driver.c | 130  
> ++++++++++++++++++++++++++++++++++++++++++++++- 
> >  1 file changed, 129 insertions(+), 1 deletion(-) 
> >  
> > diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c 
> > index 18a0891..8900644 100644 
> > --- a/src/libxl/libxl_driver.c 
> > +++ b/src/libxl/libxl_driver.c 
> > @@ -3024,6 +3024,55 @@ libxlDomainAttachHostPCIDevice(libxlDriverPrivatePtr  
> driver, 
> >  } 
> >   
> >  static int 
> > +libxlDomainAttachHostUSBDevice(libxlDriverPrivatePtr driver, 
> > +                               virDomainObjPtr vm, 
> > +                               virDomainHostdevDefPtr hostdev) 
> > +{ 
> > +    libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver); 
> > +    libxl_device_usbdev usbdev; 
> > +    virHostdevManagerPtr hostdev_mgr = driver->hostdevMgr; 
> > +    int ret = -1; 
> > + 
> > +    libxl_device_usbdev_init(&usbdev); 
>  
> Move the init below the check for hostdev->mode and hostdev->source.subsys.type, 
> otherwise it is not disposed if the check fails. 

Yes, will update.

>  
> > + 
> > +    if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS || 
> > +        hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB) 
> > +        return ret; 
> > + 
> > +    if (VIR_REALLOC_N(vm->def->hostdevs, vm->def->nhostdevs + 1) < 0) 
> > +        goto cleanup; 
> > + 
> > +    if (virHostdevPrepareUSBDevices(hostdev_mgr, LIBXL_DRIVER_NAME, 
> > +                                    vm->def->name, &hostdev, 1, 0) < 0) 
> > +        goto cleanup; 
> > + 
> > +    if (libxlMakeUSB(hostdev, &usbdev) < 0) 
> > +        goto error; 
> > + 
> > +    if (libxl_device_usbdev_add(cfg->ctx, vm->def->id, &usbdev, 0) < 0) { 
> > +        virReportError(VIR_ERR_INTERNAL_ERROR, 
> > +                       _("libxenlight failed to attach usb device  
> Busnum:%3x, Devnum:%3x"), 
> > +                       hostdev->source.subsys.u.usb.bus, 
> > +                       hostdev->source.subsys.u.usb.device); 
> > +        goto error; 
> > +    } 
> > + 
> > +    vm->def->hostdevs[vm->def->nhostdevs++] = hostdev; 
> > +    ret = 0; 
> > +    goto cleanup; 
> > + 
> > + error: 
>  
> I think 'reattach' is a better name for this label. 

Will update.

>  
> > +    virHostdevReAttachUSBDevices(hostdev_mgr, LIBXL_DRIVER_NAME, 
> > +                                 vm->def->name, &hostdev, 1); 
> > + 
> > + cleanup: 
> > +    virObjectUnref(cfg); 
> > +    libxl_device_usbdev_dispose(&usbdev); 
> > +    return ret; 
> > +} 
> > + 
> > + 
> > +static int 
> >  libxlDomainAttachHostDevice(libxlDriverPrivatePtr driver, 
> >                              virDomainObjPtr vm, 
> >                              virDomainHostdevDefPtr hostdev) 
> > @@ -3041,6 +3090,11 @@ libxlDomainAttachHostDevice(libxlDriverPrivatePtr  
> driver, 
> >              return -1; 
> >          break; 
> >   
> > +    case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: 
> > +        if (libxlDomainAttachHostUSBDevice(driver, vm, hostdev) < 0) 
> > +            return -1; 
> > +        break; 
> > + 
> >      default: 
> >          virReportError(VIR_ERR_CONFIG_UNSUPPORTED, 
> >                         _("hostdev subsys type '%s' not supported"), 
> > @@ -3266,7 +3320,8 @@ libxlDomainAttachDeviceConfig(virDomainDefPtr vmdef,  
> virDomainDeviceDefPtr dev) 
> >          case VIR_DOMAIN_DEVICE_HOSTDEV: 
> >              hostdev = dev->data.hostdev; 
> >   
> > -            if (hostdev->source.subsys.type !=  
> VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) 
> > +            if (hostdev->source.subsys.type != 
> > +                (VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB ||  
> VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI)) 
>  
>         if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB  
> || 
>             hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) 
>  
> >                  return -1; 
> >   
> >              if (virDomainHostdevFind(vmdef, hostdev, &found) >= 0) { 
> > @@ -3385,6 +3440,76 @@ libxlDomainDetachHostPCIDevice(libxlDriverPrivatePtr  
> driver, 
> >  } 
> >   
> >  static int 
> > +libxlDomainDetachHostUSBDevice(libxlDriverPrivatePtr driver, 
> > +                               virDomainObjPtr vm, 
> > +                               virDomainHostdevDefPtr hostdev) 
> > +{ 
> > +    libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver); 
> > +    virDomainHostdevSubsysPtr subsys = &hostdev->source.subsys; 
> > +    virDomainHostdevSubsysUSBPtr usbsrc = &subsys->u.usb; 
> > +    virHostdevManagerPtr hostdev_mgr = driver->hostdevMgr; 
> > +    libxl_device_usbdev usbdev; 
> > +    libxl_device_usbdev *usbdevs = NULL; 
> > +    int num = 0; 
> > +    virDomainHostdevDefPtr detach; 
> > +    int idx; 
> > +    size_t i; 
> > +    bool found = false; 
> > +    int ret = -1; 
> > + 
> > +    libxl_device_usbdev_init(&usbdev); 
> > + 
> > +    idx = virDomainHostdevFind(vm->def, hostdev, &detach); 
> > +    if (idx < 0) { 
> > +        virReportError(VIR_ERR_OPERATION_FAILED, 
> > +                       _("host USB device Busnum: %3x, Devnum:%3x not  
> found"), 
>  
> Space after Busnum, but not Devnum. 

Will update.

>  
> > +                       usbsrc->bus, usbsrc->device); 
> > +        goto cleanup; 
> > +    } 
> > + 
> > +    usbdevs = libxl_device_usbdev_list(cfg->ctx, vm->def->id, &num); 
> > +    for (i = 0; i < num; i++) { 
> > +        if (usbdevs[i].u.hostdev.hostbus == usbsrc->bus && 
> > +            usbdevs[i].u.hostdev.hostaddr == usbsrc->device) { 
> > +            libxl_device_usbdev_copy(cfg->ctx, &usbdev, &usbdevs[i]); 
> > +            found = true; 
> > +            break; 
> > +        } 
> > +    } 
> > +    libxl_device_usbdev_list_free(usbdevs, num); 
> > + 
> > +    if (!found) { 
> > +        virReportError(VIR_ERR_OPERATION_FAILED, 
> > +                       _("host USB device Busnum: %3x, Devnum:%3x not  
> found"), 
>  
> Same here. 
>  
> > +                       usbsrc->bus, usbsrc->device); 
> > +        goto cleanup; 
> > +    } 
> > + 
> > +    if (libxl_device_usbdev_remove(cfg->ctx, vm->def->id, &usbdev, 0) < 0) { 
> > +        virReportError(VIR_ERR_INTERNAL_ERROR, 
> > +                       _("libxenlight failed to detach USB device\ 
> > +                          Busnum: %3x, Devnum:%3x"), 
>  
> And here. 
>  
> > +                       usbsrc->bus, usbsrc->device); 
> > +        goto error; 
> > +    } 
> > + 
> > +    virDomainHostdevRemove(vm->def, idx); 
> > + 
> > +    virHostdevReAttachUSBDevices(hostdev_mgr, LIBXL_DRIVER_NAME, 
> > +                                 vm->def->name, &hostdev, 1); 
> > + 
> > +    ret = 0; 
> > + 
> > + error: 
> > +    virDomainHostdevDefFree(detach); 
>  
> The error label will be hit on success of the function. Should 
> virDomainHostdevDefFree() be called in 'cleanup' and 'error' dropped? 

Yes, that's better. Will update.

Thanks,
Chunyan

>  
> Regards, 
> Jim 
>  
> > + 
> > + cleanup: 
> > +    virObjectUnref(cfg); 
> > +    libxl_device_usbdev_dispose(&usbdev); 
> > +    return ret; 
> > +} 
> > + 
> > +static int 
> >  libxlDomainDetachHostDevice(libxlDriverPrivatePtr driver, 
> >                              virDomainObjPtr vm, 
> >                              virDomainHostdevDefPtr hostdev) 
> > @@ -3402,6 +3527,9 @@ libxlDomainDetachHostDevice(libxlDriverPrivatePtr  
> driver, 
> >          case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: 
> >              return libxlDomainDetachHostPCIDevice(driver, vm, hostdev); 
> >   
> > +        case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: 
> > +            return libxlDomainDetachHostUSBDevice(driver, vm, hostdev); 
> > + 
> >          default: 
> >              virReportError(VIR_ERR_INTERNAL_ERROR, 
> >                             _("unexpected hostdev type %d"), subsys->type); 
>  
>