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
