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

["Jan Beulich" <jbeulich@xxxxxxxx>]

>>> "Xu, Quan" <quan.xu@xxxxxxxxx> 06/13/16 5:22 PM >>>
>From: Quan Xu <quan.xu@xxxxxxxxx>
>
>The propagation value from IOMMU flush interfaces may be positive, which
>indicates callers need to flush cache, not one of faliures.
>
>when the propagation value is positive, this patch fixes this flush issue
>as follows:
>- call iommu_flush_write_buffer() to flush cache.
>- return zero.
>
>Signed-off-by: Quan Xu <quan.xu@xxxxxxxxx>
>Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
>
>CC: Kevin Tian <kevin.tian@xxxxxxxxx>
>CC: Feng Wu <feng.wu@xxxxxxxxx>
>CC: Keir Fraser <keir@xxxxxxx>
>CC: Jan Beulich <jbeulich@xxxxxxxx>
>CC: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>
>v8: drop assertion and check both flush_context and flush_iotlb
>whether the return values are positive.
>(note: this change is not under Jan's R-b).

In which case you should simply have dropped it.

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

And then the variable naming is strange - both operations are IOMMU ones, so
prefixing the variables with iommu_ doesn't help much here. How about ctxt_rc
and iotlb_rc or some such?

Jan


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