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