<div dir="ltr"> <div class="gmail_extra"> <div class="gmail_quote">2014-02-17 23:26 GMT+08:00 Laine Stump <<a href="mailto:laine@laine.org" target="_blank">laine@laine.org</a>>: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div class="HOEnZb"><div class="h5">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 <<a href="mailto:cyliu@suse.com">cyliu@suse.com</a>> >> --- >> 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? </div></div>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. <div><div class="h5"> </div></div></blockquote><div> </div><div>Thanks for explanation. Will correct that. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div><div class="h5"> >> 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: </div></div>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". <div><div class="h5"> >> 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? </div></div>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) </blockquote></div> </div></div>