[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v2 1/4] x86: Support enable CDP by boot parameter and add get CDP status
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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |