detect and initialize CDP (Code/Data Prioritization) feature

tools/libxl/xl_cmdimpl.c +0 -1

@@ -8445,29 +8447,46 @@ out:
     return rc;
 }
 
-static void psr_cat_print_one_domain_cbm(uint32_t domid, uint32_t
 socketid)

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

Reviewed-by Andrew Cooper <[email protected]>

---
Changes in v7:
* write CDP data mask (mask1) before turning CDP on
* check CDP status before clear enabled bit in function cat_cpu_fini

xen/arch/x86/psr.c static bool_t psr_check_cbm(unsigned int cbm_len, uint64_t cbm) · line 363

Quoted 10 lines

Quoted 1 line

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

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

@@ -331,19 +357,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;
 };

The bool_t field really fits well in the gap between the first and second one; I'll take the liberty to adjust this when committing (unless Chao comes forward with further comments; otherwise I'm kind of hoping for a Reviewed-by-him).

Jan

tools/libxl/xl_cmdimpl.c +0 -0

         printf("%-16s: %u\n", "Maximum COS", info[i].cos_max);
         printf("%-16s: %u\n", "CBM length", info[i].cbm_len);
         printf("%-16s: %#llx\n", "Default CBM",

@@ -8445,29 +8447,46 @@ out:
     return rc;
 }

Quoted 7 lines

tools/libxl/xl_cmdimpl.c +0 -0

index 365798b..8db0132 100644
@@ -8434,6 +8434,8 @@ static int psr_cat_hwinfo(void)
         }
         printf("%-16s: %u\n", "Socket ID", info[i].id);

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

Quoted 3 lines

tools/libxl/xl_cmdimpl.c +2 -0

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

Sorry I didn't notice this before but I guess 'info->cdp_enabled' here really would be 'info[i].cdp_enabled' as CDP status is per-socket. Current code will always print the data for the first socket in each iteration, which is undesirable.

With this and the indention issue proposed by Ian being fixed:

Reviewed-by Chao Peng <[email protected]>

Quoted 6 lines

tools/libxl/xl_cmdimpl.c +0 -0

         printf("%-16s: %u\n", "Maximum COS", info[i].cos_max);
         printf("%-16s: %u\n", "CBM length", info[i].cbm_len);
         printf("%-16s: %#llx\n", "Default CBM",

@@ -8445,29 +8447,46 @@ out:
     return rc;
 }

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

index c32e25c..9b08ee3 100644
@@ -127,6 +127,59 @@ Per domain CBM settings can be shown by:
 
 `xl psr-cat-show`
 
+## Code and Data Prioritization (CDP)
+
+Code and 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

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

+cache in a software configurable manner, which can enable workload
+prioritization and tuning of cache capacity to the characteristics

of the

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

+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 command line.
+
+When CDP is enabled,
+
+ * the CAT masks are re-mapped into interleaved pairs of masks for

data or

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

+   code fetches.
+
+ * the range of COS for CAT is re-indexed, with the lower-half of

the COS

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

+   range available for CDP.
+
+CDP allows the OS or Hypervisor to partition cache allocation in a

more
+fine-grained.

Quoted 35 lines · He Chen

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

index c32e25c..9b08ee3 100644
@@ -127,6 +127,59 @@ Per domain CBM settings can be shown by:
 
 `xl psr-cat-show`
 
+## Code and Data Prioritization (CDP)
+
+Code and 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

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

+cache in a software configurable manner, which can enable workload
+prioritization and tuning of cache capacity to the characteristics

of the

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

+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 command line.
+
+When CDP is enabled,
+
+ * the CAT masks are re-mapped into interleaved pairs of masks for

data or

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

+   code fetches.
+
+ * the range of COS for CAT is re-indexed, with the lower-half of

the COS

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

+   range available for CDP.
+
+CDP allows the OS or Hypervisor to partition cache allocation in a

Quoted 3 lines

more
+fine-grained.

"In a more fine-grained" "way"? Or "manner"?

(I'm not a native speaker, though, so if that's not the case, sorry for the noise.)

Regards,
Dario

Quoted 1 line

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

This is the xl/xc changes to support Intel Code/Data Prioritization. CAT xl commands to set/get CBMs are extended to support CDP. 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]>

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

