[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