[libvirt] [PATCH v12 04/11] qemu_hostdev: parse BACKEND_DEFAULT outside

Chunyan Liu cyliu at suse.com
Thu Feb 20 07:26:14 UTC 2014


2014-02-17 23:26 GMT+08:00 Laine Stump <laine at laine.org>:

> On 02/17/2014 11:38 AM, Cedric Bosdonnat wrote:
> > On Mon, 2014-02-17 at 14:32 +0800, Chunyan Liu wrote:
> >> For extracting hostdev codes from qemu_hostdev.c to common library,
> change
> >> original paring VIR_DOMAIN_HOSTDEV_PCI_BACKEND_DEFAULT in hostdev
> function to
> >> qemuDomainDeviceDefPostParse.
> > typo: paring -> parsing.
> >
> >> Signed-off-by: Chunyan Liu <cyliu at suse.com>
> >> ---
> >>  src/qemu/qemu_domain.c                             |   22
> +++++++++++++++
> >>  src/qemu/qemu_hostdev.c                            |   28
> +++-----------------
> >>  src/qemu/qemu_hostdev.h                            |    2 -
> >>  src/qemu/qemu_hotplug.c                            |    2 +-
> >>  src/qemu/qemu_process.c                            |    3 +-
> >>  .../qemuxml2argv-hostdev-pci-address.xml           |    1 +
> >>  .../qemuxml2argvdata/qemuxml2argv-net-hostdev.xml  |    1 +
> >>  tests/qemuxml2argvdata/qemuxml2argv-pci-rom.xml    |    2 +
> >>  8 files changed, 32 insertions(+), 29 deletions(-)
> >>
> >> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> >> index a665061..55e707e 100644
> >> --- a/src/qemu/qemu_domain.c
> >> +++ b/src/qemu/qemu_domain.c
> >> @@ -38,6 +38,7 @@
> >>  #include "virtime.h"
> >>  #include "virstoragefile.h"
> >>  #include "virstring.h"
> >> +#include "qemu_hostdev.h"
> >>
> >>  #include <sys/time.h>
> >>  #include <fcntl.h>
> >> @@ -821,6 +822,7 @@ qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr
> dev,
> >>      int ret = -1;
> >>      virQEMUDriverPtr driver = opaque;
> >>      virQEMUDriverConfigPtr cfg = NULL;
> >> +    virQEMUCapsPtr qemuCaps = NULL;
> >>
> >>      if (dev->type == VIR_DOMAIN_DEVICE_NET &&
> >>          dev->data.net->type != VIR_DOMAIN_NET_TYPE_HOSTDEV &&
> >> @@ -899,6 +901,26 @@ qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr
> dev,
> >>          dev->data.chr->source.data.nix.listen = true;
> >>      }
> >>
> >> +    if (dev->type == VIR_DOMAIN_DEVICE_HOSTDEV) {
> >> +        virDomainHostdevDefPtr hostdev = dev->data.hostdev;
> >> +        if (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS &&
> >> +            hostdev->source.subsys.type ==
> VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI &&
> >> +            hostdev->source.subsys.u.pci.backend ==
> VIR_DOMAIN_HOSTDEV_PCI_BACKEND_DEFAULT) {
> >> +
> >> +            hostdev->source.subsys.u.pci.backend =
> VIR_DOMAIN_HOSTDEV_PCI_BACKEND_KVM;
> >> +            if (driver && driver->qemuCapsCache) {
> >> +                bool supportsPassthroughVFIO =
> qemuHostdevHostSupportsPassthroughVFIO();
> >> +                qemuCaps =
> virQEMUCapsCacheLookupCopy(driver->qemuCapsCache,
> >> +                                                      def->emulator);
> >> +                if (supportsPassthroughVFIO && qemuCaps &&
> >> +                    virQEMUCapsGet(qemuCaps,
> QEMU_CAPS_DEVICE_VFIO_PCI))
> >> +                    hostdev->source.subsys.u.pci.backend =
> VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO;
> >> +
> >> +                virObjectUnref(qemuCaps);
> >> +            }
> >> +        }
> >> +    }
> >> +
> >>      ret = 0;
> > KVM passthrough support isn't checked here but was checked in the
> > removed code, is that intended?
>
> The fact that the code doing the KVM check is fairly new, suggests to me
> that this omission *wasn't* intentional. I'm really glad that you're
> looking at that in detail, because it's the potential omission of recent
> bugfixes that has me nervous.
>
> Beyond that, this isn't the proper place to be moving this to - anything
> that is done by the PostParse callback function ends up being written to
> persistent config and is there effectively forever, but the
> interpretation of the hostdev driver "default" backend is something that
> must be re-done exactly at the moment the device it going to be
> attached. This patch instead causes that decision to be made when the
> domain is defined, and forever encoded in the config.
>
> I think that instead you need to either call it from
> qemuPrepareHostDevices() and qemuDomainAttachHostPciDevice(), OR send a
> callback function pointer into your new virHostdevPreparePciHostdevs(),
> with that function returning the "backend to use this time" based on the
> config setting and current system state.
>
>
Thanks for explanation. Will correct that.


