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

[He Chen <he.chen@xxxxxxxxxxxxxxx>]

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 <he.chen@xxxxxxxxxxxxxxx>
> > ---
> >  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 <xen/cpu.h>
> >  #include <xen/err.h>
> >  #include <xen/sched.h>
> > +#include <xen/domain.h>
> 
> Is this still needed?
> 

Obviously not, my mistake.

> >  #include <asm/psr.h>
> >  
> >  #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(&info->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
> > 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                0x00000c8d
> >  #define MSR_IA32_CMT_CTR           0x00000c8e
> >  #define MSR_IA32_PSR_ASSOC         0x00000c8f
> > +#define MSR_IA32_PSR_L3_QOS_CFG    0x00000c81
> >  #define MSR_IA32_PSR_L3_MASK(n)    (0x00000c90 + (n))
> > +#define MSR_IA32_PSR_L3_MASK_CBM_CODE(n)   (0x00000c90 + (n * 2 + 1))
> > +#define MSR_IA32_PSR_L3_MASK_CBM_DATA(n)   (0x00000c90 + (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

Sure and thanks.

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