All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 0/7] enable Cache Allocation Technology (CAT) for VMs
@ 2015-03-19 10:41 Chao Peng
  2015-03-19 10:41 ` [PATCH v2 1/7] x86: clean up psr boot parameter parsing Chao Peng
                   ` (6 more replies)
  0 siblings, 7 replies; 23+ messages in thread
From: Chao Peng @ 2015-03-19 10:41 UTC (permalink / raw)
  To: xen-devel
  Cc: keir, Ian.Campbell, stefano.stabellini, andrew.cooper3,
	Ian.Jackson, will.auld, JBeulich, wei.liu2, dgdegra

Changes in v2:
* Address comments from Konrad and Jan(Detail in patch):
* Make all cat unrelated changes into the preparation patches. 

This patch serial enable the new Cache Allocation Technology (CAT) feature
found in Intel Broadwell and later server platform. On XEN implementation,
CAT is used to control cache allocation on VM basis.

Detail hardware spec can be found in section 17.15 of the Intel SDM [1].
The design for XEN can be found at [2].

[1] Intel SDM (http://www.intel.com/content/dam/www/public/us/en/documents/manuals/64-ia-32-architectures-software-developer-manual-325462.pdf)
[2] CAT design for XEN( http://lists.xen.org/archives/html/xen-devel/2014-12/msg01382.html)

Chao Peng (7):
  X86: clean up psr boot parameter parsing
  x86: improve psr scheduling code
  x86: detect and initialize Intel CAT feature
  x86: add support for COS/CBM manangement
  x86: add scheduling support for Intel CAT
  xsm: add CAT related xsm policies
  tools: add tools support for Intel CAT

 docs/misc/xen-command-line.markdown          |  19 +-
 tools/flask/policy/policy/modules/xen/xen.if |   2 +-
 tools/flask/policy/policy/modules/xen/xen.te |   4 +-
 tools/libxc/include/xenctrl.h                |  15 +
 tools/libxc/xc_psr.c                         |  74 +++++
 tools/libxl/libxl.h                          |  18 ++
 tools/libxl/libxl_psr.c                      | 107 ++++++-
 tools/libxl/libxl_types.idl                  |   4 +
 tools/libxl/xl.h                             |   4 +
 tools/libxl/xl_cmdimpl.c                     | 225 ++++++++++++-
 tools/libxl/xl_cmdtable.c                    |  13 +
 xen/arch/x86/domain.c                        |  13 +-
 xen/arch/x86/domctl.c                        |  18 ++
 xen/arch/x86/psr.c                           | 460 +++++++++++++++++++++++++--
 xen/arch/x86/sysctl.c                        |  20 ++
 xen/include/asm-x86/cpufeature.h             |   1 +
 xen/include/asm-x86/domain.h                 |   5 +-
 xen/include/asm-x86/msr-index.h              |   1 +
 xen/include/asm-x86/psr.h                    |  11 +-
 xen/include/public/domctl.h                  |  12 +
 xen/include/public/sysctl.h                  |  16 +
 xen/xsm/flask/hooks.c                        |   6 +
 xen/xsm/flask/policy/access_vectors          |   6 +
 23 files changed, 993 insertions(+), 61 deletions(-)

-- 
1.9.1

^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH v2 1/7] x86: clean up psr boot parameter parsing
  2015-03-19 10:41 [PATCH v2 0/7] enable Cache Allocation Technology (CAT) for VMs Chao Peng
@ 2015-03-19 10:41 ` Chao Peng
  2015-03-20 16:47   ` Jan Beulich
  2015-03-19 10:41 ` [PATCH v2 2/7] x86: improve psr scheduling code Chao Peng
                   ` (5 subsequent siblings)
  6 siblings, 1 reply; 23+ messages in thread
From: Chao Peng @ 2015-03-19 10:41 UTC (permalink / raw)
  To: xen-devel
  Cc: keir, Ian.Campbell, stefano.stabellini, andrew.cooper3,
	Ian.Jackson, will.auld, JBeulich, wei.liu2, dgdegra

Change type of opt_psr from bool to int so more psr features can fit.

Introduce a new routine to parse bool parameter so that both cmt and
future psr features like cat can use it.

Signed-off-by: Chao Peng <chao.p.peng@linux.intel.com>
---
 xen/arch/x86/psr.c | 37 +++++++++++++++++++++----------------
 1 file changed, 21 insertions(+), 16 deletions(-)

diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c
index 2ef83df..68d5f4e 100644
--- a/xen/arch/x86/psr.c
+++ b/xen/arch/x86/psr.c
@@ -26,11 +26,29 @@ struct psr_assoc {
 };
 
 struct psr_cmt *__read_mostly psr_cmt;
-static bool_t __initdata opt_psr;
+static unsigned int __initdata opt_psr;
 static unsigned int __initdata opt_rmid_max = 255;
 static uint64_t rmid_mask;
 static DEFINE_PER_CPU(struct psr_assoc, psr_assoc);
 
+static void __init parse_psr_bool(char* s, char* value, char* feature, int bit)
+{
+    if ( !strcmp(s, feature) )
+    {
+        if ( !value )
+            opt_psr |= bit;
+        else
+        {
+            int val_int = parse_bool(value);
+
+            if ( val_int == 1 )
+                opt_psr |= bit;
+            else if ( val_int != 0 )
+                printk("PSR: unknown %s value: %s\n", feature, value);
+        }
+    }
+}
+
 static void __init parse_psr_param(char *s)
 {
     char *ss, *val_str;
@@ -44,21 +62,8 @@ static void __init parse_psr_param(char *s)
         if ( val_str )
             *val_str++ = '\0';
 
-        if ( !strcmp(s, "cmt") )
-        {
-            if ( !val_str )
-                opt_psr |= PSR_CMT;
-            else
-            {
-                int val_int = parse_bool(val_str);
-                if ( val_int == 1 )
-                    opt_psr |= PSR_CMT;
-                else if ( val_int != 0 )
-                    printk("PSR: unknown cmt value: %s - CMT disabled!\n",
-                                    val_str);
-            }
-        }
-        else if ( val_str && !strcmp(s, "rmid_max") )
+        parse_psr_bool(s, val_str, "cmt", PSR_CMT);
+        if ( val_str && !strcmp(s, "rmid_max") )
             opt_rmid_max = simple_strtoul(val_str, NULL, 0);
 
         s = ss + 1;
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH v2 2/7] x86: improve psr scheduling code
  2015-03-19 10:41 [PATCH v2 0/7] enable Cache Allocation Technology (CAT) for VMs Chao Peng
  2015-03-19 10:41 ` [PATCH v2 1/7] x86: clean up psr boot parameter parsing Chao Peng
@ 2015-03-19 10:41 ` Chao Peng
  2015-03-19 10:41 ` [PATCH v2 3/7] x86: detect and initialize Intel CAT feature Chao Peng
                   ` (4 subsequent siblings)
  6 siblings, 0 replies; 23+ messages in thread
From: Chao Peng @ 2015-03-19 10:41 UTC (permalink / raw)
  To: xen-devel
  Cc: keir, Ian.Campbell, stefano.stabellini, andrew.cooper3,
	Ian.Jackson, will.auld, JBeulich, wei.liu2, dgdegra

Switching RMID from previous vcpu to next vcpu only needs to write
MSR_IA32_PSR_ASSOC once. Write it with the value of next vcpu is enough,
no need to write '0' first. Idle domain has RMID set to 0 and because MSR
is already updated lazily, so just switch it as it does.

Also move the initialization of per-CPU variable which used for lazy
update from context switch to CPU starting.

Signed-off-by: Chao Peng <chao.p.peng@linux.intel.com>
---
Changes in v2:
* Move initialization for psr_assoc from context switch to CPU_STARTING.
---
 xen/arch/x86/domain.c     |  7 ++--
 xen/arch/x86/psr.c        | 89 ++++++++++++++++++++++++++++++++++++-----------
 xen/include/asm-x86/psr.h |  3 +-
 3 files changed, 73 insertions(+), 26 deletions(-)

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 21f0766..0b5374a 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -1443,8 +1443,6 @@ static void __context_switch(void)
     {
         memcpy(&p->arch.user_regs, stack_regs, CTXT_SWITCH_STACK_BYTES);
         vcpu_save_fpu(p);
-        if ( psr_cmt_enabled() )
-            psr_assoc_rmid(0);
         p->arch.ctxt_switch_from(p);
     }
 
@@ -1469,11 +1467,10 @@ static void __context_switch(void)
         }
         vcpu_restore_fpu_eager(n);
         n->arch.ctxt_switch_to(n);
-
-        if ( psr_cmt_enabled() && n->domain->arch.psr_rmid > 0 )
-            psr_assoc_rmid(n->domain->arch.psr_rmid);
     }
 
+    psr_ctxt_switch_to(n->domain);
+
     gdt = !is_pv_32on64_vcpu(n) ? per_cpu(gdt_table, cpu) :
                                   per_cpu(compat_gdt_table, cpu);
     if ( need_full_gdt(n) )
diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c
index 68d5f4e..015bc07 100644
--- a/xen/arch/x86/psr.c
+++ b/xen/arch/x86/psr.c
@@ -22,7 +22,6 @@
 
 struct psr_assoc {
     uint64_t val;
-    bool_t initialized;
 };
 
 struct psr_cmt *__read_mostly psr_cmt;
@@ -120,14 +119,6 @@ static void __init init_psr_cmt(unsigned int rmid_max)
     printk(XENLOG_INFO "Cache Monitoring Technology enabled\n");
 }
 
-static int __init init_psr(void)
-{
-    if ( (opt_psr & PSR_CMT) && opt_rmid_max )
-        init_psr_cmt(opt_rmid_max);
-    return 0;
-}
-__initcall(init_psr);
-
 /* Called with domain lock held, no psr specific lock needed */
 int psr_alloc_rmid(struct domain *d)
 {
@@ -173,26 +164,84 @@ void psr_free_rmid(struct domain *d)
     d->arch.psr_rmid = 0;
 }
 
-void psr_assoc_rmid(unsigned int rmid)
+static inline void psr_assoc_init(void)
 {
-    uint64_t val;
-    uint64_t new_val;
     struct psr_assoc *psra = &this_cpu(psr_assoc);
 
-    if ( !psra->initialized )
-    {
+    if ( psr_cmt_enabled() )
         rdmsrl(MSR_IA32_PSR_ASSOC, psra->val);
-        psra->initialized = 1;
+}
+
+static inline void psr_assoc_reg_read(struct psr_assoc *psra, uint64_t *reg)
+{
+    *reg = psra->val;
+}
+
+static inline void psr_assoc_reg_write(struct psr_assoc *psra, uint64_t reg)
+{
+    if ( reg != psra->val )
+    {
+        wrmsrl(MSR_IA32_PSR_ASSOC, reg);
+        psra->val = reg;
     }
-    val = psra->val;
+}
+
+static inline void psr_assoc_rmid(uint64_t *reg, unsigned int rmid)
+{
+    *reg = (*reg & ~rmid_mask) | (rmid & rmid_mask);
+}
+
+void psr_ctxt_switch_to(struct domain *d)
+{
+    uint64_t reg;
+    struct psr_assoc *psra = &this_cpu(psr_assoc);
+
+    psr_assoc_reg_read(psra, &reg);
 
-    new_val = (val & ~rmid_mask) | (rmid & rmid_mask);
-    if ( val != new_val )
+    if ( psr_cmt_enabled() )
+        psr_assoc_rmid(&reg, d->arch.psr_rmid);
+
+    psr_assoc_reg_write(psra, reg);
+}
+
+static void psr_cpu_init(unsigned int cpu)
+{
+    psr_assoc_init();
+}
+
+static int cpu_callback(
+    struct notifier_block *nfb, unsigned long action, void *hcpu)
+{
+    unsigned int cpu = (unsigned long)hcpu;
+
+    switch ( action )
+    {
+    case CPU_STARTING:
+        psr_cpu_init(cpu);
+        break;
+    }
+
+    return NOTIFY_DONE;
+}
+
+static struct notifier_block cpu_nfb = {
+    .notifier_call = cpu_callback
+};
+
+static int __init psr_presmp_init(void)
+{
+    if ( (opt_psr & PSR_CMT) && opt_rmid_max )
+        init_psr_cmt(opt_rmid_max);
+
+    if (  psr_cmt_enabled() )
     {
-        wrmsrl(MSR_IA32_PSR_ASSOC, new_val);
-        psra->val = new_val;
+        psr_cpu_init(smp_processor_id());
+        register_cpu_notifier(&cpu_nfb);
     }
+
+    return 0;
 }
+presmp_initcall(psr_presmp_init);
 
 /*
  * Local variables:
diff --git a/xen/include/asm-x86/psr.h b/xen/include/asm-x86/psr.h
index c6076e9..585350c 100644
--- a/xen/include/asm-x86/psr.h
+++ b/xen/include/asm-x86/psr.h
@@ -46,7 +46,8 @@ static inline bool_t psr_cmt_enabled(void)
 
 int psr_alloc_rmid(struct domain *d);
 void psr_free_rmid(struct domain *d);
-void psr_assoc_rmid(unsigned int rmid);
+
+void psr_ctxt_switch_to(struct domain *d);
 
 #endif /* __ASM_PSR_H__ */
 
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH v2 3/7] x86: detect and initialize Intel CAT feature
  2015-03-19 10:41 [PATCH v2 0/7] enable Cache Allocation Technology (CAT) for VMs Chao Peng
  2015-03-19 10:41 ` [PATCH v2 1/7] x86: clean up psr boot parameter parsing Chao Peng
  2015-03-19 10:41 ` [PATCH v2 2/7] x86: improve psr scheduling code Chao Peng
@ 2015-03-19 10:41 ` Chao Peng
  2015-03-19 11:38   ` Tim Deegan
                     ` (2 more replies)
  2015-03-19 10:41 ` [PATCH v2 4/7] x86: add support for COS/CBM manangement Chao Peng
                   ` (3 subsequent siblings)
  6 siblings, 3 replies; 23+ messages in thread
From: Chao Peng @ 2015-03-19 10:41 UTC (permalink / raw)
  To: xen-devel
  Cc: keir, Ian.Campbell, stefano.stabellini, andrew.cooper3,
	Ian.Jackson, will.auld, JBeulich, wei.liu2, dgdegra

Detect Intel Cache Allocation Technology(CAT) feature and store the
cpuid information for later use. Currently only L3 cache allocation is
supported. The L3 CAT features may vary among sockets so per-socket
feature information is stored. The initialization can happen either at
boot time or when CPU(s) is hot plugged after booting.

Signed-off-by: Chao Peng <chao.p.peng@linux.intel.com>
---
Changes in v2:
* socket_num => num_sockets and fix several documentaion issues.
* refactor boot line parameters parsing into standlone patch.
* set opt_num_sockets = NR_CPUS when opt_num_sockets > NR_CPUS.
* replace CPU_ONLINE with CPU_STARTING and integrate that into scheduling
  improvement patch.
* reimplement get_max_socket() with cpu_to_socket();
* cbm is still uint64 as there is a path forward for supporting long masks.
---
 docs/misc/xen-command-line.markdown |  19 ++++++-
 xen/arch/x86/psr.c                  | 105 +++++++++++++++++++++++++++++++++---
 xen/include/asm-x86/cpufeature.h    |   1 +
 3 files changed, 115 insertions(+), 10 deletions(-)

diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index 63871cb..768c55f 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -1090,9 +1090,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> )`
+> `= List of ( cmt:<boolean> | rmid_max:<integer> | cat:<boolean> | num_sockets:<interger> )`
 