>
> >>  cleanup:
> >> diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c
> >> index ce5012d..80552cd 100644
> >> --- a/src/qemu/qemu_hostdev.c
> >> +++ b/src/qemu/qemu_hostdev.c
> >> @@ -583,8 +583,7 @@ qemuHostdevHostSupportsPassthroughLegacy(void)
> >>
> >>  static bool
> >>  qemuPrepareHostdevPCICheckSupport(virDomainHostdevDefPtr *hostdevs,
> >> -                                  size_t nhostdevs,
> >> -                                  virQEMUCapsPtr qemuCaps)
> >> +                                  size_t nhostdevs)
> >>  {
> >>      bool supportsPassthroughKVM =
> qemuHostdevHostSupportsPassthroughLegacy();
> >>      bool supportsPassthroughVFIO =
> qemuHostdevHostSupportsPassthroughVFIO();
> >> @@ -601,23 +600,6 @@
> qemuPrepareHostdevPCICheckSupport(virDomainHostdevDefPtr *hostdevs,
> >>              continue;
> >>
> >>          switch ((virDomainHostdevSubsysPciBackendType) *backend) {
> >> -        case VIR_DOMAIN_HOSTDEV_PCI_BACKEND_DEFAULT:
> >> -            if (supportsPassthroughVFIO &&
> >> -                virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VFIO_PCI)) {
> >> -                *backend = VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO;
> >> -            } else if (supportsPassthroughKVM &&
> >> -                       (virQEMUCapsGet(qemuCaps, QEMU_CAPS_PCIDEVICE)
> ||
> >> -                        virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE))) {
> >> -                *backend = VIR_DOMAIN_HOSTDEV_PCI_BACKEND_KVM;
> >> -            } else {
> >> -                virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> >> -                               _("host doesn't support passthrough of "
> >> -                                 "host PCI devices"));
> >> -                return false;
> >> -            }
> >> -
> >> -            break;
> >> -
> >>          case VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO:
> >>              if (!supportsPassthroughVFIO) {
> >>                  virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> >> @@ -635,7 +617,7 @@
> qemuPrepareHostdevPCICheckSupport(virDomainHostdevDefPtr *hostdevs,
> >>
> >>              break;
> >>
> >> -        case VIR_DOMAIN_HOSTDEV_PCI_BACKEND_TYPE_LAST:
> >> +        default:
>
>
> I'm pretty sure that Peter intentionally *didn't* put a default case
> into this switch (and many others) specifically so that somebody adding
> a new enum value would get compiler errors telling them all the places
> they needed to handle the new case, i.e. please do not replace "case
> VIR_DOMAIN_HOSTDEV_PCI_BACKEND_TYPE_LAST" with "default".
>
>
> >>              break;
> >>          }
> >>      }
> >> @@ -650,7 +632,6 @@ qemuPrepareHostdevPCIDevices(virQEMUDriverPtr
> driver,
> >>                               const unsigned char *uuid,
> >>                               virDomainHostdevDefPtr *hostdevs,
> >>                               int nhostdevs,
> >> -                             virQEMUCapsPtr qemuCaps,
> >>                               unsigned int flags)
> >>  {
> >>      virPCIDeviceListPtr pcidevs = NULL;
> >> @@ -659,7 +640,7 @@ qemuPrepareHostdevPCIDevices(virQEMUDriverPtr
> driver,
> >>      int ret = -1;
> >>      virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
> >>
> >> -    if (!qemuPrepareHostdevPCICheckSupport(hostdevs, nhostdevs,
> qemuCaps))
> >> +    if (!qemuPrepareHostdevPCICheckSupport(hostdevs, nhostdevs))
> >>          goto cleanup;
> >>
> >>      virObjectLock(driver->activePciHostdevs);
> >> @@ -1189,7 +1170,6 @@ cleanup:
> >>  int
> >>  qemuPrepareHostDevices(virQEMUDriverPtr driver,
> >>                         virDomainDefPtr def,
> >> -                       virQEMUCapsPtr qemuCaps,
> >>                         unsigned int flags)
> >>  {
> >>      if (!def->nhostdevs)
> >> @@ -1197,7 +1177,7 @@ qemuPrepareHostDevices(virQEMUDriverPtr driver,
> >>
> >>      if (qemuPrepareHostdevPCIDevices(driver, def->name, def->uuid,
> >>                                       def->hostdevs, def->nhostdevs,
> >> -                                     qemuCaps, flags) < 0)
> >> +                                     flags) < 0)
> >>          return -1;
> >>
> >>      if (qemuPrepareHostUSBDevices(driver, def, flags) < 0)
> >> diff --git a/src/qemu/qemu_hostdev.h b/src/qemu/qemu_hostdev.h
> >> index 710867d..128032d 100644
> >> --- a/src/qemu/qemu_hostdev.h
> >> +++ b/src/qemu/qemu_hostdev.h
> >> @@ -45,7 +45,6 @@ int qemuPrepareHostdevPCIDevices(virQEMUDriverPtr
> driver,
> >>                                   const unsigned char *uuid,
> >>                                   virDomainHostdevDefPtr *hostdevs,
> >>                                   int nhostdevs,
> >> -                                 virQEMUCapsPtr qemuCaps,
> >>                                   unsigned int flags);
> >>  int qemuFindHostdevUSBDevice(virDomainHostdevDefPtr hostdev,
> >>                               bool mandatory,
> >> @@ -59,7 +58,6 @@ int qemuPrepareHostdevSCSIDevices(virQEMUDriverPtr
> driver,
> >>                                    int nhostdevs);
> >>  int qemuPrepareHostDevices(virQEMUDriverPtr driver,
> >>                             virDomainDefPtr def,
> >> -                           virQEMUCapsPtr qemuCaps,
> >>                             unsigned int flags);
> >>  void qemuDomainReAttachHostScsiDevices(virQEMUDriverPtr driver,
> >>                                         const char *name,
> >> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> >> index c47c5e8..8486f25 100644
> >> --- a/src/qemu/qemu_hotplug.c
> >> +++ b/src/qemu/qemu_hotplug.c
> >> @@ -1163,7 +1163,7 @@ qemuDomainAttachHostPciDevice(virQEMUDriverPtr
> driver,
> >>      if (!cfg->relaxedACS)
> >>          flags |= VIR_STRICT_ACS_CHECK;
> >>      if (qemuPrepareHostdevPCIDevices(driver, vm->def->name,
> vm->def->uuid,
> >> -                                     &hostdev, 1, priv->qemuCaps,
> flags) < 0)
> >> +                                     &hostdev, 1, flags) < 0)
> >>          return -1;
> >>
> >>      /* this could have been changed by qemuPrepareHostdevPCIDevices */
> >> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> >> index f1fe35e..e938649 100644
> >> --- a/src/qemu/qemu_process.c
> >> +++ b/src/qemu/qemu_process.c
> >> @@ -3690,8 +3690,7 @@ int qemuProcessStart(virConnectPtr conn,
> >>          hostdev_flags |= VIR_STRICT_ACS_CHECK;
> >>      if (!migrateFrom)
> >>          hostdev_flags |= VIR_COLD_BOOT;
> >> -    if (qemuPrepareHostDevices(driver, vm->def, priv->qemuCaps,
> >> -                                                    hostdev_flags) < 0)
> >> +    if (qemuPrepareHostDevices(driver, vm->def, hostdev_flags) < 0)
> >>          goto cleanup;
> >>
> >>      VIR_DEBUG("Preparing chr devices");
> >> diff --git
> a/tests/qemuxml2argvdata/qemuxml2argv-hostdev-pci-address.xml
> b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-pci-address.xml
> >> index 422127c..b9a221a 100644
> >> --- a/tests/qemuxml2argvdata/qemuxml2argv-hostdev-pci-address.xml
> >> +++ b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-pci-address.xml
> >> @@ -24,6 +24,7 @@
> >>      <controller type='ide' index='0'/>
> >>      <controller type='pci' index='0' model='pci-root'/>
> >>      <hostdev mode='subsystem' type='pci' managed='yes'>
> >> +      <driver name='kvm'/>
> >>        <source>
> >>          <address domain='0x0000' bus='0x06' slot='0x12'
> function='0x5'/>
> >>        </source>
> >> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-net-hostdev.xml
> b/tests/qemuxml2argvdata/qemuxml2argv-net-hostdev.xml
> >> index d65ef87..9e79348 100644
> >> --- a/tests/qemuxml2argvdata/qemuxml2argv-net-hostdev.xml
> >> +++ b/tests/qemuxml2argvdata/qemuxml2argv-net-hostdev.xml
> >> @@ -24,6 +24,7 @@
> >>      <controller type='pci' index='0' model='pci-root'/>
> >>      <interface type='hostdev' managed='yes'>
> >>        <mac address='00:11:22:33:44:55'/>
> >> +      <driver name='kvm'/>
> >>        <source>
> >>          <address type='pci' domain='0x0002' bus='0x03' slot='0x07'
> function='0x1'/>
> >>        </source>
> >> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pci-rom.xml
> b/tests/qemuxml2argvdata/qemuxml2argv-pci-rom.xml
> >> index a5e59b2..924842b 100644
> >> --- a/tests/qemuxml2argvdata/qemuxml2argv-pci-rom.xml
> >> +++ b/tests/qemuxml2argvdata/qemuxml2argv-pci-rom.xml
> >> @@ -33,12 +33,14 @@
> >>        <rom file='/etc/fake/bootrom.bin'/>
> >>      </interface>
> >>      <hostdev mode='subsystem' type='pci' managed='yes'>
> >> +      <driver name='kvm'/>
> >>        <source>
> >>          <address domain='0x0000' bus='0x06' slot='0x12'
> function='0x5'/>
> >>        </source>
> >>        <rom bar='off'/>
> >>      </hostdev>
> >>      <hostdev mode='subsystem' type='pci' managed='yes'>
> >> +      <driver name='kvm'/>
> >>        <source>
> >>          <address domain='0x0000' bus='0x06' slot='0x12'
> function='0x6'/>
> >>        </source>
> > Can't those hostdev definitions keep the default backend like before?
>
> A changes in the test case is sometimes correct, but should be
> considered a red flag. My first guest would be that it was necessary due
> to the code that was added to the PostParse function (i.e. it's
> indicating a problem)
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140220/f596df48/attachment-0001.htm>


More information about the libvir-list mailing list