Re: [PATCH v10 02/12] xen/arm: add initial support for LLC coloring on arm64

[Michal Orzel <michal.orzel@xxxxxxx>]

On 19/11/2024 15:13, Carlo Nonato wrote:
> 
> 
> LLC coloring needs to know the last level cache layout in order to make the
> best use of it. This can be probed by inspecting the CLIDR_EL1 register,
> so the Last Level is defined as the last level visible by this register.
> Note that this excludes system caches in some platforms.
> 
> Static memory allocation and cache coloring are incompatible because static
> memory can't be guaranteed to use only colors assigned to the domain.
> Panic during DomUs creation when both are enabled.
> 
> 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>
> ---
> v10:
> - moved CONFIG_NUMA check in arch/arm/Kconfig
> v9:
> - no changes
> v8:
> - no changes
> v7:
> - only minor changes
> v6:
> - get_llc_way_size() now checks for at least separate I/D caches
> v5:
> - used - instead of _ for filenames
> - moved static-mem check in this patch
> - moved dom0 colors parsing in next patch
> - moved color allocation and configuration in next patch
> - moved check_colors() in next patch
> - colors are now printed in short form
> v4:
> - added "llc-coloring" cmdline option for the boot-time switch
> - dom0 colors are now checked during domain init as for any other domain
> - fixed processor.h masks bit width
> - check for overflow in parse_color_config()
> - check_colors() now checks also that colors are sorted and unique
> ---
>  docs/misc/cache-coloring.rst         | 14 +++++
>  xen/arch/arm/Kconfig                 |  1 +
>  xen/arch/arm/Makefile                |  1 +
>  xen/arch/arm/dom0less-build.c        |  6 +++
>  xen/arch/arm/include/asm/processor.h | 16 ++++++
>  xen/arch/arm/llc-coloring.c          | 78 ++++++++++++++++++++++++++++
>  xen/arch/arm/setup.c                 |  3 ++
>  xen/common/llc-coloring.c            |  2 +-
>  xen/include/xen/llc-coloring.h       |  4 ++
>  9 files changed, 124 insertions(+), 1 deletion(-)
>  create mode 100644 xen/arch/arm/llc-coloring.c
> 
> diff --git a/docs/misc/cache-coloring.rst b/docs/misc/cache-coloring.rst
> index 0fe3830c40..b608284e9b 100644
> --- a/docs/misc/cache-coloring.rst
> +++ b/docs/misc/cache-coloring.rst
> @@ -111,6 +111,20 @@ Auto-probing of LLC specs
> 
>  LLC size and number of ways are probed automatically by default.
> 
> +In the Arm implementation, this is done by inspecting the CLIDR_EL1 register.
> +This means that other system caches that aren't visible there are ignored.
> +
>  LLC specs can be manually set via the above command line parameters. This
>  bypasses any auto-probing and it's used to overcome failing situations, such 
> as
>  flawed probing logic, or for debugging/testing purposes.
> +
> +Known issues and limitations
> +****************************
> +
> +"xen,static-mem" isn't supported when coloring is enabled
> +#########################################################
> +
> +In the domain configuration, "xen,static-mem" allows memory to be statically
> +allocated to the domain. This isn't possible when LLC coloring is enabled,
> +because that memory can't be guaranteed to use only colors assigned to the
> +domain.
> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> index 15b2e4a227..7f0712e674 100644
> --- a/xen/arch/arm/Kconfig
> +++ b/xen/arch/arm/Kconfig
> @@ -8,6 +8,7 @@ config ARM_64
>         depends on !ARM_32
>         select 64BIT
>         select HAS_FAST_MULTIPLY
> +       select HAS_LLC_COLORING if !NUMA
> 
>  config ARM
>         def_bool y
> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> index e4ad1ce851..ccbfc61f88 100644
> --- a/xen/arch/arm/Makefile
> +++ b/xen/arch/arm/Makefile
> @@ -35,6 +35,7 @@ obj-$(CONFIG_IOREQ_SERVER) += ioreq.o
>  obj-y += irq.o
>  obj-y += kernel.init.o
>  obj-$(CONFIG_LIVEPATCH) += livepatch.o
> +obj-$(CONFIG_LLC_COLORING) += llc-coloring.o
>  obj-$(CONFIG_MEM_ACCESS) += mem_access.o
>  obj-y += mm.o
>  obj-y += monitor.o
> diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c
> index f328a044e9..d93a85434e 100644
> --- a/xen/arch/arm/dom0less-build.c
> +++ b/xen/arch/arm/dom0less-build.c
> @@ -5,6 +5,7 @@
>  #include <xen/grant_table.h>
>  #include <xen/iocap.h>
>  #include <xen/libfdt/libfdt.h>
> +#include <xen/llc-coloring.h>
>  #include <xen/sched.h>
>  #include <xen/serial.h>
>  #include <xen/sizes.h>
> @@ -890,7 +891,12 @@ void __init create_domUs(void)
>              panic("No more domain IDs available\n");
> 
>          if ( dt_find_property(node, "xen,static-mem", NULL) )
> +        {
> +            if ( llc_coloring_enabled )
> +                panic("LLC coloring and static memory are incompatible\n");
> +
>              flags |= CDF_staticmem;
> +        }
> 
>          if ( dt_property_read_bool(node, "direct-map") )
>          {
> diff --git a/xen/arch/arm/include/asm/processor.h 
> b/xen/arch/arm/include/asm/processor.h
> index 8e02410465..ef33ea198c 100644
> --- a/xen/arch/arm/include/asm/processor.h
> +++ b/xen/arch/arm/include/asm/processor.h
> @@ -18,6 +18,22 @@
>  #define CTR_IDC_SHIFT       28
>  #define CTR_DIC_SHIFT       29
> 
> +/* CCSIDR Current Cache Size ID Register */
> +#define CCSIDR_LINESIZE_MASK            _AC(0x7, UL)
> +#define CCSIDR_NUMSETS_SHIFT            13
> +#define CCSIDR_NUMSETS_MASK             _AC(0x3fff, UL)
> +#define CCSIDR_NUMSETS_SHIFT_FEAT_CCIDX 32
> +#define CCSIDR_NUMSETS_MASK_FEAT_CCIDX  _AC(0xffffff, UL)
> +
> +/* CSSELR Cache Size Selection Register */
> +#define CSSELR_LEVEL_MASK  _AC(0x7, UL)
Seems unused. If so, please remove.

> +#define CSSELR_LEVEL_SHIFT 1
> +
> +/* CLIDR Cache Level ID Register */
> +#define CLIDR_CTYPEn_SHIFT(n) (3 * ((n) - 1))
> +#define CLIDR_CTYPEn_MASK     _AC(0x7, UL)
> +#define CLIDR_CTYPEn_LEVELS   7
> +
>  #define ICACHE_POLICY_VPIPT  0
>  #define ICACHE_POLICY_AIVIVT 1
>  #define ICACHE_POLICY_VIPT   2
> diff --git a/xen/arch/arm/llc-coloring.c b/xen/arch/arm/llc-coloring.c
> new file mode 100644
> index 0000000000..a7e0907816
> --- /dev/null
> +++ b/xen/arch/arm/llc-coloring.c
> @@ -0,0 +1,78 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Last Level Cache (LLC) coloring support for ARM
> + *
> + * Copyright (C) 2024, Advanced Micro Devices, Inc.
> + * Copyright (C) 2024, Minerva Systems SRL
> + */
> +#include <xen/llc-coloring.h>
> +#include <xen/types.h>
> +
> +#include <asm/processor.h>
> +#include <asm/sysregs.h>
> +
> +/* Return the LLC way size by probing the hardware */
> +unsigned int __init get_llc_way_size(void)
> +{
> +    register_t ccsidr_el1;
> +    register_t clidr_el1 = READ_SYSREG(CLIDR_EL1);
> +    register_t csselr_el1 = READ_SYSREG(CSSELR_EL1);
> +    register_t id_aa64mmfr2_el1 = READ_SYSREG(ID_AA64MMFR2_EL1);
> +    uint32_t ccsidr_numsets_shift = CCSIDR_NUMSETS_SHIFT;
> +    uint32_t ccsidr_numsets_mask = CCSIDR_NUMSETS_MASK;
> +    unsigned int n, line_size, num_sets;
> +
> +    for ( n = CLIDR_CTYPEn_LEVELS; n != 0; n-- )
> +    {
> +        uint8_t ctype_n = (clidr_el1 >> CLIDR_CTYPEn_SHIFT(n)) &
> +                          CLIDR_CTYPEn_MASK;
NIT: align CLIDR_xxx to opening (

Other than that:
Reviewed-by: Michal Orzel <michal.orzel@xxxxxxx>

~Michal