Re: [Xen-devel] [PATCH v2 1/4] x86: Support enable CDP by boot parameter and add get CDP status

2015-09-09 Thread Chao Peng 
On Wed, Sep 09, 2015 at 01:16:45PM +0800, He Chen wrote:
> Intel Code/Data Prioritization(CDP) feature is based on CAT. cdp_enabled
> is added to CAT socket info to indicate CDP is on or off on the socket,
> note that cos_max would be half when CDP is on. struct psr_cat_cbm is
> extended to support CDP operation. Extend psr_get_cat_l3_info sysctl to
> get CDP status.
> 
> Signed-off-by: He Chen 
> ---
>  xen/arch/x86/psr.c  | 84 
> ++---
>  xen/arch/x86/sysctl.c   |  5 ++-
>  xen/include/asm-x86/msr-index.h |  3 ++
>  xen/include/asm-x86/psr.h   | 11 +-
>  xen/include/public/sysctl.h |  2 +
>  5 files changed, 89 insertions(+), 16 deletions(-)
> 
> diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c
> index c0daa2e..ba0f726 100644
> --- a/xen/arch/x86/psr.c
> +++ b/xen/arch/x86/psr.c
> @@ -17,13 +17,21 @@
>  #include 
>  #include 
>  #include 
> +#include 

Is this still needed?

>  #include 
>  
>  #define PSR_CMT(1<<0)
>  #define PSR_CAT(1<<1)
> +#define PSR_CDP(1<<2)
>  
>  struct psr_cat_cbm {
> -uint64_t cbm;
> +union {
> +uint64_t cbm;
> +struct {
> +uint64_t code;
> +uint64_t data;
> +} cdp;
> +} u;
>  unsigned int ref;
>  };
>  
> @@ -32,6 +40,7 @@ struct psr_cat_socket_info {
>  unsigned int cos_max;
>  struct psr_cat_cbm *cos_to_cbm;
>  spinlock_t cbm_lock;

As you defined the cdp stauts for each socket here ...
> +bool_t cdp_enabled;
>  };
>  
>  struct psr_assoc {
> @@ -43,6 +52,7 @@ struct psr_cmt *__read_mostly psr_cmt;
>  
>  static unsigned long *__read_mostly cat_socket_enable;
>  static struct psr_cat_socket_info *__read_mostly cat_socket_info;
> +static unsigned long *__read_mostly cdp_socket_enable;

... this one then perhaps is redundency. If only one is need then I really
like to keep the latter one.

> @@ -470,6 +500,7 @@ static void cat_cpu_init(void)
>  struct psr_cat_socket_info *info;
>  unsigned int socket;
>  unsigned int cpu = smp_processor_id();
> +uint64_t val;
>  const struct cpuinfo_x86 *c = cpu_data + cpu;
>  
>  if ( !cpu_has(c, X86_FEATURE_CAT) || c->cpuid_level < 
> PSR_CPUID_LEVEL_CAT )
> @@ -490,13 +521,34 @@ static void cat_cpu_init(void)
>  info->cos_to_cbm = temp_cos_to_cbm;
>  temp_cos_to_cbm = NULL;
>  /* cos=0 is reserved as default cbm(all ones). */
> -info->cos_to_cbm[0].cbm = (1ull << info->cbm_len) - 1;
> +info->cos_to_cbm[0].u.cbm = (1ull << info->cbm_len) - 1;
>  
>  spin_lock_init(>cbm_lock);
>  
>  set_bit(socket, cat_socket_enable);
>  printk(XENLOG_INFO "CAT: enabled on socket %u, cos_max:%u, 
> cbm_len:%u\n",
> socket, info->cos_max, info->cbm_len);

If CDP is enable below, then this information is quite misleading.

> +
> +if ( (ecx & PSR_CAT_CDP_CAPABILITY) && (opt_psr & PSR_CDP) )
> +{
> +if ( test_bit(socket, cdp_socket_enable) )
> +return;
> +
> +rdmsrl(MSR_IA32_PSR_L3_QOS_CFG, val);
> +wrmsrl(MSR_IA32_PSR_L3_QOS_CFG, val | 1 << 
> PSR_L3_QOS_CDP_ENABLE_BIT);
> +
> +/* No need to write register since CBMs are fully open as 
> default by HW */
> +info->cos_to_cbm[0].u.cdp.code = (1ull << info->cbm_len) - 1;
> +info->cos_to_cbm[0].u.cdp.data = (1ull << info->cbm_len) - 1;

If I remember correctly, HW is supposed to boot as CAT mode by default and
only IA32_L3_QOS_Mask_0 is all ones, other mask msrs however are not mentioned
in the spec which then may contain arbitrary value. So I guesss as
least you need write all ones to IA32_L3_QOS_Mask_1 explicitly and that
should be done before you enabled CDP.

> +
> +/* Cut half of cos_max when CDP enabled */
> +info->cos_max = info->cos_max / 2;
> +
> +info->cdp_enabled = 1;
> +set_bit(socket, cdp_socket_enable);
> +printk(XENLOG_INFO "CDP: enabled on socket %u, cos_max:%u, 
> cbm_len:%u\n",
> +   socket, info->cos_max, info->cbm_len);
> +}
>  }
>  }
>  
> diff --git a/xen/include/asm-x86/msr-index.h b/xen/include/asm-x86/msr-index.h
> index e9c4723..402e7a7 100644
> --- a/xen/include/asm-x86/msr-index.h
> +++ b/xen/include/asm-x86/msr-index.h
> @@ -328,7 +328,10 @@
>  #define MSR_IA32_CMT_EVTSEL  0x0c8d
>  #define MSR_IA32_CMT_CTR 0x0c8e
>  #define MSR_IA32_PSR_ASSOC   0x0c8f
> +#define MSR_IA32_PSR_L3_QOS_CFG  0x0c81
>  #define MSR_IA32_PSR_L3_MASK(n)  (0x0c90 + (n))
> +#define MSR_IA32_PSR_L3_MASK_CBM_CODE(n) (0x0c90 + (n * 2 + 1))
> +#define MSR_IA32_PSR_L3_MASK_CBM_DATA(n) (0x0c90 + (n * 2))