-> Default: `psr=cmt:0,rmid_max:255`
+> Default: `psr=cmt:0,rmid_max:255,cat:0,num_sockets:0(Detect at boot time)`
 
 Platform Shared Resource(PSR) Services.  Intel Haswell and later server
 platforms offer information about the sharing of resources.
@@ -1102,6 +1102,11 @@ Monitoring ID(RMID) is used to bind the domain to corresponding shared
 resource.  RMID is a hardware-provided layer of abstraction between software
 and logical processors.
 
+To use the PSR cache allocation service for a certain domain, a capacity
+bitmasks(CBM) is used to bind the domain to corresponding shared resource.
+CBM represents cache capacity and indicates the degree of overlap and isolation
+between domains.
+
 The following resources are available:
 
 * Cache Monitoring Technology (Haswell and later).  Information regarding the
@@ -1112,6 +1117,16 @@ The following resources are available:
   total/local memory bandwidth. Follow the same options with Cache Monitoring
   Technology.
 
+* Cache Alllocation Technology (Broadwell and later).  Information regarding
+  the cache allocation.
+  * `cat` instructs Xen to enable/disable Cache Allocation Technology.
+  * `num_sockets` indicates the number of available sockets for CAT feature
+    detection. All the sockets up to num_sockets will be checked for CAT
+    feature. The value is normally detected at boot time automatically if not
+    specified(0). While the value may need to be specified manually for CPU
+    hot-plug scenario in which case the automatic sockets number detection may
+    be not correct.
+
 ### reboot
 > `= t[riple] | k[bd] | a[cpi] | p[ci] | n[o] [, [w]arm | [c]old]`
 
diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c
index 015bc07..5946122 100644
--- a/xen/arch/x86/psr.c
+++ b/xen/arch/x86/psr.c
@@ -19,17 +19,39 @@
 #include <asm/psr.h>
 
 #define PSR_CMT        (1<<0)
+#define PSR_CAT        (1<<1)
+
+struct psr_cat_socket_info {
+    bool_t initialized;
+    bool_t enabled;
+    unsigned int cbm_len;
+    unsigned int cos_max;
+};
 
 struct psr_assoc {
     uint64_t val;
 };
 
 struct psr_cmt *__read_mostly psr_cmt;
+static struct psr_cat_socket_info *__read_mostly cat_socket_info;
+
 static unsigned int __initdata opt_psr;
 static unsigned int __initdata opt_rmid_max = 255;
+static unsigned int __read_mostly opt_num_sockets;
 static uint64_t rmid_mask;
 static DEFINE_PER_CPU(struct psr_assoc, psr_assoc);
 
+static unsigned int get_max_socket(void)
+{
+    unsigned int cpu, socket = 0;
+
+    for_each_present_cpu(cpu)
+        if ( socket < cpu_to_socket(cpu) )
+            socket = cpu_to_socket(cpu);
+
+    return socket;
+}
+
 static void __init parse_psr_bool(char* s, char* value, char* feature, int bit)
 {
     if ( !strcmp(s, feature) )
@@ -62,8 +84,15 @@ static void __init parse_psr_param(char *s)
             *val_str++ = '\0';
 
         parse_psr_bool(s, val_str, "cmt", PSR_CMT);
-        if ( val_str && !strcmp(s, "rmid_max") )
-            opt_rmid_max = simple_strtoul(val_str, NULL, 0);
+        parse_psr_bool(s, val_str, "cat", PSR_CAT);
+
+        if ( val_str )
+        {
+            if ( !strcmp(s, "rmid_max") )
+                opt_rmid_max = simple_strtoul(val_str, NULL, 0);
+            else if ( !strcmp(s, "num_sockets") )
+                opt_num_sockets = simple_strtoul(val_str, NULL, 0);
+        }
 
         s = ss + 1;
     } while ( ss );
@@ -204,9 +233,66 @@ void psr_ctxt_switch_to(struct domain *d)
     psr_assoc_reg_write(psra, reg);
 }
 
+static void cat_cpu_init(unsigned int cpu)
+{
+    unsigned int eax, ebx, ecx, edx;
+    struct psr_cat_socket_info *info;
+    unsigned int socket;
+    const struct cpuinfo_x86 *c;
+
+    socket = cpu_to_socket(cpu);
+    if ( socket >= opt_num_sockets )
+    {
+        printk(XENLOG_WARNING "CAT: disabled on socket %d as num_sockets is %d\n",
+               socket, opt_num_sockets);
+        return;
+    }
+
+    info = cat_socket_info + socket;
+
+    /* Avoid initializing more than one times for the same socket. */
+    if ( test_and_set_bool(info->initialized) )
+        return;
+
+    c = cpu_data + cpu;
+    if ( !cpu_has(c, X86_FEATURE_CAT) )
+        return;
+
+    cpuid_count(0x10, 0, &eax, &ebx, &ecx, &edx);
+    if ( ebx & PSR_RESOURCE_TYPE_L3 )
+    {
+        cpuid_count(0x10, 1, &eax, &ebx, &ecx, &edx);
+        info->cbm_len = (eax & 0x1f) + 1;
+        info->cos_max = (edx & 0xffff);
+
+        info->enabled = 1;
+        printk(XENLOG_DEBUG "CAT: enabled on socket %u, cos_max:%u, cbm_len:%u\n",
+               socket, info->cos_max, info->cbm_len);
+    }
+}
+
+static void __init init_psr_cat(void)
+{
+    if ( opt_num_sockets == 0 )
+        opt_num_sockets = get_max_socket() + 1;
+    else if ( opt_num_sockets > NR_CPUS )
+    {
+        opt_num_sockets = NR_CPUS;
+        printk(XENLOG_WARNING "num_sockets > NR_CPUS(%d), set to NR_CPUS\n",
+                NR_CPUS);
+    }
+
+    cat_socket_info = xzalloc_array(struct psr_cat_socket_info,
+                                    opt_num_sockets);
+}
+
 static void psr_cpu_init(unsigned int cpu)
 {
-    psr_assoc_init();
+    if ( cat_socket_info )
+        cat_cpu_init(cpu);
+
+    if (  psr_cmt_enabled() )
+        psr_assoc_init();
 }
 
 static int cpu_callback(
@@ -233,14 +319,17 @@ static int __init psr_presmp_init(void)
     if ( (opt_psr & PSR_CMT) && opt_rmid_max )
         init_psr_cmt(opt_rmid_max);
 
-    if (  psr_cmt_enabled() )
-    {
-        psr_cpu_init(smp_processor_id());
-        register_cpu_notifier(&cpu_nfb);
-    }
+    if ( opt_psr & PSR_CAT )
+        init_psr_cat();
+
+    psr_cpu_init(smp_processor_id());
+
+    if ( psr_cmt_enabled() || cat_socket_info )
+          register_cpu_notifier(&cpu_nfb);
 
     return 0;
 }
+
 presmp_initcall(psr_presmp_init);
 
 /*
diff --git a/xen/include/asm-x86/cpufeature.h b/xen/include/asm-x86/cpufeature.h
index 7963a3a..8c0f0a6 100644
--- a/xen/include/asm-x86/cpufeature.h
+++ b/xen/include/asm-x86/cpufeature.h
@@ -149,6 +149,7 @@
 #define X86_FEATURE_CMT 	(7*32+12) /* Cache Monitoring Technology */
 #define X86_FEATURE_NO_FPU_SEL 	(7*32+13) /* FPU CS/DS stored as zero */
 #define X86_FEATURE_MPX		(7*32+14) /* Memory Protection Extensions */
+#define X86_FEATURE_CAT 	(7*32+15) /* Cache Allocation Technology */
 #define X86_FEATURE_RDSEED	(7*32+18) /* RDSEED instruction */
 #define X86_FEATURE_ADX		(7*32+19) /* ADCX, ADOX instructions */
 #define X86_FEATURE_SMAP	(7*32+20) /* Supervisor Mode Access Prevention */
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH v2 4/7] x86: add support for COS/CBM manangement
  2015-03-19 10:41 [PATCH v2 0/7] enable Cache Allocation Technology (CAT) for VMs Chao Peng
                   ` (2 preceding siblings ...)
  2015-03-19 10:41 ` [PATCH v2 3/7] x86: detect and initialize Intel CAT feature Chao Peng
@ 2015-03-19 10:41 ` Chao Peng
  2015-03-20 17:13   ` Jan Beulich
  2015-03-19 10:41 ` [PATCH v2 5/7] x86: add scheduling support for Intel CAT Chao Peng
                   ` (2 subsequent siblings)
  6 siblings, 1 reply; 23+ messages in thread
From: Chao Peng @ 2015-03-19 10:41 UTC (permalink / raw)
  To: xen-devel
  Cc: keir, Ian.Campbell, stefano.stabellini, andrew.cooper3,
	Ian.Jackson, will.auld, JBeulich, wei.liu2, dgdegra

CAT introduces a mechanism for software to enable cache allocation based
on application priority or Class of Service(COS). Each COS can be
configured using capacity bitmasks(CBM) to represent cache capacity
and indicate the degree of overlap and isolation between COSs.

In XEN implementation, the cache allocation granularity is domain. All
VCPUs of a domain have the same COS, and therefore, correspond to the
same CBM. COS is maintained in hypervisor only while CBM is exposed to
user space directly to allow getting/setting domain's cache capacity.

Both CBM/COS may be socket-different for the same domain.

General Cache Allocation Technology(CAT) information such as maximum COS
and CBM length are exposed to user space by a SYSCTRL hypercall, to help
to construct the CBM from user side.

Signed-off-by: Chao Peng <chao.p.peng@linux.intel.com>
---
Changes in v2:
* set d->arch.psr_cos_ids when free it.
* check zero_bit < cbm_len when use it.
* change cpumask_check assertion to return error code.
---
 xen/arch/x86/domain.c           |   6 +-
 xen/arch/x86/domctl.c           |  18 ++++
 xen/arch/x86/psr.c              | 222 +++++++++++++++++++++++++++++++++++++++-
 xen/arch/x86/sysctl.c           |  20 ++++
 xen/include/asm-x86/domain.h    |   5 +-
 xen/include/asm-x86/msr-index.h |   1 +
 xen/include/asm-x86/psr.h       |   8 ++
 xen/include/public/domctl.h     |  12 +++
 xen/include/public/sysctl.h     |  16 +++
 9 files changed, 305 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 0b5374a..c9be83b 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -615,6 +615,9 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags)
         /* 64-bit PV guest by default. */
         d->arch.is_32bit_pv = d->arch.has_32bit_shinfo = 0;
 
+    if ( (rc = psr_domain_init(d)) != 0 )
+        goto fail;
+
     /* initialize default tsc behavior in case tools don't */
     tsc_set_info(d, TSC_MODE_DEFAULT, 0UL, 0, 0);
     spin_lock_init(&d->arch.vtsc_lock);
@@ -633,6 +636,7 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags)
     free_perdomain_mappings(d);
     if ( is_pv_domain(d) )
         free_xenheap_page(d->arch.pv_domain.gdt_ldt_l1tab);
+    psr_domain_free(d);
     return rc;
 }
 
@@ -656,7 +660,7 @@ void arch_domain_destroy(struct domain *d)
     free_xenheap_page(d->shared_info);
     cleanup_domain_irq_mapping(d);
 
-    psr_free_rmid(d);
+    psr_domain_free(d);
 }
 
 void arch_domain_shutdown(struct domain *d)
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index a1c5db0..c9f0f4f 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -1324,6 +1324,24 @@ long arch_do_domctl(
         }
         break;
 
+    case XEN_DOMCTL_psr_cat_op:
+        switch ( domctl->u.psr_cat_op.cmd )
+        {
+        case XEN_DOMCTL_PSR_CAT_OP_SET_L3_CBM:
+            ret = psr_set_l3_cbm(d, domctl->u.psr_cat_op.target,
+                                 domctl->u.psr_cat_op.data);
+            break;
+        case XEN_DOMCTL_PSR_CAT_OP_GET_L3_CBM:
+            ret = psr_get_l3_cbm(d, domctl->u.psr_cat_op.target,
+                                 &domctl->u.psr_cat_op.data);
+            copyback = 1;
+            break;
+        default:
+            ret = -EOPNOTSUPP;
+            break;
+        }
+        break;
+
     default:
         ret = iommu_do_domctl(domctl, d, u_domctl);
         break;
diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c
index 5946122..af54aeb 100644
--- a/xen/arch/x86/psr.c
+++ b/xen/arch/x86/psr.c
@@ -21,11 +21,17 @@
 #define PSR_CMT        (1<<0)
 #define PSR_CAT        (1<<1)
 
+struct psr_cat_cbm {
+    unsigned int ref;
+    uint64_t cbm;
+};
+
 struct psr_cat_socket_info {
     bool_t initialized;
     bool_t enabled;
     unsigned int cbm_len;
     unsigned int cos_max;
+    struct psr_cat_cbm *cos_cbm_map;
 };
 
 struct psr_assoc {
@@ -52,6 +58,16 @@ static unsigned int get_max_socket(void)
     return socket;
 }
 
+static unsigned int get_socket_cpu(unsigned int socket)
+{
+    unsigned int cpu;
+
+    for_each_online_cpu ( cpu )
+       if ( cpu_to_socket(cpu) == socket )
+           return cpu;
+    return nr_cpu_ids;
+}
+
 static void __init parse_psr_bool(char* s, char* value, char* feature, int bit)
 {
     if ( !strcmp(s, feature) )
@@ -233,11 +249,204 @@ void psr_ctxt_switch_to(struct domain *d)
     psr_assoc_reg_write(psra, reg);
 }
 
+static int get_cat_socket_info(unsigned int socket,
+                               struct psr_cat_socket_info **info)
+{
+    if ( !cat_socket_info )
+        return -ENODEV;
+
+    if ( socket >= opt_num_sockets )
+        return -EBADSLT;
+
+    if ( !cat_socket_info[socket].enabled )
+        return -ENOENT;
+
+    *info = cat_socket_info + socket;
+    return 0;
+}
+
+int psr_get_cat_l3_info(unsigned int socket, uint32_t *cbm_len,
+                        uint32_t *cos_max)
+{
+    struct psr_cat_socket_info *info;
+    int ret = get_cat_socket_info(socket, &info);
+
+    if ( ret )
+        return ret;
+
+    *cbm_len = info->cbm_len;
+    *cos_max = info->cos_max;
+
+    return 0;
+}
+
+int psr_get_l3_cbm(struct domain *d, unsigned int socket, uint64_t *cbm)
+{
+    unsigned int cos;
+    struct psr_cat_socket_info *info;
+    int ret = get_cat_socket_info(socket, &info);
+
+    if ( ret )
+        return ret;
+
+    cos = d->arch.psr_cos_ids[socket];
+    *cbm = info->cos_cbm_map[cos].cbm;
+    return 0;
+}
+
+static bool_t psr_check_cbm(unsigned int cbm_len, uint64_t cbm)
+{
+    unsigned int first_bit, zero_bit;
+
+    /* Set bits should only in the range of [0, cbm_len). */
+    if ( cbm & (~0ull << cbm_len) )
+        return 0;
+
+    /* At least two contiguous bits need to be set. */
+    if ( hweight_long(cbm) < 2 )
+        return 0;
+
+    first_bit = find_first_bit(&cbm, cbm_len);
+    zero_bit = find_next_zero_bit(&cbm, cbm_len, first_bit);
+
+    /* Set bits should be contiguous. */
+    if ( zero_bit < cbm_len &&
+         find_next_bit(&cbm, cbm_len, zero_bit) < cbm_len )
+        return 0;
+
+    return 1;
+}
+
+struct cos_cbm_info
+{
+    unsigned int cos;
+    uint64_t cbm;
+};
+
+static void do_write_l3_cbm(void *data)
+{
+    struct cos_cbm_info *info = data;
+    wrmsrl(MSR_IA32_L3_MASK(info->cos), info->cbm);
+}
+
+static int write_l3_cbm(unsigned int socket, unsigned int cos, uint64_t cbm)
+{
+    struct cos_cbm_info info = {.cos = cos, .cbm = cbm };
+
+    if ( socket == cpu_to_socket(smp_processor_id()) )
+        do_write_l3_cbm(&info);
+    else
+    {
+        unsigned int cpu = get_socket_cpu(socket);
+
+        if ( cpu == nr_cpu_ids )
+            return -EBADSLT;
+        on_selected_cpus(cpumask_of(cpu), do_write_l3_cbm, &info, 1);
+    }
+
+    return 0;
+}
+
+int psr_set_l3_cbm(struct domain *d, unsigned int socket, uint64_t cbm)
+{
+    unsigned int old_cos, cos;
+    struct psr_cat_cbm *map, *find;
+    struct psr_cat_socket_info *info;
+    int ret = get_cat_socket_info(socket, &info);
+
+    if ( ret )
+        return ret;
+
+    if ( !psr_check_cbm(info->cbm_len, cbm) )
+        return -EINVAL;
+
+    old_cos = d->arch.psr_cos_ids[socket];
+    map = info->cos_cbm_map;
+    find = NULL;
+
+    for ( cos = 0; cos <= info->cos_max; cos++ )
+    {
+        /* If still not found, then keep unused one. */
+        if ( !find && cos != 0 && map[cos].ref == 0 )
+            find = map + cos;
+        else if ( map[cos].cbm == cbm )
+        {
+            if ( unlikely(cos == old_cos) )
+                return -EEXIST;
+            find = map + cos;
+            break;
+        }
+    }
+
+    /* If old cos is referred only by the domain, then use it. */
+    if ( !find && map[old_cos].ref == 1 )
+        find = map + old_cos;
+
+    if ( !find )
+        return -EUSERS;
+
+    cos = find - map;
+    if ( find->cbm != cbm )
+    {
+        ret = write_l3_cbm(socket, cos, cbm);
+        if ( ret )
+            return ret;
+        find->cbm = cbm;
+    }
+    find->ref++;
+    map[old_cos].ref--;
+    d->arch.psr_cos_ids[socket] = cos;
+    return 0;
+}
+
+/* Called with domain lock held, no psr specific lock needed */
+static void psr_free_cos(struct domain *d)
+{
+    unsigned int socket;
+    unsigned int cos;
+    struct psr_cat_cbm *map;
+
+    if( !d->arch.psr_cos_ids )
+        return;
+
+    for ( socket = 0; socket < opt_num_sockets; socket++)
+    {
+        cos = d->arch.psr_cos_ids[socket];
+        if ( cos == 0 )
+            continue;
+
+        map = cat_socket_info[socket].cos_cbm_map;
+        if ( map )
+            map[cos].ref--;
+    }
+
+    xfree(d->arch.psr_cos_ids);
+    d->arch.psr_cos_ids = NULL;
+}
+
+int psr_domain_init(struct domain *d)
+{
+    if ( cat_socket_info )
+    {
+        d->arch.psr_cos_ids = xzalloc_array(unsigned int, opt_num_sockets);
+        if ( !d->arch.psr_cos_ids )
+            return -ENOMEM;
+    }
+
+    return 0;
+}
+
+void psr_domain_free(struct domain *d)
+{
+    psr_free_rmid(d);
+    psr_free_cos(d);
+}
+
 static void cat_cpu_init(unsigned int cpu)
 {
     unsigned int eax, ebx, ecx, edx;
     struct psr_cat_socket_info *info;
-    unsigned int socket;
+    unsigned int socket, cos;
     const struct cpuinfo_x86 *c;
 
     socket = cpu_to_socket(cpu);
@@ -265,6 +474,17 @@ static void cat_cpu_init(unsigned int cpu)
         info->cbm_len = (eax & 0x1f) + 1;
         info->cos_max = (edx & 0xffff);
 
+        info->cos_cbm_map = xmalloc_array(struct psr_cat_cbm,
+                                          info->cos_max + 1UL);
+        if ( !info->cos_cbm_map )
+            return;
+
+        for ( cos = 0; cos <= info->cos_max; cos++ )
+            info->cos_cbm_map[cos].ref = 0;
+
+        /* cos=0 is reserved as default cbm(all ones). */
+        info->cos_cbm_map[0].cbm =  (1ull << info->cbm_len) - 1;
+
         info->enabled = 1;
         printk(XENLOG_DEBUG "CAT: enabled on socket %u, cos_max:%u, cbm_len:%u\n",
                socket, info->cos_max, info->cbm_len);
diff --git a/xen/arch/x86/sysctl.c b/xen/arch/x86/sysctl.c
index 611a291..096dc31 100644
--- a/xen/arch/x86/sysctl.c
+++ b/xen/arch/x86/sysctl.c
@@ -171,6 +171,26 @@ long arch_do_sysctl(
 
         break;
 
+    case XEN_SYSCTL_psr_cat_op:
+        switch ( sysctl->u.psr_cat_op.cmd )
+        {
+        case XEN_SYSCTL_PSR_CAT_get_l3_info:
+            ret = psr_get_cat_l3_info(sysctl->u.psr_cat_op.target,
+                                      &sysctl->u.psr_cat_op.u.l3_info.cbm_len,
+                                      &sysctl->u.psr_cat_op.u.l3_info.cos_max);
+            if ( ret )
+                break;
+
+            if ( __copy_to_guest(u_sysctl, sysctl, 1) )
+                ret = -EFAULT;
+
+            break;
+        default:
+            ret = -ENOSYS;
+            break;
+        }
+        break;
+
     default:
         ret = -ENOSYS;
         break;
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index 9cdffa8..9c4d0e6 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -333,7 +333,10 @@ struct arch_domain
     struct e820entry *e820;
     unsigned int nr_e820;
 
-    unsigned int psr_rmid; /* RMID assigned to the domain for CMT */
+    /* RMID assigned to the domain for CMT */
+    unsigned int psr_rmid;
+    /* COS assigned to the domain for each socket */
+    unsigned int *psr_cos_ids;
 
     /* Shared page for notifying that explicit PIRQ EOI is required. */
     unsigned long *pirq_eoi_map;
diff --git a/xen/include/asm-x86/msr-index.h b/xen/include/asm-x86/msr-index.h
index 83f2f70..b96f0f6 100644
--- a/xen/include/asm-x86/msr-index.h
+++ b/xen/include/asm-x86/msr-index.h
@@ -327,6 +327,7 @@
 #define MSR_IA32_CMT_EVTSEL		0x00000c8d
 #define MSR_IA32_CMT_CTR		0x00000c8e
 #define MSR_IA32_PSR_ASSOC		0x00000c8f
+#define MSR_IA32_L3_MASK(n)		(0x00000c90 + (n))
 
 /* Intel Model 6 */
 #define MSR_P6_PERFCTR(n)		(0x000000c1 + (n))
diff --git a/xen/include/asm-x86/psr.h b/xen/include/asm-x86/psr.h
index 585350c..74f150b 100644
--- a/xen/include/asm-x86/psr.h
+++ b/xen/include/asm-x86/psr.h
@@ -49,6 +49,14 @@ void psr_free_rmid(struct domain *d);
 
 void psr_ctxt_switch_to(struct domain *d);
 
+int psr_get_cat_l3_info(unsigned int socket, uint32_t *cbm_len,
+                        uint32_t *cos_max);
+int psr_get_l3_cbm(struct domain *d, unsigned int socket, uint64_t *cbm);
+int psr_set_l3_cbm(struct domain *d, unsigned int socket, uint64_t cbm);
+
+int psr_domain_init(struct domain *d);
+void psr_domain_free(struct domain *d);
+
 #endif /* __ASM_PSR_H__ */
 
 /*
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index ca0e51e..7c7baed 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -1005,6 +1005,16 @@ struct xen_domctl_psr_cmt_op {
 typedef struct xen_domctl_psr_cmt_op xen_domctl_psr_cmt_op_t;
 DEFINE_XEN_GUEST_HANDLE(xen_domctl_psr_cmt_op_t);
 
+struct xen_domctl_psr_cat_op {
+#define XEN_DOMCTL_PSR_CAT_OP_SET_L3_CBM     0
+#define XEN_DOMCTL_PSR_CAT_OP_GET_L3_CBM     1
+    uint32_t cmd;       /* IN: XEN_DOMCTL_PSR_CAT_OP_* */
+    uint32_t target;    /* IN: socket or cpu to be operated on */
+    uint64_t data;      /* IN/OUT */
+};
+typedef struct xen_domctl_psr_cat_op xen_domctl_psr_cat_op_t;
+DEFINE_XEN_GUEST_HANDLE(xen_domctl_psr_cat_op_t);
+
 struct xen_domctl {
     uint32_t cmd;
 #define XEN_DOMCTL_createdomain                   1
@@ -1080,6 +1090,7 @@ struct xen_domctl {
 #define XEN_DOMCTL_setvnumainfo                  74
 #define XEN_DOMCTL_psr_cmt_op                    75
 #define XEN_DOMCTL_arm_configure_domain          76
+#define XEN_DOMCTL_psr_cat_op                    77
 #define XEN_DOMCTL_gdbsx_guestmemio            1000
 #define XEN_DOMCTL_gdbsx_pausevcpu             1001
 #define XEN_DOMCTL_gdbsx_unpausevcpu           1002
@@ -1145,6 +1156,7 @@ struct xen_domctl {
         struct xen_domctl_gdbsx_domstatus   gdbsx_domstatus;
         struct xen_domctl_vnuma             vnuma;
         struct xen_domctl_psr_cmt_op        psr_cmt_op;
+        struct xen_domctl_psr_cat_op        psr_cat_op;
         uint8_t                             pad[128];
     } u;
 };
diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
index 8552dc6..9d8ed10 100644
--- a/xen/include/public/sysctl.h
+++ b/xen/include/public/sysctl.h
@@ -656,6 +656,20 @@ struct xen_sysctl_psr_cmt_op {
 typedef struct xen_sysctl_psr_cmt_op xen_sysctl_psr_cmt_op_t;
 DEFINE_XEN_GUEST_HANDLE(xen_sysctl_psr_cmt_op_t);
 
+#define XEN_SYSCTL_PSR_CAT_get_l3_info               0
+struct xen_sysctl_psr_cat_op {
+    uint32_t cmd;       /* IN: XEN_SYSCTL_PSR_CAT_* */
+    uint32_t target;    /* IN: socket or cpu to be operated on */
+    union {
+        struct {
+            uint32_t cbm_len;   /* OUT: CBM length */
+            uint32_t cos_max;   /* OUT: Maximum COS */
+        } l3_info;
+    } u;
+};
+typedef struct xen_sysctl_psr_cat_op xen_sysctl_psr_cat_op_t;
+DEFINE_XEN_GUEST_HANDLE(xen_sysctl_psr_cat_op_t);
+
 struct xen_sysctl {
     uint32_t cmd;
 #define XEN_SYSCTL_readconsole                    1
@@ -678,6 +692,7 @@ struct xen_sysctl {
 #define XEN_SYSCTL_scheduler_op                  19
 #define XEN_SYSCTL_coverage_op                   20
 #define XEN_SYSCTL_psr_cmt_op                    21
+#define XEN_SYSCTL_psr_cat_op                    22
     uint32_t interface_version; /* XEN_SYSCTL_INTERFACE_VERSION */
     union {
         struct xen_sysctl_readconsole       readconsole;
@@ -700,6 +715,7 @@ struct xen_sysctl {
         struct xen_sysctl_scheduler_op      scheduler_op;
         struct xen_sysctl_coverage_op       coverage_op;
         struct xen_sysctl_psr_cmt_op        psr_cmt_op;
+        struct xen_sysctl_psr_cat_op        psr_cat_op;
         uint8_t                             pad[128];
     } u;
 };
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH v2 5/7] x86: add scheduling support for Intel CAT
  2015-03-19 10:41 [PATCH v2 0/7] enable Cache Allocation Technology (CAT) for VMs Chao Peng
                   ` (3 preceding siblings ...)
  2015-03-19 10:41 ` [PATCH v2 4/7] x86: add support for COS/CBM manangement Chao Peng
@ 2015-03-19 10:41 ` Chao Peng
  2015-03-19 10:41 ` [PATCH v2 6/7] xsm: add CAT related xsm policies Chao Peng
  2015-03-19 10:41 ` [PATCH v2 7/7] tools: add tools support for Intel CAT Chao Peng
  6 siblings, 0 replies; 23+ messages in thread
From: Chao Peng @ 2015-03-19 10:41 UTC (permalink / raw)
  To: xen-devel
  Cc: keir, Ian.Campbell, stefano.stabellini, andrew.cooper3,
	Ian.Jackson, will.auld, JBeulich, wei.liu2, dgdegra

On context switch, write the the domain's Class of Service(COS) to MSR
IA32_PQR_ASSOC, to notify hardware to use the new COS.

For performance reason, the socket number and COS mask for current cpu
is also cached in the local per-CPU variable.

Signed-off-by: Chao Peng <chao.p.peng@linux.intel.com>
---
Changes in v2:
* merge common scheduling changes into scheduling improvement patch.
* use readable expr for psra->cos_mask.
---
 xen/arch/x86/psr.c | 33 +++++++++++++++++++++++++++++++--
 1 file changed, 31 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c
index af54aeb..e7e1dc0 100644
--- a/xen/arch/x86/psr.c
+++ b/xen/arch/x86/psr.c
@@ -36,6 +36,8 @@ struct psr_cat_socket_info {
 
 struct psr_assoc {
     uint64_t val;
+    unsigned int socket;
+    uint64_t cos_mask;
 };
 
 struct psr_cmt *__read_mostly psr_cmt;
@@ -211,9 +213,21 @@ void psr_free_rmid(struct domain *d)
 
 static inline void psr_assoc_init(void)
 {
+    unsigned int socket;
+    struct psr_cat_socket_info *info;
     struct psr_assoc *psra = &this_cpu(psr_assoc);
 
-    if ( psr_cmt_enabled() )
+    socket = cpu_to_socket(smp_processor_id());
+    psra->socket = socket;
+    if ( cat_socket_info && socket < opt_num_sockets )
+    {
+        info = cat_socket_info + socket;
+        if ( info->enabled )
+            psra->cos_mask = ((1ull << get_count_order(info->cos_max)) - 1)
+                             << 32;
+    }
+
+    if ( psr_cmt_enabled() || psra->cos_mask )
         rdmsrl(MSR_IA32_PSR_ASSOC, psra->val);
 }
 
@@ -236,6 +250,12 @@ static inline void psr_assoc_rmid(uint64_t *reg, unsigned int rmid)
     *reg = (*reg & ~rmid_mask) | (rmid & rmid_mask);
 }
 
+static inline void psr_assoc_cos(uint64_t *reg, unsigned int cos,
+                                 uint64_t cos_mask)
+{
+    *reg = (*reg & ~cos_mask) | (((uint64_t)cos << 32) & cos_mask);
+}
+
 void psr_ctxt_switch_to(struct domain *d)
 {
     uint64_t reg;
@@ -246,6 +266,15 @@ void psr_ctxt_switch_to(struct domain *d)
     if ( psr_cmt_enabled() )
         psr_assoc_rmid(&reg, d->arch.psr_rmid);
 
+    if ( psra->cos_mask )
+    {
+        if ( d->arch.psr_cos_ids )
+            psr_assoc_cos(&reg, d->arch.psr_cos_ids[psra->socket],
+                          psra->cos_mask);
+        else
+            psr_assoc_cos(&reg, 0, psra->cos_mask);
+    }
+
     psr_assoc_reg_write(psra, reg);
 }
 
@@ -511,7 +540,7 @@ static void psr_cpu_init(unsigned int cpu)
     if ( cat_socket_info )
         cat_cpu_init(cpu);
 
-    if (  psr_cmt_enabled() )
+    if (  psr_cmt_enabled() || cat_socket_info )
         psr_assoc_init();
 }
 
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH v2 6/7] xsm: add CAT related xsm policies
  2015-03-19 10:41 [PATCH v2 0/7] enable Cache Allocation Technology (CAT) for VMs Chao Peng
                   ` (4 preceding siblings ...)
  2015-03-19 10:41 ` [PATCH v2 5/7] x86: add scheduling support for Intel CAT Chao Peng
@ 2015-03-19 10:41 ` Chao Peng
  2015-03-19 10:41 ` [PATCH v2 7/7] tools: add tools support for Intel CAT Chao Peng
  6 siblings, 0 replies; 23+ messages in thread
From: Chao Peng @ 2015-03-19 10:41 UTC (permalink / raw)
  To: xen-devel
  Cc: keir, Ian.Campbell, stefano.stabellini, andrew.cooper3,
	Ian.Jackson, will.auld, JBeulich, wei.liu2, dgdegra

Add xsm policies for Cache Allocation Technology(CAT) related hypercalls
to restrict the functions visibility to control domain only.

Signed-off-by: Chao Peng <chao.p.peng@linux.intel.com>
Acked-by:  Daniel De Graaf <dgdegra@tycho.nsa.gov>
---
 tools/flask/policy/policy/modules/xen/xen.if | 2 +-
 tools/flask/policy/policy/modules/xen/xen.te | 4 +++-
 xen/xsm/flask/hooks.c                        | 6 ++++++
 xen/xsm/flask/policy/access_vectors          | 6 ++++++
 4 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/tools/flask/policy/policy/modules/xen/xen.if b/tools/flask/policy/policy/modules/xen/xen.if
index 2d32e1c..8bb081a 100644
--- a/tools/flask/policy/policy/modules/xen/xen.if
+++ b/tools/flask/policy/policy/modules/xen/xen.if
@@ -51,7 +51,7 @@ define(`create_domain_common', `
 			getaffinity setaffinity setvcpuextstate };
 	allow $1 $2:domain2 { set_cpuid settsc setscheduler setclaim
 			set_max_evtchn set_vnumainfo get_vnumainfo cacheflush
-			psr_cmt_op configure_domain };
+			psr_cmt_op configure_domain psr_cat_op };
 	allow $1 $2:security check_context;
 	allow $1 $2:shadow enable;
 	allow $1 $2:mmu { map_read map_write adjust memorymap physmap pinpage mmuext_op updatemp };
diff --git a/tools/flask/policy/policy/modules/xen/xen.te b/tools/flask/policy/policy/modules/xen/xen.te
index c0128aa..d431aaf 100644
--- a/tools/flask/policy/policy/modules/xen/xen.te
+++ b/tools/flask/policy/policy/modules/xen/xen.te
@@ -67,6 +67,7 @@ allow dom0_t xen_t:xen {
 allow dom0_t xen_t:xen2 {
     resource_op
     psr_cmt_op
+    psr_cat_op
 };
 allow dom0_t xen_t:mmu memorymap;
 
@@ -80,7 +81,8 @@ allow dom0_t dom0_t:domain {
 	getpodtarget setpodtarget set_misc_info set_virq_handler
 };
 allow dom0_t dom0_t:domain2 {
-	set_cpuid gettsc settsc setscheduler set_max_evtchn set_vnumainfo get_vnumainfo psr_cmt_op
+	set_cpuid gettsc settsc setscheduler set_max_evtchn set_vnumainfo
+	get_vnumainfo psr_cmt_op psr_cat_op
 };
 allow dom0_t dom0_t:resource { add remove };
 
diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
index 65094bb..12a3c61 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -729,6 +729,9 @@ static int flask_domctl(struct domain *d, int cmd)
     case XEN_DOMCTL_psr_cmt_op:
         return current_has_perm(d, SECCLASS_DOMAIN2, DOMAIN2__PSR_CMT_OP);
 
+    case XEN_DOMCTL_psr_cat_op:
+        return current_has_perm(d, SECCLASS_DOMAIN2, DOMAIN2__PSR_CAT_OP);
+
     case XEN_DOMCTL_arm_configure_domain:
         return current_has_perm(d, SECCLASS_DOMAIN2, DOMAIN2__CONFIGURE_DOMAIN);
 
@@ -790,6 +793,9 @@ static int flask_sysctl(int cmd)
     case XEN_SYSCTL_psr_cmt_op:
         return avc_current_has_perm(SECINITSID_XEN, SECCLASS_XEN2,
                                     XEN2__PSR_CMT_OP, NULL);
+    case XEN_SYSCTL_psr_cat_op:
+        return avc_current_has_perm(SECINITSID_XEN, SECCLASS_XEN2,
+                                    XEN2__PSR_CAT_OP, NULL);
 
     default:
         printk("flask_sysctl: Unknown op %d\n", cmd);
diff --git a/xen/xsm/flask/policy/access_vectors b/xen/xsm/flask/policy/access_vectors
index 8f44b9d..8cc1ef3 100644
--- a/xen/xsm/flask/policy/access_vectors
+++ b/xen/xsm/flask/policy/access_vectors
@@ -84,6 +84,9 @@ class xen2
     resource_op
 # XEN_SYSCTL_psr_cmt_op
     psr_cmt_op
+# XEN_SYSCTL_psr_cat_op
+    psr_cat_op
+
 }
 
 # Classes domain and domain2 consist of operations that a domain performs on
@@ -221,6 +224,9 @@ class domain2
     psr_cmt_op
 # XEN_DOMCTL_configure_domain
     configure_domain
+# XEN_DOMCTL_psr_cat_op
+    psr_cat_op
+
 }
 
 # Similar to class domain, but primarily contains domctls related to HVM domains
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH v2 7/7] tools: add tools support for Intel CAT
  2015-03-19 10:41 [PATCH v2 0/7] enable Cache Allocation Technology (CAT) for VMs Chao Peng
                   ` (5 preceding siblings ...)
  2015-03-19 10:41 ` [PATCH v2 6/7] xsm: add CAT related xsm policies Chao Peng
@ 2015-03-19 10:41 ` Chao Peng
  2015-03-19 12:51   ` Ian Campbell
  6 siblings, 1 reply; 23+ messages in thread
From: Chao Peng @ 2015-03-19 10:41 UTC (permalink / raw)
  To: xen-devel
  Cc: keir, Ian.Campbell, stefano.stabellini, andrew.cooper3,
	Ian.Jackson, will.auld, JBeulich, wei.liu2, dgdegra

This is the xc/xl changes to support Intel Cache Allocation
Technology(CAT). Two commands are introduced:
- xl psr-cat-cbm-set [-s socket] <domain> <cbm>
  Set cache capacity bitmasks(CBM) for a domain.
- xl psr-cat-show <domain>
  Show Cache Allocation Technology information.

Signed-off-by: Chao Peng <chao.p.peng@linux.intel.com>
---
 tools/libxc/include/xenctrl.h |  15 +++
 tools/libxc/xc_psr.c          |  74 ++++++++++++++
 tools/libxl/libxl.h           |  18 ++++
 tools/libxl/libxl_psr.c       | 107 ++++++++++++++++++--
 tools/libxl/libxl_types.idl   |   4 +
 tools/libxl/xl.h              |   4 +
 tools/libxl/xl_cmdimpl.c      | 225 ++++++++++++++++++++++++++++++++++++++++--
 tools/libxl/xl_cmdtable.c     |  13 +++
 8 files changed, 445 insertions(+), 15 deletions(-)

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index df18292..1373a46 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -2692,6 +2692,12 @@ enum xc_psr_cmt_type {
     XC_PSR_CMT_LOCAL_MEM_COUNT,
 };
 typedef enum xc_psr_cmt_type xc_psr_cmt_type;
+
+enum xc_psr_cat_type {
+    XC_PSR_CAT_L3_CBM = 1,
+};
+typedef enum xc_psr_cat_type xc_psr_cat_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,
@@ -2706,6 +2712,15 @@ int xc_psr_cmt_get_data(xc_interface *xch, uint32_t rmid, uint32_t cpu,
                         uint32_t psr_cmt_type, uint64_t *monitor_data,
                         uint64_t *tsc);
 int xc_psr_cmt_enabled(xc_interface *xch);
+
+int xc_psr_cat_set_domain_data(xc_interface *xch, uint32_t domid,
+                               xc_psr_cat_type type, uint32_t target,
+                               uint64_t data);
+int xc_psr_cat_get_domain_data(xc_interface *xch, uint32_t domid,
+                               xc_psr_cat_type type, uint32_t target,
+                               uint64_t *data);
+int xc_psr_cat_get_l3_info(xc_interface *xch, uint32_t socket,
+                           uint32_t *cos_max, uint32_t *cbm_len);
 #endif
 
 #endif /* XENCTRL_H */
diff --git a/tools/libxc/xc_psr.c b/tools/libxc/xc_psr.c
index e367a80..6c670d5 100644
--- a/tools/libxc/xc_psr.c
+++ b/tools/libxc/xc_psr.c
@@ -248,6 +248,80 @@ int xc_psr_cmt_enabled(xc_interface *xch)
 
     return 0;
 }
+int xc_psr_cat_set_domain_data(xc_interface *xch, uint32_t domid,
+                               xc_psr_cat_type type, uint32_t target,
+                               uint64_t data)
+{
+    DECLARE_DOMCTL;
+    uint32_t cmd;
+
+    switch ( type )
+    {
+    case XC_PSR_CAT_L3_CBM:
+        cmd = XEN_DOMCTL_PSR_CAT_OP_SET_L3_CBM;
+        break;
+    default:
+        return -1;
+    }
+
+    domctl.cmd = XEN_DOMCTL_psr_cat_op;
+    domctl.domain = (domid_t)domid;
+    domctl.u.psr_cat_op.cmd = cmd;
+    domctl.u.psr_cat_op.target = target;
+    domctl.u.psr_cat_op.data = data;
+
+    return do_domctl(xch, &domctl);
+}
+
+int xc_psr_cat_get_domain_data(xc_interface *xch, uint32_t domid,
+                               xc_psr_cat_type type, uint32_t target,
+                               uint64_t *data)
+{
+    int rc;
+    DECLARE_DOMCTL;
+    uint32_t cmd;
+
+    switch ( type )
+    {
+    case XC_PSR_CAT_L3_CBM:
+        cmd = XEN_DOMCTL_PSR_CAT_OP_GET_L3_CBM;
+        break;
+    default:
+        return -1;
+    }
+
+    domctl.cmd = XEN_DOMCTL_psr_cat_op;
+    domctl.domain = (domid_t)domid;
+    domctl.u.psr_cat_op.cmd = cmd;
+    domctl.u.psr_cat_op.target = target;
+
+    rc = do_domctl(xch, &domctl);
+
+    if ( !rc )
+        *data = domctl.u.psr_cat_op.data;
+
+    return rc;
+}
+
+int xc_psr_cat_get_l3_info(xc_interface *xch, uint32_t socket,
+                           uint32_t *cos_max, uint32_t *cbm_len)
+{
+    int rc;
+    DECLARE_SYSCTL;
+
+    sysctl.cmd = XEN_SYSCTL_psr_cat_op;
+    sysctl.u.psr_cat_op.cmd = XEN_SYSCTL_PSR_CAT_get_l3_info;
+    sysctl.u.psr_cat_op.target = socket;
+
+    rc = xc_sysctl(xch, &sysctl);
+    if ( !rc )
+    {
+        *cos_max = sysctl.u.psr_cat_op.u.l3_info.cos_max;
+        *cbm_len = sysctl.u.psr_cat_op.u.l3_info.cbm_len;
+    }
+
+    return rc;
+}
 
 /*
  * Local variables:
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 6bbc52d..dd2c28f 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -718,6 +718,13 @@ void libxl_mac_copy(libxl_ctx *ctx, libxl_mac *dst, libxl_mac *src);
  * If this is defined, the Memory Bandwidth Monitoring feature is supported.
  */
 #define LIBXL_HAVE_PSR_MBM 1
+
+/*
+ * LIBXL_HAVE_PSR_CAT
+ *
+ * If this is defined, the Cache Allocation Technology feature is supported.
+ */
+#define LIBXL_HAVE_PSR_CAT 1
 #endif
 
 typedef char **libxl_string_list;
@@ -1501,6 +1508,17 @@ int libxl_psr_cmt_get_sample(libxl_ctx *ctx,
                              uint64_t *tsc_r);
 #endif
 
