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

Commit Message

Carlo Nonato March 15, 2024, 10:58 a.m. UTC

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.

Based on original work from: Luca Miccio <[e-mail]>

Signed-off-by: Carlo Nonato <[e-mail]>
Signed-off-by: Marco Solieri <[e-mail]>
---
v7:
- parse_color_config() doesn't accept leading/trailing commas anymore
- removed alloc_colors() helper
v6:
- moved domain_llc_coloring_free() in this patch
- removed domain_alloc_colors() in favor of a more explicit allocation
- parse_color_config() now accepts the size of the array to be filled
- allocate_memory() moved in another patch
v5:
- Carlo Nonato as the new author
- moved dom0 colors parsing (parse_colors()) in this patch
- added dom0_set_llc_colors() to set dom0 colors after creation
- moved color allocation and checking in this patch
- error handling when allocating color arrays
- FIXME: copy pasted allocate_memory() cause it got moved
v4:
- dom0 colors are dynamically allocated as for any other domain
  (colors are duplicated in dom0_colors and in the new array, but logic
  is simpler)
---
 docs/misc/cache-coloring.rst      |  29 +++++++
 docs/misc/xen-command-line.pandoc |   9 +++
 xen/arch/arm/domain_build.c       |  10 ++-
 xen/common/domain.c               |   3 +
 xen/common/llc-coloring.c         | 128 ++++++++++++++++++++++++++++++
 xen/include/xen/llc-coloring.h    |   3 +
 6 files changed, 181 insertions(+), 1 deletion(-)

Comments

Jan Beulich March 19, 2024, 3:30 p.m. UTC | #1

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

Jan Beulich March 19, 2024, 3:45 p.m. UTC | #2

On 15.03.2024 11:58, Carlo Nonato wrote:
> --- 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;
> +
> +        if ( *s == ',' )
> +            s++;
> +        else if ( *s != '\0' )
> +            break;
> +    }
> +
> +    return *s ? -EINVAL : 0;
> +}
> +
> +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?

Jan

Carlo Nonato March 21, 2024, 3:04 p.m. UTC | #3

Hi Jan,

On Tue, Mar 19, 2024 at 4:30 PM Jan Beulich <[e-mail]> 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.

Jan Beulich March 21, 2024, 3:57 p.m. UTC | #4

On 21.03.2024 16:04, Carlo Nonato wrote:
> On Tue, Mar 19, 2024 at 4:30 PM Jan Beulich <[e-mail]> 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?

For example by using a different separator:

dom0=llc-colors=0-3+12-15,other-param=...

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

By checking against that one special array instance.

Jan

Carlo Nonato March 21, 2024, 5:31 p.m. UTC | #5

On Thu, Mar 21, 2024 at 4:57 PM Jan Beulich <[e-mail]> wrote:
>
> On 21.03.2024 16:04, Carlo Nonato wrote:
> > On Tue, Mar 19, 2024 at 4:30 PM Jan Beulich <[e-mail]> 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?
>
> For example by using a different separator:
>
> dom0=llc-colors=0-3+12-15,other-param=...

Ok, but that would mean to change the implementation of the parsing function
and to adopt this syntax also in other places, something that I would've
preferred to avoid. Anyway I'll follow your suggestion.

> >>> @@ -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?
>
> By checking against that one special array instance.

Ok.

> Jan

Thanks.

Jan Beulich March 22, 2024, 7:26 a.m. UTC | #6

On 21.03.2024 18:31, Carlo Nonato wrote:
> On Thu, Mar 21, 2024 at 4:57 PM Jan Beulich <[e-mail]> wrote:
>>
>> On 21.03.2024 16:04, Carlo Nonato wrote:
>>> On Tue, Mar 19, 2024 at 4:30 PM Jan Beulich <[e-mail]> 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?
>>
>> For example by using a different separator:
>>
>> dom0=llc-colors=0-3+12-15,other-param=...
> 
> Ok, but that would mean to change the implementation of the parsing function
> and to adopt this syntax also in other places, something that I would've
> preferred to avoid. Anyway I'll follow your suggestion.

