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

["Xu, Quan" <quan.xu@xxxxxxxxx>]

On March 17, 2016 4:17pm, Tian, Kevin <kevin.tian@xxxxxxxxx> wrote:
> > 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
> > +    {
> > +        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)?
> 
No, it's not a temporary one.
dev_invalidate_iotlb_sync -- for Device-TLB invalidation sync, as we need SBDF 
to indicate which device flush timed out.
invalidate_sync -- for VT-d iotlb/iec/context invalidation sync.


> >  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...
> 

Now, does it still look not consistent?

> >      }
> >      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
>           */

I'd like this TODO, and I would add it in v8.

Quan

>          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