+#ifdef LIBXL_HAVE_PSR_CAT
+int libxl_psr_cat_set_domain_data(libxl_ctx *ctx, uint32_t domid,
+                                  libxl_psr_cat_type type, uint32_t target,
+                                  uint64_t data);
+int libxl_psr_cat_get_domain_data(libxl_ctx *ctx, uint32_t domid,
+                                  libxl_psr_cat_type type, uint32_t target,
+                                  uint64_t *data_r);
+int libxl_psr_cat_get_l3_info(libxl_ctx *ctx, uint32_t socket,
+                              uint32_t *cos_max_r, uint32_t *cbm_len_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 3e1c792..c7f1e9c 100644
--- a/tools/libxl/libxl_psr.c
+++ b/tools/libxl/libxl_psr.c
@@ -19,14 +19,37 @@
 
 #define IA32_QM_CTR_ERROR_MASK         (0x3ul << 62)
 
-static void libxl__psr_cmt_log_err_msg(libxl__gc *gc, int err)
+static void libxl__psr_log_err_msg(libxl__gc *gc, int err)
 {
     char *msg;
 
     switch (err) {
     case ENOSYS:
+    case EOPNOTSUPP:
         msg = "unsupported operation";
         break;
+    case ESRCH:
+        msg = "invalid domain ID";
+        break;
+    case EBADSLT:
+        msg = "socket is not supported";
+        break;
+    case EFAULT:
+        msg = "failed to exchange data with Xen";
+        break;
+    default:
+        msg = "unknown error";
+        break;
+    }
+
+    LOGE(ERROR, "%s", msg);
+}
+
+static void libxl__psr_cmt_log_err_msg(libxl__gc *gc, int err)
+{
+    char *msg;
+
+    switch (err) {
     case ENODEV:
         msg = "CMT is not supported in this system";
         break;
@@ -39,15 +62,35 @@ static void libxl__psr_cmt_log_err_msg(libxl__gc *gc, int err)
     case EUSERS:
         msg = "no free RMID available";
         break;
-    case ESRCH:
-        msg = "invalid domain ID";
+    default:
+        libxl__psr_log_err_msg(gc, err);
+        return;
+    }
+
+    LOGE(ERROR, "%s", msg);
+}
+
+static void libxl__psr_cat_log_err_msg(libxl__gc *gc, int err)
+{
+    char *msg;
+
+    switch (err) {
+    case ENODEV:
+        msg = "CAT is not supported in this system";
         break;
-    case EFAULT:
-        msg = "failed to exchange data with Xen";
+    case ENOENT:
+        msg = "CAT is not enabled on the socket";
         break;
-    default:
-        msg = "unknown error";
+    case EUSERS:
+        msg = "no free COS available";
         break;
+    case EEXIST:
+        msg = "The same CBM is already set to this domain";
+        break;
+
+    default:
+        libxl__psr_log_err_msg(gc, err);
+        return;
     }
 
     LOGE(ERROR, "%s", msg);
@@ -247,6 +290,56 @@ out:
     return rc;
 }
 
+int libxl_psr_cat_set_domain_data(libxl_ctx *ctx, uint32_t domid,
+                                  libxl_psr_cat_type type, uint32_t target,
+                                  uint64_t data)
+{
+    GC_INIT(ctx);
+    int rc;
+
+    rc = xc_psr_cat_set_domain_data(ctx->xch, domid, type, target, data);
+    if (rc < 0) {
+        libxl__psr_cat_log_err_msg(gc, errno);
+        rc = ERROR_FAIL;
+    }
+
+    GC_FREE;
+    return rc;
+}
+
+int libxl_psr_cat_get_domain_data(libxl_ctx *ctx, uint32_t domid,
+                                  libxl_psr_cat_type type, uint32_t target,
+                                  uint64_t *data_r)
+{
+    GC_INIT(ctx);
+    int rc;
+
+    rc = xc_psr_cat_get_domain_data(ctx->xch, domid, type, target, data_r);
+    if (rc < 0) {
+        libxl__psr_cat_log_err_msg(gc, errno);
+        rc = ERROR_FAIL;
+    }
+
+    GC_FREE;
+    return rc;
+}
+
+int libxl_psr_cat_get_l3_info(libxl_ctx *ctx, uint32_t socket,
+                              uint32_t *cos_max_r, uint32_t *cbm_len_r)
+{
+    GC_INIT(ctx);
+    int rc;
+
+    rc = xc_psr_cat_get_l3_info(ctx->xch, socket, cos_max_r, cbm_len_r);
+    if (rc < 0) {
+        libxl__psr_cat_log_err_msg(gc, errno);
+        rc = ERROR_FAIL;
+    }
+
+    GC_FREE;
+    return rc;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 47af340..228f37c 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -699,3 +699,7 @@ libxl_psr_cmt_type = Enumeration("psr_cmt_type", [
     (2, "TOTAL_MEM_COUNT"),
     (3, "LOCAL_MEM_COUNT"),
     ])
+
+libxl_psr_cat_type = Enumeration("psr_cat_type", [
+    (1, "L3_CBM"),
+    ])
diff --git a/tools/libxl/xl.h b/tools/libxl/xl.h
index 5bc138c..85fa997 100644
--- a/tools/libxl/xl.h
+++ b/tools/libxl/xl.h
@@ -117,6 +117,10 @@ int main_psr_cmt_attach(int argc, char **argv);
 int main_psr_cmt_detach(int argc, char **argv);
 int main_psr_cmt_show(int argc, char **argv);
 #endif
+#ifdef LIBXL_HAVE_PSR_CAT
+int main_psr_cat_cbm_set(int argc, char **argv);
+int main_psr_cat_show(int argc, char **argv);
+#endif
 
 void help(const char *command);
 
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 5c40e84..fb0a9a4 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -85,6 +85,7 @@ libxl_ctx *ctx;
 
 xlchild children[child_max];
 
+#define ALL_SOCKETS ~0
 #define INVALID_DOMID ~0
 static const char *common_domname;
 static int fd_lock = -1;
@@ -7824,6 +7825,23 @@ out:
     return ret;
 }
 
+static uint32_t get_phy_socket_num(void)
+{
+    int rc;
+    uint32_t nr_sockets;
+    libxl_physinfo info;
+    libxl_physinfo_init(&info);
+    rc = libxl_get_physinfo(ctx, &info);
+    if (rc < 0) {
+        libxl_physinfo_dispose(&info);
+        return 0;
+    }
+    nr_sockets = info.nr_cpus / info.threads_per_core / info.cores_per_socket;
+    libxl_physinfo_dispose(&info);
+    return nr_sockets;
+
+}
+
 #ifdef LIBXL_HAVE_PSR_CMT
 
 #define MBM_SAMPLE_RETRY_MAX 4
@@ -7913,7 +7931,6 @@ 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;
-    libxl_physinfo info;
     int rc, nr_domains;
 
     if (!libxl_psr_cmt_enabled(ctx)) {
@@ -7927,15 +7944,11 @@ static int psr_cmt_show(libxl_psr_cmt_type type, uint32_t domid)
         return -1;
     }
 
-    libxl_physinfo_init(&info);
-    rc = libxl_get_physinfo(ctx, &info);
-    if (rc < 0) {
-        fprintf(stderr, "Failed getting physinfo, rc: %d\n", rc);
-        libxl_physinfo_dispose(&info);
+    nr_sockets = get_phy_socket_num();
+    if (nr_sockets == 0) {
+        fprintf(stderr, "Failed getting physinfo\n");
         return -1;
     }
-    nr_sockets = info.nr_cpus / info.threads_per_core / info.cores_per_socket;
-    libxl_physinfo_dispose(&info);
 
     rc = libxl_psr_cmt_get_total_rmid(ctx, &total_rmid);
     if (rc < 0) {
@@ -8057,6 +8070,202 @@ int main_psr_cmt_show(int argc, char **argv)
 }
 #endif
 
+#ifdef LIBXL_HAVE_PSR_CAT
+static int psr_cat_l3_cbm_set(uint32_t domid, uint32_t socket, uint64_t cbm)
+{
+    int rc;
+    uint32_t i, nr_sockets;
+
+    if (socket != ALL_SOCKETS) {
+        return libxl_psr_cat_set_domain_data(ctx, domid,
+                                             LIBXL_PSR_CAT_TYPE_L3_CBM,
+                                             socket, cbm);
+    } else {
+        nr_sockets = get_phy_socket_num();
+        if (nr_sockets == 0) {
+            fprintf(stderr, "Failed getting physinfo\n");
+            return -1;
+        }
+        for (i = 0; i < nr_sockets; i++) {
+            rc = libxl_psr_cat_set_domain_data(ctx, domid,
+                                               LIBXL_PSR_CAT_TYPE_L3_CBM,
+                                               i, cbm);
+            if (rc < 0) {
+                fprintf(stderr, "Failed to set l3 cbm for socket:%d\n", i);
+            return -1;
+            }
+        }
+    }
+    return 0;
+}
+
+struct psr_cat_l3_info
+{
+    uint32_t cos_max;
+    uint32_t cbm_len;
+};
+
+static void psr_cat_print_domain_info(libxl_dominfo *dominfo,
+                                      struct psr_cat_l3_info *l3_info,
+                                      uint32_t nr_sockets)
+{
+    char *domain_name;
+    uint32_t socketid;
+    uint64_t cbm;
+
+    domain_name = libxl_domid_to_name(ctx, dominfo->domid);
+    printf("%-40s %5d", domain_name, dominfo->domid);
+    free(domain_name);
+
+    for (socketid = 0; socketid < nr_sockets; socketid++) {
+        if (l3_info[socketid].cbm_len > 0 &&
+            !libxl_psr_cat_get_domain_data(ctx, dominfo->domid,
+                                           LIBXL_PSR_CAT_TYPE_L3_CBM,
+                                           socketid, &cbm) )
+            printf("%#16"PRIx64, cbm);
+    }
+
+    printf("\n");
+}
+
+static int psr_cat_show(uint32_t domid)
+{
+    uint32_t i, socketid, nr_sockets;
+    int rc, nr_domains;
+    uint32_t l3_cache_size;
+    struct psr_cat_l3_info *l3_info;
+
+    nr_sockets = get_phy_socket_num();
+    if (nr_sockets == 0) {
+        fprintf(stderr, "Failed getting physinfo\n");
+        return -1;
+    }
+
+    /* Header */
+    printf("%-40s %5s", "Name", "ID");
+    for (socketid = 0; socketid < nr_sockets; socketid++)
+        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);
+    }
+    printf("\n");
+
+    /* Max COS and CBM length */
+    l3_info = malloc(sizeof(l3_info) * nr_sockets);
+    //if (!l3_info)
+    for (socketid = 0; socketid < nr_sockets; socketid++) {
+        rc = libxl_psr_cat_get_l3_info(ctx, socketid,
+                                       &l3_info[socketid].cos_max,
+                                       &l3_info[socketid].cbm_len);
+        if (rc < 0) {
+            fprintf(stderr, "Failed to get system l3 info for socket:%d\n",
+                            socketid);
+            rc = -1;
+            goto out;
+        }
+    }
+    printf("%-46s", "Max COS");
+    for (socketid = 0; socketid < nr_sockets; socketid++) {
+        printf("%16u", l3_info[socketid].cos_max);
+    }
+    printf("\n");
+    printf("%-46s", "Default CBM");
+    for (socketid = 0; socketid < nr_sockets; socketid++) {
+        printf("%#16"PRIx64, (1ul << l3_info[socketid].cbm_len) - 1);
+    }
+    printf("\n");
+
+    /* Each domain */
+    if (domid != INVALID_DOMID) {
+        libxl_dominfo dominfo;
+        if (libxl_domain_info(ctx, &dominfo, domid)) {
+            fprintf(stderr, "Failed to get domain info for %d\n", domid);
+            rc = -1;
+            goto out;
+        }
+        psr_cat_print_domain_info(&dominfo, l3_info, nr_sockets);
+    }
+    else
+    {
+        libxl_dominfo *list;
+        if (!(list = libxl_list_domain(ctx, &nr_domains))) {
+            fprintf(stderr, "Failed to get domain info for domain list.\n");
+            rc = -1;
+            goto out;
+        }
+        for (i = 0; i < nr_domains; i++)
+            psr_cat_print_domain_info(list + i, l3_info, nr_sockets);
+        libxl_dominfo_list_free(list, nr_domains);
+    }
+    rc = 0;
+out:
+    if (l3_info)
+        free(l3_info);
+    return rc;
+}
+
+int main_psr_cat_cbm_set(int argc, char **argv)
+{
+    uint32_t domid;
+    uint32_t socket = ALL_SOCKETS;
+    uint64_t cbm;
+    char *ptr;
+    int opt = 0;
+
+    static struct option opts[] = {
+        {"socket", 0, 0, 's'},
+        {0, 0, 0, 0}
+    };
+
+    SWITCH_FOREACH_OPT(opt, "s", opts, "psr-cat-cbm-set", 1) {
+    case 's':
+        socket = strtol(optarg, NULL, 10);
+        break;
+    }
+
+    domid = find_domain(argv[optind]);
+    ptr = argv[optind + 1];
+    if (strlen(ptr) > 2 && ptr[0] == '0' && ptr[1] == 'x')
+        cbm = strtoll(ptr, NULL , 16);
+    else
+        cbm = strtoll(ptr, NULL , 10);
+
+    return psr_cat_l3_cbm_set(domid, socket, cbm);
+}
+
+int main_psr_cat_show(int argc, char **argv)
+{
+    int opt;
+    uint32_t domid;
+
+    SWITCH_FOREACH_OPT(opt, "", NULL, "psr-cat-show", 0) {
+        /* No options */
+    }
+
+    if (optind >= argc)
+        domid = INVALID_DOMID;
+    else if (optind == argc - 1)
+        domid = find_domain(argv[optind]);
+    else {
+        help("psr-cat-show");
+        return 2;
+    }
+
+    return psr_cat_show(domid);
+}
+
+#endif
+
 /*
  * Local variables:
  * mode: C
diff --git a/tools/libxl/xl_cmdtable.c b/tools/libxl/xl_cmdtable.c
index 22ab63b..ffaf4ed 100644
--- a/tools/libxl/xl_cmdtable.c
+++ b/tools/libxl/xl_cmdtable.c
@@ -542,6 +542,19 @@ struct cmd_spec cmd_table[] = {
       "\"total_mem_bandwidth\":     Show total memory bandwidth(KB/s)\n"
       "\"local_mem_bandwidth\":     Show local memory bandwidth(KB/s)\n",
     },
+    { "psr-cat-cbm-set",
+      &main_psr_cat_cbm_set, 0, 1,
+      "Set cache capacity bitmasks(CBM) for a domain",
+      "-s <socket>       Specify the socket to process, all sockets when not"
+      "specified\n"
+      "<Domain> <CBM>",
+    },
+    { "psr-cat-show",
+      &main_psr_cat_show, 0, 1,
+      "Show Cache Allocation Technology information",
+      "<Domain>",
+    },
+
 #endif
 };
 
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 23+ messages in thread

* Re: [PATCH v2 3/7] x86: detect and initialize Intel CAT feature
  2015-03-19 10:41 ` [PATCH v2 3/7] x86: detect and initialize Intel CAT feature Chao Peng
@ 2015-03-19 11:38   ` Tim Deegan
  2015-03-19 12:44   ` Dario Faggioli
  2015-03-20 17:00   ` Jan Beulich
  2 siblings, 0 replies; 23+ messages in thread
From: Tim Deegan @ 2015-03-19 11:38 UTC (permalink / raw)
  To: Chao Peng
  Cc: keir, Ian.Campbell, stefano.stabellini, andrew.cooper3,
	Ian.Jackson, xen-devel, will.auld, JBeulich, wei.liu2, dgdegra

Hi, 

At 18:41 +0800 on 19 Mar (1426790491), Chao Peng wrote:
> @@ -62,8 +84,15 @@ static void __init parse_psr_param(char *s)
>              *val_str++ = '\0';
>  
>          parse_psr_bool(s, val_str, "cmt", PSR_CMT);
> -        if ( val_str && !strcmp(s, "rmid_max") )
> -            opt_rmid_max = simple_strtoul(val_str, NULL, 0);
> +        parse_psr_bool(s, val_str, "cat", PSR_CAT);
> +
> +        if ( val_str )
> +        {
> +            if ( !strcmp(s, "rmid_max") )
> +                opt_rmid_max = simple_strtoul(val_str, NULL, 0);
> +            else if ( !strcmp(s, "num_sockets") )
> +                opt_num_sockets = simple_strtoul(val_str, NULL, 0);

This is an unfortunate interface for the user.  Can Xen not find out
how many sockets there are without needing to be told?

Also, per-socket data seems like a more generally useful thing that
other features will want to use.  Do you think future Intel cache
features will also have per-socket state?  If so, I think it would be
better to extend the per-cpu data system to handle per-socket areas.

Cheers,

Tim.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v2 3/7] x86: detect and initialize Intel CAT feature
  2015-03-19 10:41 ` [PATCH v2 3/7] x86: detect and initialize Intel CAT feature Chao Peng
  2015-03-19 11:38   ` Tim Deegan
@ 2015-03-19 12:44   ` Dario Faggioli
  2015-03-19 13:35     ` Jan Beulich
  2015-03-20 17:00   ` Jan Beulich
  2 siblings, 1 reply; 23+ messages in thread
From: Dario Faggioli @ 2015-03-19 12:44 UTC (permalink / raw)
  To: chao.p.peng
  Cc: Wei Liu, Ian Campbell, Andrew Cooper, xen-devel, will.auld,
	Stefano Stabellini, JBeulich, Ian Jackson, dgdegra,
	Keir (Xen.org)


[-- Attachment #1.1: Type: text/plain, Size: 1543 bytes --]

On Thu, 2015-03-19 at 18:41 +0800, Chao Peng wrote:

> diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
> index 63871cb..768c55f 100644
> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -1090,9 +1090,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> )`
> +> `= List of ( cmt:<boolean> | rmid_max:<integer> | cat:<boolean> | num_sockets:<interger> )`
>
As Tim, I also don't like the 'num_sockets' part of the interface.

Actually, I'm not sure I understand what the idea behind this parameter
is. You say:
 
> +* Cache Alllocation Technology (Broadwell and later).  Information regarding
> +  the cache allocation.
> +  * `cat` instructs Xen to enable/disable Cache Allocation Technology.
> +  * `num_sockets` indicates the number of available sockets for CAT feature
> +    detection. All the sockets up to num_sockets will be checked for CAT
> +    feature. 
>
What it the use case of specifying, say, num_sockets=2, on a 4 socket
system? 

Is it for making it possible for the user to disable the feature on some
of the socket? If yes, I think it does make sense, but then why always
the last ones? shouldn't this be a list of socket  IDs?

Something like:

 sockets=0,2

meaning that the user wants the feature enabled _exactly_ on socket 0
and 2 (if available there, of course).

Regards,
Dario

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v2 7/7] tools: add tools support for Intel CAT
  2015-03-19 10:41 ` [PATCH v2 7/7] tools: add tools support for Intel CAT Chao Peng
@ 2015-03-19 12:51   ` Ian Campbell
  2015-03-24 11:20     ` Chao Peng
  0 siblings, 1 reply; 23+ messages in thread
From: Ian Campbell @ 2015-03-19 12:51 UTC (permalink / raw)
  To: Chao Peng
  Cc: keir, stefano.stabellini, andrew.cooper3, Ian.Jackson, xen-devel,
	will.auld, JBeulich, wei.liu2, dgdegra

On Thu, 2015-03-19 at 18:41 +0800, Chao Peng wrote:
> This is the xc/xl changes to support Intel Cache Allocation
> Technology(CAT). Two commands are introduced:
> - xl psr-cat-cbm-set [-s socket] <domain> <cbm>
>   Set cache capacity bitmasks(CBM) for a domain.
> - xl psr-cat-show <domain>
>   Show Cache Allocation Technology information.
> 
> Signed-off-by: Chao Peng <chao.p.peng@linux.intel.com>
> ---
>  tools/libxc/include/xenctrl.h |  15 +++
>  tools/libxc/xc_psr.c          |  74 ++++++++++++++

At the libxc level this looks like a reasonable wrapping of the
hypercall interface, so if the hypervisor folks are happy with that then
I'm happy with this.

>  tools/libxl/libxl.h           |  18 ++++

At the libxl level things seem to be rather opaque to me, i.e. what is
domain_data? what does it contain? does it have any more semantics than
a 64-bit number?

What future new values might there be for libxl_psr_cat_type?

>  tools/libxl/libxl_psr.c       | 107 ++++++++++++++++++--
>  tools/libxl/libxl_types.idl   |   4 +
>  tools/libxl/xl.h              |   4 +
>  tools/libxl/xl_cmdimpl.c      | 225 ++++++++++++++++++++++++++++++++++++++++--
>  tools/libxl/xl_cmdtable.c     |  13 +++

You've missed updating the manpage.

>  8 files changed, 445 insertions(+), 15 deletions(-)
> 

> +#ifdef LIBXL_HAVE_PSR_CAT
> +int libxl_psr_cat_set_domain_data(libxl_ctx *ctx, uint32_t domid,
> +                                  libxl_psr_cat_type type, uint32_t target,
> +                                  uint64_t data);
> +int libxl_psr_cat_get_domain_data(libxl_ctx *ctx, uint32_t domid,
> +                                  libxl_psr_cat_type type, uint32_t target,
> +                                  uint64_t *data_r);
> +int libxl_psr_cat_get_l3_info(libxl_ctx *ctx, uint32_t socket,
> +                              uint32_t *cos_max_r, uint32_t *cbm_len_r);
> +#endif
> +


> [...]
> +static uint32_t get_phy_socket_num(void)

I think this would be better named "count_physical_sockets" or
something, otherwise I might think it takes a lock.

> +{
> +    int rc;
> +    uint32_t nr_sockets;

       uint32_t nr_sockets = 0;

> +    libxl_physinfo info;
> +    libxl_physinfo_init(&info);
> +    rc = libxl_get_physinfo(ctx, &info);

Then:
       if (!rc)
           nr_sockets = info.nr_cpus / info.threads_per_core / info.cores_per_socket;
      
       libxl_physinfo_dispose(&info);
       return nr_sockets;

means you only have one return path to do clean up on.

Also, does this function need to be in an ifdef, since it isn't called
unless LIBXL_HAVE_... is set? (IOW: does this compile for ARM)


> @@ -8057,6 +8070,202 @@ int main_psr_cmt_show(int argc, char **argv)
>  }
>  #endif
>  
> +#ifdef LIBXL_HAVE_PSR_CAT
> +static int psr_cat_l3_cbm_set(uint32_t domid, uint32_t socket, uint64_t cbm)
> +{
> +    int rc;
> +    uint32_t i, nr_sockets;
> +
> +    if (socket != ALL_SOCKETS) {
> +        return libxl_psr_cat_set_domain_data(ctx, domid,
> +                                             LIBXL_PSR_CAT_TYPE_L3_CBM,
> +                                             socket, cbm);
> +    } else {
> +        nr_sockets = get_phy_socket_num();

I wonder if the libxl API ought to allow for an "all sockets" argument
and then do all this internally instead of making the callers do it?

> +        if (nr_sockets == 0) {
> +            fprintf(stderr, "Failed getting physinfo\n");
> +            return -1;
> +        }
> +        for (i = 0; i < nr_sockets; i++) {
> +            rc = libxl_psr_cat_set_domain_data(ctx, domid,
> +                                               LIBXL_PSR_CAT_TYPE_L3_CBM,
> +                                               i, cbm);
> +            if (rc < 0) {
> +                fprintf(stderr, "Failed to set l3 cbm for socket:%d\n", i);
> +            return -1;

Indentation.

> +            }
> +        }
> +    }
> +    return 0;
> +}
> +
> +struct psr_cat_l3_info
> +{
> +    uint32_t cos_max;
> +    uint32_t cbm_len;
> +};

Is it every useful to retrieve these independently? Perhaps this struct
should be in the libxl API and be what is passed to
libxl_psr_cat_get_l3_info?

> +
> +static void psr_cat_print_domain_info(libxl_dominfo *dominfo,
> +                                      struct psr_cat_l3_info *l3_info,
> +                                      uint32_t nr_sockets)
> +{
> +    char *domain_name;
> +    uint32_t socketid;
> +    uint64_t cbm;
> +
> +    domain_name = libxl_domid_to_name(ctx, dominfo->domid);
> +    printf("%-40s %5d", domain_name, dominfo->domid);
> +    free(domain_name);
> +
> +    for (socketid = 0; socketid < nr_sockets; socketid++) {
> +        if (l3_info[socketid].cbm_len > 0 &&
> +            !libxl_psr_cat_get_domain_data(ctx, dominfo->domid,
> +                                           LIBXL_PSR_CAT_TYPE_L3_CBM,
> +                                           socketid, &cbm) )
> +            printf("%#16"PRIx64, cbm);

Print socket id too?

> +
> +    /* Header */
> +    printf("%-40s %5s", "Name", "ID");
> +    for (socketid = 0; socketid < nr_sockets; socketid++)
> +        printf("%14s %d", "Socket", socketid);
> +    printf("\n");
> +
> +    /* Total L3 cache size */
> +    printf("%-46s", "Total L3 Cache Size");

