| Message ID | 20240129171811.21382-5-carlo.nonato@minervasys.tech (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Arm cache coloring | expand |
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
Hi Jan, On 01/02/2024 13:30, Jan Beulich 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? > >> --- 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. There are arguments for both approach. I agree that you can continue but technically this is not the configuration you asked. Someone may not notice the tainting until it is too late (read they have done investigation). Bear in mind that the user for cache coloring will be in very specialized environment. So if you can't enable cache coloring in production, then something really wrong has happened and continue to boot is probably not right. This matches the approach for Arm we have been using since the beginning. And I will strongly argue to continue this way. Cheers,
On 01.02.2024 14:35, Julien Grall wrote: > Hi Jan, > > On 01/02/2024 13:30, Jan Beulich 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? >> >>> --- 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. > There are arguments for both approach. In which case - perhaps allow for both? With a Kconfig-established default and a command line option to override? > I agree that you can continue but > technically this is not the configuration you asked. Someone may not > notice the tainting until it is too late (read they have done > investigation). > > Bear in mind that the user for cache coloring will be in very > specialized environment. s/will/may/ I suppose. People may enable the option without being in any specialized environment. > So if you can't enable cache coloring in > production, then something really wrong has happened and continue to > boot is probably not right. > > This matches the approach for Arm we have been using since the > beginning. And I will strongly argue to continue this way. I'm okay with this, and here (for Arm-specific code) it may even be okay to do so without further justification. But in the earlier patch where common code is affected, I'll insist on at least justifying this behavior. Jan
Hi Jan, On 01/02/2024 13:39, Jan Beulich wrote: > On 01.02.2024 14:35, Julien Grall wrote: >> Hi Jan, >> >> On 01/02/2024 13:30, Jan Beulich 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? >>> >>>> --- 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. >> There are arguments for both approach. > > In which case - perhaps allow for both? With a Kconfig-established > default and a command line option to override? Perhaps. But this is a separate discussion from this series. What Carlo has been doing match the surrounding code on Arm. > >> I agree that you can continue but >> technically this is not the configuration you asked. Someone may not >> notice the tainting until it is too late (read they have done >> investigation). >> >> Bear in mind that the user for cache coloring will be in very >> specialized environment. > > s/will/may/ I suppose. People may enable the option without being in > any specialized environment. Sure. But again, why would you want to boot with a half broken configuration? In a lot of cases, you are not making a favor to the admin to continue to boot. It is easy to say there is a warning in the logs, but this can often be overlooked and difficult to diagnostic afterwards. For instance, if you think about cache coloring the issue would be latency. I don't think a lambda users will be able to easily figure out that their configuration was wrong. Futhermore, when you operate at scale, I feel it is better to have an early boot crash rather than allowing the system to boot (parsing the logs is feasible but IMO risky as they are not stable). > >> So if you can't enable cache coloring in >> production, then something really wrong has happened and continue to >> boot is probably not right. >> >> This matches the approach for Arm we have been using since the >> beginning. And I will strongly argue to continue this way. > > I'm okay with this, and here (for Arm-specific code) it may even be okay > to do so without further justification. But in the earlier patch where > common code is affected, I'll insist on at least justifying this behavior. See above for a justification. If someone asks for cache coloring, then you most likely don't want to continue without cache coloring. If you dislike the panic() in common code, then we can simply modify the function to return an error and move the panic() in the Arm code. This is not my preference, but I am under the impression that we both have very diverging view how to handle boot error and it will be hard to reconcile them (at least in this series, this can be done afterwards if somone fancy to write a series matching what you proposed above). So this would be the second best option for me. Cheers,
Hi Jan, On Thu, Feb 1, 2024 at 2:30 PM Jan Beulich <jbeulich@suse.com> 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
On 03.02.2024 12:39, Carlo Nonato wrote: > On Thu, Feb 1, 2024 at 2:30 PM Jan Beulich <jbeulich@suse.com> wrote: >> On 29.01.2024 18:18, Carlo Nonato wrote: >>> --- 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. Am I misremembering to have spotted a non-init use in a later patch? >> Also "num_colors" looks to be misnamed - doesn't this specify an >> upper bound only? > > It's the real size of the colors array. Hence my remark: It is _not_ the number of colors. >>> +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? I don't think you can use ASSERT() for such very well. It's runtime check, when here we want a build-time one. I'd therefore rather see it be something like (void)(colors == dom0_colors); Jan
Hi Jan, On Mon, Feb 5, 2024 at 10:35 AM Jan Beulich <jbeulich@suse.com> wrote: > > On 03.02.2024 12:39, Carlo Nonato wrote: > > On Thu, Feb 1, 2024 at 2:30 PM Jan Beulich <jbeulich@suse.com> wrote: > >> On 29.01.2024 18:18, Carlo Nonato wrote: > >>> --- 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. > > Am I misremembering to have spotted a non-init use in a later patch? The only usage of the function other than for parameter parsing is in domain_set_llc_colors_from_str() which is in turn used in dom0less domain creation, so from another __init function. Thanks. > >> Also "num_colors" looks to be misnamed - doesn't this specify an > >> upper bound only? > > > > It's the real size of the colors array. > > Hence my remark: It is _not_ the number of colors. > > >>> +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? > > I don't think you can use ASSERT() for such very well. It's runtime > check, when here we want a build-time one. I'd therefore rather see > it be something like > > (void)(colors == dom0_colors); > > Jan
diff --git a/docs/misc/cache-coloring.rst b/docs/misc/cache-coloring.rst index 0535b5c656..c347725525 100644 --- a/docs/misc/cache-coloring.rst +++ b/docs/misc/cache-coloring.rst @@ -85,6 +85,35 @@ More specific documentation is available at `docs/misc/xen-command-line.pandoc`. +----------------------+-------------------------------+ | ``llc-way-size`` | set the LLC way size | +----------------------+-------------------------------+ +| ``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] | ++-------------------+-----------------------------+ Known issues and limitations **************************** diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc index 11f9f209d1..25da997b5b 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 options 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 a9e5310aff..e380d25dca 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> @@ -2161,6 +2162,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 */ @@ -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); + 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 10729e70c1..983de44a47 100644 --- 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) +{ + const char *s = buf; + + if ( !colors || !num_colors ) + return -EINVAL; + + *num_parsed = 0; + + while ( *s != '\0' ) + { + if ( *s != ',' ) + { + 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_parsed) || + (*num_parsed + (end - start)) >= num_colors ) + return -EINVAL; + for ( color = start; color <= end; color++ ) + colors[(*num_parsed)++] = color; + } + else + s++; + } + + return *s ? -EINVAL : 0; +} + +static int __init parse_dom0_colors(const char *s) +{ + return parse_color_config(s, dom0_colors, max_nr_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; @@ -47,6 +104,29 @@ static void dump_coloring_info(unsigned char key) printk("Number of LLC colors supported: %u\n", max_nr_colors); } +static bool check_colors(const unsigned int *colors, unsigned int num_colors) +{ + unsigned int i; + + if ( num_colors > max_nr_colors ) + { + printk(XENLOG_ERR "Number of LLC colors requested > %u\n", + max_nr_colors); + return false; + } + + 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) { if ( !llc_way_size && !(llc_way_size = get_llc_way_size()) ) @@ -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))); +} + 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; + + colors = xmalloc_array(unsigned int, num_colors); + if ( !colors ) + return NULL; + + return 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) ) + { + 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); + + 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); + + colors = alloc_colors(dom0_num_colors); + if ( !colors ) + return -ENOMEM; + + memcpy(colors, dom0_colors, sizeof(unsigned int) * dom0_num_colors); + d->llc_colors = colors; + d->num_llc_colors = dom0_num_colors; + + return domain_check_colors(d); +} + /* * Local variables: * mode: C diff --git a/xen/include/xen/llc-coloring.h b/xen/include/xen/llc-coloring.h index 5e12eb426f..1a73080c98 100644 --- a/xen/include/xen/llc-coloring.h +++ b/xen/include/xen/llc-coloring.h @@ -14,16 +14,19 @@ extern bool llc_coloring_enabled; void llc_coloring_init(void); +void domain_llc_coloring_free(struct domain *d); void domain_dump_llc_colors(const struct domain *d); #else #define llc_coloring_enabled false static inline void llc_coloring_init(void) {} +static inline void domain_llc_coloring_free(struct domain *d) {} static inline void domain_dump_llc_colors(const 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__ */