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

Wednesday, 14 August 2013

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)
 +        return -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?  Also, does
the libxl_device_pci need disposed?

...
 +
 +    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 */

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

2010

2009

2008

2007

2006

2005

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