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

[Luca Miccio <lucmiccio@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 9755 bytes --]

Hi Julien,


Il giorno mer 9 mar 2022 alle ore 20:09 Julien Grall <julien@xen.org> ha
scritto:

> Hi,
>
> On 04/03/2022 17:46, Marco Solieri wrote:
> > From: Luca Miccio <lucmiccio@gmail.com>
> >
> > Add three new bootargs allowing configuration of cache coloring support
> > for Xen:
>
> I would prefer if documentation of each command line is part of the
> patch introducing them. This would help understanding some of the
> parameters.
>
>
Ok.


> > - way_size: The size of a LLC way in bytes. This value is mainly used
> >    to calculate the maximum available colors on the platform.
>
> We should only add command line option when they are a strong use case.
> In documentation, you wrote that someone may want to overwrite the way
> size for "specific needs".
>
> Can you explain what would be those needs?

> - dom0_colors: The coloring configuration for Dom0, which also acts as
> >    default configuration for any DomU without an explicit configuration.
> > - xen_colors: The coloring configuration for the Xen hypervisor itself.
> >
> > A cache coloring configuration consists of a selection of colors to be
> > assigned to a VM or to the hypervisor. It is represented by a set of
> > ranges. Add a common function that parses a string with a
> > comma-separated set of hyphen-separated ranges like "0-7,15-16" and
> > returns both: the number of chosen colors, and an array containing their
> > ids.
> > Currently we support platforms with up to 128 colors.
>
> Is there any reason this value is hardcoded in Xen rather than part of
> the Kconfig?
>
>
Not really at the time when this patch was created. But as we notify in
patch 32,
there is an assert that fails if we use a certain amount of colors. Maybe
we should
find a better way to store the color information.

Luca.

