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

[Carlo Nonato <carlo.nonato@xxxxxxxxxxxxxxx>]

Hi Jan,

On Mon, Feb 5, 2024 at 10:35 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 03.02.2024 12:39, Carlo Nonato wrote:
> > On Thu, Feb 1, 2024 at 2:30 PM Jan Beulich <jbeulich@xxxxxxxx> 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