| Message ID | 1481688484-5093-12-git-send-email-yi.y.sun@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 14.12.16 at 05:07, <yi.y.sun@linux.intel.com> wrote: > --- a/xen/arch/x86/psr.c > +++ b/xen/arch/x86/psr.c > @@ -169,6 +169,23 @@ struct feat_ops { > */ > int (*compare_val)(const uint64_t val[], const struct feat_node *feat, > unsigned int cos, bool *found); > + /* > + * exceeds_cos_max is used to check if the input cos id exceeds the > + * feature's cos_max and if the input value is not the default one. > + * Even if the associated cos exceeds the cos_max, HW can work with default > + * value. That is the reason we need check if input value is default one. > + * If both criteria are fulfilled, that means the input exceeds the > + * range. Isn't this last sentence the wrong way round? > + * The return value of the function means the number of the value array > + * entries to skip or error. > + * 1 - one entry in value array. > + * 2 - two entries in value array, e.g. CDP uses two entries. > + * 0 - error, exceed cos_max and the input value is not default. > + */ > + unsigned int (*exceeds_cos_max)(const uint64_t val[], > + const struct feat_node *feat, > + unsigned int cos); IIRC I did recommend "exceeds" in the name during earlier review, but also iirc the semantics of the call were different back then. Please try to name functions such that they describe themselves in at least a minimalistic ways. My main issue with this naming is that the function returning non-zero (i.e. true in C meaning within conditionals) means "no" here rather than "yes". fits_cos_max() would therefore be a possibly better fit. > +static bool exceeds_cos_max(const uint64_t *val, > + uint32_t array_len, > + const struct psr_socket_info *info, > + unsigned int cos) > +{ > + unsigned int ret; > + const uint64_t *val_tmp = val; > + const struct feat_node *feat_tmp; > + > + list_for_each_entry(feat_tmp, &info->feat_list, list) > + { > + ret = feat_tmp->ops.exceeds_cos_max(val_tmp, feat_tmp, cos); > + if ( !ret ) > + return false; > + > + val_tmp += ret; > + if ( val_tmp - val > array_len ) > + return false; > + } > + > + return true; > +} Similarly here - name and return value don't fit together; I think you want to invert the return values or (along the lines above) rename the function. > static int alloc_new_cos(const struct psr_socket_info *info, > const uint64_t *val, uint32_t array_len, > unsigned int old_cos, > enum cbm_type type) > { > - return 0; > + unsigned int cos; > + unsigned int cos_max = 0; > + const struct feat_node *feat_tmp; > + const unsigned int *ref = info->cos_ref; > + > + /* > + * cos_max is the one of the feature which is being set. > + */ > + list_for_each_entry(feat_tmp, &info->feat_list, list) > + { > + cos_max = feat_tmp->ops.get_cos_max_from_type(feat_tmp, type); > + if ( cos_max > 0 ) > + break; > + } > + > + if ( !cos_max ) > + return -ENOENT; > + > + /* > + * If old cos is referred only by the domain, then use it. And, we cannot > + * use id 0 because it stores the default values. > + */ > + if ( ref[old_cos] == 1 && old_cos ) Please swap both sides of && - cheaper checks should come first if possible. > + if ( exceeds_cos_max(val, array_len, info, old_cos) ) Also please fold the two if()-s. > + return old_cos; And then, as indicated before, this part is not really an allocation, but a re-use, so would likely better move into the caller (or the function's name should be adjusted). > + /* Find an unused one other than cos0. */ > + for ( cos = 1; cos <= cos_max; cos++ ) > + /* > + * ref is 0 means this COS is not used by other domain and > + * can be used for current setting. > + */ > + if ( !ref[cos] ) > + { > + if ( !exceeds_cos_max(val, array_len, info, cos) ) > + return -ENOENT; > + > + return cos; > + } While a comment is just white space, this looks suspicious without another pair of braces around the for() body. Jan
On 17-01-10 08:08:19, Jan Beulich wrote: > >>> On 14.12.16 at 05:07, <yi.y.sun@linux.intel.com> wrote: > > --- a/xen/arch/x86/psr.c > > +++ b/xen/arch/x86/psr.c > > @@ -169,6 +169,23 @@ struct feat_ops { > > */ > > int (*compare_val)(const uint64_t val[], const struct feat_node *feat, > > unsigned int cos, bool *found); > > + /* > > + * exceeds_cos_max is used to check if the input cos id exceeds the > > + * feature's cos_max and if the input value is not the default one. > > + * Even if the associated cos exceeds the cos_max, HW can work with default > > + * value. That is the reason we need check if input value is default one. > > + * If both criteria are fulfilled, that means the input exceeds the > > + * range. > > Isn't this last sentence the wrong way round? > Sorry. > > + * The return value of the function means the number of the value array > > + * entries to skip or error. > > + * 1 - one entry in value array. > > + * 2 - two entries in value array, e.g. CDP uses two entries. > > + * 0 - error, exceed cos_max and the input value is not default. > > + */ > > + unsigned int (*exceeds_cos_max)(const uint64_t val[], > > + const struct feat_node *feat, > > + unsigned int cos); > > IIRC I did recommend "exceeds" in the name during earlier review, > but also iirc the semantics of the call were different back then. > Please try to name functions such that they describe themselves > in at least a minimalistic ways. My main issue with this naming is > that the function returning non-zero (i.e. true in C meaning within > conditionals) means "no" here rather than "yes". fits_cos_max() > would therefore be a possibly better fit. > Thanks for the suggestion! > > +static bool exceeds_cos_max(const uint64_t *val, > > + uint32_t array_len, > > + const struct psr_socket_info *info, > > + unsigned int cos) > > +{ > > + unsigned int ret; > > + const uint64_t *val_tmp = val; > > + const struct feat_node *feat_tmp; > > + > > + list_for_each_entry(feat_tmp, &info->feat_list, list) > > + { > > + ret = feat_tmp->ops.exceeds_cos_max(val_tmp, feat_tmp, cos); > > + if ( !ret ) > > + return false; > > + > > + val_tmp += ret; > > + if ( val_tmp - val > array_len ) > > + return false; > > + } > > + > > + return true; > > +} > > Similarly here - name and return value don't fit together; I think > you want to invert the return values or (along the lines above) > rename the function. > Will rename the callback function to make it accurate. Thanks! > > static int alloc_new_cos(const struct psr_socket_info *info, > > const uint64_t *val, uint32_t array_len, > > unsigned int old_cos, > > enum cbm_type type) > > { > > - return 0; > > + unsigned int cos; > > + unsigned int cos_max = 0; > > + const struct feat_node *feat_tmp; > > + const unsigned int *ref = info->cos_ref; > > + > > + /* > > + * cos_max is the one of the feature which is being set. > > + */ > > + list_for_each_entry(feat_tmp, &info->feat_list, list) > > + { > > + cos_max = feat_tmp->ops.get_cos_max_from_type(feat_tmp, type); > > + if ( cos_max > 0 ) > > + break; > > + } > > + > > + if ( !cos_max ) > > + return -ENOENT; > > + > > + /* > > + * If old cos is referred only by the domain, then use it. And, we cannot > > + * use id 0 because it stores the default values. > > + */ > > + if ( ref[old_cos] == 1 && old_cos ) > > Please swap both sides of && - cheaper checks should come first if > possible. > Sure, thanks! > > + if ( exceeds_cos_max(val, array_len, info, old_cos) ) > > Also please fold the two if()-s. > Ok, thanks! > > + return old_cos; > > And then, as indicated before, this part is not really an allocation, > but a re-use, so would likely better move into the caller (or the > function's name should be adjusted). > Prefer to change function name to 'pick_avail_cos'. > > + /* Find an unused one other than cos0. */ > > + for ( cos = 1; cos <= cos_max; cos++ ) > > + /* > > + * ref is 0 means this COS is not used by other domain and > > + * can be used for current setting. > > + */ > > + if ( !ref[cos] ) > > + { > > + if ( !exceeds_cos_max(val, array_len, info, cos) ) > > + return -ENOENT; > > + > > + return cos; > > + } > > While a comment is just white space, this looks suspicious without > another pair of braces around the for() body. > Sure. > Jan
diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c index 6b2b1e0..ac98c39 100644 --- a/xen/arch/x86/psr.c +++ b/xen/arch/x86/psr.c @@ -169,6 +169,23 @@ struct feat_ops { */ int (*compare_val)(const uint64_t val[], const struct feat_node *feat, unsigned int cos, bool *found); + /* + * exceeds_cos_max is used to check if the input cos id exceeds the + * feature's cos_max and if the input value is not the default one. + * Even if the associated cos exceeds the cos_max, HW can work with default + * value. That is the reason we need check if input value is default one. + * If both criteria are fulfilled, that means the input exceeds the + * range. + * + * The return value of the function means the number of the value array + * entries to skip or error. + * 1 - one entry in value array. + * 2 - two entries in value array, e.g. CDP uses two entries. + * 0 - error, exceed cos_max and the input value is not default. + */ + unsigned int (*exceeds_cos_max)(const uint64_t val[], + const struct feat_node *feat, + unsigned int cos); }; @@ -402,6 +419,28 @@ static int l3_cat_compare_val(const uint64_t val[], /* L3 CAT uses one COS. */ return 1; } + +static unsigned int l3_cat_exceeds_cos_max(const uint64_t val[], + const struct feat_node *feat, + unsigned int cos) +{ + uint64_t l3_def_cbm; + + l3_def_cbm = (1ull << feat->info.l3_cat_info.cbm_len) - 1; + + /* CAT */ + if ( cos > feat->info.l3_cat_info.cos_max && + val[0] != l3_def_cbm ) + /* + * Exceed cos_max and value to set is not default, + * return error. + */ + return 0; + + /* L3 CAT uses one COS. */ + return 1; +} + struct feat_ops l3_cat_ops = { .init_feature = l3_cat_init_feature, .get_max_cos_max = l3_cat_get_max_cos_max, @@ -412,6 +451,7 @@ struct feat_ops l3_cat_ops = { .set_new_val = l3_cat_set_new_val, .get_cos_max_from_type = l3_cat_get_cos_max_from_type, .compare_val = l3_cat_compare_val, + .exceeds_cos_max = l3_cat_exceeds_cos_max, }; static void __init parse_psr_bool(char *s, char *value, char *feature, @@ -778,12 +818,75 @@ static int find_cos(const uint64_t *val, uint32_t array_len, return -ENOENT; } +static bool exceeds_cos_max(const uint64_t *val, + uint32_t array_len, + const struct psr_socket_info *info, + unsigned int cos) +{ + unsigned int ret; + const uint64_t *val_tmp = val; + const struct feat_node *feat_tmp; + + list_for_each_entry(feat_tmp, &info->feat_list, list) + { + ret = feat_tmp->ops.exceeds_cos_max(val_tmp, feat_tmp, cos); + if ( !ret ) + return false; + + val_tmp += ret; + if ( val_tmp - val > array_len ) + return false; + } + + return true; +} + static int alloc_new_cos(const struct psr_socket_info *info, const uint64_t *val, uint32_t array_len, unsigned int old_cos, enum cbm_type type) { - return 0; + unsigned int cos; + unsigned int cos_max = 0; + const struct feat_node *feat_tmp; + const unsigned int *ref = info->cos_ref; + + /* + * cos_max is the one of the feature which is being set. + */ + list_for_each_entry(feat_tmp, &info->feat_list, list) + { + cos_max = feat_tmp->ops.get_cos_max_from_type(feat_tmp, type); + if ( cos_max > 0 ) + break; + } + + if ( !cos_max ) + return -ENOENT; + + /* + * If old cos is referred only by the domain, then use it. And, we cannot + * use id 0 because it stores the default values. + */ + if ( ref[old_cos] == 1 && old_cos ) + if ( exceeds_cos_max(val, array_len, info, old_cos) ) + return old_cos; + + /* Find an unused one other than cos0. */ + for ( cos = 1; cos <= cos_max; cos++ ) + /* + * ref is 0 means this COS is not used by other domain and + * can be used for current setting. + */ + if ( !ref[cos] ) + { + if ( !exceeds_cos_max(val, array_len, info, cos) ) + return -ENOENT; + + return cos; + } + + return -ENOENT; } static int write_psr_msr(unsigned int socket, unsigned int cos,
Continue with previous patches, if fail to find a COS ID, we need allocate a new COS ID for domain. Only COS ID that ref[COS_ID] is 1 or 0 can be allocated to input a new set feature values. Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com> --- xen/arch/x86/psr.c | 105 ++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 104 insertions(+), 1 deletion(-)