Re: [PATCH v11 01/12] xen/common: add cache coloring common code

[Michal Orzel <michal.orzel@amd.com>]

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 <lucmiccio@gmail.com>
>>
>> Signed-off-by: Carlo Nonato <carlo.nonato@minervasys.tech>
>> Signed-off-by: Marco Solieri <marco.solieri@minervasys.tech>
>> Acked-by: Michal Orzel <michal.orzel@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