I guess 'CBM' can be removed from these two macros, e.g.
MSR_IA32_PSR_L3_MASK_CODE & MSR_IA32_PSR_L3_MASK_DATA

Chao

Re: [Xen-devel] [PATCH v2 1/4] x86: Support enable CDP by boot parameter and add get CDP status

2015-09-09 Thread He Chen 
On Wed, Sep 09, 2015 at 03:04:35PM +0800, Chao Peng wrote:
> On Wed, Sep 09, 2015 at 01:16:45PM +0800, He Chen wrote:
> > Intel Code/Data Prioritization(CDP) feature is based on CAT. cdp_enabled
> > is added to CAT socket info to indicate CDP is on or off on the socket,
> > note that cos_max would be half when CDP is on. struct psr_cat_cbm is
> > extended to support CDP operation. Extend psr_get_cat_l3_info sysctl to
> > get CDP status.
> > 
> > Signed-off-by: He Chen 
> > ---
> >  xen/arch/x86/psr.c  | 84 
> > ++---
> >  xen/arch/x86/sysctl.c   |  5 ++-
> >  xen/include/asm-x86/msr-index.h |  3 ++
> >  xen/include/asm-x86/psr.h   | 11 +-
> >  xen/include/public/sysctl.h |  2 +
> >  5 files changed, 89 insertions(+), 16 deletions(-)
> > 
> > diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c
> > index c0daa2e..ba0f726 100644
> > --- a/xen/arch/x86/psr.c
> > +++ b/xen/arch/x86/psr.c
> > @@ -17,13 +17,21 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> 
> Is this still needed?
> 

Obviously not, my mistake.

> >  #include 
> >  
> >  #define PSR_CMT(1<<0)
> >  #define PSR_CAT(1<<1)
> > +#define PSR_CDP(1<<2)
> >  
> >  struct psr_cat_cbm {
> > -uint64_t cbm;
> > +union {
> > +uint64_t cbm;
> > +struct {
> > +uint64_t code;
> > +uint64_t data;
> > +} cdp;
> > +} u;
> >  unsigned int ref;
> >  };
> >  
> > @@ -32,6 +40,7 @@ struct psr_cat_socket_info {
> >  unsigned int cos_max;
> >  struct psr_cat_cbm *cos_to_cbm;
> >  spinlock_t cbm_lock;
> 
> As you defined the cdp stauts for each socket here ...
> > +bool_t cdp_enabled;
> >  };
> >  
> >  struct psr_assoc {
> > @@ -43,6 +52,7 @@ struct psr_cmt *__read_mostly psr_cmt;
> >  
> >  static unsigned long *__read_mostly cat_socket_enable;
> >  static struct psr_cat_socket_info *__read_mostly cat_socket_info;
> > +static unsigned long *__read_mostly cdp_socket_enable;
> 
> ... this one then perhaps is redundency. If only one is need then I really
> like to keep the latter one.
> 

You're right. I would refine this.

> > @@ -470,6 +500,7 @@ static void cat_cpu_init(void)
> >  struct psr_cat_socket_info *info;
> >  unsigned int socket;
> >  unsigned int cpu = smp_processor_id();
> > +uint64_t val;
> >  const struct cpuinfo_x86 *c = cpu_data + cpu;
> >  
> >  if ( !cpu_has(c, X86_FEATURE_CAT) || c->cpuid_level < 
> > PSR_CPUID_LEVEL_CAT )
> > @@ -490,13 +521,34 @@ static void cat_cpu_init(void)
> >  info->cos_to_cbm = temp_cos_to_cbm;
> >  temp_cos_to_cbm = NULL;
> >  /* cos=0 is reserved as default cbm(all ones). */
> > -info->cos_to_cbm[0].cbm = (1ull << info->cbm_len) - 1;
> > +info->cos_to_cbm[0].u.cbm = (1ull << info->cbm_len) - 1;
> >  
> >  spin_lock_init(>cbm_lock);
> >  
> >  set_bit(socket, cat_socket_enable);
> >  printk(XENLOG_INFO "CAT: enabled on socket %u, cos_max:%u, 
> > cbm_len:%u\n",
> > socket, info->cos_max, info->cbm_len);
> 
> If CDP is enable below, then this information is quite misleading.
> 

Is it proper to remove CAT printk info?

> > +
> > +if ( (ecx & PSR_CAT_CDP_CAPABILITY) && (opt_psr & PSR_CDP) )
> > +{
> > +if ( test_bit(socket, cdp_socket_enable) )
> > +return;
> > +
> > +rdmsrl(MSR_IA32_PSR_L3_QOS_CFG, val);
> > +wrmsrl(MSR_IA32_PSR_L3_QOS_CFG, val | 1 << 
> > PSR_L3_QOS_CDP_ENABLE_BIT);
> > +
> > +/* No need to write register since CBMs are fully open as 
> > default by HW */
> > +info->cos_to_cbm[0].u.cdp.code = (1ull << info->cbm_len) - 1;
> > +info->cos_to_cbm[0].u.cdp.data = (1ull << info->cbm_len) - 1;
> 
> If I remember correctly, HW is supposed to boot as CAT mode by default and
> only IA32_L3_QOS_Mask_0 is all ones, other mask msrs however are not mentioned
> in the spec which then may contain arbitrary value. So I guesss as
> least you need write all ones to IA32_L3_QOS_Mask_1 explicitly and that
> should be done before you enabled CDP.
> 

I have tested it at Broadwell server. I mean, I have used rdmsr to get mask0
and mask1 during booting and then check them, I found that they all ones by
default.
But since spec doesn't mention it as you said, I would write all ones to mask1
for better compatibility.

> > +
> > +/* Cut half of cos_max when CDP enabled */
> > +info->cos_max = info->cos_max / 2;
> > +
> > +info->cdp_enabled = 1;
> > +set_bit(socket, cdp_socket_enable);
> > +printk(XENLOG_INFO "CDP: enabled on socket %u, cos_max:%u, 
> > cbm_len:%u\n",
> > +   socket, info->cos_max, info->cbm_len);
> > +}
> >  }
> >  }
> >  
> > diff --git a/xen/include/asm-x86/msr-index.h 
> > b/xen/include/asm-x86/msr-index.h
> >

[Xen-devel] [PATCH v2 1/4] x86: Support enable CDP by boot parameter and add get CDP status

2015-09-08 Thread He Chen 
Intel Code/Data Prioritization(CDP) feature is based on CAT. cdp_enabled
is added to CAT socket info to indicate CDP is on or off on the socket,
note that cos_max would be half when CDP is on. struct psr_cat_cbm is
extended to support CDP operation. Extend psr_get_cat_l3_info sysctl to
get CDP status.

Signed-off-by: He Chen 
---
 xen/arch/x86/psr.c  | 84 ++---
 xen/arch/x86/sysctl.c   |  5 ++-
 xen/include/asm-x86/msr-index.h |  3 ++
 xen/include/asm-x86/psr.h   | 11 +-
 xen/include/public/sysctl.h |  2 +
 5 files changed, 89 insertions(+), 16 deletions(-)

diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c
index c0daa2e..ba0f726 100644
--- a/xen/arch/x86/psr.c
+++ b/xen/arch/x86/psr.c
@@ -17,13 +17,21 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 
 #define PSR_CMT(1<<0)
 #define PSR_CAT(1<<1)
+#define PSR_CDP(1<<2)
 
 struct psr_cat_cbm {
-uint64_t cbm;
+union {
+uint64_t cbm;
+struct {
+uint64_t code;
+uint64_t data;
+} cdp;
+} u;
 unsigned int ref;
 };
 
