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

[Carlo Nonato <carlo.nonato@xxxxxxxxxxxxxxx>]

Hi Jan,

On Mon, Sep 19, 2022 at 10:38 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 16.09.2022 18:07, Carlo Nonato wrote:
> > On Thu, Sep 15, 2022 at 3:25 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
> >> On 26.08.2022 14:51, Carlo Nonato wrote:
> >>> @@ -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)
> >>> +{
> >>> +    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.
>
> That's (to me at least) very surprising behavior, and hence needs
> properly calling out in a code comment at the least.
>
> Personally I'm not convinced of having a function with this behavior,
> and instead I think the normal vmap() should do. As long as you're
> only allowing for order-0 allocations, that shouldn't be an issue
> anyway.

You mean creating an array of colored mfns (I mean with a colored machine
address) and passing it to vmap()? Am I understanding you correctly?
This is the only way I can see to use the original vmap() and respect
the coloring configuration at the same time. But isn't it a waste of time
and space to create this array?

> Jan

Thanks.

- Carlo Nonato