| Message ID | 20220304174701.1453977-5-marco.solieri@minervasys.tech (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Arm cache coloring | expand |
Hi, On 04/03/2022 17:46, Marco Solieri wrote: > From: Luca Miccio <lucmiccio@gmail.com> > > 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. > - 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? > > Signed-off-by: Luca Miccio <lucmiccio@gmail.com> > Signed-off-by: Marco Solieri <marco.solieri@minervasys.tech> > Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com> > --- > 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 <lucmiccio@gmail.com> > + * > + * 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 <lucmiccio@gmail.com> > + * > + * 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,
Hi Julien, Il giorno mer 9 mar 2022 alle ore 20:09 Julien Grall <julien@xen.org> ha scritto: > Hi, > > On 04/03/2022 17:46, Marco Solieri wrote: > > From: Luca Miccio <lucmiccio@gmail.com> > > > > 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 <lucmiccio@gmail.com> > > Signed-off-by: Marco Solieri <marco.solieri@minervasys.tech> > > Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com> > > --- > > 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 <lucmiccio@gmail.com> > > + * > > + * 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 <lucmiccio@gmail.com> > > + * > > + * 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 >
On 22/03/2022 09:17, Luca Miccio wrote: > Hi Julien, Hi Luca, >>> - 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. You could use a bitmap. Xen already provide facilities to use them in the public interface (see xenctl_bitmap) and convert the Xen internal bitmap (see DECLARE_BITMAP). Cheers,
Hi Julien, I'm Carlo, the new developer that will work on this patch set and on the review. Thanks for all the comments. I'll try to answer to all the open points and also ask for feedback. On 09/03/22 20:09, Julien Grall wrote: >> - 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? This parameter is here mainly to support QEMU on which the automatic probing of the LLC size doesn't work properly. Also, since from this value we compute the maximum number of colors the architecture supports, you may want to fix the way size so as to simulate a different use case for debugging purposes. Should I add those notes somewhere (doc, commit messages, etc.)? >> 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? Having another parameter to configure can complicate things from the user perspective. Also 128 is more than enough for the current ARM processors we tested. >> Signed-off-by: Luca Miccio <lucmiccio@gmail.com> >> Signed-off-by: Marco Solieri <marco.solieri@minervasys.tech> >> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com> >> --- >> 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? Because arm32 isn't an "interesting" architecture where to have coloring since there are locking primitives that provides sufficient isolation and so the problem is not common. On x86 instead, the functions that map memory into caches are not so easy to exploit to achieve isolation. Thanks. - Carlo Nonato
On 13/05/2022 15:22, Carlo Nonato wrote: > Hi Julien, Hi Carlo, > I'm Carlo, the new developer that will work on this patch set and on the > review. > > Thanks for all the comments. I'll try to answer to all the open points > and also > ask for feedback. > > On 09/03/22 20:09, Julien Grall wrote: >>> - 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? > This parameter is here mainly to support QEMU on which the automatic > probing > of the LLC size doesn't work properly. I am not in favor of adding command line option just for QEMU. But... > > Also, since from this value we compute the maximum number of colors > the architecture supports, you may want to fix the way size so as to > simulate > a different use case for debugging purposes. ... this reason is more compelling to me. > > Should I add those notes somewhere (doc, commit messages, etc.)? So I would mention it in the commit message and also the doc description the options. > >>> 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? > Having another parameter to configure can complicate things from > the user perspective. I don't think it would be more complicated. The default would still be 128 and would help the user to easily modify the value if... > Also 128 is more than enough for the current ARM > processors we tested. ... they are using a processor you didn't tested on. >>> Signed-off-by: Luca Miccio <lucmiccio@gmail.com> >>> Signed-off-by: Marco Solieri <marco.solieri@minervasys.tech> >>> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com> >>> --- >>> 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? > Because arm32 isn't an "interesting" architecture where to have coloring > since there are locking primitives that provides sufficient isolation > and so > the problem is not common. I am afraid I don't understand this rationale. What sort of locking are you talking about? That said,I am not asking to implement the 32-bit side. I am more interested to know what's the effort required here. IOW, is it disabled because you haven't tested? Cheers,
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 + depends on ARM_64 + 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 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 <lucmiccio@gmail.com> + * + * 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> + +/* 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]; + +/* 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) +{ + int start, end, i; + 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; + + 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 <lucmiccio@gmail.com> + * + * 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__ */