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: Friday, March 18, 2016 8:22 PM
> 
> > > +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.

Thanks. I recalled it. Once you defined some INVALID seg/bus/devfn to
reuse same interface, and then the suggestion is to go with different
interfaces.:-)

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

Yes, still inconsistent. As I said, you put invalidation sync within 
dev_invalidate_iotlb, while for all other IOMMU invalidations the
sync is put after. Below would be consistent then:

        if ( flush_dev_iotlb )
            ret = dev_invalidate_iotlb(iommu, did, addr, size_order, type);
        rc = dev_invalidate_iotlb_sync(...);
        if ( !ret )
            ret = rc;

Thanks
Kevin

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