On Thu, Aug 13, 2009 at 05:44:33PM +0100, Mark McLoughlin wrote:
> It turns out that a PCI Power Management reset only affects individual
> functions, and not the whole device.
>
> The PCI Power Management spec talks about resetting the 'device' rather
> than the 'function', but Intel's Dexuan Cui informs me that it is
> actually a per-function reset.
>
> Also, Yu Zhao has added pci_pm_reset() to the kernel, and it doesn't
> reject multi-function devices, so it must be true! :-)
>
> (A side issue is that we could defer the PM reset to the kernel if we
> could detect that the kernel has PM reset support, but barring version
> number checks we don't have a way to detect that support)
>
> * src/pci.c: remove the pciDeviceContainsOtherFunctions() check from
> pciTryPowerManagementReset() and prefer PM reset over bus reset
> where both are available
>
> Cc: Cui, Dexuan <dexuan.cui@xxxxxxxxx>
> Cc: Yu Zhao <yu.zhao@xxxxxxxxx>
> ---
> src/pci.c | 48 +++++++++---------------------------------------
> 1 files changed, 9 insertions(+), 39 deletions(-)
>
> diff --git a/src/pci.c b/src/pci.c
> index 2dc2e1c..11b3e8b 100644
> --- a/src/pci.c
> +++ b/src/pci.c
> @@ -402,29 +402,6 @@ pciBusContainsOtherDevices(virConnectPtr conn, pciDevice *dev)
> return 1;
> }
>
> -/* Any other functions on this device ? */
> -static int
> -pciSharesDevice(pciDevice *a, pciDevice *b)
> -{
> - return
> - a->domain == b->domain &&
> - a->bus == b->bus &&
> - a->slot == b->slot &&
> - a->function != b->function;
> -}
> -
> -static int
> -pciDeviceContainsOtherFunctions(virConnectPtr conn, pciDevice *dev)
> -{
> - pciDevice *matched = NULL;
> - if (pciIterDevices(conn, pciSharesDevice, dev, &matched) < 0)
> - return 1;
> - if (!matched)
> - return 0;
> - pciFreeDevice(conn, matched);
> - return 1;
> -}
> -
> /* Is @a the parent of @b ? */
> static int
> pciIsParent(pciDevice *a, pciDevice *b)
> @@ -529,7 +506,7 @@ out:
> * above we require the device supports a full internal reset.
> */
> static int
> -pciTryPowerManagementReset(virConnectPtr conn, pciDevice *dev)
> +pciTryPowerManagementReset(virConnectPtr conn ATTRIBUTE_UNUSED, pciDevice *dev)
> {
> uint8_t config_space[PCI_CONF_LEN];
> uint32_t ctl;
> @@ -537,16 +514,6 @@ pciTryPowerManagementReset(virConnectPtr conn, pciDevice *dev)
> if (!dev->pci_pm_cap_pos)
> return -1;
>
> - /* For now, we just refuse to do a power management reset
> - * if there are other functions on this device.
> - * In future, we could allow it so long as those functions
> - * are not in use by the host or other guests.
> - */
> - if (pciDeviceContainsOtherFunctions(conn, dev)) {
> - VIR_WARN("%s contains other functions, not resetting", dev->name);
> - return -1;
> - }
> -
> /* Save and restore the device's config space. */
> if (pciRead(dev, 0, &config_space[0], PCI_CONF_LEN) < 0) {
> VIR_WARN("Failed to save PCI config space for %s", dev->name);
> @@ -604,14 +571,17 @@ pciResetDevice(virConnectPtr conn, pciDevice *dev)
> if (dev->has_flr)
> return 0;
>
> + /* If the device supports PCI power management reset,
> + * that's the next best thing because it only resets
> + * the function, not the whole device.
> + */
> + if (dev->has_pm_reset)
> + ret = pciTryPowerManagementReset(conn, dev);
> +
> /* Bus reset is not an option with the root bus */
> - if (dev->bus != 0)
> + if (ret < 0 && dev->bus != 0)
> ret = pciTrySecondaryBusReset(conn, dev);
>
> - /* Next best option is a PCI power management reset */
> - if (ret < 0 && dev->has_pm_reset)
> - ret = pciTryPowerManagementReset(conn, dev);
> -
> if (ret < 0)
> pciReportError(conn, VIR_ERR_NO_SUPPORT,
> _("No PCI reset capability available for %s"),
ACK
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list