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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

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

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

Jan