Re: [Xen-devel] [PATCH v3 1/5] xen: add a p2mt parameter to map_mmio_regions

[Stefano Stabellini <sstabellini@kernel.org>]

On Wed, 19 Jun 2019, Jan Beulich wrote:
> >>> On 19.06.19 at 01:20, <sstabellini@kernel.org> wrote:
> > Add a p2mt parameter to map_mmio_regions, pass p2m_mmio_direct_dev on
> > ARM and p2m_mmio_direct on x86 -- no changes in behavior. On x86,
> > introduce a macro to strip away the last parameter and rename the
> > existing implementation of map_mmio_regions to __map_mmio_regions.
> > Use __map_mmio_regions in vpci as it is x86-only today.
> > 
> > On ARM, given the similarity between map_mmio_regions after the change
> > and map_regions_p2mt, remove un/map_regions_p2mt. Also add an ASSERT to
> > check that only p2m_mmio_* types are passed to it.
> > 
> > Also fix the style of the comment on top of map_mmio_regions since we
> > are at it.
> > 
> > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> > CC: JBeulich@suse.com 
> > CC: andrew.cooper3@citrix.com 
> > ---
> > Changes in v3:
> > - code style
> > - introduce __map_mmio_regions on x86
> 
> No. At the very least the name is badly chosen: There shouldn't be
> new name space violations. But ...
> 
> > --- a/xen/include/asm-x86/p2m.h
> > +++ b/xen/include/asm-x86/p2m.h
> > @@ -1000,6 +1000,14 @@ static inline int p2m_entry_modify(struct p2m_domain *p2m, p2m_type_t nt,
> >      return 0;
> >  }
> >  
> > +/* x86 doesn't use the p2mt parameter, just strip it away */
> > +#define map_mmio_regions(d, start_gfn, nr, mfn, p2mt) \
> > +            __map_mmio_regions(d, start_gfn, nr, mfn)
> > +int __map_mmio_regions(struct domain *d,
> > +                       gfn_t start_gfn,
> > +                       unsigned long nr,
> > +                       mfn_t mfn);
> > +
> 
> ... except for this perhaps not being everyone's taste, is there
> anything wrong with just
> 
> /* x86 doesn't use the p2mt parameter, just strip it away */
> #define map_mmio_regions(d, start_gfn, nr, mfn, p2mt) \
>             map_mmio_regions(d, start_gfn, nr, mfn)
> 
> (placed ahead of the p2m-common.h inclusion point, such that
> the override would also affect the declaration)?

I couldn't go with this suggestion because then usages of
map_mmio_regions under arch/x86 would fail to compile unless adjusted
by adding one useless argument, as the macro wouldn't apply correctly.


> The next best (imo) solution would be to utilize the fact that the
> function is mis-named right now anyway: There's no point for the
> plural in its name afaics. Hence the aliasing above could also
> go between map_mmio_regions() and map_mmio_region(),

I went with this suggestion, basically I renamed __map_mmio_regions to
map_mmio_region.


> depending on whether you'd want to adjust the "common" name
> at the same time (but if you did so, then perhaps the unmap
> function should get renamed too).

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