On 02.01.2024 10:51, Carlo Nonato wrote: > This commit adds the Last Level Cache (LLC) coloring common header, Kconfig > options and functions. Since this is an arch specific feature, actual > implementation is postponed to later patches and Kconfig options are placed > under xen/arch.
As a general remark / nit: "This commit", "this patch", or alike aren't well suited for descriptions. > --- a/xen/arch/Kconfig > +++ b/xen/arch/Kconfig > @@ -31,3 +31,19 @@ config NR_NUMA_NODES > associated with multiple-nodes management. It is the upper bound of > the number of NUMA nodes that the scheduler, memory allocation and > other NUMA-aware components can handle. > + > +config LLC_COLORING > + bool "Last Level Cache (LLC) coloring" if EXPERT > + depends on HAS_LLC_COLORING > + > +config NR_LLC_COLORS > + int "Maximum number of LLC colors" > + default 128 What if I set to value to 0? Or to an unreasonably large one? You don't bound the value range at all. > --- a/xen/common/domain.c > +++ b/xen/common/domain.c > @@ -7,6 +7,7 @@ > #include <xen/compat.h> > #include <xen/init.h> > #include <xen/lib.h> > +#include <xen/llc-coloring.h> > #include <xen/ctype.h> > #include <xen/err.h> > #include <xen/param.h> > @@ -1144,6 +1145,9 @@ static void cf_check complete_domain_destroy(struct > rcu_head *head) > struct vcpu *v; > int i; > > + if ( is_domain_llc_colored(d) ) > + domain_llc_coloring_free(d); Would be nice if the freeing function could be called unconditionally, being a no-op for non-colored domains. Further - is it really necessary to do this freeing this late? > --- a/xen/common/keyhandler.c > +++ b/xen/common/keyhandler.c > @@ -6,6 +6,7 @@ > #include <xen/debugger.h> > #include <xen/delay.h> > #include <xen/keyhandler.h> > +#include <xen/llc-coloring.h> > #include <xen/param.h> > #include <xen/shutdown.h> > #include <xen/event.h> > @@ -307,6 +308,9 @@ static void cf_check dump_domains(unsigned char key) > > arch_dump_domain_info(d); > > + if ( is_domain_llc_colored(d) ) > + domain_dump_llc_colors(d); I'm less concerned of the conditional here, but along the lines of the comment above, it could of course again be the function that simply is a no-op for non-colored domains. > --- /dev/null > +++ b/xen/include/xen/llc-coloring.h > @@ -0,0 +1,46 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * Last Level Cache (LLC) coloring common header > + * > + * Copyright (C) 2022 Xilinx Inc. > + * > + * Authors: > + * Carlo Nonato <carlo.non...@minervasys.tech> > + */ > +#ifndef __COLORING_H__ > +#define __COLORING_H__ > + > +#include <xen/sched.h> > +#include <public/domctl.h> > + > +#ifdef CONFIG_HAS_LLC_COLORING Why does this matter here? IOW why ... > +#include <asm/llc-coloring.h> > + > +#ifdef CONFIG_LLC_COLORING ... is it not just this which is checked? > +extern bool llc_coloring_enabled; > +#define llc_coloring_enabled (llc_coloring_enabled) > +#endif > + > +#endif > + > +#ifndef llc_coloring_enabled > +#define llc_coloring_enabled (false) > +#endif +1 to the question Julien has raised here. > --- a/xen/include/xen/sched.h > +++ b/xen/include/xen/sched.h > @@ -626,6 +626,11 @@ struct domain > > /* Holding CDF_* constant. Internal flags for domain creation. */ > unsigned int cdf; > + > +#ifdef CONFIG_LLC_COLORING > + unsigned int *llc_colors; Can the color values change over the lifetime of a domain? If not, it may be prudent to have this be pointer-to-const. Jan > + unsigned int num_llc_colors; > +#endif > }; > > static inline struct page_list_head *page_to_list(
