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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 02.05.2024 18:55, Carlo Nonato wrote:
> --- 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
>  
> @@ -513,4 +516,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
> +
> +config MAX_LLC_COLORS_ORDER

May I ask that you consider dropping MAX_ from here (but keeping "maximum"
in prompt and text), thus ...

> --- /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.
> + */
> +#include <xen/keyhandler.h>
> +#include <xen/llc-coloring.h>
> +#include <xen/param.h>
> +
> +#define NR_LLC_COLORS          (1 << CONFIG_MAX_LLC_COLORS_ORDER)

... making this look less strange?

To match up with e.g. max_nr_colors you may also want to use 1U here.

> +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");

Since you won't accept way_size == 0 here, how about it ending up zero in
the initial if()'s body? Even more, don't you want to demand
llc_size % llc_nr_ways == 0 there (thus, together with the enclosing
condition, guaranteeing way_size != 0)?

> +    }
> +
> +    /*
> +     * 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;
> +
> +    if ( max_nr_colors & (max_nr_colors - 1) )
> +        panic("Number of LLC colors (%u) isn't a power of 2\n", 
> max_nr_colors);
> +
> +    if ( max_nr_colors > NR_LLC_COLORS )
> +    {
> +        printk(XENLOG_WARNING
> +               "Number of LLC colors (%u) too big. Using configured max 
> %u\n",
> +               max_nr_colors, NR_LLC_COLORS);
> +        max_nr_colors = NR_LLC_COLORS;
> +    } else if ( max_nr_colors < 2 )
> +        panic("Number of LLC colors %u < 2\n", max_nr_colors);

Ah, here's a check guaranteeing at least the first of the two things asked
about above.

Jan