Re: [Xen-devel] [PATCH v7 2/2] VT-d: Fix vt-d Device-TLB flush timeout issue

["Tian, Kevin" <kevin.tian@xxxxxxxxx>]

> From: Xu, Quan
> Sent: Thursday, March 17, 2016 3:13 PM
> diff --git a/xen/drivers/passthrough/vtd/qinval.c 
> b/xen/drivers/passthrough/vtd/qinval.c
> index 37a15fb..2a5c638 100644
> --- a/xen/drivers/passthrough/vtd/qinval.c
> +++ b/xen/drivers/passthrough/vtd/qinval.c
> @@ -233,6 +233,57 @@ 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)
> +{
> +    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;
> +
> +    pcidevs_lock();
> +    for_each_pdev(d, pdev)

we need a 'safe' version here since you're deleting nodes
when walking list. for_each_pdev today is based on 
list_for_each_entry. Or if it's sure that only one pdev
can match, we can break out of the loop to do removal.

> +    {
> +        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;
> +        }
> +    }
> +
> +    pcidevs_unlock();
> +
> +    if ( !is_hardware_domain(d) )
> +        domain_crash(d);
> +
> +    rcu_unlock_domain(d);
> +}
> +
> +int dev_invalidate_iotlb_sync(struct iommu *iommu, u16 did,
> +                              u16 seg, u8 bus, u8 devfn)
> +{
> +    struct qi_ctrl *qi_ctrl = iommu_qi_ctrl(iommu);
> +    int rc = 0;
> +
> +    if ( qi_ctrl->qinval_maddr )
> +    {
> +        rc = queue_invalidate_wait(iommu, 0, 1, 1);
> +        if ( rc == -ETIMEDOUT )
> +            dev_invalidate_iotlb_timeout(iommu, did, seg, bus, devfn);
> +    }
> +
> +    return rc;
> +}
> +

Is this function a temporary one which will be removed later once we
can handle timeout for all types of flushes (at that time suppose this
logic will be reflected in invalidate_sync directly)?

>  static void queue_invalidate_iec(struct iommu *iommu, u8 granu, u8 im, u16 
> iidx)
>  {
>      unsigned long flags;
> @@ -342,8 +393,6 @@ static int flush_iotlb_qi(
> 
>      if ( qi_ctrl->qinval_maddr != 0 )
>      {
> -        int rc;
> -
>          /* use queued invalidation */
>          if (cap_write_drain(iommu->cap))
>              dw = 1;
> @@ -353,11 +402,17 @@ static int flush_iotlb_qi(
>          queue_invalidate_iotlb(iommu,
>                                 type >> DMA_TLB_FLUSH_GRANU_OFFSET, dr,
>                                 dw, did, size_order, 0, addr);
> +
> +        /*
> +         * Before Device-TLB invalidation we need to synchronize
> +         * invalidation completions with hardware.
> +         */
> +        ret = invalidate_sync(iommu);
> +        if ( ret )
> +             return ret;
> +
>          if ( flush_dev_iotlb )
>              ret = dev_invalidate_iotlb(iommu, did, addr, size_order, type);
> -        rc = invalidate_sync(iommu);
> -        if ( !ret )
> -            ret = rc;

Current change looks not consistent. For IOMMU iotlb flush, we have
invalidate_sync out of invalidate operation, however below...

>      }
>      return ret;
>  }
> diff --git a/xen/drivers/passthrough/vtd/x86/ats.c
> b/xen/drivers/passthrough/vtd/x86/ats.c
> index 334b9c1..c87ffe3 100644
> --- a/xen/drivers/passthrough/vtd/x86/ats.c
> +++ b/xen/drivers/passthrough/vtd/x86/ats.c
> @@ -162,6 +162,18 @@ int dev_invalidate_iotlb(struct iommu *iommu, u16 did,
>              return -EOPNOTSUPP;
>          }
> 
> +        /*
> +         * Synchronize with hardware for Device-TLB invalidate
> +         * descriptor.
> +         */
> +        rc = dev_invalidate_iotlb_sync(iommu, did, pdev->seg,
> +                                       pdev->bus, pdev->devfn);
> +        if ( rc )
> +            printk(XENLOG_ERR
> +                   "Flush error %d on device %04x:%02x:%02x.%u.\n",
> +                   ret, pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn),
> +                   PCI_FUNC(pdev->devfn));
> +
>          if ( !ret )

for device iotlb flush, you moved the invalidate_sync inside the
invalidate operation.

If this is only temporary as I guessed earlier, is it clearer to change
like below:

> @@ -353,11 +402,17 @@ static int flush_iotlb_qi(
          queue_invalidate_iotlb(iommu,
                                 type >> DMA_TLB_FLUSH_GRANU_OFFSET, dr,
                                 dw, did, size_order, 0, addr);
 
         /*
          * Before Device-TLB invalidation we need to synchronize
          * invalidation completions with hardware. 
          * TODO: timeout error handling to be added later
          */
         ret = invalidate_sync(iommu);
         if ( ret )
              return ret;
 
          if ( flush_dev_iotlb )
              ret = dev_invalidate_iotlb(iommu, did, addr, size_order, type);

         rc = invalidate_sync(iommu);
         if ( rc == -ETIMEDOUT )
            dev_invalidate_iotlb_timeout(iommu, did, seg, bus, devfn);
         if ( !ret )
             ret = rc;

This way later when we have invalidate_sync handling timeout error
for all types of flushes, above two lines of timeout handling can be
removed.

Thanks
Kevin

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