Intel Code/Data Prioritization(CDP) feature enabling

Intel Code/Data Prioritization(CDP) feature is based on CAT. cdp_enabled is added to CAT socket info to indicate CDP is on or off on the socket, note that cos_max would be half when CDP is on. struct psr_cat_cbm is extended to support CDP operation. Extend psr_get_cat_l3_info sysctl to get CDP status.

Signed-off-by He Chen <[email protected]>

---
 xen/arch/x86/psr.c              | 84 ++++++++++++++++++++++++++++++++++-------
 xen/arch/x86/sysctl.c           |  5 ++-
 xen/include/asm-x86/msr-index.h |  3 ++
 xen/include/asm-x86/psr.h       | 11 +++++-
 xen/include/public/sysctl.h     |  2 +
 5 files changed, 89 insertions(+), 16 deletions(-)

xen/arch/x86/psr.c +0 -0

index c0daa2e..ba0f726 100644

xen/arch/x86/psr.c +8 -1

 #include <asm/psr.h>
 
 #define PSR_CMT        (1<<0)
 #define PSR_CAT        (1<<1)
+#define PSR_CDP        (1<<2)
 
 struct psr_cat_cbm {
-    uint64_t cbm;
+    union {
+        uint64_t cbm;
+        struct {
+            uint64_t code;
+            uint64_t data;
+        } cdp;
+    } u;
     unsigned int ref;
 };
 

xen/arch/x86/psr.c +1 -0

@@ -470,6 +500,7 @@ static void cat_cpu_init(void)
     struct psr_cat_socket_info *info;
     unsigned int socket;
     unsigned int cpu = smp_processor_id();
+    uint64_t val;
     const struct cpuinfo_x86 *c = cpu_data + cpu;
 
     if ( !cpu_has(c, X86_FEATURE_CAT) || c->cpuid_level < PSR_CPUID_LEVEL_CAT )

@@ -490,13 +521,34 @@ static void cat_cpu_init(void)
         info->cos_to_cbm = temp_cos_to_cbm;

xen/arch/x86/psr.c +9 -0

+
+            /* Cut half of cos_max when CDP enabled */
+            info->cos_max = info->cos_max / 2;
+
+            info->cdp_enabled = 1;
+            set_bit(socket, cdp_socket_enable);
+            printk(XENLOG_INFO "CDP: enabled on socket %u, cos_max:%u, cbm_len:%u\n",
+                   socket, info->cos_max, info->cbm_len);
+        }
     }
 }
 

xen/include/asm-x86/msr-index.h +0 -0

index e9c4723..402e7a7 100644

Quoted 15 lines · He Chen

Intel Code/Data Prioritization(CDP) feature is based on CAT. cdp_enabled is added to CAT socket info to indicate CDP is on or off on the socket, note that cos_max would be half when CDP is on. struct psr_cat_cbm is extended to support CDP operation. Extend psr_get_cat_l3_info sysctl to get CDP status.

Signed-off-by He Chen <[email protected]>

---
 xen/arch/x86/psr.c              | 84 ++++++++++++++++++++++++++++++++++-------
 xen/arch/x86/sysctl.c           |  5 ++-
 xen/include/asm-x86/msr-index.h |  3 ++
 xen/include/asm-x86/psr.h       | 11 +++++-
 xen/include/public/sysctl.h     |  2 +
 5 files changed, 89 insertions(+), 16 deletions(-)

xen/arch/x86/psr.c line 20

Quoted 9 lines

xen/arch/x86/psr.c +1 -0

index c0daa2e..ba0f726 100644
@@ -17,13 +17,21 @@
 #include <xen/cpu.h>
 #include <xen/err.h>
 #include <xen/sched.h>
+#include <xen/domain.h>

Is this still needed?

Quoted 19 lines

xen/arch/x86/psr.c +8 -1

 #include <asm/psr.h>
 
 #define PSR_CMT        (1<<0)
 #define PSR_CAT        (1<<1)
+#define PSR_CDP        (1<<2)
 
 struct psr_cat_cbm {
-    uint64_t cbm;
+    union {
+        uint64_t cbm;
+        struct {
+            uint64_t code;
+            uint64_t data;
+        } cdp;
+    } u;
     unsigned int ref;
 };
 

xen/arch/x86/psr.c struct psr_cat_socket_info { · line 42

Quoted 3 lines

xen/arch/x86/psr.c +0 -0

@@ -32,6 +40,7 @@ struct psr_cat_socket_info {
     unsigned int cos_max;
     struct psr_cat_cbm *cos_to_cbm;
     spinlock_t cbm_lock;

As you defined the cdp stauts for each socket here ...

Quoted 6 lines

xen/arch/x86/psr.c +1 -0

+    bool_t cdp_enabled;
 };
 
 struct psr_assoc {

@@ -43,6 +52,7 @@ struct psr_cmt *__read_mostly psr_cmt;
 

xen/arch/x86/psr.c struct psr_cmt *__read_mostly psr_cmt; · line 55

Quoted 3 lines

xen/arch/x86/psr.c +1 -0

 static unsigned long *__read_mostly cat_socket_enable;
 static struct psr_cat_socket_info *__read_mostly cat_socket_info;
+static unsigned long *__read_mostly cdp_socket_enable;

... this one then perhaps is redundency. If only one is need then I really like to keep the latter one.

Quoted 15 lines

xen/arch/x86/psr.c +2 -1

@@ -470,6 +500,7 @@ static void cat_cpu_init(void)
     struct psr_cat_socket_info *info;
     unsigned int socket;
     unsigned int cpu = smp_processor_id();
+    uint64_t val;
     const struct cpuinfo_x86 *c = cpu_data + cpu;
 
     if ( !cpu_has(c, X86_FEATURE_CAT) || c->cpuid_level < PSR_CPUID_LEVEL_CAT )

@@ -490,13 +521,34 @@ static void cat_cpu_init(void)
         info->cos_to_cbm = temp_cos_to_cbm;
         temp_cos_to_cbm = NULL;
         /* cos=0 is reserved as default cbm(all ones). */
-        info->cos_to_cbm[0].cbm = (1ull << info->cbm_len) - 1;
+        info->cos_to_cbm[0].u.cbm = (1ull << info->cbm_len) - 1;
 

xen/arch/x86/psr.c static void cat_cpu_init(void) · line 530

Quoted 5 lines

xen/arch/x86/psr.c +0 -0

         spin_lock_init(&info->cbm_lock);
 
         set_bit(socket, cat_socket_enable);
         printk(XENLOG_INFO "CAT: enabled on socket %u, cos_max:%u, cbm_len:%u\n",
                socket, info->cos_max, info->cbm_len);

If CDP is enable below, then this information is quite misleading.

Quoted 10 lines

xen/arch/x86/psr.c +10 -0

+
+        if ( (ecx & PSR_CAT_CDP_CAPABILITY) && (opt_psr & PSR_CDP) )
+        {
+            if ( test_bit(socket, cdp_socket_enable) )
+                return;
+
+            rdmsrl(MSR_IA32_PSR_L3_QOS_CFG, val);
+            wrmsrl(MSR_IA32_PSR_L3_QOS_CFG, val | 1 << PSR_L3_QOS_CDP_ENABLE_BIT);
+
+            /* No need to write register since CBMs are fully open as default by HW */

xen/arch/x86/psr.c static void cat_cpu_init(void) · line 542

Quoted 2 lines

xen/arch/x86/psr.c +2 -0

+            info->cos_to_cbm[0].u.cdp.code = (1ull << info->cbm_len) - 1;
+            info->cos_to_cbm[0].u.cdp.data = (1ull << info->cbm_len) - 1;

If I remember correctly, HW is supposed to boot as CAT mode by default and only IA32_L3_QOS_Mask_0 is all ones, other mask msrs however are not mentioned in the spec which then may contain arbitrary value. So I guesss as least you need write all ones to IA32_L3_QOS_Mask_1 explicitly and that should be done before you enabled CDP.

Quoted 17 lines

xen/arch/x86/psr.c +9 -0

+
+            /* Cut half of cos_max when CDP enabled */
+            info->cos_max = info->cos_max / 2;
+
+            info->cdp_enabled = 1;
+            set_bit(socket, cdp_socket_enable);
+            printk(XENLOG_INFO "CDP: enabled on socket %u, cos_max:%u, cbm_len:%u\n",
+                   socket, info->cos_max, info->cbm_len);
+        }
     }
 }
 

