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

[Jan Beulich <jbeulich@xxxxxxxx>]

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?

>> 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