| Message ID | 1487148579-7243-11-git-send-email-yi.y.sun@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Feb 15, 2017 at 04:49:25PM +0800, Yi Sun wrote: > Continue with patch: > 'x86: refactor psr: 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> > --- > xen/arch/x86/psr.c | 93 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 93 insertions(+) > > diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c > index ae108b9..7fab28b 100644 > --- a/xen/arch/x86/psr.c > +++ b/xen/arch/x86/psr.c > @@ -145,6 +145,19 @@ struct feat_ops { > const struct feat_node *feat, > enum cbm_type type, > uint64_t m); > + /* > + * 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 the amount of entries to skip in the value array > + * or error. > + * 1 - one entry in value array. > + * 2 - two entries in value array, e.g. CDP uses two entries. > + * negative - error. This doesn't match the code (yet?). What about return value 0? > + */ > + int (*compare_val)(const uint64_t val[], const struct feat_node *feat, > + unsigned int cos, bool *found); Indentation. Wei.
On 17-02-26 17:43:20, Wei Liu wrote: > On Wed, Feb 15, 2017 at 04:49:25PM +0800, Yi Sun wrote: > > Continue with patch: > > 'x86: refactor psr: 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> > > --- > > xen/arch/x86/psr.c | 93 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 93 insertions(+) > > > > diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c > > index ae108b9..7fab28b 100644 > > --- a/xen/arch/x86/psr.c > > +++ b/xen/arch/x86/psr.c > > @@ -145,6 +145,19 @@ struct feat_ops { > > const struct feat_node *feat, > > enum cbm_type type, > > uint64_t m); > > + /* > > + * 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 the amount of entries to skip in the value array > > + * or error. > > + * 1 - one entry in value array. > > + * 2 - two entries in value array, e.g. CDP uses two entries. > > + * negative - error. > > This doesn't match the code (yet?). > > What about return value 0? > Oh, sorry, per Jan's suggestion, I have change the type of the function. But I forgot the modify the comments. Will fix it. > > + */ > > + int (*compare_val)(const uint64_t val[], const struct feat_node *feat, > > + unsigned int cos, bool *found); > > Indentation. > Thanks! > > Wei.
>>> On 15.02.17 at 09:49, <yi.y.sun@linux.intel.com> wrote: > Continue with patch: ??? > 'x86: refactor psr: set value: assemble features value array' Or did you perhaps mean "continue from"? > --- a/xen/arch/x86/psr.c > +++ b/xen/arch/x86/psr.c > @@ -145,6 +145,19 @@ struct feat_ops { > const struct feat_node *feat, > enum cbm_type type, > uint64_t m); > + /* > + * 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 the amount of entries to skip in the value array > + * or error. > + * 1 - one entry in value array. > + * 2 - two entries in value array, e.g. CDP uses two entries. Isn't this get_cos_num()'s result again? Or, looking at the function below, is the comment simply stale? > @@ -356,6 +369,34 @@ static int l3_cat_set_new_val(uint64_t val[], > return 0; > } > > +static int l3_cat_compare_val(const uint64_t val[], > + const struct feat_node *feat, > + unsigned int cos, bool *found) > +{ > + uint64_t l3_def_cbm; > + > + l3_def_cbm = (1ull << feat->info.l3_cat_info.cbm_len) - 1; Please only calculate the value on the path you need it. Also this will again degenerate of cbm_len == 64. > + /* > + * 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.l3_cat_info.cos_max ) > + { > + if ( val[0] != l3_def_cbm ) > + { > + *found = false; > + return -ENOENT; What is the difference between this "not found" and ... > + } > + *found = true; > + } > + else > + *found = (val[0] == feat->cos_reg_val[cos]); > + > + return 0; ... the possible one here? I.e. why once -ENOENT and once 0 as return value? > @@ -715,6 +757,57 @@ static int find_cos(const uint64_t *val, uint32_t array_len, > enum psr_feat_type feat_type, > const struct psr_socket_info *info) > { > + unsigned int cos; > + const unsigned int *ref = info->cos_ref; > + const struct feat_node *feat; > + const uint64_t *val_tmp = val; > + int ret; > + bool found = false; > + unsigned int cos_max = 0; > + > + /* cos_max is the one of the feature which is being set. */ > + list_for_each_entry(feat, &info->feat_list, list) > + { > + if ( feat->feature != feat_type ) > + continue; > + > + cos_max = feat->ops.get_cos_max(feat); > + if ( cos_max > 0 ) What's the purpose of this check? I.e. why do you continue the loop in case you find zero? You won't find another node with the same feat_type, will you? > + break; > + } > + > + for ( cos = 0; cos <= cos_max; cos++ ) > + { > + if ( cos && !ref[cos] ) > + continue; > + > + /* Not found, need find again from beginning. */ You may not even have looked yet, so how can you say "not found"? Jan
On 17-03-08 10:03:10, Jan Beulich wrote: > >>> On 15.02.17 at 09:49, <yi.y.sun@linux.intel.com> wrote: > > Continue with patch: > > ??? > > > 'x86: refactor psr: set value: assemble features value array' > > Or did you perhaps mean "continue from"? > Should be 'from'. Thanks! > > --- a/xen/arch/x86/psr.c > > +++ b/xen/arch/x86/psr.c > > @@ -145,6 +145,19 @@ struct feat_ops { > > const struct feat_node *feat, > > enum cbm_type type, > > uint64_t m); > > + /* > > + * 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 the amount of entries to skip in the value array > > + * or error. > > + * 1 - one entry in value array. > > + * 2 - two entries in value array, e.g. CDP uses two entries. > > Isn't this get_cos_num()'s result again? Or, looking at the function > below, is the comment simply stale? > Sorry, forgot to update it. > > @@ -356,6 +369,34 @@ static int l3_cat_set_new_val(uint64_t val[], > > return 0; > > } > > > > +static int l3_cat_compare_val(const uint64_t val[], > > + const struct feat_node *feat, > > + unsigned int cos, bool *found) > > +{ > > + uint64_t l3_def_cbm; > > + > > + l3_def_cbm = (1ull << feat->info.l3_cat_info.cbm_len) - 1; > > Please only calculate the value on the path you need it. Also this > will again degenerate of cbm_len == 64. > Sorry, what do you mean? I need get the default value of L3 CAT here to check if input val equals the default one if cos exceeds cos_max. As explanation in previous patch, cbm_len is not 64. It means how many bits the CBM value has. E.g. L3 CAT may have 11 bits. L2 CAT may have 8 bits. > > + /* > > + * 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.l3_cat_info.cos_max ) > > + { > > + if ( val[0] != l3_def_cbm ) > > + { > > + *found = false; > > + return -ENOENT; > > What is the difference between this "not found" and ... > This is an error case. If input cos exceeds the cos_max and the input val is not default value, that means the input parameters exceed HW ability. We should report error back. > > + } > > + *found = true; > > + } > > + else > > + *found = (val[0] == feat->cos_reg_val[cos]); > > + > > + return 0; > > ... the possible one here? I.e. why once -ENOENT and once 0 as > return value? > 0 means success. '*found' means if the val is found or not. Per Roger's suggestion, I will refine this function to use return value to check if it is found or not. > > @@ -715,6 +757,57 @@ static int find_cos(const uint64_t *val, uint32_t array_len, > > enum psr_feat_type feat_type, > > const struct psr_socket_info *info) > > { > > + unsigned int cos; > > + const unsigned int *ref = info->cos_ref; > > + const struct feat_node *feat; > > + const uint64_t *val_tmp = val; > > + int ret; > > + bool found = false; > > + unsigned int cos_max = 0; > > + > > + /* cos_max is the one of the feature which is being set. */ > > + list_for_each_entry(feat, &info->feat_list, list) > > + { > > + if ( feat->feature != feat_type ) > > + continue; > > + > > + cos_max = feat->ops.get_cos_max(feat); > > + if ( cos_max > 0 ) > > What's the purpose of this check? I.e. why do you continue the loop > in case you find zero? You won't find another node with the same > feat_type, will you? > This is corrected in next version. Thanks! > > + break; > > + } > > + > > + for ( cos = 0; cos <= cos_max; cos++ ) > > + { > > + if ( cos && !ref[cos] ) > > + continue; > > + > > + /* Not found, need find again from beginning. */ > > You may not even have looked yet, so how can you say "not found"? > Sorry, it should be "If not found, need find again...". > Jan
>>> On 10.03.17 at 06:35, <yi.y.sun@linux.intel.com> wrote: > On 17-03-08 10:03:10, Jan Beulich wrote: >> >>> On 15.02.17 at 09:49, <yi.y.sun@linux.intel.com> wrote: >> > @@ -356,6 +369,34 @@ static int l3_cat_set_new_val(uint64_t val[], >> > return 0; >> > } >> > >> > +static int l3_cat_compare_val(const uint64_t val[], >> > + const struct feat_node *feat, >> > + unsigned int cos, bool *found) >> > +{ >> > + uint64_t l3_def_cbm; >> > + >> > + l3_def_cbm = (1ull << feat->info.l3_cat_info.cbm_len) - 1; >> >> Please only calculate the value on the path you need it. Also this >> will again degenerate of cbm_len == 64. >> > Sorry, what do you mean? I need get the default value of L3 CAT here > to check if input val equals the default one if cos exceeds cos_max. No, you don't need it at function scope. You only need it ... >> > + /* >> > + * 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.l3_cat_info.cos_max ) >> > + { >> > + if ( val[0] != l3_def_cbm ) ... in this more narrow one. Specifically no code outside the outer if() needs the variable. >> > + { >> > + *found = false; >> > + return -ENOENT; >> >> What is the difference between this "not found" and ... >> > This is an error case. If input cos exceeds the cos_max and the input val is > not default value, that means the input parameters exceed HW ability. We should > report error back. > >> > + } >> > + *found = true; >> > + } >> > + else >> > + *found = (val[0] == feat->cos_reg_val[cos]); >> > + >> > + return 0; >> >> ... the possible one here? I.e. why once -ENOENT and once 0 as >> return value? >> > 0 means success. '*found' means if the val is found or not. I still don't understand the difference, but perhaps this will all sort itself out once the cos_max checks get done in common code and the default values (suitably stored, as suggested elsewhere) can also be checked by common code. In fact with that I'm not even sure you will continue to require this feature dependent actor anymore. > Per Roger's > suggestion, I will refine this function to use return value to check > if it is found or not. Well, let's see how this ends up being. Jan
diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c index ae108b9..7fab28b 100644 --- a/xen/arch/x86/psr.c +++ b/xen/arch/x86/psr.c @@ -145,6 +145,19 @@ struct feat_ops { const struct feat_node *feat, enum cbm_type type, uint64_t m); + /* + * 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 the amount of entries to skip in the value array + * or error. + * 1 - one entry in value array. + * 2 - two entries in value array, e.g. CDP uses two entries. + * negative - error. + */ + int (*compare_val)(const uint64_t val[], const struct feat_node *feat, + unsigned int cos, bool *found); }; /* @@ -356,6 +369,34 @@ static int l3_cat_set_new_val(uint64_t val[], return 0; } +static int l3_cat_compare_val(const uint64_t val[], + const struct feat_node *feat, + unsigned int cos, bool *found) +{ + uint64_t l3_def_cbm; + + l3_def_cbm = (1ull << feat->info.l3_cat_info.cbm_len) - 1; + + /* + * 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.l3_cat_info.cos_max ) + { + if ( val[0] != l3_def_cbm ) + { + *found = false; + return -ENOENT; + } + *found = true; + } + else + *found = (val[0] == feat->cos_reg_val[cos]); + + return 0; +} + static const struct feat_ops l3_cat_ops = { .get_cos_max = l3_cat_get_cos_max, .get_feat_info = l3_cat_get_feat_info, @@ -363,6 +404,7 @@ static const struct feat_ops l3_cat_ops = { .get_cos_num = l3_cat_get_cos_num, .get_old_val = l3_cat_get_old_val, .set_new_val = l3_cat_set_new_val, + .compare_val = l3_cat_compare_val, }; static void __init parse_psr_bool(char *s, char *value, char *feature, @@ -715,6 +757,57 @@ static int find_cos(const uint64_t *val, uint32_t array_len, enum psr_feat_type feat_type, const struct psr_socket_info *info) { + unsigned int cos; + const unsigned int *ref = info->cos_ref; + const struct feat_node *feat; + const uint64_t *val_tmp = val; + int ret; + bool found = false; + unsigned int cos_max = 0; + + /* cos_max is the one of the feature which is being set. */ + list_for_each_entry(feat, &info->feat_list, list) + { + if ( feat->feature != feat_type ) + continue; + + cos_max = feat->ops.get_cos_max(feat); + if ( cos_max > 0 ) + break; + } + + for ( cos = 0; cos <= cos_max; cos++ ) + { + if ( cos && !ref[cos] ) + continue; + + /* Not found, need find again from beginning. */ + val_tmp = val; + list_for_each_entry(feat, &info->feat_list, list) + { + /* + * Compare value according to feature list order. + * We must follow this order because value array is assembled + * as this order in get_old_set_new(). + */ + ret = feat->ops.compare_val(val_tmp, feat, cos, &found); + if ( ret < 0 ) + return ret; + + /* If fail to match, go to next cos to compare. */ + if ( !found ) + break; + + val_tmp += feat->ops.get_cos_num(feat); + if ( val_tmp - val > array_len ) + return -EINVAL; + } + + /* For this COS ID all entries in the values array did match. Use it. */ + if ( found ) + return cos; + } + return -ENOENT; }
Continue with patch: 'x86: refactor psr: 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> --- xen/arch/x86/psr.c | 93 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 93 insertions(+)