[PATCH v10 0/4] enable Memory Bandwidth Monitoring (MBM) for VMs

[PATCH v10 0/4] enable Memory Bandwidth Monitoring (MBM) for VMs

[Chao Peng]

Changes from v9:
* Move libxc refactoring code into standalone patch;
* Make libxl get_sample interface more generic;

Changes from v8:
* Merge event mask patch to MBM enabling patch;
* Address comments from Ian Campbell(Detail in patch itself).

Changes from v7:
* Make obfuscating more complex as Jan suggested.
* Minor adjustment for commit message.

Changes from v6:
* Obfuscate the read value of MSR_IA32_TSC by adding a booting random;
* Minor coding style/comments adjustment;

Changes from v5:
* Remove common IRQ disable flag but instead disable IRQ when other MSR
  read followed by MSR_IA32_TSC read;
* Add comments for special handle for MSR_IA32_TSC;

Changes from v4:
* Make the counter read and timestamp read atomic by disable IRQ;
* Treat MSR_IA32_TSC as a special case and return NOW() for read path;
* Add MBM description in xl command line.

Changes from v3:
* Get timestamp information from host along with the monitoring counter;
  This is required for counter overlow detection.
* Address comments from Wei on the last patch.

Changes from v2:
* Remove the usage of "static" to cache data in xc;
  NOTE: Other places that already existed before are not touched due to
        the needs for API change. Will fix in separate patch if desirable.
* Coding style;

Changes from v1:
* Move event type check from xc to xl;
* Add retry capability for MBM sampling;
* Fix Coding style/docs;

Hypervisor part for this serial is already in, this contains only tools
side changes.

Intel Memory Bandwidth Monitoring(MBM) is a new hardware feature
which builds on the CMT infrastructure to allow monitoring of system
memory bandwidth. Event codes are provided to monitor both "total"
and "local" bandwidth, meaning bandwidth over QPI and other external
links can be monitored.

For XEN, MBM is used to monitor memory bandwidth for VMs. Due to its
dependency on CMT, the software also makes use of most of CMT codes.
Actually, besides introducing two additional events and some cpuid
feature bits, there are no extra changes compared to cache occupancy
monitoring in CMT. Due to this, CMT should be enabled first to use
this feature.

For interface changes, the patch serial introduces a new command
"XEN_SYSCTL_PSR_CMT_get_l3_event_mask" which exposes MBM feature
capability to user space and modified "resource_op" to support reading
host system time together with the monitored counter.

On the tool stack side, two additional options introduced for
"xl psr-cmt-show":
total_mem_bandwidth:     Show total memory bandwidth
local_mem_bandwidth:     Show local memory bandwidth

The usage flow keeps the same with CMT.


Chao Peng (4):
  tools: correct coding style for psr
  tools/libxc: code refactoring in xc_psr_cmt_get_data
  tools/libxl: code refactoring for MBM
  tools, docs: add total/local memory bandwith monitoring

 docs/man/xl.pod.1                   |  11 +++-
 docs/misc/xen-command-line.markdown |   3 +
 tools/libxc/include/xenctrl.h       |  14 +++--
 tools/libxc/xc_msr_x86.h            |   1 +
 tools/libxc/xc_psr.c                |  76 ++++++++++++++++++------
 tools/libxl/libxl.h                 |  28 +++++++--
 tools/libxl/libxl_psr.c             |  59 +++++++++++++++----
 tools/libxl/libxl_types.idl         |   2 +
 tools/libxl/xl_cmdimpl.c            | 113 +++++++++++++++++++++++++++++-------
 tools/libxl/xl_cmdtable.c           |   4 +-
 10 files changed, 253 insertions(+), 58 deletions(-)

-- 
1.9.1

[PATCH v10 1/4] tools: correct coding style for psr

[Chao Peng]

- space: remove space after '(' or before ')' in 'if' condition;
- indention: align function definition/call arguments;

Signed-off-by: Chao Peng <chao.p.peng@linux.intel.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxc/include/xenctrl.h | 10 +++++-----
 tools/libxc/xc_psr.c          | 10 +++++-----
 tools/libxl/libxl.h           | 11 +++++++----
 tools/libxl/libxl_psr.c       | 11 +++++++----
 tools/libxl/xl_cmdimpl.c      | 11 ++++++-----
 5 files changed, 30 insertions(+), 23 deletions(-)

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 790db53..09d819f 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -2693,14 +2693,14 @@ typedef enum xc_psr_cmt_type xc_psr_cmt_type;
 int xc_psr_cmt_attach(xc_interface *xch, uint32_t domid);
 int xc_psr_cmt_detach(xc_interface *xch, uint32_t domid);
 int xc_psr_cmt_get_domain_rmid(xc_interface *xch, uint32_t domid,
-    uint32_t *rmid);
+                               uint32_t *rmid);
 int xc_psr_cmt_get_total_rmid(xc_interface *xch, uint32_t *total_rmid);
 int xc_psr_cmt_get_l3_upscaling_factor(xc_interface *xch,
-    uint32_t *upscaling_factor);
+                                       uint32_t *upscaling_factor);
 int xc_psr_cmt_get_l3_cache_size(xc_interface *xch, uint32_t cpu,
-    uint32_t *l3_cache_size);
-int xc_psr_cmt_get_data(xc_interface *xch, uint32_t rmid,
-    uint32_t cpu, uint32_t psr_cmt_type, uint64_t *monitor_data);
+                                 uint32_t *l3_cache_size);
+int xc_psr_cmt_get_data(xc_interface *xch, uint32_t rmid, uint32_t cpu,
+                        uint32_t psr_cmt_type, uint64_t *monitor_data);
 int xc_psr_cmt_enabled(xc_interface *xch);
 #endif
 
