Re: [Xen-devel] [PATCH v2 2/4] iommu: rename wrapper functions

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 03.12.18 at 18:40, <paul.durrant@xxxxxxxxxx> wrote:
> A subsequent patch will add semantically different versions of
> iommu_map/unmap() so, in advance of that change, this patch renames the
> existing functions to iommu_legacy_map/unmap() and modifies all call-sites.

Hmm, this is the second rename in pretty short a period of time.
Have you considered simply requesting a revert of the earlier
rename? Or wait, that was a rename folded into the addition of
the order parameter. Still not very fortunate. Apparently I
wasn't fast enough to express my reservations against the
original suggestion.

> The patch also renames iommu_iotlb_flush[_all] to the shorter name(s)
> iommu_flush[_all] (also renaming an internal VT-d function to avoid a name
> clash) and co-locates the declarations with those of
> iommu_legacy_map/unmap().

But the "iotlb" part was there to distinguish from other kinds of
flushing). Furthermore the such renamed functions continue to
call iotlb_flush{,_all} hook functions.

> The only changes in this patch that are not purely cosmetic are in
> arch_iommu_populate_page_table() and iommu_hwdom_init(), which now call
> iommu_legacy_map() rather than calling the map_page() iommu_ops method
> directly.

I pretty strongly think this ought to be a separate change. First
and foremost because you add verbosity (in case of error) to
the first of these code paths. Additionally the extra overhead of
repeatedly executed conditionals and the extra function call may
end up being noticeable for sufficiently long loops in both cases.

> @@ -68,9 +67,9 @@ int arch_iommu_populate_page_table(struct domain *d)
>              {
>                  ASSERT(!(gfn >> DEFAULT_DOMAIN_ADDRESS_WIDTH));
>                  BUG_ON(SHARED_M2P(gfn));
> -                rc = hd->platform_ops->map_page(d, _dfn(gfn), _mfn(mfn),
> -                                                IOMMUF_readable |
> -                                                IOMMUF_writable);
> +                rc = iommu_legacy_map(d, _dfn(gfn), _mfn(mfn),
> +                                      PAGE_ORDER_4K, IOMMUF_readable |
> +                                      IOMMUF_writable);

The indentation here is now pretty misleading.

> --- a/xen/include/xen/iommu.h
> +++ b/xen/include/xen/iommu.h
> @@ -88,17 +88,22 @@ int iommu_construct(struct domain *d);
>  /* Function used internally, use iommu_domain_destroy */
>  void iommu_teardown(struct domain *d);
>  
> -/* iommu_map_page() takes flags to direct the mapping operation. */
>  #define _IOMMUF_readable 0
>  #define IOMMUF_readable  (1u<<_IOMMUF_readable)
>  #define _IOMMUF_writable 1
>  #define IOMMUF_writable  (1u<<_IOMMUF_writable)

I'd prefer if the comment didn't go away altogether.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel