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

[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

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