Re: [Xen-devel] [PATCH v5 7/7] VT-d: Fix vt-d Device-TLB flush timeout issue.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 05.02.16 at 11:18, <quan.xu@xxxxxxxxx> wrote:
> --- a/xen/drivers/passthrough/vtd/qinval.c
> +++ b/xen/drivers/passthrough/vtd/qinval.c
> @@ -229,6 +229,69 @@ int qinval_device_iotlb(struct iommu *iommu,
>      return 0;
>  }
>  
> +static void dev_invalidate_iotlb_timeout(struct iommu *iommu, u16 did,
> +                                         u16 seg, u8 bus, u8 devfn,
> +                                         unsigned int lock)
> +{
> +    struct domain *d = NULL;
> +    struct pci_dev *pdev;
> +
> +    if ( test_bit(did, iommu->domid_bitmap) )
> +        d = rcu_lock_domain_by_id(iommu->domid_map[did]);
> +
> +    if ( d == NULL )
> +        return;
> +
> +    for_each_pdev(d, pdev)
> +    {
> +        if ( (pdev->seg == seg) &&
> +             (pdev->bus == bus) &&
> +             (pdev->devfn == devfn) )
> +        {
> +            ASSERT ( pdev->domain );

Stray blanks inside the parentheses.

> +            list_del(&pdev->domain_list);
> +            pdev->domain = NULL;
> +
> +            if ( !(lock & PCIDEVS_LOCK) )
> +                spin_lock(&pcidevs_lock);

This is too late - you may not iterate over pdev-s without holding
that lock, and the list_del() may not be done in that case either.
Plus this should be accompanied be an ASSERT() in the else case.

> +            if ( pci_hide_device(bus, devfn) )

But now I'm _really_ puzzled: You acquire the exact lock that
pci_hide_device() acquires. Hence, unless I've overlooked an earlier
change, I can't see this as other than an unconditional dead lock. Did
you test this code path at all?

> +            {
> +                printk(XENLOG_ERR
> +                       "IOMMU hide device %04x:%02x:%02x.%02x error.",
> +                       seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn));
> +            }

I hate to repeat myself: SBDF need to be printed in canonical
ssss:bb:dd.f form, to be grep-able. Also no full stops at the end
of log messages please. And (also mentioned before) if there are
error codes available, log them to aid diagnosis of problems.

Also - unnecessary figure braces.

> @@ -350,9 +413,19 @@ static int flush_iotlb_qi(
>          queue_invalidate_iotlb(iommu,
>                                 type >> DMA_TLB_FLUSH_GRANU_OFFSET, dr,
>                                 dw, did, size_order, 0, addr);
> -        if ( flush_dev_iotlb )
> -            ret = dev_invalidate_iotlb(iommu, did, addr, size_order, type);
> +
> +        /*
> +         * Before Device-TLB invalidation we need to synchronize
> +         * invalidation completions with hardware.
> +         */
>          rc = invalidate_sync(iommu);
> +        if ( rc )
> +             return rc;
> +
> +        if ( flush_dev_iotlb )
> +            ret = dev_invalidate_iotlb(iommu, did, addr, size_order,
> +                                       type, lock);
> +
>          if ( !ret )
>              ret = rc;

These two lines have now become pointless, and hence should
be deleted.

> @@ -162,6 +162,18 @@ int dev_invalidate_iotlb(struct iommu *iommu, u16 did,
>              return -EOPNOTSUPP;
>          }
>  
> +        /*
> +         * Synchronize with hardware for Device-TLB invalidate
> +         * descriptor.
> +         */
> +        ret = dev_invalidate_iotlb_sync(iommu, did, pdev->seg,
> +                                        pdev->bus, pdev->devfn, lock);
> +        if ( ret )
> +            printk(XENLOG_ERR
> +                   "Flush error %d on device %04x:%02x:%02x.%02x \n",
> +                   ret, pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn),
> +                   PCI_FUNC(pdev->devfn));
> +
>          if ( !ret )
>              ret = rc;

The two context lines here show that you're now clobbering "ret".

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel