Re: [Xen-devel] [PATCH 2/2] iommu: elide flushing for higher order map/unmap operations

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Andrew Cooper
> Sent: 30 November 2018 12:49
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>; Julien Grall
> <julien.grall@xxxxxxx>; Suravee Suthikulpanit
> <suravee.suthikulpanit@xxxxxxx>; Brian Woods <brian.woods@xxxxxxx>; Jan
> Beulich <jbeulich@xxxxxxxx>; Kevin Tian <kevin.tian@xxxxxxxxx>; Wei Liu
> <wei.liu2@xxxxxxxxxx>; Roger Pau Monne <roger.pau@xxxxxxxxxx>
> Subject: Re: [PATCH 2/2] iommu: elide flushing for higher order map/unmap
> operations
> 
> On 30/11/2018 10:45, Paul Durrant wrote:
> > This patch removes any implicit flushing that occurs in the
> implementation
> > of map and unmap operations and, instead, adds explicit flushing at the
> > end of the loops in the iommu_map/unmap() wrapper functions.
> >
> > Because VT-d currently performs two different types of flush dependent
> upon
> > whether a PTE is being modified versus merely added (i.e. replacing a
> non-
> > present PTE) a 'iommu_flush_type' enumeration is defined by this patch
> and
> > the iommu_ops map method is modified to pass back the type of flush
> > necessary for the PTE that has been populated. When a higher order
> mapping
> > operation is done, the wrapper code performs the 'highest' level of
> flush
> > required by the individual iommu_ops method calls, where a 'modified
> PTE'
> > flush is deemed to be higher than a 'added PTE' flush. The ARM SMMU
> > implementation currently performs no implicit flushing and therefore
> > the modified map method always passes back a flush type of 'none'.
> >
> > NOTE: The per-cpu 'iommu_dont_flush_iotlb' is respected by the
> iommu_map()
> >       wrapper function and therefore this now applies to all IOMMU
> >       implementations rather than just VT-d. Use of the flag has been
> added
> >       to arch_iommu_hwdom_init() so that flushing is now fully elided
> for
> >       the hardware domain's initial table population.
> 
> iommu_dont_flush_iotlb is a misfeature.  While it still exists, the
> flushing API is fundamentally broken.
> 
> Do you have a plan to remove it?  I ask, because the only feasible
> option I see is for iommu_{map,unmap}() to pass the flush accumulation
> out to the caller, and have the caller use the appropriate flush
> interfaces.
> 
> [Edit - lunch happened around about this point, and there was a long
> discussion]
> 
> One idea with be to start with a prep patch renaming iommu_{,un}map() to
> _legacy(), nothing beside them that they have implicit flushing
> characteristics.  Then, the nonflushing versions of iommu_{,un}map() can
> be introduced, which return the accumulated flush flag, and the callers
> can DTRT.

Ok. I'll plan to do a new patch #2 that does the rename, and the move this code 
into a patch #3.

> 
> This way, we can avoid introducing a further user of
> iommu_dont_flush_iotlb in arch_iommu_hwdom_init(), and clean up the
> remaining legacy callsites at a later point when more infrastructure is
> in place.
> 
> In particular, the P2M code cannot be fixed to behave in this way at the
> moment because the point at which the IOMMU changes are hooked in lacks
> an order parameter.

The P2M code is also a mess and calls directly into VT-d and AMD IOMMU 
functions without going through the wrapper code in some cases. There is much 
cleanup to do... these patches are just a start.

  Paul

> 
> > diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
> > index da8294bac8..289e0e2772 100644
> > --- a/xen/include/xen/iommu.h
> > +++ b/xen/include/xen/iommu.h
> > @@ -155,6 +155,13 @@ struct page_info;
> >   */
> >  typedef int iommu_grdm_t(xen_pfn_t start, xen_ulong_t nr, u32 id, void
> *ctxt);
> >
> 
> This wants at least a comment stating that some IOMMUs require that we
> issue a flush when modifying a not-present/otherwise invalid entry.
> 
> > +enum iommu_flush_type
> > +{
> > +    IOMMU_FLUSH_none, /* no flush required */
> > +    IOMMU_FLUSH_added, /* no modified entries, just additional entries
> */
> 
> IOMMU_FLUSH_invalid ?  I think it is more descriptive of the scenario in
> which it is used.
> 
> > +    IOMMU_FLUSH_modified, /* modified entries */
> > +};
> > +
> >  struct iommu_ops {
> >      int (*init)(struct domain *d);
> >      void (*hwdom_init)(struct domain *d);
> > @@ -177,7 +184,8 @@ struct iommu_ops {
> >       * other by the caller in order to have meaningful results.
> >       */
> >      int __must_check (*map_page)(struct domain *d, dfn_t dfn, mfn_t
> mfn,
> > -                                 unsigned int flags);
> > +                                 unsigned int flags,
> > +                                 enum iommu_flush_type *flush_type);
> 
> Maintaining the flush type by pointer is quite awkward.
> 
> How about folding a positive flush type in with negative errors?  i.e.
> map_page() becomes < 0 on error, 0 for success/no flush and >0 for
> success/w flush.
> 
> I think the result would be rather cleaner to read.
> 
> ~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel