Re: [PATCH 05/36] xen/arm: compute LLC way size by hardware inspection

[Carlo Nonato <carlo.nonato@xxxxxxxxxxxxxxx>]

Hi Julien

On 09/03/22 21:12, Julien Grall wrote:

> > Signed-off-by: Luca Miccio <lucmiccio@xxxxxxxxx>
> > Signed-off-by: Marco Solieri <marco.solieri@xxxxxxxxxxxxxxx>
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxx>
> > ---
> >   xen/arch/arm/coloring.c | 76 +++++++++++++++++++++++++++++++++++++++++
> >   1 file changed, 76 insertions(+)
> >
> > diff --git a/xen/arch/arm/coloring.c b/xen/arch/arm/coloring.c
> > index 8f1cff6efb..e3d490b453 100644
> > --- a/xen/arch/arm/coloring.c
> > +++ b/xen/arch/arm/coloring.c
> > @@ -25,7 +25,10 @@
> >   #include <xen/lib.h>
> >   #include <xen/errno.h>
> >   #include <xen/param.h>
> > +
>
> NIT: I think this belongs to patch #4.
>
> > +#include <asm/sysregs.h>
> Please order the include alphabetically.
>
> >   #include <asm/coloring.h> > +#include <asm/io.h>
> You don't seem to use read*/write* helper. So why do you need this?
> >     /* Number of color(s) assigned to Xen */
> >   static uint32_t xen_col_num;
> > @@ -39,6 +42,79 @@ static uint32_t dom0_col_mask[MAX_COLORS_CELLS];
> >     static uint64_t way_size;
> >   +#define CTR_LINESIZE_MASK 0x7
> > +#define CTR_SIZE_SHIFT 13
> > +#define CTR_SIZE_MASK 0x3FFF
> > +#define CTR_SELECT_L2 1 << 1
> > +#define CTR_SELECT_L3 1 << 2
> > +#define CTR_CTYPEn_MASK 0x7
> > +#define CTR_CTYPE2_SHIFT 3
> > +#define CTR_CTYPE3_SHIFT 6
> > +#define CTR_LLC_ON 1 << 2
> > +#define CTR_LOC_SHIFT 24
> > +#define CTR_LOC_MASK 0x7
> > +#define CTR_LOC_L2 1 << 1
> > +#define CTR_LOC_NOT_IMPLEMENTED 1 << 0
> We already define some CTR_* in processor.h. Please any extra one there.
>
> > +
> > +
> > +/* Return the way size of last level cache by asking the hardware */
> > +static uint64_t get_llc_way_size(void)
> This will break compilation as you are introducing get_llc_way_size() 
> but not using it.
> I would suggest to fold this patch in the next one.
>
> > +{
> > +    uint32_t cache_sel = READ_SYSREG64(CSSELR_EL1);
> The return type for READ_SYSREG64() is uint64_t. That said, the 
> equivalent register on 32bit is CSSELR which is 32-bit. So this should 
> be READ_SYSREG() and the matching type is register_t.
Since we don't want to support arm32, should I stick with 
READ_SYSREG64() or switch to the generic one you
pointed me out?
> > +    uint32_t cache_global_info = READ_SYSREG64(CLIDR_EL1);
> Same remark here. Except the matching register is CLIDR.
>
> > +    uint32_t cache_info;
> > +    uint32_t cache_line_size;
> > +    uint32_t cache_set_num;
> > +    uint32_t cache_sel_tmp;
> > +
> > +    printk(XENLOG_INFO "Get information on LLC\n");
> > +    printk(XENLOG_INFO "Cache CLIDR_EL1: 0x%"PRIx32"\n", 
> > cache_global_info);
> > +
> > +    /* Check if at least L2 is implemented */
> > +    if ( ((cache_global_info >> CTR_LOC_SHIFT) & CTR_LOC_MASK)
> This is a bit confusing. cache_global_info is storing CLIDR_* but you 
> are using macro starting with CTR_*.
> Did you intend to name the macros CLIDR_*?
>
> The same remark goes for the other use of CTR_ below. The name of the 
> macros should match the register they are meant to be used on.
You are right for the naming mistakes. Should I add those defines in 
some specific file or
can they stay here?
> > +        == CTR_LOC_NOT_IMPLEMENTED )
> I am a bit confused this the check here. Shouln't you check that 
> Ctype2 is notn 0 instead?
I should check a little bit better how this automatic probing thing 
actually works
and we also have to clarify better what is the LLC for us, so that I 
know what we
should really test for in this function. Probably you're right though.
> > +    {
> > +        printk(XENLOG_ERR "ERROR: L2 Cache not implemented\n");
> > +        return 0;
> > +    }
> > +
> > +    /* Save old value of CSSELR_EL1 */
> > +    cache_sel_tmp = cache_sel;
> > +
> > +    /* Get LLC index */
> > +    if ( ((cache_global_info >> CTR_CTYPE2_SHIFT) & CTR_CTYPEn_MASK)
> > +        == CTR_LLC_ON )
> I don't understand this check. You define CTR_LLC_ON to 1 << 2. So it 
> would be 0b10. From the field you checked, this value mean "Data Cache 
> Only". How is this indicating the which level to chose?
> But then in patch #4 you wrote we will do cache coloring on L2. So why 
> are we selecting L3?
1 << 2 is actually 0b100 which stands for "Unified cache". Still I don't 
know if this is
the best way to test what we want.
> > +        cache_sel = CTR_SELECT_L2;
> > +    else
> > +        cache_sel = CTR_SELECT_L3;
> > +
> > +    printk(XENLOG_INFO "LLC selection: %u\n", cache_sel);
> > +    /* Select the correct LLC in CSSELR_EL1 */
> > +    WRITE_SYSREG64(cache_sel, CSSELR_EL1);
> This should be WRITE_SYSREG().
>
> > +
> > +    /* Ensure write */
> > +    isb();
> > +
> > +    /* Get info about the LLC */
> > +    cache_info = READ_SYSREG64(CCSIDR_EL1);
> > +
> > +    /* ARM TRM: (Log2(Number of bytes in cache line)) - 4. */
> From my understanding "TRM" in the Arm world refers to a specific 
> processor. In this case we want to quote the spec. So we usually say 
> "Arm Arm".
> > +    cache_line_size = 1 << ((cache_info & CTR_LINESIZE_MASK) + 4);
> > +    /* ARM TRM: (Number of sets in cache) - 1 */
> > +    cache_set_num = ((cache_info >> CTR_SIZE_SHIFT) & CTR_SIZE_MASK) 
> > + 1;
> The shifts here are assuming that FEAT_CCIDX is not implemented. I 
> would be OK if we decide to not support cache coloring on such 
> platform. However, we need to return an error if a user tries to use 
> cache coloring on such platform.
In my understanding, if FEAT_CCIDX is implemented then CCSIDR_EL1 is a 
64-bit register.
So it's just a matter of probing for FEAT_CCIDX and in that case 
changing the way we access
that register (since the layout changes too).

Thanks.

- Carlo Nonato