[libvirt] [PATCH v2] qemu: Introduce inactive PCI device list

[Osier Yang]

On 2012年01月18日 08:14, Eric Blake wrote:
> On 01/17/2012 01:02 PM, Osier Yang wrote:
>> pciTrySecondaryBusReset checks if there is active device on the
>> same bus, however, qemu driver doesn't maintain an effective
>> list for the inactive devices, and it passes meaningless argment
>
> s/argment/argument/
>
>> for parameter "inactiveDevs". e.g. (qemuPrepareHostdevPCIDevices)
>>
>> if (!(pcidevs = qemuGetPciHostDeviceList(hostdevs, nhostdevs)))
>>      return -1;
>>
>> ...skipped...
>>
>> if (pciResetDevice(dev, driver->activePciHostdevs, pcidevs)<  0)
>>      goto reattachdevs;
>>
>> NB, the "pcidevs" used above are extracted from domain def, and
>> thus one won't be able to attach a device of which bus has other
>> device even detached from host (nodedev-detach). To see more
>> details of the problem:
>>
>> RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=773667
>>
>> This patch is to resolve the problem by introduce an inactive
>
> s/introduce/introducing/
>
>> PCI device list (just like qemu_driver->activePciHostdevs), and
>> the whole logic is:
>>
>>    * Add the device to inactive list when do nodedev-dettach
>
> s/when do/during/
>
>>    * Remove the device from inactive list when do nodedev-reattach
>>    * Remove the device from inactive list when do attach-device
>>      (for not managed device)
>>    * Add the device to inactive list after detach-device, only
>>      if the device is not managed
>>
>> With above, we have sufficient inactive PCI device list, and thus
>> we can use it for pciResetDevice. e.g.(qemuPrepareHostdevPCIDevices)
>>
>> if (pciResetDevice(dev, driver->activePciHostdevs,
>>                     driver->inactivePciHostdevs)<  0)
>>      goto reattachdevs;
>>
>> v1 ~ v2
>>
>> Fixed a potential crash.
>> ---
>> Got a testing box from Miroslav and tested the patch, it works well.
>
> I'm glad you were able to test it; I tried to reproduce things locally,
> but didn't quite have the right hardware, so I'm reviewing this on
> inspection alone.  But it is a serious enough issue that I'm okay
> pushing once the nits are fixed.
>
>> ---
>>   src/qemu/qemu_conf.h    |    5 +++++
>>   src/qemu/qemu_driver.c  |   19 +++++++++++++++----
>>   src/qemu/qemu_hostdev.c |   32 +++++++++++++++++++++++---------
>>   src/util/pci.c          |   28 ++++++++++++++++++++++++----
>>   src/util/pci.h          |    8 ++++++--
>>   src/xen/xen_driver.c    |    4 ++--
>>   7 files changed, 77 insertions(+), 22 deletions(-)
>>
>> diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
>> index d8d7915..3f1b668 100644
>> --- a/src/qemu/qemu_conf.h
>> +++ b/src/qemu/qemu_conf.h
>> @@ -128,6 +128,11 @@ struct qemud_driver {
>>       pciDeviceList *activePciHostdevs;
>>       usbDeviceList *activeUsbHostdevs;
>>
>> +    /* The device which is not used by both host
>> +     * and any guest.
>
> The devices which are not in use by the host or any guest.
>
>> @@ -778,6 +781,7 @@ qemudShutdown(void) {
>>
>>       qemuDriverLock(qemu_driver);
>>       pciDeviceListFree(qemu_driver->activePciHostdevs);
>> +    pciDeviceListFree(qemu_driver->inactivePciHostdevs);
>>       usbDeviceListFree(qemu_driver->activeUsbHostdevs);
>
> We'll probably have to repeat this exercise for USB passthrough devices,
> but that can be a separate patch.
>
>> @@ -445,8 +452,13 @@ void qemuReattachPciDevice(pciDevice *dev, struct qemud_driver *driver)
>>   {
>>       int retries = 100;
>>
>> -    if (!pciDeviceGetManaged(dev))
>> +    /* If the device is not managed and was attached to guest
>> +     * successfully, it must had be inactive.
>
> s/had be/have been/
>
> Overall, the design looks sound.  I squashed in your followup, plus
> this, then pushed:
>

Thanks.

Osier