Re: [Xen-devel] [PATCH v8 09/11] vt-d: fix the IOMMU flush issue

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 14.06.16 at 10:10, <quan.xu@xxxxxxxxx> wrote:
> On June 13, 2016 11:52 PM, Jan Beulich <jbeulich@xxxxxxxx> wrote:
>> >>> "Xu, Quan" <quan.xu@xxxxxxxxx> 06/13/16 5:22 PM >>>
>> >From: Quan Xu <quan.xu@xxxxxxxxx>
>> >@@ -546,17 +550,37 @@ static int __must_check iommu_flush_all(void)
>> >struct acpi_drhd_unit *drhd; struct iommu *iommu; int flush_dev_iotlb;
>> >+    int rc = 0;
>>  >
>> >flush_all_cache();
>> >for_each_drhd_unit ( drhd )
>> >{
>> >+        int iommu_rc, iommu_ret;
>> >+
>> >iommu = drhd->iommu;
>> >-        iommu_flush_context_global(iommu, 0);
>> >+        iommu_rc = iommu_flush_context_global(iommu, 0);
>> >flush_dev_iotlb = find_ats_dev_drhd(iommu) ? 1 : 0;
>> >-        iommu_flush_iotlb_global(iommu, 0, flush_dev_iotlb);
>> >+        iommu_ret = iommu_flush_iotlb_global(iommu, 0,
>> >+ flush_dev_iotlb);
>> >+
>> >+        /*
>> >+         * The current logic for returns:
>> >+         *   - positive  invoke iommu_flush_write_buffer to flush cache.
>> >+         *   - zero      on success.
>> >+         *   - negative  on failure. Continue to flush IOMMU IOTLB on a
>> >+         *               best effort basis.
>> >+         */
>> >+        if ( iommu_rc > 0 || iommu_ret > 0 )
>> >+            iommu_flush_write_buffer(iommu);
>> >+        if ( rc >= 0 )
>> >+            rc = iommu_rc;
>> >+        if ( rc >= 0 )
>> >+            rc = iommu_ret;
>> 
>> First of all - is it correct to fold the two iommu_flush_write_buffer()
>> invocations?
>> 
> 
> Sure, it is correct.. 
> 
> as:
> - For updates to remapping hardware structures that require context-cache, 
> PASID-cache, IOTLB or IEC invalidation
> Operations to flush stale entries from the hardware caches, no additional 
> action is required to make the modification
> Visible to hardware. This is because, hardware performs an implicit 
> write-buffer-flushing as a pre-condition to context-cache,
> PASID-cache, IOTLB and IEC invalidation operations.
> 
> - For updates to remapping hardware structures (such as modifying a currently 
> not-present entry) that do not require
> Context-cache, IOTLB, or IEC invalidations, software must explicitly perform 
> write-buffer-flushing to ensure the updated structures
> Are visible to hardware.

But that's not the point. Instead my question was related to Kevin's
concern towards you making assumptions on the behavior of
iommu_flush_context_global() vs iommu_flush_iotlb_global(): What
if the first returned 1 but the second didn't? It would seem to me
that in such a (theoretical) case iommu_flush_write_buffer() might
need to be invoked prior to the second flush function.

Jan


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