[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH 05/36] xen/arm: compute LLC way size by hardware inspection
On 13/05/2022 15:34, Carlo Nonato wrote: Hi Julien Hi, On 09/03/22 21:12, Julien Grall wrote:Since we don't want to support arm32, should I stick with READ_SYSREG64() or switch to the generic one you+ + +/* 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.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()). You are right for the naming mistakes. Should I add those defines in some specific file or+ 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.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. 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+ == CTR_LOC_NOT_IMPLEMENTED )I am a bit confused this the check here. Shouln't you check that Ctype2 is notn 0 instead?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 isthe best way to test what we want. Would you be able to explain what you want to test? In my understanding, if FEAT_CCIDX is implemented then CCSIDR_EL1 is a 64-bit register.+ 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. 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 accessthat 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
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |