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

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

Hi Michal,

Thank you for the feedback.

On Fri, May 22, 2026 at 9:41 AM Orzel, Michal <michal.orzel@xxxxxxx> wrote:
>
>
>
> 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:

Ack.

Best regards,
Mykola