How wide are these lines going to be? Can we try and keep it to <80
columns? Perhaps you could include an example of the output of each of
the show functions in the commit message?

> +    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");
> +
> +    /* Max COS and CBM length */
> +    l3_info = malloc(sizeof(l3_info) * nr_sockets);
> +    //if (!l3_info)

Leftover.

> diff --git a/tools/libxl/xl_cmdtable.c b/tools/libxl/xl_cmdtable.c
> index 22ab63b..ffaf4ed 100644
> --- a/tools/libxl/xl_cmdtable.c
> +++ b/tools/libxl/xl_cmdtable.c
> @@ -542,6 +542,19 @@ struct cmd_spec cmd_table[] = {
>        "\"total_mem_bandwidth\":     Show total memory bandwidth(KB/s)\n"
>        "\"local_mem_bandwidth\":     Show local memory bandwidth(KB/s)\n",
>      },
> +    { "psr-cat-cbm-set",
> +      &main_psr_cat_cbm_set, 0, 1,
> +      "Set cache capacity bitmasks(CBM) for a domain",
> +      "-s <socket>       Specify the socket to process, all sockets when not"

"Specify the socket to process, otherwise all sockets are processed"

Ian.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v2 3/7] x86: detect and initialize Intel CAT feature
  2015-03-19 12:44   ` Dario Faggioli
@ 2015-03-19 13:35     ` Jan Beulich
  2015-03-20 10:24       ` Dario Faggioli
  2015-03-23  8:36       ` Chao Peng
  0 siblings, 2 replies; 23+ messages in thread
From: Jan Beulich @ 2015-03-19 13:35 UTC (permalink / raw)
  To: Dario Faggioli
  Cc: Wei Liu, Ian Campbell, Andrew Cooper, xen-devel, will.auld,
	Stefano Stabellini, chao.p.peng, Ian Jackson, dgdegra,
	Keir (Xen.org)

>>> On 19.03.15 at 13:44, <dario.faggioli@citrix.com> wrote:
> On Thu, 2015-03-19 at 18:41 +0800, Chao Peng wrote:
> 
>> diff --git a/docs/misc/xen-command-line.markdown 
> b/docs/misc/xen-command-line.markdown
>> index 63871cb..768c55f 100644
>> --- a/docs/misc/xen-command-line.markdown
>> +++ b/docs/misc/xen-command-line.markdown
>> @@ -1090,9 +1090,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> )`
>> +> `= List of ( cmt:<boolean> | rmid_max:<integer> | cat:<boolean> | 
> num_sockets:<interger> )`
>>
> As Tim, I also don't like the 'num_sockets' part of the interface.
> 
> Actually, I'm not sure I understand what the idea behind this parameter
> is. You say:
>  
>> +* Cache Alllocation Technology (Broadwell and later).  Information regarding
>> +  the cache allocation.
>> +  * `cat` instructs Xen to enable/disable Cache Allocation Technology.
>> +  * `num_sockets` indicates the number of available sockets for CAT feature
>> +    detection. All the sockets up to num_sockets will be checked for CAT
>> +    feature. 
>>
> What it the use case of specifying, say, num_sockets=2, on a 4 socket
> system? 
> 
> Is it for making it possible for the user to disable the feature on some
> of the socket? If yes, I think it does make sense, but then why always
> the last ones? shouldn't this be a list of socket  IDs?

I had asked about this before, and the answer was that this is to
_extend_ the range of sockets, to cover the CPU hotplug case. But
of course I could see ways to at least default the count to something
reasonable even taking hotplug into account (like taking the boot
time CPUs-per-socket value and dividing nr_cpu_ids by that value).

Jan

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v2 3/7] x86: detect and initialize Intel CAT feature
  2015-03-19 13:35     ` Jan Beulich
@ 2015-03-20 10:24       ` Dario Faggioli
  2015-03-23  8:36       ` Chao Peng
  1 sibling, 0 replies; 23+ messages in thread
From: Dario Faggioli @ 2015-03-20 10:24 UTC (permalink / raw)
  To: JBeulich
  Cc: Wei Liu, Ian Campbell, Andrew Cooper, xen-devel, will.auld,
	Stefano Stabellini, chao.p.peng, Ian Jackson, dgdegra,
	Keir (Xen.org)


[-- Attachment #1.1: Type: text/plain, Size: 2243 bytes --]

On Thu, 2015-03-19 at 13:35 +0000, Jan Beulich wrote:
> >>> On 19.03.15 at 13:44, <dario.faggioli@citrix.com> wrote:
> > On Thu, 2015-03-19 at 18:41 +0800, Chao Peng wrote:
> > 
> >> diff --git a/docs/misc/xen-command-line.markdown 
> > b/docs/misc/xen-command-line.markdown
> >> index 63871cb..768c55f 100644
> >> --- a/docs/misc/xen-command-line.markdown
> >> +++ b/docs/misc/xen-command-line.markdown
> >> @@ -1090,9 +1090,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> )`
> >> +> `= List of ( cmt:<boolean> | rmid_max:<integer> | cat:<boolean> | 
> > num_sockets:<interger> )`
> >>
> > As Tim, I also don't like the 'num_sockets' part of the interface.
> > 
> > Actually, I'm not sure I understand what the idea behind this parameter
> > is. You say:
> >  
> >> +* Cache Alllocation Technology (Broadwell and later).  Information regarding
> >> +  the cache allocation.
> >> +  * `cat` instructs Xen to enable/disable Cache Allocation Technology.
> >> +  * `num_sockets` indicates the number of available sockets for CAT feature
> >> +    detection. All the sockets up to num_sockets will be checked for CAT
> >> +    feature. 
> >>
> > What it the use case of specifying, say, num_sockets=2, on a 4 socket
> > system? 
> > 
> > Is it for making it possible for the user to disable the feature on some
> > of the socket? If yes, I think it does make sense, but then why always
> > the last ones? shouldn't this be a list of socket  IDs?
> 
> I had asked about this before, and the answer was that this is to
> _extend_ the range of sockets, to cover the CPU hotplug case. 
>
Ah... yes, hotplug is mentioned in the text.

> But
> of course I could see ways to at least default the count to something
> reasonable even taking hotplug into account (like taking the boot
> time CPUs-per-socket value and dividing nr_cpu_ids by that value).
> 
Indeed. I'm not that much a user of hotplug, but AFAICT, what's cool of
it is that it is dynamic, and having to define this parameter upfront
and/or then being limited by it is certainly suboptimal.

Dario

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v2 1/7] x86: clean up psr boot parameter parsing
  2015-03-19 10:41 ` [PATCH v2 1/7] x86: clean up psr boot parameter parsing Chao Peng
@ 2015-03-20 16:47   ` Jan Beulich
  2015-03-23  8:32     ` Chao Peng
  0 siblings, 1 reply; 23+ messages in thread
From: Jan Beulich @ 2015-03-20 16:47 UTC (permalink / raw)
  To: Chao Peng
  Cc: wei.liu2, Ian.Campbell, stefano.stabellini, andrew.cooper3,
	Ian.Jackson, xen-devel, will.auld, keir, dgdegra

