Re: [PATCH v5 03/13] xen/arm: add Dom0 cache coloring support

[Carlo Nonato <carlo.nonato@xxxxxxxxxxxxxxx>]

Hi Julien,

On Mon, Jan 8, 2024 at 11:25 AM Julien Grall <julien@xxxxxxx> wrote:
>
> Hi Carlo,
>
> On 08/01/2024 10:06, Carlo Nonato wrote:
> >> One of the reason is at least in the dom0less case, you will override
> >> the value afterwards.
> >
> > In that case I need to allocate the array before parsing the string.
> > I allocate a full array then the string is parsed and the actual size is 
> > found
> > at the end of this phase. Knowing the actual size would require two parsing
> > stages. Yes I'm wasting a bit of memory by oversizing the array here. Is it
> > a problem?
>
> While wasting memory is indeed not nice. This wasn't the main reason of
> this comment.
>
> The reason is that you seem to set d->num_lcc_colors will but will never
> be read before it gets overwritten. Looking again at the code, you are
> also assuming parse_colors() will always take an array of nr_colors.

Ok, I think I understood, but that happens only in dom0less case because
d->num_llc_colors is overwritten after parsing. In other cases it's ok to set
it there. Anyway I can move the assignment out of the function if that is
clearer.

> It would be better if parse_colors() takes the maximum size of the array
> in parameter. This would harden the code and it makes more sense for
> domain_alloc_colors() to set d->num_lcc_colors.

I don't understand this. parse_colors() must take only arrays of nr_colors
size (the global, maximum number of colors), otherwise the parsed string
config could exceed the array size. Since we don't know in advance the real
size before parsing, I think it's better to pass only arrays that are already
allocated with the maximum size.
Doing as you said I would still pass nr_colors as the maximum size, but that
would be strange since the global would still be accessible.
If domain_alloc_colors() setting d->num_llc_colors is so confusing,
I will just move the assignment after the function call.

> Also, I just noticed you have a global variable named nr_colors and the
> function parse_colors() takes an argument called *num_colors. This is
> quite confusing, can we have better name?
>
> Maybe rename nr_colors to nr_global_colors and and num_colors to
> nr_array_colors?

I agree with the fact that naming is confusing. I would opt for max_nr_colors
for the global.

Thanks.

> Cheers,
>
> --
> Julien Grall