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

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

On March 23, 2016 1:37pm, <kevin.tian@xxxxxxxxx> wrote:
> > From: Xu, Quan
> > Sent: Wednesday, March 23, 2016 11:30 AM
> >
> > >
> > > 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
> 
> Yes I recall this story.
> 
> What about doing this? Let's wrap a _sync version for all flush interfaces, 
> like
> below:
> 
> static int queue_invalidate_context_sync(...)
> {
>       queue_invalidate_context(...);
>       return invalidate_sync(...);
> }
> 
> Then invoke _sync version at all callers, e.g.:
> static int flush_context_qi(...)
> {
>       ...
>       if ( qi_ctrl->qinval_maddr != 0 )
>               ret = queue_invalidate_context_sync(...);
> }
> 
> similarly we'll have dev_invalidate_iotlb_sync for device IOTLB flush.
> 
> It simplifies caller logic and make code more readable. :-)
> 

Agreed, I would follow it and send out v8 ASAP, then I could focus on prereq 
patch set. :)


Quan



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