cyliu@suse.com wrote:Return type and function name on separate lines.
> From: Chunyan Liu <cyliu@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@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)
>
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 = 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();
>
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?
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");
> +*/
>
Also, does
the libxl_device_pci need disposed?
> +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 */
>