>>> On 19.03.15 at 11:41, <chao.p.peng@linux.intel.com> wrote:
> +static void __init parse_psr_bool(char* s, char* value, char* feature, int bit)
> +{
> +    if ( !strcmp(s, feature) )
> +    {
> +        if ( !value )
> +            opt_psr |= bit;
> +        else
> +        {
> +            int val_int = parse_bool(value);
> +
> +            if ( val_int == 1 )
> +                opt_psr |= bit;
> +            else if ( val_int != 0 )
> +                printk("PSR: unknown %s value: %s\n", feature, value);
> +        }

Even more so that now you try to consolidate and re-use this, you
should honor the "off" case as much as the "on" one, i.e. explicitly
disable a feature if it was requested to be off.

Jan

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v2 3/7] x86: detect and initialize Intel CAT feature
  2015-03-19 10:41 ` [PATCH v2 3/7] x86: detect and initialize Intel CAT feature Chao Peng
  2015-03-19 11:38   ` Tim Deegan
  2015-03-19 12:44   ` Dario Faggioli
@ 2015-03-20 17:00   ` Jan Beulich
  2 siblings, 0 replies; 23+ messages in thread
From: Jan Beulich @ 2015-03-20 17:00 UTC (permalink / raw)
  To: Chao Peng
  Cc: wei.liu2, Ian.Campbell, stefano.stabellini, andrew.cooper3,
	Ian.Jackson, xen-devel, will.auld, keir, dgdegra

>>> On 19.03.15 at 11:41, <chao.p.peng@linux.intel.com> wrote:
> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -1090,9 +1090,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> )`
> +> `= List of ( cmt:<boolean> | rmid_max:<integer> | cat:<boolean> | num_sockets:<interger> )`
>  
> -> Default: `psr=cmt:0,rmid_max:255`
> +> Default: `psr=cmt:0,rmid_max:255,cat:0,num_sockets:0(Detect at boot time)`

This "Detect at boot time" clearly doesn't belong on the "Default:" line.

> @@ -1112,6 +1117,16 @@ The following resources are available:
>    total/local memory bandwidth. Follow the same options with Cache Monitoring
>    Technology.
>  
> +* Cache Alllocation Technology (Broadwell and later).  Information regarding
> +  the cache allocation.
> +  * `cat` instructs Xen to enable/disable Cache Allocation Technology.
> +  * `num_sockets` indicates the number of available sockets for CAT feature
> +    detection. All the sockets up to num_sockets will be checked for CAT
> +    feature. The value is normally detected at boot time automatically if not
> +    specified(0). While the value may need to be specified manually for CPU
> +    hot-plug scenario in which case the automatic sockets number detection may
> +    be not correct.

As mentioned before, the hotplug case should be taken care of as
much as possible to avoid someone having to use the option. I.e.
this ...

> +static unsigned int get_max_socket(void)
> +{
> +    unsigned int cpu, socket = 0;
> +
> +    for_each_present_cpu(cpu)
> +        if ( socket < cpu_to_socket(cpu) )
> +            socket = cpu_to_socket(cpu);
> +
> +    return socket;
> +}

... is too simplistic.

> @@ -204,9 +233,66 @@ void psr_ctxt_switch_to(struct domain *d)
>      psr_assoc_reg_write(psra, reg);
>  }
>  
> +static void cat_cpu_init(unsigned int cpu)
> +{
> +    unsigned int eax, ebx, ecx, edx;
> +    struct psr_cat_socket_info *info;
> +    unsigned int socket;
> +    const struct cpuinfo_x86 *c;
> +
> +    socket = cpu_to_socket(cpu);
> +    if ( socket >= opt_num_sockets )
> +    {
> +        printk(XENLOG_WARNING "CAT: disabled on socket %d as num_sockets is %d\n",
> +               socket, opt_num_sockets);
> +        return;
> +    }
> +
> +    info = cat_socket_info + socket;
> +
> +    /* Avoid initializing more than one times for the same socket. */
> +    if ( test_and_set_bool(info->initialized) )
> +        return;
> +
> +    c = cpu_data + cpu;
> +    if ( !cpu_has(c, X86_FEATURE_CAT) )
> +        return;
> +
> +    cpuid_count(0x10, 0, &eax, &ebx, &ecx, &edx);
> +    if ( ebx & PSR_RESOURCE_TYPE_L3 )
> +    {
> +        cpuid_count(0x10, 1, &eax, &ebx, &ecx, &edx);

If numbers like the 0x10 here repeat, please give them names so one
can associate all uses with one another.

>  static void psr_cpu_init(unsigned int cpu)
>  {
> -    psr_assoc_init();
> +    if ( cat_socket_info )
> +        cat_cpu_init(cpu);
> +
> +    if (  psr_cmt_enabled() )
> +        psr_assoc_init();

So the previous patch put the same conditional inside psr_assoc_init().
Please don't add such obviously redundant checks.

Jan

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v2 4/7] x86: add support for COS/CBM manangement
  2015-03-19 10:41 ` [PATCH v2 4/7] x86: add support for COS/CBM manangement Chao Peng
@ 2015-03-20 17:13   ` Jan Beulich
  2015-03-23  8:47     ` Chao Peng
  0 siblings, 1 reply; 23+ messages in thread
From: Jan Beulich @ 2015-03-20 17:13 UTC (permalink / raw)
  To: Chao Peng
  Cc: wei.liu2, Ian.Campbell, stefano.stabellini, andrew.cooper3,
	Ian.Jackson, xen-devel, will.auld, keir, dgdegra

>>> On 19.03.15 at 11:41, <chao.p.peng@linux.intel.com> wrote:
> +static unsigned int get_socket_cpu(unsigned int socket)
> +{
> +    unsigned int cpu;
> +
> +    for_each_online_cpu ( cpu )
> +       if ( cpu_to_socket(cpu) == socket )
> +           return cpu;
> +    return nr_cpu_ids;
> +}

This can be a rather long loop for a huge system. I think you need to
find some better solution for this.

