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

[Carlo Nonato <carlo.nonato@xxxxxxxxxxxxxxx>]

Hi Jan,

On Thu, Sep 15, 2022 at 3:25 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> 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 the original address color is assigned to Xen, then next_xen_colored()
simply returns that address. If this isn't the case, then you're right: the
address changes to the correct, colored, one. The caller should expect
this behavior since this is the colored version of vmap, the one that takes
into account the Xen coloring configuration.

> > +        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?

Not sure I understood. The first slot is used as the starting point and then
the addr of that mfn plus next_xen_colored() are the mechanism used to select
the next mfns. Probably the first argument of vmap_colored is a bit
misleading.

> And compared to __vmap() there's no "granularity"
> parameter, which is what controls the mapping of multiple contiguous
> pages.

That's because we don't support multiple contiguous pages in the sense that
we only operate on one page at a time (like explained in the
"Known limitations" doc section and elsewhere in those discussions).

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

Thanks.

- Carlo Nonato