Re: [Xen-devel] [PATCH v9 2/3] VT-d: wrap a _sync version for all VT-d flush interfaces

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

> From: Xu, Quan
> Sent: Friday, April 08, 2016 10:21 AM
> 
> On April 07, 2016 11:29pm, Jan Beulich <JBeulich@xxxxxxxx> wrote:
> > >>> On 07.04.16 at 09:44, <quan.xu@xxxxxxxxx> wrote:
> > > On April 05, 2016 5:35pm, Jan Beulich <JBeulich@xxxxxxxx> wrote:
> > >> >>> On 01.04.16 at 16:47, <quan.xu@xxxxxxxxx> wrote:
> > >> > +{
> > >> > +    queue_invalidate_context(iommu, did, source_id,
> > >> > +                             function_mask, granu);
> > >> > +
> > >> > +    return invalidate_sync(iommu); }
> > >>
> > >> Further down you replace the only call to
> > >> queue_invalidate_context() - why keep both functions instead of just
> > >> making
> > > the
> > >> existing one do the sync? (That would the likely also apply to
> > >> qinval_device_iotlb() and others below.)
> > >>
> > >
> > > It is optional.
> > >  I think:
> > > 1. in the long term, we may need no _sync version.
> > > 2. At least, the current wrap looks good to me. e.g.
> > > queue_invalidate_context() is for context-cache Invalidate Descriptor,
> > > and the
> > > invalidate_sync() is for Invalidation Wait Descriptor. It is much clearer.
> >
> > I don't really agree, but will leave it to the VT-d maintainers to judge.
> >
> 
> +to Kevin and Feng, I am open for it.

Let's just change existing one to _sync.

> 
> 
> > >> > +        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;
> > >> >      }
> > >>
> > >> I think leaving the existing logic as is would be better - best
> > >> effort
> > > invalidation
> > >> even when an error has occurred.
> > >>
> > >
> > > I have an open:
> > > As vt-d spec(:Queued Invalidation Ordering Considerations) said,
> > >      1. If the Fence(FN) flag is 1 in a inv_wait_dsc, hardware must
> > > execute descriptors following the inv_wait_dsc only after wait command is
> > completed.
> > >      2. when a Device-TLB invalidation timeout is detected, hardware
> > > must not complete any pending inv_wait_dsc commands.
> > > In current code, the Fence(FN) is always 1.
> > > if a Device-TLB invalidation timeout is detected, this additional
> > > inv_wait_dsc is not completed.
> > > __iiuc__,
> > > the new coming descriptors, in that queue, _might_ be not executed any
> > > more, waiting for this additional inv_wait_dsc which is not completed.
> > > is it true?
> >
> > That's not a question to me, is it?
> 
> To community, but vt-d maintainers are someone who can explain to me.
> 

'not completed' here means 'abort', so your timeout check will be
hit since the status is never 'done' then.

But I'm not sure how your question is related to Jan's comment, which
looks reasonable to me. :-)

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