[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] pci: Save and restore each devices/functions behind the bus



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 veillard com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]