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

[Michal Orzel <michal.orzel@xxxxxxx>]

On 27/11/2024 14:24, Carlo Nonato wrote:
> 
> 
> Hi Michal,
> 
> On Wed, Nov 27, 2024 at 11:48 AM Michal Orzel <michal.orzel@xxxxxxx> wrote:
>> On 19/11/2024 15:13, 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>

[...]

>>> +### llc-nr-ways (arm64)
>>> +> `= <integer>`
>>> +
>>> +> Default: `Obtained from hardware`
>>> +
>>> +Specify the number of ways of the Last Level Cache. This option is 
>>> available
>>> +only when `CONFIG_LLC_COLORING` is enabled. LLC size and number of ways 
>>> are used
>>> +to find the number of supported cache colors. By default the value is
>>> +automatically computed by probing the hardware, but in case of specific 
>>> needs,
>>> +it can be manually set. Those include failing probing and debugging/testing
>>> +purposes so that it's possible to emulate platforms with different number 
>>> of
>>> +supported colors. If set, also "llc-size" must be set, otherwise the 
>>> default
>>> +will be used. Note that using both options implies "llc-coloring=on".
>> I can understand this decision, but ...
>>
>> [...]
>>
>>> +    if ( llc_size && llc_nr_ways )
>>> +    {
>>> +        llc_coloring_enabled = true;
>>> +        way_size = llc_size / llc_nr_ways;
>>> +    }
>>> +    else if ( !llc_coloring_enabled )
>>> +        return;
>> the above code does not seem to be right. When debugging or in general it is 
>> useful to have on the cmdline:
>> llc-size=1M llc-nr-ways=16 llc-coloring=on
>> and be able to disable it by just switching between on/off in llc-coloring=. 
>> However, with your solution,
>> even if I specify llc-coloring=off, it will be enabled because I specified 
>> both llc-size and llc-nr-ways.
>> I think llc-coloring= should have a precedence.
> 
> How do you differentiate from
> llc-size=1M llc-nr-ways=16 llc-coloring=off
> where llc coloring is disabled, and
> llc-size=1M llc-nr-ways=16
> where llc coloring is enabled? I mean, in both situations llc_coloring_enabled
> is going to be set to false.
I was thinking about the following:
diff --git a/xen/common/llc-coloring.c b/xen/common/llc-coloring.c
index 29b75e0e0d6a..b6684a6cf736 100644
--- a/xen/common/llc-coloring.c
+++ b/xen/common/llc-coloring.c
@@ -11,7 +11,12 @@

 #define NR_LLC_COLORS          (1U << CONFIG_LLC_COLORS_ORDER)

-static bool __ro_after_init llc_coloring_enabled;
+/*
+ * -1: not specified (disabled unless llc-size and llc-nr-ways present)
+ *  0: explicitly disabled through cmdline
+ *  1: explicitly enabled through cmdline
+ */
+static int8_t __ro_after_init llc_coloring_enabled = -1;
 boolean_param("llc-coloring", llc_coloring_enabled);

 static unsigned int __initdata llc_size;
@@ -48,7 +53,7 @@ void __init llc_coloring_init(void)
 {
     unsigned int way_size;

-    if ( llc_size && llc_nr_ways )
+    if ( (llc_coloring_enabled < 0) && (llc_size && llc_nr_ways) )
     {
         llc_coloring_enabled = true;
         way_size = llc_size / llc_nr_ways;

~Michal