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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.09.2022 16:31, Carlo Nonato wrote:
> 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?

Yes.

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

Well, that's the price to pay for non-contiguous vmap-s. If the added
function really is just an optimization, I guess this might be acceptable
if actually stated that way in the description. I intentionally say
"might", because I think there's too heavy an implications here (the
caller having done the allocation(s) in a way that matches the function's
behavior).

Jan