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

On 05/20/2016 04:32 AM, Joao Martins wrote: > On 05/19/2016 09:14 AM, 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'. >> >> For example: >> usb.xml: >> <hostdev mode='subsystem' type='usb' managed='no'> >> <source> >> <address bus='1' device='3'/> >> </source> >> </hostdev> >> #xl attach-device dom usb.xml >> #xl detach-device dom usb.xml >> >> Signed-off-by: Chunyan Liu <cyliu(a)suse.com&gt; >> --- >> Changes: >> * add LIBXL_HAVE_PVUSB check >> * fix Jim's comments >> >> src/libxl/libxl_driver.c | 136 ++++++++++++++++++++++++++++++++++++++++++++++- >> 1 file changed, 135 insertions(+), 1 deletion(-) >> >> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c >> index 960673f..a171efe 100644 >> --- a/src/libxl/libxl_driver.c >> +++ b/src/libxl/libxl_driver.c >> @@ -3028,6 +3028,56 @@ libxlDomainAttachHostPCIDevice(libxlDriverPrivatePtr driver, >> return ret; >> } >> >> +#ifdef LIBXL_HAVE_PVUSB >> +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); >> + >> + if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS || >> + hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB) >> + goto cleanup; >> + >> + 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 reattach; >> + >> + 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 reattach; >> + } >> + >> + vm->def->hostdevs[vm->def->nhostdevs++] = hostdev; >> + ret = 0; >> + goto cleanup; >> + >> + reattach: >> + virHostdevReAttachUSBDevices(hostdev_mgr, LIBXL_DRIVER_NAME, >> + vm->def->name, &hostdev, 1); >> + >> + cleanup: >> + virObjectUnref(cfg); >> + libxl_device_usbdev_dispose(&usbdev); >> + return ret; >> +} >> +#endif >> + >> static int >> libxlDomainAttachHostDevice(libxlDriverPrivatePtr driver, >> virDomainObjPtr vm, >> @@ -3046,6 +3096,13 @@ libxlDomainAttachHostDevice(libxlDriverPrivatePtr driver, >> return -1; >> break; >> >> +#ifdef LIBXL_HAVE_PVUSB >> + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: >> + if (libxlDomainAttachHostUSBDevice(driver, vm, hostdev) < 0) >> + return -1; >> + break; >> +#endif >> + >> default: >> virReportError(VIR_ERR_CONFIG_UNSUPPORTED, >> _("hostdev subsys type '%s' not supported"), >> @@ -3271,7 +3328,9 @@ 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_PCI || >> + VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB)) > Is this conditional correct when LIBXL_HAVE_PVUSB isn't there? I think there are two problems here. First, (VIR_DOMAIN_DEVICE_DISK || VIR_DOMAIN_DEVICE_LEASE) evaluates to 1, so essentially this is equivalent to if (hostdev->source.subsys.type != 1) Second, I agree that we don't want to add a USB device to the config if libxl cant support it.