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

[Laine Stump]

On 06/25/2013 01:09 PM, Daniel P. Berrange wrote:
> 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.

Interesting idea, and it will work as long as:

1) we only reset the device while it's bound to the stub driver (not,
for example, just after it's been unbound from vfio-pci and bound to the
host driver).

and

2) we really never want to reset a device that is bound to vfio-pci.

I just double-checked, and as far as I can see, both of these are true,
so we can do it. I'll make a patch to do that as soon as I finish
cleaning up the existing patches.