Re: [libvirt] [PATCHv3 2/4] add pci passthrough to libxl driver

cyliu(a)suse.com wrote: > From: Chunyan Liu <cyliu(a)suse.com&gt; > > Add pci passthrough to libxl driver, support attach-device, detach-device and > start a vm with pci hostdev specified. > > Signed-off-by: Chunyan Liu <cyliu(a)suse.com&gt; > --- > src/libxl/libxl_conf.c | 65 ++++++++++++ > src/libxl/libxl_conf.h | 5 +- > src/libxl/libxl_driver.c | 250 +++++++++++++++++++++++++++++++++++++++++++++- > 3 files changed, 315 insertions(+), 5 deletions(-) > > diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c > index 827dfdd..aa6fd1e 100644 > --- a/src/libxl/libxl_conf.c > +++ b/src/libxl/libxl_conf.c > @@ -871,6 +871,67 @@ error: > return -1; > } > > +int libxlMakePci(virDomainHostdevDefPtr hostdev, libxl_device_pci *pcidev) > Return type and function name on separate lines. > +{ > + if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) > + return -1; > + if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) > + return -1; > + > + pcidev->domain = hostdev->source.subsys.u.pci.addr.domain; > + pcidev->bus = hostdev->source.subsys.u.pci.addr.bus; > + pcidev->dev = hostdev->source.subsys.u.pci.addr.slot; > + pcidev->func = hostdev->source.subsys.u.pci.addr.function; > + > + return 0; > +} > + > +static int > +libxlMakePciList(virDomainDefPtr def, libxl_domain_config *d_config) > +{ > + virDomainHostdevDefPtr *l_hostdevs = def->hostdevs; > + size_t nhostdevs = def->nhostdevs; > + size_t npcidevs = 0; > + libxl_device_pci *x_pcidevs; > + size_t i, j; > + > + if (nhostdevs == 0) > + return 0; > + > + if (VIR_ALLOC_N(x_pcidevs, nhostdevs) < 0) { > + virReportOOMError(); > No need to report OOM error. > + return -1; > + } > + > + for (i = 0, j = 0; i < nhostdevs; i++) { > + if (l_hostdevs[i]->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) > + continue; > + if (l_hostdevs[i]->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) > + continue; > + > + libxl_device_pci_init(&x_pcidevs[j]); > + > + if (libxlMakePci(l_hostdevs[i], &x_pcidevs[j]) < 0) > + goto error; > + > + npcidevs++; > + j++; > + } > + > + VIR_SHRINK_N(x_pcidevs, nhostdevs, nhostdevs - npcidevs); > + d_config->pcidevs = x_pcidevs; > + d_config->num_pcidevs = npcidevs; > + > + return 0; > + > +error: > + for (i = 0; i < npcidevs; i++) { > + libxl_device_pci_dispose(&x_pcidevs[i]); > + } > No need for the braces. From HACKING: Omit the curly braces around an "if", "while", "for" etc. body only when that body occupies a single line. > + VIR_FREE(x_pcidevs); > + return -1; > +} > + > virCapsPtr > libxlMakeCapabilities(libxl_ctx *ctx) > { > @@ -932,6 +993,10 @@ libxlBuildDomainConfig(libxlDriverPrivatePtr driver, > return -1; > } > > + if (libxlMakePciList(def, d_config) < 0) { > + return -1; > + } > + > Same here, no need for the braces. (I should put together a cleanup patch to fix the existing offenses in this file.) > d_config->on_reboot = def->onReboot; > d_config->on_poweroff = def->onPoweroff; > d_config->on_crash = def->onCrash; > diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h > index aa57710..db3e53d 100644 > --- a/src/libxl/libxl_conf.h > +++ b/src/libxl/libxl_conf.h > @@ -36,7 +36,7 @@ > # include "virobject.h" > # include "virchrdev.h" > > - > Spurious whitespace change. > +# define LIBXL_DRIVER_NAME "xenlight" > # define LIBXL_VNC_PORT_MIN 5900 > # define LIBXL_VNC_PORT_MAX 65535 > > @@ -126,7 +126,8 @@ libxlMakeNic(virDomainNetDefPtr l_nic, libxl_device_nic *x_nic); > int > libxlMakeVfb(libxlDriverPrivatePtr driver, > virDomainGraphicsDefPtr l_vfb, libxl_device_vfb *x_vfb); > - > Same here. I'd prefer a line between all these function declarations. > +int > +libxlMakePci(virDomainHostdevDefPtr hostdev, libxl_device_pci *pcidev); > int > libxlBuildDomainConfig(libxlDriverPrivatePtr driver, > virDomainObjPtr vm, libxl_domain_config *d_config); > diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c > index 9e9bc89..8166baf 100644 > --- a/src/libxl/libxl_driver.c > +++ b/src/libxl/libxl_driver.c > @@ -49,6 +49,7 @@ > #include "virstring.h" > #include "virsysinfo.h" > #include "viraccessapicheck.h" > +#include "virhostdev.h" > > #define VIR_FROM_THIS VIR_FROM_LIBXL > > @@ -698,6 +699,7 @@ libxlVmReap(libxlDriverPrivatePtr driver, > virDomainShutoffReason reason) > { > libxlDomainObjPrivatePtr priv = vm->privateData; > + virHostdevManagerPtr hostdev_mgr; > > if (libxl_domain_destroy(priv->ctx, vm->def->id, NULL) < 0) { > virReportError(VIR_ERR_INTERNAL_ERROR, > @@ -705,6 +707,10 @@ libxlVmReap(libxlDriverPrivatePtr driver, > return -1; > } > > + hostdev_mgr = virHostdevManagerGetDefault(); > + virHostdevReAttachDomainHostdevs(hostdev_mgr, LIBXL_DRIVER_NAME, > + vm->def, VIR_SP_PCI_HOSTDEV); > Parameter indentation looks wrong. > + > libxlVmCleanup(driver, vm, reason); > return 0; > } > @@ -928,6 +934,7 @@ libxlVmStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm, > char *managed_save_path = NULL; > int managed_save_fd = -1; > libxlDomainObjPrivatePtr priv = vm->privateData; > + virHostdevManagerPtr hostdev_mgr; > > /* If there is a managed saved state restore it instead of starting > * from scratch. The old state is removed once the restoring succeeded. */ > @@ -982,6 +989,12 @@ libxlVmStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm, > goto error; > } > > + VIR_DEBUG("Preparing host PCI devices"); > + hostdev_mgr = virHostdevManagerGetDefault(); > + if (virHostdevPrepareDomainHostdevs(hostdev_mgr, LIBXL_DRIVER_NAME, > + vm->def, VIR_SP_PCI_HOSTDEV) < 0) > + goto error; > + > /* use as synchronous operations => ao_how = NULL and no intermediate reports => ao_progress = NULL */ > > if (restore_fd < 0) > @@ -1075,6 +1088,7 @@ libxlReconnectDomain(virDomainObjPtr vm, > libxl_dominfo d_info; > int len; > uint8_t *data = NULL; > + virHostdevManagerPtr hostdev_mgr; > > virObjectLock(vm); > > @@ -1097,6 +1111,13 @@ libxlReconnectDomain(virDomainObjPtr vm, > > /* Update domid in case it changed (e.g. reboot) while we were gone? */ > vm->def->id = d_info.domid; > + > + /* Update hostdev state */ > + hostdev_mgr = virHostdevManagerGetDefault(); > + if (virHostdevPrepareDomainHostdevs(hostdev_mgr, LIBXL_DRIVER_NAME, > + vm->def, VIR_SP_PCI_HOSTDEV) < 0) > + goto out; > + > virDomainObjSetState(vm, VIR_DOMAIN_RUNNING, VIR_DOMAIN_RUNNING_UNKNOWN); > > if (!driver->nactive && driver->inhibitCallback) > @@ -3521,6 +3542,112 @@ cleanup: > } > > static int > +libxlDomainAttachHostPciDevice(libxlDomainObjPrivatePtr priv, > + virDomainObjPtr vm, > + virDomainHostdevDefPtr hostdev) > +{ > + libxl_device_pci pcidev; > + virDomainHostdevDefPtr found; > + virHostdevManagerPtr hostdev_mgr; > + > + if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) > + return -1; > + > + if (virDomainHostdevFind(vm->def, hostdev, &found) >= 0) { > + virReportError(VIR_ERR_OPERATION_FAILED, > + _("target pci device %.4x:%.2x:%.2x.%.1x already exists"), > + hostdev->source.subsys.u.pci.addr.domain, > + hostdev->source.subsys.u.pci.addr.bus, > + hostdev->source.subsys.u.pci.addr.slot, > + hostdev->source.subsys.u.pci.addr.function); > + return -1; > + } > + > + if (VIR_REALLOC_N(vm->def->hostdevs, vm->def->nhostdevs + 1) < 0) { > + virReportOOMError(); > No need to report OOM error. > + return -1; > + } > + > + hostdev_mgr = virHostdevManagerGetDefault(); > + if (virHostdevPreparePciHostdevs(hostdev_mgr, LIBXL_DRIVER_NAME, > + vm->def->name, vm->def->uuid, > + &hostdev, 1, 0) < 0) { > + goto cleanup; > + } > + > + if (libxlMakePci(hostdev, &pcidev) < 0) > + goto reattach_hostdev; > + > + if (libxl_device_pci_add(priv->ctx, vm->def->id, &pcidev, 0) < 0) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("libxenlight failed to attach pci device %.4x:%.2x:%.2x.%.1x"), > + hostdev->source.subsys.u.pci.addr.domain, > + hostdev->source.subsys.u.pci.addr.bus, > + hostdev->source.subsys.u.pci.addr.slot, > + hostdev->source.subsys.u.pci.addr.function); > + goto reattach_hostdev; > + } > + > + vm->def->hostdevs[vm->def->nhostdevs++] = hostdev; > + return 0; > + > +reattach_hostdev: > + virHostdevReAttachPciHostdevs(hostdev_mgr, LIBXL_DRIVER_NAME, > + vm->def->name, &hostdev, 1); > + > +cleanup: > + return -1; > +} > + > +static int > +libxlDomainAttachHostDevice(libxlDomainObjPrivatePtr priv, > + virDomainObjPtr vm, > + virDomainDeviceDefPtr dev) > +{ > + virDomainHostdevDefPtr hostdev = dev->data.hostdev; > + > + if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > + _("hostdev mode '%s' not supported"), > + virDomainHostdevModeTypeToString(hostdev->mode)); > + return -1; > + } > +/* > + if (qemuSetupHostdevCGroup(vm, hostdev) < 0) > + return -1; > + > + if (virSecurityManagerSetHostdevLabel(driver->securityManager, > + vm->def, hostdev, NULL) < 0) > + goto cleanup; > +*/ > Looks like this should be removed from the libxl driver :). > + switch (hostdev->source.subsys.type) { > + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: > + if (libxlDomainAttachHostPciDevice(priv, vm, hostdev) < 0) > + goto error; > + break; > + > + default: > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > + _("hostdev subsys type '%s' not supported"), > + virDomainHostdevSubsysTypeToString(hostdev->source.subsys.type)); > + goto error; > + } > + > + return 0; > + > +error: > +/* if (virSecurityManagerRestoreHostdevLabel(driver->securityManager, > + vm->def, hostdev, NULL) < 0) > + VIR_WARN("Unable to restore host device labelling on hotplug fail"); > + > +cleanup: > + if (qemuTeardownHostdevCgroup(vm, hostdev) < 0) > + VIR_WARN("Unable to remove host device cgroup ACL on hotplug fail"); > +*/ > + return -1; > Again, don't think this is needed in the libxl driver. Also, there is nothing to cleanup so might as well just return -1 when errors are encountered. > +} > + > +static int > libxlDomainDetachDeviceDiskLive(libxlDomainObjPrivatePtr priv, > virDomainObjPtr vm, virDomainDeviceDefPtr dev) > { > @@ -3586,7 +3713,11 @@ libxlDomainAttachDeviceLive(libxlDomainObjPrivatePtr priv, virDomainObjPtr vm, > if (!ret) > dev->data.disk = NULL; > break; > - > Spurious whitespace change. > + case VIR_DOMAIN_DEVICE_HOSTDEV: > + ret = libxlDomainAttachHostDevice(priv, vm, dev); > + if (!ret) > + dev->data.hostdev = NULL; > + break; > Generally there is a line between case statements, including the default case. > default: > virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > _("device type '%s' cannot be attached"), > @@ -3601,6 +3732,8 @@ static int > libxlDomainAttachDeviceConfig(virDomainDefPtr vmdef, virDomainDeviceDefPtr dev) > { > virDomainDiskDefPtr disk; > + virDomainHostdevDefPtr hostdev; > + virDomainHostdevDefPtr found; > > switch (dev->type) { > case VIR_DOMAIN_DEVICE_DISK: > @@ -3615,6 +3748,25 @@ libxlDomainAttachDeviceConfig(virDomainDefPtr vmdef, virDomainDeviceDefPtr dev) > /* vmdef has the pointer. Generic codes for vmdef will do all jobs */ > dev->data.disk = NULL; > break; > Same here, line before the next case. > + case VIR_DOMAIN_DEVICE_HOSTDEV: > + hostdev = dev->data.hostdev; > + > + if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) > + return -1; > + > + if (virDomainHostdevFind(vmdef, hostdev, &found) >= 0) { > + virReportError(VIR_ERR_OPERATION_FAILED, > + _("target pci device %.4x:%.2x:%.2x.%.1x\ > + already exists"), > + hostdev->source.subsys.u.pci.addr.domain, > + hostdev->source.subsys.u.pci.addr.bus, > + hostdev->source.subsys.u.pci.addr.slot, > + hostdev->source.subsys.u.pci.addr.function); > + return -1; > + } > + > + virDomainHostdevInsert(vmdef, hostdev); > + break; > > default: > virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > @@ -3625,6 +3777,95 @@ libxlDomainAttachDeviceConfig(virDomainDefPtr vmdef, virDomainDeviceDefPtr dev) > } > > static int > +libxlDomainDetachHostPciDevice(libxlDomainObjPrivatePtr priv, > + virDomainObjPtr vm, > + virDomainHostdevDefPtr hostdev) > +{ > + virDomainHostdevSubsysPtr subsys = &hostdev->source.subsys; > + libxl_device_pci pcidev; > + virDomainHostdevDefPtr detach; > + int idx; > + virHostdevManagerPtr hostdev_mgr; > + > +/* if (qemuIsMultiFunctionDevice(vm->def, detach->info)) { > + virReportError(VIR_ERR_OPERATION_FAILED, > + _("cannot hot unplug multifunction PCI device: %.4x:%.2x:%.2x.%.1x"), > + subsys->u.pci.addr.domain, subsys->u.pci.addr.bus, > + subsys->u.pci.addr.slot, subsys->u.pci.addr.function); > + goto cleanup; > + } > +*/ > Hmm, is a similar check needed in the libxl driver? It looks to me like this is checking for other devices with same domain, bus, and slot but different function. Isn't it possible to detach one function of a multi-function device, even if another function of the device is used by the domain?

+ if (subsys->type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) > + r eturn -1; > + > + idx = virDomainHostdevFind(vm->def, hostdev, &detach); > + if (idx < 0) { > + virReportError(VIR_ERR_OPERATION_FAILED, > + _("host pci device %.4x:%.2x:%.2x.%.1x not found"), > + subsys->u.pci.addr.domain, subsys->u.pci.addr.bus, > + subsys->u.pci.addr.slot, subsys->u.pci.addr.function); > + return -1; > + } > + > + if (libxlMakePci(detach, &pcidev) < 0) > + goto cleanup; > + > + if (libxl_device_pci_remove(priv->ctx, vm->def->id, &pcidev, 0) < 0) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("libxenlight failed to detach pci device\ > + %.4x:%.2x:%.2x.%.1x"), > + subsys->u.pci.addr.domain, subsys->u.pci.addr.bus, > + subsys->u.pci.addr.slot, subsys->u.pci.addr.function); > + goto cleanup; > + } > + > + virDomainHostdevRemove(vm->def, idx); > + > + hostdev_mgr = virHostdevManagerGetDefault(); > + virHostdevReAttachPciHostdevs(hostdev_mgr, LIBXL_DRIVER_NAME, > + vm->def->name, &hostdev, 1); > + > +/* > + if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE) && > + qemuDomainPCIAddressReleaseSlot(priv->pciaddrs, > + &detach->info->addr.pci) < 0) > + VIR_WARN("Unable to release PCI address on host device"); > +*/ > I don't think this is needed for the libxl driver, right?

