Re: [PATCH v9 01/13] xen/common: add cache coloring common code

[Jan Beulich <jbeulich@xxxxxxxx>]

On 25.10.2024 11:50, 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.
> MAX_LLC_COLORS_ORDER Kconfig option has a range maximum of 10 (2^10 = 1024)

Is the MAX_ here stale ...

> 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>
> ---
> v9:
> - dropped _MAX_ from CONFIG_MAX_LLC_COLORS_ORDER

... with this change?

> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -1708,6 +1708,43 @@ This option is intended for debugging purposes only.  
> Enable MSR_DEBUGCTL.LBR
>  in hypervisor context to be able to dump the Last Interrupt/Exception To/From
>  record with other registers.
>  
> +### llc-coloring (arm64)
> +> `= <boolean>`
> +
> +> Default: `false`
> +
> +Flag to enable or disable LLC coloring support at runtime. This option is
> +available only when `CONFIG_LLC_COLORING` is enabled. See the general
> +cache coloring documentation for more info.
> +
> +### 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 these two options implies "llc-coloring=on".

Nit: Both here and ...

> +### llc-size (arm64)
> +> `= <size>`
> +
> +> Default: `Obtained from hardware`
> +
> +Specify the size 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-nr-ways" must be set, otherwise the default will be
> +used. Note that using these two options implies "llc-coloring=on".

... here, maybe better s/these two/both/?

> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -71,6 +71,9 @@ config HAS_IOPORTS
>  config HAS_KEXEC
>       bool
>  
> +config HAS_LLC_COLORING
> +     bool
> +
>  config HAS_PIRQ
>       bool
>  
> @@ -516,4 +519,23 @@ config TRACEBUFFER
>         to be collected at run time for debugging or performance analysis.
>         Memory and execution overhead when not active is minimal.
>  
> +config LLC_COLORING
> +     bool "Last Level Cache (LLC) coloring" if EXPERT
> +     depends on HAS_LLC_COLORING
> +     depends on !NUMA

Instead of this dependency, wouldn't it be more natural to suppress the
setting of HAS_LLC_COLORING by an arch when NUMA is on?

> --- /dev/null
> +++ b/xen/common/llc-coloring.c
> @@ -0,0 +1,111 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Last Level Cache (LLC) coloring common code
> + *
> + * Copyright (C) 2022 Xilinx Inc.

Does this need updating (if it can't be dropped)?

> + */
> +#include <xen/keyhandler.h>
> +#include <xen/llc-coloring.h>
> +#include <xen/param.h>
> +
> +#define NR_LLC_COLORS          (1U << CONFIG_LLC_COLORS_ORDER)
> +
> +static bool __ro_after_init llc_coloring_enabled;
> +boolean_param("llc-coloring", llc_coloring_enabled);
> +
> +static unsigned int __initdata llc_size;
> +size_param("llc-size", llc_size);
> +static unsigned int __initdata llc_nr_ways;
> +integer_param("llc-nr-ways", llc_nr_ways);
> +/* Number of colors available in the LLC */
> +static unsigned int __ro_after_init max_nr_colors;
> +
> +static void print_colors(const unsigned int *colors, unsigned int num_colors)

Just to mention it here as well (I mentioned it elsewhere in the past):
Personally I think that when function parameters denote array, array
notation would also better be used. I.e. "const unsigned int colors[]"
here. That'll then probably also bring us closer to using the upcoming
(in gcc) counted_by attribute.

> +void __init llc_coloring_init(void)
> +{
> +    unsigned int way_size;
> +
> +    if ( llc_size && llc_nr_ways )
> +    {
> +        llc_coloring_enabled = true;
> +        way_size = llc_size / llc_nr_ways;
> +    }
> +    else if ( !llc_coloring_enabled )
> +        return;
> +    else
> +    {
> +        way_size = get_llc_way_size();
> +        if ( !way_size )
> +            panic("LLC probing failed and 'llc-size' or 'llc-nr-ways' 
> missing\n");
> +    }
> +
> +    /*
> +     * The maximum number of colors must be a power of 2 in order to 
> correctly
> +     * map them to bits of an address.
> +     */
> +    max_nr_colors = way_size >> PAGE_SHIFT;

This discards low bits of the quotient calculated above, bearing a certain
risk that ...

> +    if ( max_nr_colors & (max_nr_colors - 1) )
> +        panic("Number of LLC colors (%u) isn't a power of 2\n", 
> max_nr_colors);

... this panic() wrongly doesn't trigger.

Jan