Re: [PATCH 01/12] xen/arm: add cache coloring initialization

[Julien Grall <julien@xxxxxxx>]

Hi Carlo,

On 26/08/2022 13:51, Carlo Nonato wrote:
> 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.
>
> Signed-off-by: Carlo Nonato <carlo.nonato@xxxxxxxxxxxxxxx>
> Signed-off-by: Marco Solieri <marco.solieri@xxxxxxxxxxxxxxx>
> ---
>   docs/misc/arm/cache-coloring.rst     | 112 ++++++++++++++
>   docs/misc/xen-command-line.pandoc    |  22 +++
>   xen/arch/arm/Kconfig                 |  16 ++
>   xen/arch/arm/Makefile                |   1 +
>   xen/arch/arm/coloring.c              | 222 +++++++++++++++++++++++++++
>   xen/arch/arm/include/asm/coloring.h  |  31 ++++
>   xen/arch/arm/include/asm/processor.h |  16 ++
>   xen/arch/arm/setup.c                 |   8 +
>   8 files changed, 428 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..c7adcb0f1f
> --- /dev/null
> +++ b/docs/misc/arm/cache-coloring.rst
> @@ -0,0 +1,112 @@
> +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 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
> +a 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 to 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".
> \ No newline at end of file
> diff --git a/docs/misc/xen-command-line.pandoc 
> b/docs/misc/xen-command-line.pandoc
> index 9a79385a37..910ebeb2eb 100644
> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -889,6 +889,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>`
>   
> @@ -1631,6 +1639,20 @@ 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.
> +
>   ### 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 33e004d702..8acff9682c 100644
> --- a/xen/arch/arm/Kconfig
> +++ b/xen/arch/arm/Kconfig
> @@ -131,6 +131,22 @@ 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
> +       default n
> +       depends on ARM_64
> +
> +config MAX_CACHE_COLORS
> +       int "Maximum number of cache colors"
> +       default 128
> +       range 0 65536
> +       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. Note that if, at any time, a color configuration with 
> more
> +         colors than the maximum will be employed an error will be 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
The Makefile should be ordered alphabetically based on the filename. So 
please move this to correct position.
>   
>   extra-y += xen.lds
>   
> diff --git a/xen/arch/arm/coloring.c b/xen/arch/arm/coloring.c
> new file mode 100644
> index 0000000000..c010ebc01b
> --- /dev/null
> +++ b/xen/arch/arm/coloring.c
> @@ -0,0 +1,222 @@
> +/*
> + * xen/arch/arm/coloring.c
> + *
> + * Coloring support for ARM
> + *
> + * Copyright (C) 2019 Xilinx Inc.
> + *
> + * Authors:
> + *    Luca Miccio <lucmiccio@xxxxxxxxx>
> + *    Carlo Nonato <carlo.nonato@xxxxxxxxxxxxxxx>
> + *
> + * 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/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 a LLC way */
> +static unsigned int llc_way_size;
> +/* Number of colors available in the LLC */
> +static unsigned int max_colors;
> +/* Mask to retrieve coloring relevant bits */
> +static uint64_t addr_col_mask;
IIUC, this is meant to be applied on a host physical address. If so, 
please use 'paddr_t' rather than uint64_t. Same below.
> +
> +#define addr_to_color(addr) (((addr) & addr_col_mask) >> PAGE_SHIFT)
> +#define addr_set_color(addr, color) (((addr) & ~addr_col_mask) \
> +                                     | ((color) << PAGE_SHIFT))
I don't see any use of addr_to_color() and addr_set_color() below. So it 
is not clear to me what is the type of 'color'. This may need to be 
casted to (paddr_t).
But as the helpers are quite small, it would be better to introduce the 
macros where they are used.
> +
> +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 >= CONFIG_MAX_CACHE_COLORS )
In theory, *num_colors + end - start could overflow. I would also check 
that given you are sanity checking the values.
Lastly, may I ask to surround *num_colors + end - start with ()? This 
would make easier to read.
> +                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 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)
> +{
> +    int i;
i should not be signed. So please use "unsigned int".

> +
> +    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;
I find a bit odd that we return 0 if 'colors' is NULL. In what situation 
do you expect 'colors' to be NULL? Is this a programming error?
> + > +    for ( i = 0; i < max_colors; i++ )
> +        colors[i] = i;
Coding style: Newline please.

> +    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;
> +    }
> +
> +    max_colors = llc_way_size / PAGE_SIZE;
> +    /* The maximum number of colors must be a power of 2 in order to correctly
> +       map colors to bits of an address. */
> +    ASSERT((max_colors & (max_colors - 1)) == 0);
> +    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..dd7eff5f07
> --- /dev/null
> +++ b/xen/arch/arm/include/asm/coloring.h
> @@ -0,0 +1,31 @@
> +/*
> + * xen/arm/include/asm/coloring.h
> + *
> + * Coloring support for ARM
> + *
> + * Copyright (C) 2019 Xilinx Inc.
> + *
> + * Authors:
> + *    Luca Miccio <lucmiccio@xxxxxxxxx>
> + *    Carlo Nonato <carlo.nonato@xxxxxxxxxxxxxxx>
> + *
> + * 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__
> +
> +#include <xen/init.h>
> +
> +bool __init coloring_init(void);
> +
> +#endif /* !__ASM_ARM_COLORING_H__ */
Missing emags magic as you added in coloring.c.

> 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
Please use ULL here otherwise someone using MASK << SHIFT will have the 
expected result.
My preference would be that all the MASK should have the same type. So 
this is easier to spot any mistake.
> +
> +/* 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 500307edc0..c02f21c0e6 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -53,6 +53,9 @@
>   #include <asm/setup.h>
>   #include <xsm/xsm.h>
>   #include <asm/acpi.h>
> +#ifdef CONFIG_CACHE_COLORING
> +#include <asm/coloring.h>
> +#endif
I would rather prefer if the #ifdef are moved in the header. So...

>   
>   struct bootinfo __initdata bootinfo;
>   
> @@ -930,6 +933,11 @@ void __init start_xen(unsigned long boot_phys_offset,
>       printk("Command line: %s\n", cmdline);
>       cmdline_parse(cmdline);
>   
> +#ifdef CONFIG_CACHE_COLORING
> +    if ( !coloring_init() )
... a stub for coloring_init() is provided and we avoid sprinkling the 
common code with #ifdef.
> +        panic("Xen Coloring support: setup failed\n");
> +#endif
> +
>       setup_mm();
>   
>       /* Parse the ACPI tables for possible boot-time configuration */
Cheers,

--
Julien Grall