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@xxxxxxxx>
>> ---
>> 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.
>> 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)
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list