Re: [Xen-devel] [PATCH v10 3/3] vt-d: fix vt-d Device-TLB flush timeout issue

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 22.04.16 at 12:54, <quan.xu@xxxxxxxxx> wrote:
> --- a/xen/drivers/passthrough/vtd/qinval.c
> +++ b/xen/drivers/passthrough/vtd/qinval.c
> @@ -206,10 +206,71 @@ static int invalidate_sync(struct iommu *iommu)
>      return 0;
>  }
>  
> +static void dev_invalidate_iotlb_timeout(struct iommu *iommu, u16 did,
> +                                         u16 seg, u8 bus, u8 devfn)
> +{
> +    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]);
> +
> +    /*
> +     * In case the domain has been freed or the IOMMU domid bitmap is
> +     * not valid, the device no longer belongs to this domain.
> +     */
> +    if ( d == NULL )
> +        return;
> +
> +    pcidevs_lock();
> +
> +    for_each_pdev(d, pdev)
> +    {
> +        if ( (pdev->seg == seg) &&
> +             (pdev->bus == bus) &&
> +             (pdev->devfn == devfn) )
> +        {
> +            ASSERT(pdev->domain);
> +            list_del(&pdev->domain_list);
> +            pdev->domain = NULL;
> +            pci_hide_existing_device(pdev);
> +            break;
> +        }
> +    }

A loop like this is of course not ideal (especially for Dom0, which may
have many devices). And I wonder why you, ...

> @@ -134,8 +133,9 @@ int dev_invalidate_iotlb(struct iommu *iommu, u16 did,
>              /* invalidate all translations: sbit=1,bit_63=0,bit[62:12]=1 */
>              sbit = 1;
>              addr = (~0UL << PAGE_SHIFT_4K) & 0x7FFFFFFFFFFFFFFF;
> -            rc = qinval_device_iotlb_sync(iommu, pdev->ats_queue_depth,
> -                                          sid, sbit, addr);
> +            rc = qinval_device_iotlb_sync(iommu, pdev->ats_queue_depth, did,
> +                                          pdev->seg, pdev->bus, pdev->devfn,
> +                                          sbit, addr);
>              break;
>          case DMA_TLB_PSI_FLUSH:
>              if ( !device_in_domain(iommu, pdev, did) )
> @@ -154,8 +154,9 @@ int dev_invalidate_iotlb(struct iommu *iommu, u16 did,
>                  addr |= (((u64)1 << (size_order - 1)) - 1) << PAGE_SHIFT_4K;
>              }
>  
> -            rc = qinval_device_iotlb_sync(iommu, pdev->ats_queue_depth,
> -                                          sid, sbit, addr);
> +            rc = qinval_device_iotlb_sync(iommu, pdev->ats_queue_depth, did,
> +                                          pdev->seg, pdev->bus, pdev->devfn,
> +                                          sbit, addr);
>              break;

... holding pdev in your hands here, don't just pass it down (which
at once would make the function signature less convoluted: you
could even eliminate the currently 2nd parameter that way).

Jan


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