Re: [PATCH 12/12] xen/arm: add cache coloring support for Xen

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.08.2022 14:51, Carlo Nonato wrote:
> --- a/xen/common/vmap.c
> +++ b/xen/common/vmap.c
> @@ -8,6 +8,9 @@
>  #include <xen/types.h>
>  #include <xen/vmap.h>
>  #include <asm/page.h>
> +#ifdef CONFIG_CACHE_COLORING
> +#include <asm/coloring.h>
> +#endif

Even independent of my earlier question towards more code becoming common,
I think there will want to be a xen/coloring.h which takes care of this
abstraction, requiring such #ifdef in just a single place.

> @@ -218,6 +221,28 @@ void *__vmap(const mfn_t *mfn, unsigned int granularity,
>      return va;
>  }
>  
> +#ifdef CONFIG_CACHE_COLORING
> +void * __vmap_colored(const mfn_t *mfn, unsigned int nr, unsigned int align,
> +                      unsigned int flags, enum vmap_region type)

Please no new functions with double underscores as prefix. Only static
symbol names may start with an underscore, and then also only with a
single one.

> +{
> +    void *va = vm_alloc(nr, align, type);
> +    unsigned long cur = (unsigned long)va;
> +    paddr_t pa = mfn_to_maddr(*mfn);
> +
> +    for ( ; va && nr-- ; cur += PAGE_SIZE )
> +    {
> +        pa = next_xen_colored(pa);

This may alter the address, yet the caller expects that the original
address be mapped. I must be missing something?

> +        if ( map_pages_to_xen(cur, maddr_to_mfn(pa), 1, flags) )
> +        {
> +            vunmap(va);
> +            return NULL;
> +        }
> +        pa += PAGE_SIZE;
> +    }
> +    return va;
> +}

Afaics you only consume the first slot of *mfn. What about the other
(nr - 1) ones? And compared to __vmap() there's no "granularity"
parameter, which is what controls the mapping of multiple contiguous
pages.

> --- a/xen/include/xen/vmap.h
> +++ b/xen/include/xen/vmap.h
> @@ -14,6 +14,10 @@ void vm_init_type(enum vmap_region type, void *start, void 
> *end);
>  
>  void *__vmap(const mfn_t *mfn, unsigned int granularity, unsigned int nr,
>               unsigned int align, unsigned int flags, enum vmap_region);
> +#ifdef CONFIG_CACHE_COLORING
> +void *__vmap_colored(const mfn_t *mfn, unsigned int nr, unsigned int align,
> +                     unsigned int flags, enum vmap_region);
> +#endif

I don't think such a declaration really needs putting inside #ifdef.

Jan