<div dir="ltr"> <div class="gmail_extra"> <div class="gmail_quote">2013/8/14 Jim Fehlig <<a href="mailto:jfehlig@suse.com" target="_blank">jfehlig@suse.com</a>> <blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <div class="im"><a href="mailto:cyliu@suse.com">cyliu@suse.com</a> wrote: > From: Chunyan Liu <<a href="mailto:cyliu@suse.com">cyliu@suse.com</a>> > > Add pci passthrough to libxl driver, support attach-device, detach-device and > start a vm with pci hostdev specified. > > Signed-off-by: Chunyan Liu <<a href="mailto:cyliu@suse.com">cyliu@suse.com</a>> > --- > 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) > </div>Return type and function name on separate lines. <div><div class="h5"> > +{ > + 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(); > </div></div>No need to report OOM error. <div><div class="h5"> > + 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]); > + } > </div></div>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. <div class="im"> > + 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; > + } > + > </div>Same here, no need for the braces. (I should put together a cleanup patch to fix the existing offenses in this file.) <div class="im"> > 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" > > - > </div>Spurious whitespace change. <div class="im"> > +# 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); > - > </div>Same here. I'd prefer a line between all these function declarations. <div><div class="h5"> > +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); > </div></div>Parameter indentation looks wrong. <div><div class="h5"> > + > 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(); > </div></div>No need to report OOM error. <div><div class="h5"> > + 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; > +*/ > </div></div>Looks like this should be removed from the libxl driver :). <div><div class="h5"> > + 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; > </div></div>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. <div class="im"> > +} > + > +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; > - > </div>Spurious whitespace change. <div class="im"> > + case VIR_DOMAIN_DEVICE_HOSTDEV: > + ret = libxlDomainAttachHostDevice(priv, vm, dev); > + if (!ret) > + dev->data.hostdev = NULL; > + break; > </div>Generally there is a line between case statements, including the default case. <div class="im"> > 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; > </div>Same here, line before the next case. <div><div class="h5"> > + 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; > + } > +*/ > </div></div>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? <div><div></div></div></blockquote><div> Not sure. Some one could explain? Quoted from Fedora documentation: "Red Hat Enterprise Linux 6.0 and newer supports hot plugging assigned PCI devices into virtual machines. However, PCI device hot plugging operates at the slot level and therefore does not support multi-function PCI devices. Multi-function PCI devices are recommended for static device configuration only." </div><div>But in qemu driver, there isn't limit to ATTACH a multi-function PCI device, only limit to DETACH. </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <div><div class="h5"> > + 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"); > +*/ > </div></div>I don't think this is needed for the libxl driver, right?</blockquote><div> Will remove. </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> Also, does the libxl_device_pci need disposed? <div class="im"> </div></blockquote><div> libxl_device_pci_dispose, not necessary (only does memset work) but better to keep code more readable.Will update. </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <div class="im"> > + > + return 0; > + > +cleanup: > + virDomainHostdevDefFree(detach); > + return -1; > +} > + > +static int libxlDomainDetachHostDevice(libxlDomainObjPrivatePtr priv, > + virDomainObjPtr vm, > + virDomainDeviceDefPtr dev) > </div>Return type and function name on different lines. <div class="im"> > +{ > + 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); > </div>Line between cases. <div class="im"> > + 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; > - > </div>Spurious whitespace change. <div class="im"> > + case VIR_DOMAIN_DEVICE_HOSTDEV: > + ret = libxlDomainDetachHostDevice(priv, vm, dev); > + break; > </div>Line between cases. Regards, Jim <div class=""><div class="h5"> > 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 */ > </div></div></blockquote></div> </div></div>