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

From: Carlo Nonato <carlo.nonato@minervasys.tech>
To: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>,
	George Dunlap <george.dunlap@citrix.com>,
	 Julien Grall <julien@xen.org>,
	Stefano Stabellini <sstabellini@kernel.org>, Wei Liu <wl@xen.org>,
	 Bertrand Marquis <bertrand.marquis@arm.com>,
	Michal Orzel <michal.orzel@amd.com>,
	 Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>,
	Marco Solieri <marco.solieri@minervasys.tech>,
	 xen-devel@lists.xenproject.org,
	 Andrea Bastoni <andrea.bastoni@minervasys.tech>
Subject: Re: [PATCH v7 04/14] xen/arm: add Dom0 cache coloring support
Date: Thu, 21 Mar 2024 18:31:40 +0100	[thread overview]
Message-ID: <CAG+AhRUzj+hY0+F-zCSaq5nkC5CTLRdtcbmtr7PZEQKrdMWWWQ@mail.gmail.com> (raw)
In-Reply-To: <3ff10fdd-5a85-43b3-853a-fe69c7388ca9@suse.com>

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