Hi Julien
On 09/03/22 21:12, Julien Grall wrote:
Signed-off-by: Luca Miccio <lucmic...@gmail.com>
Signed-off-by: Marco Solieri <marco.soli...@minervasys.tech>
Signed-off-by: Stefano Stabellini <stefano.stabell...@xilinx.com>
---
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
