On Mon, Sep 03, 2012 at 04:57:01PM +0800, Osier Yang wrote:
> Previously it refuses to do the secondary bus reset as long as
> there is(are) devices/functions behind the same bus, regardless
> of whether the devices/functions are being used or not. And it
> only save and restore the device itself's PCI config space.
>
> But later it was changed to allow the secondary bus reset as long
> as the devices/functions behind the same bus are not being
> used. Unfortunately, it still just saves and restores the device
> itself's PCI config space. It means we will lose the PCI config
> space for the devices share same bus when doing passthrough.
>
> Also, (hope my guess is right) as it assumes the secondary reset
> is allowed unless the device doesn't have devices/functions behind
> the same bus, so it only reads the bridge control register from the
> device, but not the parent.
>
> This patch fixes the problem by finding out all the devices/functions
> behind the same bus of the device to be reset, and save/restore
> PCI config space for all of them. And read the bridge control register
> from the device's parent (bridge) before resetting.
>
> * src/util/pci.c:
> - New helper pciSharesBus to check if two devices share same bus.
>
> - New helper pciDevicesShareBus to return a list containg all of
> the devices/functions which share same bus with the device
>
> - pciTrySecondaryBusReset: Save and restore PCI config space for
> all the devices/functions behind the same bus; Read the bridge
> control register from the device's parent instead before resetting.
> ---
> src/util/pci.c | 80 ++++++++++++++++++++++++++++++++++++++++++++++++++++---
> 1 files changed, 75 insertions(+), 5 deletions(-)
>
> diff --git a/src/util/pci.c b/src/util/pci.c
> index 0742d07..1a9777a 100644
> --- a/src/util/pci.c
> +++ b/src/util/pci.c
> @@ -517,6 +517,39 @@ pciBusContainsActiveDevices(pciDevice *dev,
> return active;
> }
>
> +/*
> + * Check if the @dev and @check share bus.
> + */
> +static int
> +pciSharesBus(pciDevice *dev, pciDevice *check, void *data ATTRIBUTE_UNUSED)
> +{
> + if ((dev->domain == check->domain) &&
> + (dev->bus == check->bus) &&
> + (dev->slot == check->slot))
> + return 1;
> +
> + return 0;
> +}
I'm a bit puzzled ... seems to me the function check if the devices
share the same slot on the same bus, not just sharing of the same bus
> +/*
> + * Return all the devices/functions share same bus with @dev
> + * as a list.
> + */
> +static pciDeviceList *
> +pciDevicesShareBus(pciDevice *dev)
> +{
> + pciDevice *match = NULL;
> + pciDeviceList *pcis = NULL;
> +
> + if (!(pcis = pciDeviceListNew()))
> + return NULL;
> +
> + if (pciIterDevices(pciSharesBus, dev, &match, NULL))
> + pciDeviceListAdd(pcis, match);
> +
> + return pcis;
> +}
> +
> /* Is @check the parent of @dev ? */
> static int
> pciIsParent(pciDevice *dev, pciDevice *check, void *data)
> @@ -604,6 +637,9 @@ pciTrySecondaryBusReset(pciDevice *dev,
> uint8_t config_space[PCI_CONF_LEN];
> uint16_t ctl;
> int ret = -1;
> + pciDeviceList *list = NULL;
> + uint8_t (*config_spaces)[PCI_CONF_LEN];
> + int i;
>
> /* Refuse to do a secondary bus reset if there are other
> * devices/functions behind the bus are used by the host
> @@ -628,10 +664,7 @@ pciTrySecondaryBusReset(pciDevice *dev,
>
> VIR_DEBUG("%s %s: doing a secondary bus reset", dev->id, dev->name);
>
> - /* Save and restore the device's config space; we only do this
> - * for the supplied device since we refuse to do a reset if there
> - * are multiple devices/functions
> - */
> + /* Save the device's config space */
> if (pciRead(dev, 0, config_space, PCI_CONF_LEN) < 0) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("Failed to read PCI config space for %s"),
> @@ -639,10 +672,29 @@ pciTrySecondaryBusReset(pciDevice *dev,
> goto out;
> }
>
> + /* Save the config space of devices behind the same bus */
> + if ((list = pciDevicesShareBus(dev))) {
> + if (VIR_ALLOC_N(config_spaces, list->count) < 0) {
> + virReportOOMError();
> + goto out;
> + }
> +
> + for (i = 0; i < list->count; i++) {
> + pciDevice *pci = list->devs[i];
> +
> + if (pciRead(pci, 0, config_spaces[i], PCI_CONF_LEN) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Failed to read PCI config space for %s"),
> + pci->name);
> + goto out;
> + }
> + }
> + }
> +
> /* Read the control register, set the reset flag, wait 200ms,
> * unset the reset flag and wait 200ms.
> */
> - ctl = pciRead16(dev, PCI_BRIDGE_CONTROL);
> + ctl = pciRead16(parent, PCI_BRIDGE_CONTROL);
>
> pciWrite16(parent, PCI_BRIDGE_CONTROL, ctl | PCI_BRIDGE_CTL_RESET);
>
> @@ -652,14 +704,32 @@ pciTrySecondaryBusReset(pciDevice *dev,
>
> usleep(200 * 1000); /* sleep 200ms */
>
> + /* Restore the device's config space */
> if (pciWrite(dev, 0, config_space, PCI_CONF_LEN) < 0) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("Failed to restore PCI config space for %s"),
> dev->name);
> goto out;
> }
> +
> + /* Restore the config space of devices behind the same bus */
> + if (list) {
> + for (i = 0; i < list->count; i++) {
> + pciDevice *pci = list->devs[i];
> +
> + if (pciWrite(pci, 0, config_spaces[i], PCI_CONF_LEN) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Failed to restore PCI config space for %s"),
> + pci->name);
> + goto out;
> + }
> + }
> + }
> +
> ret = 0;
> out:
> + pciDeviceListFree(list);
> + VIR_FREE(config_spaces);
> pciFreeDevice(parent);
> return ret;
> }
Otherwise this seems to make sense to me, but it would be good
if someone else could review too before ACK'ing as this is rather
sensitive code
ACK/2
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel@xxxxxxxxxxxx | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list