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

[Michal Orzel <michal.orzel@xxxxxxx>]

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@xxxxxxxxx>
> 
> Signed-off-by: Carlo Nonato <carlo.nonato@xxxxxxxxxxxxxxx>
> Signed-off-by: Marco Solieri <marco.solieri@xxxxxxxxxxxxxxx>
> Acked-by: Michal Orzel <michal.orzel@xxxxxxx>

[...]
> 
> +### 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