[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



Hi,

On 04/03/2022 17:46, Marco Solieri wrote:
From: Luca Miccio <lucmiccio@xxxxxxxxx>

The size of the LLC way is a crucial parameter for the cache coloring
support, since it determines the maximum number of available colors on
the platform.  This parameter can currently be retrieved only from
the way_size bootarg and it is prone to misconfiguration nullifying the
coloring mechanism and breaking cache isolation.

Reading this sentence, I think the command line option should be introduced after this patch (assuming this is necessary). This will avoid undoing/fixing a "bug" that was introduced by the same series.


Add an alternative and more safe method to retrieve the way size by
directly asking the hardware, namely using CCSIDR_EL1 and CSSELR_EL1
registers.

This method has to check also if at least L2 is implemented in the
hardware since there are scenarios where only L1 cache is availble, e.g,

In the previous patch, the description for the Kconfig suggests that the cache coloring will only happen on L2. But here you are also adding L1. So I think the documentation needs to be updated.

Typo: s/availble/available/

QEMU.

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.

+    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.

+        == CTR_LOC_NOT_IMPLEMENTED )

I am a bit confused this the check here. Shouln't you check that Ctype2 is notn 0 instead?

+    {
+        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?

+        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.

+
+    printk(XENLOG_INFO "Cache line size: %u bytes\n", cache_line_size);
+    printk(XENLOG_INFO "Cache sets num: %u\n", cache_set_num);
+
+    /* Restore value in CSSELR_EL1 */
+    WRITE_SYSREG64(cache_sel_tmp, CSSELR_EL1);
+
+    /* Ensure write */
+    isb();
+
+    return (cache_line_size * cache_set_num);
+}
+
  /*************************
   * PARSING COLORING BOOTARGS
   */

Cheers,

--
Julien Grall



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.