+int psr_set_l3_cbm(struct domain *d, unsigned int socket,
+                   uint64_t cbm, enum cbm_type type)
 {
 
+    uint64_t cbm_data, cbm_code;
 
+    switch ( type )
     {
 
+    case PSR_CBM_TYPE_L3:
+        cbm_code = cbm;
+        break;
 
+    case PSR_CBM_TYPE_L3_CODE:
+        cbm_code = cbm;

Quoted 15 lines · He Chen

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

+int psr_set_l3_cbm(struct domain *d, unsigned int socket,
+                   uint64_t cbm, enum cbm_type type)
 {
 
+    uint64_t cbm_data, cbm_code;
 
+    switch ( type )
     {
 
+    case PSR_CBM_TYPE_L3:
+        cbm_code = cbm;
+        break;
 
+    case PSR_CBM_TYPE_L3_CODE:
+        cbm_code = cbm;

xen/arch/x86/psr.c int psr_set_l3_cbm(struct domain *d, unsigned int socket, uint64_t cbm) · line 483

Quoted 10 lines

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

+        break;
 
+    case PSR_CBM_TYPE_L3_DATA:
+        cbm_code = map[old_cos].code;
+        break;
+
+    default:
+        ASSERT_UNREACHABLE();
 
+    cos = find_cos(map, cos_max, cbm_code, cbm_data, cdp_enabled);

gcc5 does not like ASSERT_UNREACHABLE(), whatever is in staging fails:

[ 2144s] psr.c: In function 'psr_set_l3_cbm':
[ 2144s] psr.c:381:25: error: 'cbm_code' may be used uninitialized in this function [-Werror=maybe-uninitialized]
[ 2144s] struct cos_cbm_info info =
[ 2144s] ^
[ 2144s] psr.c:442:24: note: 'cbm_code' was declared here
[ 2144s] uint64_t cbm_data, cbm_code;
[ 2144s] ^
[ 2144s] psr.c:513:27: error: 'cbm_data' may be used uninitialized in this function [-Werror=maybe-uninitialized]
[ 2144s] map[cos].data = cbm_data;
[ 2144s] ^
[ 2145s] cc1: all warnings being treated as errors
[ 2145s] /home/abuild/rpmbuild/BUILD/xen-4.7.31795/non-dbg/xen/Rules.mk:174: recipe for target 'psr.o' failed [ 2145s] make[4]: *** [psr.o] Error 1

Havent checked if there is already a fix in-flight.

Olaf

Quoted 1 line · Olaf Hering

gcc5 does not like ASSERT_UNREACHABLE(), whatever is in staging fails:

Is it because in non-debug build ASSERT_UNREACHABLE is nop?

Quoted 3 lines · Wei Liu

Quoted 1 line · Olaf Hering

gcc5 does not like ASSERT_UNREACHABLE(), whatever is in staging fails:

Is it because in non-debug build ASSERT_UNREACHABLE is nop?

That might explain it.
Does the staging build test catch this bug anyway?

Olaf

Quoted 6 lines · Olaf Hering

Quoted 3 lines · Wei Liu

Quoted 1 line · Olaf Hering

gcc5 does not like ASSERT_UNREACHABLE(), whatever is in staging fails:

Is it because in non-debug build ASSERT_UNREACHABLE is nop?

Quoted 3 lines

That might explain it.
Does the staging build test catch this bug anyway?

No, because 1) staging is not using GCC5, 2) staging has debug=y.

Wei.

Quoted 1 line

Olaf

Quoted 1 line

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

Quoted 30 lines · Olaf Hering

Quoted 15 lines · He Chen

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

+int psr_set_l3_cbm(struct domain *d, unsigned int socket,
+                   uint64_t cbm, enum cbm_type type)
 {
 
+    uint64_t cbm_data, cbm_code;
 
+    switch ( type )
     {
 
+    case PSR_CBM_TYPE_L3:
+        cbm_code = cbm;
+        break;
 
+    case PSR_CBM_TYPE_L3_CODE:
+        cbm_code = cbm;

xen/arch/x86/psr.c int psr_set_l3_cbm(struct domain *d, unsigned int socket, uint64_t cbm) · line 483

Quoted 10 lines

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

+        break;
 
+    case PSR_CBM_TYPE_L3_DATA:
+        cbm_code = map[old_cos].code;
+        break;
+
+    default:
+        ASSERT_UNREACHABLE();
 
+    cos = find_cos(map, cos_max, cbm_code, cbm_data, cdp_enabled);

gcc5 does not like ASSERT_UNREACHABLE(), whatever is in staging fails:

Quoted 14 lines

[ 2144s] psr.c: In function 'psr_set_l3_cbm':
[ 2144s] psr.c:381:25: error: 'cbm_code' may be used uninitialized in this function [-Werror=maybe-uninitialized]
[ 2144s] struct cos_cbm_info info =
[ 2144s] ^
[ 2144s] psr.c:442:24: note: 'cbm_code' was declared here
[ 2144s] uint64_t cbm_data, cbm_code;
[ 2144s] ^
[ 2144s] psr.c:513:27: error: 'cbm_data' may be used uninitialized in this function [-Werror=maybe-uninitialized]
[ 2144s] map[cos].data = cbm_data;
[ 2144s] ^
[ 2145s] cc1: all warnings being treated as errors
[ 2145s] /home/abuild/rpmbuild/BUILD/xen-4.7.31795/non-dbg/xen/Rules.mk:174: recipe for target 'psr.o' failed [ 2145s] make[4]: *** [psr.o] Error 1

Is it because in non-debug build ASSERT_UNREACHABLE is nop?

Ah, yes, in cases like this it should always be followed by return (or whatever else is suitable). Sorry for not having spotted this during review.

Quoted 1 line

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

Quoted 5 lines · Jan Beulich

Ah, yes, in cases like this it should always be followed by return (or whatever else is suitable). Sorry for not having spotted this during review.

Sorry for this bug. Is it proper to fix this bug by just adding a return after ASSERT_UNREACHABLE? Or do some changes in ASSERT_UNREACHABLE?

No, because 1) staging is not using GCC5, 2) staging has debug=y.