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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 04 December 2018 14:51
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: Julien Grall <julien.grall@xxxxxxx>; Andrew Cooper
> <Andrew.Cooper3@xxxxxxxxxx>; Roger Pau Monne <roger.pau@xxxxxxxxxx>; Wei
> Liu <wei.liu2@xxxxxxxxxx>; George Dunlap <George.Dunlap@xxxxxxxxxx>; Ian
> Jackson <Ian.Jackson@xxxxxxxxxx>; Jun Nakajima <jun.nakajima@xxxxxxxxx>;
> Kevin Tian <kevin.tian@xxxxxxxxx>; Stefano Stabellini
> <sstabellini@xxxxxxxxxx>; xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>;
> Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>; Tim (Xen.org)
> <tim@xxxxxxx>
> Subject: Re: [PATCH v2 2/4] iommu: rename wrapper functions
> 
> >>> 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.

Ok. I'll leave these names alone.

> 
> > 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.
> 

Ok, I'll make this patch purely cosmetic and split those out.

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

Ok, I'll move the IOMMUF_readable down a line.

> > --- 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.
> 

Ok. I didn't think it was particularly helpful but I'll re-instate it with a 
modified name.

  Paul

> Jan
> 


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