the libxl_device_pci need disposed? libxl_device_pci_dispose, not necessary (only does memset work) but

+ > + return 0; > + > +cleanup: > + virDomainHostdevDefFree(detach); > + return -1; > +} > + > +static int libxlDomainDetachHostDevice(libxlDomainObjPrivatePtr priv, > + virDomainObjPtr vm, > + virDomainDeviceDefPtr dev) > Return type and function name on different lines. > +{ > + virDomainHostdevDefPtr hostdev = dev->data.hostdev; > + virDomainHostdevSubsysPtr subsys = &hostdev->source.subsys; > + > + if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > + _("hostdev mode '%s' not supported"), > + virDomainHostdevModeTypeToString(hostdev->mode)); > + return -1; > + } > + > + switch (subsys->type) { > + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: > + return libxlDomainDetachHostPciDevice(priv, vm, hostdev); > Line between cases. > + default: > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("unexpected hostdev type %d"), subsys->type); > + break; > + } > + > + return -1; > +} > + > +static int > libxlDomainDetachDeviceLive(libxlDomainObjPrivatePtr priv, virDomainObjPtr vm, > virDomainDeviceDefPtr dev) > { > @@ -3634,7 +3875,9 @@ libxlDomainDetachDeviceLive(libxlDomainObjPrivatePtr priv, virDomainObjPtr vm, > case VIR_DOMAIN_DEVICE_DISK: > ret = libxlDomainDetachDeviceDiskLive(priv, vm, dev); > break; > - > Spurious whitespace change. > + case VIR_DOMAIN_DEVICE_HOSTDEV: > + ret = libxlDomainDetachHostDevice(priv, vm, dev); > + break; > Line between cases. Regards, Jim > default: > virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > _("device type '%s' cannot be detached"), > @@ -3645,6 +3888,7 @@ libxlDomainDetachDeviceLive(libxlDomainObjPrivatePtr priv, virDomainObjPtr vm, > return ret; > } > > + > static int > libxlDomainDetachDeviceConfig(virDomainDefPtr vmdef, virDomainDeviceDefPtr dev) > { > @@ -4903,7 +5147,7 @@ libxlConnectSupportsFeature(virConnectPtr conn, int feature) > > static virDriver libxlDriver = { > .no = VIR_DRV_LIBXL, > - .name = "xenlight", > + .name = LIBXL_DRIVER_NAME, > .connectOpen = libxlConnectOpen, /* 0.9.0 */ > .connectClose = libxlConnectClose, /* 0.9.0 */ > .connectGetType = libxlConnectGetType, /* 0.9.0 */ >