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

[Carlo Nonato <carlo.nonato@xxxxxxxxxxxxxxx>]

Hi Jan,

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)
> >      flush_xen_tlb_local();
> >  }
> >
> > +static void __init create_llc_coloring_mappings(void)
> > +{
> > +    lpae_t pte;
> > +    unsigned int i;
> > +    struct bootmodule *xen_bootmodule = 
> > boot_module_find_by_kind(BOOTMOD_XEN);
> > +    mfn_t mfn = maddr_to_mfn(xen_bootmodule->start);
> > +
> > +    for_each_xen_colored_mfn( mfn, i )
>
> Nit: Either you consider the construct a pseudo-keyword, then
>
>     for_each_xen_colored_mfn ( mfn, i )
>
> or you don't, then
>
>     for_each_xen_colored_mfn(mfn, i)
>
> please.
>
> > --- 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?

> > @@ -316,6 +318,27 @@ unsigned int get_max_nr_llc_colors(void)
> >      return max_nr_colors;
> >  }
> >
> > +paddr_t __init xen_colored_map_size(void)
> > +{
> > +    return ROUNDUP((_end - _start) * max_nr_colors, XEN_PADDR_ALIGN);
> > +}
> > +
> > +mfn_t __init xen_colored_mfn(mfn_t mfn)
> > +{
> > +    unsigned int i, color = mfn_to_color(mfn);
> > +
> > +    for( i = 0; i < xen_num_colors; i++ )
>
> Nit: Missing blank.
>
> > +    {
> > +        if ( color == xen_colors[i] )
> > +            return mfn;
> > +        else if ( color < xen_colors[i] )
> > +            return mfn_set_color(mfn, xen_colors[i]);
>
> How do you know that this or ...
>
> > +    }
> > +
> > +    /* Jump to next color space (max_nr_colors mfns) and use the first 
> > color */
> > +    return mfn_set_color(mfn_add(mfn, max_nr_colors), xen_colors[0]);
>
> ... this MFN are actually valid and in (available) RAM?

Xen must be relocated in a valid and available physically colored space.
In arm we do that by searching in RAM for a contiguous region that can contain
the colored version of Xen (including "holes" of memory that is skipped due to
coloring). So we know that if mfn is in this region then the computed colored
MFN is in the same valid region as well. I should ASSERT that somehow.
This should be something like virt_to_mfn(_start) < mfn < virt_to_mfn(_end)
(abusing a bit of syntax), but the problem is that this function is called
also when page tables are still not set so I can't count on virt_to_mfn().

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

Thanks.

> Jan