| Message ID | 1497402776-22348-14-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 >>> > @@ -253,6 +271,26 @@ static void cat_init_feature(const struct cpuid_leaf *regs, > > break; > > + case PSR_SOCKET_L3_CDP: > + { > + uint64_t val; > + > + /* Cut half of cos_max when CDP is enabled. */ > + feat->cos_max >>= 1; I'm afraid this is off by one in the unusual but possible case of cos_max being an even number. > + wrmsrl(MSR_IA32_PSR_L3_MASK(0), cat_default_val(feat->cbm_len)); > + wrmsrl(MSR_IA32_PSR_L3_MASK(1), cat_default_val(feat->cbm_len)); > + rdmsrl(MSR_IA32_PSR_L3_QOS_CFG, val); > + wrmsrl(MSR_IA32_PSR_L3_QOS_CFG, > + val | (1ull << PSR_L3_QOS_CDP_ENABLE_BIT)); > + > + /* cos=0 is reserved as default cbm(all bits within cbm_len are 1). */ Along the lines of a comment to an earlier patch, please add a blank ahead of the opeing paren. > + get_cdp_code(feat, 0) = cat_default_val(feat->cbm_len); > + get_cdp_data(feat, 0) = cat_default_val(feat->cbm_len); Wouldn't you better do this prior to enabling CDP? > @@ -1294,11 +1344,21 @@ static void psr_cpu_init(void) > { > cpuid_count_leaf(PSR_CPUID_LEVEL_CAT, 1, ®s); > > - feat = feat_l3_cat; > - feat_l3_cat = NULL; > - feat_props[PSR_SOCKET_L3_CAT] = &l3_cat_props; > - > - cat_init_feature(®s, feat, info, PSR_SOCKET_L3_CAT); > + if ( (regs.c & PSR_CAT_CDP_CAPABILITY) && (opt_psr & PSR_CDP) && > + !info->features[PSR_SOCKET_L3_CDP] ) Doesn't this last check mean you'd set up CAT in case would come here for the 2nd CPU on a socket? In the end the check is simplky pointless afaict, due to psr_cpu_init() calling cat_init_feature() only if ->feat_init is still false. But please remove it as being potentially confusing (and inconsistent with the else branch). Jan
On 17-06-30 00:40:35, Jan Beulich wrote: > >>> Yi Sun <yi.y.sun@linux.intel.com> 06/14/17 3:26 AM >>> > > @@ -253,6 +271,26 @@ static void cat_init_feature(const struct cpuid_leaf *regs, > > > > break; > > > > + case PSR_SOCKET_L3_CDP: > > + { > > + uint64_t val; > > + > > + /* Cut half of cos_max when CDP is enabled. */ > > + feat->cos_max >>= 1; > > I'm afraid this is off by one in the unusual but possible case of cos_max > being an even number. > This accords to spec: "For CDP operations, COS_MAX_CDP is equal to (CPUID.(EAX=10H, ECX=1):EDX.COS_MAX_CAT >>1)." HW should make sure it is even number. > > + wrmsrl(MSR_IA32_PSR_L3_MASK(0), cat_default_val(feat->cbm_len)); > > + wrmsrl(MSR_IA32_PSR_L3_MASK(1), cat_default_val(feat->cbm_len)); > > + rdmsrl(MSR_IA32_PSR_L3_QOS_CFG, val); > > + wrmsrl(MSR_IA32_PSR_L3_QOS_CFG, > > + val | (1ull << PSR_L3_QOS_CDP_ENABLE_BIT)); > > + > > + /* cos=0 is reserved as default cbm(all bits within cbm_len are 1). */ > > Along the lines of a comment to an earlier patch, please add a blank ahead of > the opeing paren. > Got it. > > + get_cdp_code(feat, 0) = cat_default_val(feat->cbm_len); > > + get_cdp_data(feat, 0) = cat_default_val(feat->cbm_len); > > Wouldn't you better do this prior to enabling CDP? > Sure. > > @@ -1294,11 +1344,21 @@ static void psr_cpu_init(void) > > { > > cpuid_count_leaf(PSR_CPUID_LEVEL_CAT, 1, ®s); > > > > - feat = feat_l3_cat; > > - feat_l3_cat = NULL; > > - feat_props[PSR_SOCKET_L3_CAT] = &l3_cat_props; > > - > > - cat_init_feature(®s, feat, info, PSR_SOCKET_L3_CAT); > > + if ( (regs.c & PSR_CAT_CDP_CAPABILITY) && (opt_psr & PSR_CDP) && > > + !info->features[PSR_SOCKET_L3_CDP] ) > > Doesn't this last check mean you'd set up CAT in case would come here for > the 2nd CPU on a socket? In the end the check is simplky pointless afaict, > due to psr_cpu_init() calling cat_init_feature() only if ->feat_init is still > false. But please remove it as being potentially confusing (and inconsistent > with the else branch). > Ok, will remove the last check here. > Jan
>>> Yi Sun <yi.y.sun@linux.intel.com> 06/30/17 9:01 AM >>> >On 17-06-30 00:40:35, Jan Beulich wrote: >> >>> Yi Sun <yi.y.sun@linux.intel.com> 06/14/17 3:26 AM >>> >> > @@ -253,6 +271,26 @@ static void cat_init_feature(const struct cpuid_leaf *regs, >> > >> > break; >> > >> > + case PSR_SOCKET_L3_CDP: >> > + { >> > + uint64_t val; >> > + >> > + /* Cut half of cos_max when CDP is enabled. */ >> > + feat->cos_max >>= 1; >> >> I'm afraid this is off by one in the unusual but possible case of cos_max >> being an even number. >> >This accords to spec: >"For CDP operations, COS_MAX_CDP is equal to (CPUID.(EAX=10H, ECX=1):EDX.COS_MAX_CAT >>1)." > >HW should make sure it is even number. And how about someone using the command line option to shrink the to be used set? Jan
On 17-06-30 01:33:02, Jan Beulich wrote: > >>> Yi Sun <yi.y.sun@linux.intel.com> 06/30/17 9:01 AM >>> > >On 17-06-30 00:40:35, Jan Beulich wrote: > >> >>> Yi Sun <yi.y.sun@linux.intel.com> 06/14/17 3:26 AM >>> > >> > @@ -253,6 +271,26 @@ static void cat_init_feature(const struct cpuid_leaf *regs, > >> > > >> > break; > >> > > >> > + case PSR_SOCKET_L3_CDP: > >> > + { > >> > + uint64_t val; > >> > + > >> > + /* Cut half of cos_max when CDP is enabled. */ > >> > + feat->cos_max >>= 1; > >> > >> I'm afraid this is off by one in the unusual but possible case of cos_max > >> being an even number. > >> > >This accords to spec: > >"For CDP operations, COS_MAX_CDP is equal to (CPUID.(EAX=10H, ECX=1):EDX.COS_MAX_CAT >>1)." > > > >HW should make sure it is even number. > > And how about someone using the command line option to shrink the to be used set? > Good question. The command line option saved in 'opt_cos_max', even it is not even number and less than 'EDX.COS_MAX_CAT' so that the 'cos_max' here is same as it, the right shift operation does not cause any issue I think. The description in docs/misc/xen-command-line.markdown is clear that "the cos_max in use will automatically reduce to half when CDP is enabled". E.g. 'opt_cos_max' is 5, then we get 2 for CDP. I think user should be aware of this by reading the markdown file. > Jan
>>> Yi Sun <yi.y.sun@linux.intel.com> 06/30/17 10:05 AM >>> >On 17-06-30 01:33:02, Jan Beulich wrote: >> >>> Yi Sun <yi.y.sun@linux.intel.com> 06/30/17 9:01 AM >>> >> >This accords to spec: >> >"For CDP operations, COS_MAX_CDP is equal to (CPUID.(EAX=10H, ECX=1):EDX.COS_MAX_CAT >>1)." >> > >> >HW should make sure it is even number. >> >> And how about someone using the command line option to shrink the to be used set? >> >Good question. The command line option saved in 'opt_cos_max', even it is >not even number and less than 'EDX.COS_MAX_CAT' so that the 'cos_max' here is >same as it, the right shift operation does not cause any issue I think. > >The description in docs/misc/xen-command-line.markdown is clear that "the cos_max >in use will automatically reduce to half when CDP is enabled". E.g. 'opt_cos_max' >is 5, then we get 2 for CDP. I think user should be aware of this by reading the >markdown file. I don't really follow all this argumentation - what's the problem of simply subtracting 1 before doing the shift? Talking of which - isn't the general cos_max > 1 also insufficient for the CDP case? Jan
On 17-06-30 03:18:53, Jan Beulich wrote: > >>> Yi Sun <yi.y.sun@linux.intel.com> 06/30/17 10:05 AM >>> > >On 17-06-30 01:33:02, Jan Beulich wrote: > >> >>> Yi Sun <yi.y.sun@linux.intel.com> 06/30/17 9:01 AM >>> > >> >This accords to spec: > >> >"For CDP operations, COS_MAX_CDP is equal to (CPUID.(EAX=10H, ECX=1):EDX.COS_MAX_CAT >>1)." > >> > > >> >HW should make sure it is even number. > >> > >> And how about someone using the command line option to shrink the to be used set? > >> > >Good question. The command line option saved in 'opt_cos_max', even it is > >not even number and less than 'EDX.COS_MAX_CAT' so that the 'cos_max' here is > >same as it, the right shift operation does not cause any issue I think. > > > >The description in docs/misc/xen-command-line.markdown is clear that "the cos_max > >in use will automatically reduce to half when CDP is enabled". E.g. 'opt_cos_max' > >is 5, then we get 2 for CDP. I think user should be aware of this by reading the > >markdown file. > > I don't really follow all this argumentation - what's the problem of simply subtracting > 1 before doing the shift? Talking of which - isn't the general cos_max > 1 also There will be problem if we substract 1 before doing the shift for the even number. E.g. the original cos_max is 2, (2-1)>>1 will be 0. That is not we want. For the odd number, direct right shift is good too. E.g. the original cos_max is 3, 3 >> 1 is 1 which is what we want. > insufficient for the CDP case? We do not need check if cos_max > 1 because the 'cos_max = 0' works for us. That means only COS ID 0 can be used. You can see, we use "cos <= cos_max" to find COS ID. Of course, the ID 0 saves the default value which cannot be overwritten. > > Jan
>>> On 04.07.17 at 03:40, <yi.y.sun@linux.intel.com> wrote: > On 17-06-30 03:18:53, Jan Beulich wrote: >> >>> Yi Sun <yi.y.sun@linux.intel.com> 06/30/17 10:05 AM >>> >> >On 17-06-30 01:33:02, Jan Beulich wrote: >> >> >>> Yi Sun <yi.y.sun@linux.intel.com> 06/30/17 9:01 AM >>> >> >> >This accords to spec: >> >> >"For CDP operations, COS_MAX_CDP is equal to (CPUID.(EAX=10H, ECX=1):EDX.COS_MAX_CAT >>1)." >> >> > >> >> >HW should make sure it is even number. >> >> >> >> And how about someone using the command line option to shrink the to be used set? >> >> >> >Good question. The command line option saved in 'opt_cos_max', even it is >> >not even number and less than 'EDX.COS_MAX_CAT' so that the 'cos_max' here is >> >same as it, the right shift operation does not cause any issue I think. >> > >> >The description in docs/misc/xen-command-line.markdown is clear that "the cos_max >> >in use will automatically reduce to half when CDP is enabled". E.g. 'opt_cos_max' >> >is 5, then we get 2 for CDP. I think user should be aware of this by reading the >> >markdown file. >> >> I don't really follow all this argumentation - what's the problem of simply subtracting >> 1 before doing the shift? Talking of which - isn't the general cos_max > 1 also > > There will be problem if we substract 1 before doing the shift for the even > number. E.g. the original cos_max is 2, (2-1)>>1 will be 0. That is not we > want. For the odd number, direct right shift is good too. E.g. the original > cos_max is 3, 3 >> 1 is 1 which is what we want. What we want is raw CDP 0 unusable 1 0 2 0 3 1 4 1 5 2 6 2 Other than what you say, for original cos_max = 2 we indeed need to convert it to 0, or else CDP would use indexes 2 ( = 1 * 2 + 0) and 3 ( = 1 * 2 + 1), exceeding the mandated upper bound. >> insufficient for the CDP case? > > We do not need check if cos_max > 1 because the 'cos_max = 0' works for us. > That means only COS ID 0 can be used. You can see, we use "cos <= cos_max" to > find COS ID. Of course, the ID 0 saves the default value which cannot be > overwritten. Please see the (current) check in init_psr_cat(), which your series moves into init_psr(). Logically in the CDP case we should require cos_max >= 3 to have at least one usable COS ID. Jan
On 17-07-04 01:28:32, Jan Beulich wrote: > >>> On 04.07.17 at 03:40, <yi.y.sun@linux.intel.com> wrote: > > On 17-06-30 03:18:53, Jan Beulich wrote: > >> >>> Yi Sun <yi.y.sun@linux.intel.com> 06/30/17 10:05 AM >>> > >> >On 17-06-30 01:33:02, Jan Beulich wrote: > >> >> >>> Yi Sun <yi.y.sun@linux.intel.com> 06/30/17 9:01 AM >>> > > There will be problem if we substract 1 before doing the shift for the even > > number. E.g. the original cos_max is 2, (2-1)>>1 will be 0. That is not we > > want. For the odd number, direct right shift is good too. E.g. the original > > cos_max is 3, 3 >> 1 is 1 which is what we want. > > What we want is > > raw CDP > 0 unusable > 1 0 > 2 0 > 3 1 > 4 1 > 5 2 > 6 2 > > Other than what you say, for original cos_max = 2 we indeed need > to convert it to 0, or else CDP would use indexes 2 ( = 1 * 2 + 0) > and 3 ( = 1 * 2 + 1), exceeding the mandated upper bound. > > >> insufficient for the CDP case? > > > > We do not need check if cos_max > 1 because the 'cos_max = 0' works for us. > > That means only COS ID 0 can be used. You can see, we use "cos <= cos_max" to > > find COS ID. Of course, the ID 0 saves the default value which cannot be > > overwritten. > > Please see the (current) check in init_psr_cat(), which your series > moves into init_psr(). Logically in the CDP case we should require > cos_max >= 3 to have at least one usable COS ID. > > Jan Thank you! You are right. I will modify codes to do so. BRs, Sun Yi
diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c index 81d9a78..814f0e1 100644 --- a/xen/arch/x86/psr.c +++ b/xen/arch/x86/psr.c @@ -62,6 +62,7 @@ enum psr_feat_type { PSR_SOCKET_L3_CAT, + PSR_SOCKET_L3_CDP, PSR_SOCKET_FEAT_NUM, PSR_SOCKET_FEAT_UNKNOWN, }; @@ -152,11 +153,28 @@ static DEFINE_PER_CPU(struct psr_assoc, psr_assoc); * array creation. It is used to transiently store a spare node. */ static struct feat_node *feat_l3_cat; +static struct feat_node *feat_l3_cdp; /* Common functions */ #define cat_default_val(len) (0xffffffff >> (32 - (len))) /* + * get_cdp_data - get DATA COS register value from input COS ID. + * @feat: the feature node. + * @cos: the COS ID. + */ +#define get_cdp_data(feat, cos) \ + ((feat)->cos_reg_val[(cos) * 2]) + +/* + * get_cdp_code - get CODE COS register value from input COS ID. + * @feat: the feature node. + * @cos: the COS ID. + */ +#define get_cdp_code(feat, cos) \ + ((feat)->cos_reg_val[(cos) * 2 + 1]) + +/* * Use this function to check if any allocation feature has been enabled * in cmdline. */ @@ -253,6 +271,26 @@ static void cat_init_feature(const struct cpuid_leaf *regs, break; + case PSR_SOCKET_L3_CDP: + { + uint64_t val; + + /* Cut half of cos_max when CDP is enabled. */ + feat->cos_max >>= 1; + + wrmsrl(MSR_IA32_PSR_L3_MASK(0), cat_default_val(feat->cbm_len)); + wrmsrl(MSR_IA32_PSR_L3_MASK(1), cat_default_val(feat->cbm_len)); + rdmsrl(MSR_IA32_PSR_L3_QOS_CFG, val); + wrmsrl(MSR_IA32_PSR_L3_QOS_CFG, + val | (1ull << PSR_L3_QOS_CDP_ENABLE_BIT)); + + /* cos=0 is reserved as default cbm(all bits within cbm_len are 1). */ + get_cdp_code(feat, 0) = cat_default_val(feat->cbm_len); + get_cdp_data(feat, 0) = cat_default_val(feat->cbm_len); + + break; + } + default: return; } @@ -263,7 +301,8 @@ static void cat_init_feature(const struct cpuid_leaf *regs, if ( !opt_cpu_info ) return; - printk(XENLOG_INFO "CAT: enabled on socket %u, cos_max:%u, cbm_len:%u\n", + printk(XENLOG_INFO "%s: enabled on socket %u, cos_max:%u, cbm_len:%u\n", + ((type == PSR_SOCKET_L3_CDP) ? "CDP" : "L3 CAT"), cpu_to_socket(smp_processor_id()), feat->cos_max, feat->cbm_len); } @@ -293,6 +332,13 @@ static const struct feat_props l3_cat_props = { .write_msr = l3_cat_write_msr, }; +/* L3 CDP props */ +static const struct feat_props l3_cdp_props = { + .cos_num = 2, + .type[0] = PSR_CBM_TYPE_L3_DATA, + .type[1] = PSR_CBM_TYPE_L3_CODE, +}; + static void __init parse_psr_bool(char *s, char *value, char *feature, unsigned int mask) { @@ -1263,6 +1309,10 @@ static int psr_cpu_prepare(void) (feat_l3_cat = xzalloc(struct feat_node)) == NULL ) return -ENOMEM; + if ( feat_l3_cdp == NULL && + (feat_l3_cdp = xzalloc(struct feat_node)) == NULL ) + return -ENOMEM; + return 0; } @@ -1294,11 +1344,21 @@ static void psr_cpu_init(void) { cpuid_count_leaf(PSR_CPUID_LEVEL_CAT, 1, ®s); - feat = feat_l3_cat; - feat_l3_cat = NULL; - feat_props[PSR_SOCKET_L3_CAT] = &l3_cat_props; - - cat_init_feature(®s, feat, info, PSR_SOCKET_L3_CAT); + if ( (regs.c & PSR_CAT_CDP_CAPABILITY) && (opt_psr & PSR_CDP) && + !info->features[PSR_SOCKET_L3_CDP] ) + { + feat = feat_l3_cdp; + feat_l3_cdp = NULL; + feat_props[PSR_SOCKET_L3_CDP] = &l3_cdp_props; + cat_init_feature(®s, feat, info, PSR_SOCKET_L3_CDP); + } + else + { + feat = feat_l3_cat; + feat_l3_cat = NULL; + feat_props[PSR_SOCKET_L3_CAT] = &l3_cat_props; + cat_init_feature(®s, feat, info, PSR_SOCKET_L3_CAT); + } info->feat_init = true; } @@ -1360,7 +1420,7 @@ static int __init psr_presmp_init(void) if ( (opt_psr & PSR_CMT) && opt_rmid_max ) init_psr_cmt(opt_rmid_max); - if ( opt_psr & PSR_CAT ) + if ( opt_psr & (PSR_CAT | PSR_CDP) ) init_psr(); if ( psr_cpu_prepare() )
This patch implements the CPU init flow for CDP. The flow is almost same as L3 CAT. Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com> --- v12: - move 'type[]' assignment into l3_cdp_props declaration to make it be 'const'. (suggested by Jan Beulich) - remove "L2 CAT" indication in printk. (suggested by Jan Beulich) - fix coding style issue. (suggested by Jan Beulich) - change 'val' type to uint64_t. (suggested by Jan Beulich) - use 1ull. (suggested by Jan Beulich) - restore mask(0) MSR to default value. (suggested by Jan Beulich) v11: - changes about 'feat_props'. (suggested by Jan Beulich) - remove MSR restore action which is unnecessary. (suggested by Jan Beulich) - modify commit message. v10: - fix comment. (suggested by Jan Beulich) - use swith in 'cat_init_feature' to handle different feature types. (suggested by Jan Beulich) - changes about 'props'. (suggested by Jan Beulich) - restore MSRs to default value when cpu online. (suggested by Jan Beulich) - remove feat_mask. (suggested by Jan Beulich) v9: - modify commit message to describe flow clearer. - handle cpu offline and online again case to read MSRs registers values back and save them into cos array to make user can get real data. - modify error handling process in 'psr_cpu_prepare' to reduce redundant codes. - modify 'get_cdp_data' and 'get_cdp_code' to make them standard. (suggested by Roger Pau and Jan Beulich) - encapsulate CDP operations into 'cat_init_feature' to reduce redundant codes. (suggested by Roger Pau) - reuse 'cat_get_cos_max' for CDP. (suggested by Roger Pau) - handle 'PSR_CDP' in psr_presmp_init to make init work can be done when there is only 'psr=cdp' in cmdline. - remove unnecessary comment. (suggested by Jan Beulich) - move CDP related codes in 'cpu_init_work' into 'psr_cpu_init'. (suggested by Jan Beulich) - add codes to handle CDP's 'cos_num'. (suggested by Jan Beulich) - fix coding style issue. (suggested by Jan Beulich) - do not free resources when allocation fails in 'psr_cpu_prepare'. (suggested by Jan Beulich) - changes about 'uint64_t' to 'uint32_t'. (suggested by Jan Beulich) v7: - initialize 'l3_cdp'. (suggested by Konrad Rzeszutek Wilk) v6: - use 'cpuid_leaf'. (suggested by Konrad Rzeszutek Wilk and Jan Beulich) v5: - remove codes to free 'feat_l3_cdp' 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 'l3_cdp_get_max_cos_max' to 'l3_cdp_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 to make codes easier to understand. (suggested by Jan Beulich) --- --- xen/arch/x86/psr.c | 74 ++++++++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 67 insertions(+), 7 deletions(-)