[PATCH v4 0/4] detect and initialize CDP (Code/Data Prioritization) feature

[PATCH v4 0/4] detect and initialize CDP (Code/Data Prioritization) feature

[He Chen]

Changes in v4:
- x86:
  * remove union member name in struct `psr_cat_cbm` (suggested by Jan)
  * fix log info of CAT & CDP (suggested by Chao & Jan)
  * add a helper `cdp_is_enabled` to tell the status of CDP and CDP initialize
    failed is considered (Jan's comment)
  * XEN_SYSCTL_INTERFACE_VERSION 0x0000000C -> 0x0000000D (suggested by Jan)
  * refine CBM type check logic in get/set CBM function (suggested by Jan)
  * loop optimization in function `find_cos` (suggested by Jan)
- tools: Address Chao's comments.
- docs: Address Chao's comments.
- code style

Changes in v3:
- x86: remove redundant CDP field in cat_socket_enable (suggested by Chao)
- tools: simplify CBM setting function in tools (suggested by Jan)
- docs: Add boot parameter description (suggested by Chao & Ian)
- code style

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 v4 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/xen-command-line.markdown |  11 +-
 docs/misc/xl-psr.markdown           |  44 ++++++-
 tools/libxc/include/xenctrl.h       |   7 +-
 tools/libxc/xc_psr.c                |  17 ++-
 tools/libxl/libxl.h                 |   7 ++
 tools/libxl/libxl_psr.c             |   5 +-
 tools/libxl/libxl_types.idl         |   3 +
 tools/libxl/xl_cmdimpl.c            |  49 ++++++--
 tools/libxl/xl_cmdtable.c           |   3 +
 xen/arch/x86/domctl.c               |  32 ++++-
 xen/arch/x86/psr.c                  | 239 +++++++++++++++++++++++++++++-------
 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         |   4 +-
 17 files changed, 395 insertions(+), 75 deletions(-)

-- 
1.9.1

[PATCH v4 1/4] x86: Support enable CDP by boot parameter and add get CDP status

[He Chen]

Add boot parameter `psr=cdp` to enable CDP at boot time.
Intel Code/Data Prioritization(CDP) feature is based on CAT. 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 <he.chen@linux.intel.com>
---
 docs/misc/xen-command-line.markdown | 11 ++++-
 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         |  4 +-
 6 files changed, 98 insertions(+), 20 deletions(-)

diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index a2e427c..d92e323 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -1165,9 +1165,9 @@ This option can be specified more than once (up to 8 times at present).
 > `= <integer>`
 
 ### psr (Intel)
-> `= List of ( cmt:<boolean> | rmid_max:<integer> | cat:<boolean> | cos_max:<integer> )`
+> `= List of ( cmt:<boolean> | rmid_max:<integer> | cat:<boolean> | cos_max:<integer> | cdp:<boolean> )`
 
-> Default: `psr=cmt:0,rmid_max:255,cat:0,cos_max:255`
+> Default: `psr=cmt:0,rmid_max:255,cat:0,cos_max:255,cdp:0`
 
 Platform Shared Resource(PSR) Services.  Intel Haswell and later server
 platforms offer information about the sharing of resources.
@@ -1197,6 +1197,13 @@ The following resources are available:
   the cache allocation.
   * `cat` instructs Xen to enable/disable Cache Allocation Technology.
   * `cos_max` indicates the max value for COS ID.
+* Code and Data Prioritization Technology (Broadwell and later). Information
+  regarding the code cache and the data cache allocation. CDP is based on CAT.
+  * `cdp` instructs Xen to enable/disable Code and Data Prioritization. Note
+    that `cos_max` of CDP is a little different from `cos_max` of CAT. With
+    CDP, one COS will corespond two CBMs other than one with CAT, due to the
+    sum of CBMs is fixed, that means actual `cos_max` in use will automatically
+    reduce to half when CDP is enabled.
 
 ### reboot
 > `= t[riple] | k[bd] | a[cpi] | p[ci] | P[ower] | e[fi] | n[o] [, [w]arm | [c]old]`
diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c
index c0daa2e..e44ed8b 100644
--- a/xen/arch/x86/psr.c
+++ b/xen/arch/x86/psr.c
@@ -21,9 +21,16 @@
 
 #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;
+        };
+    } u;
     unsigned int ref;
 };
 
@@ -43,6 +50,7 @@ struct psr_cmt *__read_mostly psr_cmt;
 
 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;
 
 static unsigned int __initdata opt_psr;
 static unsigned int __initdata opt_rmid_max = 255;
@@ -94,6 +102,7 @@ static void __init parse_psr_param(char *s)
 
         parse_psr_bool(s, val_str, "cmt", PSR_CMT);
         parse_psr_bool(s, val_str, "cat", PSR_CAT);
+        parse_psr_bool(s, val_str, "cdp", PSR_CDP);
 
         if ( val_str && !strcmp(s, "rmid_max") )
             opt_rmid_max = simple_strtoul(val_str, NULL, 0);
@@ -261,8 +270,14 @@ static struct psr_cat_socket_info *get_cat_socket_info(unsigned int socket)
     return cat_socket_info + socket;
 }
 
+static inline bool_t cdp_is_enabled(unsigned int socket,
+                                    unsigned long *cdp_socket_enable)
+{
+    return (cdp_socket_enable && test_bit(socket, cdp_socket_enable));
+}
+
 int psr_get_cat_l3_info(unsigned int socket, uint32_t *cbm_len,
-                        uint32_t *cos_max)
+                        uint32_t *cos_max, uint32_t *flags)
 {
     struct psr_cat_socket_info *info = get_cat_socket_info(socket);
 
@@ -272,6 +287,10 @@ int psr_get_cat_l3_info(unsigned int socket, uint32_t *cbm_len,
     *cbm_len = info->cbm_len;
     *cos_max = info->cos_max;
 
+    *flags = 0;
+    if ( cdp_is_enabled(socket, cdp_socket_enable) )
+        *flags |= PSR_CAT_FLAG_L3_CDP;
+
     return 0;
 }
 
@@ -282,7 +301,7 @@ int psr_get_l3_cbm(struct domain *d, unsigned int socket, uint64_t *cbm)
     if ( IS_ERR(info) )
         return PTR_ERR(info);
 
-    *cbm = info->cos_to_cbm[d->arch.psr_cos_ids[socket]].cbm;
+    *cbm = info->cos_to_cbm[d->arch.psr_cos_ids[socket]].u.cbm;
 
     return 0;
 }
@@ -313,19 +332,34 @@ static bool_t psr_check_cbm(unsigned int cbm_len, uint64_t cbm)
 struct cos_cbm_info
 {
     unsigned int cos;
-    uint64_t cbm;
+    uint64_t cbm_code;
+    uint64_t cbm_data;
+    bool_t cdp;
 };
 
 static void do_write_l3_cbm(void *data)
 {
     struct cos_cbm_info *info = data;
 
-    wrmsrl(MSR_IA32_PSR_L3_MASK(info->cos), info->cbm);
+    if ( info->cdp )
+    {
+        wrmsrl(MSR_IA32_PSR_L3_MASK_CODE(info->cos), info->cbm_code);
+        wrmsrl(MSR_IA32_PSR_L3_MASK_DATA(info->cos), info->cbm_data);
+    }
+    else
+        wrmsrl(MSR_IA32_PSR_L3_MASK(info->cos), info->cbm_code);
 }
 
-static int write_l3_cbm(unsigned int socket, unsigned int cos, uint64_t cbm)
+static int write_l3_cbm(unsigned int socket, unsigned int cos,
+                        uint64_t cbm_code, uint64_t cbm_data, bool_t cdp)
 {
-    struct cos_cbm_info info = { .cos = cos, .cbm = cbm };
+    struct cos_cbm_info info =
+    {
+      .cos = cos,
+      .cbm_code = cbm_code,
+      .cbm_data = cbm_data,
+      .cdp = cdp,
+    };
 
     if ( socket == cpu_to_socket(smp_processor_id()) )
         do_write_l3_cbm(&info);
@@ -363,7 +397,7 @@ int psr_set_l3_cbm(struct domain *d, unsigned int socket, uint64_t cbm)
         /* If still not found, then keep unused one. */
         if ( !found && cos != 0 && map[cos].ref == 0 )
             found = map + cos;
-        else if ( map[cos].cbm == cbm )
+        else if ( map[cos].u.cbm == cbm )
         {
             if ( unlikely(cos == old_cos) )
             {
@@ -387,16 +421,16 @@ int psr_set_l3_cbm(struct domain *d, unsigned int socket, uint64_t cbm)
     }
 
     cos = found - map;
-    if ( found->cbm != cbm )
+    if ( found->u.cbm != cbm )
     {
-        int ret = write_l3_cbm(socket, cos, cbm);
+        int ret = write_l3_cbm(socket, cos, cbm, 0, 0);
 
         if ( ret )
         {
             spin_unlock(&info->cbm_lock);
             return ret;
         }
-        found->cbm = cbm;
+        found->u.cbm = cbm;
     }
 
     found->ref++;
@@ -470,6 +504,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 +525,32 @@ 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;
 
         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 ( (ecx & PSR_CAT_CDP_CAPABILITY) && (opt_psr & PSR_CDP) &&
+             cdp_socket_enable && !test_bit(socket, cdp_socket_enable) )
+        {
+            rdmsrl(MSR_IA32_PSR_L3_QOS_CFG, val);
+            wrmsrl(MSR_IA32_PSR_L3_QOS_CFG, val | (1 << PSR_L3_QOS_CDP_ENABLE_BIT));
+
+            info->cos_to_cbm[0].u.code = (1ull << info->cbm_len) - 1;
+            info->cos_to_cbm[0].u.data = (1ull << info->cbm_len) - 1;
+
+            /* We only write mask1 since mask0 is always all ones by default. */
+            wrmsrl(MSR_IA32_PSR_L3_MASK(1), (1ull << info->cbm_len) - 1);
+
+            /* Cut half of cos_max when CDP is enabled. */
+            info->cos_max >>= 1;
+
+            set_bit(socket, cdp_socket_enable);
+        }
+        printk(XENLOG_INFO "CAT: enabled on socket %u, cos_max:%u, cbm_len:%u, CDP:%s\n",
+               socket, info->cos_max, info->cbm_len,
+               cdp_is_enabled(socket, cdp_socket_enable) ? "on" : "off");
     }
 }
 
@@ -513,6 +567,7 @@ static void cat_cpu_fini(unsigned int cpu)
             xfree(info->cos_to_cbm);
             info->cos_to_cbm = NULL;
         }
+        clear_bit(socket, cdp_socket_enable);
         clear_bit(socket, cat_socket_enable);
     }
 }
@@ -535,6 +590,7 @@ static void __init init_psr_cat(void)
 
     cat_socket_enable = xzalloc_array(unsigned long, BITS_TO_LONGS(nr_sockets));
     cat_socket_info = xzalloc_array(struct psr_cat_socket_info, nr_sockets);
+    cdp_socket_enable = xzalloc_array(unsigned long, BITS_TO_LONGS(nr_sockets));
 
     if ( !cat_socket_enable || !cat_socket_info )
         psr_cat_free();
diff --git a/xen/arch/x86/sysctl.c b/xen/arch/x86/sysctl.c
index f36b52f..e42c5e7 100644
--- a/xen/arch/x86/sysctl.c
+++ b/xen/arch/x86/sysctl.c
@@ -177,12 +177,13 @@ long arch_do_sysctl(
         case XEN_SYSCTL_PSR_CAT_get_l3_info:
             ret = psr_get_cat_l3_info(sysctl->u.psr_cat_op.target,
                                       &sysctl->u.psr_cat_op.u.l3_info.cbm_len,
-                                      &sysctl->u.psr_cat_op.u.l3_info.cos_max);
+                                      &sysctl->u.psr_cat_op.u.l3_info.cos_max,
+                                      &sysctl->u.psr_cat_op.u.l3_info.flags);
 
             if ( !ret && __copy_field_to_guest(u_sysctl, sysctl, u.psr_cat_op) )
                 ret = -EFAULT;
-
             break;
+
         default:
             ret = -EOPNOTSUPP;
             break;
diff --git a/xen/include/asm-x86/msr-index.h b/xen/include/asm-x86/msr-index.h
index e9c4723..65c1d02 100644
--- a/xen/include/asm-x86/msr-index.h
+++ b/xen/include/asm-x86/msr-index.h
@@ -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_CODE(n)	(0x00000c90 + (n) * 2 + 1)
+#define MSR_IA32_PSR_L3_MASK_DATA(n)	(0x00000c90 + (n) * 2)
 
 /* Intel Model 6 */
 #define MSR_P6_PERFCTR(n)		(0x000000c1 + (n))
diff --git a/xen/include/asm-x86/psr.h b/xen/include/asm-x86/psr.h
index 081750f..422b053 100644
--- a/xen/include/asm-x86/psr.h
+++ b/xen/include/asm-x86/psr.h
@@ -27,6 +27,15 @@
 /* L3 Monitoring Features */
 #define PSR_CMT_L3_OCCUPANCY           0x1
 
+/* CDP Capability */
+#define PSR_CAT_CDP_CAPABILITY       (1u << 2)
+
+/* L3 CDP Enable bit*/
+#define PSR_L3_QOS_CDP_ENABLE_BIT       0x0
+
+/* L3 CDP flag bit */
+#define PSR_CAT_FLAG_L3_CDP       (1u << 0)
+
 struct psr_cmt_l3 {
     unsigned int features;
     unsigned int upscaling_factor;
@@ -52,7 +61,7 @@ void psr_free_rmid(struct domain *d);
 void psr_ctxt_switch_to(struct domain *d);
 
 int psr_get_cat_l3_info(unsigned int socket, uint32_t *cbm_len,
-                        uint32_t *cos_max);
+                        uint32_t *cos_max, uint32_t *flags);
 int psr_get_l3_cbm(struct domain *d, unsigned int socket, uint64_t *cbm);
 int psr_set_l3_cbm(struct domain *d, unsigned int socket, uint64_t cbm);
 
diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
index 58c9be2..0e2834f 100644
--- a/xen/include/public/sysctl.h
+++ b/xen/include/public/sysctl.h
@@ -35,7 +35,7 @@
 #include "domctl.h"
 #include "physdev.h"
 
-#define XEN_SYSCTL_INTERFACE_VERSION 0x0000000C
+#define XEN_SYSCTL_INTERFACE_VERSION 0x0000000D
 
 /*
  * Read console content from Xen buffer ring.
@@ -704,6 +704,8 @@ struct xen_sysctl_psr_cat_op {
         struct {
             uint32_t cbm_len;   /* OUT: CBM length */
             uint32_t cos_max;   /* OUT: Maximum COS */
+            #define XEN_SYSCTL_PSR_CAT_L3_CDP       (1u << 0)
+            uint32_t flags;     /* OUT: CAT flags */
         } l3_info;
     } u;
 };
-- 
1.9.1

[PATCH v4 2/4] x86: add domctl cmd to set/get CDP code/data CBM

[He Chen]

CDP extends CAT and provides the capacity to control L3 code & data
cache. With CDP, one COS corresponds to two CMBs(code & data). cbm_type
is added to distinguish different CBM operations. 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 <he.chen@linux.intel.com>
---
 xen/arch/x86/domctl.c       |  32 ++++++++-
 xen/arch/x86/psr.c          | 165 ++++++++++++++++++++++++++++++++++----------
 xen/include/asm-x86/psr.h   |  12 +++-
 xen/include/public/domctl.h |   4 ++
 4 files changed, 172 insertions(+), 41 deletions(-)

diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index bf62a88..734fddb 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -1167,12 +1167,40 @@ long arch_do_domctl(
         {
         case XEN_DOMCTL_PSR_CAT_OP_SET_L3_CBM:
             ret = psr_set_l3_cbm(d, domctl->u.psr_cat_op.target,
-                                 domctl->u.psr_cat_op.data);
+                                 domctl->u.psr_cat_op.data,
+                                 PSR_CBM_TYPE_L3);
+            break;
+
+        case XEN_DOMCTL_PSR_CAT_OP_SET_L3_CODE:
+            ret = psr_set_l3_cbm(d, domctl->u.psr_cat_op.target,
+                                 domctl->u.psr_cat_op.data,
+                                 PSR_CBM_TYPE_L3_CODE);
+            break;
+
+        case XEN_DOMCTL_PSR_CAT_OP_SET_L3_DATA:
+            ret = psr_set_l3_cbm(d, domctl->u.psr_cat_op.target,
+                                 domctl->u.psr_cat_op.data,
+                                 PSR_CBM_TYPE_L3_DATA);
             break;
 
         case XEN_DOMCTL_PSR_CAT_OP_GET_L3_CBM:
             ret = psr_get_l3_cbm(d, domctl->u.psr_cat_op.target,
-                                 &domctl->u.psr_cat_op.data);
+                                 &domctl->u.psr_cat_op.data,
+                                 PSR_CBM_TYPE_L3);
+            copyback = 1;
+            break;
+
+        case XEN_DOMCTL_PSR_CAT_OP_GET_L3_CODE:
+            ret = psr_get_l3_cbm(d, domctl->u.psr_cat_op.target,
+                                 &domctl->u.psr_cat_op.data,
+                                 PSR_CBM_TYPE_L3_CODE);
+            copyback = 1;
+            break;
+
+        case XEN_DOMCTL_PSR_CAT_OP_GET_L3_DATA:
+            ret = psr_get_l3_cbm(d, domctl->u.psr_cat_op.target,
+                                 &domctl->u.psr_cat_op.data,
+                                 PSR_CBM_TYPE_L3_DATA);
             copyback = 1;
             break;
 
diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c
index e44ed8b..c5519b7 100644
--- a/xen/arch/x86/psr.c
+++ b/xen/arch/x86/psr.c
@@ -294,14 +294,40 @@ int psr_get_cat_l3_info(unsigned int socket, uint32_t *cbm_len,
     return 0;
 }
 
-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);
+    bool_t cdp_enabled = cdp_is_enabled(socket, cdp_socket_enable);
 
     if ( IS_ERR(info) )
         return PTR_ERR(info);
 
-    *cbm = info->cos_to_cbm[d->arch.psr_cos_ids[socket]].u.cbm;
+    switch ( type )
+    {
+    case PSR_CBM_TYPE_L3:
+        if ( type == PSR_CBM_TYPE_L3 && cdp_enabled )
+            return -EXDEV;
+        *cbm = info->cos_to_cbm[d->arch.psr_cos_ids[socket]].u.cbm;
+        break;
+
+    case PSR_CBM_TYPE_L3_CODE:
+        if ( !cdp_enabled )
+            *cbm = info->cos_to_cbm[d->arch.psr_cos_ids[socket]].u.cbm;
+        else
+            *cbm = info->cos_to_cbm[d->arch.psr_cos_ids[socket]].u.code;
+        break;
+
+    case PSR_CBM_TYPE_L3_DATA:
+        if ( !cdp_enabled )
+            *cbm = info->cos_to_cbm[d->arch.psr_cos_ids[socket]].u.cbm;
+        else
+            *cbm = info->cos_to_cbm[d->arch.psr_cos_ids[socket]].u.data;
+        break;
+
+    default:
+        ASSERT_UNREACHABLE();
+    }
 
     return 0;
 }
@@ -375,10 +401,48 @@ 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;
+
+    for ( cos = 0; cos <= cos_max; cos++ )
+    {
+        if( map[cos].ref &&
+            ((!cdp_enabled && map[cos].u.cbm == cbm_code) ||
+            (cdp_enabled && map[cos].u.code == cbm_code &&
+                            map[cos].u.data == cbm_data)))
+            return cos;
+    }
+
+    return -ENOENT;
+}
+
+static int pick_avail_cos(struct psr_cat_cbm *map, unsigned int cos_max,
+                          unsigned int old_cos)
+{
+    unsigned int cos;
+
+    /* If old cos is referred only by the domain, then use it. */
+    if ( map[old_cos].ref == 1 )
+        return old_cos;
+
+    /* Find an unused one other than cos0. */
+    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;
+    uint64_t cbm_data, cbm_code;
+    bool_t cdp_enabled = cdp_is_enabled(socket, cdp_socket_enable);
+    struct psr_cat_cbm *map;
     struct psr_cat_socket_info *info = get_cat_socket_info(socket);
 
     if ( IS_ERR(info) )
@@ -387,53 +451,80 @@ int psr_set_l3_cbm(struct domain *d, unsigned int socket, uint64_t cbm)
     if ( !psr_check_cbm(info->cbm_len, cbm) )
         return -EINVAL;
 
+    if ( !cdp_enabled && (type == PSR_CBM_TYPE_L3_CODE ||
+                          type == PSR_CBM_TYPE_L3_DATA) )
+        return -EINVAL;
+
+    cos_max = info->cos_max;
     old_cos = d->arch.psr_cos_ids[socket];
     map = info->cos_to_cbm;
 
-    spin_lock(&info->cbm_lock);
-
-    for ( cos = 0; cos <= info->cos_max; cos++ )
+    switch( type )
     {
-        /* 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;
-            break;
-        }
-    }
+    case PSR_CBM_TYPE_L3:
+        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:
+        cbm_code = cbm;
+        cbm_data = map[old_cos].u.data;
+        break;
 
-    if ( !found )
-    {
-        spin_unlock(&info->cbm_lock);
-        return -EOVERFLOW;
+    case PSR_CBM_TYPE_L3_DATA:
+        cbm_code = map[old_cos].u.code;
+        cbm_data = cbm;
+        break;
+
+    default:
+            ASSERT_UNREACHABLE();
     }
 
-    cos = found - map;
-    if ( found->u.cbm != cbm )
+    spin_lock(&info->cbm_lock);
+    cos = find_cos(map, cos_max, cbm_code, cbm_data, cdp_enabled);
+    if ( cos >= 0 )
+    {
+        if ( cos == old_cos )
+        {
+            spin_unlock(&info->cbm_lock);
+            return 0;
+        }
+    }
+    else
     {
-        int ret = write_l3_cbm(socket, cos, cbm, 0, 0);
+        bool_t need_write = 1;
 
-        if ( ret )
+        cos = pick_avail_cos(map, cos_max, old_cos);
+        if ( cos < 0 )
         {
             spin_unlock(&info->cbm_lock);
-            return ret;
+            return cos;
+        }
+
+        /* We try to avoid writing MSR. */
+        if ( cdp_enabled )
+        {
+            if ( map[cos].u.code == cbm_code &&
+                 map[cos].u.data == cbm_data )
+                need_write = 0;
+        }
+        else
+            need_write = !(map[cos].u.cbm == cbm_code);
+
+        if ( need_write )
+        {
+            ret = write_l3_cbm(socket, cos, cbm_code, cbm_data, cdp_enabled);
+            if ( ret )
+            {
+                spin_unlock(&info->cbm_lock);
+                return ret;
+            }
+            map[cos].u.code = cbm_code;
+            map[cos].u.data = cbm_data;
         }
-        found->u.cbm = cbm;
     }
 
-    found->ref++;
+    map[cos].ref++;
     map[old_cos].ref--;
     spin_unlock(&info->cbm_lock);
 
diff --git a/xen/include/asm-x86/psr.h b/xen/include/asm-x86/psr.h
index 422b053..04d0c1a 100644
--- a/xen/include/asm-x86/psr.h
+++ b/xen/include/asm-x86/psr.h
@@ -49,6 +49,12 @@ struct psr_cmt {
     struct psr_cmt_l3 l3;
 };
 
+enum cbm_type {
+    PSR_CBM_TYPE_L3,
+    PSR_CBM_TYPE_L3_CODE,
+    PSR_CBM_TYPE_L3_DATA,
+};
+
 extern struct psr_cmt *psr_cmt;
 
 static inline bool_t psr_cmt_enabled(void)
@@ -62,8 +68,10 @@ void psr_ctxt_switch_to(struct domain *d);
 
 int psr_get_cat_l3_info(unsigned int socket, uint32_t *cbm_len,
                         uint32_t *cos_max, uint32_t *flags);
-int psr_get_l3_cbm(struct domain *d, unsigned int socket, uint64_t *cbm);
-int psr_set_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);
+int psr_set_l3_cbm(struct domain *d, unsigned int socket,
+                   uint64_t cbm, enum cbm_type type);
 
 int psr_domain_init(struct domain *d);
 void psr_domain_free(struct domain *d);
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index 675f021..f438cae 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -1056,6 +1056,10 @@ DEFINE_XEN_GUEST_HANDLE(xen_domctl_monitor_op_t);
 struct xen_domctl_psr_cat_op {
 #define XEN_DOMCTL_PSR_CAT_OP_SET_L3_CBM     0
 #define XEN_DOMCTL_PSR_CAT_OP_GET_L3_CBM     1
+#define XEN_DOMCTL_PSR_CAT_OP_SET_L3_CODE    2
+#define XEN_DOMCTL_PSR_CAT_OP_SET_L3_DATA    3
+#define XEN_DOMCTL_PSR_CAT_OP_GET_L3_CODE    4
+#define XEN_DOMCTL_PSR_CAT_OP_GET_L3_DATA    5
     uint32_t cmd;       /* IN: XEN_DOMCTL_PSR_CAT_OP_* */
     uint32_t target;    /* IN */
     uint64_t data;      /* IN/OUT */
-- 
1.9.1

[PATCH v4 3/4] tools: add tools support for Intel CDP

[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 <he.chen@linux.intel.com>
---
 tools/libxc/include/xenctrl.h |  7 +++++--
 tools/libxc/xc_psr.c          | 17 ++++++++++++++-
 tools/libxl/libxl.h           |  7 +++++++
 tools/libxl/libxl_psr.c       |  5 ++++-
 tools/libxl/libxl_types.idl   |  3 +++
 tools/libxl/xl_cmdimpl.c      | 49 ++++++++++++++++++++++++++++++++++---------
 tools/libxl/xl_cmdtable.c     |  3 +++
 7 files changed, 77 insertions(+), 14 deletions(-)

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index de3c0ad..54f2069 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -2798,7 +2798,9 @@ enum xc_psr_cmt_type {
 typedef enum xc_psr_cmt_type xc_psr_cmt_type;
 
 enum xc_psr_cat_type {
-    XC_PSR_CAT_L3_CBM = 1,
+    XC_PSR_CAT_L3_CBM  = 1,
+    XC_PSR_CAT_L3_CODE = 2,
+    XC_PSR_CAT_L3_DATA = 3,
 };
 typedef enum xc_psr_cat_type xc_psr_cat_type;
 
@@ -2824,7 +2826,8 @@ int xc_psr_cat_get_domain_data(xc_interface *xch, uint32_t domid,
                                xc_psr_cat_type type, uint32_t target,
                                uint64_t *data);
 int xc_psr_cat_get_l3_info(xc_interface *xch, uint32_t socket,
-                           uint32_t *cos_max, uint32_t *cbm_len);
+                           uint32_t *cos_max, uint32_t *cbm_len,
+                           bool *cdp_enabled);
 #endif
 
 #endif /* XENCTRL_H */
diff --git a/tools/libxc/xc_psr.c b/tools/libxc/xc_psr.c
index d8b3a51..5bbe950 100644
--- a/tools/libxc/xc_psr.c
+++ b/tools/libxc/xc_psr.c
@@ -260,6 +260,12 @@ int xc_psr_cat_set_domain_data(xc_interface *xch, uint32_t domid,
     case XC_PSR_CAT_L3_CBM:
         cmd = XEN_DOMCTL_PSR_CAT_OP_SET_L3_CBM;
         break;
+    case XC_PSR_CAT_L3_CODE:
+        cmd = XEN_DOMCTL_PSR_CAT_OP_SET_L3_CODE;
+        break;
+    case XC_PSR_CAT_L3_DATA:
+        cmd = XEN_DOMCTL_PSR_CAT_OP_SET_L3_DATA;
+        break;
     default:
         errno = EINVAL;
         return -1;
@@ -287,6 +293,12 @@ int xc_psr_cat_get_domain_data(xc_interface *xch, uint32_t domid,
     case XC_PSR_CAT_L3_CBM:
         cmd = XEN_DOMCTL_PSR_CAT_OP_GET_L3_CBM;
         break;
+    case XC_PSR_CAT_L3_CODE:
+        cmd = XEN_DOMCTL_PSR_CAT_OP_GET_L3_CODE;
+        break;
+    case XC_PSR_CAT_L3_DATA:
+        cmd = XEN_DOMCTL_PSR_CAT_OP_GET_L3_DATA;
+        break;
     default:
         errno = EINVAL;
         return -1;
@@ -306,7 +318,8 @@ int xc_psr_cat_get_domain_data(xc_interface *xch, uint32_t domid,
 }
 
 int xc_psr_cat_get_l3_info(xc_interface *xch, uint32_t socket,
-                           uint32_t *cos_max, uint32_t *cbm_len)
+                           uint32_t *cos_max, uint32_t *cbm_len,
+                           bool *cdp_enabled)
 {
     int rc;
     DECLARE_SYSCTL;
@@ -320,6 +333,8 @@ int xc_psr_cat_get_l3_info(xc_interface *xch, uint32_t socket,
     {
         *cos_max = sysctl.u.psr_cat_op.u.l3_info.cos_max;
         *cbm_len = sysctl.u.psr_cat_op.u.l3_info.cbm_len;
+        *cdp_enabled = sysctl.u.psr_cat_op.u.l3_info.flags &
+                       XEN_SYSCTL_PSR_CAT_L3_CDP;
     }
 
     return rc;
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 5f9047c..611e98d 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -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 and Data Prioritization feature is supported.
+ */
+#define LIBXL_HAVE_PSR_CDP 1
 #endif
 
 /*
diff --git a/tools/libxl/libxl_psr.c b/tools/libxl/libxl_psr.c
index 3378239..62963cf 100644
--- a/tools/libxl/libxl_psr.c
+++ b/tools/libxl/libxl_psr.c
@@ -87,6 +87,9 @@ static void libxl__psr_cat_log_err_msg(libxl__gc *gc, int err)
     case EEXIST:
         msg = "The same CBM is already set to this domain";
         break;
+    case EINVAL:
+        msg = "Unable to set code or data CBM when CDP is disabled";
+        break;
 
     default:
         libxl__psr_log_err_msg(gc, err);
@@ -352,7 +355,7 @@ int libxl_psr_cat_get_l3_info(libxl_ctx *ctx, libxl_psr_cat_info **info,
 
     for (i = 0; i < nr_sockets; i++) {
         if (xc_psr_cat_get_l3_info(ctx->xch, i, &ptr[i].cos_max,
-                                                &ptr[i].cbm_len)) {
+                                   &ptr[i].cbm_len, &ptr[i].cdp_enabled)) {
             libxl__psr_cat_log_err_msg(gc, errno);
             rc = ERROR_FAIL;
             free(ptr);
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index ef346e7..fa017ad 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -787,9 +787,12 @@ libxl_psr_cmt_type = Enumeration("psr_cmt_type", [
 libxl_psr_cbm_type = Enumeration("psr_cbm_type", [
     (0, "UNKNOWN"),
     (1, "L3_CBM"),
+    (2, "L3_CODE"),
+    (3, "L3_DATA"),
     ])
 
 libxl_psr_cat_info = Struct("psr_cat_info", [
     ("cos_max", uint32),
     ("cbm_len", uint32),
+    ("cdp_enabled", bool),
     ])
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index ebbb9a5..8128f54 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -8390,6 +8390,10 @@ static int psr_cat_hwinfo(void)
         }
         printf("%-16s: %u\n", "Socket ID", socketid);
         printf("%-16s: %uKB\n", "L3 Cache", l3_cache_size);
+        if (info->cdp_enabled)
+            printf("%-16s: Enabled\n", "CDP Status");
+        else
+            printf("%-16s: Disabled\n", "CDP Status");
         printf("%-16s: %u\n", "Maximum COS", info->cos_max);
         printf("%-16s: %u\n", "CBM length", info->cbm_len);
         printf("%-16s: %#llx\n", "Default CBM",
@@ -8401,7 +8405,8 @@ out:
     return rc;
 }
 
-static void psr_cat_print_one_domain_cbm(uint32_t domid, uint32_t socketid)
+static void psr_cat_print_one_domain_cbm(uint32_t domid, uint32_t socketid,
+                                         bool cdp_enabled)
 {
     char *domain_name;
     uint64_t cbm;
@@ -8410,20 +8415,29 @@ static void psr_cat_print_one_domain_cbm(uint32_t domid, uint32_t socketid)
     printf("%5d%25s", domid, domain_name);
     free(domain_name);
 
-    if (!libxl_psr_cat_get_cbm(ctx, domid, LIBXL_PSR_CBM_TYPE_L3_CBM,
-                               socketid, &cbm))
-         printf("%#16"PRIx64, cbm);
-
+    if (!cdp_enabled) {
+        if (!libxl_psr_cat_get_cbm(ctx, domid, LIBXL_PSR_CBM_TYPE_L3_CBM,
+                                   socketid, &cbm))
+            printf("%#16"PRIx64, cbm);
+    } else {
+        if (!libxl_psr_cat_get_cbm(ctx, domid, LIBXL_PSR_CBM_TYPE_L3_CODE,
+                                   socketid, &cbm))
+            printf("%10s%#8"PRIx64, "code:", cbm);
+        if (!libxl_psr_cat_get_cbm(ctx, domid, LIBXL_PSR_CBM_TYPE_L3_DATA,
+                                   socketid, &cbm))
+            printf("%10s%#8"PRIx64, "data:", cbm);
+    }
     printf("\n");
 }
 
-static int psr_cat_print_domain_cbm(uint32_t domid, uint32_t socketid)
+static int psr_cat_print_domain_cbm(uint32_t domid, uint32_t socketid,
+                                    bool cdp_enabled)
 {
     int i, nr_domains;
     libxl_dominfo *list;
 
     if (domid != INVALID_DOMID) {
-        psr_cat_print_one_domain_cbm(domid, socketid);
+        psr_cat_print_one_domain_cbm(domid, socketid, cdp_enabled);
         return 0;
     }
 
@@ -8433,7 +8447,7 @@ static int psr_cat_print_domain_cbm(uint32_t domid, uint32_t socketid)
     }
 
     for (i = 0; i < nr_domains; i++)
-        psr_cat_print_one_domain_cbm(list[i].domid, socketid);
+        psr_cat_print_one_domain_cbm(list[i].domid, socketid, cdp_enabled);
     libxl_dominfo_list_free(list, nr_domains);
 
     return 0;
@@ -8457,7 +8471,7 @@ static int psr_cat_print_socket(uint32_t domid, uint32_t socketid,
     printf("%-16s: %#llx\n", "Default CBM", (1ull << info->cbm_len) - 1);
     printf("%5s%25s%16s\n", "ID", "NAME", "CBM");
 
-    return psr_cat_print_domain_cbm(domid, socketid);
+    return psr_cat_print_domain_cbm(domid, socketid, info->cdp_enabled);
 }
 
 static int psr_cat_show(uint32_t domid)
@@ -8489,6 +8503,8 @@ int main_psr_cat_cbm_set(int argc, char **argv)
     libxl_psr_cbm_type type = LIBXL_PSR_CBM_TYPE_L3_CBM;
     uint64_t cbm;
     int ret, opt = 0;
+    int opt_data = 0;
+    int opt_code = 0;
     libxl_bitmap target_map;
     char *value;
     libxl_string_list socket_list;
@@ -8497,13 +8513,15 @@ int main_psr_cat_cbm_set(int argc, char **argv)
 
     static struct option opts[] = {
         {"socket", 1, 0, 's'},
+        {"data", 0, 0, 'd'},
+        {"code", 0, 0, 'c'},
         COMMON_LONG_OPTS
     };
 
     libxl_socket_bitmap_alloc(ctx, &target_map, 0);
     libxl_bitmap_set_none(&target_map);
 
-    SWITCH_FOREACH_OPT(opt, "s:", opts, "psr-cat-cbm-set", 2) {
+    SWITCH_FOREACH_OPT(opt, "s:cd", opts, "psr-cat-cbm-set", 2) {
     case 's':
         trim(isspace, optarg, &value);
         split_string_into_string_list(value, ",", &socket_list);
@@ -8517,8 +8535,19 @@ int main_psr_cat_cbm_set(int argc, char **argv)
         libxl_string_list_dispose(&socket_list);
         free(value);
         break;
+    case 'd':
+        type = LIBXL_PSR_CBM_TYPE_L3_DATA;
+        opt_data = 1;
+        break;
+    case 'c':
+        type = LIBXL_PSR_CBM_TYPE_L3_CODE;
+        opt_code = 1;
+        break;
     }
 
+    if (opt_data && opt_code)
+        type = LIBXL_PSR_CBM_TYPE_L3_CBM;
+
     if (libxl_bitmap_is_empty(&target_map))
         libxl_bitmap_set_any(&target_map);
 
diff --git a/tools/libxl/xl_cmdtable.c b/tools/libxl/xl_cmdtable.c
index 0071f12..13e7aa1 100644
--- a/tools/libxl/xl_cmdtable.c
+++ b/tools/libxl/xl_cmdtable.c
@@ -543,6 +543,8 @@ struct cmd_spec cmd_table[] = {
       "Set cache capacity bitmasks(CBM) for a domain",
       "[options] <Domain> <CBM>",
       "-s <socket>       Specify the socket to process, otherwise all sockets are processed\n"
+      "-c                Set code CBM if CDP is supported\n"
+      "-d                Set data CBM if CDP is supported\n"
     },
     { "psr-cat-show",
       &main_psr_cat_show, 0, 1,
@@ -551,6 +553,7 @@ struct cmd_spec cmd_table[] = {
     },
 
 #endif
+
 };
 
 int cmdtable_len = sizeof(cmd_table)/sizeof(struct cmd_spec);
-- 
1.9.1

[PATCH v4 4/4] docs: add document to introduce CDP command

[He Chen]

Add new CDP options with CAT commands in xl interface man page.
Add description of CDP in xl-psr.markdown.

Signed-off-by: He Chen <he.chen@linux.intel.com>
---
 docs/man/xl.pod.1         | 14 ++++++++++++++
 docs/misc/xl-psr.markdown | 44 +++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 53 insertions(+), 5 deletions(-)

diff --git a/docs/man/xl.pod.1 b/docs/man/xl.pod.1
index f22c3f3..3c7107d 100644
--- a/docs/man/xl.pod.1
+++ b/docs/man/xl.pod.1
@@ -1530,6 +1530,12 @@ applications. In the Xen implementation, CAT is used to control cache allocation
 on VM basis. To enforce cache on a specific domain, just set capacity bitmasks
 (CBM) for the domain.
 
+Intel Broadwell and later server platforms also offer Code/Data Prioritization
+(CDP) for cache allocation, which support specifying code or data cache for
+applications. CDP is used on VM basis in the Xen implementation. To specify
+code or data CBM for the domain, CDP feature must be enabled and CBM type
+options need to be specified when setting CBM.
+
 =over 4
 
 =item B<psr-cat-cbm-set> [I<OPTIONS>] I<domain-id> I<cbm>
@@ -1545,6 +1551,14 @@ B<OPTIONS>
 
 Specify the socket to process, otherwise all sockets are processed.
 
+=item B<-c>, B<--code>
+
+Set code CBM when CDP is enabled.
+
+=item B<-d>, B<--data>
+
+Set data CBM when CDP is enabled.
+
 =back
 
 =item B<psr-cat-show> [I<domain-id>]
diff --git a/docs/misc/xl-psr.markdown b/docs/misc/xl-psr.markdown
index 3545912..5eb97cc 100644
--- a/docs/misc/xl-psr.markdown
+++ b/docs/misc/xl-psr.markdown
@@ -14,7 +14,7 @@ tracks cache utilization of memory accesses according to the RMID and reports
 monitored data via a counter register.
 
 For more detailed information please refer to Intel SDM chapter
-"17.14 - Platform Shared Resource Monitoring: Cache Monitoring Technology".
+"17.15 - Platform Shared Resource Monitoring: Cache Monitoring Technology".
 
 In Xen's implementation, each domain in the system can be assigned a RMID
 independently, while RMID=0 is reserved for monitoring domains that don't
@@ -91,17 +91,42 @@ For example, assuming a system with 8 portions and 3 domains:
    first domain exclusive access to half the cache, and the other two exclusive
    access to one quarter each.
 
-For more detailed information please refer to Intel SDM chapter
-"17.15 - Platform Shared Resource Control: Cache Allocation Technology".
-
 In Xen's implementation, CBM can be configured with libxl/xl interfaces but
 COS is maintained in hypervisor only. The cache partition granularity is per
 domain, each domain has COS=0 assigned by default, the corresponding CBM is
 all-ones, which means all the cache resource can be used by default.
 
+Code/Data Prioritization (CDP) Technology is an extension of CAT, which is
+available on Intel Broadwell and later server platforms. 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 masks
+per Class of Service (COS).
+
+CDP can be enabled by adding `psr=cdp` to Xen bootline.
+
+When CDP is enabled,
+
+ * the CAT masks are re-mapped into interleaved pairs of masks for data or code
+   fetches.
+
+ * the range of COS for CAT is re-indexed, with the lower-half of the COS
+   range available for CDP.
+
+CDP allows OS or Hypervisor to partition cache allocation more fine-grained,
+code cache and data cache can be specified respectively. With CDP enabled,
+one COS corresponds to two CBMs (code CBM & data CBM), since the sum of CBMs
+is fixed, that means the number of available COSes will reduce to half when
+CDP is on.
+
+For more detailed information please refer to Intel SDM chapter
+"17.16 - Platform Shared Resource Control: Cache Allocation Technology".
+
 ### xl interfaces
 
-System CAT information such as maximum COS and CBM length can be obtained by:
+System CAT information such as maximum COS, CBM length and CDP status can be
+obtained by:
 
 `xl psr-hwinfo --cat`
 
@@ -116,6 +141,15 @@ A cbm is valid only when:
    obtained with `xl psr-hwinfo --cat`.
  * All the set bits are contiguous.
 
+When CDP is enabled, `-c` or `--code` option is available to set code CBM for
+the domain.
+
+When CDP is enabled, `-d` or `--data` option is available to set data CBM for
+the domain.
+
+If neither `-c` nor `-d` option is specified when CDP is on, the same code CBM
+and data CBM will be set for the domain.
+
 In a multi-socket system, the same cbm will be set on each socket by default.
 Per socket cbm can be specified with the `--socket SOCKET` option.
 
-- 
1.9.1

Re: [PATCH v4 1/4] x86: Support enable CDP by boot parameter and add get CDP status

[Andrew Cooper]

On 17/09/15 10:35, He Chen wrote:
> Add boot parameter `psr=cdp` to enable CDP at boot time.
> Intel Code/Data Prioritization(CDP) feature is based on CAT. 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 <he.chen@linux.intel.com>
> ---
>  docs/misc/xen-command-line.markdown | 11 ++++-
>  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         |  4 +-
>  6 files changed, 98 insertions(+), 20 deletions(-)
>
> diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
> index a2e427c..d92e323 100644
> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -1165,9 +1165,9 @@ This option can be specified more than once (up to 8 times at present).
>  > `= <integer>`
>  
>  ### psr (Intel)
> -> `= List of ( cmt:<boolean> | rmid_max:<integer> | cat:<boolean> | cos_max:<integer> )`
> +> `= List of ( cmt:<boolean> | rmid_max:<integer> | cat:<boolean> | cos_max:<integer> | cdp:<boolean> )`
>  
> -> Default: `psr=cmt:0,rmid_max:255,cat:0,cos_max:255`
> +> Default: `psr=cmt:0,rmid_max:255,cat:0,cos_max:255,cdp:0`
>  
>  Platform Shared Resource(PSR) Services.  Intel Haswell and later server
>  platforms offer information about the sharing of resources.
> @@ -1197,6 +1197,13 @@ The following resources are available:
>    the cache allocation.
>    * `cat` instructs Xen to enable/disable Cache Allocation Technology.
>    * `cos_max` indicates the max value for COS ID.
> +* Code and Data Prioritization Technology (Broadwell and later). Information
> +  regarding the code cache and the data cache allocation. CDP is based on CAT.
> +  * `cdp` instructs Xen to enable/disable Code and Data Prioritization. Note
> +    that `cos_max` of CDP is a little different from `cos_max` of CAT. With
> +    CDP, one COS will corespond two CBMs other than one with CAT, due to the
> +    sum of CBMs is fixed, that means actual `cos_max` in use will automatically
> +    reduce to half when CDP is enabled.
>  
>  ### reboot
>  > `= t[riple] | k[bd] | a[cpi] | p[ci] | P[ower] | e[fi] | n[o] [, [w]arm | [c]old]`
> diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c
> index c0daa2e..e44ed8b 100644
> --- a/xen/arch/x86/psr.c
> +++ b/xen/arch/x86/psr.c
> @@ -21,9 +21,16 @@
>  
>  #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;
> +        };
> +    } u;

You can also drop this u which will further reduce the diff later in the
patch.

With this change, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Re: [PATCH v4 2/4] x86: add domctl cmd to set/get CDP code/data CBM

[Andrew Cooper]

On 17/09/15 10:35, He Chen wrote:
> @@ -375,10 +401,48 @@ 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;
> +
> +    for ( cos = 0; cos <= cos_max; cos++ )
> +    {
> +        if( map[cos].ref &&
> +            ((!cdp_enabled && map[cos].u.cbm == cbm_code) ||
> +            (cdp_enabled && map[cos].u.code == cbm_code &&

Correct alignment here would have one extra space, as the ( should match
the inner ( of the line above.

> @@ -387,53 +451,80 @@ int psr_set_l3_cbm(struct domain *d, unsigned int socket, uint64_t cbm)
>      if ( !psr_check_cbm(info->cbm_len, cbm) )
>          return -EINVAL;
>  
> +    if ( !cdp_enabled && (type == PSR_CBM_TYPE_L3_CODE ||
> +                          type == PSR_CBM_TYPE_L3_DATA) )
> +        return -EINVAL;
> +
> +    cos_max = info->cos_max;
>      old_cos = d->arch.psr_cos_ids[socket];
>      map = info->cos_to_cbm;
>  
> -    spin_lock(&info->cbm_lock);
> -
> -    for ( cos = 0; cos <= info->cos_max; cos++ )
> +    switch( type )
>      {
> -        /* 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;
> -            break;
> -        }
> -    }
> +    case PSR_CBM_TYPE_L3:
> +        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:
> +        cbm_code = cbm;
> +        cbm_data = map[old_cos].u.data;
> +        break;
>  
> -    if ( !found )
> -    {
> -        spin_unlock(&info->cbm_lock);
> -        return -EOVERFLOW;
> +    case PSR_CBM_TYPE_L3_DATA:
> +        cbm_code = map[old_cos].u.code;
> +        cbm_data = cbm;
> +        break;
> +
> +    default:
> +            ASSERT_UNREACHABLE();

Alignment here as well.

With these two alignment issues fixed, and the knock-on effect of
dropping .u from the previous patch, Reviewed-by: Andrew Cooper
<andrew.cooper3@citrix.com>

Re: [PATCH v4 3/4] tools: add tools support for Intel CDP

[Andrew Cooper]

On 17/09/15 10:35, He Chen wrote:
> @@ -2798,7 +2798,9 @@ enum xc_psr_cmt_type {
>  typedef enum xc_psr_cmt_type xc_psr_cmt_type;
>  
>  enum xc_psr_cat_type {
> -    XC_PSR_CAT_L3_CBM = 1,
> +    XC_PSR_CAT_L3_CBM  = 1,
> +    XC_PSR_CAT_L3_CODE = 2,
> +    XC_PSR_CAT_L3_DATA = 3,
>  };

No need for the explicit assignments here.  The exact values are not
interesting and guaranteed to be as currently assigned.

>  typedef enum xc_psr_cat_type xc_psr_cat_type;
>
> @@ -306,7 +318,8 @@ int xc_psr_cat_get_domain_data(xc_interface *xch, uint32_t domid,
>  }
>  
>  int xc_psr_cat_get_l3_info(xc_interface *xch, uint32_t socket,
> -                           uint32_t *cos_max, uint32_t *cbm_len)
> +                           uint32_t *cos_max, uint32_t *cbm_len,
> +                           bool *cdp_enabled)
>  {
>      int rc;
>      DECLARE_SYSCTL;
> @@ -320,6 +333,8 @@ int xc_psr_cat_get_l3_info(xc_interface *xch, uint32_t socket,
>      {
>          *cos_max = sysctl.u.psr_cat_op.u.l3_info.cos_max;
>          *cbm_len = sysctl.u.psr_cat_op.u.l3_info.cbm_len;
> +        *cdp_enabled = sysctl.u.psr_cat_op.u.l3_info.flags &
> +                       XEN_SYSCTL_PSR_CAT_L3_CDP;

!!(sysctl.u.psr_cat_op.u.l3_info.flags & XEN_SYSCTL_PSR_CAT_L3_CDP);

To turn it into a proper boolean, rather than a just a non-zero integer.

> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index ebbb9a5..8128f54 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -8390,6 +8390,10 @@ static int psr_cat_hwinfo(void)
>          }
>          printf("%-16s: %u\n", "Socket ID", socketid);
>          printf("%-16s: %uKB\n", "L3 Cache", l3_cache_size);
> +        if (info->cdp_enabled)
> +            printf("%-16s: Enabled\n", "CDP Status");
> +        else
> +            printf("%-16s: Disabled\n", "CDP Status");

printf("%-16s: %sabled\n", "CDP Status", info->cdp_enabled ? "En" : "Dis");

is rather shorter, if you prefer.

>          printf("%-16s: %u\n", "Maximum COS", info->cos_max);
>          printf("%-16s: %u\n", "CBM length", info->cbm_len);
>          printf("%-16s: %#llx\n", "Default CBM",
> @@ -8401,7 +8405,8 @@ out:
>      return rc;
>  }
>  
> -static void psr_cat_print_one_domain_cbm(uint32_t domid, uint32_t socketid)
> +static void psr_cat_print_one_domain_cbm(uint32_t domid, uint32_t socketid,
> +                                         bool cdp_enabled)
>  {
>      char *domain_name;
>      uint64_t cbm;
> @@ -8410,20 +8415,29 @@ static void psr_cat_print_one_domain_cbm(uint32_t domid, uint32_t socketid)
>      printf("%5d%25s", domid, domain_name);
>      free(domain_name);
>  
> -    if (!libxl_psr_cat_get_cbm(ctx, domid, LIBXL_PSR_CBM_TYPE_L3_CBM,
> -                               socketid, &cbm))
> -         printf("%#16"PRIx64, cbm);
> -
> +    if (!cdp_enabled) {
> +        if (!libxl_psr_cat_get_cbm(ctx, domid, LIBXL_PSR_CBM_TYPE_L3_CBM,
> +                                   socketid, &cbm))
> +            printf("%#16"PRIx64, cbm);

Mixing # and an explicit width of the integer will clip two nibbles of
the integer.

In this situation, the correct formatting is "0x%016"PRIx64, or
"%#018"PRIx64

> diff --git a/tools/libxl/xl_cmdtable.c b/tools/libxl/xl_cmdtable.c
> index 0071f12..13e7aa1 100644
> --- a/tools/libxl/xl_cmdtable.c
> +++ b/tools/libxl/xl_cmdtable.c
> @@ -543,6 +543,8 @@ struct cmd_spec cmd_table[] = {
>        "Set cache capacity bitmasks(CBM) for a domain",
>        "[options] <Domain> <CBM>",
>        "-s <socket>       Specify the socket to process, otherwise all sockets are processed\n"
> +      "-c                Set code CBM if CDP is supported\n"
> +      "-d                Set data CBM if CDP is supported\n"
>      },
>      { "psr-cat-show",
>        &main_psr_cat_show, 0, 1,
> @@ -551,6 +553,7 @@ struct cmd_spec cmd_table[] = {
>      },
>  
>  #endif
> +

Spurious whitespace change.

~Andrew

Re: [PATCH v4 3/4] tools: add tools support for Intel CDP

[Ian Campbell]

On Thu, 2015-09-17 at 11:38 +0100, Andrew Cooper wrote:
> On 17/09/15 10:35, He Chen wrote:
> > @@ -2798,7 +2798,9 @@ enum xc_psr_cmt_type {
> >  typedef enum xc_psr_cmt_type xc_psr_cmt_type;
> >  
> >  enum xc_psr_cat_type {
> > -    XC_PSR_CAT_L3_CBM = 1,
> > +    XC_PSR_CAT_L3_CBM  = 1,
> > +    XC_PSR_CAT_L3_CODE = 2,
> > +    XC_PSR_CAT_L3_DATA = 3,
> >  };
> 
> No need for the explicit assignments here.  The exact values are not
> interesting and guaranteed to be as currently assigned.

No harm though I suppose.

> > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> > index ebbb9a5..8128f54 100644
> > --- a/tools/libxl/xl_cmdimpl.c
> > +++ b/tools/libxl/xl_cmdimpl.c
> > @@ -8390,6 +8390,10 @@ static int psr_cat_hwinfo(void)
> >          }
> >          printf("%-16s: %u\n", "Socket ID", socketid);
> >          printf("%-16s: %uKB\n", "L3 Cache", l3_cache_size);
> > +        if (info->cdp_enabled)
> > +            printf("%-16s: Enabled\n", "CDP Status");
> > +        else
> > +            printf("%-16s: Disabled\n", "CDP Status");
> 
> printf("%-16s: %sabled\n", "CDP Status", info->cdp_enabled ? "En" :
> "Dis");
> 
> is rather shorter, if you prefer.

My preference would be along these lines but without the trick regarding
the common suffix on both words, i.e.

printf("%-16s: %s\n", "CDP Status",
       info->cdp_enabled ? "Enabled" : "Disabled");

As well as avoiding splitting the words this avoids duplicating the %-16s
and "CDP Status" compared with the original, which IMHO is the important
thing.

Ian.

Re: [PATCH v4 3/4] tools: add tools support for Intel CDP

[Ian Campbell]

On Thu, 2015-09-17 at 11:38 +0100, Andrew Cooper wrote:
> @@ -320,6 +333,8 @@ int xc_psr_cat_get_l3_info(xc_interface *xch,
> > uint32_t socket,
> >      {
> >          *cos_max = sysctl.u.psr_cat_op.u.l3_info.cos_max;
> >          *cbm_len = sysctl.u.psr_cat_op.u.l3_info.cbm_len;
> > +        *cdp_enabled = sysctl.u.psr_cat_op.u.l3_info.flags &
> > +                       XEN_SYSCTL_PSR_CAT_L3_CDP;
> 
> !!(sysctl.u.psr_cat_op.u.l3_info.flags & XEN_SYSCTL_PSR_CAT_L3_CDP);
> 
> To turn it into a proper boolean, rather than a just a non-zero integer.

Given that *cdp_endabled is bool type does this not happen automagically?

Re: [PATCH v4 3/4] tools: add tools support for Intel CDP

[Ian Campbell]

On Thu, 2015-09-17 at 17:35 +0800, He Chen wrote:
> diff --git a/tools/libxl/libxl_psr.c b/tools/libxl/libxl_psr.c
> index 3378239..62963cf 100644
> --- a/tools/libxl/libxl_psr.c
> +++ b/tools/libxl/libxl_psr.c
> @@ -87,6 +87,9 @@ static void libxl__psr_cat_log_err_msg(libxl__gc *gc,
> int err)
>      case EEXIST:
>          msg = "The same CBM is already set to this domain";
>          break;
> +    case EINVAL:
> +        msg = "Unable to set code or data CBM when CDP is disabled";
> +        break;

These overloading of the errno values are getting a bit thinly stretched.
The more so that EINVAL has a widely used more generic meaning.

Hypervisor maintainers, what is your opinion of this?

Since this is a sysctl I suppose we could consider adding a new PSR
specific error type with appropriate codes?

Re: [PATCH v4 3/4] tools: add tools support for Intel CDP

[Ian Campbell]

On Thu, 2015-09-17 at 17:35 +0800, He Chen wrote:
> @@ -8410,20 +8415,29 @@ static void psr_cat_print_one_domain_cbm(uint32_t
> domid, uint32_t socketid)
>      printf("%5d%25s", domid, domain_name);
>      free(domain_name);
>  
> -    if (!libxl_psr_cat_get_cbm(ctx, domid, LIBXL_PSR_CBM_TYPE_L3_CBM,
> -                               socketid, &cbm))
> -         printf("%#16"PRIx64, cbm);
> -
> +    if (!cdp_enabled) {
> +        if (!libxl_psr_cat_get_cbm(ctx, domid, LIBXL_PSR_CBM_TYPE_L3_CBM,
> +                                   socketid, &cbm))
> +            printf("%#16"PRIx64, cbm);
> +    } else {
> +        if (!libxl_psr_cat_get_cbm(ctx, domid, LIBXL_PSR_CBM_TYPE_L3_CODE,
> +                                   socketid, &cbm))
> +            printf("%10s%#8"PRIx64, "code:", cbm);
> +        if (!libxl_psr_cat_get_cbm(ctx, domid, LIBXL_PSR_CBM_TYPE_L3_DATA,
> +                                   socketid, &cbm))
> +            printf("%10s%#8"PRIx64, "data:", cbm);
> +    }

Does cdp being enabled mean that the original L3_CBM functionality is no
longer available then?

Please could you give an example of the new output format for this command
in the commit message.

>  static int psr_cat_show(uint32_t domid)
> @@ -8489,6 +8503,8 @@ int main_psr_cat_cbm_set(int argc, char **argv)
>      libxl_psr_cbm_type type = LIBXL_PSR_CBM_TYPE_L3_CBM;
>      uint64_t cbm;
>      int ret, opt = 0;
> +    int opt_data = 0;
> +    int opt_code = 0;
>      libxl_bitmap target_map;
>      char *value;
>      libxl_string_list socket_list;
> 
> [...]

> @@ -8517,8 +8535,19 @@ int main_psr_cat_cbm_set(int argc, char **argv)
>          libxl_string_list_dispose(&socket_list);
>          free(value);
>          break;
> +    case 'd':
> +        type = LIBXL_PSR_CBM_TYPE_L3_DATA;
> +        opt_data = 1;
> +        break;
> +    case 'c':
> +        type = LIBXL_PSR_CBM_TYPE_L3_CODE;
> +        opt_code = 1;
> +        break;
>      }
>  
> +    if (opt_data && opt_code)

Do you not mean !opt_data && !opt_code?

But also, isn't this assignment unnecessary since type is initialised to
the same value when it is declared?

In fact, because of that initialisation, aren't opt_data and opt_code
unnecessary, since you set type appropriately elsewhere.

Are -d and -c mutually exclusive, or is it expected that both can be given?

Re: [PATCH v4 3/4] tools: add tools support for Intel CDP

[Andrew Cooper]

On 24/09/15 11:57, Ian Campbell wrote:
> On Thu, 2015-09-17 at 11:38 +0100, Andrew Cooper wrote:
>> @@ -320,6 +333,8 @@ int xc_psr_cat_get_l3_info(xc_interface *xch,
>>> uint32_t socket,
>>>       {
>>>           *cos_max = sysctl.u.psr_cat_op.u.l3_info.cos_max;
>>>           *cbm_len = sysctl.u.psr_cat_op.u.l3_info.cbm_len;
>>> +        *cdp_enabled = sysctl.u.psr_cat_op.u.l3_info.flags &
>>> +                       XEN_SYSCTL_PSR_CAT_L3_CDP;
>> !!(sysctl.u.psr_cat_op.u.l3_info.flags & XEN_SYSCTL_PSR_CAT_L3_CDP);
>>
>> To turn it into a proper boolean, rather than a just a non-zero integer.
> Given that *cdp_endabled is bool type does this not happen automagically?
>

Hmm - spec says yes.  Any assignment of a non-zero integer to a _Bool is 
will be converted to a 1.

~Andrew

Re: [PATCH v4 4/4] docs: add document to introduce CDP command

[Ian Campbell]

On Thu, 2015-09-17 at 17:35 +0800, He Chen wrote:
> Add new CDP options with CAT commands in xl interface man page.
> Add description of CDP in xl-psr.markdown.

It would have been fine to include this in the previous patch by the way.

> 
> Signed-off-by: He Chen <he.chen@linux.intel.com>
> ---
>  docs/man/xl.pod.1         | 14 ++++++++++++++
>  docs/misc/xl-psr.markdown | 44 +++++++++++++++++++++++++++++++++++++++--
> ---
>  2 files changed, 53 insertions(+), 5 deletions(-)
> 
> diff --git a/docs/man/xl.pod.1 b/docs/man/xl.pod.1
> index f22c3f3..3c7107d 100644
> --- a/docs/man/xl.pod.1
> +++ b/docs/man/xl.pod.1
> @@ -1530,6 +1530,12 @@ applications. In the Xen implementation, CAT is
> used to control cache allocation
>  on VM basis. To enforce cache on a specific domain, just set capacity
> bitmasks
>  (CBM) for the domain.
>  
> +Intel Broadwell and later server platforms also offer Code/Data Prioritization
> +(CDP) for cache allocation, which support

                   allocations,

>  specifying code or data cache for

It seems like a word is missing, perhaps "size" as in "code or data case
size"? Or perhaps "priority"?

> +applications. CDP is used on VM basis in the Xen implementation. To specify

"used on a per VM basis"

> +code or data CBM for the domain, CDP feature must be enabled and CBM type
> +options need to be specified when setting CBM.

I asked on patch 3 whether these options were mutually exclusive or not,
the answer should be reflected in the documentation too please.

> diff --git a/docs/misc/xl-psr.markdown b/docs/misc/xl-psr.markdown
> index 3545912..5eb97cc 100644
> --- a/docs/misc/xl-psr.markdown
> +++ b/docs/misc/xl-psr.markdown
> @@ -14,7 +14,7 @@ tracks cache utilization of memory accesses according
> to the RMID and reports
>  monitored data via a counter register.
>  
>  For more detailed information please refer to Intel SDM chapter
> -"17.14 - Platform Shared Resource Monitoring: Cache Monitoring Technology".
> +"17.15 - Platform Shared Resource Monitoring: Cache Monitoring Technology".

Was the chapter number just wrong, or are these things prone to changing?

If the latter then we should either omit them or we need to refer to a
specific revision of the SDM as well.

>  
>  In Xen's implementation, each domain in the system can be assigned a
> RMID
>  independently, while RMID=0 is reserved for monitoring domains that
> don't
> @@ -91,17 +91,42 @@ For example, assuming a system with 8 portions and 3
> domains:
>     first domain exclusive access to half the cache, and the other two
> exclusive
>     access to one quarter each.
>  
> -For more detailed information please refer to Intel SDM chapter
> -"17.15 - Platform Shared Resource Control: Cache Allocation Technology".
> -


>  In Xen's implementation, CBM can be configured with libxl/xl interfaces but
>  COS is maintained in hypervisor only. The cache partition granularity is per
>  domain, each domain has COS=0 assigned by default, the corresponding CBM is
>  all-ones, which means all the cache resource can be used by default.
>  
> +Code/Data Prioritization (CDP) Technology is an extension of CAT, which is
> +available on Intel Broadwell and later server platforms. 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 masks
> +per Class of Service (COS).

Looks good.

> +
> +CDP can be enabled by adding `psr=cdp` to Xen bootline.

"bootline" => "command line"

> +
> +When CDP is enabled,
> +
> + * the CAT masks are re-mapped into interleaved pairs of masks for data or code
> +   fetches.
> +
> + * the range of COS for CAT is re-indexed, with the lower-half of the COS
> +   range available for CDP.
> +
> +CDP allows OS or Hypervisor to partition cache allocation more fine-grained,

             ^the                                           ^in a          

> +code cache and data cache can be specified respectively. With CDP enabled,

End fine-grained with a full-stop and then:

    The code cache and data cache can be specified separately

or s/separately/independently/?

(I think, I'm not 100% sure what you meant by "respectively", so maybe the
suggestion is wrong)

> +one COS corresponds to two CBMs (code CBM & data CBM), since the sum of CBMs
> +is fixed, that means the number of available COSes will reduce to half when

                                                           reduce by half

Ian.

Re: [PATCH v4 3/4] tools: add tools support for Intel CDP

[Ian Campbell]

On Thu, 2015-09-24 at 12:07 +0100, Ian Campbell wrote:
> @@ -8517,8 +8535,19 @@ int main_psr_cat_cbm_set(int argc, char **argv)
> >          libxl_string_list_dispose(&socket_list);
> >          free(value);
> >          break;
> > +    case 'd':
> > +        type = LIBXL_PSR_CBM_TYPE_L3_DATA;
> > +        opt_data = 1;
> > +        break;
> > +    case 'c':
> > +        type = LIBXL_PSR_CBM_TYPE_L3_CODE;
> > +        opt_code = 1;
> > +        break;
> >      }
> >  
> > +    if (opt_data && opt_code)
> 
> Do you not mean !opt_data && !opt_code?
> 
> But also, isn't this assignment unnecessary since type is initialised to
> the same value when it is declared?
> 
> In fact, because of that initialisation, aren't opt_data and opt_code
> unnecessary, since you set type appropriately elsewhere.
> 
> Are -d and -c mutually exclusive, or is it expected that both can be
> given?

Also, is there error checking for passing -c or -d when CDP is not enabled
somewhere else?

Ian.

Re: [PATCH v4 3/4] tools: add tools support for Intel CDP

[Jan Beulich]

>>> On 24.09.15 at 13:00, <ian.campbell@citrix.com> wrote:
> On Thu, 2015-09-17 at 17:35 +0800, He Chen wrote:
>> diff --git a/tools/libxl/libxl_psr.c b/tools/libxl/libxl_psr.c
>> index 3378239..62963cf 100644
>> --- a/tools/libxl/libxl_psr.c
>> +++ b/tools/libxl/libxl_psr.c
>> @@ -87,6 +87,9 @@ static void libxl__psr_cat_log_err_msg(libxl__gc *gc,
>> int err)
>>      case EEXIST:
>>          msg = "The same CBM is already set to this domain";
>>          break;
>> +    case EINVAL:
>> +        msg = "Unable to set code or data CBM when CDP is disabled";
>> +        break;
> 
> These overloading of the errno values are getting a bit thinly stretched.
> The more so that EINVAL has a widely used more generic meaning.
> 
> Hypervisor maintainers, what is your opinion of this?
> 
> Since this is a sysctl I suppose we could consider adding a new PSR
> specific error type with appropriate codes?

I'd prefer using recognizable -E... values; a specific error type
to me seems to go too far. Surely out of the several dozen
possibilities a handful of not-so-common ones can be picked?

Jan

Re: [PATCH v4 4/4] docs: add document to introduce CDP command

[Jan Beulich]

>>> On 24.09.15 at 13:22, <ian.campbell@citrix.com> wrote:
> On Thu, 2015-09-17 at 17:35 +0800, He Chen wrote:
>> Add new CDP options with CAT commands in xl interface man page.
>> --- a/docs/man/xl.pod.1
>> +++ b/docs/man/xl.pod.1
>> @@ -1530,6 +1530,12 @@ applications. In the Xen implementation, CAT is
>> used to control cache allocation
>>  on VM basis. To enforce cache on a specific domain, just set capacity
>> bitmasks
>>  (CBM) for the domain.
>>  
>> +Intel Broadwell and later server platforms also offer Code/Data 
> Prioritization
>> +(CDP) for cache allocation, which support
> 
>                    allocations,
> 
>>  specifying code or data cache for
> 
> It seems like a word is missing, perhaps "size" as in "code or data case
> size"? Or perhaps "priority"?

No, talk is really about the kind of cache.

>> --- a/docs/misc/xl-psr.markdown
>> +++ b/docs/misc/xl-psr.markdown
>> @@ -14,7 +14,7 @@ tracks cache utilization of memory accesses according
>> to the RMID and reports
>>  monitored data via a counter register.
>>  
>>  For more detailed information please refer to Intel SDM chapter
>> -"17.14 - Platform Shared Resource Monitoring: Cache Monitoring Technology".
>> +"17.15 - Platform Shared Resource Monitoring: Cache Monitoring Technology".
> 
> Was the chapter number just wrong, or are these things prone to changing?
> 
> If the latter then we should either omit them or we need to refer to a
> specific revision of the SDM as well.

They change all the time; I previously suggested to Intel folks to
omit the numbers and just make sure the titles get quoted fully
and correctly.

Jan

Re: [PATCH v4 3/4] tools: add tools support for Intel CDP

[Ian Campbell]

On Thu, 2015-09-24 at 05:50 -0600, Jan Beulich wrote:
> > > > On 24.09.15 at 13:00, <ian.campbell@citrix.com> wrote:
> > On Thu, 2015-09-17 at 17:35 +0800, He Chen wrote:
> > > diff --git a/tools/libxl/libxl_psr.c b/tools/libxl/libxl_psr.c
> > > index 3378239..62963cf 100644
> > > --- a/tools/libxl/libxl_psr.c
> > > +++ b/tools/libxl/libxl_psr.c
> > > @@ -87,6 +87,9 @@ static void libxl__psr_cat_log_err_msg(libxl__gc
> > > *gc,
> > > int err)
> > >      case EEXIST:
> > >          msg = "The same CBM is already set to this domain";
> > >          break;
> > > +    case EINVAL:
> > > +        msg = "Unable to set code or data CBM when CDP is disabled";
> > > +        break;
> > 
> > These overloading of the errno values are getting a bit thinly
> > stretched.
> > The more so that EINVAL has a widely used more generic meaning.
> > 
> > Hypervisor maintainers, what is your opinion of this?
> > 
> > Since this is a sysctl I suppose we could consider adding a new PSR
> > specific error type with appropriate codes?
> 
> I'd prefer using recognizable -E... values;

_If_ the -E values somehow map sensibly onto the PSR errors, otherwise they
aren't really recognisable any more.

>  a specific error type
> to me seems to go too far. Surely out of the several dozen
> possibilities a handful of not-so-common ones can be picked?

I was thinking in particular EINVAL was not in the not-so-common bracket.

The current code already uses 9 values FWIW.

Ian.

Re: [PATCH v4 3/4] tools: add tools support for Intel CDP

[Jan Beulich]

>>> On 24.09.15 at 14:07, <ian.campbell@citrix.com> wrote:
> On Thu, 2015-09-24 at 05:50 -0600, Jan Beulich wrote:
>>  a specific error type
>> to me seems to go too far. Surely out of the several dozen
>> possibilities a handful of not-so-common ones can be picked?
> 
> I was thinking in particular EINVAL was not in the not-so-common bracket.

Which I fully agree with.

> The current code already uses 9 values FWIW.

Right, but ENXIO would e.g. seem to be a reasonable fit for
"Unable to set code or data CBM when CDP is disabled".

Jan

Re: [PATCH v4 3/4] tools: add tools support for Intel CDP

[Ian Campbell]

On Thu, 2015-09-24 at 06:20 -0600, Jan Beulich wrote:
> > > > On 24.09.15 at 14:07, <ian.campbell@citrix.com> wrote:
> > On Thu, 2015-09-24 at 05:50 -0600, Jan Beulich wrote:
> > >  a specific error type
> > > to me seems to go too far. Surely out of the several dozen
> > > possibilities a handful of not-so-common ones can be picked?
> > 
> > I was thinking in particular EINVAL was not in the not-so-common
> > bracket.
> 
> Which I fully agree with.
> 
> > The current code already uses 9 values FWIW.
> 
> Right, but ENXIO would e.g. seem to be a reasonable fit for
> "Unable to set code or data CBM when CDP is disabled".

That would be better yes (sorry I didn't check if this came up in the
review of the hypervisor side)

Ian.

Re: [PATCH v4 1/4] x86: Support enable CDP by boot parameter and add get CDP status

[Jan Beulich]

>>> On 17.09.15 at 11:35, <he.chen@linux.intel.com> wrote:
> Add boot parameter `psr=cdp` to enable CDP at boot time.
> Intel Code/Data Prioritization(CDP) feature is based on CAT. 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 <he.chen@linux.intel.com>

Brief list of changes in v4 missing here.

> --- a/xen/arch/x86/psr.c
> +++ b/xen/arch/x86/psr.c
> @@ -21,9 +21,16 @@
>  
>  #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;
> +        };
> +    } u;

Didn't we already agree to remove this unnecessary u?

> @@ -261,8 +270,14 @@ static struct psr_cat_socket_info *get_cat_socket_info(unsigned int socket)
>      return cat_socket_info + socket;
>  }
>  
> +static inline bool_t cdp_is_enabled(unsigned int socket,
> +                                    unsigned long *cdp_socket_enable)
> +{
> +    return (cdp_socket_enable && test_bit(socket, cdp_socket_enable));

Stray pair of parentheses.

> @@ -387,16 +421,16 @@ int psr_set_l3_cbm(struct domain *d, unsigned int socket, uint64_t cbm)
>      }
>  
>      cos = found - map;
> -    if ( found->cbm != cbm )
> +    if ( found->u.cbm != cbm )
>      {
> -        int ret = write_l3_cbm(socket, cos, cbm);
> +        int ret = write_l3_cbm(socket, cos, cbm, 0, 0);

I think it would be more natural to pass cbm twice instead of passing
zero for cbm_data. The change also looks misplaced here, as there's
no other caller of write_l3_cbm() until (presumably) the next patch.

> --- a/xen/include/asm-x86/psr.h
> +++ b/xen/include/asm-x86/psr.h
> @@ -27,6 +27,15 @@
>  /* L3 Monitoring Features */
>  #define PSR_CMT_L3_OCCUPANCY           0x1
>  
> +/* CDP Capability */
> +#define PSR_CAT_CDP_CAPABILITY       (1u << 2)
> +
> +/* L3 CDP Enable bit*/
> +#define PSR_L3_QOS_CDP_ENABLE_BIT       0x0
> +
> +/* L3 CDP flag bit */
> +#define PSR_CAT_FLAG_L3_CDP       (1u << 0)

Afaict this is the same as ...

> @@ -704,6 +704,8 @@ struct xen_sysctl_psr_cat_op {
>          struct {
>              uint32_t cbm_len;   /* OUT: CBM length */
>              uint32_t cos_max;   /* OUT: Maximum COS */
> +            #define XEN_SYSCTL_PSR_CAT_L3_CDP       (1u << 0)

... this. In which case only the latter (public) one should be kept.

Also at the very least the # belongs in column 1.

Jan

Re: [PATCH v4 3/4] tools: add tools support for Intel CDP

[He Chen]

On Thu, Sep 24, 2015 at 12:07:27PM +0100, Ian Campbell wrote:
> On Thu, 2015-09-17 at 17:35 +0800, He Chen wrote:
> > @@ -8410,20 +8415,29 @@ static void psr_cat_print_one_domain_cbm(uint32_t
> > domid, uint32_t socketid)
> >      printf("%5d%25s", domid, domain_name);
> >      free(domain_name);
> >  
> > -    if (!libxl_psr_cat_get_cbm(ctx, domid, LIBXL_PSR_CBM_TYPE_L3_CBM,
> > -                               socketid, &cbm))
> > -         printf("%#16"PRIx64, cbm);
> > -
> > +    if (!cdp_enabled) {
> > +        if (!libxl_psr_cat_get_cbm(ctx, domid, LIBXL_PSR_CBM_TYPE_L3_CBM,
> > +                                   socketid, &cbm))
> > +            printf("%#16"PRIx64, cbm);
> > +    } else {
> > +        if (!libxl_psr_cat_get_cbm(ctx, domid, LIBXL_PSR_CBM_TYPE_L3_CODE,
> > +                                   socketid, &cbm))
> > +            printf("%10s%#8"PRIx64, "code:", cbm);
> > +        if (!libxl_psr_cat_get_cbm(ctx, domid, LIBXL_PSR_CBM_TYPE_L3_DATA,
> > +                                   socketid, &cbm))
> > +            printf("%10s%#8"PRIx64, "data:", cbm);
> > +    }
> 
> Does cdp being enabled mean that the original L3_CBM functionality is no
> longer available then?
> 
> Please could you give an example of the new output format for this command
> in the commit message.
> 

For the get side, the answer is Yes. But for the set side, L3_CBM means that
setting the same code CBM and data CBM when CDP is enabled.

> >  static int psr_cat_show(uint32_t domid)
> > @@ -8489,6 +8503,8 @@ int main_psr_cat_cbm_set(int argc, char **argv)
> >      libxl_psr_cbm_type type = LIBXL_PSR_CBM_TYPE_L3_CBM;
> >      uint64_t cbm;
> >      int ret, opt = 0;
> > +    int opt_data = 0;
> > +    int opt_code = 0;
> >      libxl_bitmap target_map;
> >      char *value;
> >      libxl_string_list socket_list;
> > 
> > [...]
> 
> > @@ -8517,8 +8535,19 @@ int main_psr_cat_cbm_set(int argc, char **argv)
> >          libxl_string_list_dispose(&socket_list);
> >          free(value);
> >          break;
> > +    case 'd':
> > +        type = LIBXL_PSR_CBM_TYPE_L3_DATA;
> > +        opt_data = 1;
> > +        break;
> > +    case 'c':
> > +        type = LIBXL_PSR_CBM_TYPE_L3_CODE;
> > +        opt_code = 1;
> > +        break;
> >      }
> >  
> > +    if (opt_data && opt_code)
> 
> Do you not mean !opt_data && !opt_code?
> 
> But also, isn't this assignment unnecessary since type is initialised to
> the same value when it is declared?
> 
> In fact, because of that initialisation, aren't opt_data and opt_code
> unnecessary, since you set type appropriately elsewhere.
> 
> Are -d and -c mutually exclusive, or is it expected that both can be given?
> 

-d and -c can be both given.

The initialisation of type is L3_CBM, which corresponds to the situation
that neither -d nor -c is given.

I add `if (opt_data && opt_code)` to address the situation that -d and -c
are both given.
If user gives both -d and -c options, image that without if() statement,
-d will be overwritten by the latter -c in switch, and type will be
L3_CODE instead of L3_CBM (means set both and what user wants).

I hope I had made myself clear, or is there something wrong with my
understanding?

Thanks.

> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH v4 3/4] tools: add tools support for Intel CDP

[He Chen]

On Thu, Sep 24, 2015 at 12:22:47PM +0100, Ian Campbell wrote:
> On Thu, 2015-09-24 at 12:07 +0100, Ian Campbell wrote:
> > @@ -8517,8 +8535,19 @@ int main_psr_cat_cbm_set(int argc, char **argv)
> > >          libxl_string_list_dispose(&socket_list);
> > >          free(value);
> > >          break;
> > > +    case 'd':
> > > +        type = LIBXL_PSR_CBM_TYPE_L3_DATA;
> > > +        opt_data = 1;
> > > +        break;
> > > +    case 'c':
> > > +        type = LIBXL_PSR_CBM_TYPE_L3_CODE;
> > > +        opt_code = 1;
> > > +        break;
> > >      }
> > >  
> > > +    if (opt_data && opt_code)
> > 
> > Do you not mean !opt_data && !opt_code?
> > 
> > But also, isn't this assignment unnecessary since type is initialised to
> > the same value when it is declared?
> > 
> > In fact, because of that initialisation, aren't opt_data and opt_code
> > unnecessary, since you set type appropriately elsewhere.
> > 
> > Are -d and -c mutually exclusive, or is it expected that both can be
> > given?
> 
> Also, is there error checking for passing -c or -d when CDP is not enabled
> somewhere else?
> 
> Ian.

Yes, there is error checking in hypervisor.
hypervisor would reture an error code if -c or -d is given when CDP is
not enable.

In fact, the reture error code would be catched by
`libxl__psr_cat_log_err_msg`, and that is exactly what you and Jan
discussed in Patch 3.

Re: [PATCH v4 3/4] tools: add tools support for Intel CDP

[Ian Campbell]

On Fri, 2015-09-25 at 16:43 +0800, He Chen wrote:
> On Thu, Sep 24, 2015 at 12:07:27PM +0100, Ian Campbell wrote:
> > On Thu, 2015-09-17 at 17:35 +0800, He Chen wrote:
> > > @@ -8410,20 +8415,29 @@ static void
> > > psr_cat_print_one_domain_cbm(uint32_t
> > > domid, uint32_t socketid)
> > >      printf("%5d%25s", domid, domain_name);
> > >      free(domain_name);
> > >  
> > > -    if (!libxl_psr_cat_get_cbm(ctx, domid,
> > > LIBXL_PSR_CBM_TYPE_L3_CBM,
> > > -                               socketid, &cbm))
> > > -         printf("%#16"PRIx64, cbm);
> > > -
> > > +    if (!cdp_enabled) {
> > > +        if (!libxl_psr_cat_get_cbm(ctx, domid,
> > > LIBXL_PSR_CBM_TYPE_L3_CBM,
> > > +                                   socketid, &cbm))
> > > +            printf("%#16"PRIx64, cbm);
> > > +    } else {
> > > +        if (!libxl_psr_cat_get_cbm(ctx, domid,
> > > LIBXL_PSR_CBM_TYPE_L3_CODE,
> > > +                                   socketid, &cbm))
> > > +            printf("%10s%#8"PRIx64, "code:", cbm);
> > > +        if (!libxl_psr_cat_get_cbm(ctx, domid,
> > > LIBXL_PSR_CBM_TYPE_L3_DATA,
> > > +                                   socketid, &cbm))
> > > +            printf("%10s%#8"PRIx64, "data:", cbm);
> > > +    }
> > 
> > Does cdp being enabled mean that the original L3_CBM functionality is
> > no
> > longer available then?
> > 
> > Please could you give an example of the new output format for this
> > command
> > in the commit message.
> > 
> 
> For the get side, the answer is Yes. But for the set side, L3_CBM means
> that
> setting the same code CBM and data CBM when CDP is enabled.
> 
> > >  static int psr_cat_show(uint32_t domid)
> > > @@ -8489,6 +8503,8 @@ int main_psr_cat_cbm_set(int argc, char **argv)
> > >      libxl_psr_cbm_type type = LIBXL_PSR_CBM_TYPE_L3_CBM;
> > >      uint64_t cbm;
> > >      int ret, opt = 0;
> > > +    int opt_data = 0;
> > > +    int opt_code = 0;
> > >      libxl_bitmap target_map;
> > >      char *value;
> > >      libxl_string_list socket_list;
> > > 
> > > [...]
> > 
> > > @@ -8517,8 +8535,19 @@ int main_psr_cat_cbm_set(int argc, char
> > > **argv)
> > >          libxl_string_list_dispose(&socket_list);
> > >          free(value);
> > >          break;
> > > +    case 'd':
> > > +        type = LIBXL_PSR_CBM_TYPE_L3_DATA;
> > > +        opt_data = 1;
> > > +        break;
> > > +    case 'c':
> > > +        type = LIBXL_PSR_CBM_TYPE_L3_CODE;
> > > +        opt_code = 1;
> > > +        break;
> > >      }
> > >  
> > > +    if (opt_data && opt_code)
> > 
> > Do you not mean !opt_data && !opt_code?
> > 
> > But also, isn't this assignment unnecessary since type is initialised
> > to
> > the same value when it is declared?
> > 
> > In fact, because of that initialisation, aren't opt_data and opt_code
> > unnecessary, since you set type appropriately elsewhere.
> > 
> > Are -d and -c mutually exclusive, or is it expected that both can be
> > given?
> > 
> 
> -d and -c can be both given.
> 
> The initialisation of type is L3_CBM, which corresponds to the situation
> that neither -d nor -c is given.
> 
> I add `if (opt_data && opt_code)` to address the situation that -d and -c
> are both given.
> If user gives both -d and -c options, image that without if() statement,
> -d will be overwritten by the latter -c in switch, and type will be
> L3_CODE instead of L3_CBM (means set both and what user wants).
> 
> I hope I had made myself clear, or is there something wrong with my
> understanding?

Quoting the relevant bits of code for clarity:
     libxl_psr_cbm_type type = LIBXL_PSR_CBM_TYPE_L3_CBM;
    ...
    case 'd':
        type = LIBXL_PSR_CBM_TYPE_L3_DATA;
        opt_data = 1;
        break;
    case 'c':
        type = LIBXL_PSR_CBM_TYPE_L3_CODE;
        opt_code = 1;
        break;
     }
 
    if (opt_data && opt_code)
        type = LIBXL_PSR_CBM_TYPE_L3_CBM;

So the behaviour if -d and -c are given is exactly the same as if neither
of them were given, i.e. type = LIBXL_PSR_CBM_TYPE_L3_CBM? Is that correct
and intended?

If so then I think it would be clearer to only set opt_* during option
parsing and then to figure out the correct LIBXL_PSR_CBM_TYPE_* explicitly
afterwards, rather than have the code cycle through data->code->cbm.

Or just outlaw passing both -d and -c together since it is confusing and
equivalent to passing neither anyway.

Ian.

Re: [PATCH v4 3/4] tools: add tools support for Intel CDP

[Ian Campbell]

On Fri, 2015-09-25 at 17:04 +0800, He Chen wrote:
> On Thu, Sep 24, 2015 at 12:22:47PM +0100, Ian Campbell wrote:
> > On Thu, 2015-09-24 at 12:07 +0100, Ian Campbell wrote:
> > > @@ -8517,8 +8535,19 @@ int main_psr_cat_cbm_set(int argc, char
> > > **argv)
> > > >          libxl_string_list_dispose(&socket_list);
> > > >          free(value);
> > > >          break;
> > > > +    case 'd':
> > > > +        type = LIBXL_PSR_CBM_TYPE_L3_DATA;
> > > > +        opt_data = 1;
> > > > +        break;
> > > > +    case 'c':
> > > > +        type = LIBXL_PSR_CBM_TYPE_L3_CODE;
> > > > +        opt_code = 1;
> > > > +        break;
> > > >      }
> > > >  
> > > > +    if (opt_data && opt_code)
> > > 
> > > Do you not mean !opt_data && !opt_code?
> > > 
> > > But also, isn't this assignment unnecessary since type is initialised
> > > to
> > > the same value when it is declared?
> > > 
> > > In fact, because of that initialisation, aren't opt_data and opt_code
> > > unnecessary, since you set type appropriately elsewhere.
> > > 
> > > Are -d and -c mutually exclusive, or is it expected that both can be
> > > given?
> > 
> > Also, is there error checking for passing -c or -d when CDP is not
> > enabled
> > somewhere else?
> > 
> > Ian.
> 
> Yes, there is error checking in hypervisor.
> hypervisor would reture an error code if -c or -d is given when CDP is
> not enable.
> 
> In fact, the reture error code would be catched by
> `libxl__psr_cat_log_err_msg`, and that is exactly what you and Jan
> discussed in Patch 3.

Great, thanks.

Re: [PATCH v4 4/4] docs: add document to introduce CDP command

[He Chen]

On Thu, Sep 24, 2015 at 12:22:02PM +0100, Ian Campbell wrote:
> On Thu, 2015-09-17 at 17:35 +0800, He Chen wrote:
> > Add new CDP options with CAT commands in xl interface man page.
> > Add description of CDP in xl-psr.markdown.
> 
> It would have been fine to include this in the previous patch by the way.
> 

You mean include both xl man page and xl-psr.markdown in the previous
patch? Or just only xl man page?

> > +code or data CBM for the domain, CDP feature must be enabled and CBM type
> > +options need to be specified when setting CBM.
> 
> I asked on patch 3 whether these options were mutually exclusive or not,
> the answer should be reflected in the documentation too please.
> 

Agreed.
Both code and data options can be specified at the same time.
I will make it clear in the documentation as possible as I can.

> > +
> > +When CDP is enabled,
> > +
> > + * the CAT masks are re-mapped into interleaved pairs of masks for data or code
> > +   fetches.
> > +
> > + * the range of COS for CAT is re-indexed, with the lower-half of the COS
> > +   range available for CDP.
> > +
> > +CDP allows OS or Hypervisor to partition cache allocation more fine-grained,
> 
>              ^the                                           ^in a          
> 
> > +code cache and data cache can be specified respectively. With CDP enabled,
> 
> End fine-grained with a full-stop and then:
> 
>     The code cache and data cache can be specified separately
> 
> or s/separately/independently/?
> 
> (I think, I'm not 100% sure what you meant by "respectively", so maybe the
> suggestion is wrong)
> 

Maybe I make words a little confused here.
With CDP enabled, user can specify code cache (or data cache) only, and
the other would remain previous value (if no previous value, it would
keep as default value).
So, I think independently would be fine here.

Re: [PATCH v4 3/4] tools: add tools support for Intel CDP

[He Chen]

> Quoting the relevant bits of code for clarity:
>      libxl_psr_cbm_type type = LIBXL_PSR_CBM_TYPE_L3_CBM;
>     ...
>     case 'd':
>         type = LIBXL_PSR_CBM_TYPE_L3_DATA;
>         opt_data = 1;
>         break;
>     case 'c':
>         type = LIBXL_PSR_CBM_TYPE_L3_CODE;
>         opt_code = 1;
>         break;
>      }
>  
>     if (opt_data && opt_code)
>         type = LIBXL_PSR_CBM_TYPE_L3_CBM;
> 
> So the behaviour if -d and -c are given is exactly the same as if neither
> of them were given, i.e. type = LIBXL_PSR_CBM_TYPE_L3_CBM? Is that correct
> and intended?

Yes.

> If so then I think it would be clearer to only set opt_* during option
> parsing and then to figure out the correct LIBXL_PSR_CBM_TYPE_* explicitly
> afterwards, rather than have the code cycle through data->code->cbm.
> 
> Or just outlaw passing both -d and -c together since it is confusing and
> equivalent to passing neither anyway.

Yes, as you said, if user just passes one option -d (or -c), things would
be done during option parsing, there is no need to add the if().

But the key point is that I am not sure how to address outlaw passing both
-d and -c together (is it allowed?). If it is permitted, the behaviour is
the same as passing neither indeed, and the if() is needed to avoid latter
option overwritting former option.

What's your suggestion? Sorry, I am a little confused.
Omit former opiton when both options are given and remove if()?
Or something else?

Thanks for your time.

Re: [PATCH v4 4/4] docs: add document to introduce CDP command

[Ian Campbell]

On Fri, 2015-09-25 at 17:29 +0800, He Chen wrote:
> On Thu, Sep 24, 2015 at 12:22:02PM +0100, Ian Campbell wrote:
> > On Thu, 2015-09-17 at 17:35 +0800, He Chen wrote:
> > > Add new CDP options with CAT commands in xl interface man page.
> > > Add description of CDP in xl-psr.markdown.
> > 
> > It would have been fine to include this in the previous patch by the
> > way.
> > 
> 
> You mean include both xl man page and xl-psr.markdown in the previous
> patch? Or just only xl man page?

All of the docs could validly be included with the patch which introduces
the feature. i.e. you could fold patch #4 into #3 completely.

You don't have to if you don't want to.

> Maybe I make words a little confused here.
> With CDP enabled, user can specify code cache (or data cache) only, and
> the other would remain previous value (if no previous value, it would
> keep as default value).

The value is just enabled or disabled, so do I understand correctly that
this is what you can do if cdp is enabled:

    [now: code cbm=<default>; data cbm=<default>]

    xl psr-cat-cbm-set -c <domid> 0xdeadbeed
    [now: code cbm=0xdeadbeef; data cbm=<default>]

    xl psr-cat-cbm-set -d <domid> 0xf00fb00f
    [now: code cbm=0xdeadbeef; data cbm=0xf00fb00f]

    xl psr-cat-cbm-set -c -d <domid> 0xd00dfeed

    [now: code cbm=0xd00dfeed; data cbm=0xd00dfeed]

    xl psr-cat-cbm-set <domid> 0xee11ee11

    [now: code cbm=0xee11ee11; data cbm=0xee11ee11]

Is that right?

And if cdp is not enabled:

    [now: cbm=<default>]

    xl psr-cat-cbm-set -c <domid> 0xdeadbeed
    *ERRROR*
    [now: cbm=<default>]

xl psr-cat-cbm-set -d <domid> 0xf00fb00f
    *ERRROR*
    [now: cbm=<default>]

xl psr-cat-cbm-set -c -d <domid> 0xd00dfeed

*ERRROR*
[now: cbm=<default>]

xl psr-cat-cbm-set <domid> 0xee11ee11

[now: cbm=0xee11ee11]

> So, I think independently would be fine here.

If I'm correct in all the above then yes.

Ian.

Re: [PATCH v4 4/4] docs: add document to introduce CDP command

[He Chen]

On Fri, Sep 25, 2015 at 10:58:58AM +0100, Ian Campbell wrote:
> On Fri, 2015-09-25 at 17:29 +0800, He Chen wrote:
> > On Thu, Sep 24, 2015 at 12:22:02PM +0100, Ian Campbell wrote:
> > > On Thu, 2015-09-17 at 17:35 +0800, He Chen wrote:
> > > > Add new CDP options with CAT commands in xl interface man page.
> > > > Add description of CDP in xl-psr.markdown.
> > > 
> > > It would have been fine to include this in the previous patch by the
> > > way.
> > > 
> > 
> > You mean include both xl man page and xl-psr.markdown in the previous
> > patch? Or just only xl man page?
> 
> All of the docs could validly be included with the patch which introduces
> the feature. i.e. you could fold patch #4 into #3 completely.
> 
> You don't have to if you don't want to.
> 

Thanks for your reminding, I will merge patch #3 and #4 in next version.

> > Maybe I make words a little confused here.
> > With CDP enabled, user can specify code cache (or data cache) only, and
> > the other would remain previous value (if no previous value, it would
> > keep as default value).
> 
> The value is just enabled or disabled, so do I understand correctly that
> this is what you can do if cdp is enabled:
> 
>     [now: code cbm=<default>; data cbm=<default>]
> 
>     xl psr-cat-cbm-set -c <domid> 0xdeadbeed
>     [now: code cbm=0xdeadbeef; data cbm=<default>]
> 
>     xl psr-cat-cbm-set -d <domid> 0xf00fb00f
>     [now: code cbm=0xdeadbeef; data cbm=0xf00fb00f]
> 
>     xl psr-cat-cbm-set -c -d <domid> 0xd00dfeed
> 
>     [now: code cbm=0xd00dfeed; data cbm=0xd00dfeed]
> 
>     xl psr-cat-cbm-set <domid> 0xee11ee11
> 
>     [now: code cbm=0xee11ee11; data cbm=0xee11ee11]
> 
> Is that right?

Yes.

> 
> And if cdp is not enabled:
> 
>     [now: cbm=<default>]
> 
>     xl psr-cat-cbm-set -c <domid> 0xdeadbeed
>     *ERRROR*
>     [now: cbm=<default>]
> 
> xl psr-cat-cbm-set -d <domid> 0xf00fb00f
>     *ERRROR*
>     [now: cbm=<default>]
> 

Right above.

> xl psr-cat-cbm-set -c -d <domid> 0xd00dfeed
> 
> *ERRROR*
> [now: cbm=<default>]
> 

In current code, it is valid since -c & -d have the same behaviour as
neither of them.
So, it will not show error, and cbm=0xd00dfeed

What do you think of this? Is it proper to do so?

> xl psr-cat-cbm-set <domid> 0xee11ee11
> 
> [now: cbm=0xee11ee11]
> 

Right.

> > So, I think independently would be fine here.
> 
> If I'm correct in all the above then yes.
> 
> Ian.

Re: [PATCH v4 3/4] tools: add tools support for Intel CDP

[Ian Campbell]

On Fri, 2015-09-25 at 17:53 +0800, He Chen wrote:
> > Quoting the relevant bits of code for clarity:
> >      libxl_psr_cbm_type type = LIBXL_PSR_CBM_TYPE_L3_CBM;
> >     ...
> >     case 'd':
> >         type = LIBXL_PSR_CBM_TYPE_L3_DATA;
> >         opt_data = 1;
> >         break;
> >     case 'c':
> >         type = LIBXL_PSR_CBM_TYPE_L3_CODE;
> >         opt_code = 1;
> >         break;
> >      }
> >  
> >     if (opt_data && opt_code)
> >         type = LIBXL_PSR_CBM_TYPE_L3_CBM;
> > 
> > So the behaviour if -d and -c are given is exactly the same as if
> > neither
> > of them were given, i.e. type = LIBXL_PSR_CBM_TYPE_L3_CBM? Is that
> > correct
> > and intended?
> 
> Yes.
> 
> > If so then I think it would be clearer to only set opt_* during option
> > parsing and then to figure out the correct LIBXL_PSR_CBM_TYPE_*
> > explicitly
> > afterwards, rather than have the code cycle through data->code->cbm.
> > 
> > Or just outlaw passing both -d and -c together since it is confusing
> > and
> > equivalent to passing neither anyway.
> 
> Yes, as you said, if user just passes one option -d (or -c), things would
> be done during option parsing, there is no need to add the if().
> 
> But the key point is that I am not sure how to address outlaw passing
> both
> -d and -c together (is it allowed?). If it is permitted, the behaviour is
> the same as passing neither indeed, and the if() is needed to avoid
> latter
> option overwritting former option.
> 
> What's your suggestion? Sorry, I am a little confused.
> Omit former opiton when both options are given and remove if()?
> Or something else?

I was trying to make one suggestion for restructuring the code and one
design choice to make, let me see if I can clarify.

I think the basic code structure should be:

    libxl_psr_cbm_type type;
    int opt_data = 0, opt_code = 1;
    [...]
    case 'd':
        opt_data = 1;
        break;
    case 'c':
        opt_code = 1;
            break;
[...]

    [... now figure out correct type= based on opt_data + opt+code... ]
Which separates the option parsing from the logic of what they mean.

Then the choice I mentioned is whether passing -c and -d at the same
time is valid or not.

If you want passing both -c and -d at the same time to be invalid then the
code would be something like:

    if (opt_data && opt_code) {
        log error and exit
    } else if (opt_data) {
        type = LIBXL_PSR_CBM_TYPE_L3_DATA;
    } else if (opt_code) {
        type = LIBXL_PSR_CBM_TYPE_L3_CODE;
    else { /* Neither */
        type = LIBXL_PSR_CBM_TYPE_L3_CBM;
    }

If you want passing both -c and -d to be valid and behave like passing
neither then it would be something like:

    if (opt_data && opt_code) {
        type = LIBXL_PSR_CBM_TYPE_L3_CBM;
    } else if
    (opt_data) {
        type = LIBXL_PSR_CBM_TYPE_L3_DATA;
    } else if (opt_code) {
        type =
        LIBXL_PSR_CBM_TYPE_L3_CODE;
    else { /* Neither, same as both */
        type =
        LIBXL_PSR_CBM_TYPE_L3_CBM;
    }

Which one you use is up to you, depending on what you think the most sensible semantics are.

Ian.

Re: [PATCH v4 4/4] docs: add document to introduce CDP command

[Ian Campbell]

On Fri, 2015-09-25 at 18:16 +0800, He Chen wrote:
> 
[...]
> > And if cdp is not enabled:
> > 
[...]
> Right above.
> 
> > xl psr-cat-cbm-set -c -d <domid> 0xd00dfeed
> > 
> > *ERRROR*
> > [now: cbm=<default>]
> > 
> 
> In current code, it is valid since -c & -d have the same behaviour as
> neither of them.
> So, it will not show error, and cbm=0xd00dfeed

I think with cdp disabled this seems like surprising behaviour, but you
might want to argue that when CDP is disabled the single CBM acts like  a
unified code and data CBM and it therefore makes sense to act this way.

If you agree this behaviour is surprising then the simplest answer would be
to disallow -c and -d together in both cdp enabled and disabled
configurations (since otherwise you would have to ask Xen if it was on to
validate the options, which is too much faff).

If you think it makes sense then you could leave it as is.

> What do you think of this? Is it proper to do so?

Like I say I found it surprising, but I'm not an expert in CDP/CBM or what
their semantics should be, so my surprise might be misplaced. Please do
feel free to disagree if you think the correct behaviour is as the current
behaviour (or indeed something else).

Ultimately if you have considered these corner cases and have an opinion on
what the right behaviour is then I'm happy to go with what you choose.

Ian.