* [PATCH v3 0/9] Arm cache coloring
@ 2022-10-22 15:51 Carlo Nonato
2022-10-22 15:51 ` [PATCH v3 1/9] xen/arm: add cache coloring initialization Carlo Nonato
` (9 more replies)
0 siblings, 10 replies; 26+ messages in thread
From: Carlo Nonato @ 2022-10-22 15:51 UTC (permalink / raw)
To: xen-devel
Cc: marco.solieri, andrea.bastoni, lucmiccio, Carlo Nonato,
Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk, Andrew Cooper, George Dunlap, Jan Beulich,
Wei Liu, Anthony PERARD, Juergen Gross
Shared caches in multi-core CPU architectures represent a problem for
predictability of memory access latency. This jeopardizes applicability
of many Arm platform in real-time critical and mixed-criticality
scenarios. We introduce support for cache partitioning with page
coloring, a transparent software technique that enables isolation
between domains and Xen, and thus avoids cache interference.
When creating a domain, a simple syntax (e.g. `0-3` or `4-11`) allows
the user to define assignments of cache partitions ids, called colors,
where assigning different colors guarantees no mutual eviction on cache
will ever happen. This instructs the Xen memory allocator to provide
the i-th color assignee only with pages that maps to color i, i.e. that
are indexed in the i-th cache partition.
The proposed implementation supports the dom0less feature.
The proposed implementation doesn't support the static-mem feature.
The solution has been tested in several scenarios, including Xilinx Zynq
MPSoCs.
Commits structure:
- [1-3] Coloring initialization, cache layout auto-probing and coloring
data for domains.
- [4-5] xl and Device Tree support for coloring.
- [6] New page allocator for domain memory that implement the cache
coloring mechanism.
- [7-9] Coloring support for Xen.
Global changes in v3:
- fixed a compilation error because of a forgotten "\"
- replaced some #ifdef with if ( IS_ENABLED )
- other minor changes (docs, typos, variable types, style, etc.)
- better acknowledged Luca Miccio as the original author
- removed #8 since the bootmodule address and size can be replaced without
the need of this particular revert
- removed #9 since it wasn't a clean revert and thanks to Julien things can
be done in a smarter way sticking with map_pages_to_xen() (see new #9)
Open points:
- The allocator proposed in #6 works only with order-0 pages and inserts
them in a sorted list using a linear search. This behavior can be slow if
large amount of memory is given to it, so the user is warned in the
documentation for that.
In a following patch, that I'm going to send separately, a simple buddy
allocator that indexes pages by color is presented. It can serve higher
order pages and doesn't need the linear search. Unfortunately, it has
some flaws that I will discuss there.
- I will address the latest v2 comments from Julien in v4
Acknowledgements
----------------
This work is sponsored by Xilinx Inc., and supported by University of
Modena and Reggio Emilia and Minerva Systems.
Carlo Nonato (6):
xen/arm: add cache coloring initialization
xen/arm: add cache coloring initialization for domains
tools/xl: add support for cache coloring configuration
xen/arm: add support for cache coloring configuration via device-tree
Revert "xen/arm: Remove unused BOOT_RELOC_VIRT_START"
xen/arm: add cache coloring support for Xen
Luca Miccio (3):
xen/arm: dump cache colors in domain info debug-key
xen/common: add cache coloring allocator for domains
xen/arm: add Xen cache colors command line parameter
docs/man/xl.cfg.5.pod.in | 10 +
docs/misc/arm/cache-coloring.rst | 229 ++++++++++++++
docs/misc/arm/device-tree/booting.txt | 4 +
docs/misc/xen-command-line.pandoc | 49 +++
tools/libs/light/libxl_create.c | 12 +
tools/libs/light/libxl_types.idl | 1 +
tools/xl/xl_parse.c | 52 +++-
xen/arch/arm/Kconfig | 34 +++
xen/arch/arm/Makefile | 1 +
xen/arch/arm/alternative.c | 9 +-
xen/arch/arm/arm64/head.S | 48 +++
xen/arch/arm/coloring.c | 411 ++++++++++++++++++++++++++
xen/arch/arm/domain.c | 9 +
xen/arch/arm/domain_build.c | 26 +-
xen/arch/arm/include/asm/coloring.h | 91 ++++++
xen/arch/arm/include/asm/config.h | 4 +-
xen/arch/arm/include/asm/domain.h | 4 +
xen/arch/arm/include/asm/mm.h | 19 +-
xen/arch/arm/include/asm/processor.h | 16 +
xen/arch/arm/mm.c | 95 +++++-
xen/arch/arm/p2m.c | 7 +-
xen/arch/arm/psci.c | 4 +-
xen/arch/arm/setup.c | 81 ++++-
xen/arch/arm/smpboot.c | 3 +-
xen/arch/arm/xen.lds.S | 2 +-
xen/common/Kconfig | 3 +
xen/common/page_alloc.c | 259 ++++++++++++++--
xen/include/public/arch-arm.h | 8 +
xen/include/xen/mm.h | 43 +++
29 files changed, 1487 insertions(+), 47 deletions(-)
create mode 100644 docs/misc/arm/cache-coloring.rst
create mode 100644 xen/arch/arm/coloring.c
create mode 100644 xen/arch/arm/include/asm/coloring.h
--
2.34.1
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v3 1/9] xen/arm: add cache coloring initialization
2022-10-22 15:51 [PATCH v3 0/9] Arm cache coloring Carlo Nonato
@ 2022-10-22 15:51 ` Carlo Nonato
2022-10-22 15:51 ` [PATCH v3 2/9] xen/arm: add cache coloring initialization for domains Carlo Nonato
` (8 subsequent siblings)
9 siblings, 0 replies; 26+ messages in thread
From: Carlo Nonato @ 2022-10-22 15:51 UTC (permalink / raw)
To: xen-devel
Cc: marco.solieri, andrea.bastoni, lucmiccio, Carlo Nonato,
Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk, Andrew Cooper, George Dunlap, Jan Beulich,
Wei Liu, Marco Solieri
This commit adds the cache coloring support initialization, Kconfig
options, command line parameters and the initial documentation.
The initialization consists of an auto probing of the cache layout
necessary to retrieve the LLC way size which is used to compute the
number of available colors. The Dom0 colors are then initialized with
default colors (all available ones) if not provided from the command line,
and they are checked for bad configuration.
It also adds a debug-key to dump general cache coloring info.
This includes LLC way size, total available colors and the mask used to
extract colors from physical addresses.
Based on original work from: Luca Miccio <lucmiccio@gmail.com>
Signed-off-by: Carlo Nonato <carlo.nonato@minervasys.tech>
Signed-off-by: Marco Solieri <marco.solieri@minervasys.tech>
---
v3:
- HAS_CACHE_COLORING config added
- fixed CONFIG_MAX_CACHE_COLORS range and explained why such range
- __ro_after_init for coloring globals
- stub functions in case of coloring disabled, in coloring.c (like in v1)
- check number of colors in range [2, CONFIG_MAX_CACHE_COLORS] at runtime
- LLC way size and number of colors must be a power of 2
(explained in docs and checked at runtime)
---
docs/misc/arm/cache-coloring.rst | 135 +++++++++++++++
docs/misc/xen-command-line.pandoc | 26 +++
xen/arch/arm/Kconfig | 22 +++
xen/arch/arm/Makefile | 1 +
xen/arch/arm/coloring.c | 243 +++++++++++++++++++++++++++
xen/arch/arm/include/asm/coloring.h | 38 +++++
xen/arch/arm/include/asm/processor.h | 16 ++
xen/arch/arm/setup.c | 7 +
xen/common/Kconfig | 3 +
9 files changed, 491 insertions(+)
create mode 100644 docs/misc/arm/cache-coloring.rst
create mode 100644 xen/arch/arm/coloring.c
create mode 100644 xen/arch/arm/include/asm/coloring.h
diff --git a/docs/misc/arm/cache-coloring.rst b/docs/misc/arm/cache-coloring.rst
new file mode 100644
index 0000000000..b0f9a2e917
--- /dev/null
+++ b/docs/misc/arm/cache-coloring.rst
@@ -0,0 +1,135 @@
+Xen cache coloring user guide
+=============================
+
+The cache coloring support in Xen allows to reserve Last Level Cache (LLC)
+partition for Dom0, DomUs and Xen itself. Currently only ARM64 is supported.
+
+In order to enable and use it, few steps are needed.
+
+- Enable expert mode in Xen configuration file.
+
+ CONFIG_EXPERT=y
+- Enable cache coloring in Xen configuration file.
+
+ CONFIG_CACHE_COLORING=y
+- If needed, change the maximum number of colors in Xen configuration file
+ (refer to menuconfig help for value meaning and when it should be changed).
+
+ CONFIG_MAX_CACHE_COLORS=<n>
+- Assign colors to Dom0 using the `Color selection format`_ (see
+ `Coloring parameters`_ for more documentation pointers).
+
+Background
+**********
+
+Cache hierarchy of a modern multi-core CPU typically has first levels dedicated
+to each core (hence using multiple cache units), while the last level is shared
+among all of them. Such configuration implies that memory operations on one
+core (e.g. running a DomU) are able to generate interference on another core
+(e.g .hosting another DomU). Cache coloring allows eliminating this
+mutual interference, and thus guaranteeing higher and more predictable
+performances for memory accesses.
+The key concept underlying cache coloring is a fragmentation of the memory
+space into a set of sub-spaces called colors that are mapped to disjoint cache
+partitions. Technically, the whole memory space is first divided into a number
+of subsequent regions. Then each region is in turn divided into a number of
+subsequent sub-colors. The generic i-th color is then obtained by all the
+i-th sub-colors in each region.
+
+.. raw:: html
+
+ <pre>
+ Region j Region j+1
+ ..................... ............
+ . . .
+ . .
+ _ _ _______________ _ _____________________ _ _
+ | | | | | | |
+ | c_0 | c_1 | | c_n | c_0 | c_1 |
+ _ _ _|_____|_____|_ _ _|_____|_____|_____|_ _ _
+ : :
+ : :... ... .
+ : color 0
+ :........................... ... .
+ :
+ . . ..................................:
+ </pre>
+
+There are two pragmatic lesson to be learnt.
+
+1. If one wants to avoid cache interference between two domains, different
+ colors needs to be used for their memory.
+
+2. Color assignment must privilege contiguity in the partitioning. E.g.,
+ assigning colors (0,1) to domain I and (2,3) to domain J is better than
+ assigning colors (0,2) to I and (1,3) to J.
+
+How to compute the number of colors
+***********************************
+
+To compute the number of available colors for a specific platform, the size of
+an LLC way and the page size used by Xen must be known. The first parameter can
+be found in the processor manual or can be also computed dividing the total
+cache size by the number of its ways. The second parameter is the minimum amount
+of memory that can be mapped by the hypervisor, thus dividing the way size by
+the page size, the number of total cache partitions is found. So for example,
+an Arm Cortex-A53 with a 16-ways associative 1 MiB LLC, can isolate up to 16
+colors when pages are 4 KiB in size.
+
+Cache layout is probed automatically by Xen itself, but a possibility to
+manually set the way size it's left for the user to overcome failing situations
+or for debugging/testing purposes. See `Coloring parameters`_ section for more
+information on that.
+
+Colors selection format
+***********************
+
+Regardless of the memory pool that has to be colored (Xen, Dom0/DomUs),
+the color selection can be expressed using the same syntax. In particular a
+comma-separated list of colors or ranges of colors is used.
+Ranges are hyphen-separated intervals (such as `0-4`) and are inclusive on both
+sides.
+
+Note that:
+ - no spaces are allowed between values.
+ - no overlapping ranges or duplicated colors are allowed.
+ - values must be written in ascending order.
+
+Examples:
+
++---------------------+-----------------------------------+
+|**Configuration** |**Actual selection** |
++---------------------+-----------------------------------+
+| 1-2,5-8 | [1, 2, 5, 6, 7, 8] |
++---------------------+-----------------------------------+
+| 4-8,10,11,12 | [4, 5, 6, 7, 8, 10, 11, 12] |
++---------------------+-----------------------------------+
+| 0 | [0] |
++---------------------+-----------------------------------+
+
+Coloring parameters
+*******************
+
+LLC way size (as previously discussed) and Dom0 colors can be set using the
+appropriate command line parameters. See the relevant documentation in
+"docs/misc/xen-command-line.pandoc".
+
+Known issues and limitations
+****************************
+
+Cache coloring is intended only for embedded systems
+####################################################
+
+The current implementation aims to satisfy the need of predictability in
+embedded systems with small amount of memory to be managed in a colored way.
+Given that, some shortcuts are taken in the development. Expect worse
+performances on larger systems.
+
+The maximum number of colors supported is 32768
+###############################################
+
+The upper bound of the CONFIG_MAX_CACHE_COLORS range (which is an upper bound
+too) is set to 2^15 = 32768 colors because of some limitation on the domain
+configuration structure size used in domain creation. "uint16_t" is the biggest
+integer type that fit the constraint and 2^15 is the biggest power of 2 it can
+easily represent. This value is big enough for the generic case, though.
diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index 68389843b2..3f04414134 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -903,6 +903,14 @@ Controls for the dom0 IOMMU setup.
Incorrect use of this option may result in a malfunctioning system.
+### dom0-colors (arm64)
+> `= List of [ <integer> | <integer>-<integer> ]`
+
+> Default: `All available colors`
+
+Specify dom0 color configuration. If the parameter is not set, all available
+colors are chosen and the user is warned on Xen's serial console.
+
### dom0_ioports_disable (x86)
> `= List of <hex>-<hex>`
@@ -1645,6 +1653,24 @@ This option is intended for debugging purposes only. Enable MSR_DEBUGCTL.LBR
in hypervisor context to be able to dump the Last Interrupt/Exception To/From
record with other registers.
+### llc-way-size (arm64)
+> `= <size>`
+
+> Default: `Obtained from the hardware`
+
+Specify the way size of the Last Level Cache. This parameter is only useful with
+cache coloring support enabled. It is an optional, expert-only parameter and it
+is used to calculate the number of available colors on the platform. It can be
+obtained by dividing the total LLC size by the number of its associative ways.
+By default, the value is automatically computed by probing the hardware, but in
+case of specific needs, it can be manually set. Those include failing probing
+and debugging/testing purposes so that it's possibile to emulate platforms with
+different number of supported colors.
+An important detail to highlight is that the current implementation of the
+cache coloring technique requires the number of colors to be a power of 2, and
+consequently, also the LLC way size must be so. A value that doesn't match this
+requirement is aligned down to the previous power of 2.
+
### loglvl
> `= <level>[/<rate-limited level>]` where level is `none | error | warning | info | debug | all`
diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
index 1fe5faf847..c45a9c5917 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -9,6 +9,7 @@ config ARM_64
select 64BIT
select ARM_EFI
select HAS_FAST_MULTIPLY
+ select HAS_CACHE_COLORING
config ARM
def_bool y
@@ -131,6 +132,27 @@ config ARM64_BTI
Branch Target Identification support.
This feature is not supported in Xen.
+config CACHE_COLORING
+ bool "Last Level Cache (LLC) coloring" if EXPERT
+ depends on HAS_CACHE_COLORING
+
+config MAX_CACHE_COLORS
+ int "Maximum number of cache colors"
+ default 128
+ range 2 32768
+ depends on CACHE_COLORING
+ help
+ This config value is an upper bound for the actual number of cache colors
+ supported by the architecture. Xen preallocates this amount of cache
+ colors at boot. Refer to the documentation for how to compute the number
+ of colors supported by the platform.
+ The default value corresponds to an 8 MiB 16-ways LLC, which should be
+ more than what needed in the normal case.
+ The max value corresponds to a 2 GiB 16-ways LLC which should never be
+ reached.
+ Note that if, at any time, a color configuration with more colors than the
+ maximum is employed, an error is produced.
+
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 4d076b278b..12940ba761 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -67,6 +67,7 @@ obj-$(CONFIG_SBSA_VUART_CONSOLE) += vpl011.o
obj-y += vsmc.o
obj-y += vpsci.o
obj-y += vuart.o
+obj-$(CONFIG_CACHE_COLORING) += coloring.o
extra-y += xen.lds
diff --git a/xen/arch/arm/coloring.c b/xen/arch/arm/coloring.c
new file mode 100644
index 0000000000..36eea2d6c0
--- /dev/null
+++ b/xen/arch/arm/coloring.c
@@ -0,0 +1,243 @@
+/*
+ * xen/arch/arm/coloring.c
+ *
+ * Coloring support for ARM
+ *
+ * Copyright (C) 2019 Xilinx Inc.
+ *
+ * Authors:
+ * Luca Miccio <lucmiccio@gmail.com>
+ * Carlo Nonato <carlo.nonato@minervasys.tech>
+ *
+ * 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/bitops.h>
+#include <xen/errno.h>
+#include <xen/keyhandler.h>
+#include <xen/param.h>
+#include <xen/types.h>
+
+#include <asm/coloring.h>
+#include <asm/processor.h>
+#include <asm/sysregs.h>
+
+/* Size of an LLC way */
+static unsigned int __ro_after_init llc_way_size;
+/* Number of colors available in the LLC */
+static unsigned int __ro_after_init max_colors = CONFIG_MAX_CACHE_COLORS;
+/* Mask to retrieve coloring relevant bits */
+static uint64_t __ro_after_init addr_col_mask;
+
+#define addr_to_color(addr) (((addr) & addr_col_mask) >> PAGE_SHIFT)
+#define addr_set_color(addr, color) (((addr) & ~addr_col_mask) \
+ | ((color) << PAGE_SHIFT))
+
+static unsigned int dom0_colors[CONFIG_MAX_CACHE_COLORS];
+static unsigned int dom0_num_colors;
+
+/*
+ * Parse the coloring configuration given in the buf string, following the
+ * syntax below.
+ *
+ * COLOR_CONFIGURATION ::= COLOR | RANGE,...,COLOR | RANGE
+ * RANGE ::= COLOR-COLOR
+ *
+ * Example: "0,2-6,15-16" represents the set of colors: 0,2,3,4,5,6,15,16.
+ */
+static int parse_color_config(const char *buf, unsigned int *colors,
+ unsigned int *num_colors)
+{
+ const char *s = buf;
+
+ if ( !colors || !num_colors )
+ return -EINVAL;
+
+ while ( *s != '\0' )
+ {
+ if ( *s != ',' )
+ {
+ unsigned int color, start, end;
+
+ start = simple_strtoul(s, &s, 0);
+
+ if ( *s == '-' ) /* Range */
+ {
+ s++;
+ end = simple_strtoul(s, &s, 0);
+ }
+ else /* Single value */
+ end = start;
+
+ if ( start > end ||
+ *num_colors + end - start >= max_colors )
+ return -EINVAL;
+ for ( color = start; color <= end; color++ )
+ colors[(*num_colors)++] = color;
+ }
+ else
+ s++;
+ }
+
+ return *s ? -EINVAL : 0;
+}
+
+size_param("llc-way-size", llc_way_size);
+
+static int __init parse_dom0_colors(const char *s)
+{
+ return parse_color_config(s, dom0_colors, &dom0_num_colors);
+}
+custom_param("dom0-colors", parse_dom0_colors);
+
+/* Return the LLC way size by probing the hardware */
+static unsigned int __init get_llc_way_size(void)
+{
+ register_t ccsidr_el1;
+ register_t clidr_el1 = READ_SYSREG(CLIDR_EL1);
+ register_t csselr_el1 = READ_SYSREG(CSSELR_EL1);
+ register_t id_aa64mmfr2_el1 = READ_SYSREG(ID_AA64MMFR2_EL1);
+ uint32_t ccsidr_numsets_shift = CCSIDR_NUMSETS_SHIFT;
+ uint32_t ccsidr_numsets_mask = CCSIDR_NUMSETS_MASK;
+ unsigned int n, line_size, num_sets;
+
+ for ( n = CLIDR_CTYPEn_LEVELS;
+ n != 0 && !((clidr_el1 >> CLIDR_CTYPEn_SHIFT(n)) & CLIDR_CTYPEn_MASK);
+ n-- );
+
+ if ( n == 0 )
+ return 0;
+
+ WRITE_SYSREG(((n - 1) & CCSELR_LEVEL_MASK) << CCSELR_LEVEL_SHIFT,
+ CSSELR_EL1);
+ isb();
+
+ ccsidr_el1 = READ_SYSREG(CCSIDR_EL1);
+
+ /* Arm ARM: (Log2(Number of bytes in cache line)) - 4 */
+ line_size = 1 << ((ccsidr_el1 & CCSIDR_LINESIZE_MASK) + 4);
+
+ /* If FEAT_CCIDX is enabled, CCSIDR_EL1 has a different bit layout */
+ if ( (id_aa64mmfr2_el1 >> ID_AA64MMFR2_CCIDX_SHIFT) & 0x7 )
+ {
+ ccsidr_numsets_shift = CCSIDR_NUMSETS_SHIFT_FEAT_CCIDX;
+ ccsidr_numsets_mask = CCSIDR_NUMSETS_MASK_FEAT_CCIDX;
+ }
+ /* Arm ARM: (Number of sets in cache) - 1 */
+ num_sets = ((ccsidr_el1 >> ccsidr_numsets_shift) & ccsidr_numsets_mask) + 1;
+
+ printk(XENLOG_INFO "LLC found: L%u (line size: %u bytes, sets num: %u)\n",
+ n, line_size, num_sets);
+
+ /* Restore value in CSSELR_EL1 */
+ WRITE_SYSREG(csselr_el1, CSSELR_EL1);
+ isb();
+
+ return line_size * num_sets;
+}
+
+static bool check_colors(unsigned int *colors, unsigned int num_colors)
+{
+ unsigned int i;
+
+ if ( num_colors > max_colors )
+ return false;
+
+ for ( i = 0; i < num_colors; i++ )
+ if ( colors[i] >= max_colors )
+ return false;
+
+ return true;
+}
+
+static unsigned int set_default_domain_colors(unsigned int *colors)
+{
+ unsigned int i;
+
+ if ( !colors )
+ return 0;
+
+ for ( i = 0; i < max_colors; i++ )
+ colors[i] = i;
+ return max_colors;
+}
+
+static void dump_coloring_info(unsigned char key)
+{
+ printk("'%c' pressed -> dumping coloring general info\n", key);
+ printk("LLC way size: %u KiB\n", llc_way_size >> 10);
+ printk("Number of LLC colors supported: %u\n", max_colors);
+ printk("Address color mask: 0x%lx\n", addr_col_mask);
+}
+
+bool __init coloring_init(void)
+{
+ if ( !llc_way_size && !(llc_way_size = get_llc_way_size()) )
+ {
+ printk(XENLOG_ERR
+ "Probed LLC way size is 0 and no custom value provided\n");
+ return false;
+ }
+
+ /*
+ * The maximum number of colors must be a power of 2 in order to correctly
+ * map colors to bits of an address, so also the LLC way size must be so.
+ */
+ if ( llc_way_size & (llc_way_size - 1) )
+ {
+ printk(XENLOG_WARNING "LLC way size (%u) isn't a power of 2.\n",
+ llc_way_size);
+ llc_way_size = 1U << flsl(llc_way_size);
+ printk(XENLOG_WARNING
+ "Using %u instead. Performances will be suboptimal\n",
+ llc_way_size);
+ }
+
+ max_colors = llc_way_size >> PAGE_SHIFT;
+
+ if ( max_colors < 2 || max_colors > CONFIG_MAX_CACHE_COLORS )
+ {
+ printk(XENLOG_ERR
+ "Max number of colors (%u) not in range [2, config max (%u)]\n",
+ max_colors, CONFIG_MAX_CACHE_COLORS);
+ return false;
+ }
+
+ addr_col_mask = (max_colors - 1) << PAGE_SHIFT;
+
+ if ( !dom0_num_colors )
+ {
+ printk(XENLOG_WARNING
+ "Dom0 color config not found. Using default (all colors)\n");
+ dom0_num_colors = set_default_domain_colors(dom0_colors);
+ }
+
+ if ( !check_colors(dom0_colors, dom0_num_colors) )
+ {
+ printk(XENLOG_ERR "Bad color config for Dom0\n");
+ return false;
+ }
+
+ register_keyhandler('K', dump_coloring_info, "dump coloring info", 1);
+
+ return true;
+}
+
+/*
+ * 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..3b563d3b90
--- /dev/null
+++ b/xen/arch/arm/include/asm/coloring.h
@@ -0,0 +1,38 @@
+/*
+ * xen/arm/include/asm/coloring.h
+ *
+ * Coloring support for ARM
+ *
+ * Copyright (C) 2019 Xilinx Inc.
+ *
+ * Authors:
+ * Luca Miccio <lucmiccio@gmail.com>
+ * Carlo Nonato <carlo.nonato@minervasys.tech>
+ *
+ * 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__
+
+#ifdef CONFIG_CACHE_COLORING
+
+#include <xen/init.h>
+
+bool __init coloring_init(void);
+
+#else /* !CONFIG_CACHE_COLORING */
+
+static inline bool __init coloring_init(void) { return true; }
+
+#endif /* CONFIG_CACHE_COLORING */
+#endif /* __ASM_ARM_COLORING_H__ */
diff --git a/xen/arch/arm/include/asm/processor.h b/xen/arch/arm/include/asm/processor.h
index 1dd81d7d52..85ff0caf1e 100644
--- a/xen/arch/arm/include/asm/processor.h
+++ b/xen/arch/arm/include/asm/processor.h
@@ -18,6 +18,22 @@
#define CTR_IDC_SHIFT 28
#define CTR_DIC_SHIFT 29
+/* CCSIDR Current Cache Size ID Register */
+#define CCSIDR_LINESIZE_MASK 0x7
+#define CCSIDR_NUMSETS_SHIFT 13
+#define CCSIDR_NUMSETS_MASK 0x3FFF
+#define CCSIDR_NUMSETS_SHIFT_FEAT_CCIDX 32
+#define CCSIDR_NUMSETS_MASK_FEAT_CCIDX 0xFFFFFF
+
+/* CCSELR Cache Size Selection Register */
+#define CCSELR_LEVEL_MASK 0x7
+#define CCSELR_LEVEL_SHIFT 1
+
+/* CLIDR Cache Level ID Register */
+#define CLIDR_CTYPEn_SHIFT(n) (3 * (n - 1))
+#define CLIDR_CTYPEn_MASK 0x7
+#define CLIDR_CTYPEn_LEVELS 7
+
#define ICACHE_POLICY_VPIPT 0
#define ICACHE_POLICY_AIVIVT 1
#define ICACHE_POLICY_VIPT 2
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 4395640019..acc3e4ad72 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -53,6 +53,7 @@
#include <asm/setup.h>
#include <xsm/xsm.h>
#include <asm/acpi.h>
+#include <asm/coloring.h>
struct bootinfo __initdata bootinfo;
@@ -1035,6 +1036,12 @@ void __init start_xen(unsigned long boot_phys_offset,
printk("Command line: %s\n", cmdline);
cmdline_parse(cmdline);
+ if ( IS_ENABLED(CONFIG_CACHE_COLORING) )
+ {
+ if ( !coloring_init() )
+ panic("Xen cache coloring support: setup failed\n");
+ }
+
setup_mm();
/* Parse the ACPI tables for possible boot-time configuration */
diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index f1ea3199c8..d7968127be 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -31,6 +31,9 @@ config ARCH_MAP_DOMAIN_PAGE
config HAS_ALTERNATIVE
bool
+config HAS_CACHE_COLORING
+ bool
+
config HAS_COMPAT
bool
--
2.34.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v3 2/9] xen/arm: add cache coloring initialization for domains
2022-10-22 15:51 [PATCH v3 0/9] Arm cache coloring Carlo Nonato
2022-10-22 15:51 ` [PATCH v3 1/9] xen/arm: add cache coloring initialization Carlo Nonato
@ 2022-10-22 15:51 ` Carlo Nonato
2022-10-25 12:07 ` Carlo Nonato
2022-11-21 14:50 ` Carlo Nonato
2022-10-22 15:51 ` [PATCH v3 3/9] xen/arm: dump cache colors in domain info debug-key Carlo Nonato
` (7 subsequent siblings)
9 siblings, 2 replies; 26+ messages in thread
From: Carlo Nonato @ 2022-10-22 15:51 UTC (permalink / raw)
To: xen-devel
Cc: marco.solieri, andrea.bastoni, lucmiccio, Carlo Nonato,
Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk, Marco Solieri
This commit adds array pointers to domains as well as to the hypercall
and configuration structure employed in domain creation. The latter is used
both by the toolstack and by Xen itself to pass configuration data to the
domain creation function, so the XEN_GUEST_HANDLE macro must be adopted to
be able to access guest memory in the first case. This implies special care
for the copy of the configuration data into the domain data, meaning that a
discrimination variable for the two possible code paths (coming from Xen or
from the toolstack) is needed.
The initialization and free functions for colored domains are also added.
The former is responsible for allocating and populating the color array
of the domain and it also checks for configuration issues. One of those
issues is enabling both coloring and directmap for the domain because they
contradicts one another. Since that, Dom0 must not be created with the
directmap flag.
The latter instead frees allocated memory.
Based on original work from: Luca Miccio <lucmiccio@gmail.com>
Signed-off-by: Carlo Nonato <carlo.nonato@minervasys.tech>
Signed-off-by: Marco Solieri <marco.solieri@minervasys.tech>
---
v3:
- xfree() for colors array in case of errors in domain_coloring_init()
---
docs/misc/arm/cache-coloring.rst | 14 ++++++-
xen/arch/arm/coloring.c | 57 +++++++++++++++++++++++++++++
xen/arch/arm/domain.c | 7 ++++
xen/arch/arm/domain_build.c | 13 ++++++-
xen/arch/arm/include/asm/coloring.h | 10 +++++
xen/arch/arm/include/asm/domain.h | 4 ++
xen/include/public/arch-arm.h | 8 ++++
7 files changed, 110 insertions(+), 3 deletions(-)
diff --git a/docs/misc/arm/cache-coloring.rst b/docs/misc/arm/cache-coloring.rst
index b0f9a2e917..e8ee8fafde 100644
--- a/docs/misc/arm/cache-coloring.rst
+++ b/docs/misc/arm/cache-coloring.rst
@@ -16,7 +16,7 @@ In order to enable and use it, few steps are needed.
(refer to menuconfig help for value meaning and when it should be changed).
CONFIG_MAX_CACHE_COLORS=<n>
-- Assign colors to Dom0 using the `Color selection format`_ (see
+- Assign colors to domains using the `Color selection format`_ (see
`Coloring parameters`_ for more documentation pointers).
Background
@@ -114,6 +114,9 @@ LLC way size (as previously discussed) and Dom0 colors can be set using the
appropriate command line parameters. See the relevant documentation in
"docs/misc/xen-command-line.pandoc".
+Note that if no color configuration is provided for domains, they fallback to
+the default one, which corresponds simply to all available colors.
+
Known issues and limitations
****************************
@@ -133,3 +136,12 @@ too) is set to 2^15 = 32768 colors because of some limitation on the domain
configuration structure size used in domain creation. "uint16_t" is the biggest
integer type that fit the constraint and 2^15 is the biggest power of 2 it can
easily represent. This value is big enough for the generic case, though.
+
+
+"xen,static-mem" isn't supported when coloring is enabled
+#########################################################
+
+In the domain configuration, "xen,static-mem" allows memory to be statically
+allocated to the domain. This isn't possibile when cache coloring is enabled,
+because that memory can't be guaranteed to be of the same colors assigned to
+that domain.
diff --git a/xen/arch/arm/coloring.c b/xen/arch/arm/coloring.c
index 36eea2d6c0..a7b59f5aba 100644
--- a/xen/arch/arm/coloring.c
+++ b/xen/arch/arm/coloring.c
@@ -23,6 +23,7 @@
*/
#include <xen/bitops.h>
#include <xen/errno.h>
+#include <xen/guest_access.h>
#include <xen/keyhandler.h>
#include <xen/param.h>
#include <xen/types.h>
@@ -232,6 +233,62 @@ bool __init coloring_init(void)
return true;
}
+int domain_coloring_init(struct domain *d,
+ const struct xen_arch_domainconfig *config)
+{
+ if ( is_domain_direct_mapped(d) )
+ {
+ printk(XENLOG_ERR
+ "Can't enable coloring and directmap at the same time for %pd\n",
+ d);
+ return -EINVAL;
+ }
+
+ if ( is_hardware_domain(d) )
+ {
+ d->arch.colors = dom0_colors;
+ d->arch.num_colors = dom0_num_colors;
+ }
+ else if ( config->num_colors == 0 )
+ {
+ printk(XENLOG_WARNING
+ "Color config not found for %pd. Using default\n", d);
+ d->arch.colors = xzalloc_array(unsigned int, max_colors);
+ d->arch.num_colors = set_default_domain_colors(d->arch.colors);
+ }
+ else
+ {
+ d->arch.colors = xzalloc_array(unsigned int, config->num_colors);
+ d->arch.num_colors = config->num_colors;
+ if ( config->from_guest )
+ copy_from_guest(d->arch.colors, config->colors, config->num_colors);
+ else
+ memcpy(d->arch.colors, config->colors.p,
+ sizeof(unsigned int) * config->num_colors);
+ }
+
+ if ( !d->arch.colors )
+ {
+ printk(XENLOG_ERR "Colors allocation failed for %pd\n", d);
+ return -ENOMEM;
+ }
+
+ if ( !check_colors(d->arch.colors, d->arch.num_colors) )
+ {
+ printk(XENLOG_ERR "Bad color config for %pd\n", d);
+ domain_coloring_free(d);
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+void domain_coloring_free(struct domain *d)
+{
+ if ( !is_hardware_domain(d) )
+ xfree(d->arch.colors);
+}
+
/*
* Local variables:
* mode: C
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 2d6253181a..b4dd64dff4 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -23,6 +23,7 @@
#include <xen/wait.h>
#include <asm/alternative.h>
+#include <asm/coloring.h>
#include <asm/cpuerrata.h>
#include <asm/cpufeature.h>
#include <asm/current.h>
@@ -712,6 +713,10 @@ int arch_domain_create(struct domain *d,
ioreq_domain_init(d);
#endif
+ if ( IS_ENABLED(CONFIG_CACHE_COLORING) &&
+ (rc = domain_coloring_init(d, &config->arch)) )
+ goto fail;
+
/* p2m_init relies on some value initialized by the IOMMU subsystem */
if ( (rc = iommu_domain_init(d, config->iommu_opts)) != 0 )
goto fail;
@@ -807,6 +812,8 @@ void arch_domain_destroy(struct domain *d)
get_order_from_bytes(d->arch.efi_acpi_len));
#endif
domain_io_free(d);
+ if ( IS_ENABLED(CONFIG_CACHE_COLORING) )
+ domain_coloring_free(d);
}
void arch_domain_shutdown(struct domain *d)
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 40e3c2e119..97f2060007 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -35,6 +35,12 @@
#define STATIC_EVTCHN_NODE_SIZE_CELLS 2
+#ifdef CONFIG_CACHE_COLORING
+#define XEN_DOM0_CREATE_FLAGS CDF_privileged
+#else
+#define XEN_DOM0_CREATE_FLAGS CDF_privileged | CDF_directmap
+#endif
+
static unsigned int __initdata opt_dom0_max_vcpus;
integer_param("dom0_max_vcpus", opt_dom0_max_vcpus);
@@ -3963,7 +3969,10 @@ static int __init construct_dom0(struct domain *d)
/* type must be set before allocate_memory */
d->arch.type = kinfo.type;
#endif
- allocate_memory_11(d, &kinfo);
+ if ( IS_ENABLED(CONFIG_CACHE_COLORING) )
+ allocate_memory(d, &kinfo);
+ else
+ allocate_memory_11(d, &kinfo);
find_gnttab_region(d, &kinfo);
#ifdef CONFIG_STATIC_SHM
@@ -4025,7 +4034,7 @@ void __init create_dom0(void)
if ( iommu_enabled )
dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu;
- dom0 = domain_create(0, &dom0_cfg, CDF_privileged | CDF_directmap);
+ dom0 = domain_create(0, &dom0_cfg, XEN_DOM0_CREATE_FLAGS);
if ( IS_ERR(dom0) || (alloc_dom0_vcpu0(dom0) == NULL) )
panic("Error creating domain 0\n");
diff --git a/xen/arch/arm/include/asm/coloring.h b/xen/arch/arm/include/asm/coloring.h
index 3b563d3b90..0d2dfada10 100644
--- a/xen/arch/arm/include/asm/coloring.h
+++ b/xen/arch/arm/include/asm/coloring.h
@@ -27,12 +27,22 @@
#ifdef CONFIG_CACHE_COLORING
#include <xen/init.h>
+#include <xen/sched.h>
+
+#include <public/arch-arm.h>
bool __init coloring_init(void);
+int domain_coloring_init(struct domain *d,
+ const struct xen_arch_domainconfig *config);
+void domain_coloring_free(struct domain *d);
+
#else /* !CONFIG_CACHE_COLORING */
static inline bool __init coloring_init(void) { return true; }
+static inline int domain_coloring_init(
+ struct domain *d, const struct xen_arch_domainconfig *config) { return 0; }
+static inline void domain_coloring_free(struct domain *d) {}
#endif /* CONFIG_CACHE_COLORING */
#endif /* __ASM_ARM_COLORING_H__ */
diff --git a/xen/arch/arm/include/asm/domain.h b/xen/arch/arm/include/asm/domain.h
index 26a8348eed..291f7c375d 100644
--- a/xen/arch/arm/include/asm/domain.h
+++ b/xen/arch/arm/include/asm/domain.h
@@ -58,6 +58,10 @@ struct arch_domain
#ifdef CONFIG_ARM_64
enum domain_type type;
#endif
+#ifdef CONFIG_CACHE_COLORING
+ unsigned int *colors;
+ unsigned int num_colors;
+#endif
/* Virtual MMU */
struct p2m_domain p2m;
diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
index c8b6058d3a..adf843a7a1 100644
--- a/xen/include/public/arch-arm.h
+++ b/xen/include/public/arch-arm.h
@@ -314,6 +314,8 @@ DEFINE_XEN_GUEST_HANDLE(vcpu_guest_context_t);
#define XEN_DOMCTL_CONFIG_TEE_NONE 0
#define XEN_DOMCTL_CONFIG_TEE_OPTEE 1
+__DEFINE_XEN_GUEST_HANDLE(color_t, unsigned int);
+
struct xen_arch_domainconfig {
/* IN/OUT */
uint8_t gic_version;
@@ -335,6 +337,12 @@ struct xen_arch_domainconfig {
*
*/
uint32_t clock_frequency;
+ /* IN */
+ uint8_t from_guest;
+ /* IN */
+ uint16_t num_colors;
+ /* IN */
+ XEN_GUEST_HANDLE(color_t) colors;
};
#endif /* __XEN__ || __XEN_TOOLS__ */
--
2.34.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v3 3/9] xen/arm: dump cache colors in domain info debug-key
2022-10-22 15:51 [PATCH v3 0/9] Arm cache coloring Carlo Nonato
2022-10-22 15:51 ` [PATCH v3 1/9] xen/arm: add cache coloring initialization Carlo Nonato
2022-10-22 15:51 ` [PATCH v3 2/9] xen/arm: add cache coloring initialization for domains Carlo Nonato
@ 2022-10-22 15:51 ` Carlo Nonato
2022-10-22 15:51 ` [PATCH v3 4/9] tools/xl: add support for cache coloring configuration Carlo Nonato
` (6 subsequent siblings)
9 siblings, 0 replies; 26+ messages in thread
From: Carlo Nonato @ 2022-10-22 15:51 UTC (permalink / raw)
To: xen-devel
Cc: marco.solieri, andrea.bastoni, lucmiccio, Stefano Stabellini,
Julien Grall, Bertrand Marquis, Volodymyr Babchuk, Marco Solieri,
Carlo Nonato
From: Luca Miccio <lucmiccio@gmail.com>
This commit adds cache colors to the information dumped with the domain
info debug-key.
Signed-off-by: Luca Miccio <lucmiccio@gmail.com>
Signed-off-by: Marco Solieri <marco.solieri@minervasys.tech>
Signed-off-by: Carlo Nonato <carlo.nonato@minervasys.tech>
---
xen/arch/arm/coloring.c | 16 ++++++++++++++++
xen/arch/arm/domain.c | 2 ++
xen/arch/arm/include/asm/coloring.h | 2 ++
3 files changed, 20 insertions(+)
diff --git a/xen/arch/arm/coloring.c b/xen/arch/arm/coloring.c
index a7b59f5aba..cf8aa8a2ca 100644
--- a/xen/arch/arm/coloring.c
+++ b/xen/arch/arm/coloring.c
@@ -172,6 +172,16 @@ static unsigned int set_default_domain_colors(unsigned int *colors)
return max_colors;
}
+static void print_colors(unsigned int *colors, unsigned int num_colors)
+{
+ unsigned int i;
+
+ printk("[ ");
+ for ( i = 0; i < num_colors; i++ )
+ printk("%u ", colors[i]);
+ printk("]\n");
+}
+
static void dump_coloring_info(unsigned char key)
{
printk("'%c' pressed -> dumping coloring general info\n", key);
@@ -289,6 +299,12 @@ void domain_coloring_free(struct domain *d)
xfree(d->arch.colors);
}
+void domain_dump_coloring_info(struct domain *d)
+{
+ printk("Domain %pd has %u colors: ", d, d->arch.num_colors);
+ print_colors(d->arch.colors, d->arch.num_colors);
+}
+
/*
* Local variables:
* mode: C
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index b4dd64dff4..b174a192d4 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -1083,6 +1083,8 @@ int domain_relinquish_resources(struct domain *d)
void arch_dump_domain_info(struct domain *d)
{
p2m_dump_info(d);
+ if ( IS_ENABLED(CONFIG_CACHE_COLORING) )
+ domain_dump_coloring_info(d);
}
diff --git a/xen/arch/arm/include/asm/coloring.h b/xen/arch/arm/include/asm/coloring.h
index 0d2dfada10..a16736819e 100644
--- a/xen/arch/arm/include/asm/coloring.h
+++ b/xen/arch/arm/include/asm/coloring.h
@@ -36,6 +36,7 @@ bool __init coloring_init(void);
int domain_coloring_init(struct domain *d,
const struct xen_arch_domainconfig *config);
void domain_coloring_free(struct domain *d);
+void domain_dump_coloring_info(struct domain *d);
#else /* !CONFIG_CACHE_COLORING */
@@ -43,6 +44,7 @@ static inline bool __init coloring_init(void) { return true; }
static inline int domain_coloring_init(
struct domain *d, const struct xen_arch_domainconfig *config) { return 0; }
static inline void domain_coloring_free(struct domain *d) {}
+static inline void domain_dump_coloring_info(struct domain *d) {}
#endif /* CONFIG_CACHE_COLORING */
#endif /* __ASM_ARM_COLORING_H__ */
--
2.34.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v3 4/9] tools/xl: add support for cache coloring configuration
2022-10-22 15:51 [PATCH v3 0/9] Arm cache coloring Carlo Nonato
` (2 preceding siblings ...)
2022-10-22 15:51 ` [PATCH v3 3/9] xen/arm: dump cache colors in domain info debug-key Carlo Nonato
@ 2022-10-22 15:51 ` Carlo Nonato
2022-11-24 15:21 ` Anthony PERARD
2022-10-22 15:51 ` [PATCH v3 5/9] xen/arm: add support for cache coloring configuration via device-tree Carlo Nonato
` (5 subsequent siblings)
9 siblings, 1 reply; 26+ messages in thread
From: Carlo Nonato @ 2022-10-22 15:51 UTC (permalink / raw)
To: xen-devel
Cc: marco.solieri, andrea.bastoni, lucmiccio, Carlo Nonato, Wei Liu,
Anthony PERARD, Juergen Gross, Marco Solieri
Add a new "colors" parameter that defines the color assignment for a
domain. The user can specify one or more color ranges using the same
syntax used everywhere else for color config described in the
documentation. The parameter is defined as a list of strings that represent
the color ranges.
Also documentation is added.
Based on original work from: Luca Miccio <lucmiccio@gmail.com>
Signed-off-by: Carlo Nonato <carlo.nonato@minervasys.tech>
Signed-off-by: Marco Solieri <marco.solieri@minervasys.tech>
---
docs/man/xl.cfg.5.pod.in | 10 ++++++
tools/libs/light/libxl_create.c | 12 ++++++++
tools/libs/light/libxl_types.idl | 1 +
tools/xl/xl_parse.c | 52 ++++++++++++++++++++++++++++++--
4 files changed, 73 insertions(+), 2 deletions(-)
diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
index b2901e04cf..5f53cec8bf 100644
--- a/docs/man/xl.cfg.5.pod.in
+++ b/docs/man/xl.cfg.5.pod.in
@@ -2880,6 +2880,16 @@ Currently, only the "sbsa_uart" model is supported for ARM.
=back
+=over 4
+
+=item B<colors=[ "COLORS_RANGE", "COLORS_RANGE", ...]>
+
+Specify the LLC color configuration for the guest. B<COLORS_RANGE> can be either
+a single color value or a hypen-separated closed interval of colors
+(such as "0-4").
+
+=back
+
=head3 x86
=over 4
diff --git a/tools/libs/light/libxl_create.c b/tools/libs/light/libxl_create.c
index b9dd2deedf..94c511912c 100644
--- a/tools/libs/light/libxl_create.c
+++ b/tools/libs/light/libxl_create.c
@@ -615,6 +615,7 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config,
struct xs_permissions rwperm[1];
struct xs_permissions noperm[1];
xs_transaction_t t = 0;
+ DECLARE_HYPERCALL_BUFFER(unsigned int, colors);
/* convenience aliases */
libxl_domain_create_info *info = &d_config->c_info;
@@ -676,6 +677,16 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config,
goto out;
}
+ if (d_config->b_info.num_colors) {
+ size_t bytes = sizeof(unsigned int) * d_config->b_info.num_colors;
+ colors = xc_hypercall_buffer_alloc(ctx->xch, colors, bytes);
+ memcpy(colors, d_config->b_info.colors, bytes);
+ set_xen_guest_handle(create.arch.colors, colors);
+ create.arch.num_colors = d_config->b_info.num_colors;
+ create.arch.from_guest = 1;
+ LOG(DEBUG, "Setup %u domain colors", d_config->b_info.num_colors);
+ }
+
for (;;) {
uint32_t local_domid;
bool recent;
@@ -922,6 +933,7 @@ retry_transaction:
rc = 0;
out:
if (t) xs_transaction_end(ctx->xsh, t, 1);
+ if (colors) xc_hypercall_buffer_free(ctx->xch, colors);
return rc;
}
diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
index d634f304cd..642173af1a 100644
--- a/tools/libs/light/libxl_types.idl
+++ b/tools/libs/light/libxl_types.idl
@@ -557,6 +557,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
("ioports", Array(libxl_ioport_range, "num_ioports")),
("irqs", Array(uint32, "num_irqs")),
("iomem", Array(libxl_iomem_range, "num_iomem")),
+ ("colors", Array(uint32, "num_colors")),
("claim_mode", libxl_defbool),
("event_channels", uint32),
("kernel", string),
diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
index 1b5381cef0..e6b2c7acff 100644
--- a/tools/xl/xl_parse.c
+++ b/tools/xl/xl_parse.c
@@ -1220,8 +1220,9 @@ void parse_config_data(const char *config_source,
XLU_ConfigList *cpus, *vbds, *nics, *pcis, *cvfbs, *cpuids, *vtpms,
*usbctrls, *usbdevs, *p9devs, *vdispls, *pvcallsifs_devs;
XLU_ConfigList *channels, *ioports, *irqs, *iomem, *viridian, *dtdevs,
- *mca_caps;
- int num_ioports, num_irqs, num_iomem, num_cpus, num_viridian, num_mca_caps;
+ *mca_caps, *colors;
+ int num_ioports, num_irqs, num_iomem, num_cpus, num_viridian, num_mca_caps,
+ num_colors;
int pci_power_mgmt = 0;
int pci_msitranslate = 0;
int pci_permissive = 0;
@@ -1370,6 +1371,53 @@ void parse_config_data(const char *config_source,
if (!xlu_cfg_get_long (config, "maxmem", &l, 0))
b_info->max_memkb = l * 1024;
+ if (!xlu_cfg_get_list(config, "colors", &colors, &num_colors, 0)) {
+ int k, p, cur_index = 0;
+
+ b_info->num_colors = 0;
+ /* Get number of colors based on ranges */
+ for (i = 0; i < num_colors; i++) {
+ uint32_t start = 0, end = 0;
+
+ buf = xlu_cfg_get_listitem(colors, i);
+ if (!buf) {
+ fprintf(stderr,
+ "xl: Unable to get element %d in colors range list\n", i);
+ exit(1);
+ }
+
+ if (sscanf(buf, "%u-%u", &start, &end) != 2) {
+ if (sscanf(buf, "%u", &start) != 1) {
+ fprintf(stderr, "xl: Invalid color range: %s\n", buf);
+ exit(1);
+ }
+ end = start;
+ }
+ else if (start > end) {
+ fprintf(stderr,
+ "xl: Start color is greater than end color: %s\n", buf);
+ exit(1);
+ }
+
+ /* Check for overlaps */
+ for (k = start; k <= end; k++) {
+ for (p = 0; p < b_info->num_colors; p++) {
+ if (b_info->colors[p] == k) {
+ fprintf(stderr, "xl: Overlapped ranges not allowed\n");
+ exit(1);
+ }
+ }
+ }
+
+ b_info->num_colors += (end - start) + 1;
+ b_info->colors = (uint32_t *)realloc(b_info->colors,
+ sizeof(*b_info->colors) * b_info->num_colors);
+
+ for (k = start; k <= end; k++)
+ b_info->colors[cur_index++] = k;
+ }
+ }
+
if (!xlu_cfg_get_long (config, "vcpus", &l, 0)) {
vcpus = l;
if (libxl_cpu_bitmap_alloc(ctx, &b_info->avail_vcpus, l)) {
--
2.34.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v3 5/9] xen/arm: add support for cache coloring configuration via device-tree
2022-10-22 15:51 [PATCH v3 0/9] Arm cache coloring Carlo Nonato
` (3 preceding siblings ...)
2022-10-22 15:51 ` [PATCH v3 4/9] tools/xl: add support for cache coloring configuration Carlo Nonato
@ 2022-10-22 15:51 ` Carlo Nonato
2022-10-22 15:51 ` [PATCH v3 6/9] xen/common: add cache coloring allocator for domains Carlo Nonato
` (4 subsequent siblings)
9 siblings, 0 replies; 26+ messages in thread
From: Carlo Nonato @ 2022-10-22 15:51 UTC (permalink / raw)
To: xen-devel
Cc: marco.solieri, andrea.bastoni, lucmiccio, Carlo Nonato,
Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk, Marco Solieri
This commit adds the "colors" Device Tree attribute that can be used for
DomUs and Dom0less color configurations. The syntax is the same used
for every color config.
Based on original work from: Luca Miccio <lucmiccio@gmail.com>
Signed-off-by: Carlo Nonato <carlo.nonato@minervasys.tech>
Signed-off-by: Marco Solieri <marco.solieri@minervasys.tech>
---
docs/misc/arm/cache-coloring.rst | 45 +++++++++++++++++++++++++++
docs/misc/arm/device-tree/booting.txt | 4 +++
xen/arch/arm/coloring.c | 17 ++++++++++
xen/arch/arm/domain_build.c | 13 ++++++++
xen/arch/arm/include/asm/coloring.h | 5 +++
5 files changed, 84 insertions(+)
diff --git a/docs/misc/arm/cache-coloring.rst b/docs/misc/arm/cache-coloring.rst
index e8ee8fafde..dd2e851a26 100644
--- a/docs/misc/arm/cache-coloring.rst
+++ b/docs/misc/arm/cache-coloring.rst
@@ -114,6 +114,51 @@ LLC way size (as previously discussed) and Dom0 colors can be set using the
appropriate command line parameters. See the relevant documentation in
"docs/misc/xen-command-line.pandoc".
+DomUs colors can be set either in the xl configuration file (relative
+documentation at "docs/man/xl.cfg.pod.5.in") or via Device Tree, also for
+Dom0less configurations, as in the following example:
+
+.. raw:: html
+
+ <pre>
+ xen,xen-bootargs = "console=dtuart dtuart=serial0 dom0_mem=1G dom0_max_vcpus=1 sched=null llc-way-size=64K xen-colors=0-1 dom0-colors=2-6";
+ xen,dom0-bootargs "console=hvc0 earlycon=xen earlyprintk=xen root=/dev/ram0"
+
+ dom0 {
+ compatible = "xen,linux-zimage" "xen,multiboot-module";
+ reg = <0x0 0x1000000 0x0 15858176>;
+ };
+
+ dom0-ramdisk {
+ compatible = "xen,linux-initrd" "xen,multiboot-module";
+ reg = <0x0 0x2000000 0x0 20638062>;
+ };
+
+ domU0 {
+ #address-cells = <0x1>;
+ #size-cells = <0x1>;
+ compatible = "xen,domain";
+ memory = <0x0 0x40000>;
+ colors = "4-8,10,11,12";
+ cpus = <0x1>;
+ vpl011 = <0x1>;
+
+ module@2000000 {
+ compatible = "multiboot,kernel", "multiboot,module";
+ reg = <0x2000000 0xffffff>;
+ bootargs = "console=ttyAMA0";
+ };
+
+ module@30000000 {
+ compatible = "multiboot,ramdisk", "multiboot,module";
+ reg = <0x3000000 0xffffff>;
+ };
+ };
+ </pre>
+
+Please refer to the relative documentation in
+"docs/misc/arm/device-tree/booting.txt".
+
Note that if no color configuration is provided for domains, they fallback to
the default one, which corresponds simply to all available colors.
diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
index c47a05e0da..3aa493c66d 100644
--- a/docs/misc/arm/device-tree/booting.txt
+++ b/docs/misc/arm/device-tree/booting.txt
@@ -162,6 +162,10 @@ with the following properties:
An integer specifying the number of vcpus to allocate to the guest.
+- colors
+ A string specifying the color configuration for the guest. Refer to
+ "docs/misc/arm/cache_coloring.rst" for syntax.
+
- vpl011
An empty property to enable/disable a virtual pl011 for the guest to
diff --git a/xen/arch/arm/coloring.c b/xen/arch/arm/coloring.c
index cf8aa8a2ca..685a431c3d 100644
--- a/xen/arch/arm/coloring.c
+++ b/xen/arch/arm/coloring.c
@@ -273,8 +273,11 @@ int domain_coloring_init(struct domain *d,
if ( config->from_guest )
copy_from_guest(d->arch.colors, config->colors, config->num_colors);
else
+ {
memcpy(d->arch.colors, config->colors.p,
sizeof(unsigned int) * config->num_colors);
+ xfree(config->colors.p);
+ }
}
if ( !d->arch.colors )
@@ -305,6 +308,20 @@ void domain_dump_coloring_info(struct domain *d)
print_colors(d->arch.colors, d->arch.num_colors);
}
+void prepare_color_domain_config(struct xen_arch_domainconfig *config,
+ const char *colors_str)
+{
+ unsigned int num_colors;
+
+ config->colors.p = xzalloc_array(unsigned int, max_colors);
+ if ( !config->colors.p )
+ panic("Unable to allocate cache colors\n");
+
+ if ( parse_color_config(colors_str, config->colors.p, &num_colors) )
+ panic("Error parsing the color configuration\n");
+ config->num_colors = (uint16_t)num_colors;
+}
+
/*
* Local variables:
* mode: C
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 97f2060007..b95e655331 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -25,6 +25,7 @@
#include <asm/platform.h>
#include <asm/psci.h>
#include <asm/setup.h>
+#include <asm/coloring.h>
#include <asm/cpufeature.h>
#include <asm/domain_build.h>
#include <xen/event.h>
@@ -3826,6 +3827,7 @@ void __init create_domUs(void)
struct dt_device_node *node;
const struct dt_device_node *cpupool_node,
*chosen = dt_find_node_by_path("/chosen");
+ const char *colors_str;
BUG_ON(chosen == NULL);
dt_for_each_child_node(chosen, node)
@@ -3914,6 +3916,17 @@ void __init create_domUs(void)
d_cfg.cpupool_id = pool_id;
}
+ if ( !dt_property_read_string(node, "colors", &colors_str) )
+ {
+ if ( !IS_ENABLED(CONFIG_CACHE_COLORING) )
+ printk(XENLOG_WARNING
+ "Property 'colors' found, but coloring is disabled\n");
+ else if ( dt_find_property(node, "xen,static-mem", NULL) )
+ panic("static-mem isn't allowed when coloring is enabled\n");
+ else
+ prepare_color_domain_config(&d_cfg.arch, colors_str);
+ }
+
/*
* The variable max_init_domid is initialized with zero, so here it's
* very important to use the pre-increment operator to call
diff --git a/xen/arch/arm/include/asm/coloring.h b/xen/arch/arm/include/asm/coloring.h
index a16736819e..549eb408a3 100644
--- a/xen/arch/arm/include/asm/coloring.h
+++ b/xen/arch/arm/include/asm/coloring.h
@@ -38,6 +38,9 @@ int domain_coloring_init(struct domain *d,
void domain_coloring_free(struct domain *d);
void domain_dump_coloring_info(struct domain *d);
+void prepare_color_domain_config(struct xen_arch_domainconfig *config,
+ const char *colors_str);
+
#else /* !CONFIG_CACHE_COLORING */
static inline bool __init coloring_init(void) { return true; }
@@ -45,6 +48,8 @@ static inline int domain_coloring_init(
struct domain *d, const struct xen_arch_domainconfig *config) { return 0; }
static inline void domain_coloring_free(struct domain *d) {}
static inline void domain_dump_coloring_info(struct domain *d) {}
+static inline void prepare_color_domain_config(
+ struct xen_arch_domainconfig *config, const char *colors_str) {}
#endif /* CONFIG_CACHE_COLORING */
#endif /* __ASM_ARM_COLORING_H__ */
--
2.34.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v3 6/9] xen/common: add cache coloring allocator for domains
2022-10-22 15:51 [PATCH v3 0/9] Arm cache coloring Carlo Nonato
` (4 preceding siblings ...)
2022-10-22 15:51 ` [PATCH v3 5/9] xen/arm: add support for cache coloring configuration via device-tree Carlo Nonato
@ 2022-10-22 15:51 ` Carlo Nonato
2022-10-25 12:08 ` Carlo Nonato
2022-11-10 16:47 ` Jan Beulich
2022-10-22 15:51 ` [PATCH v3 7/9] Revert "xen/arm: Remove unused BOOT_RELOC_VIRT_START" Carlo Nonato
` (3 subsequent siblings)
9 siblings, 2 replies; 26+ messages in thread
From: Carlo Nonato @ 2022-10-22 15:51 UTC (permalink / raw)
To: xen-devel
Cc: marco.solieri, andrea.bastoni, lucmiccio, Stefano Stabellini,
Julien Grall, Bertrand Marquis, Volodymyr Babchuk, Andrew Cooper,
George Dunlap, Jan Beulich, Wei Liu, Marco Solieri, Carlo Nonato
From: Luca Miccio <lucmiccio@gmail.com>
This commit adds a new memory page allocator that implements the cache
coloring mechanism. The allocation algorithm follows the given domain color
configuration and maximizes contiguity in the page selection of multiple
subsequent requests.
Pages are stored in a color-indexed array of lists, each one sorted by
machine address, that is called the colored heap. A simple initialization
function computes the color of any available page and inserts it in the
corresponding list. When a domain requests a page, the allocator takes one
from the subset of lists whose colors equals the domain configuration. It
chooses the page with the highest machine address such that contiguous
pages are sequentially allocated, if this is made possible by a color
assignment which includes adjacent colors.
The allocator can handle only requests with order equals to 0 since the
single color granularity is represented in memory by one page.
The buddy allocator must coexist with the colored one because the Xen heap
isn't colored. For this reason a new Kconfig option and a command line
parameter are added to let the user set the amount of memory reserved for
the buddy allocator. Even when cache coloring is enabled, this memory isn't
managed by the colored allocator.
Colored heap information is dumped in the dump_heap() debug-key function.
Signed-off-by: Luca Miccio <lucmiccio@gmail.com>
Signed-off-by: Marco Solieri <marco.solieri@minervasys.tech>
Signed-off-by: Carlo Nonato <carlo.nonato@minervasys.tech>
---
v3:
- fixed PGC_colored bits values
- merged debug-key for dump_color_heap() with the one for dump_heap()
- number of pages for each color in an array to easily dump color heap info
- heap_lock in colored allocator to ensure atomicity and clarify it with a
comment
- added page_list_add_{next|prev} to add pages in the middle of the list
- p2m tables use pages of same colors as domain
- CONFIG_BUDDY_ALLOCATOR_SIZE is now an int (MiB)
- buddy allocator reserved size is now respected as configured in Kconfig
- removed useless functions and refactored the code
- fixed PGC_colored flag that was removed when a page was allocated
- merged with #7 since it would have been too small
---
docs/misc/arm/cache-coloring.rst | 39 ++++-
docs/misc/xen-command-line.pandoc | 14 ++
xen/arch/arm/Kconfig | 12 ++
xen/arch/arm/coloring.c | 10 ++
xen/arch/arm/include/asm/coloring.h | 6 +
xen/arch/arm/include/asm/mm.h | 3 +
xen/arch/arm/p2m.c | 7 +-
xen/common/page_alloc.c | 259 +++++++++++++++++++++++++---
xen/include/xen/mm.h | 43 +++++
9 files changed, 371 insertions(+), 22 deletions(-)
diff --git a/docs/misc/arm/cache-coloring.rst b/docs/misc/arm/cache-coloring.rst
index dd2e851a26..0c89278aee 100644
--- a/docs/misc/arm/cache-coloring.rst
+++ b/docs/misc/arm/cache-coloring.rst
@@ -16,6 +16,9 @@ In order to enable and use it, few steps are needed.
(refer to menuconfig help for value meaning and when it should be changed).
CONFIG_MAX_CACHE_COLORS=<n>
+- If needed, change the amount of memory reserved for the buddy allocator either
+ from the Xen configuration file, via the CONFIG_BUDDY_ALLOCATOR_SIZE value,
+ or with the command line option. See `Colored allocator and buddy allocator`.
- Assign colors to domains using the `Color selection format`_ (see
`Coloring parameters`_ for more documentation pointers).
@@ -162,6 +165,18 @@ Please refer to the relative documentation in
Note that if no color configuration is provided for domains, they fallback to
the default one, which corresponds simply to all available colors.
+Colored allocator and buddy allocator
+*************************************
+
+The colored allocator distributes pages based on color configurations of
+domains so that each domains only gets pages of its own colors.
+The colored allocator is meant as an alternative to the buddy allocator because
+its allocation policy is by definition incompatible with the generic one. Since
+the Xen heap is not colored yet, we need to support the coexistence of the two
+allocators and some memory must be left for the buddy one.
+The buddy allocator memory can be reserved from the Xen configuration file or
+with the help of a command-line option.
+
Known issues and limitations
****************************
@@ -182,7 +197,6 @@ configuration structure size used in domain creation. "uint16_t" is the biggest
integer type that fit the constraint and 2^15 is the biggest power of 2 it can
easily represent. This value is big enough for the generic case, though.
-
"xen,static-mem" isn't supported when coloring is enabled
#########################################################
@@ -190,3 +204,26 @@ In the domain configuration, "xen,static-mem" allows memory to be statically
allocated to the domain. This isn't possibile when cache coloring is enabled,
because that memory can't be guaranteed to be of the same colors assigned to
that domain.
+
+Colored allocator can only make use of order-0 pages
+####################################################
+
+The cache coloring technique relies on memory mappings and on the smallest
+amount of memory that can be mapped to achieve the maximum number of colors
+(cache partitions) possible. This amount is what is normally called a page and,
+in Xen terminology, the order-0 page is the smallest one. The fairly simple
+colored allocator currently implemented, makes use only of such pages.
+It must be said that a more complex one could, in theory, adopt higher order
+pages if the colors selection contained adjacent colors. Two subsequent colors,
+for example, can be represented by an order-1 page, four colors correspond to
+an order-2 page, etc.
+
+Fail to boot colored DomUs with large memory size
+#################################################
+
+If the Linux kernel used for Dom0 does not contain the upstream commit
+3941552aec1e04d63999988a057ae09a1c56ebeb and uses the hypercall buffer device,
+colored DomUs with memory size larger then 127 MB cannot be created. This is
+caused by the default limit of this buffer of 64 pages. The solution is to
+manually apply the above patch, or to check if there is an updated version of
+the kernel in use for Dom0 that contains this change.
diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index 3f04414134..25a59dd6a9 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -299,6 +299,20 @@ can be maintained with the pv-shim mechanism.
cause Xen not to use Indirect Branch Tracking even when support is
available in hardware.
+### buddy-alloc-size (arm64)
+> `= <size>`
+
+> Default: `64M`
+
+Amount of memory reserved for the buddy allocator when colored allocator is
+active. This options is parsed only when cache coloring support is enabled.
+The colored allocator is meant as an alternative to the buddy allocator,
+because its allocation policy is by definition incompatible with the
+generic one. Since the Xen heap systems is not colored yet, we need to
+support the coexistence of the two allocators for now. This parameter, which is
+optional and for expert only, it's used to set the amount of memory reserved to
+the buddy allocator.
+
### clocksource (x86)
> `= pit | hpet | acpi | tsc`
diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
index c45a9c5917..4cfa75b2ef 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -153,6 +153,18 @@ config MAX_CACHE_COLORS
Note that if, at any time, a color configuration with more colors than the
maximum is employed, an error is produced.
+config BUDDY_ALLOCATOR_SIZE
+ int "Buddy allocator reserved memory size (MiB)"
+ default "64"
+ depends on CACHE_COLORING
+ help
+ Amount of memory reserved for the buddy allocator to work alongside
+ the colored one. The colored allocator is meant as an alternative to the
+ buddy allocator because its allocation policy is by definition
+ incompatible with the generic one. Since the Xen heap is not colored yet,
+ we need to support the coexistence of the two allocators and some memory
+ must be left for the buddy one.
+
config TEE
bool "Enable TEE mediators support (UNSUPPORTED)" if UNSUPPORTED
default n
diff --git a/xen/arch/arm/coloring.c b/xen/arch/arm/coloring.c
index 685a431c3d..2cae215cd2 100644
--- a/xen/arch/arm/coloring.c
+++ b/xen/arch/arm/coloring.c
@@ -322,6 +322,16 @@ void prepare_color_domain_config(struct xen_arch_domainconfig *config,
config->num_colors = (uint16_t)num_colors;
}
+unsigned int page_to_color(const struct page_info *pg)
+{
+ return addr_to_color(page_to_maddr(pg));
+}
+
+unsigned int get_max_colors(void)
+{
+ return max_colors;
+}
+
/*
* Local variables:
* mode: C
diff --git a/xen/arch/arm/include/asm/coloring.h b/xen/arch/arm/include/asm/coloring.h
index 549eb408a3..0147f95968 100644
--- a/xen/arch/arm/include/asm/coloring.h
+++ b/xen/arch/arm/include/asm/coloring.h
@@ -31,6 +31,8 @@
#include <public/arch-arm.h>
+struct page_info;
+
bool __init coloring_init(void);
int domain_coloring_init(struct domain *d,
@@ -41,6 +43,10 @@ void domain_dump_coloring_info(struct domain *d);
void prepare_color_domain_config(struct xen_arch_domainconfig *config,
const char *colors_str);
+unsigned int page_to_color(const struct page_info *pg);
+
+unsigned int get_max_colors(void);
+
#else /* !CONFIG_CACHE_COLORING */
static inline bool __init coloring_init(void) { return true; }
diff --git a/xen/arch/arm/include/asm/mm.h b/xen/arch/arm/include/asm/mm.h
index 68adcac9fa..e848fa4adf 100644
--- a/xen/arch/arm/include/asm/mm.h
+++ b/xen/arch/arm/include/asm/mm.h
@@ -128,6 +128,9 @@ struct page_info
#else
#define PGC_static 0
#endif
+/* Page is cache colored */
+#define _PGC_colored PG_shift(4)
+#define PGC_colored PG_mask(1, 4)
/* ... */
/* Page is broken? */
#define _PGC_broken PG_shift(7)
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 8449f97fe7..9ac7dc6216 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -661,7 +661,12 @@ static int p2m_create_table(struct p2m_domain *p2m, lpae_t *entry)
ASSERT(!p2m_is_valid(*entry));
- page = alloc_domheap_page(NULL, 0);
+ /* If cache coloring is enabled, p2m tables are allocated using the domain
+ * coloring configuration to prevent cache interference. */
+ if ( IS_ENABLED(CONFIG_CACHE_COLORING) )
+ page = alloc_domheap_page(p2m->domain, MEMF_no_refcount);
+ else
+ page = alloc_domheap_page(NULL, 0);
if ( page == NULL )
return -ENOMEM;
diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 62afb07bc6..fe214cd6ac 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -150,6 +150,9 @@
#define p2m_pod_offline_or_broken_hit(pg) 0
#define p2m_pod_offline_or_broken_replace(pg) BUG_ON(pg != NULL)
#endif
+#ifdef CONFIG_HAS_CACHE_COLORING
+#include <asm/coloring.h>
+#endif
#ifndef PGC_static
#define PGC_static 0
@@ -231,6 +234,14 @@ static bool __read_mostly scrub_debug;
#define scrub_debug false
#endif
+/* Memory required for buddy allocator to work with colored one */
+#ifdef CONFIG_BUDDY_ALLOCATOR_SIZE
+static unsigned long __initdata buddy_alloc_size =
+ CONFIG_BUDDY_ALLOCATOR_SIZE << 20;
+#else
+ static unsigned long __initdata buddy_alloc_size = 0;
+#endif
+
/*
* Bit width of the DMA heap -- used to override NUMA-node-first.
* allocation strategy, which can otherwise exhaust low memory.
@@ -440,7 +451,180 @@ mfn_t __init alloc_boot_pages(unsigned long nr_pfns, unsigned long pfn_align)
BUG();
}
+static DEFINE_SPINLOCK(heap_lock);
+/* Initialise fields which have other uses for free pages. */
+static void init_free_page_fields(struct page_info *pg)
+{
+ pg->u.inuse.type_info = PGT_TYPE_INFO_INITIALIZER;
+ page_set_owner(pg, NULL);
+}
+
+#ifdef CONFIG_CACHE_COLORING
+/*************************
+ * COLORED SIDE-ALLOCATOR
+ *
+ * Pages are stored by their color in separate lists. Each list defines a color
+ * and it is initialized during end_boot_allocator, where each page's color
+ * is calculated and the page itself is put in the correct list.
+ * After initialization there will be N lists where N is the number of
+ * available colors on the platform.
+ * The {free|alloc}_color_heap_page overwrite pg->count_info, but they do it in
+ * the same way as the buddy allocator corresponding functions do:
+ * protecting the access with a critical section using heap_lock.
+ */
+typedef struct page_list_head colored_pages_t;
+static colored_pages_t *__ro_after_init _color_heap;
+static unsigned long *__ro_after_init free_colored_pages;
+
+#define color_heap(color) (&_color_heap[color])
+
+static void free_color_heap_page(struct page_info *pg)
+{
+ struct page_info *pos;
+ unsigned int color = page_to_color(pg);
+ colored_pages_t *head = color_heap(color);
+
+ spin_lock(&heap_lock);
+
+ pg->count_info = PGC_state_free | PGC_colored;
+ page_set_owner(pg, NULL);
+ free_colored_pages[color]++;
+
+ page_list_for_each( pos, head )
+ {
+ if ( page_to_maddr(pos) < page_to_maddr(pg) )
+ break;
+ }
+
+ page_list_add_next(pg, pos, head);
+
+ spin_unlock(&heap_lock);
+}
+
+static struct page_info *alloc_color_heap_page(unsigned int memflags,
+ const unsigned int *colors,
+ unsigned int num_colors)
+{
+ struct page_info *pg = NULL;
+ unsigned int i, color;
+ bool need_tlbflush = false;
+ uint32_t tlbflush_timestamp = 0;
+
+ spin_lock(&heap_lock);
+
+ for ( i = 0; i < num_colors; i++ )
+ {
+ struct page_info *tmp;
+
+ if ( page_list_empty(color_heap(colors[i])) )
+ continue;
+
+ tmp = page_list_first(color_heap(colors[i]));
+ if ( !pg || page_to_maddr(tmp) > page_to_maddr(pg) )
+ pg = tmp;
+ }
+
+ if ( !pg )
+ {
+ spin_unlock(&heap_lock);
+ return NULL;
+ }
+
+ pg->count_info = PGC_state_inuse | PGC_colored;
+
+ if ( !(memflags & MEMF_no_tlbflush) )
+ accumulate_tlbflush(&need_tlbflush, pg, &tlbflush_timestamp);
+
+ init_free_page_fields(pg);
+ flush_page_to_ram(mfn_x(page_to_mfn(pg)),
+ !(memflags & MEMF_no_icache_flush));
+
+ color = page_to_color(pg);
+ free_colored_pages[color]--;
+ page_list_del(pg, color_heap(color));
+
+ spin_unlock(&heap_lock);
+
+ if ( need_tlbflush )
+ filtered_flush_tlb_mask(tlbflush_timestamp);
+
+ return pg;
+}
+
+static void __init init_color_heap_pages(struct page_info *pg,
+ unsigned long nr_pages)
+{
+ unsigned int i;
+
+ if ( !_color_heap )
+ {
+ unsigned int max_colors = get_max_colors();
+
+ _color_heap = xmalloc_array(colored_pages_t, max_colors);
+ BUG_ON(!_color_heap);
+ free_colored_pages = xzalloc_array(unsigned long, max_colors);
+ BUG_ON(!free_colored_pages);
+
+ for ( i = 0; i < max_colors; i++ )
+ INIT_PAGE_LIST_HEAD(color_heap(i));
+ }
+
+ printk(XENLOG_DEBUG
+ "Init color heap with %lu pages starting from: %#"PRIx64"\n",
+ nr_pages, page_to_maddr(pg));
+
+ for ( i = 0; i < nr_pages; i++ )
+ free_color_heap_page(&pg[i]);
+}
+
+static struct page_info *alloc_color_domheap_page(struct domain *d,
+ unsigned int memflags)
+{
+ struct page_info *pg;
+
+ pg = alloc_color_heap_page(memflags, d->arch.colors, d->arch.num_colors);
+ if ( !pg )
+ return NULL;
+
+ if ( !(memflags & MEMF_no_owner) )
+ {
+ if ( memflags & MEMF_no_refcount )
+ pg->count_info |= PGC_extra;
+ if ( assign_page(pg, 0, d, memflags) )
+ {
+ free_color_heap_page(pg);
+ return NULL;
+ }
+ }
+
+ return pg;
+}
+
+static void dump_color_heap(void)
+{
+ unsigned int color;
+
+ printk("Dumping coloring heap info\n");
+ for ( color = 0; color < get_max_colors(); color++ )
+ printk("Color heap[%u]: %lu pages\n", color, free_colored_pages[color]);
+}
+
+integer_param("buddy-alloc-size", buddy_alloc_size);
+
+#else /* !CONFIG_CACHE_COLORING */
+
+static void __init init_color_heap_pages(struct page_info *pg,
+ unsigned long nr_pages) {}
+static struct page_info *alloc_color_domheap_page(struct domain *d,
+ unsigned int memflags)
+{
+ return NULL;
+}
+static void free_color_heap_page(struct page_info *pg) {}
+static void dump_color_heap(void) {}
+
+#endif /* CONFIG_CACHE_COLORING */
/*************************
* BINARY BUDDY ALLOCATOR
@@ -462,7 +646,6 @@ static unsigned long node_need_scrub[MAX_NUMNODES];
static unsigned long *avail[MAX_NUMNODES];
static long total_avail_pages;
-static DEFINE_SPINLOCK(heap_lock);
static long outstanding_claims; /* total outstanding claims by all domains */
unsigned long domain_adjust_tot_pages(struct domain *d, long pages)
@@ -1027,10 +1210,7 @@ static struct page_info *alloc_heap_pages(
accumulate_tlbflush(&need_tlbflush, &pg[i],
&tlbflush_timestamp);
- /* Initialise fields which have other uses for free pages. */
- pg[i].u.inuse.type_info = PGT_TYPE_INFO_INITIALIZER;
- page_set_owner(&pg[i], NULL);
-
+ init_free_page_fields(&pg[i]);
}
spin_unlock(&heap_lock);
@@ -1926,24 +2106,49 @@ static unsigned long avail_heap_pages(
void __init end_boot_allocator(void)
{
unsigned int i;
+ unsigned long buddy_pages;
- /* Pages that are free now go to the domain sub-allocator. */
- for ( i = 0; i < nr_bootmem_regions; i++ )
+ buddy_pages = PFN_DOWN(buddy_alloc_size);
+
+ if ( !IS_ENABLED(CONFIG_CACHE_COLORING) )
{
- struct bootmem_region *r = &bootmem_region_list[i];
- if ( (r->s < r->e) &&
- (phys_to_nid(pfn_to_paddr(r->s)) == cpu_to_node(0)) )
+ /* Pages that are free now go to the domain sub-allocator. */
+ for ( i = 0; i < nr_bootmem_regions; i++ )
{
- init_heap_pages(mfn_to_page(_mfn(r->s)), r->e - r->s);
- r->e = r->s;
- break;
+ struct bootmem_region *r = &bootmem_region_list[i];
+ if ( (r->s < r->e) &&
+ (phys_to_nid(pfn_to_paddr(r->s)) == cpu_to_node(0)) )
+ {
+ init_heap_pages(mfn_to_page(_mfn(r->s)), r->e - r->s);
+ r->e = r->s;
+ break;
+ }
}
}
+
for ( i = nr_bootmem_regions; i-- > 0; )
{
- struct bootmem_region *r = &bootmem_region_list[i];
+ struct bootmem_region *r;
+
+ if ( IS_ENABLED(CONFIG_CACHE_COLORING) )
+ r = &bootmem_region_list[nr_bootmem_regions - i - 1];
+ else
+ r = &bootmem_region_list[i];
+
+ if ( buddy_pages && (r->s < r->e) )
+ {
+ unsigned long pages = MIN(r->e - r->s, buddy_pages);
+ init_heap_pages(mfn_to_page(_mfn(r->s)), pages);
+ r->s += pages;
+ buddy_pages -= pages;
+ }
if ( r->s < r->e )
- init_heap_pages(mfn_to_page(_mfn(r->s)), r->e - r->s);
+ {
+ if ( IS_ENABLED(CONFIG_CACHE_COLORING) )
+ init_color_heap_pages(mfn_to_page(_mfn(r->s)), r->e - r->s);
+ else
+ init_heap_pages(mfn_to_page(_mfn(r->s)), r->e - r->s);
+ }
}
nr_bootmem_regions = 0;
@@ -2344,7 +2549,8 @@ int assign_pages(
for ( i = 0; i < nr; i++ )
{
- ASSERT(!(pg[i].count_info & ~(PGC_extra | PGC_static)));
+ ASSERT(!(pg[i].count_info & ~(PGC_extra | PGC_static |
+ PGC_colored)));
if ( pg[i].count_info & PGC_extra )
extra_pages++;
}
@@ -2429,6 +2635,15 @@ struct page_info *alloc_domheap_pages(
ASSERT_ALLOC_CONTEXT();
+ /* Only domains are supported for coloring */
+ if ( IS_ENABLED(CONFIG_CACHE_COLORING) && d )
+ {
+ /* Colored allocation must be done on 0 order */
+ if ( order )
+ return NULL;
+ return alloc_color_domheap_page(d, memflags);
+ }
+
bits = domain_clamp_alloc_bitsize(memflags & MEMF_no_owner ? NULL : d,
bits ? : (BITS_PER_LONG+PAGE_SHIFT));
@@ -2546,7 +2761,10 @@ void free_domheap_pages(struct page_info *pg, unsigned int order)
scrub = 1;
}
- free_heap_pages(pg, order, scrub);
+ if ( pg->count_info & PGC_colored )
+ free_color_heap_page(pg);
+ else
+ free_heap_pages(pg, order, scrub);
}
if ( drop_dom_ref )
@@ -2653,6 +2871,9 @@ static void cf_check dump_heap(unsigned char key)
continue;
printk("Node %d has %lu unscrubbed pages\n", i, node_need_scrub[i]);
}
+
+ if ( IS_ENABLED(CONFIG_CACHE_COLORING) )
+ dump_color_heap();
}
static __init int cf_check register_heap_trigger(void)
@@ -2785,9 +3006,7 @@ static bool prepare_staticmem_pages(struct page_info *pg, unsigned long nr_mfns,
* to PGC_state_inuse.
*/
pg[i].count_info = PGC_static | PGC_state_inuse;
- /* Initialise fields which have other uses for free pages. */
- pg[i].u.inuse.type_info = PGT_TYPE_INFO_INITIALIZER;
- page_set_owner(&pg[i], NULL);
+ init_free_page_fields(&pg[i]);
}
spin_unlock(&heap_lock);
diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
index a925028ab3..0d48502e75 100644
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -297,6 +297,37 @@ page_list_add_tail(struct page_info *page, struct page_list_head *head)
}
head->tail = page;
}
+static inline void
+_page_list_add(struct page_info *new, struct page_info *prev,
+ struct page_info *next)
+{
+ new->list.prev = page_to_pdx(prev);
+ new->list.next = page_to_pdx(next);
+ prev->list.next = page_to_pdx(new);
+ next->list.prev = page_to_pdx(new);
+}
+static inline void
+page_list_add_next(struct page_info *new, struct page_info *prev,
+ struct page_list_head *head)
+{
+ struct page_info *next = page_list_next(prev, head);
+
+ if ( !next )
+ page_list_add_tail(new, head);
+ else
+ _page_list_add(new, prev, next);
+}
+static inline void
+page_list_add_prev(struct page_info *new, struct page_info *next,
+ struct page_list_head *head)
+{
+ struct page_info *prev = page_list_prev(next, head);
+
+ if ( !prev )
+ page_list_add(new, head);
+ else
+ _page_list_add(new, prev, next);
+}
static inline bool_t
__page_list_del_head(struct page_info *page, struct page_list_head *head,
struct page_info *next, struct page_info *prev)
@@ -449,6 +480,18 @@ page_list_add_tail(struct page_info *page, struct page_list_head *head)
list_add_tail(&page->list, head);
}
static inline void
+page_list_add_next(struct page_info *new, struct page_info *prev,
+ struct page_list_head *head)
+{
+ page_list_add_tail(new, &prev->list);
+}
+static inline void
+page_list_add_prev(struct page_info *new, struct page_info *next,
+ struct page_list_head *head)
+{
+ page_list_add(new, &next->list);
+}
+static inline void
page_list_del(struct page_info *page, struct page_list_head *head)
{
list_del(&page->list);
--
2.34.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v3 7/9] Revert "xen/arm: Remove unused BOOT_RELOC_VIRT_START"
2022-10-22 15:51 [PATCH v3 0/9] Arm cache coloring Carlo Nonato
` (5 preceding siblings ...)
2022-10-22 15:51 ` [PATCH v3 6/9] xen/common: add cache coloring allocator for domains Carlo Nonato
@ 2022-10-22 15:51 ` Carlo Nonato
2022-10-22 15:51 ` [PATCH v3 8/9] xen/arm: add Xen cache colors command line parameter Carlo Nonato
` (2 subsequent siblings)
9 siblings, 0 replies; 26+ messages in thread
From: Carlo Nonato @ 2022-10-22 15:51 UTC (permalink / raw)
To: xen-devel
Cc: marco.solieri, andrea.bastoni, lucmiccio, Carlo Nonato,
Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk, Marco Solieri
This reverts commit 0c18fb76323bfb13615b6f13c98767face2d8097.
Cache coloring support for Xen needs to relocate Xen code and data in a new
colored physical space. The BOOT_RELOC_VIRT_START will be used as the
virtual base address for a temporary mapping to this new space.
Signed-off-by: Carlo Nonato <carlo.nonato@minervasys.tech>
Signed-off-by: Marco Solieri <marco.solieri@minervasys.tech>
---
v3:
- new revert because the commit reverted was introduced after v2
---
xen/arch/arm/include/asm/config.h | 4 +++-
xen/arch/arm/mm.c | 1 +
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/xen/arch/arm/include/asm/config.h b/xen/arch/arm/include/asm/config.h
index 0fefed1b8a..ca6f775668 100644
--- a/xen/arch/arm/include/asm/config.h
+++ b/xen/arch/arm/include/asm/config.h
@@ -77,7 +77,8 @@
* 2M - 4M Xen text, data, bss
* 4M - 6M Fixmap: special-purpose 4K mapping slots
* 6M - 10M Early boot mapping of FDT
- * 10M - 12M Livepatch vmap (if compiled in)
+ * 10M - 12M Early relocation address (used when relocating Xen)
+ * and later for livepatch vmap (if compiled in)
*
* ARM32 layout:
* 0 - 12M <COMMON>
@@ -113,6 +114,7 @@
#define BOOT_FDT_VIRT_START _AT(vaddr_t,0x00600000)
#define BOOT_FDT_VIRT_SIZE _AT(vaddr_t, MB(4))
+#define BOOT_RELOC_VIRT_START _AT(vaddr_t,0x00a00000)
#ifdef CONFIG_LIVEPATCH
#define LIVEPATCH_VMAP_START _AT(vaddr_t,0x00a00000)
#define LIVEPATCH_VMAP_SIZE _AT(vaddr_t, MB(2))
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 6ccffeaea5..a81b8f9286 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -154,6 +154,7 @@ static void __init __maybe_unused build_assertions(void)
/* 2MB aligned regions */
BUILD_BUG_ON(XEN_VIRT_START & ~SECOND_MASK);
BUILD_BUG_ON(FIXMAP_ADDR(0) & ~SECOND_MASK);
+ BUILD_BUG_ON(BOOT_RELOC_VIRT_START & ~SECOND_MASK);
/* 1GB aligned regions */
#ifdef CONFIG_ARM_32
BUILD_BUG_ON(XENHEAP_VIRT_START & ~FIRST_MASK);
--
2.34.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v3 8/9] xen/arm: add Xen cache colors command line parameter
2022-10-22 15:51 [PATCH v3 0/9] Arm cache coloring Carlo Nonato
` (6 preceding siblings ...)
2022-10-22 15:51 ` [PATCH v3 7/9] Revert "xen/arm: Remove unused BOOT_RELOC_VIRT_START" Carlo Nonato
@ 2022-10-22 15:51 ` Carlo Nonato
2022-10-22 15:51 ` [PATCH v3 9/9] xen/arm: add cache coloring support for Xen Carlo Nonato
2022-12-03 17:53 ` [PATCH v3 0/9] Arm cache coloring Julien Grall
9 siblings, 0 replies; 26+ messages in thread
From: Carlo Nonato @ 2022-10-22 15:51 UTC (permalink / raw)
To: xen-devel
Cc: marco.solieri, andrea.bastoni, lucmiccio, Stefano Stabellini,
Julien Grall, Bertrand Marquis, Volodymyr Babchuk, Andrew Cooper,
George Dunlap, Jan Beulich, Wei Liu, Marco Solieri, Carlo Nonato
From: Luca Miccio <lucmiccio@gmail.com>
This commit adds a new command line parameter to configure Xen cache
colors. These colors can be dumped with the cache coloring info debug-key.
By default, Xen uses the first color.
Benchmarking the VM interrupt response time provides an estimation of
LLC usage by Xen's most latency-critical runtime task. Results on Arm
Cortex-A53 on Xilinx Zynq UltraScale+ XCZU9EG show that one color, which
reserves 64 KiB of L2, is enough to attain best responsiveness.
More colors are instead very likely to be needed on processors whose L1
cache is physically-indexed and physically-tagged, such as Cortex-A57.
In such cases, coloring applies to L1 also, and there typically are two
distinct L1-colors. Therefore, reserving only one color for Xen would
senselessly partitions a cache memory that is already private, i.e.
underutilize it. The default amount of Xen colors is thus set to one.
Signed-off-by: Luca Miccio <lucmiccio@gmail.com>
Signed-off-by: Marco Solieri <marco.solieri@minervasys.tech>
Signed-off-by: Carlo Nonato <carlo.nonato@minervasys.tech>
---
docs/misc/arm/cache-coloring.rst | 8 ++++----
docs/misc/xen-command-line.pandoc | 9 +++++++++
xen/arch/arm/coloring.c | 30 ++++++++++++++++++++++++++++++
3 files changed, 43 insertions(+), 4 deletions(-)
diff --git a/docs/misc/arm/cache-coloring.rst b/docs/misc/arm/cache-coloring.rst
index 0c89278aee..80eb259dfa 100644
--- a/docs/misc/arm/cache-coloring.rst
+++ b/docs/misc/arm/cache-coloring.rst
@@ -19,8 +19,8 @@ In order to enable and use it, few steps are needed.
- If needed, change the amount of memory reserved for the buddy allocator either
from the Xen configuration file, via the CONFIG_BUDDY_ALLOCATOR_SIZE value,
or with the command line option. See `Colored allocator and buddy allocator`.
-- Assign colors to domains using the `Color selection format`_ (see
- `Coloring parameters`_ for more documentation pointers).
+- Assign colors to each memory pool (Xen, Dom0/DomUs) using the
+ `Color selection format`_ for `Coloring parameters`_ configuration.
Background
**********
@@ -113,8 +113,8 @@ Examples:
Coloring parameters
*******************
-LLC way size (as previously discussed) and Dom0 colors can be set using the
-appropriate command line parameters. See the relevant documentation in
+LLC way size (as previously discussed), Xen colors and Dom0 colors can be set
+using the appropriate command line parameters. See the relevant documentation in
"docs/misc/xen-command-line.pandoc".
DomUs colors can be set either in the xl configuration file (relative
diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index 25a59dd6a9..d831cf1196 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -2754,6 +2754,15 @@ In the case that x2apic is in use, this option switches between physical and
clustered mode. The default, given no hint from the **FADT**, is cluster
mode.
+### xen-colors (arm64)
+> `= List of [ <integer> | <integer>-<integer> ]`
+
+> Default: `0: the lowermost color`
+
+Specify Xen color configuration.
+Two colors are most likely needed on platforms where private caches are
+physically indexed, e.g. the L1 instruction cache of the Arm Cortex-A57.
+
### xenheap_megabytes (arm32)
> `= <size>`
diff --git a/xen/arch/arm/coloring.c b/xen/arch/arm/coloring.c
index 2cae215cd2..80c76c057f 100644
--- a/xen/arch/arm/coloring.c
+++ b/xen/arch/arm/coloring.c
@@ -32,6 +32,10 @@
#include <asm/processor.h>
#include <asm/sysregs.h>
+/* By default Xen uses the lowest color */
+#define XEN_DEFAULT_COLOR 0
+#define XEN_DEFAULT_NUM_COLORS 1
+
/* Size of an LLC way */
static unsigned int __ro_after_init llc_way_size;
/* Number of colors available in the LLC */
@@ -43,6 +47,9 @@ static uint64_t __ro_after_init addr_col_mask;
#define addr_set_color(addr, color) (((addr) & ~addr_col_mask) \
| ((color) << PAGE_SHIFT))
+static unsigned int xen_colors[CONFIG_MAX_CACHE_COLORS];
+static unsigned int xen_num_colors;
+
static unsigned int dom0_colors[CONFIG_MAX_CACHE_COLORS];
static unsigned int dom0_num_colors;
@@ -94,6 +101,12 @@ static int parse_color_config(const char *buf, unsigned int *colors,
size_param("llc-way-size", llc_way_size);
+static int __init parse_xen_colors(const char *s)
+{
+ return parse_color_config(s, xen_colors, &xen_num_colors);
+}
+custom_param("xen-colors", parse_xen_colors);
+
static int __init parse_dom0_colors(const char *s)
{
return parse_color_config(s, dom0_colors, &dom0_num_colors);
@@ -188,6 +201,8 @@ static void dump_coloring_info(unsigned char key)
printk("LLC way size: %u KiB\n", llc_way_size >> 10);
printk("Number of LLC colors supported: %u\n", max_colors);
printk("Address color mask: 0x%lx\n", addr_col_mask);
+ printk("Xen colors: ");
+ print_colors(xen_colors, xen_num_colors);
}
bool __init coloring_init(void)
@@ -225,6 +240,21 @@ bool __init coloring_init(void)
addr_col_mask = (max_colors - 1) << PAGE_SHIFT;
+ if ( !xen_num_colors )
+ {
+ printk(XENLOG_WARNING
+ "Xen color config not found. Using default color: %u\n",
+ XEN_DEFAULT_COLOR);
+ xen_colors[0] = XEN_DEFAULT_COLOR;
+ xen_num_colors = XEN_DEFAULT_NUM_COLORS;
+ }
+
+ if ( !check_colors(xen_colors, xen_num_colors) )
+ {
+ printk(XENLOG_ERR "Bad color config for Xen\n");
+ return false;
+ }
+
if ( !dom0_num_colors )
{
printk(XENLOG_WARNING
--
2.34.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v3 9/9] xen/arm: add cache coloring support for Xen
2022-10-22 15:51 [PATCH v3 0/9] Arm cache coloring Carlo Nonato
` (7 preceding siblings ...)
2022-10-22 15:51 ` [PATCH v3 8/9] xen/arm: add Xen cache colors command line parameter Carlo Nonato
@ 2022-10-22 15:51 ` Carlo Nonato
2022-12-03 17:53 ` [PATCH v3 0/9] Arm cache coloring Julien Grall
9 siblings, 0 replies; 26+ messages in thread
From: Carlo Nonato @ 2022-10-22 15:51 UTC (permalink / raw)
To: xen-devel
Cc: marco.solieri, andrea.bastoni, lucmiccio, Carlo Nonato,
Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk, Marco Solieri
This commit adds the cache coloring support for Xen own physical space.
It extends the implementation of setup_pagetables to make use of Xen cache
coloring configuration. Page tables construction is essentially the same
except for the fact that the physical addresses, in case of cache coloring,
are taken from the translation of a new, temporary, virtual space that is
physically colored.
The temporary mapping is also used to relocate Xen to the new physical
space starting at the address taken from the old get_xen_paddr() function
which is brought back for the occasion.
The temporary mapping is finally converted to a mapping of the "old"
(meaning the original physical space) Xen code, so that the boot CPU can
actually address the variables and functions used by secondary CPUs.
This happens when the boot CPU needs to bring up other CPUs (psci.c and
smpboot.c) and when the TTBR value is passed to them
(init_secondary_pagetables).
Finally, since the alternative framework needs to remap the Xen text and
inittext sections, this operation must be done in a coloring-aware way.
The function xen_remap_colored() is introduced for that.
Based on original work from: Luca Miccio <lucmiccio@gmail.com>
Signed-off-by: Carlo Nonato <carlo.nonato@minervasys.tech>
Signed-off-by: Marco Solieri <marco.solieri@minervasys.tech>
---
v3:
- changed next_xen_colored() to xen_colored_mfn() to work with MFNs instead
of addresses
- new macro for_each_xen_colored_mfn to iterate over Xen colored MFNs
- new function xen_remap_colored() to remap colored Xen instead of
__vmap_colored()
- use map_pages_to_xen() instead of custom mapping function during
setup_pagetables() (thanks to Julien)
- reintroduce relocate_xen() to switch to colored space
- removed useless virt_to_maddr_colored()
---
xen/arch/arm/alternative.c | 9 ++-
xen/arch/arm/arm64/head.S | 48 +++++++++++++++
xen/arch/arm/coloring.c | 38 ++++++++++++
xen/arch/arm/include/asm/coloring.h | 30 +++++++++
xen/arch/arm/include/asm/mm.h | 16 ++++-
xen/arch/arm/mm.c | 94 ++++++++++++++++++++++++++---
xen/arch/arm/psci.c | 4 +-
xen/arch/arm/setup.c | 74 ++++++++++++++++++++++-
xen/arch/arm/smpboot.c | 3 +-
xen/arch/arm/xen.lds.S | 2 +-
10 files changed, 297 insertions(+), 21 deletions(-)
diff --git a/xen/arch/arm/alternative.c b/xen/arch/arm/alternative.c
index f03cd943c6..a795aeec98 100644
--- a/xen/arch/arm/alternative.c
+++ b/xen/arch/arm/alternative.c
@@ -28,6 +28,7 @@
#include <asm/alternative.h>
#include <asm/atomic.h>
#include <asm/byteorder.h>
+#include <asm/coloring.h>
#include <asm/cpufeature.h>
#include <asm/insn.h>
#include <asm/page.h>
@@ -220,8 +221,12 @@ void __init apply_alternatives_all(void)
* The text and inittext section are read-only. So re-map Xen to
* be able to patch the code.
*/
- xenmap = __vmap(&xen_mfn, 1U << xen_order, 1, 1, PAGE_HYPERVISOR,
- VMAP_DEFAULT);
+ if ( IS_ENABLED(CONFIG_CACHE_COLORING) )
+ xenmap = xen_remap_colored(xen_mfn, xen_size);
+ else
+ xenmap = __vmap(&xen_mfn, 1U << xen_order, 1, 1, PAGE_HYPERVISOR,
+ VMAP_DEFAULT);
+
/* Re-mapping Xen is not expected to fail during boot. */
BUG_ON(!xenmap);
diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index ad014716db..71cffb54fe 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -813,6 +813,54 @@ ENDPROC(fail)
GLOBAL(_end_boot)
+/* Copy Xen to new location and switch TTBR
+ * x0 ttbr
+ * x1 source address
+ * x2 destination address
+ * x3 length
+ *
+ * Source and destination must be word aligned, length is rounded up
+ * to a 16 byte boundary.
+ *
+ * MUST BE VERY CAREFUL when saving things to RAM over the copy */
+ENTRY(relocate_xen)
+ /* Copy 16 bytes at a time using:
+ * x9: counter
+ * x10: data
+ * x11: data
+ * x12: source
+ * x13: destination
+ */
+ mov x9, x3
+ mov x12, x1
+ mov x13, x2
+
+1: ldp x10, x11, [x12], #16
+ stp x10, x11, [x13], #16
+
+ subs x9, x9, #16
+ bgt 1b
+
+ /* Flush destination from dcache using:
+ * x9: counter
+ * x10: step
+ * x11: vaddr
+ */
+ dsb sy /* So the CPU issues all writes to the range */
+
+ mov x9, x3
+ ldr x10, =dcache_line_bytes /* x10 := step */
+ ldr x10, [x10]
+ mov x11, x2
+
+1: dc cvac, x11
+
+ add x11, x11, x10
+ subs x9, x9, x10
+ bgt 1b
+
+ b switch_ttbr
+
/*
* Switch TTBR
*
diff --git a/xen/arch/arm/coloring.c b/xen/arch/arm/coloring.c
index 80c76c057f..857a798d8a 100644
--- a/xen/arch/arm/coloring.c
+++ b/xen/arch/arm/coloring.c
@@ -27,6 +27,7 @@
#include <xen/keyhandler.h>
#include <xen/param.h>
#include <xen/types.h>
+#include <xen/vmap.h>
#include <asm/coloring.h>
#include <asm/processor.h>
@@ -362,6 +363,43 @@ unsigned int get_max_colors(void)
return max_colors;
}
+mfn_t xen_colored_mfn(mfn_t mfn)
+{
+ paddr_t maddr = mfn_to_maddr(mfn);
+ unsigned int i, color = addr_to_color(maddr);
+
+ for( i = 0; i < xen_num_colors; i++ )
+ {
+ if ( color == xen_colors[i] )
+ return mfn;
+ else if ( color < xen_colors[i] )
+ return maddr_to_mfn(addr_set_color(maddr, xen_colors[i]));
+ }
+
+ /* Jump to next color space (llc_way_size bytes) and use the first color */
+ return maddr_to_mfn(addr_set_color(maddr + llc_way_size, xen_colors[0]));
+}
+
+void *xen_remap_colored(mfn_t xen_mfn, paddr_t xen_size)
+{
+ unsigned int i;
+ void *xenmap;
+ mfn_t *xen_colored_mfns = xmalloc_array(mfn_t, xen_size >> PAGE_SHIFT);
+
+ if ( !xen_colored_mfns )
+ panic("Can't allocate colored MFNs\n");
+
+ for_each_xen_colored_mfn( xen_mfn, i )
+ {
+ xen_colored_mfns[i] = xen_mfn;
+ }
+
+ xenmap = vmap(xen_colored_mfns, xen_size >> PAGE_SHIFT);
+ xfree(xen_colored_mfns);
+
+ return xenmap;
+}
+
/*
* Local variables:
* mode: C
diff --git a/xen/arch/arm/include/asm/coloring.h b/xen/arch/arm/include/asm/coloring.h
index 0147f95968..6e9c1212f5 100644
--- a/xen/arch/arm/include/asm/coloring.h
+++ b/xen/arch/arm/include/asm/coloring.h
@@ -27,10 +27,31 @@
#ifdef CONFIG_CACHE_COLORING
#include <xen/init.h>
+#include <xen/lib.h>
#include <xen/sched.h>
#include <public/arch-arm.h>
+/*
+ * Amount of memory that we need to map in order to color Xen. The value
+ * depends on the maximum number of available colors of the hardware. The
+ * memory size is pessimistically calculated assuming only one color is used,
+ * which means that any pages belonging to any other color has to be skipped.
+ */
+#define XEN_COLOR_MAP_SIZE \
+ ROUNDUP((_end - _start) * get_max_colors(), XEN_PADDR_ALIGN)
+
+/**
+ * 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)) )
+
struct page_info;
bool __init coloring_init(void);
@@ -47,8 +68,13 @@ unsigned int page_to_color(const struct page_info *pg);
unsigned int get_max_colors(void);
+mfn_t xen_colored_mfn(mfn_t mfn);
+void *xen_remap_colored(mfn_t xen_fn, paddr_t xen_size);
+
#else /* !CONFIG_CACHE_COLORING */
+#define XEN_COLOR_MAP_SIZE (_end - _start)
+
static inline bool __init coloring_init(void) { return true; }
static inline int domain_coloring_init(
struct domain *d, const struct xen_arch_domainconfig *config) { return 0; }
@@ -56,6 +82,10 @@ static inline void domain_coloring_free(struct domain *d) {}
static inline void domain_dump_coloring_info(struct domain *d) {}
static inline void prepare_color_domain_config(
struct xen_arch_domainconfig *config, const char *colors_str) {}
+static inline void *xen_remap_colored(mfn_t xen_fn, paddr_t xen_size)
+{
+ return NULL;
+}
#endif /* CONFIG_CACHE_COLORING */
#endif /* __ASM_ARM_COLORING_H__ */
diff --git a/xen/arch/arm/include/asm/mm.h b/xen/arch/arm/include/asm/mm.h
index e848fa4adf..f3f76a20b3 100644
--- a/xen/arch/arm/include/asm/mm.h
+++ b/xen/arch/arm/include/asm/mm.h
@@ -195,12 +195,26 @@ extern unsigned long total_pages;
#define PDX_GROUP_SHIFT SECOND_SHIFT
+#ifdef CONFIG_CACHE_COLORING
+#define virt_to_boot_virt(virt) (virt - XEN_VIRT_START + BOOT_RELOC_VIRT_START)
+#define set_value_for_secondary(var, val) \
+ *(typeof(var) *)(virt_to_boot_virt((vaddr_t)&var)) = val; \
+ clean_dcache(var);
+#else
+#define virt_to_boot_virt(virt) (virt)
+#define set_value_for_secondary(var, val) \
+ var = val; \
+ clean_dcache(var);
+#endif
+
/* Boot-time pagetable setup */
-extern void setup_pagetables(unsigned long boot_phys_offset);
+extern void setup_pagetables(unsigned long boot_phys_offset, paddr_t xen_paddr);
/* Map FDT in boot pagetable */
extern void *early_fdt_map(paddr_t fdt_paddr);
/* Remove early mappings */
extern void remove_early_mappings(void);
+/* Remove early coloring mappings */
+extern void remove_coloring_mappings(void);
/* Allocate and initialise pagetables for a secondary CPU. Sets init_ttbr to the
* new page table */
extern int init_secondary_pagetables(int cpu);
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index a81b8f9286..4721fd4a04 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -33,6 +33,7 @@
#include <xsm/xsm.h>
+#include <asm/coloring.h>
#include <asm/fixmap.h>
#include <asm/setup.h>
@@ -105,6 +106,9 @@ DEFINE_BOOT_PAGE_TABLE(boot_third);
static DEFINE_PAGE_TABLE(xen_pgtable);
static DEFINE_PAGE_TABLE(xen_first);
#define THIS_CPU_PGTABLE xen_pgtable
+#ifdef CONFIG_CACHE_COLORING
+static DEFINE_PAGE_TABLE(xen_colored_temp);
+#endif
#else
#define HYP_PT_ROOT_LEVEL 1
/* Per-CPU pagetable pages */
@@ -364,7 +368,11 @@ void flush_page_to_ram(unsigned long mfn, bool sync_icache)
static inline lpae_t pte_of_xenaddr(vaddr_t va)
{
+#ifdef CONFIG_CACHE_COLORING
+ paddr_t ma = virt_to_maddr(virt_to_boot_virt(va));
+#else
paddr_t ma = va + phys_offset;
+#endif
return mfn_to_xen_entry(maddr_to_mfn(ma), MT_NORMAL);
}
@@ -450,6 +458,7 @@ static void xen_pt_enforce_wnx(void)
flush_xen_tlb_local();
}
+extern void relocate_xen(uint64_t ttbr, void *src, void *dst, size_t len);
extern void switch_ttbr(uint64_t ttbr);
/* Clear a translation table and clean & invalidate the cache */
@@ -459,9 +468,54 @@ static void clear_table(void *table)
clean_and_invalidate_dcache_va_range(table, PAGE_SIZE);
}
-/* Boot-time pagetable setup.
- * Changes here may need matching changes in head.S */
-void __init setup_pagetables(unsigned long boot_phys_offset)
+#ifdef CONFIG_CACHE_COLORING
+static void __init create_coloring_temp_mappings(paddr_t xen_paddr)
+{
+ lpae_t pte;
+ unsigned int i;
+ mfn_t mfn = maddr_to_mfn(xen_paddr);
+
+ for_each_xen_colored_mfn( mfn, i )
+ {
+ pte = mfn_to_xen_entry(mfn, MT_NORMAL);
+ pte.pt.table = 1; /* level 3 mappings always have this bit set */
+ xen_colored_temp[i] = pte;
+ }
+
+ pte = mfn_to_xen_entry(virt_to_mfn(xen_colored_temp), MT_NORMAL);
+ pte.pt.table = 1;
+ write_pte(&boot_second[second_table_offset(BOOT_RELOC_VIRT_START)], pte);
+}
+
+void __init remove_coloring_mappings(void)
+{
+ int rc;
+
+ /* destroy the _PAGE_BLOCK mapping */
+ rc = modify_xen_mappings(BOOT_RELOC_VIRT_START,
+ BOOT_RELOC_VIRT_START + SZ_2M,
+ _PAGE_BLOCK);
+ BUG_ON(rc);
+}
+#else
+static void __init create_coloring_temp_mappings(paddr_t xen_paddr) {}
+void __init remove_coloring_mappings(void) {}
+#endif /* CONFIG_CACHE_COLORING */
+
+/*
+ * Boot-time pagetable setup with coloring support
+ * Changes here may need matching changes in head.S
+ *
+ * The coloring support consists of:
+ * - Create a temporary colored mapping that conforms to Xen color selection.
+ * - pte_of_xenaddr takes care of translating the virtual addresses to the
+ * new colored physical space and the returns the pte, so that the page table
+ * initialization can remain the same.
+ * - Copy Xen to the new colored physical space by exploiting the temporary
+ * mapping.
+ * - Update TTBR0_EL2 with the new root page table address.
+ */
+void __init setup_pagetables(unsigned long boot_phys_offset, paddr_t xen_paddr)
{
uint64_t ttbr;
lpae_t pte, *p;
@@ -469,6 +523,9 @@ void __init setup_pagetables(unsigned long boot_phys_offset)
phys_offset = boot_phys_offset;
+ if ( IS_ENABLED(CONFIG_CACHE_COLORING) )
+ create_coloring_temp_mappings(xen_paddr);
+
#ifdef CONFIG_ARM_64
p = (void *) xen_pgtable;
p[0] = pte_of_xenaddr((uintptr_t)xen_first);
@@ -515,13 +572,30 @@ void __init setup_pagetables(unsigned long boot_phys_offset)
pte.pt.table = 1;
xen_second[second_table_offset(FIXMAP_ADDR(0))] = pte;
+ if ( IS_ENABLED(CONFIG_CACHE_COLORING) )
+ {
+ ttbr = virt_to_maddr(virt_to_boot_virt((vaddr_t)xen_pgtable));
+ relocate_xen(ttbr, _start, (void *)BOOT_RELOC_VIRT_START,
+ _end - _start);
+ /*
+ * Keep original Xen memory mapped because secondary CPUs still point to it
+ * and a few variables needs to be accessed by the master CPU in order to
+ * let them boot. This mapping will also replace the one created at the
+ * beginning of setup_pagetables.
+ */
+ map_pages_to_xen(BOOT_RELOC_VIRT_START,
+ maddr_to_mfn(XEN_VIRT_START + phys_offset),
+ SZ_2M >> PAGE_SHIFT, PAGE_HYPERVISOR_RW | _PAGE_BLOCK);
+ }
+ else
+ {
#ifdef CONFIG_ARM_64
- ttbr = (uintptr_t) xen_pgtable + phys_offset;
+ ttbr = (uintptr_t) xen_pgtable + phys_offset;
#else
- ttbr = (uintptr_t) cpu0_pgtable + phys_offset;
+ ttbr = (uintptr_t) cpu0_pgtable + phys_offset;
#endif
-
- switch_ttbr(ttbr);
+ switch_ttbr(ttbr);
+ }
xen_pt_enforce_wnx();
@@ -552,8 +626,8 @@ int init_secondary_pagetables(int cpu)
/* Set init_ttbr for this CPU coming up. All CPus share a single setof
* pagetables, but rewrite it each time for consistency with 32 bit. */
- init_ttbr = (uintptr_t) xen_pgtable + phys_offset;
- clean_dcache(init_ttbr);
+ set_value_for_secondary(init_ttbr, virt_to_maddr(xen_pgtable));
+
return 0;
}
#else
@@ -1109,7 +1183,7 @@ static int xen_pt_update(unsigned long virt,
*
* XXX: Add a check.
*/
- const mfn_t root = virt_to_mfn(THIS_CPU_PGTABLE);
+ const mfn_t root = maddr_to_mfn(READ_SYSREG64(TTBR0_EL2));
/*
* The hardware was configured to forbid mapping both writeable and
diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c
index 0c90c2305c..4782f64c17 100644
--- a/xen/arch/arm/psci.c
+++ b/xen/arch/arm/psci.c
@@ -49,8 +49,8 @@ int call_psci_cpu_on(int cpu)
{
struct arm_smccc_res res;
- arm_smccc_smc(psci_cpu_on_nr, cpu_logical_map(cpu), __pa(init_secondary),
- &res);
+ arm_smccc_smc(psci_cpu_on_nr, cpu_logical_map(cpu),
+ __pa(virt_to_boot_virt((vaddr_t)init_secondary)), &res);
return PSCI_RET(res);
}
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index acc3e4ad72..6ad68b7f7e 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -465,7 +465,7 @@ static void * __init relocate_fdt(paddr_t dtb_paddr, size_t dtb_size)
return fdt;
}
-#ifdef CONFIG_ARM_32
+#if defined (CONFIG_ARM_32) || (CONFIG_CACHE_COLORING)
/*
* Returns the end address of the highest region in the range s..e
* with required size and alignment that does not conflict with the
@@ -557,7 +557,9 @@ static paddr_t __init consider_modules(paddr_t s, paddr_t e,
}
return e;
}
+#endif
+#ifdef CONFIG_ARM_32
/*
* Find a contiguous region that fits in the static heap region with
* required size and alignment, and return the end address of the region
@@ -631,6 +633,62 @@ static paddr_t __init next_module(paddr_t s, paddr_t *end)
return lowest;
}
+#ifdef CONFIG_CACHE_COLORING
+/**
+ * get_xen_paddr - get physical address to relocate Xen to
+ *
+ * Xen is relocated to as near to the top of RAM as possible and
+ * aligned to a XEN_PADDR_ALIGN boundary.
+ */
+static paddr_t __init get_xen_paddr(uint32_t xen_size)
+{
+ struct meminfo *mi = &bootinfo.mem;
+ paddr_t min_size;
+ paddr_t paddr = 0;
+ int i;
+
+ min_size = (xen_size + (XEN_PADDR_ALIGN-1)) & ~(XEN_PADDR_ALIGN-1);
+
+ /* Find the highest bank with enough space. */
+ for ( i = 0; i < mi->nr_banks; i++ )
+ {
+ const struct membank *bank = &mi->bank[i];
+ paddr_t s, e;
+
+ if ( bank->size >= min_size )
+ {
+ e = consider_modules(bank->start, bank->start + bank->size,
+ min_size, XEN_PADDR_ALIGN, 0);
+ if ( !e )
+ continue;
+
+#ifdef CONFIG_ARM_32
+ /* Xen must be under 4GB */
+ if ( e > 0x100000000ULL )
+ e = 0x100000000ULL;
+ if ( e < bank->start )
+ continue;
+#endif
+
+ s = e - min_size;
+
+ if ( s > paddr )
+ paddr = s;
+ }
+ }
+
+ if ( !paddr )
+ panic("Not enough memory to relocate Xen\n");
+
+ printk("Placing Xen at 0x%"PRIpaddr"-0x%"PRIpaddr"\n",
+ paddr, paddr + min_size);
+
+ return paddr;
+}
+#else
+static paddr_t __init get_xen_paddr(uint32_t xen_size) { return 0; }
+#endif
+
static void __init init_pdx(void)
{
paddr_t bank_start, bank_size, bank_end;
@@ -1013,8 +1071,6 @@ void __init start_xen(unsigned long boot_phys_offset,
/* Initialize traps early allow us to get backtrace when an error occurred */
init_traps();
- setup_pagetables(boot_phys_offset);
-
smp_clear_cpu_maps();
device_tree_flattened = early_fdt_map(fdt_paddr);
@@ -1040,8 +1096,13 @@ void __init start_xen(unsigned long boot_phys_offset,
{
if ( !coloring_init() )
panic("Xen cache coloring support: setup failed\n");
+ xen_bootmodule->size = XEN_COLOR_MAP_SIZE;
+ xen_bootmodule->start = get_xen_paddr(xen_bootmodule->size);
}
+ setup_pagetables(boot_phys_offset, xen_bootmodule->start);
+ device_tree_flattened = early_fdt_map(fdt_paddr);
+
setup_mm();
/* Parse the ACPI tables for possible boot-time configuration */
@@ -1156,6 +1217,13 @@ void __init start_xen(unsigned long boot_phys_offset,
setup_virt_paging();
+ /*
+ * The removal is done earlier than discard_initial_modules beacuse the
+ * livepatch init uses a virtual address equal to BOOT_RELOC_VIRT_START.
+ * Remove coloring mappings to expose a clear state to the livepatch module.
+ */
+ if ( IS_ENABLED(CONFIG_CACHE_COLORING) )
+ remove_coloring_mappings();
do_initcalls();
/*
diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
index f7bda3a18b..e7166ad79b 100644
--- a/xen/arch/arm/smpboot.c
+++ b/xen/arch/arm/smpboot.c
@@ -470,8 +470,7 @@ int __cpu_up(unsigned int cpu)
init_data.cpuid = cpu;
/* Open the gate for this CPU */
- smp_up_cpu = cpu_logical_map(cpu);
- clean_dcache(smp_up_cpu);
+ set_value_for_secondary(smp_up_cpu, cpu_logical_map(cpu));
rc = arch_cpu_up(cpu);
diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
index 92c2984052..333589c344 100644
--- a/xen/arch/arm/xen.lds.S
+++ b/xen/arch/arm/xen.lds.S
@@ -210,7 +210,7 @@ SECTIONS
. = ALIGN(POINTER_ALIGN);
__bss_end = .;
} :text
- _end = . ;
+ _end = ALIGN(PAGE_SIZE);
/* Section for the device tree blob (if any). */
.dtb : { *(.dtb) } :text
--
2.34.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v3 2/9] xen/arm: add cache coloring initialization for domains
2022-10-22 15:51 ` [PATCH v3 2/9] xen/arm: add cache coloring initialization for domains Carlo Nonato
@ 2022-10-25 12:07 ` Carlo Nonato
2022-11-21 14:50 ` Carlo Nonato
1 sibling, 0 replies; 26+ messages in thread
From: Carlo Nonato @ 2022-10-25 12:07 UTC (permalink / raw)
To: xen-devel
Cc: marco.solieri, andrea.bastoni, lucmiccio, Stefano Stabellini,
Julien Grall, Bertrand Marquis, Volodymyr Babchuk, Marco Solieri
On Sat, Oct 22, 2022 at 5:51 PM Carlo Nonato
<carlo.nonato@minervasys.tech> wrote:
>
> This commit adds array pointers to domains as well as to the hypercall
> and configuration structure employed in domain creation. The latter is used
> both by the toolstack and by Xen itself to pass configuration data to the
> domain creation function, so the XEN_GUEST_HANDLE macro must be adopted to
> be able to access guest memory in the first case. This implies special care
> for the copy of the configuration data into the domain data, meaning that a
> discrimination variable for the two possible code paths (coming from Xen or
> from the toolstack) is needed.
>
> The initialization and free functions for colored domains are also added.
> The former is responsible for allocating and populating the color array
> of the domain and it also checks for configuration issues. One of those
> issues is enabling both coloring and directmap for the domain because they
> contradicts one another. Since that, Dom0 must not be created with the
> directmap flag.
> The latter instead frees allocated memory.
>
> Based on original work from: Luca Miccio <lucmiccio@gmail.com>
>
> Signed-off-by: Carlo Nonato <carlo.nonato@minervasys.tech>
> Signed-off-by: Marco Solieri <marco.solieri@minervasys.tech>
> ---
> v3:
> - xfree() for colors array in case of errors in domain_coloring_init()
> ---
> docs/misc/arm/cache-coloring.rst | 14 ++++++-
> xen/arch/arm/coloring.c | 57 +++++++++++++++++++++++++++++
> xen/arch/arm/domain.c | 7 ++++
> xen/arch/arm/domain_build.c | 13 ++++++-
> xen/arch/arm/include/asm/coloring.h | 10 +++++
> xen/arch/arm/include/asm/domain.h | 4 ++
> xen/include/public/arch-arm.h | 8 ++++
> 7 files changed, 110 insertions(+), 3 deletions(-)
>
> diff --git a/docs/misc/arm/cache-coloring.rst b/docs/misc/arm/cache-coloring.rst
> index b0f9a2e917..e8ee8fafde 100644
> --- a/docs/misc/arm/cache-coloring.rst
> +++ b/docs/misc/arm/cache-coloring.rst
> @@ -16,7 +16,7 @@ In order to enable and use it, few steps are needed.
> (refer to menuconfig help for value meaning and when it should be changed).
>
> CONFIG_MAX_CACHE_COLORS=<n>
> -- Assign colors to Dom0 using the `Color selection format`_ (see
> +- Assign colors to domains using the `Color selection format`_ (see
> `Coloring parameters`_ for more documentation pointers).
>
> Background
> @@ -114,6 +114,9 @@ LLC way size (as previously discussed) and Dom0 colors can be set using the
> appropriate command line parameters. See the relevant documentation in
> "docs/misc/xen-command-line.pandoc".
>
> +Note that if no color configuration is provided for domains, they fallback to
> +the default one, which corresponds simply to all available colors.
> +
> Known issues and limitations
> ****************************
>
> @@ -133,3 +136,12 @@ too) is set to 2^15 = 32768 colors because of some limitation on the domain
> configuration structure size used in domain creation. "uint16_t" is the biggest
> integer type that fit the constraint and 2^15 is the biggest power of 2 it can
> easily represent. This value is big enough for the generic case, though.
> +
> +
> +"xen,static-mem" isn't supported when coloring is enabled
> +#########################################################
> +
> +In the domain configuration, "xen,static-mem" allows memory to be statically
> +allocated to the domain. This isn't possibile when cache coloring is enabled,
> +because that memory can't be guaranteed to be of the same colors assigned to
> +that domain.
> diff --git a/xen/arch/arm/coloring.c b/xen/arch/arm/coloring.c
> index 36eea2d6c0..a7b59f5aba 100644
> --- a/xen/arch/arm/coloring.c
> +++ b/xen/arch/arm/coloring.c
> @@ -23,6 +23,7 @@
> */
> #include <xen/bitops.h>
> #include <xen/errno.h>
> +#include <xen/guest_access.h>
> #include <xen/keyhandler.h>
> #include <xen/param.h>
> #include <xen/types.h>
> @@ -232,6 +233,62 @@ bool __init coloring_init(void)
> return true;
> }
>
> +int domain_coloring_init(struct domain *d,
> + const struct xen_arch_domainconfig *config)
> +{
> + if ( is_domain_direct_mapped(d) )
> + {
> + printk(XENLOG_ERR
> + "Can't enable coloring and directmap at the same time for %pd\n",
> + d);
> + return -EINVAL;
> + }
> +
> + if ( is_hardware_domain(d) )
> + {
> + d->arch.colors = dom0_colors;
> + d->arch.num_colors = dom0_num_colors;
> + }
> + else if ( config->num_colors == 0 )
> + {
> + printk(XENLOG_WARNING
> + "Color config not found for %pd. Using default\n", d);
> + d->arch.colors = xzalloc_array(unsigned int, max_colors);
> + d->arch.num_colors = set_default_domain_colors(d->arch.colors);
> + }
> + else
> + {
> + d->arch.colors = xzalloc_array(unsigned int, config->num_colors);
> + d->arch.num_colors = config->num_colors;
> + if ( config->from_guest )
> + copy_from_guest(d->arch.colors, config->colors, config->num_colors);
> + else
> + memcpy(d->arch.colors, config->colors.p,
> + sizeof(unsigned int) * config->num_colors);
> + }
> +
> + if ( !d->arch.colors )
> + {
> + printk(XENLOG_ERR "Colors allocation failed for %pd\n", d);
> + return -ENOMEM;
> + }
> +
> + if ( !check_colors(d->arch.colors, d->arch.num_colors) )
> + {
> + printk(XENLOG_ERR "Bad color config for %pd\n", d);
> + domain_coloring_free(d);
This results in a double free since the colors array is freed also
in arch_domain_destroy(). This will be reverted in v4.
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +void domain_coloring_free(struct domain *d)
> +{
> + if ( !is_hardware_domain(d) )
> + xfree(d->arch.colors);
> +}
> +
> /*
> * Local variables:
> * mode: C
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 2d6253181a..b4dd64dff4 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -23,6 +23,7 @@
> #include <xen/wait.h>
>
> #include <asm/alternative.h>
> +#include <asm/coloring.h>
> #include <asm/cpuerrata.h>
> #include <asm/cpufeature.h>
> #include <asm/current.h>
> @@ -712,6 +713,10 @@ int arch_domain_create(struct domain *d,
> ioreq_domain_init(d);
> #endif
>
> + if ( IS_ENABLED(CONFIG_CACHE_COLORING) &&
> + (rc = domain_coloring_init(d, &config->arch)) )
> + goto fail;
> +
> /* p2m_init relies on some value initialized by the IOMMU subsystem */
> if ( (rc = iommu_domain_init(d, config->iommu_opts)) != 0 )
> goto fail;
> @@ -807,6 +812,8 @@ void arch_domain_destroy(struct domain *d)
> get_order_from_bytes(d->arch.efi_acpi_len));
> #endif
> domain_io_free(d);
> + if ( IS_ENABLED(CONFIG_CACHE_COLORING) )
> + domain_coloring_free(d);
> }
>
> void arch_domain_shutdown(struct domain *d)
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 40e3c2e119..97f2060007 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -35,6 +35,12 @@
>
> #define STATIC_EVTCHN_NODE_SIZE_CELLS 2
>
> +#ifdef CONFIG_CACHE_COLORING
> +#define XEN_DOM0_CREATE_FLAGS CDF_privileged
> +#else
> +#define XEN_DOM0_CREATE_FLAGS CDF_privileged | CDF_directmap
> +#endif
> +
> static unsigned int __initdata opt_dom0_max_vcpus;
> integer_param("dom0_max_vcpus", opt_dom0_max_vcpus);
>
> @@ -3963,7 +3969,10 @@ static int __init construct_dom0(struct domain *d)
> /* type must be set before allocate_memory */
> d->arch.type = kinfo.type;
> #endif
> - allocate_memory_11(d, &kinfo);
> + if ( IS_ENABLED(CONFIG_CACHE_COLORING) )
> + allocate_memory(d, &kinfo);
> + else
> + allocate_memory_11(d, &kinfo);
> find_gnttab_region(d, &kinfo);
>
> #ifdef CONFIG_STATIC_SHM
> @@ -4025,7 +4034,7 @@ void __init create_dom0(void)
> if ( iommu_enabled )
> dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu;
>
> - dom0 = domain_create(0, &dom0_cfg, CDF_privileged | CDF_directmap);
> + dom0 = domain_create(0, &dom0_cfg, XEN_DOM0_CREATE_FLAGS);
> if ( IS_ERR(dom0) || (alloc_dom0_vcpu0(dom0) == NULL) )
> panic("Error creating domain 0\n");
>
> diff --git a/xen/arch/arm/include/asm/coloring.h b/xen/arch/arm/include/asm/coloring.h
> index 3b563d3b90..0d2dfada10 100644
> --- a/xen/arch/arm/include/asm/coloring.h
> +++ b/xen/arch/arm/include/asm/coloring.h
> @@ -27,12 +27,22 @@
> #ifdef CONFIG_CACHE_COLORING
>
> #include <xen/init.h>
> +#include <xen/sched.h>
> +
> +#include <public/arch-arm.h>
>
> bool __init coloring_init(void);
>
> +int domain_coloring_init(struct domain *d,
> + const struct xen_arch_domainconfig *config);
> +void domain_coloring_free(struct domain *d);
> +
> #else /* !CONFIG_CACHE_COLORING */
>
> static inline bool __init coloring_init(void) { return true; }
> +static inline int domain_coloring_init(
> + struct domain *d, const struct xen_arch_domainconfig *config) { return 0; }
> +static inline void domain_coloring_free(struct domain *d) {}
>
> #endif /* CONFIG_CACHE_COLORING */
> #endif /* __ASM_ARM_COLORING_H__ */
> diff --git a/xen/arch/arm/include/asm/domain.h b/xen/arch/arm/include/asm/domain.h
> index 26a8348eed..291f7c375d 100644
> --- a/xen/arch/arm/include/asm/domain.h
> +++ b/xen/arch/arm/include/asm/domain.h
> @@ -58,6 +58,10 @@ struct arch_domain
> #ifdef CONFIG_ARM_64
> enum domain_type type;
> #endif
> +#ifdef CONFIG_CACHE_COLORING
> + unsigned int *colors;
> + unsigned int num_colors;
> +#endif
>
> /* Virtual MMU */
> struct p2m_domain p2m;
> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
> index c8b6058d3a..adf843a7a1 100644
> --- a/xen/include/public/arch-arm.h
> +++ b/xen/include/public/arch-arm.h
> @@ -314,6 +314,8 @@ DEFINE_XEN_GUEST_HANDLE(vcpu_guest_context_t);
> #define XEN_DOMCTL_CONFIG_TEE_NONE 0
> #define XEN_DOMCTL_CONFIG_TEE_OPTEE 1
>
> +__DEFINE_XEN_GUEST_HANDLE(color_t, unsigned int);
> +
> struct xen_arch_domainconfig {
> /* IN/OUT */
> uint8_t gic_version;
> @@ -335,6 +337,12 @@ struct xen_arch_domainconfig {
> *
> */
> uint32_t clock_frequency;
> + /* IN */
> + uint8_t from_guest;
> + /* IN */
> + uint16_t num_colors;
> + /* IN */
> + XEN_GUEST_HANDLE(color_t) colors;
> };
> #endif /* __XEN__ || __XEN_TOOLS__ */
>
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 6/9] xen/common: add cache coloring allocator for domains
2022-10-22 15:51 ` [PATCH v3 6/9] xen/common: add cache coloring allocator for domains Carlo Nonato
@ 2022-10-25 12:08 ` Carlo Nonato
2022-11-10 16:47 ` Jan Beulich
1 sibling, 0 replies; 26+ messages in thread
From: Carlo Nonato @ 2022-10-25 12:08 UTC (permalink / raw)
To: xen-devel
Cc: marco.solieri, andrea.bastoni, lucmiccio, Stefano Stabellini,
Julien Grall, Bertrand Marquis, Volodymyr Babchuk, Andrew Cooper,
George Dunlap, Jan Beulich, Wei Liu, Marco Solieri
On Sat, Oct 22, 2022 at 5:51 PM Carlo Nonato
<carlo.nonato@minervasys.tech> wrote:
>
> From: Luca Miccio <lucmiccio@gmail.com>
>
> This commit adds a new memory page allocator that implements the cache
> coloring mechanism. The allocation algorithm follows the given domain color
> configuration and maximizes contiguity in the page selection of multiple
> subsequent requests.
>
> Pages are stored in a color-indexed array of lists, each one sorted by
> machine address, that is called the colored heap. A simple initialization
> function computes the color of any available page and inserts it in the
> corresponding list. When a domain requests a page, the allocator takes one
> from the subset of lists whose colors equals the domain configuration. It
> chooses the page with the highest machine address such that contiguous
> pages are sequentially allocated, if this is made possible by a color
> assignment which includes adjacent colors.
>
> The allocator can handle only requests with order equals to 0 since the
> single color granularity is represented in memory by one page.
>
> The buddy allocator must coexist with the colored one because the Xen heap
> isn't colored. For this reason a new Kconfig option and a command line
> parameter are added to let the user set the amount of memory reserved for
> the buddy allocator. Even when cache coloring is enabled, this memory isn't
> managed by the colored allocator.
>
> Colored heap information is dumped in the dump_heap() debug-key function.
>
> Signed-off-by: Luca Miccio <lucmiccio@gmail.com>
> Signed-off-by: Marco Solieri <marco.solieri@minervasys.tech>
> Signed-off-by: Carlo Nonato <carlo.nonato@minervasys.tech>
> ---
> v3:
> - fixed PGC_colored bits values
> - merged debug-key for dump_color_heap() with the one for dump_heap()
> - number of pages for each color in an array to easily dump color heap info
> - heap_lock in colored allocator to ensure atomicity and clarify it with a
> comment
> - added page_list_add_{next|prev} to add pages in the middle of the list
> - p2m tables use pages of same colors as domain
> - CONFIG_BUDDY_ALLOCATOR_SIZE is now an int (MiB)
> - buddy allocator reserved size is now respected as configured in Kconfig
> - removed useless functions and refactored the code
> - fixed PGC_colored flag that was removed when a page was allocated
> - merged with #7 since it would have been too small
> ---
> docs/misc/arm/cache-coloring.rst | 39 ++++-
> docs/misc/xen-command-line.pandoc | 14 ++
> xen/arch/arm/Kconfig | 12 ++
> xen/arch/arm/coloring.c | 10 ++
> xen/arch/arm/include/asm/coloring.h | 6 +
> xen/arch/arm/include/asm/mm.h | 3 +
> xen/arch/arm/p2m.c | 7 +-
> xen/common/page_alloc.c | 259 +++++++++++++++++++++++++---
> xen/include/xen/mm.h | 43 +++++
> 9 files changed, 371 insertions(+), 22 deletions(-)
>
> diff --git a/docs/misc/arm/cache-coloring.rst b/docs/misc/arm/cache-coloring.rst
> index dd2e851a26..0c89278aee 100644
> --- a/docs/misc/arm/cache-coloring.rst
> +++ b/docs/misc/arm/cache-coloring.rst
> @@ -16,6 +16,9 @@ In order to enable and use it, few steps are needed.
> (refer to menuconfig help for value meaning and when it should be changed).
>
> CONFIG_MAX_CACHE_COLORS=<n>
> +- If needed, change the amount of memory reserved for the buddy allocator either
> + from the Xen configuration file, via the CONFIG_BUDDY_ALLOCATOR_SIZE value,
> + or with the command line option. See `Colored allocator and buddy allocator`.
> - Assign colors to domains using the `Color selection format`_ (see
> `Coloring parameters`_ for more documentation pointers).
>
> @@ -162,6 +165,18 @@ Please refer to the relative documentation in
> Note that if no color configuration is provided for domains, they fallback to
> the default one, which corresponds simply to all available colors.
>
> +Colored allocator and buddy allocator
> +*************************************
> +
> +The colored allocator distributes pages based on color configurations of
> +domains so that each domains only gets pages of its own colors.
> +The colored allocator is meant as an alternative to the buddy allocator because
> +its allocation policy is by definition incompatible with the generic one. Since
> +the Xen heap is not colored yet, we need to support the coexistence of the two
> +allocators and some memory must be left for the buddy one.
> +The buddy allocator memory can be reserved from the Xen configuration file or
> +with the help of a command-line option.
> +
> Known issues and limitations
> ****************************
>
> @@ -182,7 +197,6 @@ configuration structure size used in domain creation. "uint16_t" is the biggest
> integer type that fit the constraint and 2^15 is the biggest power of 2 it can
> easily represent. This value is big enough for the generic case, though.
>
> -
> "xen,static-mem" isn't supported when coloring is enabled
> #########################################################
>
> @@ -190,3 +204,26 @@ In the domain configuration, "xen,static-mem" allows memory to be statically
> allocated to the domain. This isn't possibile when cache coloring is enabled,
> because that memory can't be guaranteed to be of the same colors assigned to
> that domain.
> +
> +Colored allocator can only make use of order-0 pages
> +####################################################
> +
> +The cache coloring technique relies on memory mappings and on the smallest
> +amount of memory that can be mapped to achieve the maximum number of colors
> +(cache partitions) possible. This amount is what is normally called a page and,
> +in Xen terminology, the order-0 page is the smallest one. The fairly simple
> +colored allocator currently implemented, makes use only of such pages.
> +It must be said that a more complex one could, in theory, adopt higher order
> +pages if the colors selection contained adjacent colors. Two subsequent colors,
> +for example, can be represented by an order-1 page, four colors correspond to
> +an order-2 page, etc.
> +
> +Fail to boot colored DomUs with large memory size
> +#################################################
> +
> +If the Linux kernel used for Dom0 does not contain the upstream commit
> +3941552aec1e04d63999988a057ae09a1c56ebeb and uses the hypercall buffer device,
> +colored DomUs with memory size larger then 127 MB cannot be created. This is
> +caused by the default limit of this buffer of 64 pages. The solution is to
> +manually apply the above patch, or to check if there is an updated version of
> +the kernel in use for Dom0 that contains this change.
> diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
> index 3f04414134..25a59dd6a9 100644
> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -299,6 +299,20 @@ can be maintained with the pv-shim mechanism.
> cause Xen not to use Indirect Branch Tracking even when support is
> available in hardware.
>
> +### buddy-alloc-size (arm64)
> +> `= <size>`
> +
> +> Default: `64M`
> +
> +Amount of memory reserved for the buddy allocator when colored allocator is
> +active. This options is parsed only when cache coloring support is enabled.
> +The colored allocator is meant as an alternative to the buddy allocator,
> +because its allocation policy is by definition incompatible with the
> +generic one. Since the Xen heap systems is not colored yet, we need to
> +support the coexistence of the two allocators for now. This parameter, which is
> +optional and for expert only, it's used to set the amount of memory reserved to
> +the buddy allocator.
> +
> ### clocksource (x86)
> > `= pit | hpet | acpi | tsc`
>
> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> index c45a9c5917..4cfa75b2ef 100644
> --- a/xen/arch/arm/Kconfig
> +++ b/xen/arch/arm/Kconfig
> @@ -153,6 +153,18 @@ config MAX_CACHE_COLORS
> Note that if, at any time, a color configuration with more colors than the
> maximum is employed, an error is produced.
>
> +config BUDDY_ALLOCATOR_SIZE
> + int "Buddy allocator reserved memory size (MiB)"
> + default "64"
> + depends on CACHE_COLORING
> + help
> + Amount of memory reserved for the buddy allocator to work alongside
> + the colored one. The colored allocator is meant as an alternative to the
> + buddy allocator because its allocation policy is by definition
> + incompatible with the generic one. Since the Xen heap is not colored yet,
> + we need to support the coexistence of the two allocators and some memory
> + must be left for the buddy one.
> +
> config TEE
> bool "Enable TEE mediators support (UNSUPPORTED)" if UNSUPPORTED
> default n
> diff --git a/xen/arch/arm/coloring.c b/xen/arch/arm/coloring.c
> index 685a431c3d..2cae215cd2 100644
> --- a/xen/arch/arm/coloring.c
> +++ b/xen/arch/arm/coloring.c
> @@ -322,6 +322,16 @@ void prepare_color_domain_config(struct xen_arch_domainconfig *config,
> config->num_colors = (uint16_t)num_colors;
> }
>
> +unsigned int page_to_color(const struct page_info *pg)
> +{
> + return addr_to_color(page_to_maddr(pg));
> +}
> +
> +unsigned int get_max_colors(void)
> +{
> + return max_colors;
> +}
> +
> /*
> * Local variables:
> * mode: C
> diff --git a/xen/arch/arm/include/asm/coloring.h b/xen/arch/arm/include/asm/coloring.h
> index 549eb408a3..0147f95968 100644
> --- a/xen/arch/arm/include/asm/coloring.h
> +++ b/xen/arch/arm/include/asm/coloring.h
> @@ -31,6 +31,8 @@
>
> #include <public/arch-arm.h>
>
> +struct page_info;
> +
> bool __init coloring_init(void);
>
> int domain_coloring_init(struct domain *d,
> @@ -41,6 +43,10 @@ void domain_dump_coloring_info(struct domain *d);
> void prepare_color_domain_config(struct xen_arch_domainconfig *config,
> const char *colors_str);
>
> +unsigned int page_to_color(const struct page_info *pg);
> +
> +unsigned int get_max_colors(void);
> +
> #else /* !CONFIG_CACHE_COLORING */
>
> static inline bool __init coloring_init(void) { return true; }
> diff --git a/xen/arch/arm/include/asm/mm.h b/xen/arch/arm/include/asm/mm.h
> index 68adcac9fa..e848fa4adf 100644
> --- a/xen/arch/arm/include/asm/mm.h
> +++ b/xen/arch/arm/include/asm/mm.h
> @@ -128,6 +128,9 @@ struct page_info
> #else
> #define PGC_static 0
> #endif
> +/* Page is cache colored */
> +#define _PGC_colored PG_shift(4)
> +#define PGC_colored PG_mask(1, 4)
> /* ... */
> /* Page is broken? */
> #define _PGC_broken PG_shift(7)
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 8449f97fe7..9ac7dc6216 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -661,7 +661,12 @@ static int p2m_create_table(struct p2m_domain *p2m, lpae_t *entry)
>
> ASSERT(!p2m_is_valid(*entry));
>
> - page = alloc_domheap_page(NULL, 0);
> + /* If cache coloring is enabled, p2m tables are allocated using the domain
> + * coloring configuration to prevent cache interference. */
> + if ( IS_ENABLED(CONFIG_CACHE_COLORING) )
> + page = alloc_domheap_page(p2m->domain, MEMF_no_refcount);
> + else
> + page = alloc_domheap_page(NULL, 0);
> if ( page == NULL )
> return -ENOMEM;
This diff can't be applied to the current master. I need to check
how things have changed in the meantime.
> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
> index 62afb07bc6..fe214cd6ac 100644
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -150,6 +150,9 @@
> #define p2m_pod_offline_or_broken_hit(pg) 0
> #define p2m_pod_offline_or_broken_replace(pg) BUG_ON(pg != NULL)
> #endif
> +#ifdef CONFIG_HAS_CACHE_COLORING
> +#include <asm/coloring.h>
> +#endif
>
> #ifndef PGC_static
> #define PGC_static 0
> @@ -231,6 +234,14 @@ static bool __read_mostly scrub_debug;
> #define scrub_debug false
> #endif
>
> +/* Memory required for buddy allocator to work with colored one */
> +#ifdef CONFIG_BUDDY_ALLOCATOR_SIZE
> +static unsigned long __initdata buddy_alloc_size =
> + CONFIG_BUDDY_ALLOCATOR_SIZE << 20;
> +#else
> + static unsigned long __initdata buddy_alloc_size = 0;
> +#endif
> +
> /*
> * Bit width of the DMA heap -- used to override NUMA-node-first.
> * allocation strategy, which can otherwise exhaust low memory.
> @@ -440,7 +451,180 @@ mfn_t __init alloc_boot_pages(unsigned long nr_pfns, unsigned long pfn_align)
> BUG();
> }
>
> +static DEFINE_SPINLOCK(heap_lock);
>
> +/* Initialise fields which have other uses for free pages. */
> +static void init_free_page_fields(struct page_info *pg)
> +{
> + pg->u.inuse.type_info = PGT_TYPE_INFO_INITIALIZER;
> + page_set_owner(pg, NULL);
> +}
> +
> +#ifdef CONFIG_CACHE_COLORING
> +/*************************
> + * COLORED SIDE-ALLOCATOR
> + *
> + * Pages are stored by their color in separate lists. Each list defines a color
> + * and it is initialized during end_boot_allocator, where each page's color
> + * is calculated and the page itself is put in the correct list.
> + * After initialization there will be N lists where N is the number of
> + * available colors on the platform.
> + * The {free|alloc}_color_heap_page overwrite pg->count_info, but they do it in
> + * the same way as the buddy allocator corresponding functions do:
> + * protecting the access with a critical section using heap_lock.
> + */
> +typedef struct page_list_head colored_pages_t;
> +static colored_pages_t *__ro_after_init _color_heap;
> +static unsigned long *__ro_after_init free_colored_pages;
> +
> +#define color_heap(color) (&_color_heap[color])
> +
> +static void free_color_heap_page(struct page_info *pg)
> +{
> + struct page_info *pos;
> + unsigned int color = page_to_color(pg);
> + colored_pages_t *head = color_heap(color);
> +
> + spin_lock(&heap_lock);
> +
> + pg->count_info = PGC_state_free | PGC_colored;
> + page_set_owner(pg, NULL);
> + free_colored_pages[color]++;
> +
> + page_list_for_each( pos, head )
> + {
> + if ( page_to_maddr(pos) < page_to_maddr(pg) )
> + break;
> + }
> +
> + page_list_add_next(pg, pos, head);
> +
> + spin_unlock(&heap_lock);
> +}
> +
> +static struct page_info *alloc_color_heap_page(unsigned int memflags,
> + const unsigned int *colors,
> + unsigned int num_colors)
> +{
> + struct page_info *pg = NULL;
> + unsigned int i, color;
> + bool need_tlbflush = false;
> + uint32_t tlbflush_timestamp = 0;
> +
> + spin_lock(&heap_lock);
> +
> + for ( i = 0; i < num_colors; i++ )
> + {
> + struct page_info *tmp;
> +
> + if ( page_list_empty(color_heap(colors[i])) )
> + continue;
> +
> + tmp = page_list_first(color_heap(colors[i]));
> + if ( !pg || page_to_maddr(tmp) > page_to_maddr(pg) )
> + pg = tmp;
> + }
> +
> + if ( !pg )
> + {
> + spin_unlock(&heap_lock);
> + return NULL;
> + }
> +
> + pg->count_info = PGC_state_inuse | PGC_colored;
> +
> + if ( !(memflags & MEMF_no_tlbflush) )
> + accumulate_tlbflush(&need_tlbflush, pg, &tlbflush_timestamp);
> +
> + init_free_page_fields(pg);
> + flush_page_to_ram(mfn_x(page_to_mfn(pg)),
> + !(memflags & MEMF_no_icache_flush));
> +
> + color = page_to_color(pg);
> + free_colored_pages[color]--;
> + page_list_del(pg, color_heap(color));
> +
> + spin_unlock(&heap_lock);
> +
> + if ( need_tlbflush )
> + filtered_flush_tlb_mask(tlbflush_timestamp);
> +
> + return pg;
> +}
> +
> +static void __init init_color_heap_pages(struct page_info *pg,
> + unsigned long nr_pages)
> +{
> + unsigned int i;
> +
> + if ( !_color_heap )
> + {
> + unsigned int max_colors = get_max_colors();
> +
> + _color_heap = xmalloc_array(colored_pages_t, max_colors);
> + BUG_ON(!_color_heap);
> + free_colored_pages = xzalloc_array(unsigned long, max_colors);
> + BUG_ON(!free_colored_pages);
> +
> + for ( i = 0; i < max_colors; i++ )
> + INIT_PAGE_LIST_HEAD(color_heap(i));
> + }
> +
> + printk(XENLOG_DEBUG
> + "Init color heap with %lu pages starting from: %#"PRIx64"\n",
> + nr_pages, page_to_maddr(pg));
> +
> + for ( i = 0; i < nr_pages; i++ )
> + free_color_heap_page(&pg[i]);
> +}
> +
> +static struct page_info *alloc_color_domheap_page(struct domain *d,
> + unsigned int memflags)
> +{
> + struct page_info *pg;
> +
> + pg = alloc_color_heap_page(memflags, d->arch.colors, d->arch.num_colors);
> + if ( !pg )
> + return NULL;
> +
> + if ( !(memflags & MEMF_no_owner) )
> + {
> + if ( memflags & MEMF_no_refcount )
> + pg->count_info |= PGC_extra;
> + if ( assign_page(pg, 0, d, memflags) )
> + {
> + free_color_heap_page(pg);
> + return NULL;
> + }
> + }
> +
> + return pg;
> +}
> +
> +static void dump_color_heap(void)
> +{
> + unsigned int color;
> +
> + printk("Dumping coloring heap info\n");
> + for ( color = 0; color < get_max_colors(); color++ )
> + printk("Color heap[%u]: %lu pages\n", color, free_colored_pages[color]);
> +}
> +
> +integer_param("buddy-alloc-size", buddy_alloc_size);
> +
> +#else /* !CONFIG_CACHE_COLORING */
> +
> +static void __init init_color_heap_pages(struct page_info *pg,
> + unsigned long nr_pages) {}
> +static struct page_info *alloc_color_domheap_page(struct domain *d,
> + unsigned int memflags)
> +{
> + return NULL;
> +}
> +static void free_color_heap_page(struct page_info *pg) {}
> +static void dump_color_heap(void) {}
> +
> +#endif /* CONFIG_CACHE_COLORING */
>
> /*************************
> * BINARY BUDDY ALLOCATOR
> @@ -462,7 +646,6 @@ static unsigned long node_need_scrub[MAX_NUMNODES];
> static unsigned long *avail[MAX_NUMNODES];
> static long total_avail_pages;
>
> -static DEFINE_SPINLOCK(heap_lock);
> static long outstanding_claims; /* total outstanding claims by all domains */
>
> unsigned long domain_adjust_tot_pages(struct domain *d, long pages)
> @@ -1027,10 +1210,7 @@ static struct page_info *alloc_heap_pages(
> accumulate_tlbflush(&need_tlbflush, &pg[i],
> &tlbflush_timestamp);
>
> - /* Initialise fields which have other uses for free pages. */
> - pg[i].u.inuse.type_info = PGT_TYPE_INFO_INITIALIZER;
> - page_set_owner(&pg[i], NULL);
> -
> + init_free_page_fields(&pg[i]);
> }
>
> spin_unlock(&heap_lock);
> @@ -1926,24 +2106,49 @@ static unsigned long avail_heap_pages(
> void __init end_boot_allocator(void)
> {
> unsigned int i;
> + unsigned long buddy_pages;
>
> - /* Pages that are free now go to the domain sub-allocator. */
> - for ( i = 0; i < nr_bootmem_regions; i++ )
> + buddy_pages = PFN_DOWN(buddy_alloc_size);
> +
> + if ( !IS_ENABLED(CONFIG_CACHE_COLORING) )
> {
> - struct bootmem_region *r = &bootmem_region_list[i];
> - if ( (r->s < r->e) &&
> - (phys_to_nid(pfn_to_paddr(r->s)) == cpu_to_node(0)) )
> + /* Pages that are free now go to the domain sub-allocator. */
> + for ( i = 0; i < nr_bootmem_regions; i++ )
> {
> - init_heap_pages(mfn_to_page(_mfn(r->s)), r->e - r->s);
> - r->e = r->s;
> - break;
> + struct bootmem_region *r = &bootmem_region_list[i];
> + if ( (r->s < r->e) &&
> + (phys_to_nid(pfn_to_paddr(r->s)) == cpu_to_node(0)) )
> + {
> + init_heap_pages(mfn_to_page(_mfn(r->s)), r->e - r->s);
> + r->e = r->s;
> + break;
> + }
> }
> }
> +
> for ( i = nr_bootmem_regions; i-- > 0; )
> {
> - struct bootmem_region *r = &bootmem_region_list[i];
> + struct bootmem_region *r;
> +
> + if ( IS_ENABLED(CONFIG_CACHE_COLORING) )
> + r = &bootmem_region_list[nr_bootmem_regions - i - 1];
> + else
> + r = &bootmem_region_list[i];
> +
> + if ( buddy_pages && (r->s < r->e) )
> + {
> + unsigned long pages = MIN(r->e - r->s, buddy_pages);
> + init_heap_pages(mfn_to_page(_mfn(r->s)), pages);
> + r->s += pages;
> + buddy_pages -= pages;
> + }
> if ( r->s < r->e )
> - init_heap_pages(mfn_to_page(_mfn(r->s)), r->e - r->s);
> + {
> + if ( IS_ENABLED(CONFIG_CACHE_COLORING) )
> + init_color_heap_pages(mfn_to_page(_mfn(r->s)), r->e - r->s);
> + else
> + init_heap_pages(mfn_to_page(_mfn(r->s)), r->e - r->s);
> + }
> }
> nr_bootmem_regions = 0;
>
> @@ -2344,7 +2549,8 @@ int assign_pages(
>
> for ( i = 0; i < nr; i++ )
> {
> - ASSERT(!(pg[i].count_info & ~(PGC_extra | PGC_static)));
> + ASSERT(!(pg[i].count_info & ~(PGC_extra | PGC_static |
> + PGC_colored)));
> if ( pg[i].count_info & PGC_extra )
> extra_pages++;
> }
> @@ -2429,6 +2635,15 @@ struct page_info *alloc_domheap_pages(
>
> ASSERT_ALLOC_CONTEXT();
>
> + /* Only domains are supported for coloring */
> + if ( IS_ENABLED(CONFIG_CACHE_COLORING) && d )
> + {
> + /* Colored allocation must be done on 0 order */
> + if ( order )
> + return NULL;
> + return alloc_color_domheap_page(d, memflags);
> + }
> +
> bits = domain_clamp_alloc_bitsize(memflags & MEMF_no_owner ? NULL : d,
> bits ? : (BITS_PER_LONG+PAGE_SHIFT));
>
> @@ -2546,7 +2761,10 @@ void free_domheap_pages(struct page_info *pg, unsigned int order)
> scrub = 1;
> }
>
> - free_heap_pages(pg, order, scrub);
> + if ( pg->count_info & PGC_colored )
> + free_color_heap_page(pg);
> + else
> + free_heap_pages(pg, order, scrub);
> }
>
> if ( drop_dom_ref )
> @@ -2653,6 +2871,9 @@ static void cf_check dump_heap(unsigned char key)
> continue;
> printk("Node %d has %lu unscrubbed pages\n", i, node_need_scrub[i]);
> }
> +
> + if ( IS_ENABLED(CONFIG_CACHE_COLORING) )
> + dump_color_heap();
> }
>
> static __init int cf_check register_heap_trigger(void)
> @@ -2785,9 +3006,7 @@ static bool prepare_staticmem_pages(struct page_info *pg, unsigned long nr_mfns,
> * to PGC_state_inuse.
> */
> pg[i].count_info = PGC_static | PGC_state_inuse;
> - /* Initialise fields which have other uses for free pages. */
> - pg[i].u.inuse.type_info = PGT_TYPE_INFO_INITIALIZER;
> - page_set_owner(&pg[i], NULL);
> + init_free_page_fields(&pg[i]);
> }
>
> spin_unlock(&heap_lock);
> diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
> index a925028ab3..0d48502e75 100644
> --- a/xen/include/xen/mm.h
> +++ b/xen/include/xen/mm.h
> @@ -297,6 +297,37 @@ page_list_add_tail(struct page_info *page, struct page_list_head *head)
> }
> head->tail = page;
> }
> +static inline void
> +_page_list_add(struct page_info *new, struct page_info *prev,
> + struct page_info *next)
> +{
> + new->list.prev = page_to_pdx(prev);
> + new->list.next = page_to_pdx(next);
> + prev->list.next = page_to_pdx(new);
> + next->list.prev = page_to_pdx(new);
> +}
> +static inline void
> +page_list_add_next(struct page_info *new, struct page_info *prev,
> + struct page_list_head *head)
> +{
> + struct page_info *next = page_list_next(prev, head);
> +
> + if ( !next )
> + page_list_add_tail(new, head);
> + else
> + _page_list_add(new, prev, next);
> +}
> +static inline void
> +page_list_add_prev(struct page_info *new, struct page_info *next,
> + struct page_list_head *head)
> +{
> + struct page_info *prev = page_list_prev(next, head);
> +
> + if ( !prev )
> + page_list_add(new, head);
> + else
> + _page_list_add(new, prev, next);
> +}
> static inline bool_t
> __page_list_del_head(struct page_info *page, struct page_list_head *head,
> struct page_info *next, struct page_info *prev)
> @@ -449,6 +480,18 @@ page_list_add_tail(struct page_info *page, struct page_list_head *head)
> list_add_tail(&page->list, head);
> }
> static inline void
> +page_list_add_next(struct page_info *new, struct page_info *prev,
> + struct page_list_head *head)
> +{
> + page_list_add_tail(new, &prev->list);
> +}
> +static inline void
> +page_list_add_prev(struct page_info *new, struct page_info *next,
> + struct page_list_head *head)
> +{
> + page_list_add(new, &next->list);
> +}
> +static inline void
> page_list_del(struct page_info *page, struct page_list_head *head)
> {
> list_del(&page->list);
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 6/9] xen/common: add cache coloring allocator for domains
2022-10-22 15:51 ` [PATCH v3 6/9] xen/common: add cache coloring allocator for domains Carlo Nonato
2022-10-25 12:08 ` Carlo Nonato
@ 2022-11-10 16:47 ` Jan Beulich
2022-11-14 15:04 ` Carlo Nonato
1 sibling, 1 reply; 26+ messages in thread
From: Jan Beulich @ 2022-11-10 16:47 UTC (permalink / raw)
To: Carlo Nonato
Cc: marco.solieri, andrea.bastoni, lucmiccio, Stefano Stabellini,
Julien Grall, Bertrand Marquis, Volodymyr Babchuk, Andrew Cooper,
George Dunlap, Wei Liu, Marco Solieri, xen-devel
On 22.10.2022 17:51, Carlo Nonato wrote:
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -661,7 +661,12 @@ static int p2m_create_table(struct p2m_domain *p2m, lpae_t *entry)
>
> ASSERT(!p2m_is_valid(*entry));
>
> - page = alloc_domheap_page(NULL, 0);
> + /* If cache coloring is enabled, p2m tables are allocated using the domain
> + * coloring configuration to prevent cache interference. */
> + if ( IS_ENABLED(CONFIG_CACHE_COLORING) )
> + page = alloc_domheap_page(p2m->domain, MEMF_no_refcount);
Are you sure you don't mean MEMF_no_owner (which implies MEMF_no_refcount)
here? And then ...
> + else
> + page = alloc_domheap_page(NULL, 0);
... is it really necessary to keep the two cases separate?
Also nit: Comment style.
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -150,6 +150,9 @@
> #define p2m_pod_offline_or_broken_hit(pg) 0
> #define p2m_pod_offline_or_broken_replace(pg) BUG_ON(pg != NULL)
> #endif
> +#ifdef CONFIG_HAS_CACHE_COLORING
> +#include <asm/coloring.h>
> +#endif
>
> #ifndef PGC_static
> #define PGC_static 0
> @@ -231,6 +234,14 @@ static bool __read_mostly scrub_debug;
> #define scrub_debug false
> #endif
>
> +/* Memory required for buddy allocator to work with colored one */
> +#ifdef CONFIG_BUDDY_ALLOCATOR_SIZE
> +static unsigned long __initdata buddy_alloc_size =
> + CONFIG_BUDDY_ALLOCATOR_SIZE << 20;
> +#else
> + static unsigned long __initdata buddy_alloc_size = 0;
Nit: Bogus indentation. I wonder anyway whether if wouldn't better
be
static unsigned long __initdata buddy_alloc_size =
#ifdef CONFIG_BUDDY_ALLOCATOR_SIZE
CONFIG_BUDDY_ALLOCATOR_SIZE << 20;
#else
0;
#endif
or
static unsigned long __initdata buddy_alloc_size
#ifdef CONFIG_BUDDY_ALLOCATOR_SIZE
= CONFIG_BUDDY_ALLOCATOR_SIZE << 20
#endif
;
> +static void free_color_heap_page(struct page_info *pg)
> +{
> + struct page_info *pos;
> + unsigned int color = page_to_color(pg);
> + colored_pages_t *head = color_heap(color);
> +
> + spin_lock(&heap_lock);
> +
> + pg->count_info = PGC_state_free | PGC_colored;
> + page_set_owner(pg, NULL);
> + free_colored_pages[color]++;
> +
> + page_list_for_each( pos, head )
> + {
> + if ( page_to_maddr(pos) < page_to_maddr(pg) )
> + break;
> + }
I continue to view such loops as problematic. With them in place I don't
think this feature can move to being (security) supported, so I think this
and similar places want annotating with a FIXME or alike comment.
> + page_list_add_next(pg, pos, head);
>
> + spin_unlock(&heap_lock);
> +}
> +
> +static struct page_info *alloc_color_heap_page(unsigned int memflags,
> + const unsigned int *colors,
> + unsigned int num_colors)
> +{
> + struct page_info *pg = NULL;
> + unsigned int i, color;
> + bool need_tlbflush = false;
> + uint32_t tlbflush_timestamp = 0;
> +
> + spin_lock(&heap_lock);
> +
> + for ( i = 0; i < num_colors; i++ )
> + {
> + struct page_info *tmp;
> +
> + if ( page_list_empty(color_heap(colors[i])) )
> + continue;
> +
> + tmp = page_list_first(color_heap(colors[i]));
> + if ( !pg || page_to_maddr(tmp) > page_to_maddr(pg) )
> + pg = tmp;
> + }
> +
> + if ( !pg )
> + {
> + spin_unlock(&heap_lock);
> + return NULL;
> + }
> +
> + pg->count_info = PGC_state_inuse | PGC_colored;
> +
> + if ( !(memflags & MEMF_no_tlbflush) )
> + accumulate_tlbflush(&need_tlbflush, pg, &tlbflush_timestamp);
> +
> + init_free_page_fields(pg);
> + flush_page_to_ram(mfn_x(page_to_mfn(pg)),
> + !(memflags & MEMF_no_icache_flush));
> +
> + color = page_to_color(pg);
You don't really need to retrieve the color here, do you? You could as
well latch it in the loop above.
> +static void dump_color_heap(void)
> +{
> + unsigned int color;
> +
> + printk("Dumping coloring heap info\n");
> + for ( color = 0; color < get_max_colors(); color++ )
> + printk("Color heap[%u]: %lu pages\n", color, free_colored_pages[color]);
> +}
> +
> +integer_param("buddy-alloc-size", buddy_alloc_size);
This would preferably live next to the variable it controls, e.g. (taking
the earlier comment into account)
static unsigned long __initdata buddy_alloc_size =
#ifdef CONFIG_CACHE_COLORING
CONFIG_BUDDY_ALLOCATOR_SIZE << 20;
integer_param("buddy-alloc-size", buddy_alloc_size);
#else
0;
#endif
(Assuming buddy_alloc_size is indeed used anywhere outside any #ifdef
CONFIG_CACHE_COLORING in the first place.)
> @@ -1926,24 +2106,49 @@ static unsigned long avail_heap_pages(
> void __init end_boot_allocator(void)
> {
> unsigned int i;
> + unsigned long buddy_pages;
>
> - /* Pages that are free now go to the domain sub-allocator. */
> - for ( i = 0; i < nr_bootmem_regions; i++ )
> + buddy_pages = PFN_DOWN(buddy_alloc_size);
Any reason this can't be the initializer of the variable?
> + if ( !IS_ENABLED(CONFIG_CACHE_COLORING) )
> {
> - struct bootmem_region *r = &bootmem_region_list[i];
> - if ( (r->s < r->e) &&
> - (phys_to_nid(pfn_to_paddr(r->s)) == cpu_to_node(0)) )
> + /* Pages that are free now go to the domain sub-allocator. */
> + for ( i = 0; i < nr_bootmem_regions; i++ )
> {
> - init_heap_pages(mfn_to_page(_mfn(r->s)), r->e - r->s);
> - r->e = r->s;
> - break;
> + struct bootmem_region *r = &bootmem_region_list[i];
> + if ( (r->s < r->e) &&
Even if you're only re-indenting the original code (which personally I'd
prefer if it was avoided), please add the missing blank line between
declaration and statement here.
> + (phys_to_nid(pfn_to_paddr(r->s)) == cpu_to_node(0)) )
> + {
> + init_heap_pages(mfn_to_page(_mfn(r->s)), r->e - r->s);
> + r->e = r->s;
> + break;
> + }
> }
> }
> +
> for ( i = nr_bootmem_regions; i-- > 0; )
> {
> - struct bootmem_region *r = &bootmem_region_list[i];
> + struct bootmem_region *r;
> +
> + if ( IS_ENABLED(CONFIG_CACHE_COLORING) )
> + r = &bootmem_region_list[nr_bootmem_regions - i - 1];
If you want to handle things low-to-high, why don't you alter the
earlier loop rather than skipping (and re-indenting) it? However,
considering that in alloc_color_heap_page() you prefer pages at
higher addresses, I continue to find it odd that here you want to
process low address pages first.
> + else
> + r = &bootmem_region_list[i];
> +
> + if ( buddy_pages && (r->s < r->e) )
> + {
> + unsigned long pages = MIN(r->e - r->s, buddy_pages);
> + init_heap_pages(mfn_to_page(_mfn(r->s)), pages);
Nit: Blank line between declaration(s) and statement(s) please. Also:
Any reason the type-safe min() cannot be used here?
> --- a/xen/include/xen/mm.h
> +++ b/xen/include/xen/mm.h
> @@ -297,6 +297,37 @@ page_list_add_tail(struct page_info *page, struct page_list_head *head)
> }
> head->tail = page;
> }
> +static inline void
> +_page_list_add(struct page_info *new, struct page_info *prev,
> + struct page_info *next)
> +{
> + new->list.prev = page_to_pdx(prev);
> + new->list.next = page_to_pdx(next);
> + prev->list.next = page_to_pdx(new);
> + next->list.prev = page_to_pdx(new);
Nit: Several hard tabs here, and ...
> +}
> +static inline void
> +page_list_add_next(struct page_info *new, struct page_info *prev,
> + struct page_list_head *head)
> +{
> + struct page_info *next = page_list_next(prev, head);
... one more here (and at least one more further down).
Afaict you're passing a NULL "pos" in here from free_color_heap_page()
if the list was previously empty and page lists aren't simply "normal"
(xen/list.h) lists. I don't consider it valid to call page_list_next()
with a NULL first argument, even if it looks as if this would work
right now as long as the list is empty (but I think we'd see a NULL
prev here also if all other pages looked at by free_color_heap_page()
are at lower addresses). So perhaps ...
> + if ( !next )
> + page_list_add_tail(new, head);
> + else
> + _page_list_add(new, prev, next);
if ( !prev )
page_list_add_tail(new, head);
else
_page_list_add(new, prev, page_list_next(prev, head));
?
> +}
> +static inline void
> +page_list_add_prev(struct page_info *new, struct page_info *next,
> + struct page_list_head *head)
> +{
> + struct page_info *prev = page_list_prev(next, head);
> +
> + if ( !prev )
> + page_list_add(new, head);
> + else
> + _page_list_add(new, prev, next);
> +}
This function looks to not be used anywhere.
Jan
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 6/9] xen/common: add cache coloring allocator for domains
2022-11-10 16:47 ` Jan Beulich
@ 2022-11-14 15:04 ` Carlo Nonato
2022-11-15 7:53 ` Jan Beulich
0 siblings, 1 reply; 26+ messages in thread
From: Carlo Nonato @ 2022-11-14 15:04 UTC (permalink / raw)
To: Jan Beulich
Cc: marco.solieri, andrea.bastoni, lucmiccio, Stefano Stabellini,
Julien Grall, Bertrand Marquis, Volodymyr Babchuk, Andrew Cooper,
George Dunlap, Wei Liu, Marco Solieri, xen-devel
Hi Jan,
On Thu, Nov 10, 2022 at 5:47 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 22.10.2022 17:51, Carlo Nonato wrote:
> > --- a/xen/arch/arm/p2m.c
> > +++ b/xen/arch/arm/p2m.c
> > @@ -661,7 +661,12 @@ static int p2m_create_table(struct p2m_domain *p2m, lpae_t *entry)
> >
> > ASSERT(!p2m_is_valid(*entry));
> >
> > - page = alloc_domheap_page(NULL, 0);
> > + /* If cache coloring is enabled, p2m tables are allocated using the domain
> > + * coloring configuration to prevent cache interference. */
> > + if ( IS_ENABLED(CONFIG_CACHE_COLORING) )
> > + page = alloc_domheap_page(p2m->domain, MEMF_no_refcount);
>
> Are you sure you don't mean MEMF_no_owner (which implies MEMF_no_refcount)
> here? And then ...
Yes. I've already fixed it in the v4 that I'm working on right now.
> > + else
> > + page = alloc_domheap_page(NULL, 0);
>
> ... is it really necessary to keep the two cases separate?
Not sure. I don't know the reason behind the original code.
> Also nit: Comment style.
>
> > --- a/xen/common/page_alloc.c
> > +++ b/xen/common/page_alloc.c
> > @@ -150,6 +150,9 @@
> > #define p2m_pod_offline_or_broken_hit(pg) 0
> > #define p2m_pod_offline_or_broken_replace(pg) BUG_ON(pg != NULL)
> > #endif
> > +#ifdef CONFIG_HAS_CACHE_COLORING
> > +#include <asm/coloring.h>
> > +#endif
> >
> > #ifndef PGC_static
> > #define PGC_static 0
> > @@ -231,6 +234,14 @@ static bool __read_mostly scrub_debug;
> > #define scrub_debug false
> > #endif
> >
> > +/* Memory required for buddy allocator to work with colored one */
> > +#ifdef CONFIG_BUDDY_ALLOCATOR_SIZE
> > +static unsigned long __initdata buddy_alloc_size =
> > + CONFIG_BUDDY_ALLOCATOR_SIZE << 20;
> > +#else
> > + static unsigned long __initdata buddy_alloc_size = 0;
>
> Nit: Bogus indentation. I wonder anyway whether if wouldn't better
> be
>
> static unsigned long __initdata buddy_alloc_size =
> #ifdef CONFIG_BUDDY_ALLOCATOR_SIZE
> CONFIG_BUDDY_ALLOCATOR_SIZE << 20;
> #else
> 0;
> #endif
>
> or
>
> static unsigned long __initdata buddy_alloc_size
> #ifdef CONFIG_BUDDY_ALLOCATOR_SIZE
> = CONFIG_BUDDY_ALLOCATOR_SIZE << 20
> #endif
> ;
>
> > +static void free_color_heap_page(struct page_info *pg)
> > +{
> > + struct page_info *pos;
> > + unsigned int color = page_to_color(pg);
> > + colored_pages_t *head = color_heap(color);
> > +
> > + spin_lock(&heap_lock);
> > +
> > + pg->count_info = PGC_state_free | PGC_colored;
> > + page_set_owner(pg, NULL);
> > + free_colored_pages[color]++;
> > +
> > + page_list_for_each( pos, head )
> > + {
> > + if ( page_to_maddr(pos) < page_to_maddr(pg) )
> > + break;
> > + }
>
> I continue to view such loops as problematic. With them in place I don't
> think this feature can move to being (security) supported, so I think this
> and similar places want annotating with a FIXME or alike comment.
So I have another change for that but I don't think it solves much.
I've turned free_color_heap_page() into free_color_heap_pages() to free more
pages with a single call. By doing so I can do the linear search once for
each color: after finding the right insert position, all the pages that
share the same color can be inserted one after the other. This should
speed up the init phase, but it doesn't solve the domain destroy phase where
pages are freed one by one.
> > + page_list_add_next(pg, pos, head);
> >
> > + spin_unlock(&heap_lock);
> > +}
> > +
> > +static struct page_info *alloc_color_heap_page(unsigned int memflags,
> > + const unsigned int *colors,
> > + unsigned int num_colors)
> > +{
> > + struct page_info *pg = NULL;
> > + unsigned int i, color;
> > + bool need_tlbflush = false;
> > + uint32_t tlbflush_timestamp = 0;
> > +
> > + spin_lock(&heap_lock);
> > +
> > + for ( i = 0; i < num_colors; i++ )
> > + {
> > + struct page_info *tmp;
> > +
> > + if ( page_list_empty(color_heap(colors[i])) )
> > + continue;
> > +
> > + tmp = page_list_first(color_heap(colors[i]));
> > + if ( !pg || page_to_maddr(tmp) > page_to_maddr(pg) )
> > + pg = tmp;
> > + }
> > +
> > + if ( !pg )
> > + {
> > + spin_unlock(&heap_lock);
> > + return NULL;
> > + }
> > +
> > + pg->count_info = PGC_state_inuse | PGC_colored;
> > +
> > + if ( !(memflags & MEMF_no_tlbflush) )
> > + accumulate_tlbflush(&need_tlbflush, pg, &tlbflush_timestamp);
> > +
> > + init_free_page_fields(pg);
> > + flush_page_to_ram(mfn_x(page_to_mfn(pg)),
> > + !(memflags & MEMF_no_icache_flush));
> > +
> > + color = page_to_color(pg);
>
> You don't really need to retrieve the color here, do you? You could as
> well latch it in the loop above.
Yes.
> > +static void dump_color_heap(void)
> > +{
> > + unsigned int color;
> > +
> > + printk("Dumping coloring heap info\n");
> > + for ( color = 0; color < get_max_colors(); color++ )
> > + printk("Color heap[%u]: %lu pages\n", color, free_colored_pages[color]);
> > +}
> > +
> > +integer_param("buddy-alloc-size", buddy_alloc_size);
>
> This would preferably live next to the variable it controls, e.g. (taking
> the earlier comment into account)
>
> static unsigned long __initdata buddy_alloc_size =
> #ifdef CONFIG_CACHE_COLORING
> CONFIG_BUDDY_ALLOCATOR_SIZE << 20;
> integer_param("buddy-alloc-size", buddy_alloc_size);
> #else
> 0;
> #endif
>
> (Assuming buddy_alloc_size is indeed used anywhere outside any #ifdef
> CONFIG_CACHE_COLORING in the first place.)
>
> > @@ -1926,24 +2106,49 @@ static unsigned long avail_heap_pages(
> > void __init end_boot_allocator(void)
> > {
> > unsigned int i;
> > + unsigned long buddy_pages;
> >
> > - /* Pages that are free now go to the domain sub-allocator. */
> > - for ( i = 0; i < nr_bootmem_regions; i++ )
> > + buddy_pages = PFN_DOWN(buddy_alloc_size);
>
> Any reason this can't be the initializer of the variable?
Nope. The end_boot_allocator() changes are a bit messy. In v4 I'm doing
things more nicely, moving everything in init_color_heap_pages().
> > + if ( !IS_ENABLED(CONFIG_CACHE_COLORING) )
> > {
> > - struct bootmem_region *r = &bootmem_region_list[i];
> > - if ( (r->s < r->e) &&
> > - (phys_to_nid(pfn_to_paddr(r->s)) == cpu_to_node(0)) )
> > + /* Pages that are free now go to the domain sub-allocator. */
> > + for ( i = 0; i < nr_bootmem_regions; i++ )
> > {
> > - init_heap_pages(mfn_to_page(_mfn(r->s)), r->e - r->s);
> > - r->e = r->s;
> > - break;
> > + struct bootmem_region *r = &bootmem_region_list[i];
> > + if ( (r->s < r->e) &&
>
> Even if you're only re-indenting the original code (which personally I'd
> prefer if it was avoided), please add the missing blank line between
> declaration and statement here.
>
> > + (phys_to_nid(pfn_to_paddr(r->s)) == cpu_to_node(0)) )
> > + {
> > + init_heap_pages(mfn_to_page(_mfn(r->s)), r->e - r->s);
> > + r->e = r->s;
> > + break;
> > + }
> > }
> > }
> > +
> > for ( i = nr_bootmem_regions; i-- > 0; )
> > {
> > - struct bootmem_region *r = &bootmem_region_list[i];
> > + struct bootmem_region *r;
> > +
> > + if ( IS_ENABLED(CONFIG_CACHE_COLORING) )
> > + r = &bootmem_region_list[nr_bootmem_regions - i - 1];
>
> If you want to handle things low-to-high, why don't you alter the
> earlier loop rather than skipping (and re-indenting) it?
Yes, you're right.
> However,
> considering that in alloc_color_heap_page() you prefer pages at
> higher addresses, I continue to find it odd that here you want to
> process low address pages first.
It doesn't matter if alloc_color_heap_page() returns higher or lower
addresses. The important thing is to create a sorted list so that min or
max are easily found. Having a sorted list means that it's easier to insert
pages if their addresses are always increasing or always decreasing, so that
starting either from the head or from the tail, the position where to insert
is found in O(1). If regions are processed high-to-low but pages of each
region are instead low-to-high, the always-decreasing/always-increasing
property doesn't hold anymore and the linear search needs to be repeated
multiple times. This problem can be solved in many ways and doing
everything low-to-high is one solution.
> > + else
> > + r = &bootmem_region_list[i];
> > +
> > + if ( buddy_pages && (r->s < r->e) )
> > + {
> > + unsigned long pages = MIN(r->e - r->s, buddy_pages);
> > + init_heap_pages(mfn_to_page(_mfn(r->s)), pages);
>
> Nit: Blank line between declaration(s) and statement(s) please. Also:
> Any reason the type-safe min() cannot be used here?
Not really. I've changed it.
> > --- a/xen/include/xen/mm.h
> > +++ b/xen/include/xen/mm.h
> > @@ -297,6 +297,37 @@ page_list_add_tail(struct page_info *page, struct page_list_head *head)
> > }
> > head->tail = page;
> > }
> > +static inline void
> > +_page_list_add(struct page_info *new, struct page_info *prev,
> > + struct page_info *next)
> > +{
> > + new->list.prev = page_to_pdx(prev);
> > + new->list.next = page_to_pdx(next);
> > + prev->list.next = page_to_pdx(new);
> > + next->list.prev = page_to_pdx(new);
>
> Nit: Several hard tabs here, and ...
>
> > +}
> > +static inline void
> > +page_list_add_next(struct page_info *new, struct page_info *prev,
> > + struct page_list_head *head)
> > +{
> > + struct page_info *next = page_list_next(prev, head);
>
> ... one more here (and at least one more further down).
Sorry, I don't really know how I've added those since my editor only uses
spaces...
> Afaict you're passing a NULL "pos" in here from free_color_heap_page()
> if the list was previously empty and page lists aren't simply "normal"
> (xen/list.h) lists. I don't consider it valid to call page_list_next()
> with a NULL first argument, even if it looks as if this would work
> right now as long as the list is empty (but I think we'd see a NULL
> prev here also if all other pages looked at by free_color_heap_page()
> are at lower addresses). So perhaps ...
>
> > + if ( !next )
> > + page_list_add_tail(new, head);
> > + else
> > + _page_list_add(new, prev, next);
>
> if ( !prev )
> page_list_add_tail(new, head);
> else
> _page_list_add(new, prev, page_list_next(prev, head));
>
> ?
Note: I was wrongly calling page_list_add_next() while I'm inserting a
predecessor instead. Anyway, yes, you're right about the fact that both next and
prev need to be checked since both can be NULL. This is my last version of
page_list_add_prev().
static inline void
page_list_add_prev(struct page_info *page, struct page_info *next,
struct page_list_head *head)
{
struct page_info *prev;
if ( !next )
{
page_list_add_tail(page, head);
return;
}
prev = page_list_prev(next, head);
if ( !prev )
page_list_add(page, head);
else
_page_list_add(page, prev, next);
}
> > +}
> > +static inline void
> > +page_list_add_prev(struct page_info *new, struct page_info *next,
> > + struct page_list_head *head)
> > +{
> > + struct page_info *prev = page_list_prev(next, head);
> > +
> > + if ( !prev )
> > + page_list_add(new, head);
> > + else
> > + _page_list_add(new, prev, next);
> > +}
>
> This function looks to not be used anywhere.
Yes. I've added it only for completeness. I'm gonna drop it.
> Jan
Thanks.
- Carlo Nonato
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 6/9] xen/common: add cache coloring allocator for domains
2022-11-14 15:04 ` Carlo Nonato
@ 2022-11-15 7:53 ` Jan Beulich
0 siblings, 0 replies; 26+ messages in thread
From: Jan Beulich @ 2022-11-15 7:53 UTC (permalink / raw)
To: Carlo Nonato
Cc: marco.solieri, andrea.bastoni, lucmiccio, Stefano Stabellini,
Julien Grall, Bertrand Marquis, Volodymyr Babchuk, Andrew Cooper,
George Dunlap, Wei Liu, Marco Solieri, xen-devel
On 14.11.2022 16:04, Carlo Nonato wrote:
> On Thu, Nov 10, 2022 at 5:47 PM Jan Beulich <jbeulich@suse.com> wrote:
>> On 22.10.2022 17:51, Carlo Nonato wrote:
>>> --- a/xen/arch/arm/p2m.c
>>> +++ b/xen/arch/arm/p2m.c
>>> @@ -661,7 +661,12 @@ static int p2m_create_table(struct p2m_domain *p2m, lpae_t *entry)
>>>
>>> ASSERT(!p2m_is_valid(*entry));
>>>
>>> - page = alloc_domheap_page(NULL, 0);
>>> + /* If cache coloring is enabled, p2m tables are allocated using the domain
>>> + * coloring configuration to prevent cache interference. */
>>> + if ( IS_ENABLED(CONFIG_CACHE_COLORING) )
>>> + page = alloc_domheap_page(p2m->domain, MEMF_no_refcount);
>>
>> Are you sure you don't mean MEMF_no_owner (which implies MEMF_no_refcount)
>> here? And then ...
>
> Yes. I've already fixed it in the v4 that I'm working on right now.
>
>>> + else
>>> + page = alloc_domheap_page(NULL, 0);
>>
>> ... is it really necessary to keep the two cases separate?
>
> Not sure. I don't know the reason behind the original code.
The difference becomes noticable in the NUMA case, which is only being
worked on for Arm. Yet that means that converting the original call in
a prereq patch, stating the NUMA effect as the justification, might be
the way to go. (See commit a7596378e454, which introduces the flag.)
>>> @@ -1926,24 +2106,49 @@ static unsigned long avail_heap_pages(
>>> void __init end_boot_allocator(void)
>>> {
>>> unsigned int i;
>>> + unsigned long buddy_pages;
>>>
>>> - /* Pages that are free now go to the domain sub-allocator. */
>>> - for ( i = 0; i < nr_bootmem_regions; i++ )
>>> + buddy_pages = PFN_DOWN(buddy_alloc_size);
>>
>> Any reason this can't be the initializer of the variable?
>
> Nope. The end_boot_allocator() changes are a bit messy. In v4 I'm doing
> things more nicely, moving everything in init_color_heap_pages().
>
>>> + if ( !IS_ENABLED(CONFIG_CACHE_COLORING) )
>>> {
>>> - struct bootmem_region *r = &bootmem_region_list[i];
>>> - if ( (r->s < r->e) &&
>>> - (phys_to_nid(pfn_to_paddr(r->s)) == cpu_to_node(0)) )
>>> + /* Pages that are free now go to the domain sub-allocator. */
>>> + for ( i = 0; i < nr_bootmem_regions; i++ )
>>> {
>>> - init_heap_pages(mfn_to_page(_mfn(r->s)), r->e - r->s);
>>> - r->e = r->s;
>>> - break;
>>> + struct bootmem_region *r = &bootmem_region_list[i];
>>> + if ( (r->s < r->e) &&
>>
>> Even if you're only re-indenting the original code (which personally I'd
>> prefer if it was avoided), please add the missing blank line between
>> declaration and statement here.
>>
>>> + (phys_to_nid(pfn_to_paddr(r->s)) == cpu_to_node(0)) )
>>> + {
>>> + init_heap_pages(mfn_to_page(_mfn(r->s)), r->e - r->s);
>>> + r->e = r->s;
>>> + break;
>>> + }
>>> }
>>> }
>>> +
>>> for ( i = nr_bootmem_regions; i-- > 0; )
>>> {
>>> - struct bootmem_region *r = &bootmem_region_list[i];
>>> + struct bootmem_region *r;
>>> +
>>> + if ( IS_ENABLED(CONFIG_CACHE_COLORING) )
>>> + r = &bootmem_region_list[nr_bootmem_regions - i - 1];
>>
>> If you want to handle things low-to-high, why don't you alter the
>> earlier loop rather than skipping (and re-indenting) it?
>
> Yes, you're right.
>
>> However,
>> considering that in alloc_color_heap_page() you prefer pages at
>> higher addresses, I continue to find it odd that here you want to
>> process low address pages first.
>
> It doesn't matter if alloc_color_heap_page() returns higher or lower
> addresses. The important thing is to create a sorted list so that min or
> max are easily found. Having a sorted list means that it's easier to insert
> pages if their addresses are always increasing or always decreasing, so that
> starting either from the head or from the tail, the position where to insert
> is found in O(1). If regions are processed high-to-low but pages of each
> region are instead low-to-high, the always-decreasing/always-increasing
> property doesn't hold anymore and the linear search needs to be repeated
> multiple times. This problem can be solved in many ways and doing
> everything low-to-high is one solution.
Of course. But please also put code churn in the set of a criteria to
apply. Page lists are doubly linked, so it shouldn't matter whether
you insert forwards or backwards.
Jan
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 2/9] xen/arm: add cache coloring initialization for domains
2022-10-22 15:51 ` [PATCH v3 2/9] xen/arm: add cache coloring initialization for domains Carlo Nonato
2022-10-25 12:07 ` Carlo Nonato
@ 2022-11-21 14:50 ` Carlo Nonato
2022-11-21 15:14 ` Jan Beulich
1 sibling, 1 reply; 26+ messages in thread
From: Carlo Nonato @ 2022-11-21 14:50 UTC (permalink / raw)
To: Jan Beulich, Andrew Cooper
Cc: marco.solieri, xen-devel, andrea.bastoni, lucmiccio,
Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk, Marco Solieri
Hi x86 devs,
I want to ask you some questions about this patch because in the previous
version me and Julien have discussed how cache colors should be passed in
domain creation. You should be able to read that discussion, anyway here is
a link to it
https://marc.info/?l=xen-devel&m=166151802002263
In short, using struct xen_arch_domainconfig works fine only when domctl
hypercall is issued. That struct contains a XEN_GUEST_HANDLE so it
should point to guest memory and must not be used when creating a domain
from Xen itself (i.e. dom0 or dom0less domains). The easy way to go is then
changing the domain_create() signature to require also a color array and its
length to be passed in for these latter cases.
Are you ok with that? See below for more comments.
Another question is then if xen_arch_domainconfig is the right place where to
put the coloring fields for domctl hypercall value passing.
See below for more comments.
I know that these two questions are very specific so let me know if something
is unclear.
On Sat, Oct 22, 2022 at 5:51 PM Carlo Nonato
<carlo.nonato@minervasys.tech> wrote:
>
> This commit adds array pointers to domains as well as to the hypercall
> and configuration structure employed in domain creation. The latter is used
> both by the toolstack and by Xen itself to pass configuration data to the
> domain creation function, so the XEN_GUEST_HANDLE macro must be adopted to
> be able to access guest memory in the first case. This implies special care
> for the copy of the configuration data into the domain data, meaning that a
> discrimination variable for the two possible code paths (coming from Xen or
> from the toolstack) is needed.
>
> The initialization and free functions for colored domains are also added.
> The former is responsible for allocating and populating the color array
> of the domain and it also checks for configuration issues. One of those
> issues is enabling both coloring and directmap for the domain because they
> contradicts one another. Since that, Dom0 must not be created with the
> directmap flag.
> The latter instead frees allocated memory.
>
> Based on original work from: Luca Miccio <lucmiccio@gmail.com>
>
> Signed-off-by: Carlo Nonato <carlo.nonato@minervasys.tech>
> Signed-off-by: Marco Solieri <marco.solieri@minervasys.tech>
> ---
> v3:
> - xfree() for colors array in case of errors in domain_coloring_init()
> ---
> docs/misc/arm/cache-coloring.rst | 14 ++++++-
> xen/arch/arm/coloring.c | 57 +++++++++++++++++++++++++++++
> xen/arch/arm/domain.c | 7 ++++
> xen/arch/arm/domain_build.c | 13 ++++++-
> xen/arch/arm/include/asm/coloring.h | 10 +++++
> xen/arch/arm/include/asm/domain.h | 4 ++
> xen/include/public/arch-arm.h | 8 ++++
> 7 files changed, 110 insertions(+), 3 deletions(-)
>
> diff --git a/docs/misc/arm/cache-coloring.rst b/docs/misc/arm/cache-coloring.rst
> index b0f9a2e917..e8ee8fafde 100644
> --- a/docs/misc/arm/cache-coloring.rst
> +++ b/docs/misc/arm/cache-coloring.rst
> @@ -16,7 +16,7 @@ In order to enable and use it, few steps are needed.
> (refer to menuconfig help for value meaning and when it should be changed).
>
> CONFIG_MAX_CACHE_COLORS=<n>
> -- Assign colors to Dom0 using the `Color selection format`_ (see
> +- Assign colors to domains using the `Color selection format`_ (see
> `Coloring parameters`_ for more documentation pointers).
>
> Background
> @@ -114,6 +114,9 @@ LLC way size (as previously discussed) and Dom0 colors can be set using the
> appropriate command line parameters. See the relevant documentation in
> "docs/misc/xen-command-line.pandoc".
>
> +Note that if no color configuration is provided for domains, they fallback to
> +the default one, which corresponds simply to all available colors.
> +
> Known issues and limitations
> ****************************
>
> @@ -133,3 +136,12 @@ too) is set to 2^15 = 32768 colors because of some limitation on the domain
> configuration structure size used in domain creation. "uint16_t" is the biggest
> integer type that fit the constraint and 2^15 is the biggest power of 2 it can
> easily represent. This value is big enough for the generic case, though.
> +
> +
> +"xen,static-mem" isn't supported when coloring is enabled
> +#########################################################
> +
> +In the domain configuration, "xen,static-mem" allows memory to be statically
> +allocated to the domain. This isn't possibile when cache coloring is enabled,
> +because that memory can't be guaranteed to be of the same colors assigned to
> +that domain.
> diff --git a/xen/arch/arm/coloring.c b/xen/arch/arm/coloring.c
> index 36eea2d6c0..a7b59f5aba 100644
> --- a/xen/arch/arm/coloring.c
> +++ b/xen/arch/arm/coloring.c
> @@ -23,6 +23,7 @@
> */
> #include <xen/bitops.h>
> #include <xen/errno.h>
> +#include <xen/guest_access.h>
> #include <xen/keyhandler.h>
> #include <xen/param.h>
> #include <xen/types.h>
> @@ -232,6 +233,62 @@ bool __init coloring_init(void)
> return true;
> }
>
> +int domain_coloring_init(struct domain *d,
> + const struct xen_arch_domainconfig *config)
> +{
> + if ( is_domain_direct_mapped(d) )
> + {
> + printk(XENLOG_ERR
> + "Can't enable coloring and directmap at the same time for %pd\n",
> + d);
> + return -EINVAL;
> + }
> +
> + if ( is_hardware_domain(d) )
> + {
> + d->arch.colors = dom0_colors;
> + d->arch.num_colors = dom0_num_colors;
> + }
> + else if ( config->num_colors == 0 )
> + {
> + printk(XENLOG_WARNING
> + "Color config not found for %pd. Using default\n", d);
> + d->arch.colors = xzalloc_array(unsigned int, max_colors);
> + d->arch.num_colors = set_default_domain_colors(d->arch.colors);
> + }
> + else
> + {
> + d->arch.colors = xzalloc_array(unsigned int, config->num_colors);
> + d->arch.num_colors = config->num_colors;
> + if ( config->from_guest )
> + copy_from_guest(d->arch.colors, config->colors, config->num_colors);
> + else
> + memcpy(d->arch.colors, config->colors.p,
> + sizeof(unsigned int) * config->num_colors);
> + }
Question 1:
Here is the current hacky solution in action: using config->from_guest to
decide whether to call copy_from_guest() or memcpy(). This is a no go for
Julien (and also for me right now). In my current work, I tried to get rid
of this field simply by calling copy_from_guest() only in domctl.c, but this
solution still isn't easy to maintain because the config->colors.p field can
either be a guest pointer or a Xen one and mixing the two semantics can be
problematic.
> +
> + if ( !d->arch.colors )
> + {
> + printk(XENLOG_ERR "Colors allocation failed for %pd\n", d);
> + return -ENOMEM;
> + }
> +
> + if ( !check_colors(d->arch.colors, d->arch.num_colors) )
> + {
> + printk(XENLOG_ERR "Bad color config for %pd\n", d);
> + domain_coloring_free(d);
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +void domain_coloring_free(struct domain *d)
> +{
> + if ( !is_hardware_domain(d) )
> + xfree(d->arch.colors);
> +}
> +
> /*
> * Local variables:
> * mode: C
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 2d6253181a..b4dd64dff4 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -23,6 +23,7 @@
> #include <xen/wait.h>
>
> #include <asm/alternative.h>
> +#include <asm/coloring.h>
> #include <asm/cpuerrata.h>
> #include <asm/cpufeature.h>
> #include <asm/current.h>
> @@ -712,6 +713,10 @@ int arch_domain_create(struct domain *d,
> ioreq_domain_init(d);
> #endif
>
> + if ( IS_ENABLED(CONFIG_CACHE_COLORING) &&
> + (rc = domain_coloring_init(d, &config->arch)) )
> + goto fail;
> +
> /* p2m_init relies on some value initialized by the IOMMU subsystem */
> if ( (rc = iommu_domain_init(d, config->iommu_opts)) != 0 )
> goto fail;
> @@ -807,6 +812,8 @@ void arch_domain_destroy(struct domain *d)
> get_order_from_bytes(d->arch.efi_acpi_len));
> #endif
> domain_io_free(d);
> + if ( IS_ENABLED(CONFIG_CACHE_COLORING) )
> + domain_coloring_free(d);
> }
>
> void arch_domain_shutdown(struct domain *d)
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 40e3c2e119..97f2060007 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -35,6 +35,12 @@
>
> #define STATIC_EVTCHN_NODE_SIZE_CELLS 2
>
> +#ifdef CONFIG_CACHE_COLORING
> +#define XEN_DOM0_CREATE_FLAGS CDF_privileged
> +#else
> +#define XEN_DOM0_CREATE_FLAGS CDF_privileged | CDF_directmap
> +#endif
> +
> static unsigned int __initdata opt_dom0_max_vcpus;
> integer_param("dom0_max_vcpus", opt_dom0_max_vcpus);
>
> @@ -3963,7 +3969,10 @@ static int __init construct_dom0(struct domain *d)
> /* type must be set before allocate_memory */
> d->arch.type = kinfo.type;
> #endif
> - allocate_memory_11(d, &kinfo);
> + if ( IS_ENABLED(CONFIG_CACHE_COLORING) )
> + allocate_memory(d, &kinfo);
> + else
> + allocate_memory_11(d, &kinfo);
> find_gnttab_region(d, &kinfo);
>
> #ifdef CONFIG_STATIC_SHM
> @@ -4025,7 +4034,7 @@ void __init create_dom0(void)
> if ( iommu_enabled )
> dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu;
>
> - dom0 = domain_create(0, &dom0_cfg, CDF_privileged | CDF_directmap);
> + dom0 = domain_create(0, &dom0_cfg, XEN_DOM0_CREATE_FLAGS);
> if ( IS_ERR(dom0) || (alloc_dom0_vcpu0(dom0) == NULL) )
> panic("Error creating domain 0\n");
>
> diff --git a/xen/arch/arm/include/asm/coloring.h b/xen/arch/arm/include/asm/coloring.h
> index 3b563d3b90..0d2dfada10 100644
> --- a/xen/arch/arm/include/asm/coloring.h
> +++ b/xen/arch/arm/include/asm/coloring.h
> @@ -27,12 +27,22 @@
> #ifdef CONFIG_CACHE_COLORING
>
> #include <xen/init.h>
> +#include <xen/sched.h>
> +
> +#include <public/arch-arm.h>
>
> bool __init coloring_init(void);
>
> +int domain_coloring_init(struct domain *d,
> + const struct xen_arch_domainconfig *config);
> +void domain_coloring_free(struct domain *d);
> +
> #else /* !CONFIG_CACHE_COLORING */
>
> static inline bool __init coloring_init(void) { return true; }
> +static inline int domain_coloring_init(
> + struct domain *d, const struct xen_arch_domainconfig *config) { return 0; }
> +static inline void domain_coloring_free(struct domain *d) {}
>
> #endif /* CONFIG_CACHE_COLORING */
> #endif /* __ASM_ARM_COLORING_H__ */
> diff --git a/xen/arch/arm/include/asm/domain.h b/xen/arch/arm/include/asm/domain.h
> index 26a8348eed..291f7c375d 100644
> --- a/xen/arch/arm/include/asm/domain.h
> +++ b/xen/arch/arm/include/asm/domain.h
> @@ -58,6 +58,10 @@ struct arch_domain
> #ifdef CONFIG_ARM_64
> enum domain_type type;
> #endif
> +#ifdef CONFIG_CACHE_COLORING
> + unsigned int *colors;
> + unsigned int num_colors;
> +#endif
>
> /* Virtual MMU */
> struct p2m_domain p2m;
> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
> index c8b6058d3a..adf843a7a1 100644
> --- a/xen/include/public/arch-arm.h
> +++ b/xen/include/public/arch-arm.h
> @@ -314,6 +314,8 @@ DEFINE_XEN_GUEST_HANDLE(vcpu_guest_context_t);
> #define XEN_DOMCTL_CONFIG_TEE_NONE 0
> #define XEN_DOMCTL_CONFIG_TEE_OPTEE 1
>
> +__DEFINE_XEN_GUEST_HANDLE(color_t, unsigned int);
Question 2:
This color_t definition is employed because the guest handle for
"unsigned int" (uint) is defined later (public/xen.h) and (citing Julien):
> Hmmm... And I guess we can't define "unsigned int" earlier because they
> rely on macro defined in arch-arm.h?
So the solution could be to move everything up a level in
xen_domctl_createdomain, where using uint wouldn't be a problem.
If this goes to common code then should it be guarded with some #ifdef
or not?
> +
> struct xen_arch_domainconfig {
> /* IN/OUT */
> uint8_t gic_version;
> @@ -335,6 +337,12 @@ struct xen_arch_domainconfig {
> *
> */
> uint32_t clock_frequency;
> + /* IN */
> + uint8_t from_guest;
> + /* IN */
> + uint16_t num_colors;
> + /* IN */
> + XEN_GUEST_HANDLE(color_t) colors;
> };
> #endif /* __XEN__ || __XEN_TOOLS__ */
>
> --
> 2.34.1
>
Thanks.
- Carlo Nonato
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 2/9] xen/arm: add cache coloring initialization for domains
2022-11-21 14:50 ` Carlo Nonato
@ 2022-11-21 15:14 ` Jan Beulich
2022-11-21 16:23 ` Carlo Nonato
0 siblings, 1 reply; 26+ messages in thread
From: Jan Beulich @ 2022-11-21 15:14 UTC (permalink / raw)
To: Carlo Nonato
Cc: marco.solieri, xen-devel, andrea.bastoni, lucmiccio,
Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk, Marco Solieri, Andrew Cooper,
Roger Pau Monné
On 21.11.2022 15:50, Carlo Nonato wrote:
> Hi x86 devs,
Any reason you didn't include Roger?
> I want to ask you some questions about this patch because in the previous
> version me and Julien have discussed how cache colors should be passed in
> domain creation. You should be able to read that discussion, anyway here is
> a link to it
>
> https://marc.info/?l=xen-devel&m=166151802002263
I've looked at the first few entries, without finding an answer to ...
> In short, using struct xen_arch_domainconfig works fine only when domctl
> hypercall is issued. That struct contains a XEN_GUEST_HANDLE so it
> should point to guest memory and must not be used when creating a domain
> from Xen itself (i.e. dom0 or dom0less domains). The easy way to go is then
> changing the domain_create() signature to require also a color array and its
> length to be passed in for these latter cases.
> Are you ok with that? See below for more comments.
... my immediate question: Does supplying the colors necessarily need to
done right at domain creation? Wouldn't it suffice to be done before first
allocating memory to the new domain, i.e. via a separate domctl (and then
for Xen-created domains via a separate Xen-internal function, which the
new domctl handling would also call)? Or do colors also affect the
allocation of struct domain itself (and pointers hanging off of it)?
> Another question is then if xen_arch_domainconfig is the right place where to
> put the coloring fields for domctl hypercall value passing.
> See below for more comments.
I think I said so before in other contexts: To me this coloring thing
isn't Arm-specific, and hence - despite only being implemented for Arm
right now - would preferably be generic at the interface level.
>> @@ -232,6 +233,62 @@ bool __init coloring_init(void)
>> return true;
>> }
>>
>> +int domain_coloring_init(struct domain *d,
>> + const struct xen_arch_domainconfig *config)
>> +{
>> + if ( is_domain_direct_mapped(d) )
>> + {
>> + printk(XENLOG_ERR
>> + "Can't enable coloring and directmap at the same time for %pd\n",
>> + d);
>> + return -EINVAL;
>> + }
>> +
>> + if ( is_hardware_domain(d) )
>> + {
>> + d->arch.colors = dom0_colors;
>> + d->arch.num_colors = dom0_num_colors;
>> + }
>> + else if ( config->num_colors == 0 )
>> + {
>> + printk(XENLOG_WARNING
>> + "Color config not found for %pd. Using default\n", d);
>> + d->arch.colors = xzalloc_array(unsigned int, max_colors);
>> + d->arch.num_colors = set_default_domain_colors(d->arch.colors);
>> + }
>> + else
>> + {
>> + d->arch.colors = xzalloc_array(unsigned int, config->num_colors);
>> + d->arch.num_colors = config->num_colors;
>> + if ( config->from_guest )
>> + copy_from_guest(d->arch.colors, config->colors, config->num_colors);
>> + else
>> + memcpy(d->arch.colors, config->colors.p,
>> + sizeof(unsigned int) * config->num_colors);
>> + }
>
> Question 1:
> Here is the current hacky solution in action: using config->from_guest to
> decide whether to call copy_from_guest() or memcpy(). This is a no go for
> Julien (and also for me right now). In my current work, I tried to get rid
> of this field simply by calling copy_from_guest() only in domctl.c, but this
> solution still isn't easy to maintain because the config->colors.p field can
> either be a guest pointer or a Xen one and mixing the two semantics can be
> problematic.
You simply cannot expect copy_from_guest() to work when the source pointer
is not a guest one.
>> --- a/xen/include/public/arch-arm.h
>> +++ b/xen/include/public/arch-arm.h
>> @@ -314,6 +314,8 @@ DEFINE_XEN_GUEST_HANDLE(vcpu_guest_context_t);
>> #define XEN_DOMCTL_CONFIG_TEE_NONE 0
>> #define XEN_DOMCTL_CONFIG_TEE_OPTEE 1
>>
>> +__DEFINE_XEN_GUEST_HANDLE(color_t, unsigned int);
>
> Question 2:
> This color_t definition is employed because the guest handle for
> "unsigned int" (uint) is defined later (public/xen.h) and (citing Julien):
>
>> Hmmm... And I guess we can't define "unsigned int" earlier because they
>> rely on macro defined in arch-arm.h?
>
> So the solution could be to move everything up a level in
> xen_domctl_createdomain, where using uint wouldn't be a problem.
> If this goes to common code then should it be guarded with some #ifdef
> or not?
As per above I'd say it shouldn't. But then you also shouldn't use
"unsigned int" in any new additions to the public interface. Only
fixed width types are suitable to use here.
Jan
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 2/9] xen/arm: add cache coloring initialization for domains
2022-11-21 15:14 ` Jan Beulich
@ 2022-11-21 16:23 ` Carlo Nonato
2022-11-21 16:40 ` Jan Beulich
0 siblings, 1 reply; 26+ messages in thread
From: Carlo Nonato @ 2022-11-21 16:23 UTC (permalink / raw)
To: Jan Beulich
Cc: marco.solieri, xen-devel, andrea.bastoni, lucmiccio,
Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk, Marco Solieri, Andrew Cooper,
Roger Pau Monné
On Mon, Nov 21, 2022 at 4:14 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 21.11.2022 15:50, Carlo Nonato wrote:
> > Hi x86 devs,
>
> Any reason you didn't include Roger?
Nope. Sorry, forgot to add him.
> > I want to ask you some questions about this patch because in the previous
> > version me and Julien have discussed how cache colors should be passed in
> > domain creation. You should be able to read that discussion, anyway here is
> > a link to it
> >
> > https://marc.info/?l=xen-devel&m=166151802002263
>
> I've looked at the first few entries, without finding an answer to ...
>
> > In short, using struct xen_arch_domainconfig works fine only when domctl
> > hypercall is issued. That struct contains a XEN_GUEST_HANDLE so it
> > should point to guest memory and must not be used when creating a domain
> > from Xen itself (i.e. dom0 or dom0less domains). The easy way to go is then
> > changing the domain_create() signature to require also a color array and its
> > length to be passed in for these latter cases.
> > Are you ok with that? See below for more comments.
>
> ... my immediate question: Does supplying the colors necessarily need to
> done right at domain creation? Wouldn't it suffice to be done before first
> allocating memory to the new domain, i.e. via a separate domctl (and then
> for Xen-created domains via a separate Xen-internal function, which the
> new domctl handling would also call)? Or do colors also affect the
> allocation of struct domain itself (and pointers hanging off of it)?
This would be really good. The only problem I can see is the p2m allocation
which is done during domain creation. With the current set of patches it
results in a "Failed to allocate P2M pages" since we want to have p2m tables
allocated with the same color of the domain and a null page is returned
because we have no colors.
> > Another question is then if xen_arch_domainconfig is the right place where to
> > put the coloring fields for domctl hypercall value passing.
> > See below for more comments.
>
> I think I said so before in other contexts: To me this coloring thing
> isn't Arm-specific, and hence - despite only being implemented for Arm
> right now - would preferably be generic at the interface level.
Ok, I'll try to do that.
> >> @@ -232,6 +233,62 @@ bool __init coloring_init(void)
> >> return true;
> >> }
> >>
> >> +int domain_coloring_init(struct domain *d,
> >> + const struct xen_arch_domainconfig *config)
> >> +{
> >> + if ( is_domain_direct_mapped(d) )
> >> + {
> >> + printk(XENLOG_ERR
> >> + "Can't enable coloring and directmap at the same time for %pd\n",
> >> + d);
> >> + return -EINVAL;
> >> + }
> >> +
> >> + if ( is_hardware_domain(d) )
> >> + {
> >> + d->arch.colors = dom0_colors;
> >> + d->arch.num_colors = dom0_num_colors;
> >> + }
> >> + else if ( config->num_colors == 0 )
> >> + {
> >> + printk(XENLOG_WARNING
> >> + "Color config not found for %pd. Using default\n", d);
> >> + d->arch.colors = xzalloc_array(unsigned int, max_colors);
> >> + d->arch.num_colors = set_default_domain_colors(d->arch.colors);
> >> + }
> >> + else
> >> + {
> >> + d->arch.colors = xzalloc_array(unsigned int, config->num_colors);
> >> + d->arch.num_colors = config->num_colors;
> >> + if ( config->from_guest )
> >> + copy_from_guest(d->arch.colors, config->colors, config->num_colors);
> >> + else
> >> + memcpy(d->arch.colors, config->colors.p,
> >> + sizeof(unsigned int) * config->num_colors);
> >> + }
> >
> > Question 1:
> > Here is the current hacky solution in action: using config->from_guest to
> > decide whether to call copy_from_guest() or memcpy(). This is a no go for
> > Julien (and also for me right now). In my current work, I tried to get rid
> > of this field simply by calling copy_from_guest() only in domctl.c, but this
> > solution still isn't easy to maintain because the config->colors.p field can
> > either be a guest pointer or a Xen one and mixing the two semantics can be
> > problematic.
>
> You simply cannot expect copy_from_guest() to work when the source pointer
> is not a guest one.
>
> >> --- a/xen/include/public/arch-arm.h
> >> +++ b/xen/include/public/arch-arm.h
> >> @@ -314,6 +314,8 @@ DEFINE_XEN_GUEST_HANDLE(vcpu_guest_context_t);
> >> #define XEN_DOMCTL_CONFIG_TEE_NONE 0
> >> #define XEN_DOMCTL_CONFIG_TEE_OPTEE 1
> >>
> >> +__DEFINE_XEN_GUEST_HANDLE(color_t, unsigned int);
> >
> > Question 2:
> > This color_t definition is employed because the guest handle for
> > "unsigned int" (uint) is defined later (public/xen.h) and (citing Julien):
> >
> >> Hmmm... And I guess we can't define "unsigned int" earlier because they
> >> rely on macro defined in arch-arm.h?
> >
> > So the solution could be to move everything up a level in
> > xen_domctl_createdomain, where using uint wouldn't be a problem.
> > If this goes to common code then should it be guarded with some #ifdef
> > or not?
>
> As per above I'd say it shouldn't. But then you also shouldn't use
> "unsigned int" in any new additions to the public interface. Only
> fixed width types are suitable to use here.
Got it.
> Jan
Thanks.
- Carlo Nonato
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 2/9] xen/arm: add cache coloring initialization for domains
2022-11-21 16:23 ` Carlo Nonato
@ 2022-11-21 16:40 ` Jan Beulich
2022-11-22 20:25 ` Julien Grall
0 siblings, 1 reply; 26+ messages in thread
From: Jan Beulich @ 2022-11-21 16:40 UTC (permalink / raw)
To: Carlo Nonato
Cc: marco.solieri, xen-devel, andrea.bastoni, lucmiccio,
Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk, Marco Solieri, Andrew Cooper,
Roger Pau Monné
On 21.11.2022 17:23, Carlo Nonato wrote:
> On Mon, Nov 21, 2022 at 4:14 PM Jan Beulich <jbeulich@suse.com> wrote:
>> On 21.11.2022 15:50, Carlo Nonato wrote:
>>> I want to ask you some questions about this patch because in the previous
>>> version me and Julien have discussed how cache colors should be passed in
>>> domain creation. You should be able to read that discussion, anyway here is
>>> a link to it
>>>
>>> https://marc.info/?l=xen-devel&m=166151802002263
>>
>> I've looked at the first few entries, without finding an answer to ...
>>
>>> In short, using struct xen_arch_domainconfig works fine only when domctl
>>> hypercall is issued. That struct contains a XEN_GUEST_HANDLE so it
>>> should point to guest memory and must not be used when creating a domain
>>> from Xen itself (i.e. dom0 or dom0less domains). The easy way to go is then
>>> changing the domain_create() signature to require also a color array and its
>>> length to be passed in for these latter cases.
>>> Are you ok with that? See below for more comments.
>>
>> ... my immediate question: Does supplying the colors necessarily need to
>> done right at domain creation? Wouldn't it suffice to be done before first
>> allocating memory to the new domain, i.e. via a separate domctl (and then
>> for Xen-created domains via a separate Xen-internal function, which the
>> new domctl handling would also call)? Or do colors also affect the
>> allocation of struct domain itself (and pointers hanging off of it)?
>
> This would be really good. The only problem I can see is the p2m allocation
> which is done during domain creation. With the current set of patches it
> results in a "Failed to allocate P2M pages" since we want to have p2m tables
> allocated with the same color of the domain and a null page is returned
> because we have no colors.
Hmm, I see. It would seem to me that this p2m init really is happening
too early. Ideally domain_create would merely mean creating a largely
empty container, with stuff being populated subsequently as necessary.
But I guess this is too much of a re-work to be done in the context
here, plus of course I may be overlooking something which actually
makes it necessary for domain creation to be structured the way it is
right now. (Imo the reason for the early minimal population of the p2m,
added only quite recently, wasn't a good one, and the vGIC init would
better be deferred. Yet once again I may lack details on why that's not
possible.)
Jan
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 2/9] xen/arm: add cache coloring initialization for domains
2022-11-21 16:40 ` Jan Beulich
@ 2022-11-22 20:25 ` Julien Grall
2022-11-23 9:41 ` Jan Beulich
0 siblings, 1 reply; 26+ messages in thread
From: Julien Grall @ 2022-11-22 20:25 UTC (permalink / raw)
To: Jan Beulich, Carlo Nonato
Cc: marco.solieri, xen-devel, andrea.bastoni, lucmiccio,
Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk,
Marco Solieri, Andrew Cooper, Roger Pau Monné
Hi Jan,
On 21/11/2022 16:40, Jan Beulich wrote:
> On 21.11.2022 17:23, Carlo Nonato wrote:
>> On Mon, Nov 21, 2022 at 4:14 PM Jan Beulich <jbeulich@suse.com> wrote:
>>> On 21.11.2022 15:50, Carlo Nonato wrote:
>>>> I want to ask you some questions about this patch because in the previous
>>>> version me and Julien have discussed how cache colors should be passed in
>>>> domain creation. You should be able to read that discussion, anyway here is
>>>> a link to it
>>>>
>>>> https://marc.info/?l=xen-devel&m=166151802002263
>>>
>>> I've looked at the first few entries, without finding an answer to ...
>>>
>>>> In short, using struct xen_arch_domainconfig works fine only when domctl
>>>> hypercall is issued. That struct contains a XEN_GUEST_HANDLE so it
>>>> should point to guest memory and must not be used when creating a domain
>>>> from Xen itself (i.e. dom0 or dom0less domains). The easy way to go is then
>>>> changing the domain_create() signature to require also a color array and its
>>>> length to be passed in for these latter cases.
>>>> Are you ok with that? See below for more comments.
>>>
>>> ... my immediate question: Does supplying the colors necessarily need to
>>> done right at domain creation? Wouldn't it suffice to be done before first
>>> allocating memory to the new domain, i.e. via a separate domctl (and then
>>> for Xen-created domains via a separate Xen-internal function, which the
>>> new domctl handling would also call)? Or do colors also affect the
>>> allocation of struct domain itself (and pointers hanging off of it)?
>>
>> This would be really good. The only problem I can see is the p2m allocation
>> which is done during domain creation. With the current set of patches it
>> results in a "Failed to allocate P2M pages" since we want to have p2m tables
>> allocated with the same color of the domain and a null page is returned
>> because we have no colors.
>
> Hmm, I see. It would seem to me that this p2m init really is happening
> too early. Ideally domain_create would merely mean creating a largely
> empty container, with stuff being populated subsequently as necessary.
The vGIC is not optional. So to me it sounds wrong to defer the decision
to after the domain is created.
It is not clear to me how you would check that mandatory components have
been properly initialized.
> But I guess this is too much of a re-work to be done in the context
> here, plus of course I may be overlooking something which actually
> makes it necessary for domain creation to be structured the way it is
> right now. (Imo the reason for the early minimal population of the p2m,
> added only quite recently, wasn't a good one, and the vGIC init would
> better be deferred. Yet once again I may lack details on why that's not
> possible.)
See above for the theoritical part. For the practice part, we need to
know the vGIC version at domain creation because it impact the maximum
of vCPU we can expose.
It is also not very clear where this could be initialized. Are you
suggesting to add an extra mandatory hypercall? FAOD, I don't think
p2m_set_allocation() would be the right place.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 2/9] xen/arm: add cache coloring initialization for domains
2022-11-22 20:25 ` Julien Grall
@ 2022-11-23 9:41 ` Jan Beulich
2022-11-23 9:48 ` Jan Beulich
0 siblings, 1 reply; 26+ messages in thread
From: Jan Beulich @ 2022-11-23 9:41 UTC (permalink / raw)
To: Julien Grall
Cc: marco.solieri, xen-devel, andrea.bastoni, lucmiccio,
Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk,
Marco Solieri, Andrew Cooper, Roger Pau Monné,
Carlo Nonato
On 22.11.2022 21:25, Julien Grall wrote:
> Hi Jan,
>
> On 21/11/2022 16:40, Jan Beulich wrote:
>> On 21.11.2022 17:23, Carlo Nonato wrote:
>>> On Mon, Nov 21, 2022 at 4:14 PM Jan Beulich <jbeulich@suse.com> wrote:
>>>> On 21.11.2022 15:50, Carlo Nonato wrote:
>>>>> I want to ask you some questions about this patch because in the previous
>>>>> version me and Julien have discussed how cache colors should be passed in
>>>>> domain creation. You should be able to read that discussion, anyway here is
>>>>> a link to it
>>>>>
>>>>> https://marc.info/?l=xen-devel&m=166151802002263
>>>>
>>>> I've looked at the first few entries, without finding an answer to ...
>>>>
>>>>> In short, using struct xen_arch_domainconfig works fine only when domctl
>>>>> hypercall is issued. That struct contains a XEN_GUEST_HANDLE so it
>>>>> should point to guest memory and must not be used when creating a domain
>>>>> from Xen itself (i.e. dom0 or dom0less domains). The easy way to go is then
>>>>> changing the domain_create() signature to require also a color array and its
>>>>> length to be passed in for these latter cases.
>>>>> Are you ok with that? See below for more comments.
>>>>
>>>> ... my immediate question: Does supplying the colors necessarily need to
>>>> done right at domain creation? Wouldn't it suffice to be done before first
>>>> allocating memory to the new domain, i.e. via a separate domctl (and then
>>>> for Xen-created domains via a separate Xen-internal function, which the
>>>> new domctl handling would also call)? Or do colors also affect the
>>>> allocation of struct domain itself (and pointers hanging off of it)?
>>>
>>> This would be really good. The only problem I can see is the p2m allocation
>>> which is done during domain creation. With the current set of patches it
>>> results in a "Failed to allocate P2M pages" since we want to have p2m tables
>>> allocated with the same color of the domain and a null page is returned
>>> because we have no colors.
>>
>> Hmm, I see. It would seem to me that this p2m init really is happening
>> too early. Ideally domain_create would merely mean creating a largely
>> empty container, with stuff being populated subsequently as necessary.
>
> The vGIC is not optional. So to me it sounds wrong to defer the decision
> to after the domain is created.
>
> It is not clear to me how you would check that mandatory components have
> been properly initialized.
There could be final checking right before unpausing a domain for the
first time.
>> But I guess this is too much of a re-work to be done in the context
>> here, plus of course I may be overlooking something which actually
>> makes it necessary for domain creation to be structured the way it is
>> right now. (Imo the reason for the early minimal population of the p2m,
>> added only quite recently, wasn't a good one, and the vGIC init would
>> better be deferred. Yet once again I may lack details on why that's not
>> possible.)
>
> See above for the theoritical part. For the practice part, we need to
> know the vGIC version at domain creation because it impact the maximum
> of vCPU we can expose.
Sure - I didn't suggest to leave that information out of domain_create.
> It is also not very clear where this could be initialized. Are you
> suggesting to add an extra mandatory hypercall? FAOD, I don't think
> p2m_set_allocation() would be the right place.
I agree with the latter - that would at best be a bodge. And yes, I was
thinking of having a separate domctl for that purpose.
Jan
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 2/9] xen/arm: add cache coloring initialization for domains
2022-11-23 9:41 ` Jan Beulich
@ 2022-11-23 9:48 ` Jan Beulich
0 siblings, 0 replies; 26+ messages in thread
From: Jan Beulich @ 2022-11-23 9:48 UTC (permalink / raw)
To: Julien Grall
Cc: marco.solieri, xen-devel, andrea.bastoni, lucmiccio,
Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk,
Marco Solieri, Andrew Cooper, Roger Pau Monné,
Carlo Nonato
On 23.11.2022 10:41, Jan Beulich wrote:
> On 22.11.2022 21:25, Julien Grall wrote:
>> On 21/11/2022 16:40, Jan Beulich wrote:
>>> On 21.11.2022 17:23, Carlo Nonato wrote:
>>>> This would be really good. The only problem I can see is the p2m allocation
>>>> which is done during domain creation. With the current set of patches it
>>>> results in a "Failed to allocate P2M pages" since we want to have p2m tables
>>>> allocated with the same color of the domain and a null page is returned
>>>> because we have no colors.
>>>
>>> Hmm, I see. It would seem to me that this p2m init really is happening
>>> too early. Ideally domain_create would merely mean creating a largely
>>> empty container, with stuff being populated subsequently as necessary.
>>
>> The vGIC is not optional. So to me it sounds wrong to defer the decision
>> to after the domain is created.
>>
>> It is not clear to me how you would check that mandatory components have
>> been properly initialized.
>
> There could be final checking right before unpausing a domain for the
> first time.
>
>>> But I guess this is too much of a re-work to be done in the context
>>> here, plus of course I may be overlooking something which actually
>>> makes it necessary for domain creation to be structured the way it is
>>> right now. (Imo the reason for the early minimal population of the p2m,
>>> added only quite recently, wasn't a good one, and the vGIC init would
>>> better be deferred. Yet once again I may lack details on why that's not
>>> possible.)
>>
>> See above for the theoritical part. For the practice part, we need to
>> know the vGIC version at domain creation because it impact the maximum
>> of vCPU we can expose.
>
> Sure - I didn't suggest to leave that information out of domain_create.
>
>> It is also not very clear where this could be initialized. Are you
>> suggesting to add an extra mandatory hypercall? FAOD, I don't think
>> p2m_set_allocation() would be the right place.
>
> I agree with the latter - that would at best be a bodge. And yes, I was
> thinking of having a separate domctl for that purpose.
The expand further: I think setvnumainfo would also better be issued
before _any_ memory allocations for a domain. And I view vNUMA data as
kind of comparable to the coloring data here.
Jan
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 4/9] tools/xl: add support for cache coloring configuration
2022-10-22 15:51 ` [PATCH v3 4/9] tools/xl: add support for cache coloring configuration Carlo Nonato
@ 2022-11-24 15:21 ` Anthony PERARD
2022-12-22 15:28 ` Carlo Nonato
0 siblings, 1 reply; 26+ messages in thread
From: Anthony PERARD @ 2022-11-24 15:21 UTC (permalink / raw)
To: Carlo Nonato
Cc: xen-devel, marco.solieri, andrea.bastoni, lucmiccio, Wei Liu,
Juergen Gross, Marco Solieri
On Sat, Oct 22, 2022 at 05:51:15PM +0200, Carlo Nonato wrote:
> diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
> index b2901e04cf..5f53cec8bf 100644
> --- a/docs/man/xl.cfg.5.pod.in
> +++ b/docs/man/xl.cfg.5.pod.in
> @@ -2880,6 +2880,16 @@ Currently, only the "sbsa_uart" model is supported for ARM.
>
> =back
>
> +=over 4
> +
> +=item B<colors=[ "COLORS_RANGE", "COLORS_RANGE", ...]>
Instead of COLORS_RANGE, maybe NUMBER_RANGE? Or just RANGE?
> +Specify the LLC color configuration for the guest. B<COLORS_RANGE> can be either
> +a single color value or a hypen-separated closed interval of colors
> +(such as "0-4").
Does "yellow-blue" works? Or "red-violet", to have a rainbow? :-)
So, "colors" as the name of a new configuration option isn't going to
work. To me, that would refer to VM managment, like maybe help to
categorise VM in some kind of tools, and not a part of the configuration
of the domain.
Could you invent a name that is more specific? Maybe "cache_colors" or
something, or "vcpu_cache_colors".
> +=back
> +
> =head3 x86
>
> =over 4
> diff --git a/tools/libs/light/libxl_create.c b/tools/libs/light/libxl_create.c
> index b9dd2deedf..94c511912c 100644
> --- a/tools/libs/light/libxl_create.c
> +++ b/tools/libs/light/libxl_create.c
> @@ -615,6 +615,7 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config,
> struct xs_permissions rwperm[1];
> struct xs_permissions noperm[1];
> xs_transaction_t t = 0;
> + DECLARE_HYPERCALL_BUFFER(unsigned int, colors);
>
> /* convenience aliases */
> libxl_domain_create_info *info = &d_config->c_info;
> @@ -676,6 +677,16 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config,
> goto out;
> }
>
> + if (d_config->b_info.num_colors) {
> + size_t bytes = sizeof(unsigned int) * d_config->b_info.num_colors;
> + colors = xc_hypercall_buffer_alloc(ctx->xch, colors, bytes);
Hypercall stuff is normally done in another library, libxenctrl (or
maybe others like libxenguest). Is there a reason to do that here?
> + memcpy(colors, d_config->b_info.colors, bytes);
> + set_xen_guest_handle(create.arch.colors, colors);
> + create.arch.num_colors = d_config->b_info.num_colors;
> + create.arch.from_guest = 1;
"arch" stuff is better dealt with in libxl__arch_domain_prepare_config().
(unless it isn't arch specific in the next revision of the series)
> + LOG(DEBUG, "Setup %u domain colors", d_config->b_info.num_colors);
> + }
> +
> for (;;) {
> uint32_t local_domid;
> bool recent;
> @@ -922,6 +933,7 @@ retry_transaction:
> rc = 0;
> out:
> if (t) xs_transaction_end(ctx->xsh, t, 1);
> + if (colors) xc_hypercall_buffer_free(ctx->xch, colors);
> return rc;
> }
>
> diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
> index d634f304cd..642173af1a 100644
> --- a/tools/libs/light/libxl_types.idl
> +++ b/tools/libs/light/libxl_types.idl
> @@ -557,6 +557,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
> ("ioports", Array(libxl_ioport_range, "num_ioports")),
> ("irqs", Array(uint32, "num_irqs")),
> ("iomem", Array(libxl_iomem_range, "num_iomem")),
> + ("colors", Array(uint32, "num_colors")),
So the colors is added to arch specific config in
xen_domctl_createdomain, maybe we should do the same here and move it to
the "arch_arm" struct. Or if that is declared common in hypervisor, then
it is file to leave it here.
Also, "colors" needs to be rename to something more specific.
> ("claim_mode", libxl_defbool),
> ("event_channels", uint32),
> ("kernel", string),
> diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
> index 1b5381cef0..e6b2c7acff 100644
> --- a/tools/xl/xl_parse.c
> +++ b/tools/xl/xl_parse.c
> @@ -1220,8 +1220,9 @@ void parse_config_data(const char *config_source,
> XLU_ConfigList *cpus, *vbds, *nics, *pcis, *cvfbs, *cpuids, *vtpms,
> *usbctrls, *usbdevs, *p9devs, *vdispls, *pvcallsifs_devs;
> XLU_ConfigList *channels, *ioports, *irqs, *iomem, *viridian, *dtdevs,
> - *mca_caps;
> - int num_ioports, num_irqs, num_iomem, num_cpus, num_viridian, num_mca_caps;
> + *mca_caps, *colors;
> + int num_ioports, num_irqs, num_iomem, num_cpus, num_viridian, num_mca_caps,
> + num_colors;
Please, add a new lines instead of increasing the number of declared
variable on a single line.
> int pci_power_mgmt = 0;
> int pci_msitranslate = 0;
> int pci_permissive = 0;
> @@ -1370,6 +1371,53 @@ void parse_config_data(const char *config_source,
> if (!xlu_cfg_get_long (config, "maxmem", &l, 0))
> b_info->max_memkb = l * 1024;
>
> + if (!xlu_cfg_get_list(config, "colors", &colors, &num_colors, 0)) {
> + int k, p, cur_index = 0;
> +
> + b_info->num_colors = 0;
> + /* Get number of colors based on ranges */
> + for (i = 0; i < num_colors; i++) {
> + uint32_t start = 0, end = 0;
> +
> + buf = xlu_cfg_get_listitem(colors, i);
> + if (!buf) {
> + fprintf(stderr,
> + "xl: Unable to get element %d in colors range list\n", i);
> + exit(1);
> + }
> +
> + if (sscanf(buf, "%u-%u", &start, &end) != 2) {
> + if (sscanf(buf, "%u", &start) != 1) {
I think you want %"SCNu32" instead of %u as both start and end are
uint32_t.
> + fprintf(stderr, "xl: Invalid color range: %s\n", buf);
> + exit(1);
> + }
> + end = start;
> + }
> + else if (start > end) {
Can you put the "else" on the same line as "}" ?
> + fprintf(stderr,
> + "xl: Start color is greater than end color: %s\n", buf);
> + exit(1);
> + }
> +
> + /* Check for overlaps */
> + for (k = start; k <= end; k++) {
> + for (p = 0; p < b_info->num_colors; p++) {
> + if (b_info->colors[p] == k) {
> + fprintf(stderr, "xl: Overlapped ranges not allowed\n");
Why is that an issue? Could overlap just been ignored?
> + exit(1);
> + }
> + }
> + }
> +
> + b_info->num_colors += (end - start) + 1;
> + b_info->colors = (uint32_t *)realloc(b_info->colors,
> + sizeof(*b_info->colors) * b_info->num_colors);
> +
> + for (k = start; k <= end; k++)
> + b_info->colors[cur_index++] = k;
This `b_info->colors` feels like it could be a bitmap like for "vcpus"
or other config that deal with ranges.
libxl has plenty of functions to deal with bitmap that xl can use,
starting with libxl_bitmap_alloc(), maybe it would make dealing with
cache color easier, like no need to check for overlaps, but there
doesn't seems to be a function to deal with a growing bitmap so one
would have to find the highest cache value before allocating the bitmap
are deal with realloc.
I guess using bitmap or not kind of depend of the interface with the
hypervisor, if it take a list of number, then a list of number is fine
here too.
Thanks,
--
Anthony PERARD
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 0/9] Arm cache coloring
2022-10-22 15:51 [PATCH v3 0/9] Arm cache coloring Carlo Nonato
` (8 preceding siblings ...)
2022-10-22 15:51 ` [PATCH v3 9/9] xen/arm: add cache coloring support for Xen Carlo Nonato
@ 2022-12-03 17:53 ` Julien Grall
9 siblings, 0 replies; 26+ messages in thread
From: Julien Grall @ 2022-12-03 17:53 UTC (permalink / raw)
To: Carlo Nonato, xen-devel
Cc: marco.solieri, andrea.bastoni, lucmiccio, Stefano Stabellini,
Bertrand Marquis, Volodymyr Babchuk, Andrew Cooper,
George Dunlap, Jan Beulich, Wei Liu, Anthony PERARD,
Juergen Gross
Hi Carlo,
Just in case you are waiting on my review before sending a new version.
Given that...
On 22/10/2022 16:51, Carlo Nonato wrote:
> Shared caches in multi-core CPU architectures represent a problem for
> predictability of memory access latency. This jeopardizes applicability
> of many Arm platform in real-time critical and mixed-criticality
> scenarios. We introduce support for cache partitioning with page
> coloring, a transparent software technique that enables isolation
> between domains and Xen, and thus avoids cache interference.
>
> When creating a domain, a simple syntax (e.g. `0-3` or `4-11`) allows
> the user to define assignments of cache partitions ids, called colors,
> where assigning different colors guarantees no mutual eviction on cache
> will ever happen. This instructs the Xen memory allocator to provide
> the i-th color assignee only with pages that maps to color i, i.e. that
> are indexed in the i-th cache partition.
>
> The proposed implementation supports the dom0less feature.
> The proposed implementation doesn't support the static-mem feature.
> The solution has been tested in several scenarios, including Xilinx Zynq
> MPSoCs.
>
> Commits structure:
> - [1-3] Coloring initialization, cache layout auto-probing and coloring
> data for domains.
> - [4-5] xl and Device Tree support for coloring.
> - [6] New page allocator for domain memory that implement the cache
> coloring mechanism.
> - [7-9] Coloring support for Xen.
>
> Global changes in v3:
> - fixed a compilation error because of a forgotten "\"
> - replaced some #ifdef with if ( IS_ENABLED )
> - other minor changes (docs, typos, variable types, style, etc.)
> - better acknowledged Luca Miccio as the original author
> - removed #8 since the bootmodule address and size can be replaced without
> the need of this particular revert
> - removed #9 since it wasn't a clean revert and thanks to Julien things can
> be done in a smarter way sticking with map_pages_to_xen() (see new #9)
>
> Open points:
> - The allocator proposed in #6 works only with order-0 pages and inserts
> them in a sorted list using a linear search. This behavior can be slow if
> large amount of memory is given to it, so the user is warned in the
> documentation for that.
> In a following patch, that I'm going to send separately, a simple buddy
> allocator that indexes pages by color is presented. It can serve higher
> order pages and doesn't need the linear search. Unfortunately, it has
> some flaws that I will discuss there.
> - I will address the latest v2 comments from Julien in v4
... some my comments have not been handled, I am not planning to review
this version. If there is small parts you want me to have a look before
you send a new version.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 4/9] tools/xl: add support for cache coloring configuration
2022-11-24 15:21 ` Anthony PERARD
@ 2022-12-22 15:28 ` Carlo Nonato
2023-01-06 16:32 ` Anthony PERARD
0 siblings, 1 reply; 26+ messages in thread
From: Carlo Nonato @ 2022-12-22 15:28 UTC (permalink / raw)
To: Anthony PERARD
Cc: xen-devel, marco.solieri, andrea.bastoni, lucmiccio, Wei Liu,
Juergen Gross, Marco Solieri
Hi Anthony,
On Thu, Nov 24, 2022 at 4:21 PM Anthony PERARD
<anthony.perard@citrix.com> wrote:
>
> On Sat, Oct 22, 2022 at 05:51:15PM +0200, Carlo Nonato wrote:
> > diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
> > index b2901e04cf..5f53cec8bf 100644
> > --- a/docs/man/xl.cfg.5.pod.in
> > +++ b/docs/man/xl.cfg.5.pod.in
> > @@ -2880,6 +2880,16 @@ Currently, only the "sbsa_uart" model is supported for ARM.
> >
> > =back
> >
> > +=over 4
> > +
> > +=item B<colors=[ "COLORS_RANGE", "COLORS_RANGE", ...]>
>
> Instead of COLORS_RANGE, maybe NUMBER_RANGE? Or just RANGE?
RANGE seems good.
> > +Specify the LLC color configuration for the guest. B<COLORS_RANGE> can be either
> > +a single color value or a hypen-separated closed interval of colors
> > +(such as "0-4").
>
> Does "yellow-blue" works? Or "red-violet", to have a rainbow? :-)
>
> So, "colors" as the name of a new configuration option isn't going to
> work. To me, that would refer to VM managment, like maybe help to
> categorise VM in some kind of tools, and not a part of the configuration
> of the domain.
>
> Could you invent a name that is more specific? Maybe "cache_colors" or
> something, or "vcpu_cache_colors".
What about llc_colors? LLC stands for Last Level Cache and I'm trying to use
it everywhere else since it's what is really implemented (not any cache is
colored, just the last level) and it's shorter than cache_colors.
> > +=back
> > +
> > =head3 x86
> >
> > =over 4
> > diff --git a/tools/libs/light/libxl_create.c b/tools/libs/light/libxl_create.c
> > index b9dd2deedf..94c511912c 100644
> > --- a/tools/libs/light/libxl_create.c
> > +++ b/tools/libs/light/libxl_create.c
> > @@ -615,6 +615,7 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config,
> > struct xs_permissions rwperm[1];
> > struct xs_permissions noperm[1];
> > xs_transaction_t t = 0;
> > + DECLARE_HYPERCALL_BUFFER(unsigned int, colors);
> >
> > /* convenience aliases */
> > libxl_domain_create_info *info = &d_config->c_info;
> > @@ -676,6 +677,16 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config,
> > goto out;
> > }
> >
> > + if (d_config->b_info.num_colors) {
> > + size_t bytes = sizeof(unsigned int) * d_config->b_info.num_colors;
> > + colors = xc_hypercall_buffer_alloc(ctx->xch, colors, bytes);
>
> Hypercall stuff is normally done in another library, libxenctrl (or
> maybe others like libxenguest). Is there a reason to do that here?
I moved this in xc_domain_create().
> > + memcpy(colors, d_config->b_info.colors, bytes);
> > + set_xen_guest_handle(create.arch.colors, colors);
> > + create.arch.num_colors = d_config->b_info.num_colors;
> > + create.arch.from_guest = 1;
>
> "arch" stuff is better dealt with in libxl__arch_domain_prepare_config().
> (unless it isn't arch specific in the next revision of the series)
Yes, removing arch specific parts is the current way of implementing it.
> > + LOG(DEBUG, "Setup %u domain colors", d_config->b_info.num_colors);
>
>
>
> > + }
> > +
> > for (;;) {
> > uint32_t local_domid;
> > bool recent;
> > @@ -922,6 +933,7 @@ retry_transaction:
> > rc = 0;
> > out:
> > if (t) xs_transaction_end(ctx->xsh, t, 1);
> > + if (colors) xc_hypercall_buffer_free(ctx->xch, colors);
> > return rc;
> > }
> >
> > diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
> > index d634f304cd..642173af1a 100644
> > --- a/tools/libs/light/libxl_types.idl
> > +++ b/tools/libs/light/libxl_types.idl
> > @@ -557,6 +557,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
> > ("ioports", Array(libxl_ioport_range, "num_ioports")),
> > ("irqs", Array(uint32, "num_irqs")),
> > ("iomem", Array(libxl_iomem_range, "num_iomem")),
> > + ("colors", Array(uint32, "num_colors")),
>
> So the colors is added to arch specific config in
> xen_domctl_createdomain, maybe we should do the same here and move it to
> the "arch_arm" struct. Or if that is declared common in hypervisor, then
> it is file to leave it here.
Yes, it will be declared in common.
> Also, "colors" needs to be rename to something more specific.
>
> > ("claim_mode", libxl_defbool),
> > ("event_channels", uint32),
> > ("kernel", string),
> > diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
> > index 1b5381cef0..e6b2c7acff 100644
> > --- a/tools/xl/xl_parse.c
> > +++ b/tools/xl/xl_parse.c
> > @@ -1220,8 +1220,9 @@ void parse_config_data(const char *config_source,
> > XLU_ConfigList *cpus, *vbds, *nics, *pcis, *cvfbs, *cpuids, *vtpms,
> > *usbctrls, *usbdevs, *p9devs, *vdispls, *pvcallsifs_devs;
> > XLU_ConfigList *channels, *ioports, *irqs, *iomem, *viridian, *dtdevs,
> > - *mca_caps;
> > - int num_ioports, num_irqs, num_iomem, num_cpus, num_viridian, num_mca_caps;
> > + *mca_caps, *colors;
> > + int num_ioports, num_irqs, num_iomem, num_cpus, num_viridian, num_mca_caps,
> > + num_colors;
>
> Please, add a new lines instead of increasing the number of declared
> variable on a single line.
>
> > int pci_power_mgmt = 0;
> > int pci_msitranslate = 0;
> > int pci_permissive = 0;
> > @@ -1370,6 +1371,53 @@ void parse_config_data(const char *config_source,
> > if (!xlu_cfg_get_long (config, "maxmem", &l, 0))
> > b_info->max_memkb = l * 1024;
> >
> > + if (!xlu_cfg_get_list(config, "colors", &colors, &num_colors, 0)) {
> > + int k, p, cur_index = 0;
> > +
> > + b_info->num_colors = 0;
> > + /* Get number of colors based on ranges */
> > + for (i = 0; i < num_colors; i++) {
> > + uint32_t start = 0, end = 0;
> > +
> > + buf = xlu_cfg_get_listitem(colors, i);
> > + if (!buf) {
> > + fprintf(stderr,
> > + "xl: Unable to get element %d in colors range list\n", i);
> > + exit(1);
> > + }
> > +
> > + if (sscanf(buf, "%u-%u", &start, &end) != 2) {
> > + if (sscanf(buf, "%u", &start) != 1) {
>
> I think you want %"SCNu32" instead of %u as both start and end are
> uint32_t.
Ok.
> > + fprintf(stderr, "xl: Invalid color range: %s\n", buf);
> > + exit(1);
> > + }
> > + end = start;
> > + }
> > + else if (start > end) {
>
> Can you put the "else" on the same line as "}" ?
Yep.
> > + fprintf(stderr,
> > + "xl: Start color is greater than end color: %s\n", buf);
> > + exit(1);
> > + }
> > +
> > + /* Check for overlaps */
> > + for (k = start; k <= end; k++) {
> > + for (p = 0; p < b_info->num_colors; p++) {
> > + if (b_info->colors[p] == k) {
> > + fprintf(stderr, "xl: Overlapped ranges not allowed\n");
>
> Why is that an issue? Could overlap just been ignored?
That requirement comes from the hypervisor. It assumes a sorted array without
repeated elements for simplicity. The hypervisor checks that again indeed
(it does that in v4, at least) so yes, it's a bit useless. I'll drop this.
> > + exit(1);
> > + }
> > + }
> > + }
> > +
> > + b_info->num_colors += (end - start) + 1;
> > + b_info->colors = (uint32_t *)realloc(b_info->colors,
> > + sizeof(*b_info->colors) * b_info->num_colors);
> > +
> > + for (k = start; k <= end; k++)
> > + b_info->colors[cur_index++] = k;
>
> This `b_info->colors` feels like it could be a bitmap like for "vcpus"
> or other config that deal with ranges.
>
> libxl has plenty of functions to deal with bitmap that xl can use,
> starting with libxl_bitmap_alloc(), maybe it would make dealing with
> cache color easier, like no need to check for overlaps, but there
> doesn't seems to be a function to deal with a growing bitmap so one
> would have to find the highest cache value before allocating the bitmap
> are deal with realloc.
>
> I guess using bitmap or not kind of depend of the interface with the
> hypervisor, if it take a list of number, then a list of number is fine
> here too.
I discarded bitmaps because it's not that easy to iterate over the index of
set bits and because they imposed, in the way we used them, a fixed number
of colors. At that time I wasn't really thinking of having a dynamic bitmap.
For now I will leave things as they are (using plain arrays) because it
would require other changes in the hypervisor, but this can be something
to do in the future, at least just for the passage of data between toolstack
and Xen.
> Thanks,
>
> --
> Anthony PERARD
Thank
On Thu, Nov 24, 2022 at 4:21 PM Anthony PERARD
<anthony.perard@citrix.com> wrote:
>
> On Sat, Oct 22, 2022 at 05:51:15PM +0200, Carlo Nonato wrote:
> > diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
> > index b2901e04cf..5f53cec8bf 100644
> > --- a/docs/man/xl.cfg.5.pod.in
> > +++ b/docs/man/xl.cfg.5.pod.in
> > @@ -2880,6 +2880,16 @@ Currently, only the "sbsa_uart" model is supported for ARM.
> >
> > =back
> >
> > +=over 4
> > +
> > +=item B<colors=[ "COLORS_RANGE", "COLORS_RANGE", ...]>
>
> Instead of COLORS_RANGE, maybe NUMBER_RANGE? Or just RANGE?
>
> > +Specify the LLC color configuration for the guest. B<COLORS_RANGE> can be either
> > +a single color value or a hypen-separated closed interval of colors
> > +(such as "0-4").
>
> Does "yellow-blue" works? Or "red-violet", to have a rainbow? :-)
>
> So, "colors" as the name of a new configuration option isn't going to
> work. To me, that would refer to VM managment, like maybe help to
> categorise VM in some kind of tools, and not a part of the configuration
> of the domain.
>
> Could you invent a name that is more specific? Maybe "cache_colors" or
> something, or "vcpu_cache_colors".
>
> > +=back
> > +
> > =head3 x86
> >
> > =over 4
> > diff --git a/tools/libs/light/libxl_create.c b/tools/libs/light/libxl_create.c
> > index b9dd2deedf..94c511912c 100644
> > --- a/tools/libs/light/libxl_create.c
> > +++ b/tools/libs/light/libxl_create.c
> > @@ -615,6 +615,7 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config,
> > struct xs_permissions rwperm[1];
> > struct xs_permissions noperm[1];
> > xs_transaction_t t = 0;
> > + DECLARE_HYPERCALL_BUFFER(unsigned int, colors);
> >
> > /* convenience aliases */
> > libxl_domain_create_info *info = &d_config->c_info;
> > @@ -676,6 +677,16 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config,
> > goto out;
> > }
> >
> > + if (d_config->b_info.num_colors) {
> > + size_t bytes = sizeof(unsigned int) * d_config->b_info.num_colors;
> > + colors = xc_hypercall_buffer_alloc(ctx->xch, colors, bytes);
>
> Hypercall stuff is normally done in another library, libxenctrl (or
> maybe others like libxenguest). Is there a reason to do that here?
>
> > + memcpy(colors, d_config->b_info.colors, bytes);
> > + set_xen_guest_handle(create.arch.colors, colors);
> > + create.arch.num_colors = d_config->b_info.num_colors;
> > + create.arch.from_guest = 1;
>
> "arch" stuff is better dealt with in libxl__arch_domain_prepare_config().
> (unless it isn't arch specific in the next revision of the series)
>
> > + LOG(DEBUG, "Setup %u domain colors", d_config->b_info.num_colors);
>
>
>
> > + }
> > +
> > for (;;) {
> > uint32_t local_domid;
> > bool recent;
> > @@ -922,6 +933,7 @@ retry_transaction:
> > rc = 0;
> > out:
> > if (t) xs_transaction_end(ctx->xsh, t, 1);
> > + if (colors) xc_hypercall_buffer_free(ctx->xch, colors);
> > return rc;
> > }
> >
> > diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
> > index d634f304cd..642173af1a 100644
> > --- a/tools/libs/light/libxl_types.idl
> > +++ b/tools/libs/light/libxl_types.idl
> > @@ -557,6 +557,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
> > ("ioports", Array(libxl_ioport_range, "num_ioports")),
> > ("irqs", Array(uint32, "num_irqs")),
> > ("iomem", Array(libxl_iomem_range, "num_iomem")),
> > + ("colors", Array(uint32, "num_colors")),
>
> So the colors is added to arch specific config in
> xen_domctl_createdomain, maybe we should do the same here and move it to
> the "arch_arm" struct. Or if that is declared common in hypervisor, then
> it is file to leave it here.
>
> Also, "colors" needs to be rename to something more specific.
>
> > ("claim_mode", libxl_defbool),
> > ("event_channels", uint32),
> > ("kernel", string),
> > diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
> > index 1b5381cef0..e6b2c7acff 100644
> > --- a/tools/xl/xl_parse.c
> > +++ b/tools/xl/xl_parse.c
> > @@ -1220,8 +1220,9 @@ void parse_config_data(const char *config_source,
> > XLU_ConfigList *cpus, *vbds, *nics, *pcis, *cvfbs, *cpuids, *vtpms,
> > *usbctrls, *usbdevs, *p9devs, *vdispls, *pvcallsifs_devs;
> > XLU_ConfigList *channels, *ioports, *irqs, *iomem, *viridian, *dtdevs,
> > - *mca_caps;
> > - int num_ioports, num_irqs, num_iomem, num_cpus, num_viridian, num_mca_caps;
> > + *mca_caps, *colors;
> > + int num_ioports, num_irqs, num_iomem, num_cpus, num_viridian, num_mca_caps,
> > + num_colors;
>
> Please, add a new lines instead of increasing the number of declared
> variable on a single line.
>
> > int pci_power_mgmt = 0;
> > int pci_msitranslate = 0;
> > int pci_permissive = 0;
> > @@ -1370,6 +1371,53 @@ void parse_config_data(const char *config_source,
> > if (!xlu_cfg_get_long (config, "maxmem", &l, 0))
> > b_info->max_memkb = l * 1024;
> >
> > + if (!xlu_cfg_get_list(config, "colors", &colors, &num_colors, 0)) {
> > + int k, p, cur_index = 0;
> > +
> > + b_info->num_colors = 0;
> > + /* Get number of colors based on ranges */
> > + for (i = 0; i < num_colors; i++) {
> > + uint32_t start = 0, end = 0;
> > +
> > + buf = xlu_cfg_get_listitem(colors, i);
> > + if (!buf) {
> > + fprintf(stderr,
> > + "xl: Unable to get element %d in colors range list\n", i);
> > + exit(1);
> > + }
> > +
> > + if (sscanf(buf, "%u-%u", &start, &end) != 2) {
> > + if (sscanf(buf, "%u", &start) != 1) {
>
> I think you want %"SCNu32" instead of %u as both start and end are
> uint32_t.
>
> > + fprintf(stderr, "xl: Invalid color range: %s\n", buf);
> > + exit(1);
> > + }
> > + end = start;
> > + }
> > + else if (start > end) {
>
> Can you put the "else" on the same line as "}" ?
> > + fprintf(stderr,
> > + "xl: Start color is greater than end color: %s\n", buf);
> > + exit(1);
> > + }
> > +
> > + /* Check for overlaps */
> > + for (k = start; k <= end; k++) {
> > + for (p = 0; p < b_info->num_colors; p++) {
> > + if (b_info->colors[p] == k) {
> > + fprintf(stderr, "xl: Overlapped ranges not allowed\n");
>
> Why is that an issue? Could overlap just been ignored?
>
> > + exit(1);
> > + }
> > + }
> > + }
> > +
> > + b_info->num_colors += (end - start) + 1;
> > + b_info->colors = (uint32_t *)realloc(b_info->colors,
> > + sizeof(*b_info->colors) * b_info->num_colors);
> > +
> > + for (k = start; k <= end; k++)
> > + b_info->colors[cur_index++] = k;
>
> This `b_info->colors` feels like it could be a bitmap like for "vcpus"
> or other config that deal with ranges.
>
> libxl has plenty of functions to deal with bitmap that xl can use,
> starting with libxl_bitmap_alloc(), maybe it would make dealing with
> cache color easier, like no need to check for overlaps, but there
> doesn't seems to be a function to deal with a growing bitmap so one
> would have to find the highest cache value before allocating the bitmap
> are deal with realloc.
>
> I guess using bitmap or not kind of depend of the interface with the
> hypervisor, if it take a list of number, then a list of number is fine
> here too.
>
>
> Thanks,
>
> --
> Anthony PERARD
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 4/9] tools/xl: add support for cache coloring configuration
2022-12-22 15:28 ` Carlo Nonato
@ 2023-01-06 16:32 ` Anthony PERARD
0 siblings, 0 replies; 26+ messages in thread
From: Anthony PERARD @ 2023-01-06 16:32 UTC (permalink / raw)
To: Carlo Nonato
Cc: xen-devel, marco.solieri, andrea.bastoni, lucmiccio, Wei Liu,
Juergen Gross, Marco Solieri
On Thu, Dec 22, 2022 at 04:28:56PM +0100, Carlo Nonato wrote:
> > Could you invent a name that is more specific? Maybe "cache_colors" or
> > something, or "vcpu_cache_colors".
>
> What about llc_colors? LLC stands for Last Level Cache and I'm trying to use
> it everywhere else since it's what is really implemented (not any cache is
> colored, just the last level) and it's shorter than cache_colors.
"llc_colors" sounds good.
Cheers,
--
Anthony PERARD
^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2023-01-06 16:32 UTC | newest]
Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-22 15:51 [PATCH v3 0/9] Arm cache coloring Carlo Nonato
2022-10-22 15:51 ` [PATCH v3 1/9] xen/arm: add cache coloring initialization Carlo Nonato
2022-10-22 15:51 ` [PATCH v3 2/9] xen/arm: add cache coloring initialization for domains Carlo Nonato
2022-10-25 12:07 ` Carlo Nonato
2022-11-21 14:50 ` Carlo Nonato
2022-11-21 15:14 ` Jan Beulich
2022-11-21 16:23 ` Carlo Nonato
2022-11-21 16:40 ` Jan Beulich
2022-11-22 20:25 ` Julien Grall
2022-11-23 9:41 ` Jan Beulich
2022-11-23 9:48 ` Jan Beulich
2022-10-22 15:51 ` [PATCH v3 3/9] xen/arm: dump cache colors in domain info debug-key Carlo Nonato
2022-10-22 15:51 ` [PATCH v3 4/9] tools/xl: add support for cache coloring configuration Carlo Nonato
2022-11-24 15:21 ` Anthony PERARD
2022-12-22 15:28 ` Carlo Nonato
2023-01-06 16:32 ` Anthony PERARD
2022-10-22 15:51 ` [PATCH v3 5/9] xen/arm: add support for cache coloring configuration via device-tree Carlo Nonato
2022-10-22 15:51 ` [PATCH v3 6/9] xen/common: add cache coloring allocator for domains Carlo Nonato
2022-10-25 12:08 ` Carlo Nonato
2022-11-10 16:47 ` Jan Beulich
2022-11-14 15:04 ` Carlo Nonato
2022-11-15 7:53 ` Jan Beulich
2022-10-22 15:51 ` [PATCH v3 7/9] Revert "xen/arm: Remove unused BOOT_RELOC_VIRT_START" Carlo Nonato
2022-10-22 15:51 ` [PATCH v3 8/9] xen/arm: add Xen cache colors command line parameter Carlo Nonato
2022-10-22 15:51 ` [PATCH v3 9/9] xen/arm: add cache coloring support for Xen Carlo Nonato
2022-12-03 17:53 ` [PATCH v3 0/9] Arm cache coloring Julien Grall
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.