Re: [PATCH v7 04/14] xen/arm: add Dom0 cache coloring support

[Jan Beulich <jbeulich@suse.com>]

On 15.03.2024 11:58, Carlo Nonato wrote:
> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -963,6 +963,15 @@ Controls for the dom0 IOMMU setup.
>  
>  Specify a list of IO ports to be excluded from dom0 access.
>  
> +### dom0-llc-colors
> +> `= List of [ <integer> | <integer>-<integer> ]`
> +
> +> Default: `All available LLC colors`
> +
> +Specify dom0 LLC color configuration. This option is available only when
> +`CONFIG_LLC_COLORING` is enabled. If the parameter is not set, all available
> +colors are used.

My reservation towards this being a top-level option remains.

> --- a/xen/common/llc-coloring.c
> +++ b/xen/common/llc-coloring.c
> @@ -18,6 +18,63 @@ integer_param("llc-nr-ways", llc_nr_ways);
>  /* Number of colors available in the LLC */
>  static unsigned int __ro_after_init max_nr_colors;
>  
> +static unsigned int __initdata dom0_colors[CONFIG_NR_LLC_COLORS];
> +static unsigned int __initdata 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 __init parse_color_config(const char *buf, unsigned int *colors,
> +                                     unsigned int max_num_colors,
> +                                     unsigned int *num_colors)
> +{
> +    const char *s = buf;
> +
> +    *num_colors = 0;
> +
> +    while ( *s != '\0' )
> +    {
> +        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 || (end - start) > (UINT_MAX - *num_colors) ||
> +             (*num_colors + (end - start)) >= max_num_colors )
> +            return -EINVAL;
> +
> +        for ( color = start; color <= end; color++ )
> +            colors[(*num_colors)++] = color;

I can't spot any range check on start/end/color itself. In fact I was first
meaning to ask why the return value of simple_strtoul() is silently clipped
from unsigned long to unsigned int. Don't forget that a range specification
may easily degenerate into a negative number (due to a simple oversight or
typo), which would then be converted to a huge positive one.

> @@ -41,6 +98,22 @@ static void print_colors(const unsigned int *colors, unsigned int num_colors)
>      printk(" }\n");
>  }
>  
> +static bool check_colors(const unsigned int *colors, unsigned int num_colors)
> +{
> +    unsigned int i;
> +
> +    for ( i = 0; i < num_colors; i++ )
> +    {
> +        if ( colors[i] >= max_nr_colors )
> +        {
> +            printk(XENLOG_ERR "LLC color %u >= %u\n", colors[i], max_nr_colors);
> +            return false;
> +        }
> +    }
> +
> +    return true;
> +}

Oh, here's the range checking of the color values themselves. Perhaps
a comment in parse_color_config() would help.

> @@ -91,6 +164,61 @@ void cf_check domain_dump_llc_colors(const struct domain *d)
>      print_colors(d->llc_colors, d->num_llc_colors);
>  }
>  
> +static int domain_set_default_colors(struct domain *d)
> +{
> +    unsigned int *colors = xmalloc_array(unsigned int, max_nr_colors);
> +    unsigned int i;
> +
> +    if ( !colors )
> +        return -ENOMEM;
> +
> +    printk(XENLOG_WARNING
> +           "LLC color config not found for %pd, using all colors\n", d);
> +
> +    for ( i = 0; i < max_nr_colors; i++ )
> +        colors[i] = i;
> +
> +    d->llc_colors = colors;
> +    d->num_llc_colors = max_nr_colors;
> +
> +    return 0;
> +}

If this function is expected to actually come into play, wouldn't it
make sense to set up such an array just once, and re-use it wherever
necessary?

Also right here both this and check_colors() could be __init. I
understand that subsequent patches will also want to use the
functions at runtime, but until then this looks slightly wrong. I'd
like to ask that such aspects be mentioned in the description, to
avoid respective questions.

> +int __init dom0_set_llc_colors(struct domain *d)
> +{
> +    unsigned int *colors;
> +
> +    if ( !dom0_num_colors )
> +        return domain_set_default_colors(d);
> +
> +    if ( !check_colors(dom0_colors, dom0_num_colors) )
> +    {
> +        printk(XENLOG_ERR "Bad LLC color config for %pd\n", d);
> +        return -EINVAL;
> +    }
> +
> +    colors = xmalloc_array(unsigned int, dom0_num_colors);
> +    if ( !colors )
> +        return -ENOMEM;
> +
> +    /* Static type checking */
> +    (void)(colors == dom0_colors);

Btw, a means to avoid this would by to use typeof() in the declaration
of "colors".

Jan