Re: [PATCH] xen/arm: scan CLIDR Ctype fields upwards when probing LLC

["Orzel, Michal" <michal.orzel@xxxxxxx>]

On 04-May-26 11:19, Mykola Kvach wrote:
> From: Mykola Kvach <mykola_kvach@xxxxxxxx>
> 
> get_llc_way_size() currently scans CLIDR_EL1 Ctype fields from the
> highest level downwards and stops at the first unified cache it finds.
> 
> However, CLIDR_EL1 describes the cache hierarchy from Ctype1 upwards.
> Arm ARM DDI 0487J.a, D19.2.27 says that once software has seen a
> Ctype value of 0b000 while reading from Ctype1 upwards, no caches
> manageable by the architected set/way maintenance instructions exist at
> further-out levels, and the higher Ctype fields must be ignored.
> 
> The current reverse scan can therefore select a unified cache level from
> a Ctype field above the first no-cache level. Such a field is not part of
> the architecturally described CLIDR/CCSIDR cache hierarchy and should not
> be used for selecting the CCSIDR level.
> 
> Scan Ctype fields from L1 upwards, stop at the first no-cache level, and
> keep the outermost unified cache observed before that point.
> 
> This preserves the result for regular cache hierarchies, while avoiding
> selection of an architecturally ignored Ctype field.
> 
> Fixes: f4985fce6f0b ("xen/arm: add initial support for LLC coloring on arm64")
> Signed-off-by: Mykola Kvach <mykola_kvach@xxxxxxxx>
> ---
> This patch follows the xen-devel discussion:
> https://lists.xenproject.org/archives/html/xen-devel/2026-01/msg00345.html
> 
> In that thread, Michal noted that the reverse scan was a simplification
> rather than an intentional requirement, and that changing the
> implementation would be fine.
> 
> Testing performed:
> - standalone synthetic CLIDR tests covered both regular and pathological
>   Ctype sequences and showed that the forward scan ignores unified cache
>   levels above the first Ctype == 0b000 while the reverse scan can pick
>   them
> - Renesas H3ULCB booted with llc-coloring=on
> ---
>  xen/arch/arm/llc-coloring.c | 22 +++++++++++++++++-----
>  1 file changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/arch/arm/llc-coloring.c b/xen/arch/arm/llc-coloring.c
> index 6f78817c57..3783f4c824 100644
> --- a/xen/arch/arm/llc-coloring.c
> +++ b/xen/arch/arm/llc-coloring.c
> @@ -22,21 +22,33 @@ unsigned int __init get_llc_way_size(void)
>      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-- )
> +    unsigned int n, line_size, num_sets, llc_level = 0;
> +
> +    /*
> +     * CLIDR_EL1 Ctype fields are interpreted from Ctype1 upwards. Once a
> +     * no-cache level is seen, higher Ctype fields are architecturally 
> ignored
> +     * for the CLIDR/CCSIDR set/way manageable cache hierarchy.
> +     *
> +     * Keep the outermost unified cache before that point.
> +     */
> +    for ( n = 1; n <= CLIDR_CTYPEn_LEVELS; n++ )
>      {
>          uint8_t ctype_n = (clidr_el1 >> CLIDR_CTYPEn_SHIFT(n)) &
>                             CLIDR_CTYPEn_MASK;
>  
> +        if ( ctype_n == 0b000 )
> +            break;
> +
>          /* Unified cache (see Arm ARM DDI 0487J.a D19.2.27) */
>          if ( ctype_n == 0b100 )
> -            break;
> +            llc_level = n;
>      }
>  
> -    if ( n == 0 )
> +    if ( !llc_level )
>          return 0;
>  
> +    n = llc_level;
After a loop, n does not carry any meaning, so I find this assignment a bit odd
and confusing to read. Just use llc_level below. With that:
Reviewed-by: Michal Orzel <michal.orzel@xxxxxxx>

~Michal