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