diff --git a/tools/libxc/xc_psr.c b/tools/libxc/xc_psr.c
index 872e6dc..cfae172 100644
--- a/tools/libxc/xc_psr.c
+++ b/tools/libxc/xc_psr.c
@@ -47,7 +47,7 @@ int xc_psr_cmt_detach(xc_interface *xch, uint32_t domid)
 }
 
 int xc_psr_cmt_get_domain_rmid(xc_interface *xch, uint32_t domid,
-                                    uint32_t *rmid)
+                               uint32_t *rmid)
 {
     int rc;
     DECLARE_DOMCTL;
@@ -88,7 +88,7 @@ int xc_psr_cmt_get_total_rmid(xc_interface *xch, uint32_t *total_rmid)
 }
 
 int xc_psr_cmt_get_l3_upscaling_factor(xc_interface *xch,
-                                            uint32_t *upscaling_factor)
+                                       uint32_t *upscaling_factor)
 {
     static int val = 0;
     int rc;
@@ -113,7 +113,7 @@ int xc_psr_cmt_get_l3_upscaling_factor(xc_interface *xch,
 }
 
 int xc_psr_cmt_get_l3_cache_size(xc_interface *xch, uint32_t cpu,
-                                      uint32_t *l3_cache_size)
+                                 uint32_t *l3_cache_size)
 {
     static int val = 0;
     int rc;
@@ -138,8 +138,8 @@ int xc_psr_cmt_get_l3_cache_size(xc_interface *xch, uint32_t cpu,
     return rc;
 }
 
-int xc_psr_cmt_get_data(xc_interface *xch, uint32_t rmid,
-    uint32_t cpu, xc_psr_cmt_type type, uint64_t *monitor_data)
+int xc_psr_cmt_get_data(xc_interface *xch, uint32_t rmid, uint32_t cpu,
+                        xc_psr_cmt_type type, uint64_t *monitor_data)
 {
     xc_resource_op_t op;
     xc_resource_entry_t entries[2];
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index c219f59..f784df5 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -1476,10 +1476,13 @@ int libxl_psr_cmt_detach(libxl_ctx *ctx, uint32_t domid);
 int libxl_psr_cmt_domain_attached(libxl_ctx *ctx, uint32_t domid);
 int libxl_psr_cmt_enabled(libxl_ctx *ctx);
 int libxl_psr_cmt_get_total_rmid(libxl_ctx *ctx, uint32_t *total_rmid);
-int libxl_psr_cmt_get_l3_cache_size(libxl_ctx *ctx, uint32_t socketid,
-    uint32_t *l3_cache_size);
-int libxl_psr_cmt_get_cache_occupancy(libxl_ctx *ctx, uint32_t domid,
-    uint32_t socketid, uint32_t *l3_cache_occupancy);
+int libxl_psr_cmt_get_l3_cache_size(libxl_ctx *ctx,
+                                    uint32_t socketid,
+                                    uint32_t *l3_cache_size);
+int libxl_psr_cmt_get_cache_occupancy(libxl_ctx *ctx,
+                                      uint32_t domid,
+                                      uint32_t socketid,
+                                      uint32_t *l3_cache_occupancy);
 #endif
 
 /* misc */
diff --git a/tools/libxl/libxl_psr.c b/tools/libxl/libxl_psr.c
index 0437465..ec3b6e9 100644
--- a/tools/libxl/libxl_psr.c
+++ b/tools/libxl/libxl_psr.c
@@ -135,8 +135,9 @@ int libxl_psr_cmt_get_total_rmid(libxl_ctx *ctx, uint32_t *total_rmid)
     return rc;
 }
 
-int libxl_psr_cmt_get_l3_cache_size(libxl_ctx *ctx, uint32_t socketid,
-                                         uint32_t *l3_cache_size)
+int libxl_psr_cmt_get_l3_cache_size(libxl_ctx *ctx,
+                                    uint32_t socketid,
+                                    uint32_t *l3_cache_size)
 {
     GC_INIT(ctx);
 
@@ -160,8 +161,10 @@ out:
     return rc;
 }
 
-int libxl_psr_cmt_get_cache_occupancy(libxl_ctx *ctx, uint32_t domid,
-    uint32_t socketid, uint32_t *l3_cache_occupancy)
+int libxl_psr_cmt_get_cache_occupancy(libxl_ctx *ctx,
+                                      uint32_t domid,
+                                      uint32_t socketid,
+                                      uint32_t *l3_cache_occupancy)
 {
     GC_INIT(ctx);
 
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 440db78..8b41093 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -7823,7 +7823,7 @@ out:
 
 #ifdef LIBXL_HAVE_PSR_CMT
 static void psr_cmt_print_domain_cache_occupancy(libxl_dominfo *dominfo,
-                                                    uint32_t nr_sockets)
+                                                 uint32_t nr_sockets)
 {
     char *domain_name;
     uint32_t socketid;
@@ -7837,8 +7837,8 @@ static void psr_cmt_print_domain_cache_occupancy(libxl_dominfo *dominfo,
     free(domain_name);
 
     for (socketid = 0; socketid < nr_sockets; socketid++) {
-        if ( !libxl_psr_cmt_get_cache_occupancy(ctx, dominfo->domid,
-                 socketid, &l3_cache_occupancy) )
+        if (!libxl_psr_cmt_get_cache_occupancy(ctx, dominfo->domid, socketid,
+                                               &l3_cache_occupancy))
             printf("%13u KB", l3_cache_occupancy);
     }
 
@@ -7886,8 +7886,9 @@ static int psr_cmt_show_cache_occupancy(uint32_t domid)
     for (socketid = 0; socketid < nr_sockets; socketid++) {
         rc = libxl_psr_cmt_get_l3_cache_size(ctx, socketid, &l3_cache_size);
         if (rc < 0) {
-            fprintf(stderr, "Failed to get system l3 cache size for socket:%d\n",
-                            socketid);
+            fprintf(stderr,
+                    "Failed to get system l3 cache size for socket:%d\n",
+                    socketid);
             return -1;
         }
         printf("%13u KB", l3_cache_size);
-- 
1.9.1

[PATCH v10 2/4] tools/libxc: code refactoring in xc_psr_cmt_get_data

[Chao Peng]

Use calculated array index instead of hardcoded array index.
No functional change involved.

Signed-off-by: Chao Peng <chao.p.peng@linux.intel.com>
---
 tools/libxc/xc_psr.c | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/tools/libxc/xc_psr.c b/tools/libxc/xc_psr.c
index cfae172..70d9067 100644
--- a/tools/libxc/xc_psr.c
+++ b/tools/libxc/xc_psr.c
@@ -143,7 +143,7 @@ int xc_psr_cmt_get_data(xc_interface *xch, uint32_t rmid, uint32_t cpu,
 {
     xc_resource_op_t op;
     xc_resource_entry_t entries[2];
-    uint32_t evtid;
+    uint32_t evtid, nr = 0;
     int rc;
 
     switch ( type )
@@ -155,25 +155,27 @@ int xc_psr_cmt_get_data(xc_interface *xch, uint32_t rmid, uint32_t cpu,
         return -1;
     }
 
-    entries[0].u.cmd = XEN_RESOURCE_OP_MSR_WRITE;
-    entries[0].idx = MSR_IA32_CMT_EVTSEL;
-    entries[0].val = (uint64_t)rmid << 32 | evtid;
-    entries[0].rsvd = 0;
+    entries[nr].u.cmd = XEN_RESOURCE_OP_MSR_WRITE;
+    entries[nr].idx = MSR_IA32_CMT_EVTSEL;
+    entries[nr].val = (uint64_t)rmid << 32 | evtid;
+    entries[nr].rsvd = 0;
+    nr++;
 
-    entries[1].u.cmd = XEN_RESOURCE_OP_MSR_READ;
-    entries[1].idx = MSR_IA32_CMT_CTR;
-    entries[1].val = 0;
-    entries[1].rsvd = 0;
+    entries[nr].u.cmd = XEN_RESOURCE_OP_MSR_READ;
+    entries[nr].idx = MSR_IA32_CMT_CTR;
+    entries[nr].val = 0;
+    entries[nr].rsvd = 0;
+    nr++;
 
     op.cpu = cpu;
-    op.nr_entries = 2;
+    op.nr_entries = nr;
     op.entries = entries;
 
     rc = xc_resource_op(xch, 1, &op);
     if ( rc < 0 )
         return rc;
 
-    if ( op.result !=2 || entries[1].val & IA32_CMT_CTR_ERROR_MASK )
+    if ( op.result != nr || entries[1].val & IA32_CMT_CTR_ERROR_MASK )
         return -1;
 
     *monitor_data = entries[1].val;
-- 
1.9.1

[PATCH v10 3/4] tools/libxl: code refactoring for MBM

[Chao Peng]

Make some internal routines common so that total/local memory bandwidth
monitoring in the next patch can make use of them.

Signed-off-by: Chao Peng <chao.p.peng@linux.intel.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
---
Changes in v10:
1. Merge libxl change into next patch.
2. Minor function name changes to make them more generic.
---
 tools/libxl/xl_cmdimpl.c | 54 +++++++++++++++++++++++++++++-------------------
 1 file changed, 33 insertions(+), 21 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 8b41093..846a4b2 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -7822,8 +7822,9 @@ out:
 }
 
 #ifdef LIBXL_HAVE_PSR_CMT
-static void psr_cmt_print_domain_cache_occupancy(libxl_dominfo *dominfo,
-                                                 uint32_t nr_sockets)
+static void psr_cmt_print_domain_info(libxl_dominfo *dominfo,
+                                      libxl_psr_cmt_type type,
+                                      uint32_t nr_sockets)
 {
     char *domain_name;
     uint32_t socketid;
@@ -7837,15 +7838,23 @@ static void psr_cmt_print_domain_cache_occupancy(libxl_dominfo *dominfo,
     free(domain_name);
 
     for (socketid = 0; socketid < nr_sockets; socketid++) {
-        if (!libxl_psr_cmt_get_cache_occupancy(ctx, dominfo->domid, socketid,
-                                               &l3_cache_occupancy))
-            printf("%13u KB", l3_cache_occupancy);
+        switch (type) {
+        case LIBXL_PSR_CMT_TYPE_CACHE_OCCUPANCY:
+            if (!libxl_psr_cmt_get_cache_occupancy(ctx,
+                                                   dominfo->domid,
+                                                   socketid,
+                                                   &l3_cache_occupancy))
+                printf("%13u KB", l3_cache_occupancy);
+            break;
+        default:
+            return;
+        }
     }
 
     printf("\n");
 }
 
-static int psr_cmt_show_cache_occupancy(uint32_t domid)
+static int psr_cmt_show(libxl_psr_cmt_type type, uint32_t domid)
 {
     uint32_t i, socketid, nr_sockets, total_rmid;
     uint32_t l3_cache_size;
@@ -7881,19 +7890,22 @@ static int psr_cmt_show_cache_occupancy(uint32_t domid)
         printf("%14s %d", "Socket", socketid);
     printf("\n");
 
-    /* Total L3 cache size */
-    printf("%-46s", "Total L3 Cache Size");
-    for (socketid = 0; socketid < nr_sockets; socketid++) {
-        rc = libxl_psr_cmt_get_l3_cache_size(ctx, socketid, &l3_cache_size);
-        if (rc < 0) {
-            fprintf(stderr,
-                    "Failed to get system l3 cache size for socket:%d\n",
-                    socketid);
-            return -1;
-        }
-        printf("%13u KB", l3_cache_size);
+    if (type == LIBXL_PSR_CMT_TYPE_CACHE_OCCUPANCY) {
+            /* Total L3 cache size */
+            printf("%-46s", "Total L3 Cache Size");
+            for (socketid = 0; socketid < nr_sockets; socketid++) {
+                rc = libxl_psr_cmt_get_l3_cache_size(ctx, socketid,
+                                                     &l3_cache_size);
+                if (rc < 0) {
+                    fprintf(stderr,
+                            "Failed to get system l3 cache size for socket:%d\n",
+                            socketid);
+                    return -1;
+                }
+                printf("%13u KB", l3_cache_size);
+            }
+            printf("\n");
     }
-    printf("\n");
 
     /* Each domain */
     if (domid != INVALID_DOMID) {
@@ -7902,7 +7914,7 @@ static int psr_cmt_show_cache_occupancy(uint32_t domid)
             fprintf(stderr, "Failed to get domain info for %d\n", domid);
             return -1;
         }
-        psr_cmt_print_domain_cache_occupancy(&dominfo, nr_sockets);
+        psr_cmt_print_domain_info(&dominfo, type, nr_sockets);
     }
     else
     {
@@ -7912,7 +7924,7 @@ static int psr_cmt_show_cache_occupancy(uint32_t domid)
             return -1;
         }
         for (i = 0; i < nr_domains; i++)
-            psr_cmt_print_domain_cache_occupancy(list + i, nr_sockets);
+            psr_cmt_print_domain_info(list + i, type, nr_sockets);
         libxl_dominfo_list_free(list, nr_domains);
     }
     return 0;
@@ -7971,7 +7983,7 @@ int main_psr_cmt_show(int argc, char **argv)
 
     switch (type) {
     case LIBXL_PSR_CMT_TYPE_CACHE_OCCUPANCY:
-        ret = psr_cmt_show_cache_occupancy(domid);
+        ret = psr_cmt_show(type, domid);
         break;
     default:
         help("psr-cmt-show");
-- 
1.9.1

[PATCH v10 4/4] tools, docs: add total/local memory bandwith monitoring

[Chao Peng]

Add Memory Bandwidth Monitoring(MBM) for VMs. Two types of monitoring
are supported: total and local memory bandwidth monitoring. To use it,
CMT should be enabled in hypervisor.

Signed-off-by: Chao Peng <chao.p.peng@linux.intel.com>
---
Changes in v10:
1. Move refactoring code into standalone patch.
2. Create generic interface libxl_psr_cmt_get_sample for both
   cache_occupancy and memory bandwith.
Changes in v9:
1. Refactor code in xc_psr_cmt_get_data.
2. Move bandwidth calculation(sleep) from libxl to xl.
3. Broadcast feature with LIBXL_HAVE_PSR_MBM.
4. Check event mask with libxl_psr_cmt_type_supported.
5. Coding style/Document fix.
Changes in v6:
1. Remove DISABLE_IRQ flag as hypervisor disable IRQ for MSR_IA32_TSC
   implicitly.
Changes in v5:
1. Add MBM description in xen command line.
2. Use the tsc from hypervisor directly which is already ns.
3. Call resource_op with DISABLE_IRQ flag.
Changes in v4:
1. Get timestamp from hypervisor and use that for bandwidth calculation.
2. Minor document and coding style fix.
---
 docs/man/xl.pod.1                   | 11 +++++-
 docs/misc/xen-command-line.markdown |  3 ++
 tools/libxc/include/xenctrl.h       |  6 +++-
 tools/libxc/xc_msr_x86.h            |  1 +
 tools/libxc/xc_psr.c                | 44 +++++++++++++++++++++--
 tools/libxl/libxl.h                 | 17 +++++++++
 tools/libxl/libxl_psr.c             | 56 +++++++++++++++++++++++------
 tools/libxl/libxl_types.idl         |  2 ++
 tools/libxl/xl_cmdimpl.c            | 72 +++++++++++++++++++++++++++++++++----
 tools/libxl/xl_cmdtable.c           |  4 ++-
 10 files changed, 195 insertions(+), 21 deletions(-)

diff --git a/docs/man/xl.pod.1 b/docs/man/xl.pod.1
index 6b89ba8..cd80ffc 100644
--- a/docs/man/xl.pod.1
+++ b/docs/man/xl.pod.1
@@ -1461,6 +1461,13 @@ is domain level. To monitor a specific domain, just attach the domain id with
 the monitoring service. When the domain doesn't need to be monitored any more,
 detach the domain id from the monitoring service.
 
+Intel Broadwell and later server platforms also offer total/local memory
+bandwidth monitoring. Xen supports per-domain monitoring for these two
+additional monitoring types. Both memory bandwidth monitoring and L3 cache
+occupancy monitoring share the same set of underlying monitoring service. Once
+a domain is attached to the monitoring service, monitoring data can be showed
+for any of these monitoring types.
+
 =over 4
 
 =item B<psr-cmt-attach> [I<domain-id>]
@@ -1475,7 +1482,9 @@ detach: Detach the platform shared resource monitoring service from a domain.
 
 Show monitoring data for a certain domain or all domains. Current supported
 monitor types are:
- - "cache-occupancy": showing the L3 cache occupancy.
+ - "cache-occupancy": showing the L3 cache occupancy(KB).
+ - "total-mem-bandwidth": showing the total memory bandwidth(KB/s).
+ - "local-mem-bandwidth": showing the local memory bandwidth(KB/s).
 
 =back
 
diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index bc316be..a09ec01 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -1097,6 +1097,9 @@ The following resources are available:
   L3 cache occupancy.
   * `cmt` instructs Xen to enable/disable Cache Monitoring Technology.
   * `rmid_max` indicates the max value for rmid.
+* Memory Bandwidth Monitoring (Broadwell and later). Information regarding the
+  total/local memory bandwidth. Follow the same options with Cache Monitoring
+  Technology.
 
 ### reboot
 > `= t[riple] | k[bd] | a[cpi] | p[ci] | n[o] [, [w]arm | [c]old]`
diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 09d819f..54043ee 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -2688,6 +2688,8 @@ int xc_resource_op(xc_interface *xch, uint32_t nr_ops, xc_resource_op_t *ops);
 #if defined(__i386__) || defined(__x86_64__)
 enum xc_psr_cmt_type {
     XC_PSR_CMT_L3_OCCUPANCY,
+    XC_PSR_CMT_TOTAL_MEM_BANDWIDTH,
+    XC_PSR_CMT_LOCAL_MEM_BANDWIDTH,
 };
 typedef enum xc_psr_cmt_type xc_psr_cmt_type;
 int xc_psr_cmt_attach(xc_interface *xch, uint32_t domid);
@@ -2697,10 +2699,12 @@ int xc_psr_cmt_get_domain_rmid(xc_interface *xch, uint32_t domid,
 int xc_psr_cmt_get_total_rmid(xc_interface *xch, uint32_t *total_rmid);
 int xc_psr_cmt_get_l3_upscaling_factor(xc_interface *xch,
                                        uint32_t *upscaling_factor);
+int xc_psr_cmt_get_l3_event_mask(xc_interface *xch, uint32_t *event_mask);
 int xc_psr_cmt_get_l3_cache_size(xc_interface *xch, uint32_t cpu,
                                  uint32_t *l3_cache_size);
 int xc_psr_cmt_get_data(xc_interface *xch, uint32_t rmid, uint32_t cpu,
-                        uint32_t psr_cmt_type, uint64_t *monitor_data);
+                        uint32_t psr_cmt_type, uint64_t *monitor_data,
+                        uint64_t *tsc);
 int xc_psr_cmt_enabled(xc_interface *xch);
 #endif
 
diff --git a/tools/libxc/xc_msr_x86.h b/tools/libxc/xc_msr_x86.h
index 7c3e1a3..7f100e7 100644
--- a/tools/libxc/xc_msr_x86.h
+++ b/tools/libxc/xc_msr_x86.h
@@ -20,6 +20,7 @@
 #ifndef XC_MSR_X86_H
 #define XC_MSR_X86_H
 
+#define MSR_IA32_TSC            0x00000010
 #define MSR_IA32_CMT_EVTSEL     0x00000c8d
 #define MSR_IA32_CMT_CTR        0x00000c8e
 
diff --git a/tools/libxc/xc_psr.c b/tools/libxc/xc_psr.c
index 70d9067..5482c8e 100644
--- a/tools/libxc/xc_psr.c
+++ b/tools/libxc/xc_psr.c
@@ -23,6 +23,8 @@
 #define IA32_CMT_CTR_ERROR_MASK         (0x3ull << 62)
 
 #define EVTID_L3_OCCUPANCY             0x1
+#define EVTID_TOTAL_MEM_BANDWIDTH      0x2
+#define EVTID_LOCAL_MEM_BANDWIDTH      0x3
 
 int xc_psr_cmt_attach(xc_interface *xch, uint32_t domid)
 {
@@ -112,6 +114,23 @@ int xc_psr_cmt_get_l3_upscaling_factor(xc_interface *xch,
     return rc;
 }
 
+int xc_psr_cmt_get_l3_event_mask(xc_interface *xch, uint32_t *event_mask)
+{
+    int rc;
+    DECLARE_SYSCTL;
+
+    sysctl.cmd = XEN_SYSCTL_psr_cmt_op;
+    sysctl.u.psr_cmt_op.cmd =
+        XEN_SYSCTL_PSR_CMT_get_l3_event_mask;
+    sysctl.u.psr_cmt_op.flags = 0;
+
+    rc = xc_sysctl(xch, &sysctl);
+    if ( !rc )
+        *event_mask = sysctl.u.psr_cmt_op.u.data;
+
+    return rc;
+}
+
 int xc_psr_cmt_get_l3_cache_size(xc_interface *xch, uint32_t cpu,
                                  uint32_t *l3_cache_size)
 {
@@ -139,10 +158,12 @@ int xc_psr_cmt_get_l3_cache_size(xc_interface *xch, uint32_t cpu,
 }
 
 int xc_psr_cmt_get_data(xc_interface *xch, uint32_t rmid, uint32_t cpu,
-                        xc_psr_cmt_type type, uint64_t *monitor_data)
+                        xc_psr_cmt_type type, uint64_t *monitor_data,
+                        uint64_t *tsc)
 {
     xc_resource_op_t op;
-    xc_resource_entry_t entries[2];
+    xc_resource_entry_t entries[3];
+    xc_resource_entry_t *tsc_entry = NULL;
     uint32_t evtid, nr = 0;
     int rc;
 
@@ -151,6 +172,12 @@ int xc_psr_cmt_get_data(xc_interface *xch, uint32_t rmid, uint32_t cpu,
     case XC_PSR_CMT_L3_OCCUPANCY:
         evtid = EVTID_L3_OCCUPANCY;
         break;
+    case XC_PSR_CMT_TOTAL_MEM_BANDWIDTH:
+        evtid = EVTID_TOTAL_MEM_BANDWIDTH;
+        break;
+    case XC_PSR_CMT_LOCAL_MEM_BANDWIDTH:
+        evtid = EVTID_LOCAL_MEM_BANDWIDTH;
+        break;
     default:
         return -1;
     }
@@ -167,6 +194,16 @@ int xc_psr_cmt_get_data(xc_interface *xch, uint32_t rmid, uint32_t cpu,
     entries[nr].rsvd = 0;
     nr++;
 
+    if ( tsc != NULL )
+    {
+        tsc_entry = &entries[nr];
+        entries[nr].u.cmd = XEN_RESOURCE_OP_MSR_READ;
+        entries[nr].idx = MSR_IA32_TSC;
+        entries[nr].val = 0;
+        entries[nr].rsvd = 0;
+        nr++;
+    }
+
     op.cpu = cpu;
     op.nr_entries = nr;
     op.entries = entries;
@@ -180,6 +217,9 @@ int xc_psr_cmt_get_data(xc_interface *xch, uint32_t rmid, uint32_t cpu,
 
     *monitor_data = entries[1].val;
 
+    if ( tsc_entry != NULL )
+        *tsc = tsc_entry->val;
+
     return 0;
 }
 
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index f784df5..a48431c 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -712,6 +712,13 @@ void libxl_mac_copy(libxl_ctx *ctx, libxl_mac *dst, libxl_mac *src);
  * If this is defined, the Cache Monitoring Technology feature is supported.
  */
 #define LIBXL_HAVE_PSR_CMT 1
+
+/*
+ * LIBXL_HAVE_PSR_MBM
+ *
+ * If this is defined, the Memory Bandwidth Monitoring feature is supported.
+ */
+#define LIBXL_HAVE_PSR_MBM 1
 #endif
 
 typedef char **libxl_string_list;
@@ -1485,6 +1492,16 @@ int libxl_psr_cmt_get_cache_occupancy(libxl_ctx *ctx,
                                       uint32_t *l3_cache_occupancy);
 #endif
 
+#ifdef LIBXL_HAVE_PSR_MBM
+int libxl_psr_cmt_type_supported(libxl_ctx *ctx, libxl_psr_cmt_type type);
+int libxl_psr_cmt_get_sample(libxl_ctx *ctx,
+                             uint32_t domid,
+                             libxl_psr_cmt_type type,
+                             uint64_t scope,
+                             uint64_t *sample_r,
+                             uint64_t *tsc_r);
+#endif
+
 /* misc */
 
 /* Each of these sets or clears the flag according to whether the
diff --git a/tools/libxl/libxl_psr.c b/tools/libxl/libxl_psr.c
index ec3b6e9..3e1c792 100644
--- a/tools/libxl/libxl_psr.c
+++ b/tools/libxl/libxl_psr.c
@@ -161,18 +161,36 @@ out:
     return rc;
 }
 
-int libxl_psr_cmt_get_cache_occupancy(libxl_ctx *ctx,
-                                      uint32_t domid,
-                                      uint32_t socketid,
-                                      uint32_t *l3_cache_occupancy)
+int libxl_psr_cmt_type_supported(libxl_ctx *ctx, libxl_psr_cmt_type type)
 {
     GC_INIT(ctx);
+    uint32_t event_mask;
+    int rc;
 
+    rc = xc_psr_cmt_get_l3_event_mask(ctx->xch, &event_mask);
+    if (rc < 0) {
+        libxl__psr_cmt_log_err_msg(gc, errno);
+        rc = 0;
+    } else {
+        rc = event_mask & (1 << (type - 1));
+    }
+
+    GC_FREE;
+    return rc;
+}
+
+int libxl_psr_cmt_get_sample(libxl_ctx *ctx,
+                             uint32_t domid,
+                             libxl_psr_cmt_type type,
+                             uint64_t scope,
+                             uint64_t *sample_r,
+                             uint64_t *tsc_r)
+{
+    GC_INIT(ctx);
     unsigned int rmid;
     uint32_t upscaling_factor;
     uint64_t monitor_data;
     int cpu, rc;
-    xc_psr_cmt_type type;
 
     rc = xc_psr_cmt_get_domain_rmid(ctx->xch, domid, &rmid);
     if (rc < 0 || rmid == 0) {
@@ -182,15 +200,15 @@ int libxl_psr_cmt_get_cache_occupancy(libxl_ctx *ctx,
         goto out;
     }
 
-    cpu = libxl__pick_socket_cpu(gc, socketid);
+    cpu = libxl__pick_socket_cpu(gc, scope);
     if (cpu < 0) {
         LOGE(ERROR, "failed to get socket cpu");
         rc = ERROR_FAIL;
         goto out;
     }
 
-    type = XC_PSR_CMT_L3_OCCUPANCY;
-    rc = xc_psr_cmt_get_data(ctx->xch, rmid, cpu, type, &monitor_data);
+    rc = xc_psr_cmt_get_data(ctx->xch, rmid, cpu, type - 1,
+                             &monitor_data, tsc_r);
     if (rc < 0) {
         LOGE(ERROR, "failed to get monitoring data");
         rc = ERROR_FAIL;
@@ -204,13 +222,31 @@ int libxl_psr_cmt_get_cache_occupancy(libxl_ctx *ctx,
         goto out;
     }
 
-    *l3_cache_occupancy = upscaling_factor * monitor_data / 1024;
-    rc = 0;
+    *sample_r = monitor_data * upscaling_factor;
 out:
     GC_FREE;
     return rc;
 }
 
+int libxl_psr_cmt_get_cache_occupancy(libxl_ctx *ctx,
+                                      uint32_t domid,
+                                      uint32_t socketid,
+                                      uint32_t *l3_cache_occupancy)
+{
+    uint64_t data;
+    int rc;
+
+    rc = libxl_psr_cmt_get_sample(ctx, domid,
+                                  LIBXL_PSR_CMT_TYPE_CACHE_OCCUPANCY,
+                                  socketid, &data, NULL);
+    if (rc < 0)
+        goto out;
+
+    *l3_cache_occupancy = data / 1024;
+out:
+    return rc;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 02be466..6d559ea 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -696,4 +696,6 @@ libxl_event = Struct("event",[
 
 libxl_psr_cmt_type = Enumeration("psr_cmt_type", [
     (1, "CACHE_OCCUPANCY"),
+    (2, "TOTAL_MEM_BANDWIDTH"),
+    (3, "LOCAL_MEM_BANDWIDTH"),
     ])
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 846a4b2..9846105 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -7822,13 +7822,61 @@ out:
 }
 
 #ifdef LIBXL_HAVE_PSR_CMT
+
+#define MBM_SAMPLE_RETRY_MAX 4
+static int psr_cmt_get_mem_bandwidth(uint32_t domid,
+                                     libxl_psr_cmt_type type,
+                                     uint32_t socketid,
+                                     uint64_t *bandwidth_r)
+{
+    uint64_t sample1, sample2;
+    uint64_t tsc1, tsc2;
+    int retry_attempts = 0;
+    int rc;
+
+    while (1) {
+        rc = libxl_psr_cmt_get_sample(ctx, domid, type, socketid,
+                                      &sample1, &tsc1);
+        if (rc < 0)
+            return rc;
+
+        usleep(10000);
+
+        rc = libxl_psr_cmt_get_sample(ctx, domid, type, socketid,
+                                      &sample2, &tsc2);
+        if (rc < 0)
+            return rc;
+
+        if (tsc2 <= tsc1)
+            return -1;
+
+        /*
+         * Hardware guarantees at most 1 overflow can happen if the duration
+         * between two samples is less than 1 second. Note that tsc returned
+         * from hypervisor is already-scaled time(ns).
+         */
+        if (tsc2 - tsc1 < 1000000000 && sample2 >= sample1)
+            break;
+
+        if (retry_attempts < MBM_SAMPLE_RETRY_MAX) {
+            retry_attempts++;
+        } else {
+            fprintf(stderr, "event counter overflowed\n");
+            return -1;
+        }
+    }
+
+    *bandwidth_r = (sample2 - sample1) * 1000000000 / (tsc2 - tsc1) / 1024;
+    return 0;
+}
+
 static void psr_cmt_print_domain_info(libxl_dominfo *dominfo,
                                       libxl_psr_cmt_type type,
                                       uint32_t nr_sockets)
 {
     char *domain_name;
     uint32_t socketid;
-    uint32_t l3_cache_occupancy;
+    uint64_t monitor_data;
 
     if (!libxl_psr_cmt_domain_attached(ctx, dominfo->domid))
         return;
@@ -7840,11 +7888,15 @@ static void psr_cmt_print_domain_info(libxl_dominfo *dominfo,
     for (socketid = 0; socketid < nr_sockets; socketid++) {
         switch (type) {
         case LIBXL_PSR_CMT_TYPE_CACHE_OCCUPANCY:
-            if (!libxl_psr_cmt_get_cache_occupancy(ctx,
-                                                   dominfo->domid,
-                                                   socketid,
-                                                   &l3_cache_occupancy))
-                printf("%13u KB", l3_cache_occupancy);
+            if (!libxl_psr_cmt_get_sample(ctx, dominfo->domid, type, socketid,
+                                          &monitor_data, NULL))
+                printf("%13"PRIu64" KB", monitor_data / 1024);
+            break;
+        case LIBXL_PSR_CMT_TYPE_TOTAL_MEM_BANDWIDTH:
+        case LIBXL_PSR_CMT_TYPE_LOCAL_MEM_BANDWIDTH:
+            if (!psr_cmt_get_mem_bandwidth(dominfo->domid, type, socketid,
+                                           &monitor_data))
+                printf("%11"PRIu64" KB/s", monitor_data);
             break;
         default:
             return;
@@ -7866,6 +7918,12 @@ static int psr_cmt_show(libxl_psr_cmt_type type, uint32_t domid)
         return -1;
     }
 
+    if (!libxl_psr_cmt_type_supported(ctx, type)) {
+        fprintf(stderr, "Monitor type '%s' is not supported in the system\n",
+                libxl_psr_cmt_type_to_string(type));
+        return -1;
+    }
+
     libxl_physinfo_init(&info);
     rc = libxl_get_physinfo(ctx, &info);
     if (rc < 0) {
@@ -7983,6 +8041,8 @@ int main_psr_cmt_show(int argc, char **argv)
 
     switch (type) {
     case LIBXL_PSR_CMT_TYPE_CACHE_OCCUPANCY:
+    case LIBXL_PSR_CMT_TYPE_TOTAL_MEM_BANDWIDTH:
+    case LIBXL_PSR_CMT_TYPE_LOCAL_MEM_BANDWIDTH:
         ret = psr_cmt_show(type, domid);
         break;
     default:
diff --git a/tools/libxl/xl_cmdtable.c b/tools/libxl/xl_cmdtable.c
index 4b30d3d..22ab63b 100644
--- a/tools/libxl/xl_cmdtable.c
+++ b/tools/libxl/xl_cmdtable.c
@@ -538,7 +538,9 @@ struct cmd_spec cmd_table[] = {
       "Show Cache Monitoring Technology information",
       "<PSR-CMT-Type> <Domain>",
       "Available monitor types:\n"
-      "\"cache_occupancy\":         Show L3 cache occupancy\n",
+      "\"cache_occupancy\":         Show L3 cache occupancy(KB)\n"
+      "\"total_mem_bandwidth\":     Show total memory bandwidth(KB/s)\n"
+      "\"local_mem_bandwidth\":     Show local memory bandwidth(KB/s)\n",
     },
 #endif
 };
-- 
1.9.1

Re: [PATCH v10 2/4] tools/libxc: code refactoring in xc_psr_cmt_get_data

[Ian Campbell]

On Thu, 2015-02-26 at 16:45 +0800, Chao Peng wrote:
> Use calculated array index instead of hardcoded array index.
> No functional change involved.
> 
> Signed-off-by: Chao Peng <chao.p.peng@linux.intel.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

Re: [PATCH v10 4/4] tools, docs: add total/local memory bandwith monitoring

[Ian Campbell]

On Thu, 2015-02-26 at 16:45 +0800, Chao Peng wrote:
> Add Memory Bandwidth Monitoring(MBM) for VMs. Two types of monitoring
> are supported: total and local memory bandwidth monitoring. To use it,
> CMT should be enabled in hypervisor.
> 
> Signed-off-by: Chao Peng <chao.p.peng@linux.intel.com>

This looks good. I have one question and one small comment/idea:

[...]
> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> index 09d819f..54043ee 100644
> --- a/tools/libxc/include/xenctrl.h
> +++ b/tools/libxc/include/xenctrl.h
> @@ -2688,6 +2688,8 @@ int xc_resource_op(xc_interface *xch, uint32_t nr_ops, xc_resource_op_t *ops);
>  #if defined(__i386__) || defined(__x86_64__)
>  enum xc_psr_cmt_type {
>      XC_PSR_CMT_L3_OCCUPANCY,
> +    XC_PSR_CMT_TOTAL_MEM_BANDWIDTH,
> +    XC_PSR_CMT_LOCAL_MEM_BANDWIDTH,

Is "bandwidth" still the correct term here (and more importantly in the
libxl interface e.g. enum), given that we now do the sampling at the
application level and just expose the current count from Xen via libxl?

I'm not sure what I better term would be though. "count"?

> @@ -167,6 +194,16 @@ int xc_psr_cmt_get_data(xc_interface *xch, uint32_t rmid, uint32_t cpu,
>      entries[nr].rsvd = 0;
>      nr++;
>  
> +    if ( tsc != NULL )
> +    {
> +        tsc_entry = &entries[nr];
> +        entries[nr].u.cmd = XEN_RESOURCE_OP_MSR_READ;
> +        entries[nr].idx = MSR_IA32_TSC;
> +        entries[nr].val = 0;
> +        entries[nr].rsvd = 0;
> +        nr++;
> +    }

Perhaps consider an assertion here that nr <= the number of elements in
the array (i.e. ARRAY_SIZE(entries))? (Either added here or in the patch
which switched to using nr++)

Ian.

Re: [PATCH v10 0/4] enable Memory Bandwidth Monitoring (MBM) for VMs

[Ian Campbell]

On Thu, 2015-02-26 at 16:45 +0800, Chao Peng wrote:
>   tools: correct coding style for psr
>   tools/libxc: code refactoring in xc_psr_cmt_get_data
>   tools/libxl: code refactoring for MBM

I applied these three.

>   tools, docs: add total/local memory bandwith monitoring

One minor terminology question on this one, see my reply.

Ian.

Re: [PATCH v10 4/4] tools, docs: add total/local memory bandwith monitoring

[Chao Peng]

On Mon, Mar 02, 2015 at 01:48:43PM +0000, Ian Campbell wrote:
> On Thu, 2015-02-26 at 16:45 +0800, Chao Peng wrote:
> > Add Memory Bandwidth Monitoring(MBM) for VMs. Two types of monitoring
> > are supported: total and local memory bandwidth monitoring. To use it,
> > CMT should be enabled in hypervisor.
> > 
> > Signed-off-by: Chao Peng <chao.p.peng@linux.intel.com>
> 
> This looks good. I have one question and one small comment/idea:
> 
> [...]
> > diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> > index 09d819f..54043ee 100644
> > --- a/tools/libxc/include/xenctrl.h
> > +++ b/tools/libxc/include/xenctrl.h
> > @@ -2688,6 +2688,8 @@ int xc_resource_op(xc_interface *xch, uint32_t nr_ops, xc_resource_op_t *ops);
> >  #if defined(__i386__) || defined(__x86_64__)
> >  enum xc_psr_cmt_type {
> >      XC_PSR_CMT_L3_OCCUPANCY,
> > +    XC_PSR_CMT_TOTAL_MEM_BANDWIDTH,
> > +    XC_PSR_CMT_LOCAL_MEM_BANDWIDTH,
> 
> Is "bandwidth" still the correct term here (and more importantly in the
> libxl interface e.g. enum), given that we now do the sampling at the
> application level and just expose the current count from Xen via libxl?

I feel comfortable either changing it or not. The reason to change it is
what you said here that we do return the counter value to the caller, so
a consistent name would be nice. While the reason to keep it is: the
names are listed as the "monitoring event type" from spec, so the caller
perhaps knows that the returned data is the sample value from event
counter register related to that type.

Anyway, if you feel it's better to change, then I will do.

> 
> I'm not sure what I better term would be though. "count"?

The returned value is actually read from monitor event counter, so
"count" you suggested or "sample" both sound OK to me, e.g.:
XC_PSR_CMT_TOTAL_MEM_BANDWIDTH => XC_PSR_CMT_TOTAL_MEM_BANDWIDTH_COUNT
XC_PSR_CMT_LOCAL_MEM_BANDWIDTH => XC_PSR_CMT_LOCAL_MEM_BANDWIDTH_COUNT

> 
> > @@ -167,6 +194,16 @@ int xc_psr_cmt_get_data(xc_interface *xch, uint32_t rmid, uint32_t cpu,
> >      entries[nr].rsvd = 0;
> >      nr++;
> >  
> > +    if ( tsc != NULL )
> > +    {
> > +        tsc_entry = &entries[nr];
> > +        entries[nr].u.cmd = XEN_RESOURCE_OP_MSR_READ;
> > +        entries[nr].idx = MSR_IA32_TSC;
> > +        entries[nr].val = 0;
> > +        entries[nr].rsvd = 0;
> > +        nr++;
> > +    }
> 
> Perhaps consider an assertion here that nr <= the number of elements in
> the array (i.e. ARRAY_SIZE(entries))? (Either added here or in the patch
> which switched to using nr++)

Agreed. Given that patch (switched to using nr++) is aleady in, so will
add it here.

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

Re: [PATCH v10 4/4] tools, docs: add total/local memory bandwith monitoring

[Ian Campbell]

On Tue, 2015-03-03 at 16:00 +0800, Chao Peng wrote:
> On Mon, Mar 02, 2015 at 01:48:43PM +0000, Ian Campbell wrote:
> > On Thu, 2015-02-26 at 16:45 +0800, Chao Peng wrote:
> > > Add Memory Bandwidth Monitoring(MBM) for VMs. Two types of monitoring
> > > are supported: total and local memory bandwidth monitoring. To use it,
> > > CMT should be enabled in hypervisor.
> > > 
> > > Signed-off-by: Chao Peng <chao.p.peng@linux.intel.com>
> > 
> > This looks good. I have one question and one small comment/idea:
> > 
> > [...]
> > > diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> > > index 09d819f..54043ee 100644
> > > --- a/tools/libxc/include/xenctrl.h
> > > +++ b/tools/libxc/include/xenctrl.h
> > > @@ -2688,6 +2688,8 @@ int xc_resource_op(xc_interface *xch, uint32_t nr_ops, xc_resource_op_t *ops);
> > >  #if defined(__i386__) || defined(__x86_64__)
> > >  enum xc_psr_cmt_type {
> > >      XC_PSR_CMT_L3_OCCUPANCY,
> > > +    XC_PSR_CMT_TOTAL_MEM_BANDWIDTH,
> > > +    XC_PSR_CMT_LOCAL_MEM_BANDWIDTH,
> > 
> > Is "bandwidth" still the correct term here (and more importantly in the
> > libxl interface e.g. enum), given that we now do the sampling at the
> > application level and just expose the current count from Xen via libxl?
> 
> I feel comfortable either changing it or not. The reason to change it is
> what you said here that we do return the counter value to the caller, so
> a consistent name would be nice. While the reason to keep it is: the
> names are listed as the "monitoring event type" from spec, so the caller
> perhaps knows that the returned data is the sample value from event
> counter register related to that type.
> 
> Anyway, if you feel it's better to change, then I will do.

What names does Intel's documentation use for these registers?

> > I'm not sure what I better term would be though. "count"?
> 
> The returned value is actually read from monitor event counter, so
> "count" you suggested or "sample" both sound OK to me, e.g.:
> XC_PSR_CMT_TOTAL_MEM_BANDWIDTH => XC_PSR_CMT_TOTAL_MEM_BANDWIDTH_COUNT
> XC_PSR_CMT_LOCAL_MEM_BANDWIDTH => XC_PSR_CMT_LOCAL_MEM_BANDWIDTH_COUNT

I was meaning XC_PSR_CMT_TOTAL_MEM_COUNT etc. not BANDWIDTH_COUNT (which
I think doesn't make sense).

> > > @@ -167,6 +194,16 @@ int xc_psr_cmt_get_data(xc_interface *xch, uint32_t rmid, uint32_t cpu,
> > >      entries[nr].rsvd = 0;
> > >      nr++;
> > >  
> > > +    if ( tsc != NULL )
> > > +    {
> > > +        tsc_entry = &entries[nr];
> > > +        entries[nr].u.cmd = XEN_RESOURCE_OP_MSR_READ;
> > > +        entries[nr].idx = MSR_IA32_TSC;
> > > +        entries[nr].val = 0;
> > > +        entries[nr].rsvd = 0;
> > > +        nr++;
> > > +    }
> > 
> > Perhaps consider an assertion here that nr <= the number of elements in
> > the array (i.e. ARRAY_SIZE(entries))? (Either added here or in the patch
> > which switched to using nr++)
> 
> Agreed. Given that patch (switched to using nr++) is aleady in, so will
> add it here.

Thanks.

Re: [PATCH v10 4/4] tools, docs: add total/local memory bandwith monitoring

[Chao Peng]

On Tue, Mar 03, 2015 at 10:09:39AM +0000, Ian Campbell wrote:
> On Tue, 2015-03-03 at 16:00 +0800, Chao Peng wrote:
> > On Mon, Mar 02, 2015 at 01:48:43PM +0000, Ian Campbell wrote:
> > > On Thu, 2015-02-26 at 16:45 +0800, Chao Peng wrote:
> > > > Add Memory Bandwidth Monitoring(MBM) for VMs. Two types of monitoring
> > > > are supported: total and local memory bandwidth monitoring. To use it,
> > > > CMT should be enabled in hypervisor.
> > > > 
> > > > Signed-off-by: Chao Peng <chao.p.peng@linux.intel.com>
> > > 
> > > This looks good. I have one question and one small comment/idea:
> > > 
> > > [...]
> > > > diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> > > > index 09d819f..54043ee 100644
> > > > --- a/tools/libxc/include/xenctrl.h
> > > > +++ b/tools/libxc/include/xenctrl.h
> > > > @@ -2688,6 +2688,8 @@ int xc_resource_op(xc_interface *xch, uint32_t nr_ops, xc_resource_op_t *ops);
> > > >  #if defined(__i386__) || defined(__x86_64__)
> > > >  enum xc_psr_cmt_type {
> > > >      XC_PSR_CMT_L3_OCCUPANCY,
> > > > +    XC_PSR_CMT_TOTAL_MEM_BANDWIDTH,
> > > > +    XC_PSR_CMT_LOCAL_MEM_BANDWIDTH,
> > > 
> > > Is "bandwidth" still the correct term here (and more importantly in the
> > > libxl interface e.g. enum), given that we now do the sampling at the
> > > application level and just expose the current count from Xen via libxl?
> > 
> > I feel comfortable either changing it or not. The reason to change it is
> > what you said here that we do return the counter value to the caller, so
> > a consistent name would be nice. While the reason to keep it is: the
> > names are listed as the "monitoring event type" from spec, so the caller
> > perhaps knows that the returned data is the sample value from event
> > counter register related to that type.
> > 
> > Anyway, if you feel it's better to change, then I will do.
> 
> What names does Intel's documentation use for these registers?

The register is IA32_QM_CTR(Monitoring Counter Register), to read the
count in this register, IA32_QM_EVTSEL(Monitoring Event Select Register)
must be set first. This enum corresponds to the event type in IA32_QM_EVTSEL.

> 
> > > I'm not sure what I better term would be though. "count"?
> > 
> > The returned value is actually read from monitor event counter, so
> > "count" you suggested or "sample" both sound OK to me, e.g.:
> > XC_PSR_CMT_TOTAL_MEM_BANDWIDTH => XC_PSR_CMT_TOTAL_MEM_BANDWIDTH_COUNT
> > XC_PSR_CMT_LOCAL_MEM_BANDWIDTH => XC_PSR_CMT_LOCAL_MEM_BANDWIDTH_COUNT
> 
> I was meaning XC_PSR_CMT_TOTAL_MEM_COUNT etc. not BANDWIDTH_COUNT (which
> I think doesn't make sense).

OK, I have no problem. Thanks for clarification.

Chao

Re: [PATCH v10 4/4] tools, docs: add total/local memory bandwith monitoring

[Ian Campbell]

On Wed, 2015-03-04 at 09:04 +0800, Chao Peng wrote:
> On Tue, Mar 03, 2015 at 10:09:39AM +0000, Ian Campbell wrote:
> > On Tue, 2015-03-03 at 16:00 +0800, Chao Peng wrote:
> > > On Mon, Mar 02, 2015 at 01:48:43PM +0000, Ian Campbell wrote:
> > > > On Thu, 2015-02-26 at 16:45 +0800, Chao Peng wrote:
> > > > > Add Memory Bandwidth Monitoring(MBM) for VMs. Two types of monitoring
> > > > > are supported: total and local memory bandwidth monitoring. To use it,
> > > > > CMT should be enabled in hypervisor.
> > > > > 
> > > > > Signed-off-by: Chao Peng <chao.p.peng@linux.intel.com>
> > > > 
> > > > This looks good. I have one question and one small comment/idea:
> > > > 
> > > > [...]
> > > > > diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> > > > > index 09d819f..54043ee 100644
> > > > > --- a/tools/libxc/include/xenctrl.h
> > > > > +++ b/tools/libxc/include/xenctrl.h
> > > > > @@ -2688,6 +2688,8 @@ int xc_resource_op(xc_interface *xch, uint32_t nr_ops, xc_resource_op_t *ops);
> > > > >  #if defined(__i386__) || defined(__x86_64__)
> > > > >  enum xc_psr_cmt_type {
> > > > >      XC_PSR_CMT_L3_OCCUPANCY,
> > > > > +    XC_PSR_CMT_TOTAL_MEM_BANDWIDTH,
> > > > > +    XC_PSR_CMT_LOCAL_MEM_BANDWIDTH,
> > > > 
> > > > Is "bandwidth" still the correct term here (and more importantly in the
> > > > libxl interface e.g. enum), given that we now do the sampling at the
> > > > application level and just expose the current count from Xen via libxl?
> > > 
> > > I feel comfortable either changing it or not. The reason to change it is
> > > what you said here that we do return the counter value to the caller, so
> > > a consistent name would be nice. While the reason to keep it is: the
> > > names are listed as the "monitoring event type" from spec, so the caller
> > > perhaps knows that the returned data is the sample value from event
> > > counter register related to that type.
> > > 
> > > Anyway, if you feel it's better to change, then I will do.
> > 
> > What names does Intel's documentation use for these registers?
> 
> The register is IA32_QM_CTR(Monitoring Counter Register), to read the
> count in this register,

I'm thinking then that COUNT is the right name, .e.g.
XC_PSR_CMT_TOTAL_MEM_COUNT (and for libxl names too). Does that sound
alright?

Ian.

Re: [PATCH v10 4/4] tools, docs: add total/local memory bandwith monitoring

[Chao Peng]

On Wed, Mar 04, 2015 at 10:07:56AM +0000, Ian Campbell wrote:
> On Wed, 2015-03-04 at 09:04 +0800, Chao Peng wrote:
> > On Tue, Mar 03, 2015 at 10:09:39AM +0000, Ian Campbell wrote:
> > > On Tue, 2015-03-03 at 16:00 +0800, Chao Peng wrote:
> > > > On Mon, Mar 02, 2015 at 01:48:43PM +0000, Ian Campbell wrote:
> > > > > On Thu, 2015-02-26 at 16:45 +0800, Chao Peng wrote:
> > > > > > Add Memory Bandwidth Monitoring(MBM) for VMs. Two types of monitoring
> > > > > > are supported: total and local memory bandwidth monitoring. To use it,
> > > > > > CMT should be enabled in hypervisor.
> > > > > > 
> > > > > > Signed-off-by: Chao Peng <chao.p.peng@linux.intel.com>
> > > > > 
> > > > > This looks good. I have one question and one small comment/idea:
> > > > > 
> > > > > [...]
> > > > > > diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> > > > > > index 09d819f..54043ee 100644
> > > > > > --- a/tools/libxc/include/xenctrl.h
> > > > > > +++ b/tools/libxc/include/xenctrl.h
> > > > > > @@ -2688,6 +2688,8 @@ int xc_resource_op(xc_interface *xch, uint32_t nr_ops, xc_resource_op_t *ops);
> > > > > >  #if defined(__i386__) || defined(__x86_64__)
> > > > > >  enum xc_psr_cmt_type {
> > > > > >      XC_PSR_CMT_L3_OCCUPANCY,
> > > > > > +    XC_PSR_CMT_TOTAL_MEM_BANDWIDTH,
> > > > > > +    XC_PSR_CMT_LOCAL_MEM_BANDWIDTH,
> > > > > 
> > > > > Is "bandwidth" still the correct term here (and more importantly in the
> > > > > libxl interface e.g. enum), given that we now do the sampling at the
> > > > > application level and just expose the current count from Xen via libxl?
> > > > 
> > > > I feel comfortable either changing it or not. The reason to change it is
> > > > what you said here that we do return the counter value to the caller, so
> > > > a consistent name would be nice. While the reason to keep it is: the
> > > > names are listed as the "monitoring event type" from spec, so the caller
> > > > perhaps knows that the returned data is the sample value from event
> > > > counter register related to that type.
> > > > 
> > > > Anyway, if you feel it's better to change, then I will do.
> > > 
> > > What names does Intel's documentation use for these registers?
> > 
> > The register is IA32_QM_CTR(Monitoring Counter Register), to read the
> > count in this register,
> 
> I'm thinking then that COUNT is the right name, .e.g.
> XC_PSR_CMT_TOTAL_MEM_COUNT (and for libxl names too). Does that sound
> alright?

Yes. See my changes sent in another thread.
Chao