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

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

On March 21, 2016 11:27am, Tian, Kevin <kevin.tian@xxxxxxxxx> wrote:
> > From: Xu, Quan
> > Sent: Friday, March 18, 2016 8:22 PM

> > > >  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;
> 
  Kevin,
  now I doubt that I should put invalidation sync within dev_invalidate_iotlb, 
which was also your suggestion.
As the dev_invalidate_iotlb() is invalidation for all of domain's ATS devices. 
If in this consistent way, we couldn't
Find which ATS device flush timed out, then we need to hide all of domain's ATS 
devices.
Do you recall it?
  Also I think it is reluctant to put invalidate_sync within 
queue_invalidate_iotlb() for consistent issue.
Quan


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