xen/include/asm-x86/msr-index.h +0 -0

index e9c4723..402e7a7 100644

xen/include/asm-x86/msr-index.h line 334

Quoted 7 lines

xen/include/asm-x86/msr-index.h +3 -0

@@ -328,7 +328,10 @@
 #define MSR_IA32_CMT_EVTSEL		0x00000c8d
 #define MSR_IA32_CMT_CTR		0x00000c8e
 #define MSR_IA32_PSR_ASSOC		0x00000c8f
+#define MSR_IA32_PSR_L3_QOS_CFG	0x00000c81
 #define MSR_IA32_PSR_L3_MASK(n)	(0x00000c90 + (n))
+#define MSR_IA32_PSR_L3_MASK_CBM_CODE(n)	(0x00000c90 + (n * 2 + 1))
+#define MSR_IA32_PSR_L3_MASK_CBM_DATA(n)	(0x00000c90 + (n * 2))

I guess 'CBM' can be removed from these two macros, e.g. MSR_IA32_PSR_L3_MASK_CODE & MSR_IA32_PSR_L3_MASK_DATA

Chao

CDP extends CAT and provides the capacity to control L3 code & data cache. With CDP, one COS correspond to two CMBs(code & data). cbm_type is added to support distinguish different CBM operation. Besides, new domctl cmds are introdunced to support set/get CDP CBM. Some CAT functions to operation CBMs are extended to support CDP.

Signed-off-by He Chen <[email protected]>

---
 xen/arch/x86/domctl.c       |  32 ++++++++-
 xen/arch/x86/psr.c          | 163 ++++++++++++++++++++++++++++++++++----------
 xen/include/asm-x86/psr.h   |  12 +++-
 xen/include/public/domctl.h |   4 ++
 4 files changed, 170 insertions(+), 41 deletions(-)

xen/arch/x86/domctl.c +10 -3

index bf62a88..734fddb 100644
--- a/xen/arch/x86/domctl.c
 
