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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 19.06.19 at 01:20, <sstabellini@xxxxxxxxxx> 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@xxxxxxxxxx>
> CC: JBeulich@xxxxxxxx 
> CC: andrew.cooper3@xxxxxxxxxx 
> ---
> 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)?

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(),
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).

Jan



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