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

[Carlo Nonato <carlo.nonato@xxxxxxxxxxxxxxx>]

Hi Jan,

On Tue, Mar 19, 2024 at 4:30 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> 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.

How can I turn this into a lower-level option? Moving it into "dom0=" doesn't
seem possible to me. How can I express a list (llc-colors) inside another list
(dom0)? dom0=llc-colors=0-3,12-15,other-param=... How can I stop parsing
before reaching other-param?

> > --- 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.

I'll add it.

> > @@ -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?

Then how to distinguish when to free it in domain_destroy() and when not to do
it?

> 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.

Ok, I'll do that.

> > +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".

Right.

> > +static int __init parse_dom0_colors(const char *s)
> > +{
> > +    return parse_color_config(s, dom0_colors, ARRAY_SIZE(dom0_colors),
>
> With it not being possible to pass max_nr_colors here (due to the value
> not having been established yet), don't you need to check somewhere else
> that ...
>
> > +                              &dom0_num_colors);
>
> ... dom0_num_colors isn't too large?

I can add it in dom0_set_llc_colors().

> Jan

Thanks.