-int psr_get_l3_cbm(struct domain *d, unsigned int socket, uint64_t *cbm)
+int psr_get_l3_cbm(struct domain *d, unsigned int socket,
+                   uint64_t *cbm, enum cbm_type type)
 {
     struct psr_cat_socket_info *info = get_cat_socket_info(socket);
 
     if ( IS_ERR(info) )
         return PTR_ERR(info);
 
-    *cbm = info->cos_to_cbm[d->arch.psr_cos_ids[socket]].u.cbm;
+    if ( type == PSR_CBM_TYPE_L3 )
+    {
+        if ( info->cdp_enabled )
+            return -EXDEV;
+        *cbm = info->cos_to_cbm[d->arch.psr_cos_ids[socket]].u.cbm;
+    }
+    else if ( type == PSR_CBM_TYPE_L3_CODE )
+    {

 
     return 0;

}

xen/arch/x86/domctl.c +7 -3

@@ -371,65 +389,136 @@ static int write_l3_cbm(unsigned int socket, unsigned int cos,
     return 0;
 }
 
-int psr_set_l3_cbm(struct domain *d, unsigned int socket, uint64_t cbm)
+static int find_cos(struct psr_cat_cbm *map, int cos_max,
+                    uint64_t cbm_code, uint64_t cbm_data, bool_t cdp_enabled)
 {
-    unsigned int old_cos, cos;
-    struct psr_cat_cbm *map, *found = NULL;
+    unsigned int cos;
+
+    if ( !cdp_enabled )
+    {
+        for ( cos = 0; cos <= cos_max; cos++ )

xen/arch/x86/domctl.c +12 -0

+    for ( cos = 1; cos <= cos_max; cos++ )
+        if ( map[cos].ref == 0 )
+            return cos;
+
+    return -ENOENT;
+}
+
+int psr_set_l3_cbm(struct domain *d, unsigned int socket,
+                   uint64_t cbm, enum cbm_type type)
+{
+    unsigned int old_cos, cos_max;
+    int cos, ret;

xen/arch/x86/domctl.c +57 -29

     {
-        /* If still not found, then keep unused one. */
-        if ( !found && cos != 0 && map[cos].ref == 0 )
-            found = map + cos;
-        else if ( map[cos].u.cbm == cbm )
-        {
-            if ( unlikely(cos == old_cos) )
-            {
-                ASSERT(cos == 0 || map[cos].ref != 0);
-                spin_unlock(&info->cbm_lock);
-                return 0;
-            }
-            found = map + cos;
+        case PSR_CBM_TYPE_L3:
+            if ( info->cdp_enabled )
+                return -EINVAL;
+            cbm_code = cbm;
+            cbm_data = cbm;
             break;
-        }
-    }
 
-    /* If old cos is referred only by the domain, then use it. */
-    if ( !found && map[old_cos].ref == 1 )
-        found = map + old_cos;
+        case PSR_CBM_TYPE_L3_CODE:
+            if ( !info->cdp_enabled )
+                return -EINVAL;
+            cbm_code = cbm;
+            cbm_data = map[old_cos].u.cdp.data;
+            break;
 
-    if ( !found )
-    {
-        spin_unlock(&info->cbm_lock);
-        return -EOVERFLOW;
+        case PSR_CBM_TYPE_L3_DATA:
+            if ( !info->cdp_enabled )
+                return -EINVAL;
+            cbm_code = map[old_cos].u.cdp.code;
+            cbm_data = cbm;
+            break;
+
+        default:
+            return -EINVAL;
     }
 
-    cos = found - map;
-    if ( found->u.cbm != cbm )
+    spin_lock(&info->cbm_lock);
+    cos = find_cos(map, cos_max, cbm_code, cbm_data, info->cdp_enabled);
+    if ( cos >= 0 )
     {
-        int ret = write_l3_cbm(socket, cos, cbm, 0, 0);
-
-        if ( ret )
+        if ( unlikely(cos == old_cos) )
         {
             spin_unlock(&info->cbm_lock);
-            return ret;
+            return 0;
+        }
+    }
+    else
+    {
+        cos = pick_avail_cos(map, cos_max, old_cos);
+        if ( cos < 0 )
+        {
+            spin_unlock(&info->cbm_lock);
+            return cos;
+        }
+
+        /* We try to avoid writing MSR */
+        if ( info->cdp_enabled )
+        {
+            if ( map[cos].u.cdp.code == cbm_code &&
+                 map[cos].u.cdp.data == cbm_data )
+                need_write = 0;
+        }
+        else
+            need_write = map[cos].u.cbm == cbm_code ? 0 : 1;
+
+        if ( need_write )
+        {
+            ret = write_l3_cbm(socket, cos, cbm_code, cbm_data, info->cdp_enabled);
+            if ( ret )
+            {
+                spin_unlock(&info->cbm_lock);
+                return ret;
+            }
+            map[cos].u.cdp.code = cbm_code;
+            map[cos].u.cdp.data = cbm_data;
         }
-        found->u.cbm = cbm;
     }
 
-    found->ref++;
+    map[cos].ref++;
     map[old_cos].ref--;
     spin_unlock(&info->cbm_lock);
 

Quoted 1 line

Quoted 1 line · On 09.09.15 at 09:24, <[email protected]> wrote:

Quoted 22 lines · He Chen

xen/arch/x86/psr.c +14 -2

-int psr_get_l3_cbm(struct domain *d, unsigned int socket, uint64_t *cbm)
+int psr_get_l3_cbm(struct domain *d, unsigned int socket,
+                   uint64_t *cbm, enum cbm_type type)
 {
     struct psr_cat_socket_info *info = get_cat_socket_info(socket);
 
     if ( IS_ERR(info) )
         return PTR_ERR(info);
 
-    *cbm = info->cos_to_cbm[d->arch.psr_cos_ids[socket]].u.cbm;
+    if ( type == PSR_CBM_TYPE_L3 )
+    {
+        if ( info->cdp_enabled )
+            return -EXDEV;
+        *cbm = info->cos_to_cbm[d->arch.psr_cos_ids[socket]].u.cbm;
+    }
+    else if ( type == PSR_CBM_TYPE_L3_CODE )
+    {
+        if ( !info->cdp_enabled )
+            return -EXDEV;
+        *cbm = info->cos_to_cbm[d->arch.psr_cos_ids[socket]].u.cdp.code;
+    }

xen/arch/x86/psr.c int psr_get_cat_l3_info(unsigned int socket, uint32_t *cbm_len, · line 318

Quoted 6 lines

xen/arch/x86/psr.c +6 -0

+    else
+    {
+        if ( !info->cdp_enabled )
+            return -EXDEV;
+        *cbm = info->cos_to_cbm[d->arch.psr_cos_ids[socket]].u.cdp.data;
+    }

Could I suggest to use switch? And the check can be done in advance.

Quoted 20 lines

xen/arch/x86/psr.c +12 -3

@@ -371,65 +389,136 @@ static int write_l3_cbm(unsigned int socket, unsigned int cos,
     return 0;
 }
 
-int psr_set_l3_cbm(struct domain *d, unsigned int socket, uint64_t cbm)
+static int find_cos(struct psr_cat_cbm *map, int cos_max,
+                    uint64_t cbm_code, uint64_t cbm_data, bool_t cdp_enabled)
 {
-    unsigned int old_cos, cos;
-    struct psr_cat_cbm *map, *found = NULL;
+    unsigned int cos;
+
+    if ( !cdp_enabled )
+    {
+        for ( cos = 0; cos <= cos_max; cos++ )
+            if ( map[cos].ref && map[cos].u.cbm == cbm_code )
+                return cos;
+    }
+    else
+    {

xen/arch/x86/psr.c static int write_l3_cbm(unsigned int socket, unsigned int cos, · line 409

Quoted 5 lines

xen/arch/x86/psr.c +5 -0

+        for ( cos = 0; cos <= cos_max; cos++ )
+            if ( map[cos].ref && map[cos].u.cdp.code == cbm_code &&
+                 map[cos].u.cdp.data == cbm_data )
+                return cos;
+    }

I guess the '{' and '}' can be omitted if the body contains only one sentence.

This is the xl/xc changes to support Intel Code/Data Prioritization. CAT xl commands to set/get CBMs are extended to support CDP.

Signed-off-by He Chen <[email protected]>

---

tools/libxl/libxl.h +7 -0

index 5f9047c..e4eb4df 100644
@@ -796,6 +796,13 @@ void libxl_mac_copy(libxl_ctx *ctx, libxl_mac *dst, libxl_mac *src);
  * If this is defined, the Cache Allocation Technology feature is supported.
  */
 #define LIBXL_HAVE_PSR_CAT 1
+
+/*
+ * LIBXL_HAVE_PSR_CDP
+ *
+ * If this is defined, the Code/Data Prioritization feature is supported.
+ */
+#define LIBXL_HAVE_PSR_CDP 1
 #endif
 
 /*

tools/libxl/libxl.h +0 -0

 /* misc */
 
 /* Each of these sets or clears the flag according to whether the

tools/libxl/libxl_psr.c +17 -1

index 3378239..26939a5 100644
@@ -297,6 +297,7 @@ int libxl_psr_cat_set_cbm(libxl_ctx *ctx, uint32_t domid,
     GC_INIT(ctx);
     int rc;
     int socketid, nr_sockets;
+    libxl_psr_cat_info *info;
 
     rc = libxl__count_physical_sockets(gc, &nr_sockets);
     if (rc) {

@@ -304,14 +305,41 @@ int libxl_psr_cat_set_cbm(libxl_ctx *ctx, uint32_t domid,
         goto out;
     }
 
+    rc = libxl_psr_cat_get_l3_info(ctx, &info, &nr_sockets);
+    if (rc) {
+        LOGE(ERROR, "Failed to get cat info");
+        goto out;
+    }
+
+    if (!info->cdp_enabled &&
+       (type == LIBXL_PSR_CBM_TYPE_L3_CODE ||
+        type == LIBXL_PSR_CBM_TYPE_L3_DATA))
+    {
+            LOGE(ERROR, "Unable to set Code/Data CBM with CDP disabled");
+            rc = EINVAL;
+            free(info);
+            goto out;
+    }
+
     libxl_for_each_set_bit(socketid, *target_map) {
         if (socketid >= nr_sockets)
             break;
-        if (xc_psr_cat_set_domain_data(ctx->xch, domid, type, socketid, cbm)) {

tools/libxl/libxl_psr.c +2 -0

+        } else if (xc_psr_cat_set_domain_data(ctx->xch, domid, type, socketid, cbm)) {
             libxl__psr_cat_log_err_msg(gc, errno);
             rc = ERROR_FAIL;
         }
     }
+    free(info);
 
 out:
     GC_FREE;

Quoted 27 lines · He Chen

This is the xl/xc changes to support Intel Code/Data Prioritization. CAT xl commands to set/get CBMs are extended to support CDP.

Signed-off-by He Chen <[email protected]>

---

tools/libxl/libxl.h +7 -0

index 5f9047c..e4eb4df 100644
@@ -796,6 +796,13 @@ void libxl_mac_copy(libxl_ctx *ctx, libxl_mac *dst, libxl_mac *src);
  * If this is defined, the Cache Allocation Technology feature is supported.
  */
 #define LIBXL_HAVE_PSR_CAT 1
+
+/*
+ * LIBXL_HAVE_PSR_CDP
+ *
+ * If this is defined, the Code/Data Prioritization feature is supported.
+ */
+#define LIBXL_HAVE_PSR_CDP 1
 #endif
 
 /*

@@ -1729,6 +1736,9 @@ int libxl_psr_cat_get_l3_info(libxl_ctx *ctx, libxl_psr_cat_info **info,
 void libxl_psr_cat_info_list_free(libxl_psr_cat_info *list, int nr);
 #endif
 

tools/libxl/libxl.h int libxl_psr_cat_get_l3_info(libxl_ctx *ctx, libxl_psr_cat_info **info, · line 1741

Quoted 3 lines

tools/libxl/libxl.h +3 -0

+#ifdef LIBXL_HAVE_PSR_CDP
+#endif
+

Why this? There are several of them in this patch.

Quoted 39 lines

tools/libxl/libxl.h +0 -0

 /* misc */
 
 /* Each of these sets or clears the flag according to whether the

tools/libxl/libxl_psr.c +17 -1

index 3378239..26939a5 100644
@@ -297,6 +297,7 @@ int libxl_psr_cat_set_cbm(libxl_ctx *ctx, uint32_t domid,
     GC_INIT(ctx);
     int rc;
     int socketid, nr_sockets;
+    libxl_psr_cat_info *info;
 
     rc = libxl__count_physical_sockets(gc, &nr_sockets);
     if (rc) {

@@ -304,14 +305,41 @@ int libxl_psr_cat_set_cbm(libxl_ctx *ctx, uint32_t domid,
         goto out;
     }
 
+    rc = libxl_psr_cat_get_l3_info(ctx, &info, &nr_sockets);
+    if (rc) {
+        LOGE(ERROR, "Failed to get cat info");
+        goto out;
+    }
+
+    if (!info->cdp_enabled &&
+       (type == LIBXL_PSR_CBM_TYPE_L3_CODE ||
+        type == LIBXL_PSR_CBM_TYPE_L3_DATA))
+    {
+            LOGE(ERROR, "Unable to set Code/Data CBM with CDP disabled");
+            rc = EINVAL;
+            free(info);
+            goto out;
+    }
+
     libxl_for_each_set_bit(socketid, *target_map) {
         if (socketid >= nr_sockets)
             break;
-        if (xc_psr_cat_set_domain_data(ctx->xch, domid, type, socketid, cbm)) {

tools/libxl/libxl_psr.c int libxl_psr_cat_set_cbm(libxl_ctx *ctx, uint32_t domid, · line 336

Quoted 10 lines

tools/libxl/libxl_psr.c +10 -0

+        if (info->cdp_enabled && type == LIBXL_PSR_CBM_TYPE_L3_CBM)
+        {
+            if(xc_psr_cat_set_domain_data(ctx->xch, domid,
+               LIBXL_PSR_CBM_TYPE_L3_CODE, socketid, cbm) ||
+               xc_psr_cat_set_domain_data(ctx->xch, domid,
+               LIBXL_PSR_CBM_TYPE_L3_DATA, socketid, cbm))
+            {
+                libxl__psr_cat_log_err_msg(gc, errno);
+                rc = ERROR_FAIL;
+            }

Will you merge the two if's?

Chao

tools/libxl/libxl_psr.c int libxl_psr_cat_set_cbm(libxl_ctx *ctx, uint32_t domid, · line 345

Quoted 9 lines

tools/libxl/libxl_psr.c +2 -0

+        } else if (xc_psr_cat_set_domain_data(ctx->xch, domid, type, socketid, cbm)) {
             libxl__psr_cat_log_err_msg(gc, errno);
             rc = ERROR_FAIL;
         }
     }
+    free(info);
 
 out:
     GC_FREE;

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Quoted 1 line

Quoted 1 line · On 09.09.15 at 10:10, <[email protected]> wrote:

Quoted 29 lines · Chao Peng

Quoted 28 lines · He Chen

tools/libxl/libxl_psr.c +20 -1

@@ -304,14 +305,41 @@ int libxl_psr_cat_set_cbm(libxl_ctx *ctx, uint32_t domid,
         goto out;
     }
 
+    rc = libxl_psr_cat_get_l3_info(ctx, &info, &nr_sockets);
+    if (rc) {
+        LOGE(ERROR, "Failed to get cat info");
+        goto out;
+    }
+
+    if (!info->cdp_enabled &&
+       (type == LIBXL_PSR_CBM_TYPE_L3_CODE ||
+        type == LIBXL_PSR_CBM_TYPE_L3_DATA))
+    {
+            LOGE(ERROR, "Unable to set Code/Data CBM with CDP disabled");
+            rc = EINVAL;
+            free(info);
+            goto out;
+    }
+
     libxl_for_each_set_bit(socketid, *target_map) {
         if (socketid >= nr_sockets)
             break;
-        if (xc_psr_cat_set_domain_data(ctx->xch, domid, type, socketid, cbm)) {
+        if (info->cdp_enabled && type == LIBXL_PSR_CBM_TYPE_L3_CBM)
+        {
+            if(xc_psr_cat_set_domain_data(ctx->xch, domid,
+               LIBXL_PSR_CBM_TYPE_L3_CODE, socketid, cbm) ||

Quoted 8 lines

tools/libxl/libxl_psr.c int libxl_psr_cat_set_cbm(libxl_ctx *ctx, uint32_t domid, · line 336

Quoted 6 lines

tools/libxl/libxl_psr.c +6 -0

+               xc_psr_cat_set_domain_data(ctx->xch, domid,
+               LIBXL_PSR_CBM_TYPE_L3_DATA, socketid, cbm))
+            {
+                libxl__psr_cat_log_err_msg(gc, errno);
+                rc = ERROR_FAIL;
+            }

Will you merge the two if's?

Surely.

Quoted 1 line

tools/libxl/libxl_psr.c int libxl_psr_cat_set_cbm(libxl_ctx *ctx, uint32_t domid, · line 337

Quoted 1 line

tools/libxl/libxl_psr.c +1 -0

+        } else if (xc_psr_cat_set_domain_data(ctx->xch, domid, type, socketid, cbm)) {

Quoted 40 lines

Quoted 1 line

Quoted 1 line · On 09.09.15 at 10:10, <[email protected]> wrote:

Quoted 37 lines · Chao Peng

Quoted 24 lines · He Chen

tools/libxl/libxl_psr.c +16 -1

@@ -304,14 +305,41 @@ int libxl_psr_cat_set_cbm(libxl_ctx *ctx, uint32_t domid,
         goto out;
     }
 
+    rc = libxl_psr_cat_get_l3_info(ctx, &info, &nr_sockets);
+    if (rc) {
+        LOGE(ERROR, "Failed to get cat info");
+        goto out;
+    }
+
+    if (!info->cdp_enabled &&
+       (type == LIBXL_PSR_CBM_TYPE_L3_CODE ||
+        type == LIBXL_PSR_CBM_TYPE_L3_DATA))
+    {
+            LOGE(ERROR, "Unable to set Code/Data CBM with CDP disabled");
+            rc = EINVAL;
+            free(info);
+            goto out;
+    }
+
     libxl_for_each_set_bit(socketid, *target_map) {
         if (socketid >= nr_sockets)
             break;
-        if (xc_psr_cat_set_domain_data(ctx->xch, domid, type, socketid, cbm)) {

tools/libxl/libxl_psr.c int libxl_psr_cat_set_cbm(libxl_ctx *ctx, uint32_t domid, · line 336

Quoted 10 lines

tools/libxl/libxl_psr.c +10 -0

+        if (info->cdp_enabled && type == LIBXL_PSR_CBM_TYPE_L3_CBM)
+        {
+            if(xc_psr_cat_set_domain_data(ctx->xch, domid,
+               LIBXL_PSR_CBM_TYPE_L3_CODE, socketid, cbm) ||
+               xc_psr_cat_set_domain_data(ctx->xch, domid,
+               LIBXL_PSR_CBM_TYPE_L3_DATA, socketid, cbm))
+            {
+                libxl__psr_cat_log_err_msg(gc, errno);
+                rc = ERROR_FAIL;
+            }

Will you merge the two if's?

Quoted 1 line

Surely.

Surely not you mean, or else how will this ...

Quoted 1 line

Quoted 1 line

tools/libxl/libxl_psr.c int libxl_psr_cat_set_cbm(libxl_ctx *ctx, uint32_t domid, · line 337

Quoted 1 line

tools/libxl/libxl_psr.c +1 -0

+        } else if (xc_psr_cat_set_domain_data(ctx->xch, domid, type, socketid, cbm)) {

... work?

Looking at this I'm surprised though that consumers is required to do two calls (and know whether CDP is enabled) when they want to set things up without caring for the code/data distinction. I think this should be taken care of in the hypervisor.

Changes in v2:
- x86: Enable CDP by boot parameter instead of enabling/disabling CDP at runtime (suggested by Andrew)

- tools: remove psr-cat-cdp-enable/disable xl commands
- code style

Code/Data Prioritization(CDP) is offered in Intel Broadwell and later server platforms, which is an extension of CAT. CDP enables isolation and separate prioritization of code and data fetches to the L3 cache in a software configurable manner, which can enable workload prioritization and tuning of cache capacity to the characteristics of the workload. CDP extends Cache Allocation Technology (CAT) by providing separate code and data capacity bit masks(CBM) per Class of Service (COS). CDP is used on VM basis in the Xen implementation.

More information about CDP, please refer to Intel SDM, Volumn 3, section 17.16
http://www.intel.com/content/dam/www/public/us/en/documents/manuals/64-ia-32-architectures-software-developer-manual-325462.pdf

This patch series enables CDP feature in Xen based on CAT code, and extends CBM operation functions to support CDP. For all the changes, please see in each patch.

This patchset has been tested on Intel Broadwell server platform.

To make this patchset better, any comment or suggestion is welcomed, I would really appreciate it.

Thanks

He Chen (4):
x86: Support enable CDP by boot parameter and add get CDP status
x86: add domctl cmd to set/get CDP code/data CBM
tools: add tools support for Intel CDP
docs: add document to introduce CDP command

 docs/man/xl.pod.1               |  14 +++
 docs/misc/xl-psr.markdown       |  44 +++++++-
 tools/libxc/include/xenctrl.h   |   7 +-
 tools/libxc/xc_psr.c            |  17 ++-
 tools/libxl/libxl.h             |  10 ++
 tools/libxl/libxl_psr.c         |  32 +++++-
 tools/libxl/libxl_types.idl     |   3 +
 tools/libxl/xl.h                |   2 +
 tools/libxl/xl_cmdimpl.c        |  52 +++++++--
 tools/libxl/xl_cmdtable.c       |   5 +
 xen/arch/x86/domctl.c           |  32 +++++-
 xen/arch/x86/psr.c              | 237 ++++++++++++++++++++++++++++++++--------
 xen/arch/x86/sysctl.c           |   5 +-
 xen/include/asm-x86/msr-index.h |   3 +
 xen/include/asm-x86/psr.h       |  23 +++-
 xen/include/public/domctl.h     |   4 +
 xen/include/public/sysctl.h     |   2 +
 17 files changed, 420 insertions(+), 72 deletions(-)

Signature · 7 lines

1.9.1


_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Quoted 4 lines · He Chen

Changes in v2:
- x86: Enable CDP by boot parameter instead of enabling/disabling CDP at
runtime (suggested by Andrew)

As you added a new boot option, you also need a patch for docs/misc/xen-command-line.markdown.