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

[Julien Grall <julien@xxxxxxx>]

On 13/05/2022 15:34, Carlo Nonato wrote:
> Hi Julien
Hi,

> On 09/03/22 21:12, Julien Grall wrote:
> > > +
> > > +
> > > +/* 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?
If this code is meant to only work on 64-bit, then I would prefer if we 
use READ_SYSREG() and register_t (see more below about READ_SYSREG64 vs 
READ_SYSREG()).
> > > +    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?
I would define them in arch/arm/include/asm/processor.h where we already 
define all the mask for system registers.
> > > +        == 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".
Oh yes. Sorry, I miscalculated the field.

>  Still I don't 
> know if this is
> the best way to test what we want.
Would you be able to explain what you want to test?

> > > +        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.
Technically all the system registers on arm64 are 64-bit registers. That 
said, earlier version of the Arm Arm suggested that some where 32-bit 
when in fact the top bits were RES0.
In Xen, we should try to use register_t and READ_SYSREG() when using 
system register so we don't end up to mask the top by mistake (a future 
revision of the spec may define them).
If the co-processor register is also 64-bit on 32-bit, then we should 
use register_t and READ_SYSREG64().
> 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).
Yes. I will review it if you want to implement it. But I am equally fine 
if you just want to add a check and return an error if FEAT_CCIDX is 
implemented.
Cheers,

--
Julien Grall