Re: [libvirt] [PATCHv2 08/12] qemu: don't reset PCI devices being assigned with VFIO

On Tue, Jun 25, 2013 at 01:06:55PM -0400, Laine Stump wrote: > On 06/25/2013 06:44 AM, Daniel P. Berrange wrote: >> On Mon, Jun 24, 2013 at 11:05:34PM -0400, Laine Stump wrote: >>> I just learned that VFIO resets PCI devices when they are assigned to >>> guests / returned to the host, so it is redundant for libvirt to reset >>> the devices. This patch inhibits calling virPCIDeviceReset to devices >>> that will be/were assigned using VFIO. >>> --- >>> src/qemu/qemu_hostdev.c | 4 ++++ >>> src/qemu/qemu_hotplug.c | 5 +++-- >>> 2 files changed, 7 insertions(+), 2 deletions(-) >>> >>> diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c >>> index dfe39c6..d7d54d7 100644 >>> --- a/src/qemu/qemu_hostdev.c >>> +++ b/src/qemu/qemu_hostdev.c >>> @@ -548,6 +548,8 @@ int qemuPrepareHostdevPCIDevices(virQEMUDriverPtr driver, >>> * can safely reset them */ >>> for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { >>> virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i); >>> + if (STREQ_NULLABLE(virPCIDeviceGetStubDriver(dev), "vfio-pci")) >>> + continue; >>> if (virPCIDeviceReset(dev, driver->activePciHostdevs, >>> driver->inactivePciHostdevs) < 0) >>> goto reattachdevs; >>> @@ -1114,6 +1116,8 @@ void qemuDomainReAttachHostdevDevices(virQEMUDriverPtr driver, >>> >>> for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { >>> virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i); >>> + if (STREQ_NULLABLE(virPCIDeviceGetStubDriver(dev), "vfio-pci")) >>> + continue; >>> if (virPCIDeviceReset(dev, driver->activePciHostdevs, >>> driver->inactivePciHostdevs) < 0) { >>> virErrorPtr err = virGetLastError(); >>> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c >>> index 18f5fa5..46875ad 100644 >>> --- a/src/qemu/qemu_hotplug.c >>> +++ b/src/qemu/qemu_hotplug.c >>> @@ -2528,8 +2528,9 @@ qemuDomainDetachHostPciDevice(virQEMUDriverPtr driver, >>> if (pci) { >>> activePci = virPCIDeviceListSteal(driver->activePciHostdevs, pci); >>> if (activePci && >>> - virPCIDeviceReset(activePci, driver->activePciHostdevs, >>> - driver->inactivePciHostdevs) == 0) { >>> + (subsys->u.pci.backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO || >>> + virPCIDeviceReset(activePci, driver->activePciHostdevs, >>> + driver->inactivePciHostdevs) == 0)) { >> I'm guessing that virPCIDeviceGetStubDriver() isn't returning 'pci-stub' >> here, right ? Otherwise you'd would have used the same pattern as earlier > No, the stub driver should be set correctly at this time[*]. It was just > a case of having multiple options for learning the answer in this case, > and choosing the one that's more efficient. Ok, so where I was going here, was that if the the virPCIDevicePtr object has enough info, then could we make virPCIDeviceReset() look for stub == vfio-pci, and make itself a nop. This avoids the risk of one caller forgetting to do the check itself.