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

[Carlo Nonato <carlo.nonato@xxxxxxxxxxxxxxx>]

Hi Jan,

On Wed, Feb 14, 2024 at 8:55 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> 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.

For the moment get_mfn_with_color() seems the best option.

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

Yes, this is better.

Thanks.

> Jan