Well, this is all used by Arm only for now. You will want to make sure Arm
folks are actually okay with this alternative approach.

Jan

Carlo Nonato March 27, 2024, 11:39 a.m. UTC | #7

Hi guys,

On Fri, Mar 22, 2024 at 8:26 AM Jan Beulich <[e-mail]> wrote:
>
> On 21.03.2024 18:31, Carlo Nonato wrote:
> > On Thu, Mar 21, 2024 at 4:57 PM Jan Beulich <[e-mail]> wrote:
> >>
> >> On 21.03.2024 16:04, Carlo Nonato wrote:
> >>> On Tue, Mar 19, 2024 at 4:30 PM Jan Beulich <[e-mail]> 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?
> >>
> >> For example by using a different separator:
> >>
> >> dom0=llc-colors=0-3+12-15,other-param=...
> >
> > Ok, but that would mean to change the implementation of the parsing function
> > and to adopt this syntax also in other places, something that I would've
> > preferred to avoid. Anyway I'll follow your suggestion.
>
> Well, this is all used by Arm only for now. You will want to make sure Arm
> folks are actually okay with this alternative approach.
>
> Jan

Are you Arm maintainers ok with this?

Thanks.

Julien Grall March 27, 2024, 11:56 a.m. UTC | #8

Hi,

On 27/03/2024 11:39, Carlo Nonato wrote:
> On Fri, Mar 22, 2024 at 8:26 AM Jan Beulich <[e-mail]> wrote:
>>
>> On 21.03.2024 18:31, Carlo Nonato wrote:
>>> On Thu, Mar 21, 2024 at 4:57 PM Jan Beulich <[e-mail]> wrote:
>>>>
>>>> On 21.03.2024 16:04, Carlo Nonato wrote:
>>>>> On Tue, Mar 19, 2024 at 4:30 PM Jan Beulich <[e-mail]> 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?
>>>>
>>>> For example by using a different separator:
>>>>
>>>> dom0=llc-colors=0-3+12-15,other-param=...
>>>
>>> Ok, but that would mean to change the implementation of the parsing function
>>> and to adopt this syntax also in other places, something that I would've
>>> preferred to avoid. Anyway I'll follow your suggestion.
>>
>> Well, this is all used by Arm only for now. You will want to make sure Arm
>> folks are actually okay with this alternative approach.
>>
>> Jan
> 
> Are you Arm maintainers ok with this?

Unfortunately no. I find the use of + and "nested" = extremely confusing 
to read.

