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
