On 13.02.2024 18:29, Carlo Nonato wrote:
> On Tue, Feb 13, 2024 at 4:25 PM Jan Beulich <jbeul...@suse.com> 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