There might be other symbols to use (e.g. s/=/:/ s#+#/#), but I am not 
entirely sure the value of trying to cram all the options under a single 
top-level parameter. So I right now, I would prefrr if we stick with the 
existing approach (i.e. introducing dom0-llc-colors).

Cheers,

Michal Orzel March 27, 2024, 11:57 a.m. UTC | #9

On 27/03/2024 12:39, Carlo Nonato wrote:
> 
> 
> Hi guys,
> 
> On Fri, Mar 22, 2024 at 8:26 AM Jan Beulich <[e-mail]> wrote:
>>
>> On 21.03.2024 18:31, Carlo Nonato wrote:
>>> On Thu, Mar 21, 2024 at 4:57 PM Jan Beulich <[e-mail]> wrote:
>>>>
>>>> On 21.03.2024 16:04, Carlo Nonato wrote:
>>>>> On Tue, Mar 19, 2024 at 4:30 PM Jan Beulich <[e-mail]> 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?
>>>>
>>>> For example by using a different separator:
>>>>
>>>> dom0=llc-colors=0-3+12-15,other-param=...
>>>
>>> Ok, but that would mean to change the implementation of the parsing function
>>> and to adopt this syntax also in other places, something that I would've
>>> preferred to avoid. Anyway I'll follow your suggestion.
>>
>> Well, this is all used by Arm only for now. You will want to make sure Arm
>> folks are actually okay with this alternative approach.
>>
>> Jan
> 
> Are you Arm maintainers ok with this?
I'm not a fan of this syntax and I find it more difficult to parse compared to the usual case, where
every option is clearly separated. That said, I won't oppose to it.

~Michal

Stefano Stabellini April 5, 2024, 12:20 a.m. UTC | #10

On Wed, 27 Mar 2024, Julien Grall wrote:
> Hi,
> 
> On 27/03/2024 11:39, Carlo Nonato wrote:
> > On Fri, Mar 22, 2024 at 8:26 AM Jan Beulich <[e-mail]> wrote:
> > > 
> > > On 21.03.2024 18:31, Carlo Nonato wrote:
> > > > On Thu, Mar 21, 2024 at 4:57 PM Jan Beulich <[e-mail]> wrote:
> > > > > 
> > > > > On 21.03.2024 16:04, Carlo Nonato wrote:
> > > > > > On Tue, Mar 19, 2024 at 4:30 PM Jan Beulich <[e-mail]>
> > > > > > 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?
> > > > > 
> > > > > For example by using a different separator:
> > > > > 
> > > > > dom0=llc-colors=0-3+12-15,other-param=...
> > > > 
> > > > Ok, but that would mean to change the implementation of the parsing
> > > > function
> > > > and to adopt this syntax also in other places, something that I would've
> > > > preferred to avoid. Anyway I'll follow your suggestion.
> > > 
> > > Well, this is all used by Arm only for now. You will want to make sure Arm
> > > folks are actually okay with this alternative approach.
> > > 
> > > Jan
> > 
> > Are you Arm maintainers ok with this?
> 
> Unfortunately no. I find the use of + and "nested" = extremely confusing to
> read.
> 
> There might be other symbols to use (e.g. s/=/:/ s#+#/#), but I am not
> entirely sure the value of trying to cram all the options under a single
> top-level parameter. So I right now, I would prefrr if we stick with the
> existing approach (i.e. introducing dom0-llc-colors).

I also prefer the original as suggested in this version of the patch

Patch

diff --git a/docs/misc/cache-coloring.rst b/docs/misc/cache-coloring.rst
index 871e7a3ddb..4c859135cb 100644
--- a/docs/misc/cache-coloring.rst
+++ b/docs/misc/cache-coloring.rst
@@ -114,6 +114,35 @@  Specific documentation is available at `docs/misc/xen-command-line.pandoc`.
 +----------------------+-------------------------------+
 | ``llc-nr-ways``      | set the LLC number of ways    |
 +----------------------+-------------------------------+
+| ``dom0-llc-colors``  | Dom0 color configuration      |
++----------------------+-------------------------------+
+
+Colors selection format
+***********************
+
+Regardless of the memory pool that has to be colored (Xen, Dom0/DomUs),
+the color selection can be expressed using the same syntax. In particular a
+comma-separated list of colors or ranges of colors is used.
+Ranges are hyphen-separated intervals (such as `0-4`) and are inclusive on both
+sides.
+
+Note that:
+
+- no spaces are allowed between values.
+- no overlapping ranges or duplicated colors are allowed.
+- values must be written in ascending order.
+
+Examples:
+
++-------------------+-----------------------------+
+| **Configuration** | **Actual selection**        |
++-------------------+-----------------------------+
+| 1-2,5-8           | [1, 2, 5, 6, 7, 8]          |
++-------------------+-----------------------------+
+| 4-8,10,11,12      | [4, 5, 6, 7, 8, 10, 11, 12] |
++-------------------+-----------------------------+
+| 0                 | [0]                         |
++-------------------+-----------------------------+
 
 Auto-probing of LLC specs
 #########################
diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index 2936abea2c..28035a214d 100644
--- 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.
+
 ### dom0_max_vcpus
 
 Either:
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index d21be2c57b..3de1659836 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -2,6 +2,7 @@ 
 #include <xen/init.h>
 #include <xen/compile.h>
 #include <xen/lib.h>
+#include <xen/llc-coloring.h>
 #include <xen/mm.h>
 #include <xen/param.h>
 #include <xen/domain_page.h>
@@ -2208,6 +2209,7 @@  void __init create_dom0(void)
         .max_maptrack_frames = -1,
         .grant_opts = XEN_DOMCTL_GRANT_version(opt_gnttab_max_version),
     };
+    unsigned int flags = CDF_privileged;
     int rc;
 
     /* The vGIC for DOM0 is exactly emulating the hardware GIC */
@@ -2235,10 +2237,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);
+
     if ( alloc_dom0_vcpu0(dom0) == NULL )
         panic("Error creating domain 0 vcpu0\n");
 
