Re: [PATCH 04/36] xen/arm: add parsing function for cache coloring configuration

[Julien Grall <julien@xxxxxxx>]

Hi,

On 04/03/2022 17:46, Marco Solieri wrote:
> From: Luca Miccio <lucmiccio@xxxxxxxxx>
>
> Add three new bootargs allowing configuration of cache coloring support
> for Xen:
I would prefer if documentation of each command line is part of the 
patch introducing them. This would help understanding some of the 
parameters.
> - way_size: The size of a LLC way in bytes. This value is mainly used
>    to calculate the maximum available colors on the platform.
We should only add command line option when they are a strong use case. 
In documentation, you wrote that someone may want to overwrite the way 
size for "specific needs".
Can you explain what would be those needs?

> - dom0_colors: The coloring configuration for Dom0, which also acts as
>    default configuration for any DomU without an explicit configuration.
> - xen_colors: The coloring configuration for the Xen hypervisor itself.
>
> A cache coloring configuration consists of a selection of colors to be
> assigned to a VM or to the hypervisor. It is represented by a set of
> ranges. Add a common function that parses a string with a
> comma-separated set of hyphen-separated ranges like "0-7,15-16" and
> returns both: the number of chosen colors, and an array containing their
> ids.
> Currently we support platforms with up to 128 colors.
Is there any reason this value is hardcoded in Xen rather than part of 
the Kconfig?
> Signed-off-by: Luca Miccio <lucmiccio@xxxxxxxxx>
> Signed-off-by: Marco Solieri <marco.solieri@xxxxxxxxxxxxxxx>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxx>
> ---
>   xen/arch/arm/Kconfig                |   5 ++
>   xen/arch/arm/Makefile               |   2 +-
>   xen/arch/arm/coloring.c             | 131 ++++++++++++++++++++++++++++
>   xen/arch/arm/include/asm/coloring.h |  28 ++++++
>   4 files changed, 165 insertions(+), 1 deletion(-)
>   create mode 100644 xen/arch/arm/coloring.c
>   create mode 100644 xen/arch/arm/include/asm/coloring.h
>
> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> index ecfa6822e4..f0f999d172 100644
> --- a/xen/arch/arm/Kconfig
> +++ b/xen/arch/arm/Kconfig
> @@ -97,6 +97,11 @@ config HARDEN_BRANCH_PREDICTOR
>   
>   	  If unsure, say Y.
>   
> +config COLORING
> +       bool "L2 cache coloring"
> +       default n
This wants to be gated with EXPERT for time-being. SUPPORT.MD woudl
Furthermore, I think this wants to be gated with EXPERT for the time-being.

> +       depends on ARM_64
Why is this limited to arm64?

> +
>   config TEE
>         bool "Enable TEE mediators support (UNSUPPORTED)" if UNSUPPORTED
>         default n
> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> index c993ce72a3..581896a528 100644
> --- a/xen/arch/arm/Makefile
> +++ b/xen/arch/arm/Makefile
> @@ -66,7 +66,7 @@ obj-$(CONFIG_SBSA_VUART_CONSOLE) += vpl011.o
>   obj-y += vsmc.o
>   obj-y += vpsci.o
>   obj-y += vuart.o
> -
> +obj-$(CONFIG_COLORING) += coloring.o
Please keep the newline before extra-y. The file are meant to be ordered 
alphabetically. So this should be inserted in the correct position.
>   extra-y += xen.lds
>   
>   #obj-bin-y += ....o
> diff --git a/xen/arch/arm/coloring.c b/xen/arch/arm/coloring.c
> new file mode 100644
> index 0000000000..8f1cff6efb
> --- /dev/null
> +++ b/xen/arch/arm/coloring.c
> @@ -0,0 +1,131 @@
> +/*
> + * xen/arch/arm/coloring.c
> + *
> + * Coloring support for ARM
> + *
> + * Copyright (C) 2019 Xilinx Inc.
> + *
> + * Authors:
> + *    Luca Miccio <lucmiccio@xxxxxxxxx>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +#include <xen/init.h>
> +#include <xen/types.h>
> +#include <xen/lib.h>
> +#include <xen/errno.h>
> +#include <xen/param.h>
> +#include <asm/coloring.h>
The includes should be ordered so <xen/...> are first, then <asm/...>.
They are also ordered alphabetically within their own category.

> +
> +/* Number of color(s) assigned to Xen */
> +static uint32_t xen_col_num;
> +/* Coloring configuration of Xen as bitmask */
> +static uint32_t xen_col_mask[MAX_COLORS_CELLS];
Xen provides helpers to create and use bitmaps (see 
include/xen/bitmap.h). Can you use?
> +
> +/* Number of color(s) assigned to Dom0 */
> +static uint32_t dom0_col_num;
> +/* Coloring configuration of Dom0 as bitmask */
> +static uint32_t dom0_col_mask[MAX_COLORS_CELLS];
> +
> +static uint64_t way_size;
> +
> +/*************************
> + * PARSING COLORING BOOTARGS
> + */
> +
> +/*
> + * Parse the coloring configuration given in the buf string, following the
> + * syntax below, and store the number of colors and a corresponding mask in
> + * the last two given pointers.
> + *
> + * COLOR_CONFIGURATION ::= RANGE,...,RANGE
> + * RANGE               ::= COLOR-COLOR
> + *
> + * Example: "2-6,15-16" represents the set of colors: 2,3,4,5,6,15,16.
> + */
> +static int parse_color_config(
> +    const char *buf, uint32_t *col_mask, uint32_t *col_num)

