Re: [PATCH v6 04/15] xen/arm: add Dom0 cache coloring support

[Jan Beulich <jbeulich@xxxxxxxx>]

On 29.01.2024 18:18, Carlo Nonato wrote:
> Add a command line parameter to allow the user to set the coloring
> configuration for Dom0.
> A common configuration syntax for cache colors is introduced and
> documented.
> Take the opportunity to also add:
>  - default configuration notion.
>  - function to check well-formed configurations.
> 
> Direct mapping Dom0 isn't possible when coloring is enabled, so
> CDF_directmap flag is removed when creating it.

What implications does this have?

> --- 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 options is available only when
> +`CONFIG_LLC_COLORING` is enabled. If the parameter is not set, all available
> +colors are used.

Even Arm already has a "dom0=" option. Is there a particular reason why
this doesn't become a new sub-option there?

As to meaning: With just a single <integer>, that's still a color value
then (and not a count of colors)? Wouldn't it make sense to have a
simpler variant available where you just say how many, and a suitable
set/range is then picked?

Finally a nit: "This option is ...".

> @@ -2188,10 +2190,16 @@ void __init create_dom0(void)
>              panic("SVE vector length error\n");
>      }
>  
> -    dom0 = domain_create(0, &dom0_cfg, CDF_privileged | CDF_directmap);
> +    if ( !llc_coloring_enabled )
> +        flags |= CDF_directmap;
> +
> +    dom0 = domain_create(0, &dom0_cfg, flags);
>      if ( IS_ERR(dom0) )
>          panic("Error creating domain 0 (rc = %ld)\n", PTR_ERR(dom0));
>  
> +    if ( llc_coloring_enabled && (rc = dom0_set_llc_colors(dom0)) )
> +        panic("Error initializing LLC coloring for domain 0 (rc = %d)", rc);

As for the earlier patch, I find panic()ing here dubious. You can continue
quite fine, with a warning and perhaps again tainting the system.

> --- a/xen/common/llc-coloring.c
> +++ b/xen/common/llc-coloring.c
> @@ -17,6 +17,63 @@ size_param("llc-way-size", llc_way_size);
>  /* Number of colors available in the LLC */
>  static unsigned int __ro_after_init max_nr_colors = CONFIG_NR_LLC_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 parse_color_config(const char *buf, unsigned int *colors,
> +                              unsigned int num_colors, unsigned int 
> *num_parsed)

Is this function going to be re-used? If not, it wants to be __init.
If so, I wonder where the input string is going to come from ...

Also "num_colors" looks to be misnamed - doesn't this specify an
upper bound only?

> +{
> +    const char *s = buf;
> +
> +    if ( !colors || !num_colors )
> +        return -EINVAL;

Why do you check colors but not ...

> +    *num_parsed = 0;

... num_parsed? I think internal functions don't need such NULL checks.

> +    while ( *s != '\0' )
> +    {
> +        if ( *s != ',' )

Hmm, this way you also accept leading/trailing commas as well as multiple
consecutive ones. Elsewhere we're more strict.

> @@ -70,12 +150,85 @@ void __init llc_coloring_init(void)
>      arch_llc_coloring_init();
>  }
>  
> +void domain_llc_coloring_free(struct domain *d)
> +{
> +    xfree(__va(__pa(d->llc_colors)));

This __va(__pa()) trick deserves a comment, I think.

> +}
> +
>  void domain_dump_llc_colors(const struct domain *d)
>  {
>      printk("Domain %pd has %u LLC colors: ", d, d->num_llc_colors);
>      print_colors(d->llc_colors, d->num_llc_colors);
>  }
>  
> +static unsigned int *alloc_colors(unsigned int num_colors)
> +{
> +    unsigned int *colors;
> +
> +    if ( num_colors > max_nr_colors )
> +        return NULL;

Shouldn't check_colors() have made sure of this? If so, convert to
ASSERT()?

> +    colors = xmalloc_array(unsigned int, num_colors);
> +    if ( !colors )
> +        return NULL;

These last two lines are redundant with ...

> +    return colors;

... this one. Question then is whether this is useful at all as a
separate helper function.

> +}
> +
> +static int domain_check_colors(const struct domain *d)
> +{
> +    if ( !d->num_llc_colors )
> +    {
> +        printk(XENLOG_ERR "No LLC color config found for %pd\n", d);
> +        return -ENODATA;
> +    }
> +    else if ( !check_colors(d->llc_colors, d->num_llc_colors) )

I generally recommend against use of "else" in cases like this one.

> +    {
> +        printk(XENLOG_ERR "Bad LLC color config for %pd\n", d);
> +        return -EINVAL;
> +    }
> +
> +    return 0;
> +}
> +
> +static int domain_set_default_colors(struct domain *d)
> +{
> +    unsigned int *colors = alloc_colors(max_nr_colors);
> +    unsigned int i;
> +
> +    if ( !colors )
> +        return -ENOMEM;
> +
> +    printk(XENLOG_WARNING
> +           "LLC color config not found for %pd, using default\n", d);

Leaving open what the default(s) is/are. Judging from ...

> +    for ( i = 0; i < max_nr_colors; i++ )
> +        colors[i] = i;

... this it's simply "all colors". Then perhaps have the message also
say so?

> +    d->llc_colors = colors;
> +    d->num_llc_colors = max_nr_colors;
> +
> +    return 0;
> +}
> +
> +int __init dom0_set_llc_colors(struct domain *d)
> +{
> +    unsigned int *colors;
> +
> +    if ( !dom0_num_colors )
> +        return domain_set_default_colors(d);
> +
> +    colors = alloc_colors(dom0_num_colors);
> +    if ( !colors )
> +        return -ENOMEM;
> +
> +    memcpy(colors, dom0_colors, sizeof(unsigned int) * dom0_num_colors);

sizeof(*colors) or some such please. Plus a check that colors and
dom0_colors are actually of the same type. Alternatively, how about
making dom0_colors[] __ro_after_init? Is this too much of a waste?

Jan