| Message ID | 1497402776-22348-17-git-send-email-yi.y.sun@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> Yi Sun <yi.y.sun@linux.intel.com> 06/14/17 3:26 AM >>> > @@ -279,10 +281,14 @@ static void cat_init_feature(const struct cpuid_leaf *regs, > switch ( type ) > { > case PSR_SOCKET_L3_CAT: > + case PSR_SOCKET_L2_CAT: > /* cos=0 is reserved as default cbm(all bits within cbm_len are 1). */ > feat->cos_reg_val[0] = cat_default_val(feat->cbm_len); > > - wrmsrl(MSR_IA32_PSR_L3_MASK(0), cat_default_val(feat->cbm_len)); > + if ( type == PSR_SOCKET_L3_CAT ) > + wrmsrl(MSR_IA32_PSR_L3_MASK(0), cat_default_val(feat->cbm_len)); > + else > + wrmsrl(MSR_IA32_PSR_L2_MASK(0), cat_default_val(feat->cbm_len)); Once again I think a conditional expression would yield in easier to read code, as that would make even more obvious that the second argument is the same for both cases. > @@ -317,7 +323,8 @@ static void cat_init_feature(const struct cpuid_leaf *regs, > return; > > printk(XENLOG_INFO "%s: enabled on socket %u, cos_max:%u, cbm_len:%u\n", > - ((type == PSR_SOCKET_L3_CDP) ? "CDP" : "L3 CAT"), > + ((type == PSR_SOCKET_L3_CDP) ? "CDP" : > + ((type == PSR_SOCKET_L3_CAT) ? "L3 CAT": "L2 CAT")), At this point it would probably be better to have a static const lookup array for the types, or for this descriptive string to be passed into the function. > @@ -375,6 +382,12 @@ static const struct feat_props l3_cdp_props = { > .write_msr = l3_cdp_write_msr, > }; > > +/* L2 CAT props */ > +static const struct feat_props l2_cat_props = { > + .cos_num = 1, > + .type[0] = PSR_CBM_TYPE_L2, > +}; Same remark as for CDP regarding the NULL function pointers left around here until the later patches populate them. > @@ -1407,6 +1424,19 @@ static void psr_cpu_init(void) > info->feat_init = true; > } > > + cpuid_count_leaf(PSR_CPUID_LEVEL_CAT, 0, ®s); > + if ( regs.b & PSR_RESOURCE_TYPE_L2 ) > + { > + cpuid_count_leaf(PSR_CPUID_LEVEL_CAT, 2, ®s); > + > + feat = feat_l2_cat; > + feat_l2_cat = NULL; > + feat_props[PSR_SOCKET_L2_CAT] = &l2_cat_props; > + cat_init_feature(®s, feat, info, PSR_SOCKET_L2_CAT); > + > + info->feat_init = true; This recurring setting of feat_init starts looking suspicious here. Why can't this be done once at the end of the function, outside of any if()-s? > --- a/xen/include/asm-x86/psr.h > +++ b/xen/include/asm-x86/psr.h > @@ -23,6 +23,7 @@ > > /* Resource Type Enumeration */ > #define PSR_RESOURCE_TYPE_L3 0x2 > +#define PSR_RESOURCE_TYPE_L2 0x4 These are used in psr.c only afaics, so shouldn't be put in a header. Jan
On 17-06-30 00:58:47, Jan Beulich wrote: > >>> Yi Sun <yi.y.sun@linux.intel.com> 06/14/17 3:26 AM >>> > > @@ -279,10 +281,14 @@ static void cat_init_feature(const struct cpuid_leaf *regs, > > switch ( type ) > > { > > case PSR_SOCKET_L3_CAT: > > + case PSR_SOCKET_L2_CAT: > > /* cos=0 is reserved as default cbm(all bits within cbm_len are 1). */ > > feat->cos_reg_val[0] = cat_default_val(feat->cbm_len); > > > > - wrmsrl(MSR_IA32_PSR_L3_MASK(0), cat_default_val(feat->cbm_len)); > > + if ( type == PSR_SOCKET_L3_CAT ) > > + wrmsrl(MSR_IA32_PSR_L3_MASK(0), cat_default_val(feat->cbm_len)); > > + else > > + wrmsrl(MSR_IA32_PSR_L2_MASK(0), cat_default_val(feat->cbm_len)); > > Once again I think a conditional expression would yield in easier to read code, > as that would make even more obvious that the second argument is the same for > both cases. > Ok, will use conditional expression here to make codes clearer. > > @@ -317,7 +323,8 @@ static void cat_init_feature(const struct cpuid_leaf *regs, > > return; > > > > printk(XENLOG_INFO "%s: enabled on socket %u, cos_max:%u, cbm_len:%u\n", > > - ((type == PSR_SOCKET_L3_CDP) ? "CDP" : "L3 CAT"), > > + ((type == PSR_SOCKET_L3_CDP) ? "CDP" : > > + ((type == PSR_SOCKET_L3_CAT) ? "L3 CAT": "L2 CAT")), > > At this point it would probably be better to have a static const lookup array > for the types, or for this descriptive string to be passed into the function. > Got it. > > @@ -375,6 +382,12 @@ static const struct feat_props l3_cdp_props = { > > .write_msr = l3_cdp_write_msr, > > }; > > > > +/* L2 CAT props */ > > +static const struct feat_props l2_cat_props = { > > + .cos_num = 1, > > + .type[0] = PSR_CBM_TYPE_L2, > > +}; > > Same remark as for CDP regarding the NULL function pointers left around here > until the later patches populate them. > Will highlight it in comments. > > @@ -1407,6 +1424,19 @@ static void psr_cpu_init(void) > > info->feat_init = true; > > } > > > > + cpuid_count_leaf(PSR_CPUID_LEVEL_CAT, 0, ®s); > > + if ( regs.b & PSR_RESOURCE_TYPE_L2 ) > > + { > > + cpuid_count_leaf(PSR_CPUID_LEVEL_CAT, 2, ®s); > > + > > + feat = feat_l2_cat; > > + feat_l2_cat = NULL; > > + feat_props[PSR_SOCKET_L2_CAT] = &l2_cat_props; > > + cat_init_feature(®s, feat, info, PSR_SOCKET_L2_CAT); > > + > > + info->feat_init = true; > > This recurring setting of feat_init starts looking suspicious here. Why can't > this be done once at the end of the function, outside of any if()-s? > I am afraid there is no any feature found through CPUID so I set feat_init in every statement that a feature is found. > > --- a/xen/include/asm-x86/psr.h > > +++ b/xen/include/asm-x86/psr.h > > @@ -23,6 +23,7 @@ > > > > /* Resource Type Enumeration */ > > #define PSR_RESOURCE_TYPE_L3 0x2 > > +#define PSR_RESOURCE_TYPE_L2 0x4 > > These are used in psr.c only afaics, so shouldn't be put in a header. > PSR_RESOURCE_TYPE_L3 is used in sysctl.c too. For L2, I just want to keep it same place as L3. > Jan
>>> Yi Sun <yi.y.sun@linux.intel.com> 06/30/17 9:28 AM >>> >On 17-06-30 00:58:47, Jan Beulich wrote: >> >>> Yi Sun <yi.y.sun@linux.intel.com> 06/14/17 3:26 AM >>> >> > @@ -1407,6 +1424,19 @@ static void psr_cpu_init(void) >> > info->feat_init = true; >> > } >> > >> > + cpuid_count_leaf(PSR_CPUID_LEVEL_CAT, 0, ®s); >> > + if ( regs.b & PSR_RESOURCE_TYPE_L2 ) >> > + { >> > + cpuid_count_leaf(PSR_CPUID_LEVEL_CAT, 2, ®s); >> > + >> > + feat = feat_l2_cat; >> > + feat_l2_cat = NULL; >> > + feat_props[PSR_SOCKET_L2_CAT] = &l2_cat_props; >> > + cat_init_feature(®s, feat, info, PSR_SOCKET_L2_CAT); >> > + >> > + info->feat_init = true; >> >> This recurring setting of feat_init starts looking suspicious here. Why can't >> this be done once at the end of the function, outside of any if()-s? >> >I am afraid there is no any feature found through CPUID so I set feat_init in >every statement that a feature is found. Well, even if no feature is available, you're done with initializing by the time you finish this function. >> > --- a/xen/include/asm-x86/psr.h >> > +++ b/xen/include/asm-x86/psr.h >> > @@ -23,6 +23,7 @@ >> > >> > /* Resource Type Enumeration */ >> > #define PSR_RESOURCE_TYPE_L3 0x2 >> > +#define PSR_RESOURCE_TYPE_L2 0x4 >> >> These are used in psr.c only afaics, so shouldn't be put in a header. >> >PSR_RESOURCE_TYPE_L3 is used in sysctl.c too. For L2, I just want to keep it >same place as L3. I must have overlooked that one - of course they should stay together. Jan
On 17-06-30 01:36:57, Jan Beulich wrote: > >>> Yi Sun <yi.y.sun@linux.intel.com> 06/30/17 9:28 AM >>> > >On 17-06-30 00:58:47, Jan Beulich wrote: > >> >>> Yi Sun <yi.y.sun@linux.intel.com> 06/14/17 3:26 AM >>> > >> > @@ -1407,6 +1424,19 @@ static void psr_cpu_init(void) > >> > info->feat_init = true; > >> > } > >> > > >> > + cpuid_count_leaf(PSR_CPUID_LEVEL_CAT, 0, ®s); > >> > + if ( regs.b & PSR_RESOURCE_TYPE_L2 ) > >> > + { > >> > + cpuid_count_leaf(PSR_CPUID_LEVEL_CAT, 2, ®s); > >> > + > >> > + feat = feat_l2_cat; > >> > + feat_l2_cat = NULL; > >> > + feat_props[PSR_SOCKET_L2_CAT] = &l2_cat_props; > >> > + cat_init_feature(®s, feat, info, PSR_SOCKET_L2_CAT); > >> > + > >> > + info->feat_init = true; > >> > >> This recurring setting of feat_init starts looking suspicious here. Why can't > >> this be done once at the end of the function, outside of any if()-s? > >> > >I am afraid there is no any feature found through CPUID so I set feat_init in > >every statement that a feature is found. > > Well, even if no feature is available, you're done with initializing by the time > you finish this function. > Ok, I get your point. Thanks!
diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c index 91b2122..60202b2 100644 --- a/xen/arch/x86/psr.c +++ b/xen/arch/x86/psr.c @@ -63,6 +63,7 @@ enum psr_feat_type { PSR_SOCKET_L3_CAT, PSR_SOCKET_L3_CDP, + PSR_SOCKET_L2_CAT, PSR_SOCKET_FEAT_NUM, PSR_SOCKET_FEAT_UNKNOWN, }; @@ -154,6 +155,7 @@ static DEFINE_PER_CPU(struct psr_assoc, psr_assoc); */ static struct feat_node *feat_l3_cat; static struct feat_node *feat_l3_cdp; +static struct feat_node *feat_l2_cat; /* Common functions */ #define cat_default_val(len) (0xffffffff >> (32 - (len))) @@ -279,10 +281,14 @@ static void cat_init_feature(const struct cpuid_leaf *regs, switch ( type ) { case PSR_SOCKET_L3_CAT: + case PSR_SOCKET_L2_CAT: /* cos=0 is reserved as default cbm(all bits within cbm_len are 1). */ feat->cos_reg_val[0] = cat_default_val(feat->cbm_len); - wrmsrl(MSR_IA32_PSR_L3_MASK(0), cat_default_val(feat->cbm_len)); + if ( type == PSR_SOCKET_L3_CAT ) + wrmsrl(MSR_IA32_PSR_L3_MASK(0), cat_default_val(feat->cbm_len)); + else + wrmsrl(MSR_IA32_PSR_L2_MASK(0), cat_default_val(feat->cbm_len)); break; @@ -317,7 +323,8 @@ static void cat_init_feature(const struct cpuid_leaf *regs, return; printk(XENLOG_INFO "%s: enabled on socket %u, cos_max:%u, cbm_len:%u\n", - ((type == PSR_SOCKET_L3_CDP) ? "CDP" : "L3 CAT"), + ((type == PSR_SOCKET_L3_CDP) ? "CDP" : + ((type == PSR_SOCKET_L3_CAT) ? "L3 CAT": "L2 CAT")), cpu_to_socket(smp_processor_id()), feat->cos_max, feat->cbm_len); } @@ -375,6 +382,12 @@ static const struct feat_props l3_cdp_props = { .write_msr = l3_cdp_write_msr, }; +/* L2 CAT props */ +static const struct feat_props l2_cat_props = { + .cos_num = 1, + .type[0] = PSR_CBM_TYPE_L2, +}; + static void __init parse_psr_bool(char *s, char *value, char *feature, unsigned int mask) { @@ -1357,6 +1370,10 @@ static int psr_cpu_prepare(void) (feat_l3_cdp = xzalloc(struct feat_node)) == NULL ) return -ENOMEM; + if ( feat_l2_cat == NULL && + (feat_l2_cat = xzalloc(struct feat_node)) == NULL ) + return -ENOMEM; + return 0; } @@ -1407,6 +1424,19 @@ static void psr_cpu_init(void) info->feat_init = true; } + cpuid_count_leaf(PSR_CPUID_LEVEL_CAT, 0, ®s); + if ( regs.b & PSR_RESOURCE_TYPE_L2 ) + { + cpuid_count_leaf(PSR_CPUID_LEVEL_CAT, 2, ®s); + + feat = feat_l2_cat; + feat_l2_cat = NULL; + feat_props[PSR_SOCKET_L2_CAT] = &l2_cat_props; + cat_init_feature(®s, feat, info, PSR_SOCKET_L2_CAT); + + info->feat_init = true; + } + assoc_init: psr_assoc_init(); } diff --git a/xen/include/asm-x86/msr-index.h b/xen/include/asm-x86/msr-index.h index 771e750..6c49c6d 100644 --- a/xen/include/asm-x86/msr-index.h +++ b/xen/include/asm-x86/msr-index.h @@ -345,6 +345,7 @@ #define MSR_IA32_PSR_L3_MASK(n) (0x00000c90 + (n)) #define MSR_IA32_PSR_L3_MASK_CODE(n) (0x00000c90 + (n) * 2 + 1) #define MSR_IA32_PSR_L3_MASK_DATA(n) (0x00000c90 + (n) * 2) +#define MSR_IA32_PSR_L2_MASK(n) (0x00000d10 + (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 15b9a25..276fdd6 100644 --- a/xen/include/asm-x86/psr.h +++ b/xen/include/asm-x86/psr.h @@ -23,6 +23,7 @@ /* Resource Type Enumeration */ #define PSR_RESOURCE_TYPE_L3 0x2 +#define PSR_RESOURCE_TYPE_L2 0x4 /* L3 Monitoring Features */ #define PSR_CMT_L3_OCCUPANCY 0x1 @@ -56,6 +57,7 @@ enum cbm_type { PSR_CBM_TYPE_L3, PSR_CBM_TYPE_L3_CODE, PSR_CBM_TYPE_L3_DATA, + PSR_CBM_TYPE_L2, }; extern struct psr_cmt *psr_cmt;
This patch implements the CPU init flow for L2 CAT. Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com> --- v12: - move 'type[]' assignment into l2_cat_props declaration to make it be 'const'. (suggested by Jan Beulich) - add "L2 CAT" indicator in printk. (suggested by Jan Beulich) - restore mask(0) MSR to default value. (suggested by Jan Beulich) v11: - move l2 cat 'type[]' assignement into 'psr_cpu_init'. - remove COS MSR restore action in 'cpu_init_feature'. - set 'feat_init' to true after CPU init. - modify commit message. v10: - implement L2 CAT case in 'cat_init_feature'. (suggested by Jan Beulich) - changes about 'props'. (suggested by Jan Beulich) - introduce 'PSR_CBM_TYPE_L2'. v9: - modify error handling process in 'psr_cpu_prepare' to reduce redundant codes. - reuse 'cat_init_feature' and 'cat_get_cos_max' for L2 CAT to reduce redundant codes. (suggested by Roger Pau) - remove unnecessary comment. (suggested by Jan Beulich) - move L2 CAT related codes from 'cpu_init_work' into 'psr_cpu_init'. (suggested by Jan Beulich) - do not free resource when allocation fails in 'psr_cpu_prepare'. (suggested by Jan Beulich) v7: - initialize 'l2_cat'. (suggested by Konrad Rzeszutek Wilk) v6: - use 'struct cpuid_leaf'. (suggested by Konrad Rzeszutek Wilk and Jan Beulich) v5: - remove 'feat_l2_cat' free in 'free_feature'. (suggested by Jan Beulich) - encapsulate cpuid registers into 'struct cpuid_leaf_regs'. (suggested by Jan Beulich) - print socket info when 'opt_cpu_info' is true. (suggested by Jan Beulich) - rename 'l2_cat_get_max_cos_max' to 'l2_cat_get_cos_max'. (suggested by Jan Beulich) - rename 'dat[]' to 'data[]' (suggested by Jan Beulich) - move 'cpu_prepare_work' contents into 'psr_cpu_prepare'. (suggested by Jan Beulich) v4: - create this patch because of codes architecture change. (suggested by Jan Beulich) --- --- xen/arch/x86/psr.c | 34 ++++++++++++++++++++++++++++++++-- xen/include/asm-x86/msr-index.h | 1 + xen/include/asm-x86/psr.h | 2 ++ 3 files changed, 35 insertions(+), 2 deletions(-)