2013/8/14 Jim Fehlig <jfehlig@xxxxxxxx>
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."
cyliu@xxxxxxxx wrote:Return type and function name on separate lines.
> From: Chunyan Liu <cyliu@xxxxxxxx>
>
> 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@xxxxxxxx>
> ---
> 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)
>
No need to report OOM error.
> +{
> + 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 for the braces. From HACKING:
> + 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]);
> + }
>
Omit the curly braces around an "if", "while", "for" etc. body only when
that body occupies a single line.
Same here, no need for the braces. (I should put together a cleanup
> + 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;
> + }
> +
>
patch to fix the existing offenses in this file.)
Spurious whitespace change.
> 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"
>
> -
>
Same here. I'd prefer a line between all these function declarations.
> +# 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);
> -
>
Parameter indentation looks wrong.
> +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);
>
No need to report OOM error.
> +
> 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 = ""> > + 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();
>
Looks like this should be removed from the libxl driver :).
> + 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;
> +*/
>
Again, don't think this is needed in the libxl driver. Also, there is
> + 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;
>
nothing to cleanup so might as well just return -1 when errors are
encountered.
Spurious whitespace change.
> +}
> +
> +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;
> -
>
Generally there is a line between case statements, including the default
> + case VIR_DOMAIN_DEVICE_HOSTDEV:
> + ret = libxlDomainAttachHostDevice(priv, vm, dev);
> + if (!ret)
> + dev->data.hostdev = NULL;
> + break;
>
case.
Same here, line before the next 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;
>
Hmm, is a similar check needed in the libxl driver? It looks to me like
> + 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;
> + }
> +*/
>
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.
I don't think this is needed for the libxl driver, right?> + 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");
> +*/
>
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 type and function name on different lines.
> + return 0;
> +
> +cleanup:
> + virDomainHostdevDefFree(detach);
> + return -1;
> +}
> +
> +static int libxlDomainDetachHostDevice(libxlDomainObjPrivatePtr priv,
> + virDomainObjPtr vm,
> + virDomainDeviceDefPtr dev)
>
Line between cases.
> +{
> + 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);
>
Spurious whitespace change.
> + 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;
> -
>
Line between cases.
> + case VIR_DOMAIN_DEVICE_HOSTDEV:
> + ret = libxlDomainDetachHostDevice(priv, vm, dev);
> + break;
>
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 */
>
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list