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

[Carlo Nonato <carlo.nonato@xxxxxxxxxxxxxxx>]

Hi Jan,

On Thu, Feb 1, 2024 at 2:30 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> 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?

You will need an IOMMU to boot and extra care when assigning guest physical
addresses to Dom0. We have a new patch for that and it should solve what
Michal was pointing out in the cover letter.

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

Cause this is a list and I don't think "dom0" option supports it since it's
already a list.

> As to meaning: With just a single <integer>, that's still a color value
> then (and not a count of colors)?

Exactly.

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

This can be done in the future. It's not a feature that we want to support as
of now. For the moment we just want to give the user maximum freedom.

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

You're right. It needs __init.

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

It's the real size of the colors array. Than the used size will be found in
num_parsed.

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

Ok I'll drop it.

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

I'm going to remove alloc_colors().

> > +}
> > +
> > +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?

Yep, ok.

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

You mean an ASSERT on the two arrays type?

Thanks

> Jan