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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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

Jan


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