On 03/12/2024 10:55, Michal Orzel wrote:
>
>
> On 02/12/2024 17:59, Carlo Nonato wrote:
>>
>>
>> Last Level Cache (LLC) coloring allows to partition the cache in smaller
>> chunks called cache colors.
>>
>> Since not all architectures can actually implement it, add a HAS_LLC_COLORING
>> Kconfig option.
>> LLC_COLORS_ORDER Kconfig option has a range maximum of 10 (2^10 = 1024)
>> because that's the number of colors that fit in a 4 KiB page when integers
>> are 4 bytes long.
>>
>> LLC colors are a property of the domain, so struct domain has to be extended.
>>
>> Based on original work from: Luca Miccio <lucmic...@gmail.com>
>>
>> Signed-off-by: Carlo Nonato <carlo.non...@minervasys.tech>
>> Signed-off-by: Marco Solieri <marco.soli...@minervasys.tech>
>> Acked-by: Michal Orzel <michal.or...@amd.com>
>
> [...]
>>
>> +### llc-coloring (arm64)
>> +> `= <boolean>`
>> +
>> +> Default: `false`
> By default, it is disabled. If CONFIG_ is enabled but ...
>
> [...]
>
>> + * -1: not specified (disabled unless llc-size and llc-nr-ways present)
> the user doesn't specify any llc-* options, LLC feature should be disabled.
> In your case llc_coloring_enabled is -1 and due to 'if ( llc_coloring_enabled
> ... )' checks
> all around the code base, the LLC will be enabled even though it should not.
>
> You can either set it to 0 if (llc_coloring_enabled < 0) and other llc-*
> options have not been provided
> (this would require modifying this comment to provide different meaning
> depending on the context) or
> you could do sth like that:
>
> diff --git a/xen/common/llc-coloring.c b/xen/common/llc-coloring.c
> index 2d6aed5fb4ac..560fe03aa86b 100644
> --- a/xen/common/llc-coloring.c
> +++ b/xen/common/llc-coloring.c
> @@ -18,8 +18,10 @@
> * 0: explicitly disabled through cmdline
> * 1: explicitly enabled through cmdline
> */
> -int8_t __ro_after_init llc_coloring_enabled = -1;
> -boolean_param("llc-coloring", llc_coloring_enabled);
> +int8_t __init opt_llc_coloring = -1;
> +boolean_param("llc-coloring", opt_llc_coloring);
> +
> +bool __ro_after_init llc_coloring_enabled = false;
>
> static unsigned int __initdata llc_size;
> size_param("llc-size", llc_size);
> @@ -147,15 +149,17 @@ void __init llc_coloring_init(void)
> {
> unsigned int way_size, i;
>
> - if ( (llc_coloring_enabled < 0) && (llc_size && llc_nr_ways) )
> + if ( (opt_llc_coloring < 0) && (llc_size && llc_nr_ways) )
> {
> llc_coloring_enabled = true;
> way_size = llc_size / llc_nr_ways;
> }
> - else if ( !llc_coloring_enabled )
> + else if ( !opt_llc_coloring )
> return;
> else
> {
> + llc_coloring_enabled = true;
> +
> way_size = get_llc_way_size();
> if ( !way_size )
> panic("LLC probing failed and 'llc-size' or 'llc-nr-ways'
> missing\n");
>
> I think that this would be better in terms of readability.
>
> ~Michal
>
>
On top of my previous comments, attempt to build patch 2 with LLC_COLORING
enabled results in a few
build errors. In general, this should be avoided to allow bisection. There are
2 issues:
- error: invalid use of undefined type 'const struct domain'
You need to include xen/sched.h
- error: unknown type name 'int8_t'
You need to include xen/types.h in a header (regardless of whether you stick
to int8_t or bool)
- implicit declaration of function 'isb'
~Michal
