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

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

> From: Xu, Quan
> Sent: Tuesday, June 14, 2016 5:04 PM
> 
> On June 14, 2016 4:27 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
> > >>> 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.
> 
> In this case, it is correct too.
> As , hardware performs an __implicit__ write-buffer-flushing as a 
> __pre-condition__ to
> IOTLB invalidation operation.
> So software is no need to call iommu_flush_write_buffer() to explicitly 
> perform
> write-buffer-flushing for that the first returned 1.
> 

Yes, there is no issue doing so based on spec.

Thanks
Kevin

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