> >
> > Signed-off-by: Luca Miccio <lucmiccio@gmail.com>
> > Signed-off-by: Marco Solieri <marco.solieri@minervasys.tech>
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> > ---
> >   xen/arch/arm/Kconfig                |   5 ++
> >   xen/arch/arm/Makefile               |   2 +-
> >   xen/arch/arm/coloring.c             | 131 ++++++++++++++++++++++++++++
> >   xen/arch/arm/include/asm/coloring.h |  28 ++++++
> >   4 files changed, 165 insertions(+), 1 deletion(-)
> >   create mode 100644 xen/arch/arm/coloring.c
> >   create mode 100644 xen/arch/arm/include/asm/coloring.h
> >
> > diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> > index ecfa6822e4..f0f999d172 100644
> > --- a/xen/arch/arm/Kconfig
> > +++ b/xen/arch/arm/Kconfig
> > @@ -97,6 +97,11 @@ config HARDEN_BRANCH_PREDICTOR
> >
> >         If unsure, say Y.
> >
> > +config COLORING
> > +     bool "L2 cache coloring"
> > +     default n
>
> This wants to be gated with EXPERT for time-being. SUPPORT.MD woudl
> Furthermore, I think this wants to be gated with EXPERT for the time-being.
>
> > +     depends on ARM_64
>
> Why is this limited to arm64?
>
> > +
> >   config TEE
> >       bool "Enable TEE mediators support (UNSUPPORTED)" if UNSUPPORTED
> >       default n
> > diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> > index c993ce72a3..581896a528 100644
> > --- a/xen/arch/arm/Makefile
> > +++ b/xen/arch/arm/Makefile
> > @@ -66,7 +66,7 @@ obj-$(CONFIG_SBSA_VUART_CONSOLE) += vpl011.o
> >   obj-y += vsmc.o
> >   obj-y += vpsci.o
> >   obj-y += vuart.o
> > -
> > +obj-$(CONFIG_COLORING) += coloring.o
>
> Please keep the newline before extra-y. The file are meant to be ordered
> alphabetically. So this should be inserted in the correct position.
>
> >   extra-y += xen.lds
> >
> >   #obj-bin-y += ....o
> > diff --git a/xen/arch/arm/coloring.c b/xen/arch/arm/coloring.c
> > new file mode 100644
> > index 0000000000..8f1cff6efb
> > --- /dev/null
> > +++ b/xen/arch/arm/coloring.c
> > @@ -0,0 +1,131 @@
> > +/*
> > + * xen/arch/arm/coloring.c
> > + *
> > + * Coloring support for ARM
> > + *
> > + * Copyright (C) 2019 Xilinx Inc.
> > + *
> > + * Authors:
> > + *    Luca Miccio <lucmiccio@gmail.com>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program.  If not, see <http://www.gnu.org/licenses/
> >.
> > + */
> > +#include <xen/init.h>
> > +#include <xen/types.h>
> > +#include <xen/lib.h>
> > +#include <xen/errno.h>
> > +#include <xen/param.h>
> > +#include <asm/coloring.h>
>
> The includes should be ordered so <xen/...> are first, then <asm/...>.
> They are also ordered alphabetically within their own category.
>
> > +
> > +/* Number of color(s) assigned to Xen */
> > +static uint32_t xen_col_num;
> > +/* Coloring configuration of Xen as bitmask */
> > +static uint32_t xen_col_mask[MAX_COLORS_CELLS];
> Xen provides helpers to create and use bitmaps (see
> include/xen/bitmap.h). Can you use?
>
> > +
> > +/* Number of color(s) assigned to Dom0 */
> > +static uint32_t dom0_col_num;
> > +/* Coloring configuration of Dom0 as bitmask */
> > +static uint32_t dom0_col_mask[MAX_COLORS_CELLS];
> > +
> > +static uint64_t way_size;
> > +
> > +/*************************
> > + * PARSING COLORING BOOTARGS
> > + */
> > +
> > +/*
> > + * Parse the coloring configuration given in the buf string, following
> the
> > + * syntax below, and store the number of colors and a corresponding
> mask in
> > + * the last two given pointers.
> > + *
> > + * COLOR_CONFIGURATION ::= RANGE,...,RANGE
> > + * RANGE               ::= COLOR-COLOR
> > + *
> > + * Example: "2-6,15-16" represents the set of colors: 2,3,4,5,6,15,16.
> > + */
> > +static int parse_color_config(
> > +    const char *buf, uint32_t *col_mask, uint32_t *col_num)
>
>
> Coding style. We usually declarate paremeters on the same line as the
> function name. If they can't fit on the same line, then we split in two
> with the parameter aligned to the first paremeter.
>
> > +{
> > +    int start, end, i;
>
> AFAICT, none of the 3 variables will store negative values. So can they
> be unsigned?
>
> > +    const char* s = buf;
> > +    unsigned int offset;
> > +
> > +    if ( !col_mask || !col_num )
> > +        return -EINVAL;
> > +
> > +    *col_num = 0;
> > +    for ( i = 0; i < MAX_COLORS_CELLS; i++ )
> > +        col_mask[i] = 0;
> dom0_col_mask and xen_col_mask are already zeroed. I would also expect
> the same for dynamically allocated bitmask. So can this be dropped?
>
> > +
> > +    while ( *s != '\0' )
> > +    {
> > +        if ( *s != ',' )
> > +        {
> > +            start = simple_strtoul(s, &s, 0);
> > +
> > +            /* Ranges are hyphen-separated */
> > +            if ( *s != '-' )
> > +                goto fail;
> > +            s++;
> > +
> > +            end = simple_strtoul(s, &s, 0);
> > +
> > +            for ( i = start; i <= end; i++ )
> > +            {
> > +                offset = i / 32;
> > +                if ( offset > MAX_COLORS_CELLS )
> > +                    goto fail;
> > +
> > +                if ( !(col_mask[offset] & (1 << i % 32)) )
> > +                    *col_num += 1;
> > +                col_mask[offset] |= (1 << i % 32);
> > +            }
> > +        }
> > +        else
> > +            s++;
> > +    }
> > +
> > +    return *s ? -EINVAL : 0;
> > +fail:
> > +    return -EINVAL;
> > +}
> > +
> > +static int __init parse_way_size(const char *s)
> > +{
> > +    way_size = simple_strtoull(s, &s, 0);
> > +
> > +    return *s ? -EINVAL : 0;
> > +}
> > +custom_param("way_size", parse_way_size);
> > +
> > +static int __init parse_dom0_colors(const char *s)
> > +{
> > +    return parse_color_config(s, dom0_col_mask, &dom0_col_num);
> > +}
> > +custom_param("dom0_colors", parse_dom0_colors);
> > +
> > +static int __init parse_xen_colors(const char *s)
> > +{
> > +    return parse_color_config(s, xen_col_mask, &xen_col_num);
> > +}
> > +custom_param("xen_colors", parse_xen_colors);
> > +
> > +/*
> > + * Local variables:
> > + * mode: C
> > + * c-file-style: "BSD"
> > + * c-basic-offset: 4
> > + * tab-width: 4
> > + * indent-tabs-mode: nil
> > + * End:
> > + */
> > diff --git a/xen/arch/arm/include/asm/coloring.h
> b/xen/arch/arm/include/asm/coloring.h
> > new file mode 100644
> > index 0000000000..60958d1244
> > --- /dev/null
> > +++ b/xen/arch/arm/include/asm/coloring.h
> > @@ -0,0 +1,28 @@
> > +/*
> > + * xen/arm/include/asm/coloring.h
> > + *
> > + * Coloring support for ARM
> > + *
> > + * Copyright (C) 2019 Xilinx Inc.
> > + *
> > + * Authors:
> > + *    Luca Miccio <lucmiccio@gmail.com>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program.  If not, see <http://www.gnu.org/licenses/
> >.
> > + */
> > +#ifndef __ASM_ARM_COLORING_H__
> > +#define __ASM_ARM_COLORING_H__
> > +
> > +#define MAX_COLORS_CELLS 4
> > +
> > +#endif /* !__ASM_ARM_COLORING_H__ */
>
> Cheers,
>
> --
> Julien Grall
>

[-- Attachment #2: Type: text/html, Size: 13088 bytes --]