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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

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.

> 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