@@ -32,6 +40,7 @@ struct psr_cat_socket_info {
 unsigned int cos_max;
 struct psr_cat_cbm *cos_to_cbm;
 spinlock_t cbm_lock;
+bool_t cdp_enabled;
 };
 
 struct psr_assoc {
@@ -43,6 +52,7 @@ struct psr_cmt *__read_mostly psr_cmt;
 
 static unsigned long *__read_mostly cat_socket_enable;
 static struct psr_cat_socket_info *__read_mostly cat_socket_info;
+static unsigned long *__read_mostly cdp_socket_enable;
 
 static unsigned int __initdata opt_psr;
 static unsigned int __initdata opt_rmid_max = 255;
@@ -94,6 +104,7 @@ static void __init parse_psr_param(char *s)
 
 parse_psr_bool(s, val_str, "cmt", PSR_CMT);
 parse_psr_bool(s, val_str, "cat", PSR_CAT);
+parse_psr_bool(s, val_str, "cdp", PSR_CDP);
 
 if ( val_str && !strcmp(s, "rmid_max") )
 opt_rmid_max = simple_strtoul(val_str, NULL, 0);
@@ -262,7 +273,7 @@ static struct psr_cat_socket_info 
*get_cat_socket_info(unsigned int socket)
 }
 
 int psr_get_cat_l3_info(unsigned int socket, uint32_t *cbm_len,
-uint32_t *cos_max)
+uint32_t *cos_max, uint32_t *flags)
 {
 struct psr_cat_socket_info *info = get_cat_socket_info(socket);
 
@@ -272,6 +283,10 @@ int psr_get_cat_l3_info(unsigned int socket, uint32_t 
*cbm_len,
 *cbm_len = info->cbm_len;
 *cos_max = info->cos_max;
 
+*flags = 0;
+if ( info->cdp_enabled )
+*flags |= PSR_CAT_FLAG_L3_CDP;
+
 return 0;
 }
 
@@ -282,7 +297,7 @@ int psr_get_l3_cbm(struct domain *d, unsigned int socket, 
uint64_t *cbm)
 if ( IS_ERR(info) )
 return PTR_ERR(info);
 
-*cbm = info->cos_to_cbm[d->arch.psr_cos_ids[socket]].cbm;
+*cbm = info->cos_to_cbm[d->arch.psr_cos_ids[socket]].u.cbm;
 
 return 0;
 }
@@ -313,19 +328,34 @@ static bool_t psr_check_cbm(unsigned int cbm_len, 
uint64_t cbm)
 struct cos_cbm_info
 {
 unsigned int cos;
-uint64_t cbm;
+uint64_t cbm_code;
+uint64_t cbm_data;
+bool_t cdp;
 };
 
 static void do_write_l3_cbm(void *data)
 {
 struct cos_cbm_info *info = data;
 
-wrmsrl(MSR_IA32_PSR_L3_MASK(info->cos), info->cbm);
+if ( info->cdp )
+{
+wrmsrl(MSR_IA32_PSR_L3_MASK_CBM_CODE(info->cos), info->cbm_code);
+wrmsrl(MSR_IA32_PSR_L3_MASK_CBM_DATA(info->cos), info->cbm_data);
+}
+else
+wrmsrl(MSR_IA32_PSR_L3_MASK(info->cos), info->cbm_code);
 }
 
-static int write_l3_cbm(unsigned int socket, unsigned int cos, uint64_t cbm)
+static int write_l3_cbm(unsigned int socket, unsigned int cos,
+uint64_t cbm_code, uint64_t cbm_data, bool_t cdp)
 {
-struct cos_cbm_info info = { .cos = cos, .cbm = cbm };
+struct cos_cbm_info info =
+{
+  .cos = cos,
+  .cbm_code = cbm_code,
+  .cbm_data = cbm_data,
+  .cdp = cdp,
+};
 
 if ( socket == cpu_to_socket(smp_processor_id()) )
 do_write_l3_cbm();
@@ -363,7 +393,7 @@ int psr_set_l3_cbm(struct domain *d, unsigned int socket, 
uint64_t cbm)
 /* If still not found, then keep unused one. */
 if ( !found && cos != 0 && map[cos].ref == 0 )
 found = map + cos;
-else if ( map[cos].cbm == cbm )
+else if ( map[cos].u.cbm == cbm )
 {
 if ( unlikely(cos == old_cos) )
 {
@@ -387,16 +417,16 @@ int psr_set_l3_cbm(struct domain *d, unsigned int socket, 
uint64_t cbm)
 }
 
 cos = found - map;
-if ( found->cbm != cbm )
+if ( found->u.cbm != cbm )
 {
-int ret = write_l3_cbm(socket, cos, cbm);
+int ret = write_l3_cbm(socket, cos, cbm, 0, 0);
 
 if ( ret )
 {
 spin_unlock(>cbm_lock);
 return ret;
 }
-found->cbm = cbm;
+found->u.cbm = cbm;
 }
 
 found->ref++;
@@ -470,6 +500,7 @@ static void