diff --git a/xen/common/domain.c b/xen/common/domain.c
index f6f5574996..f144b54f4f 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -33,6 +33,7 @@ 
 #include <xen/xenoprof.h>
 #include <xen/irq.h>
 #include <xen/argo.h>
+#include <xen/llc-coloring.h>
 #include <asm/p2m.h>
 #include <asm/processor.h>
 #include <public/sched.h>
@@ -1208,6 +1209,8 @@  void domain_destroy(struct domain *d)
 
     BUG_ON(!d->is_dying);
 
+    domain_llc_coloring_free(d);
+
     /* May be already destroyed, or get_domain() can race us. */
     if ( atomic_cmpxchg(&d->refcnt, 0, DOMAIN_DESTROYED) != 0 )
         return;
diff --git a/xen/common/llc-coloring.c b/xen/common/llc-coloring.c
index 51eae90ad5..ebd7087dc2 100644
--- 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;
+
+        if ( *s == ',' )
+            s++;
+        else if ( *s != '\0' )
+            break;
+    }
+
+    return *s ? -EINVAL : 0;
+}
+
+static int __init parse_dom0_colors(const char *s)
+{
+    return parse_color_config(s, dom0_colors, ARRAY_SIZE(dom0_colors),
+                              &dom0_num_colors);
+}
+custom_param("dom0-llc-colors", parse_dom0_colors);
+
 static void print_colors(const unsigned int *colors, unsigned int num_colors)
 {
     unsigned int i;
@@ -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;
+}
+
 void __init llc_coloring_init(void)
 {
     unsigned int way_size;
@@ -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;
+}
+
+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);
+    memcpy(colors, dom0_colors, sizeof(*colors) * dom0_num_colors);
+    d->llc_colors = colors;
+    d->num_llc_colors = dom0_num_colors;
+
+    return 0;
+}
+
+void domain_llc_coloring_free(struct domain *d)
+{
+    if ( !llc_coloring_enabled )
+        return;
+
+    /* free pointer-to-const using __va(__pa()) */
+    xfree(__va(__pa(d->llc_colors)));
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/include/xen/llc-coloring.h b/xen/include/xen/llc-coloring.h
index 67b27c995b..ee82932266 100644
--- a/xen/include/xen/llc-coloring.h
+++ b/xen/include/xen/llc-coloring.h
@@ -16,16 +16,19 @@  extern bool llc_coloring_enabled;
 void llc_coloring_init(void);
 void dump_llc_coloring_info(void);
 void domain_dump_llc_colors(const struct domain *d);
+void domain_llc_coloring_free(struct domain *d);
 #else
 #define llc_coloring_enabled false
 
 static inline void llc_coloring_init(void) {}
 static inline void dump_llc_coloring_info(void) {}
 static inline void domain_dump_llc_colors(const struct domain *d) {}
+static inline void domain_llc_coloring_free(struct domain *d) {}
 #endif
 
 unsigned int get_llc_way_size(void);
 void arch_llc_coloring_init(void);
+int dom0_set_llc_colors(struct domain *d);
 
 #endif /* __COLORING_H__ */