[libvirt] [PATCHv3 2/4] add pci passthrough to libxl driver
Chunyan Liu
cyliu at suse.com
Fri Aug 16 06:02:18 UTC 2013
2013/8/14 Jim Fehlig <jfehlig at suse.com>
> cyliu at suse.com wrote:
> > From: Chunyan Liu <cyliu at suse.com>
> >
> > 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 at suse.com>
> > ---
> > 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?
>
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."
But in qemu driver, there isn't limit to ATTACH a multi-function PCI
device, only limit to DETACH.
> + 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?
Will remove.
Also, does
> the libxl_device_pci need disposed?
>
> libxl_device_pci_dispose, not necessary (only does memset work) but
better to keep code more readable.Will update.
> +
> > + 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 */
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20130816/3c25fa44/attachment-0001.htm>
More information about the libvir-list
mailing list