Coding style. We usually declarate paremeters on the same line as the 
function name. If they can't fit on the same line, then we split in two 
with the parameter aligned to the first paremeter.
> +{
> +    int start, end, i;
AFAICT, none of the 3 variables will store negative values. So can they 
be unsigned?
> +    const char* s = buf;
> +    unsigned int offset;
> +
> +    if ( !col_mask || !col_num )
> +        return -EINVAL;
> +
> +    *col_num = 0;
> +    for ( i = 0; i < MAX_COLORS_CELLS; i++ )
> +        col_mask[i] = 0;
dom0_col_mask and xen_col_mask are already zeroed. I would also expect 
the same for dynamically allocated bitmask. So can this be dropped?
> +
> +    while ( *s != '\0' )
> +    {
> +        if ( *s != ',' )
> +        {
> +            start = simple_strtoul(s, &s, 0);
> +
> +            /* Ranges are hyphen-separated */
> +            if ( *s != '-' )
> +                goto fail;
> +            s++;
> +
> +            end = simple_strtoul(s, &s, 0);
> +
> +            for ( i = start; i <= end; i++ )
> +            {
> +                offset = i / 32;
> +                if ( offset > MAX_COLORS_CELLS )
> +                    goto fail;
> +
> +                if ( !(col_mask[offset] & (1 << i % 32)) )
> +                    *col_num += 1;
> +                col_mask[offset] |= (1 << i % 32);
> +            }
> +        }
> +        else
> +            s++;
> +    }
> +
> +    return *s ? -EINVAL : 0;
> +fail:
> +    return -EINVAL;
> +}
> +
> +static int __init parse_way_size(const char *s)
> +{
> +    way_size = simple_strtoull(s, &s, 0);
> +
> +    return *s ? -EINVAL : 0;
> +}
> +custom_param("way_size", parse_way_size);
> +
> +static int __init parse_dom0_colors(const char *s)
> +{
> +    return parse_color_config(s, dom0_col_mask, &dom0_col_num);
> +}
> +custom_param("dom0_colors", parse_dom0_colors);
> +
> +static int __init parse_xen_colors(const char *s)
> +{
> +    return parse_color_config(s, xen_col_mask, &xen_col_num);
> +}
> +custom_param("xen_colors", parse_xen_colors);
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * tab-width: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/arch/arm/include/asm/coloring.h 
> b/xen/arch/arm/include/asm/coloring.h
> new file mode 100644
> index 0000000000..60958d1244
> --- /dev/null
> +++ b/xen/arch/arm/include/asm/coloring.h
> @@ -0,0 +1,28 @@
> +/*
> + * xen/arm/include/asm/coloring.h
> + *
> + * Coloring support for ARM
> + *
> + * Copyright (C) 2019 Xilinx Inc.
> + *
> + * Authors:
> + *    Luca Miccio <lucmiccio@xxxxxxxxx>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +#ifndef __ASM_ARM_COLORING_H__
> +#define __ASM_ARM_COLORING_H__
> +
> +#define MAX_COLORS_CELLS 4
> +
> +#endif /* !__ASM_ARM_COLORING_H__ */
Cheers,

--
Julien Grall