> +static void psr_free_cos(struct domain *d)
> +{
> +    unsigned int socket;
> +    unsigned int cos;
> +    struct psr_cat_cbm *map;
> +
> +    if( !d->arch.psr_cos_ids )
> +        return;
> +
> +    for ( socket = 0; socket < opt_num_sockets; socket++)
> +    {
> +        cos = d->arch.psr_cos_ids[socket];
> +        if ( cos == 0 )
> +            continue;
> +
> +        map = cat_socket_info[socket].cos_cbm_map;
> +        if ( map )
> +            map[cos].ref--;

Can map really ever be NULL here? I.e. isn't this rather an
ASSERT() instead of if()?

> @@ -265,6 +474,17 @@ static void cat_cpu_init(unsigned int cpu)
>          info->cbm_len = (eax & 0x1f) + 1;
>          info->cos_max = (edx & 0xffff);
>  
> +        info->cos_cbm_map = xmalloc_array(struct psr_cat_cbm,
> +                                          info->cos_max + 1UL);
> +        if ( !info->cos_cbm_map )
> +            return;
> +
> +        for ( cos = 0; cos <= info->cos_max; cos++ )
> +            info->cos_cbm_map[cos].ref = 0;

So why not simply xzalloc_array()?

> --- a/xen/arch/x86/sysctl.c
> +++ b/xen/arch/x86/sysctl.c
> @@ -171,6 +171,26 @@ long arch_do_sysctl(
>  
>          break;
>  
> +    case XEN_SYSCTL_psr_cat_op:
> +        switch ( sysctl->u.psr_cat_op.cmd )
> +        {
> +        case XEN_SYSCTL_PSR_CAT_get_l3_info:
> +            ret = psr_get_cat_l3_info(sysctl->u.psr_cat_op.target,
> +                                      &sysctl->u.psr_cat_op.u.l3_info.cbm_len,
> +                                      &sysctl->u.psr_cat_op.u.l3_info.cos_max);
> +            if ( ret )
> +                break;
> +
> +            if ( __copy_to_guest(u_sysctl, sysctl, 1) )
> +                ret = -EFAULT;

Please fold the two if()-s together.

> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -1005,6 +1005,16 @@ struct xen_domctl_psr_cmt_op {
>  typedef struct xen_domctl_psr_cmt_op xen_domctl_psr_cmt_op_t;
>  DEFINE_XEN_GUEST_HANDLE(xen_domctl_psr_cmt_op_t);
>  
> +struct xen_domctl_psr_cat_op {
> +#define XEN_DOMCTL_PSR_CAT_OP_SET_L3_CBM     0
> +#define XEN_DOMCTL_PSR_CAT_OP_GET_L3_CBM     1
> +    uint32_t cmd;       /* IN: XEN_DOMCTL_PSR_CAT_OP_* */
> +    uint32_t target;    /* IN: socket or cpu to be operated on */

How can this be socket _or_ CPU?

Jan

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v2 1/7] x86: clean up psr boot parameter parsing
  2015-03-20 16:47   ` Jan Beulich
@ 2015-03-23  8:32     ` Chao Peng
  2015-03-23  8:47       ` Jan Beulich
  0 siblings, 1 reply; 23+ messages in thread
From: Chao Peng @ 2015-03-23  8:32 UTC (permalink / raw)
  To: Jan Beulich
  Cc: wei.liu2, Ian.Campbell, stefano.stabellini, andrew.cooper3,
	Ian.Jackson, xen-devel, will.auld, keir, dgdegra

On Fri, Mar 20, 2015 at 04:47:27PM +0000, Jan Beulich wrote:
> >>> On 19.03.15 at 11:41, <chao.p.peng@linux.intel.com> wrote:
> > +static void __init parse_psr_bool(char* s, char* value, char* feature, int bit)
> > +{
> > +    if ( !strcmp(s, feature) )
> > +    {
> > +        if ( !value )
> > +            opt_psr |= bit;
> > +        else
> > +        {
> > +            int val_int = parse_bool(value);
> > +
> > +            if ( val_int == 1 )
> > +                opt_psr |= bit;
> > +            else if ( val_int != 0 )
> > +                printk("PSR: unknown %s value: %s\n", feature, value);
> > +        }
> 
> Even more so that now you try to consolidate and re-use this, you
> should honor the "off" case as much as the "on" one, i.e. explicitly
> disable a feature if it was requested to be off.
> 
This will change the default behavior(both cmt/cat will default to on),
is this your expectation?

Chao

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v2 3/7] x86: detect and initialize Intel CAT feature
  2015-03-19 13:35     ` Jan Beulich
  2015-03-20 10:24       ` Dario Faggioli
@ 2015-03-23  8:36       ` Chao Peng
  1 sibling, 0 replies; 23+ messages in thread
From: Chao Peng @ 2015-03-23  8:36 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Wei Liu, Ian Campbell, Andrew Cooper, Dario Faggioli, xen-devel,
	will.auld, Stefano Stabellini, Ian Jackson, dgdegra,
	Keir (Xen.org)

On Thu, Mar 19, 2015 at 01:35:34PM +0000, Jan Beulich wrote:
> >>> On 19.03.15 at 13:44, <dario.faggioli@citrix.com> wrote:
> > On Thu, 2015-03-19 at 18:41 +0800, Chao Peng wrote:
> > 
> >> diff --git a/docs/misc/xen-command-line.markdown 
> > b/docs/misc/xen-command-line.markdown
> >> index 63871cb..768c55f 100644
> >> --- a/docs/misc/xen-command-line.markdown
> >> +++ b/docs/misc/xen-command-line.markdown
> >> @@ -1090,9 +1090,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> )`
> >> +> `= List of ( cmt:<boolean> | rmid_max:<integer> | cat:<boolean> | 
> > num_sockets:<interger> )`
> >>
> > As Tim, I also don't like the 'num_sockets' part of the interface.
> > 
> > Actually, I'm not sure I understand what the idea behind this parameter
> > is. You say:
> >  
> >> +* Cache Alllocation Technology (Broadwell and later).  Information regarding
> >> +  the cache allocation.
> >> +  * `cat` instructs Xen to enable/disable Cache Allocation Technology.
> >> +  * `num_sockets` indicates the number of available sockets for CAT feature
> >> +    detection. All the sockets up to num_sockets will be checked for CAT
> >> +    feature. 
> >>
> > What it the use case of specifying, say, num_sockets=2, on a 4 socket
> > system? 
> > 
> > Is it for making it possible for the user to disable the feature on some
> > of the socket? If yes, I think it does make sense, but then why always
> > the last ones? shouldn't this be a list of socket  IDs?
> 
> I had asked about this before, and the answer was that this is to
> _extend_ the range of sockets, to cover the CPU hotplug case. But
> of course I could see ways to at least default the count to something
> reasonable even taking hotplug into account (like taking the boot
> time CPUs-per-socket value and dividing nr_cpu_ids by that value).

Sounds reasonable. I will adopt this suggestion and eliminate the
necessary to introduce a boot option like "num_sockets".

Chao

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v2 1/7] x86: clean up psr boot parameter parsing
  2015-03-23  8:32     ` Chao Peng
@ 2015-03-23  8:47       ` Jan Beulich
  0 siblings, 0 replies; 23+ messages in thread
From: Jan Beulich @ 2015-03-23  8:47 UTC (permalink / raw)
  To: Chao Peng
  Cc: wei.liu2, Ian.Campbell, stefano.stabellini, andrew.cooper3,
	Ian.Jackson, xen-devel, will.auld, keir, dgdegra

>>> On 23.03.15 at 09:32, <chao.p.peng@linux.intel.com> wrote:
> On Fri, Mar 20, 2015 at 04:47:27PM +0000, Jan Beulich wrote:
>> >>> On 19.03.15 at 11:41, <chao.p.peng@linux.intel.com> wrote:
>> > +static void __init parse_psr_bool(char* s, char* value, char* feature, int 
> bit)
>> > +{
>> > +    if ( !strcmp(s, feature) )
>> > +    {
>> > +        if ( !value )
>> > +            opt_psr |= bit;
>> > +        else
>> > +        {
>> > +            int val_int = parse_bool(value);
>> > +
>> > +            if ( val_int == 1 )
>> > +                opt_psr |= bit;
>> > +            else if ( val_int != 0 )
>> > +                printk("PSR: unknown %s value: %s\n", feature, value);
>> > +        }
>> 
>> Even more so that now you try to consolidate and re-use this, you
>> should honor the "off" case as much as the "on" one, i.e. explicitly
>> disable a feature if it was requested to be off.
>> 
> This will change the default behavior(both cmt/cat will default to on),
> is this your expectation?

Certainly not. And why would that be? Note that I didn't ask you to
flip things, but I said "as much as", i.e. _both_ directions should be
supported (and hence the current and future default doesn't matter
at all here).

Jan

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v2 4/7] x86: add support for COS/CBM manangement
  2015-03-20 17:13   ` Jan Beulich
@ 2015-03-23  8:47     ` Chao Peng
  2015-03-23  9:03       ` Jan Beulich
  0 siblings, 1 reply; 23+ messages in thread
From: Chao Peng @ 2015-03-23  8:47 UTC (permalink / raw)
  To: Jan Beulich
  Cc: wei.liu2, Ian.Campbell, stefano.stabellini, andrew.cooper3,
	Ian.Jackson, xen-devel, will.auld, keir, dgdegra

On Fri, Mar 20, 2015 at 05:13:44PM +0000, Jan Beulich wrote:
> >>> On 19.03.15 at 11:41, <chao.p.peng@linux.intel.com> wrote:
> > +static unsigned int get_socket_cpu(unsigned int socket)
> > +{
> > +    unsigned int cpu;
> > +
> > +    for_each_online_cpu ( cpu )
> > +       if ( cpu_to_socket(cpu) == socket )
> > +           return cpu;
> > +    return nr_cpu_ids;
> > +}
> 
> This can be a rather long loop for a huge system. I think you need to
> find some better solution for this.

Maintain a socket_cpu_map for each socket? Sounds no existed way.

> > +struct xen_domctl_psr_cat_op {
> > +#define XEN_DOMCTL_PSR_CAT_OP_SET_L3_CBM     0
> > +#define XEN_DOMCTL_PSR_CAT_OP_GET_L3_CBM     1
> > +    uint32_t cmd;       /* IN: XEN_DOMCTL_PSR_CAT_OP_* */
> > +    uint32_t target;    /* IN: socket or cpu to be operated on */
> 
> How can this be socket _or_ CPU?

This is for future feature like L2 cache which needs to specify cpu but
not socket. Perhaps not mention here to avoid confusing.

Chao

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v2 4/7] x86: add support for COS/CBM manangement
  2015-03-23  8:47     ` Chao Peng
@ 2015-03-23  9:03       ` Jan Beulich
  0 siblings, 0 replies; 23+ messages in thread
From: Jan Beulich @ 2015-03-23  9:03 UTC (permalink / raw)
  To: Chao Peng
  Cc: wei.liu2, Ian.Campbell, stefano.stabellini, andrew.cooper3,
	Ian.Jackson, xen-devel, will.auld, keir, dgdegra

>>> On 23.03.15 at 09:47, <chao.p.peng@linux.intel.com> wrote:
> On Fri, Mar 20, 2015 at 05:13:44PM +0000, Jan Beulich wrote:
>> >>> On 19.03.15 at 11:41, <chao.p.peng@linux.intel.com> wrote:
>> > +static unsigned int get_socket_cpu(unsigned int socket)
>> > +{
>> > +    unsigned int cpu;
>> > +
>> > +    for_each_online_cpu ( cpu )
>> > +       if ( cpu_to_socket(cpu) == socket )
>> > +           return cpu;
>> > +    return nr_cpu_ids;
>> > +}
>> 
>> This can be a rather long loop for a huge system. I think you need to
>> find some better solution for this.
> 
> Maintain a socket_cpu_map for each socket? Sounds no existed way.

Perhaps your per-socket data structure could store a pointer to
a suitable cpu_sibling_mask instance (and then the function above
could simply be cpumask_any() on that mask). Of course it'll need
updating when the owning CPU goes down, but thanks to you
having the very sibling mask in your hands at that point, finding
a replacement should be trivial.

Jan

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v2 7/7] tools: add tools support for Intel CAT
  2015-03-19 12:51   ` Ian Campbell
@ 2015-03-24 11:20     ` Chao Peng
  2015-03-24 12:22       ` Ian Campbell
  0 siblings, 1 reply; 23+ messages in thread
From: Chao Peng @ 2015-03-24 11:20 UTC (permalink / raw)
  To: Ian Campbell
  Cc: keir, stefano.stabellini, andrew.cooper3, Ian.Jackson, xen-devel,
	will.auld, JBeulich, wei.liu2, dgdegra

On Thu, Mar 19, 2015 at 12:51:19PM +0000, Ian Campbell wrote:
> On Thu, 2015-03-19 at 18:41 +0800, Chao Peng wrote:
> > This is the xc/xl changes to support Intel Cache Allocation
> > Technology(CAT). Two commands are introduced:
> > - xl psr-cat-cbm-set [-s socket] <domain> <cbm>
> >   Set cache capacity bitmasks(CBM) for a domain.
> > - xl psr-cat-show <domain>
> >   Show Cache Allocation Technology information.
> > 
> > Signed-off-by: Chao Peng <chao.p.peng@linux.intel.com>
> > ---
> >  tools/libxc/include/xenctrl.h |  15 +++
> >  tools/libxc/xc_psr.c          |  74 ++++++++++++++
> 
> At the libxc level this looks like a reasonable wrapping of the
> hypercall interface, so if the hypervisor folks are happy with that then
> I'm happy with this.
> 
> >  tools/libxl/libxl.h           |  18 ++++
> 
> At the libxl level things seem to be rather opaque to me, i.e. what is
> domain_data? what does it contain? does it have any more semantics than
> a 64-bit number?

The 64-bit number now holds the cache bitmask (CBM) for the domain. In
the future, the number may represent the memory bandwidth throttling for
the domain. So use a neutral name here.

> 
> What future new values might there be for libxl_psr_cat_type?

As far as I can see, besides L3_CBM, libxl_psr_cat_type may be L2_CBM or
MEM_BANDWIDTH_THROTTLING in the future.

> 
> >  tools/libxl/libxl_psr.c       | 107 ++++++++++++++++++--
> >  tools/libxl/libxl_types.idl   |   4 +
> >  tools/libxl/xl.h              |   4 +
> >  tools/libxl/xl_cmdimpl.c      | 225 ++++++++++++++++++++++++++++++++++++++++--
> >  tools/libxl/xl_cmdtable.c     |  13 +++
> 
> You've missed updating the manpage.

Yep, thanks.

> 
> >  8 files changed, 445 insertions(+), 15 deletions(-)
> > 
> 
> > +#ifdef LIBXL_HAVE_PSR_CAT
> > +int libxl_psr_cat_set_domain_data(libxl_ctx *ctx, uint32_t domid,
> > +                                  libxl_psr_cat_type type, uint32_t target,
> > +                                  uint64_t data);
> > +int libxl_psr_cat_get_domain_data(libxl_ctx *ctx, uint32_t domid,
> > +                                  libxl_psr_cat_type type, uint32_t target,
> > +                                  uint64_t *data_r);
> > +int libxl_psr_cat_get_l3_info(libxl_ctx *ctx, uint32_t socket,
> > +                              uint32_t *cos_max_r, uint32_t *cbm_len_r);
> > +#endif
> > +
> 
> 
> > [...]
> > +static uint32_t get_phy_socket_num(void)
> 
> I think this would be better named "count_physical_sockets" or
> something, otherwise I might think it takes a lock.
> 
> > +{
> > +    int rc;
> > +    uint32_t nr_sockets;
> 
>        uint32_t nr_sockets = 0;
> 
> > +    libxl_physinfo info;
> > +    libxl_physinfo_init(&info);
> > +    rc = libxl_get_physinfo(ctx, &info);
> 
> Then:
>        if (!rc)
>            nr_sockets = info.nr_cpus / info.threads_per_core / info.cores_per_socket;
>       
>        libxl_physinfo_dispose(&info);
>        return nr_sockets;
> 
> means you only have one return path to do clean up on.

Thanks, I will adopt your suggestion.

> 
> Also, does this function need to be in an ifdef, since it isn't called
> unless LIBXL_HAVE_... is set? (IOW: does this compile for ARM)

libxl_get_physinfo is for all archs, so I think it compiles for ARM. 

> 
> 
> > @@ -8057,6 +8070,202 @@ int main_psr_cmt_show(int argc, char **argv)
> >  }
> >  #endif
> >  
> > +#ifdef LIBXL_HAVE_PSR_CAT
> > +static int psr_cat_l3_cbm_set(uint32_t domid, uint32_t socket, uint64_t cbm)
> > +{
> > +    int rc;
> > +    uint32_t i, nr_sockets;
> > +
> > +    if (socket != ALL_SOCKETS) {
> > +        return libxl_psr_cat_set_domain_data(ctx, domid,
> > +                                             LIBXL_PSR_CAT_TYPE_L3_CBM,
> > +                                             socket, cbm);
> > +    } else {
> > +        nr_sockets = get_phy_socket_num();
> 
> I wonder if the libxl API ought to allow for an "all sockets" argument
> and then do all this internally instead of making the callers do it?

I can of course. But I just think of the API semantic consistence for
future, the "target" may be cpu but not socket if we support L2_CBM, so
will that value still represent all cpus? If this is not the problem,
then I will do it.

> 
> > +        if (nr_sockets == 0) {
> > +            fprintf(stderr, "Failed getting physinfo\n");
> > +            return -1;
> > +        }
> > +        for (i = 0; i < nr_sockets; i++) {
> > +            rc = libxl_psr_cat_set_domain_data(ctx, domid,
> > +                                               LIBXL_PSR_CAT_TYPE_L3_CBM,
> > +                                               i, cbm);
> > +    /* Total L3 cache size */
> > +    printf("%-46s", "Total L3 Cache Size");
> 
> How wide are these lines going to be? Can we try and keep it to <80
> columns? Perhaps you could include an example of the output of each of
> the show functions in the commit message?

As socket number is variable, it's hard to strictly ensure <80 if all
the sockets displayed in one line. Perhaps socket by socket display is
the right direction.

Chao
> 
> > +    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");
> > +

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v2 7/7] tools: add tools support for Intel CAT
  2015-03-24 11:20     ` Chao Peng
@ 2015-03-24 12:22       ` Ian Campbell
  0 siblings, 0 replies; 23+ messages in thread
From: Ian Campbell @ 2015-03-24 12:22 UTC (permalink / raw)
  To: Chao Peng
  Cc: keir, stefano.stabellini, andrew.cooper3, Ian.Jackson, xen-devel,
	will.auld, JBeulich, wei.liu2, dgdegra

On Tue, 2015-03-24 at 19:20 +0800, Chao Peng wrote:
> On Thu, Mar 19, 2015 at 12:51:19PM +0000, Ian Campbell wrote:
> > On Thu, 2015-03-19 at 18:41 +0800, Chao Peng wrote:
> > > This is the xc/xl changes to support Intel Cache Allocation
> > > Technology(CAT). Two commands are introduced:
> > > - xl psr-cat-cbm-set [-s socket] <domain> <cbm>
> > >   Set cache capacity bitmasks(CBM) for a domain.
> > > - xl psr-cat-show <domain>
> > >   Show Cache Allocation Technology information.
> > > 
> > > Signed-off-by: Chao Peng <chao.p.peng@linux.intel.com>
> > > ---
> > >  tools/libxc/include/xenctrl.h |  15 +++
> > >  tools/libxc/xc_psr.c          |  74 ++++++++++++++
> > 
> > At the libxc level this looks like a reasonable wrapping of the
> > hypercall interface, so if the hypervisor folks are happy with that then
> > I'm happy with this.
> > 
> > >  tools/libxl/libxl.h           |  18 ++++
> > 
> > At the libxl level things seem to be rather opaque to me, i.e. what is
> > domain_data? what does it contain? does it have any more semantics than
> > a 64-bit number?
> 
> The 64-bit number now holds the cache bitmask (CBM) for the domain. In
> the future, the number may represent the memory bandwidth throttling for
> the domain. So use a neutral name here.

How is the memory bandwidth throttling for a domain represented?

Is it necessary and/or desirable to funnel such distinct operations
through a single libxl API call? Why not add more semantically
meaningful APIs for each case or set of related cases?

> > What future new values might there be for libxl_psr_cat_type?
> 
> As far as I can see, besides L3_CBM, libxl_psr_cat_type may be L2_CBM or
> MEM_BANDWIDTH_THROTTLING in the future.

> > 
> > Also, does this function need to be in an ifdef, since it isn't called
> > unless LIBXL_HAVE_... is set? (IOW: does this compile for ARM)
> 
> libxl_get_physinfo is for all archs, so I think it compiles for ARM.

I was meaning hte other way around, the callers of get_phy_socket_num
are, I think, all within LIBXL_HAVE_.... So on architectures which don't
set those defines this static function will be unused, which will cause
the compiler to complain and therefore the build will fail.

> 
> > 
> > 
> > > @@ -8057,6 +8070,202 @@ int main_psr_cmt_show(int argc, char **argv)
> > >  }
> > >  #endif
> > >  
> > > +#ifdef LIBXL_HAVE_PSR_CAT
> > > +static int psr_cat_l3_cbm_set(uint32_t domid, uint32_t socket, uint64_t cbm)
> > > +{
> > > +    int rc;
> > > +    uint32_t i, nr_sockets;
> > > +
> > > +    if (socket != ALL_SOCKETS) {
> > > +        return libxl_psr_cat_set_domain_data(ctx, domid,
> > > +                                             LIBXL_PSR_CAT_TYPE_L3_CBM,
> > > +                                             socket, cbm);
> > > +    } else {
> > > +        nr_sockets = get_phy_socket_num();
> > 
> > I wonder if the libxl API ought to allow for an "all sockets" argument
> > and then do all this internally instead of making the callers do it?
> 
> I can of course. But I just think of the API semantic consistence for
> future, the "target" may be cpu but not socket if we support L2_CBM, so
> will that value still represent all cpus?

By extension having a way to say all cpus (or indeed "all whatevers")
seems likely to be useful too.

> > > +        if (nr_sockets == 0) {
> > > +            fprintf(stderr, "Failed getting physinfo\n");
> > > +            return -1;
> > > +        }
> > > +        for (i = 0; i < nr_sockets; i++) {
> > > +            rc = libxl_psr_cat_set_domain_data(ctx, domid,
> > > +                                               LIBXL_PSR_CAT_TYPE_L3_CBM,
> > > +                                               i, cbm);
> > > +    /* Total L3 cache size */
> > > +    printf("%-46s", "Total L3 Cache Size");
> > 
> > How wide are these lines going to be? Can we try and keep it to <80
> > columns? Perhaps you could include an example of the output of each of
> > the show functions in the commit message?
> 
> As socket number is variable, it's hard to strictly ensure <80 if all
> the sockets displayed in one line. Perhaps socket by socket display is
> the right direction.

Each socket is a column? I had assumed each was a row (which highlights
why examples of the output is useful!).

Ian

^ permalink raw reply	[flat|nested] 23+ messages in thread

end of thread, other threads:[~2015-03-24 12:22 UTC | newest]

Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-03-19 10:41 [PATCH v2 0/7] enable Cache Allocation Technology (CAT) for VMs Chao Peng
2015-03-19 10:41 ` [PATCH v2 1/7] x86: clean up psr boot parameter parsing Chao Peng
2015-03-20 16:47   ` Jan Beulich
2015-03-23  8:32     ` Chao Peng
2015-03-23  8:47       ` Jan Beulich
2015-03-19 10:41 ` [PATCH v2 2/7] x86: improve psr scheduling code Chao Peng
2015-03-19 10:41 ` [PATCH v2 3/7] x86: detect and initialize Intel CAT feature Chao Peng
2015-03-19 11:38   ` Tim Deegan
2015-03-19 12:44   ` Dario Faggioli
2015-03-19 13:35     ` Jan Beulich
2015-03-20 10:24       ` Dario Faggioli
2015-03-23  8:36       ` Chao Peng
2015-03-20 17:00   ` Jan Beulich
2015-03-19 10:41 ` [PATCH v2 4/7] x86: add support for COS/CBM manangement Chao Peng
2015-03-20 17:13   ` Jan Beulich
2015-03-23  8:47     ` Chao Peng
2015-03-23  9:03       ` Jan Beulich
2015-03-19 10:41 ` [PATCH v2 5/7] x86: add scheduling support for Intel CAT Chao Peng
2015-03-19 10:41 ` [PATCH v2 6/7] xsm: add CAT related xsm policies Chao Peng
2015-03-19 10:41 ` [PATCH v2 7/7] tools: add tools support for Intel CAT Chao Peng
2015-03-19 12:51   ` Ian Campbell
2015-03-24 11:20     ` Chao Peng
2015-03-24 12:22       ` Ian Campbell

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.