Hi Julien,
Il giorno mer 9 mar 2022 alle ore 20:09 Julien Grall <[email protected]> ha scritto: > Hi, > > On 04/03/2022 17:46, Marco Solieri wrote: > > From: Luca Miccio <[email protected]> > > > > Add three new bootargs allowing configuration of cache coloring support > > for Xen: > > I would prefer if documentation of each command line is part of the > patch introducing them. This would help understanding some of the > parameters. > > Ok. > > - way_size: The size of a LLC way in bytes. This value is mainly used > > to calculate the maximum available colors on the platform. > > We should only add command line option when they are a strong use case. > In documentation, you wrote that someone may want to overwrite the way > size for "specific needs". > > Can you explain what would be those needs? > - dom0_colors: The coloring configuration for Dom0, which also acts as > > default configuration for any DomU without an explicit configuration. > > - xen_colors: The coloring configuration for the Xen hypervisor itself. > > > > A cache coloring configuration consists of a selection of colors to be > > assigned to a VM or to the hypervisor. It is represented by a set of > > ranges. Add a common function that parses a string with a > > comma-separated set of hyphen-separated ranges like "0-7,15-16" and > > returns both: the number of chosen colors, and an array containing their > > ids. > > Currently we support platforms with up to 128 colors. > > Is there any reason this value is hardcoded in Xen rather than part of > the Kconfig? > > Not really at the time when this patch was created. But as we notify in patch 32, there is an assert that fails if we use a certain amount of colors. Maybe we should find a better way to store the color information. Luca. > > > > Signed-off-by: Luca Miccio <[email protected]> > > Signed-off-by: Marco Solieri <[email protected]> > > Signed-off-by: Stefano Stabellini <[email protected]> > > --- > > xen/arch/arm/Kconfig | 5 ++ > > xen/arch/arm/Makefile | 2 +- > > xen/arch/arm/coloring.c | 131 ++++++++++++++++++++++++++++ > > xen/arch/arm/include/asm/coloring.h | 28 ++++++ > > 4 files changed, 165 insertions(+), 1 deletion(-) > > create mode 100644 xen/arch/arm/coloring.c > > create mode 100644 xen/arch/arm/include/asm/coloring.h > > > > diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig > > index ecfa6822e4..f0f999d172 100644 > > --- a/xen/arch/arm/Kconfig > > +++ b/xen/arch/arm/Kconfig > > @@ -97,6 +97,11 @@ config HARDEN_BRANCH_PREDICTOR > > > > If unsure, say Y. > > > > +config COLORING > > + bool "L2 cache coloring" > > + default n > > This wants to be gated with EXPERT for time-being. SUPPORT.MD woudl > Furthermore, I think this wants to be gated with EXPERT for the time-being. > > > + depends on ARM_64 > > Why is this limited to arm64? > > > + > > config TEE > > bool "Enable TEE mediators support (UNSUPPORTED)" if UNSUPPORTED > > default n > > diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile > > index c993ce72a3..581896a528 100644 > > --- a/xen/arch/arm/Makefile > > +++ b/xen/arch/arm/Makefile > > @@ -66,7 +66,7 @@ obj-$(CONFIG_SBSA_VUART_CONSOLE) += vpl011.o > > obj-y += vsmc.o > > obj-y += vpsci.o > > obj-y += vuart.o > > - > > +obj-$(CONFIG_COLORING) += coloring.o > > Please keep the newline before extra-y. The file are meant to be ordered > alphabetically. So this should be inserted in the correct position. > > > extra-y += xen.lds > > > > #obj-bin-y += ....o > > diff --git a/xen/arch/arm/coloring.c b/xen/arch/arm/coloring.c > > new file mode 100644 > > index 0000000000..8f1cff6efb > > --- /dev/null > > +++ b/xen/arch/arm/coloring.c > > @@ -0,0 +1,131 @@ > > +/* > > + * xen/arch/arm/coloring.c > > + * > > + * Coloring support for ARM > > + * > > + * Copyright (C) 2019 Xilinx Inc. > > + * > > + * Authors: > > + * Luca Miccio <[email protected]> > > + * > > + * This program is free software; you can redistribute it and/or modify > > + * it under the terms of the GNU General Public License version 2 as > > + * published by the Free Software Foundation. > > + * > > + * This program is distributed in the hope that it will be useful, > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > + * GNU General Public License for more details. > > + * > > + * You should have received a copy of the GNU General Public License > > + * along with this program. If not, see <http://www.gnu.org/licenses/ > >. > > + */ > > +#include <xen/init.h> > > +#include <xen/types.h> > > +#include <xen/lib.h> > > +#include <xen/errno.h> > > +#include <xen/param.h> > > +#include <asm/coloring.h> > > The includes should be ordered so <xen/...> are first, then <asm/...>. > They are also ordered alphabetically within their own category. > > > + > > +/* Number of color(s) assigned to Xen */ > > +static uint32_t xen_col_num; > > +/* Coloring configuration of Xen as bitmask */ > > +static uint32_t xen_col_mask[MAX_COLORS_CELLS]; > Xen provides helpers to create and use bitmaps (see > include/xen/bitmap.h). Can you use? > > > + > > +/* Number of color(s) assigned to Dom0 */ > > +static uint32_t dom0_col_num; > > +/* Coloring configuration of Dom0 as bitmask */ > > +static uint32_t dom0_col_mask[MAX_COLORS_CELLS]; > > + > > +static uint64_t way_size; > > + > > +/************************* > > + * PARSING COLORING BOOTARGS > > + */ > > + > > +/* > > + * Parse the coloring configuration given in the buf string, following > the > > + * syntax below, and store the number of colors and a corresponding > mask in > > + * the last two given pointers. > > + * > > + * COLOR_CONFIGURATION ::= RANGE,...,RANGE > > + * RANGE ::= COLOR-COLOR > > + * > > + * Example: "2-6,15-16" represents the set of colors: 2,3,4,5,6,15,16. > > + */ > > +static int parse_color_config( > > + const char *buf, uint32_t *col_mask, uint32_t *col_num) > > > Coding style. We usually declarate paremeters on the same line as the > function name. If they can't fit on the same line, then we split in two > with the parameter aligned to the first paremeter. > > > +{ > > + int start, end, i; > > AFAICT, none of the 3 variables will store negative values. So can they > be unsigned? > > > + const char* s = buf; > > + unsigned int offset; > > + > > + if ( !col_mask || !col_num ) > > + return -EINVAL; > > + > > + *col_num = 0; > > + for ( i = 0; i < MAX_COLORS_CELLS; i++ ) > > + col_mask[i] = 0; > dom0_col_mask and xen_col_mask are already zeroed. I would also expect > the same for dynamically allocated bitmask. So can this be dropped? > > > + > > + while ( *s != '\0' ) > > + { > > + if ( *s != ',' ) > > + { > > + start = simple_strtoul(s, &s, 0); > > + > > + /* Ranges are hyphen-separated */ > > + if ( *s != '-' ) > > + goto fail; > > + s++; > > + > > + end = simple_strtoul(s, &s, 0); > > + > > + for ( i = start; i <= end; i++ ) > > + { > > + offset = i / 32; > > + if ( offset > MAX_COLORS_CELLS ) > > + goto fail; > > + > > + if ( !(col_mask[offset] & (1 << i % 32)) ) > > + *col_num += 1; > > + col_mask[offset] |= (1 << i % 32); > > + } > > + } > > + else > > + s++; > > + } > > + > > + return *s ? -EINVAL : 0; > > +fail: > > + return -EINVAL; > > +} > > + > > +static int __init parse_way_size(const char *s) > > +{ > > + way_size = simple_strtoull(s, &s, 0); > > + > > + return *s ? -EINVAL : 0; > > +} > > +custom_param("way_size", parse_way_size); > > + > > +static int __init parse_dom0_colors(const char *s) > > +{ > > + return parse_color_config(s, dom0_col_mask, &dom0_col_num); > > +} > > +custom_param("dom0_colors", parse_dom0_colors); > > + > > +static int __init parse_xen_colors(const char *s) > > +{ > > + return parse_color_config(s, xen_col_mask, &xen_col_num); > > +} > > +custom_param("xen_colors", parse_xen_colors); > > + > > +/* > > + * Local variables: > > + * mode: C > > + * c-file-style: "BSD" > > + * c-basic-offset: 4 > > + * tab-width: 4 > > + * indent-tabs-mode: nil > > + * End: > > + */ > > diff --git a/xen/arch/arm/include/asm/coloring.h > b/xen/arch/arm/include/asm/coloring.h > > new file mode 100644 > > index 0000000000..60958d1244 > > --- /dev/null > > +++ b/xen/arch/arm/include/asm/coloring.h > > @@ -0,0 +1,28 @@ > > +/* > > + * xen/arm/include/asm/coloring.h > > + * > > + * Coloring support for ARM > > + * > > + * Copyright (C) 2019 Xilinx Inc. > > + * > > + * Authors: > > + * Luca Miccio <[email protected]> > > + * > > + * This program is free software; you can redistribute it and/or modify > > + * it under the terms of the GNU General Public License version 2 as > > + * published by the Free Software Foundation. > > + * > > + * This program is distributed in the hope that it will be useful, > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > + * GNU General Public License for more details. > > + * > > + * You should have received a copy of the GNU General Public License > > + * along with this program. If not, see <http://www.gnu.org/licenses/ > >. > > + */ > > +#ifndef __ASM_ARM_COLORING_H__ > > +#define __ASM_ARM_COLORING_H__ > > + > > +#define MAX_COLORS_CELLS 4 > > + > > +#endif /* !__ASM_ARM_COLORING_H__ */ > > Cheers, > > -- > Julien Grall >
