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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

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

Yes - it shouldn't be an array if it's not used as one.

Jan