[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v6 14/15] xen/arm: add cache coloring support for Xen
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. >>> --- 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
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |