detect and initialize CDP (Code/Data Prioritization) feature

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 <[email protected]>

---
 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(-)

docs/misc/xen-command-line.markdown +9 -2

index a2e427c..d92e323 100644
@@ -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]`

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

index c0daa2e..e44ed8b 100644
@@ -21,9 +21,16 @@
 
 #define PSR_CMT        (1<<0)
 #define PSR_CAT        (1<<1)
+#define PSR_CDP        (1<<2)

Quoted 1 line

Quoted 1 line · On 17.09.15 at 11:35, <[email protected]> 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 <[email protected]>

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

@@ -21,9 +21,16 @@
 
 #define PSR_CMT        (1<<0)
 #define PSR_CAT        (1<<1)
+#define PSR_CDP        (1<<2)

xen/include/asm-x86/psr.h +0 -0

@@ -27,6 +27,15 @@
 /* L3 Monitoring Features */

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

@@ -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)

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

@@ -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 )

tools/libxc/include/xenctrl.h +2 -1

 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;

tools/libxl/xl_cmdimpl.c +2 -1

         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);

tools/libxl/xl_cmdtable.c +2 -0

index 0071f12..13e7aa1 100644
@@ -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,

Quoted 3 lines · He Chen

tools/libxc/include/xenctrl.h +0 -0

@@ -2798,7 +2798,9 @@ enum xc_psr_cmt_type {
 typedef enum xc_psr_cmt_type xc_psr_cmt_type;
 

tools/libxc/include/xenctrl.h enum xc_psr_cmt_type { · line 2804

Quoted 6 lines

tools/libxc/include/xenctrl.h +3 -1

 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.

tools/libxl/xl_cmdimpl.c static int psr_cat_hwinfo(void) · line 8396

Quoted 12 lines

tools/libxl/xl_cmdimpl.c +4 -0

index ebbb9a5..8128f54 100644
@@ -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.

(quoted diff) +0 -0

@@ -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;

tools/libxc/xc_psr.c int xc_psr_cat_get_l3_info(xc_interface *xch, uint32_t socket, · line 337

Quoted 2 lines

tools/libxc/xc_psr.c +2 -0

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

Quoted 7 lines · Andrew Cooper

(quoted diff) +0 -0

@@ -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;

Quoted 2 lines

tools/libxc/xc_psr.c +2 -0

+        *cdp_enabled = sysctl.u.psr_cat_op.u.l3_info.flags &
+                       XEN_SYSCTL_PSR_CAT_L3_CDP;

Quoted 3 lines

!!(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?

Quoted 1 line

Quoted 1 line · On 24.09.15 at 13:00, <[email protected]> wrote:

tools/libxl/libxl_psr.c static void libxl__psr_cat_log_err_msg(libxl__gc *gc, · line 93

Quoted 12 lines · He Chen

tools/libxl/libxl_psr.c +3 -0

index 3378239..62963cf 100644
@@ -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?

Quoted 20 lines

Quoted 1 line

Quoted 1 line · On 24.09.15 at 13:00, <[email protected]> wrote:

tools/libxl/libxl_psr.c static void libxl__psr_cat_log_err_msg(libxl__gc · line 94

Quoted 13 lines · He Chen

tools/libxl/libxl_psr.c +3 -0

index 3378239..62963cf 100644
@@ -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.

Quoted 4 lines

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?

Quoted 1 line

Quoted 1 line · On 24.09.15 at 14:07, <[email protected]> wrote:

Quoted 3 lines · Jan Beulich

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.

Quoted 8 lines

Quoted 1 line

Quoted 1 line · On 24.09.15 at 14:07, <[email protected]> wrote:

Quoted 3 lines · Jan Beulich

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.

Quoted 1 line

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

tools/libxl/xl_cmdimpl.c +2 -4

@@ -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,

tools/libxl/xl_cmdimpl.c +3 -0

 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':

(quoted diff) +0 -0

@@ -8517,8 +8535,19 @@ int main_psr_cat_cbm_set(int argc, char **argv)
 >          libxl_string_list_dispose(&socket_list);
 >          free(value);
 >          break;

tools/libxl/xl_cmdimpl.c int main_psr_cat_cbm_set(int argc, char **argv) · line 8548

Quoted 11 lines

tools/libxl/xl_cmdimpl.c +9 -0

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

Quoted 24 lines · Ian Campbell

(quoted diff) +0 -0

@@ -8517,8 +8535,19 @@ int main_psr_cat_cbm_set(int argc, char **argv)
 >          libxl_string_list_dispose(&socket_list);
 >          free(value);
 >          break;

tools/libxl/xl_cmdimpl.c int main_psr_cat_cbm_set(int argc, char **argv) · line 8548

Quoted 11 lines

tools/libxl/xl_cmdimpl.c +9 -0

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

Quoted 2 lines

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.

Quoted 27 lines · Ian Campbell

Quoted 26 lines · Ian Campbell

(quoted diff) +0 -0

@@ -8517,8 +8535,19 @@ int main_psr_cat_cbm_set(int argc, char
 **argv)
 >          libxl_string_list_dispose(&socket_list);
 >          free(value);
 >          break;

tools/libxl/xl_cmdimpl.c int main_psr_cat_cbm_set(int argc, char **argv) · line 8548

Quoted 11 lines

tools/libxl/xl_cmdimpl.c +9 -0

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

Quoted 8 lines

Quoted 2 lines

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.

Quoted 11 lines · He Chen

tools/libxl/xl_cmdimpl.c +2 -4

@@ -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,

tools/libxl/xl_cmdimpl.c static void psr_cat_print_one_domain_cbm(uint32_t · line 8430

Quoted 10 lines

tools/libxl/xl_cmdimpl.c +10 -0

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

Quoted 18 lines

tools/libxl/xl_cmdimpl.c +3 -0

 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':

tools/libxl/xl_cmdimpl.c int main_psr_cat_cbm_set(int argc, char **argv) · line 8548

Quoted 10 lines

tools/libxl/xl_cmdimpl.c +8 -0

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

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

Quoted 32 lines · Ian Campbell

Quoted 23 lines · He Chen

(quoted diff) +0 -0

@@ -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,

tools/libxl/xl_cmdimpl.c +2 -3

-                               socketid, &cbm))
-         printf("%#16"PRIx64, cbm);
-
+    if (!cdp_enabled) {
+        if (!libxl_psr_cat_get_cbm(ctx, domid,

LIBXL_PSR_CBM_TYPE_L3_CBM,

tools/libxl/xl_cmdimpl.c +4 -0

+                                   socketid, &cbm))
+            printf("%#16"PRIx64, cbm);
+    } else {
+        if (!libxl_psr_cat_get_cbm(ctx, domid,

LIBXL_PSR_CBM_TYPE_L3_CODE,

tools/libxl/xl_cmdimpl.c +3 -0

+                                   socketid, &cbm))
+            printf("%10s%#8"PRIx64, "code:", cbm);
+        if (!libxl_psr_cat_get_cbm(ctx, domid,

LIBXL_PSR_CBM_TYPE_L3_DATA,

tools/libxl/xl_cmdimpl.c static void psr_cat_print_one_domain_cbm(uint32_t domid, uint32_t socketid) · line 8429

Quoted 3 lines

tools/libxl/xl_cmdimpl.c +3 -0

+                                   socketid, &cbm))
+            printf("%10s%#8"PRIx64, "data:", cbm);
+    }

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

Quoted 4 lines

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.

Quoted 36 lines

Quoted 19 lines

static int psr_cat_show(uint32_t domid)

tools/libxl/xl_cmdimpl.c +3 -0

@@ -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':

tools/libxl/xl_cmdimpl.c int main_psr_cat_cbm_set(int argc, char · line 8549

Quoted 10 lines

tools/libxl/xl_cmdimpl.c +8 -0

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

Quoted 6 lines

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.

Quoted 16 lines

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;

Quoted 5 lines

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.

Quoted 8 lines

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?

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 <[email protected]>

---
 docs/man/xl.pod.1         | 14 ++++++++++++++
 docs/misc/xl-psr.markdown | 44 +++++++++++++++++++++++++++++++++++++++--
---
 2 files changed, 53 insertions(+), 5 deletions(-)

docs/man/xl.pod.1 +0 -0

index f22c3f3..3c7107d 100644
@@ -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.

specifying code or data cache for

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

docs/misc/xl-psr.markdown +0 -0

index 3545912..5eb97cc 100644
@@ -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

docs/misc/xl-psr.markdown +0 -0

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

docs/misc/xl-psr.markdown +0 -3

-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

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

Quoted 1 line

Quoted 1 line · On 24.09.15 at 13:22, <[email protected]> wrote:

Quoted 10 lines · He Chen

Add new CDP options with CAT commands in xl interface man page.

docs/man/xl.pod.1 +0 -0

@@ -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

Quoted 1 line

+(CDP) for cache allocation, which support

allocations,

Quoted 1 line

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"?

Quoted 6 lines

docs/misc/xl-psr.markdown +0 -0

@@ -14,7 +14,7 @@ tracks cache utilization of memory accesses according

to the RMID and reports
monitored data via a counter register.

Quoted 3 lines

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.

Quoted 2 lines · He Chen

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.

docs/man/xl.pod.1 applications. In the Xen implementation, CAT is used to control cache allocation · line 1537

Quoted 2 lines

docs/man/xl.pod.1 +2 -0

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

docs/misc/xl-psr.markdown For example, assuming a system with 8 portions and 3 domains: · line 117

Quoted 10 lines

docs/misc/xl-psr.markdown +10 -0

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

Quoted 1 line

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

Quoted 7 lines · Ian Campbell

Quoted 2 lines · He Chen

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?

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.

Quoted 9 lines · He Chen

Quoted 7 lines · Ian Campbell

Quoted 2 lines · He Chen

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.

Quoted 2 lines

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.

Quoted 4 lines

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]

Quoted 1 line

So, I think independently would be fine here.

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

Ian.

Quoted 2 lines

And if cdp is not enabled:

Right above.

Quoted 5 lines

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?