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

[Carlo Nonato <carlo.nonato@xxxxxxxxxxxxxxx>]

On Thu, Mar 21, 2024 at 4:57 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 21.03.2024 16:04, Carlo Nonato wrote:
> > 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?
>
> 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.