Re: [PATCH v6 14/15] xen/arm: add cache coloring support for Xen

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.02.2024 18:29, Carlo Nonato wrote:
> On Tue, Feb 13, 2024 at 4:25 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>> On 29.01.2024 18:18, Carlo Nonato wrote:
>>> @@ -218,9 +230,44 @@ static void xen_pt_enforce_wnx(void)
>>> --- a/xen/common/llc-coloring.c
>>> +++ b/xen/common/llc-coloring.c
>>> @@ -29,6 +29,8 @@ static unsigned int __ro_after_init xen_num_colors;
>>>
>>>  #define mfn_color_mask              (max_nr_colors - 1)
>>>  #define mfn_to_color(mfn)           (mfn_x(mfn) & mfn_color_mask)
>>> +#define mfn_set_color(mfn, color)   (_mfn((mfn_x(mfn) & ~mfn_color_mask) | 
>>> \
>>> +                                     (color)))
>>
>> Nit: The wrapped line wants further indenting, such that it becomes
>> immediately clear what parentheses are still open. Alternatively:
>>
>> #define mfn_set_color(mfn, color) \
>>     (_mfn((mfn_x(mfn) & ~mfn_color_mask) | (color)))
>>
>> This is certainly an "interesting" construct: I, for one, wouldn't expect
>> that setting the color actually changes the MFN.
> 
> Would something like mfn_with_color() be a better name? I need something that
> expresses clearly that something will be returned. Maybe colored_mfn() is even
> better?

The latter reads as if it was a predicate, not a transformation. The former
or get_mfn_with_color() _may_ be okay. Without the get_ it's still a little
predicate-like, while the get_ itself somewhat collides with other uses of
that prefix, specifically e.g. get_page{,_type}(). So I'm still not overly
happy, yet e.g. mfn_from_mfn_and_color() feels clumsy to me.

>>> --- a/xen/include/xen/llc-coloring.h
>>> +++ b/xen/include/xen/llc-coloring.h
>>> @@ -24,6 +24,17 @@ static inline void domain_llc_coloring_free(struct 
>>> domain *d) {}
>>>  static inline void domain_dump_llc_colors(const struct domain *d) {}
>>>  #endif
>>>
>>> +/**
>>> + * Iterate over each Xen mfn in the colored space.
>>> + * @mfn:    the current mfn. The first non colored mfn must be provided as 
>>> the
>>> + *          starting point.
>>> + * @i:      loop index.
>>> + */
>>> +#define for_each_xen_colored_mfn(mfn, i)        \
>>> +    for ( i = 0, mfn = xen_colored_mfn(mfn);    \
>>> +          i < (_end - _start) >> PAGE_SHIFT;    \
>>> +          i++, mfn = xen_colored_mfn(mfn_add(mfn, 1)) )
>>
>> While the comment mentions it, I still consider it problematic that
>> - unlike other for_each_* constructs we have - this requires one of
>> the iteration variables to be set up front. Question is why it needs
>> to be that way: Isn't it the MFN underlying _start which you mean to
>> start from?
> 
> As said above, this is used also when page tables setup isn't complete
> so I can't easily find the first MFN.

Did you consider making the initial value a macro parameter then?

Jan