On 16.09.2022 18:07, Carlo Nonato wrote:
> On Thu, Sep 15, 2022 at 3:25 PM Jan Beulich <jbeul...@suse.com> 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
