| Message ID | 1489662495-5375-12-git-send-email-yi.y.sun@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 16.03.17 at 12:08, <yi.y.sun@linux.intel.com> wrote: > --- a/xen/arch/x86/psr.c > +++ b/xen/arch/x86/psr.c > @@ -123,6 +123,19 @@ struct feat_node { > const struct feat_node *feat, > enum cbm_type type, > uint32_t new_val); > + > + /* > + * compare_val is used in set value process to compare if the > + * input value array can match all the features' COS registers values > + * according to input cos id. > + * > + * The return value is: > + * 1 - find the entry in value array. found ... > + * 0 - not find the entry in value array. didn't find ... > +static int cat_compare_val(const uint32_t val[], > + const struct feat_node *feat, > + unsigned int cos) > +{ > + /* > + * Different features' cos_max are different. If cos id of the feature > + * being set exceeds other feature's cos_max, the val of other feature > + * must be default value. HW supports such case. > + */ > + if ( cos > feat->info.cat_info.cos_max ) > + { > + /* cos_reg_val[0] is the default value. */ > + if ( val[0] != feat->cos_reg_val[0] ) > + return -EINVAL; As you can see, with cos_max moved into the generic portion of the feature node, this entire check can move into the caller. > + /* Find */ Found (also below) > + return 1; > + } > + > + if ( val[0] == feat->cos_reg_val[cos] ) > + /* Find */ > + return 1; > + > + /* Not find */ > + return 0; > +} Or actually, the entire function then becomes feature independent, as it seems. And I think I did suggest that already during review of an earlier version. > @@ -752,7 +793,61 @@ static int find_cos(const uint32_t val[], uint32_t array_len, > enum psr_feat_type feat_type, > const struct psr_socket_info *info) > { > + unsigned int cos, i; > + const unsigned int *ref = info->cos_ref; > + const struct feat_node *feat; > + const uint32_t *val_array = val; The name doesn't match the purpose - as you increment the pointer, its name should rather be "val_ptr" or some such. > + int find = 0; "found" again, or even simply "rc"? Also I think this would better move into the outer for() scope. > + unsigned int cos_max; > + > ASSERT(spin_is_locked((spinlock_t *)(&info->ref_lock))); > + > + /* cos_max is the one of the feature which is being set. */ > + feat = info->features[feat_type]; > + if ( !feat ) > + return -ENOENT; > + > + cos_max = feat->ops.get_cos_max(feat); > + > + for ( cos = 0; cos <= cos_max; cos++ ) > + { > + if ( cos && !ref[cos] ) > + continue; > + > + /* > + * If fail to find cos in below loop, need find whole feature array > + * again from beginning. > + */ > + val_array = val; You wouldn't need to re-do this here if you moved the variable declaration (with initializer) into this scope. This then also eliminates the need for the comment, which otherwise would need its wording corrected. > + for ( i = 0; i < PSR_SOCKET_MAX_FEAT; i++ ) > + { > + if ( !info->features[i] ) > + continue; > + > + feat = info->features[i]; Please swap if() and assignment, utilizing the local variable in the if(). Jan
On 17-03-27 04:28:23, Jan Beulich wrote: > >>> On 16.03.17 at 12:08, <yi.y.sun@linux.intel.com> wrote: > > --- a/xen/arch/x86/psr.c > > +++ b/xen/arch/x86/psr.c [...] > > +static int cat_compare_val(const uint32_t val[], > > + const struct feat_node *feat, > > + unsigned int cos) > > +{ > > + /* > > + * Different features' cos_max are different. If cos id of the feature > > + * being set exceeds other feature's cos_max, the val of other feature > > + * must be default value. HW supports such case. > > + */ > > + if ( cos > feat->info.cat_info.cos_max ) > > + { > > + /* cos_reg_val[0] is the default value. */ > > + if ( val[0] != feat->cos_reg_val[0] ) > > + return -EINVAL; > > As you can see, with cos_max moved into the generic portion of the > feature node, this entire check can move into the caller. > CDP has different behavior in this callback function. We need to check val[0] and val[1] like below: static int l3_cdp_compare_val(...) { if ( cos > feat->info.cat_info.cos_max ) { if ( val[0] != get_cdp_data(feat, 0) || val[1] != get_cdp_code(feat, 0) ) return -EINVAL; /* Find */ return 1; } if ( val[0] == get_cdp_data(feat, cos) && val[1] == get_cdp_code(feat, cos) ) /* Find */ return 1; ...... } > > + /* Find */ > > Found (also below) > > > + return 1; > > + } > > + > > + if ( val[0] == feat->cos_reg_val[cos] ) > > + /* Find */ > > + return 1; > > + > > + /* Not find */ > > + return 0; > > +} > > Or actually, the entire function then becomes feature independent, > as it seems. And I think I did suggest that already during review of > an earlier version. > Per above explanation, I think we have to keep this callback function. > > @@ -752,7 +793,61 @@ static int find_cos(const uint32_t val[], uint32_t array_len, > > enum psr_feat_type feat_type, > > const struct psr_socket_info *info) > > { > > + unsigned int cos, i; > > + const unsigned int *ref = info->cos_ref; > > + const struct feat_node *feat; > > + const uint32_t *val_array = val; > > The name doesn't match the purpose - as you increment the pointer, > its name should rather be "val_ptr" or some such. > Got it, thanks! > > + int find = 0; > > "found" again, or even simply "rc"? Also I think this would better > move into the outer for() scope. > Ok, will use 'found' and move it. > > + unsigned int cos_max; > > + > > ASSERT(spin_is_locked((spinlock_t *)(&info->ref_lock))); > > + > > + /* cos_max is the one of the feature which is being set. */ > > + feat = info->features[feat_type]; > > + if ( !feat ) > > + return -ENOENT; > > + > > + cos_max = feat->ops.get_cos_max(feat); > > + > > + for ( cos = 0; cos <= cos_max; cos++ ) > > + { > > + if ( cos && !ref[cos] ) > > + continue; > > + > > + /* > > + * If fail to find cos in below loop, need find whole feature array > > + * again from beginning. > > + */ > > + val_array = val; > > You wouldn't need to re-do this here if you moved the variable > declaration (with initializer) into this scope. This then also > eliminates the need for the comment, which otherwise would > need its wording corrected. > Ok, thanks! > > + for ( i = 0; i < PSR_SOCKET_MAX_FEAT; i++ ) > > + { > > + if ( !info->features[i] ) > > + continue; > > + > > + feat = info->features[i]; > > Please swap if() and assignment, utilizing the local variable in the > if(). > Ok, thanks! > Jan
>>> On 28.03.17 at 05:26, <yi.y.sun@linux.intel.com> wrote: > On 17-03-27 04:28:23, Jan Beulich wrote: >> >>> On 16.03.17 at 12:08, <yi.y.sun@linux.intel.com> wrote: >> > --- a/xen/arch/x86/psr.c >> > +++ b/xen/arch/x86/psr.c > [...] > >> > +static int cat_compare_val(const uint32_t val[], >> > + const struct feat_node *feat, >> > + unsigned int cos) >> > +{ >> > + /* >> > + * Different features' cos_max are different. If cos id of the feature >> > + * being set exceeds other feature's cos_max, the val of other feature >> > + * must be default value. HW supports such case. >> > + */ >> > + if ( cos > feat->info.cat_info.cos_max ) >> > + { >> > + /* cos_reg_val[0] is the default value. */ >> > + if ( val[0] != feat->cos_reg_val[0] ) >> > + return -EINVAL; >> >> As you can see, with cos_max moved into the generic portion of the >> feature node, this entire check can move into the caller. >> > CDP has different behavior in this callback function. We need to check > val[0] > and val[1] like below: > static int l3_cdp_compare_val(...) > { > if ( cos > feat->info.cat_info.cos_max ) > { > if ( val[0] != get_cdp_data(feat, 0) || > val[1] != get_cdp_code(feat, 0) ) > return -EINVAL; > > /* Find */ > return 1; > } > > if ( val[0] == get_cdp_data(feat, cos) && > val[1] == get_cdp_code(feat, cos) ) > /* Find */ > return 1; > ...... > } There's no difference other than there being two values checked here. Moving this to generic code should be easily possible as long as the get_val() hook is flexible enough. Once again, please think thoroughly about where to draw the line between generic code and feature specific hooks - the latter should be reduced to a minimum. >> > + return 1; >> > + } >> > + >> > + if ( val[0] == feat->cos_reg_val[cos] ) >> > + /* Find */ >> > + return 1; >> > + >> > + /* Not find */ >> > + return 0; >> > +} >> >> Or actually, the entire function then becomes feature independent, >> as it seems. And I think I did suggest that already during review of >> an earlier version. >> > Per above explanation, I think we have to keep this callback function. Per above explanation, I think you don't need to keep this callback function. >> > + int find = 0; >> >> "found" again, or even simply "rc"? Also I think this would better >> move into the outer for() scope. >> > Ok, will use 'found' and move it. Please check its use(s): It should be "found" only if that's the meaning it always has. It's type being int (rather than bool) suggests otherwise ... Jan
diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c index 18aad8e..f2c2614 100644 --- a/xen/arch/x86/psr.c +++ b/xen/arch/x86/psr.c @@ -123,6 +123,19 @@ struct feat_node { const struct feat_node *feat, enum cbm_type type, uint32_t new_val); + + /* + * compare_val is used in set value process to compare if the + * input value array can match all the features' COS registers values + * according to input cos id. + * + * The return value is: + * 1 - find the entry in value array. + * 0 - not find the entry in value array. + * negative - error. + */ + int (*compare_val)(const uint32_t val[], const struct feat_node *feat, + unsigned int cos); } ops; /* Encapsulate feature specific HW info here. */ @@ -352,6 +365,33 @@ static int cat_set_new_val(uint32_t val[], return 0; } +static int cat_compare_val(const uint32_t val[], + const struct feat_node *feat, + unsigned int cos) +{ + /* + * Different features' cos_max are different. If cos id of the feature + * being set exceeds other feature's cos_max, the val of other feature + * must be default value. HW supports such case. + */ + if ( cos > feat->info.cat_info.cos_max ) + { + /* cos_reg_val[0] is the default value. */ + if ( val[0] != feat->cos_reg_val[0] ) + return -EINVAL; + + /* Find */ + return 1; + } + + if ( val[0] == feat->cos_reg_val[cos] ) + /* Find */ + return 1; + + /* Not find */ + return 0; +} + /* L3 CAT ops */ static const struct feat_ops l3_cat_ops = { .get_cos_max = cat_get_cos_max, @@ -359,6 +399,7 @@ static const struct feat_ops l3_cat_ops = { .get_val = cat_get_val, .get_old_val = cat_get_old_val, .set_new_val = cat_set_new_val, + .compare_val = cat_compare_val, }; static void __init parse_psr_bool(char *s, char *value, char *feature, @@ -752,7 +793,61 @@ static int find_cos(const uint32_t val[], uint32_t array_len, enum psr_feat_type feat_type, const struct psr_socket_info *info) { + unsigned int cos, i; + const unsigned int *ref = info->cos_ref; + const struct feat_node *feat; + const uint32_t *val_array = val; + int find = 0; + unsigned int cos_max; + ASSERT(spin_is_locked((spinlock_t *)(&info->ref_lock))); + + /* cos_max is the one of the feature which is being set. */ + feat = info->features[feat_type]; + if ( !feat ) + return -ENOENT; + + cos_max = feat->ops.get_cos_max(feat); + + for ( cos = 0; cos <= cos_max; cos++ ) + { + if ( cos && !ref[cos] ) + continue; + + /* + * If fail to find cos in below loop, need find whole feature array + * again from beginning. + */ + val_array = val; + for ( i = 0; i < PSR_SOCKET_MAX_FEAT; i++ ) + { + if ( !info->features[i] ) + continue; + + feat = info->features[i]; + /* + * Compare value according to feature array order. + * We must follow this order because value array is assembled + * as this order. + */ + find = feat->ops.compare_val(val_array, feat, cos); + if ( find < 0 ) + return find; + + /* If fail to match, go to next cos to compare. */ + if ( !find ) + break; + + val_array += feat->cos_num; + if ( val_array - val > array_len ) + return -ENOSPC; + } + + /* For this COS ID all entries in the values array did match. Use it. */ + if ( find ) + return cos; + } + return -ENOENT; }
Continue from patch: 'x86: refactor psr: L3 CAT: set value: assemble features value array' We can try to find if there is a COS ID on which all features' COS registers values are same as the array assembled before. Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com> --- v9: - modify comments of 'compare_val' to be same as current implementation. (suggested by Wei Liu) - fix indentation issue. (suggested by Wei Liu) - rename 'l3_cat_compare_val' to 'cat_compare_val' to cover all L3/L2 CAT features. (suggested by Roger Pau) - remove parameter 'found' from 'cat_compare_val' and modify the return values to let caller know if the id is found or not. (suggested by Roger Pau) - replace feature list handling to feature array handling. (suggested by Roger Pau) - replace 'get_cos_num' to 'feat->cos_num'. (suggested by Jan Beulich) - directly use 'cos_reg_val[0]' as default value. (suggested by Jan Beulich) - modify patch title to indicate 'L3 CAT'. (suggested by Jan Beulich) - changes about 'uint64_t' to 'uint32_t'. (suggested by Jan Beulich) v5: - modify commit message to provide exact patch name to continue from. (suggested by Jan Beulich) - remove 'get_cos_max_from_type' because it can be replaced by 'get_cos_max'. - move type check out from callback functions to caller. (suggested by Jan Beulich) - modify variables names to make them better, e.g. 'feat_tmp' to 'feat'. (suggested by Jan Beulich) - modify comments according to changes of codes. (suggested by Jan Beulich) v4: - create this patch to make codes easier to understand. (suggested by Jan Beulich) --- xen/arch/x86/psr.c | 95 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 95 insertions(+)