| Message ID | 1481688484-5093-10-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 > @@ -121,6 +121,35 @@ struct feat_ops { > /* get_val is used to get feature COS register value. */ > bool (*get_val)(const struct feat_node *feat, unsigned int cos, > enum cbm_type type, uint64_t *val); > + /* > + * get_cos_num is used to get the COS registers amount used by the > + * feature for one setting, e.g. CDP uses 2 COSs but CAT uses 1. > + */ > + unsigned int (*get_cos_num)(const struct feat_node *feat); > + /* > + * get_old_val and set_new_val are a pair of functions called together. > + * The caller will traverse all features in the list and call both > + * functions for every feature to do below two things: > + * 1. get old_cos register value of all supported features and > + * 2. set the new value for the feature. > + * > + * All the values are set into value array according the traversal order, > + * meaning the same order of feature list members. > + * > + * 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. Doesn't this match the get_cos_num() return value? Would be nice to be spelled out (and, where possible, ASSERT()ed). > @@ -186,6 +215,29 @@ static void free_feature(struct psr_socket_info *info) > } > } > > +static bool_t psr_check_cbm(unsigned int cbm_len, uint64_t cbm) bool (and then true/false in the function body) > +static int l3_cat_set_new_val(uint64_t val[], > + const struct feat_node *feat, > + unsigned int old_cos, > + enum cbm_type type, > + uint64_t m) > +{ > + if ( type != PSR_CBM_TYPE_L3 ) > + /* L3 CAT uses one COS. Skip it. */ > + return 1; If this is the wrong type, how can you return 1 (or any positive value) here? This basically suggests that, as recommended for an earlier operation already, that you want the type check done in the caller. Or wait - isn't type not matching an error here, as you iterate the same list twice in the same order? Which then gets us back to me mentioning that the set-new-value should really be done in common code, not in the callbacks; it would really only be the check (psr_check_cbm() in the L3 case above) which would require a callback. Jan
On 17-01-10 07:34:00, 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 > > @@ -121,6 +121,35 @@ struct feat_ops { > > /* get_val is used to get feature COS register value. */ > > bool (*get_val)(const struct feat_node *feat, unsigned int cos, > > enum cbm_type type, uint64_t *val); > > + /* > > + * get_cos_num is used to get the COS registers amount used by the > > + * feature for one setting, e.g. CDP uses 2 COSs but CAT uses 1. > > + */ > > + unsigned int (*get_cos_num)(const struct feat_node *feat); > > + /* > > + * get_old_val and set_new_val are a pair of functions called together. > > + * The caller will traverse all features in the list and call both > > + * functions for every feature to do below two things: > > + * 1. get old_cos register value of all supported features and > > + * 2. set the new value for the feature. > > + * > > + * All the values are set into value array according the traversal order, > > + * meaning the same order of feature list members. > > + * > > + * 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. > > Doesn't this match the get_cos_num() return value? Would be nice to > be spelled out (and, where possible, ASSERT()ed). > Yes, thanks! > > @@ -186,6 +215,29 @@ static void free_feature(struct psr_socket_info *info) > > } > > } > > > > +static bool_t psr_check_cbm(unsigned int cbm_len, uint64_t cbm) > > bool (and then true/false in the function body) > Ok, thanks! > > +static int l3_cat_set_new_val(uint64_t val[], > > + const struct feat_node *feat, > > + unsigned int old_cos, > > + enum cbm_type type, > > + uint64_t m) > > +{ > > + if ( type != PSR_CBM_TYPE_L3 ) > > + /* L3 CAT uses one COS. Skip it. */ > > + return 1; > > If this is the wrong type, how can you return 1 (or any positive > value) here? This basically suggests that, as recommended for an > earlier operation already, that you want the type check done in > the caller. Or wait - isn't type not matching an error here, as you > iterate the same list twice in the same order? Which then gets us > back to me mentioning that the set-new-value should really be > done in common code, not in the callbacks; it would really only be > the check (psr_check_cbm() in the L3 case above) which would > require a callback. > Your understanding is right. The value array will be iterated twice. First, assemble all features old value into it. Second, set the target value into array according to the target feature's position in the array. Because different features may have different behaviors, e.g. CDP uses two entries of the array, I think it would be better to make 'set_new_val' be a callback function. > Jan
>>> On 11.01.17 at 07:07, <yi.y.sun@linux.intel.com> wrote: > On 17-01-10 07:34:00, Jan Beulich wrote: >> >>> On 14.12.16 at 05:07, <yi.y.sun@linux.intel.com> wrote: >> > +static int l3_cat_set_new_val(uint64_t val[], >> > + const struct feat_node *feat, >> > + unsigned int old_cos, >> > + enum cbm_type type, >> > + uint64_t m) >> > +{ >> > + if ( type != PSR_CBM_TYPE_L3 ) >> > + /* L3 CAT uses one COS. Skip it. */ >> > + return 1; >> >> If this is the wrong type, how can you return 1 (or any positive >> value) here? This basically suggests that, as recommended for an >> earlier operation already, that you want the type check done in >> the caller. Or wait - isn't type not matching an error here, as you >> iterate the same list twice in the same order? Which then gets us >> back to me mentioning that the set-new-value should really be >> done in common code, not in the callbacks; it would really only be >> the check (psr_check_cbm() in the L3 case above) which would >> require a callback. >> > Your understanding is right. The value array will be iterated twice. First, > assemble all features old value into it. Second, set the target value into > array according to the target feature's position in the array. Because > different features may have different behaviors, e.g. CDP uses two entries > of the array, I think it would be better to make 'set_new_val' be a callback > function. Bot for common code to do that, all it takes is knowing the number of registers, which you already have a callback for. Jan
On 17-01-11 06:57:30, Jan Beulich wrote: > >>> On 11.01.17 at 07:07, <yi.y.sun@linux.intel.com> wrote: > > On 17-01-10 07:34:00, Jan Beulich wrote: > >> >>> On 14.12.16 at 05:07, <yi.y.sun@linux.intel.com> wrote: > >> > +static int l3_cat_set_new_val(uint64_t val[], > >> > + const struct feat_node *feat, > >> > + unsigned int old_cos, > >> > + enum cbm_type type, > >> > + uint64_t m) > >> > +{ > >> > + if ( type != PSR_CBM_TYPE_L3 ) > >> > + /* L3 CAT uses one COS. Skip it. */ > >> > + return 1; > >> > >> If this is the wrong type, how can you return 1 (or any positive > >> value) here? This basically suggests that, as recommended for an > >> earlier operation already, that you want the type check done in > >> the caller. Or wait - isn't type not matching an error here, as you > >> iterate the same list twice in the same order? Which then gets us > >> back to me mentioning that the set-new-value should really be > >> done in common code, not in the callbacks; it would really only be > >> the check (psr_check_cbm() in the L3 case above) which would > >> require a callback. > >> > > Your understanding is right. The value array will be iterated twice. First, > > assemble all features old value into it. Second, set the target value into > > array according to the target feature's position in the array. Because > > different features may have different behaviors, e.g. CDP uses two entries > > of the array, I think it would be better to make 'set_new_val' be a callback > > function. > > Bot for common code to do that, all it takes is knowing the number > of registers, which you already have a callback for. > Ok, will consider it. Thanks! > Jan
diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c index 838f2d1..8fcaa76 100644 --- a/xen/arch/x86/psr.c +++ b/xen/arch/x86/psr.c @@ -121,6 +121,35 @@ struct feat_ops { /* get_val is used to get feature COS register value. */ bool (*get_val)(const struct feat_node *feat, unsigned int cos, enum cbm_type type, uint64_t *val); + /* + * get_cos_num is used to get the COS registers amount used by the + * feature for one setting, e.g. CDP uses 2 COSs but CAT uses 1. + */ + unsigned int (*get_cos_num)(const struct feat_node *feat); + /* + * get_old_val and set_new_val are a pair of functions called together. + * The caller will traverse all features in the list and call both + * functions for every feature to do below two things: + * 1. get old_cos register value of all supported features and + * 2. set the new value for the feature. + * + * All the values are set into value array according the traversal order, + * meaning the same order of feature list members. + * + * 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 (*get_old_val)(uint64_t val[], + const struct feat_node *feat, + unsigned int old_cos); + int (*set_new_val)(uint64_t val[], + const struct feat_node *feat, + unsigned int old_cos, + enum cbm_type type, + uint64_t m); }; @@ -186,6 +215,29 @@ static void free_feature(struct psr_socket_info *info) } } +static bool_t psr_check_cbm(unsigned int cbm_len, uint64_t cbm) +{ + unsigned int first_bit, zero_bit; + + /* Set bits should only in the range of [0, cbm_len). */ + if ( cbm & (~0ull << cbm_len) ) + return 0; + + /* At least one bit need to be set. */ + if ( cbm == 0 ) + return 0; + + first_bit = find_first_bit(&cbm, cbm_len); + zero_bit = find_next_zero_bit(&cbm, cbm_len, first_bit); + + /* Set bits should be contiguous. */ + if ( zero_bit < cbm_len && + find_next_bit(&cbm, cbm_len, zero_bit) < cbm_len ) + return 0; + + return 1; +} + /* L3 CAT callback functions implementation. */ static void l3_cat_init_feature(unsigned int eax, unsigned int ebx, unsigned int ecx, unsigned int edx, @@ -255,11 +307,53 @@ static bool l3_cat_get_val(const struct feat_node *feat, unsigned int cos, return true; } +static unsigned int l3_cat_get_cos_num(const struct feat_node *feat) +{ + return 1; +} + +static int l3_cat_get_old_val(uint64_t val[], + const struct feat_node *feat, + unsigned int old_cos) +{ + if ( old_cos > feat->info.l3_cat_info.cos_max ) + /* Use default value. */ + old_cos = 0; + + /* CAT */ + val[0] = feat->cos_reg_val[old_cos]; + + /* L3 CAT uses one COS. */ + return 1; +} + +static int l3_cat_set_new_val(uint64_t val[], + const struct feat_node *feat, + unsigned int old_cos, + enum cbm_type type, + uint64_t m) +{ + if ( type != PSR_CBM_TYPE_L3 ) + /* L3 CAT uses one COS. Skip it. */ + return 1; + + if ( !psr_check_cbm(feat->info.l3_cat_info.cbm_len, m) ) + return -EINVAL; + + val[0] = m; + + /* 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, .get_feat_info = l3_cat_get_feat_info, .get_val = l3_cat_get_val, + .get_cos_num = l3_cat_get_cos_num, + .get_old_val = l3_cat_get_old_val, + .set_new_val = l3_cat_set_new_val, }; static void __init parse_psr_bool(char *s, char *value, char *feature, @@ -516,7 +610,14 @@ int psr_get_val(struct domain *d, unsigned int socket, /* Set value functions */ static unsigned int get_cos_num(const struct psr_socket_info *info) { - return 0; + const struct feat_node *feat_tmp; + unsigned int num = 0; + + /* Get all features total amount. */ + list_for_each_entry(feat_tmp, &info->feat_list, list) + num += feat_tmp->ops.get_cos_num(feat_tmp); + + return num; } static int get_old_set_new(uint64_t *val, @@ -526,6 +627,38 @@ static int get_old_set_new(uint64_t *val, enum cbm_type type, uint64_t m) { + const struct feat_node *feat_tmp; + int ret; + uint64_t *val_tmp = val; + + if ( !val ) + return -EINVAL; + + /* Get all features current values according to old_cos. */ + list_for_each_entry(feat_tmp, &info->feat_list, list) + { + /* value getting order is same as feature list */ + ret = feat_tmp->ops.get_old_val(val_tmp, feat_tmp, old_cos); + + val_tmp += ret; + if ( val_tmp - val > array_len) + return -EINVAL; + } + + /* Set new value into array according to feature's position in array. */ + val_tmp = val; + list_for_each_entry(feat_tmp, &info->feat_list, list) + { + /* value setting order is same as feature list */ + ret = feat_tmp->ops.set_new_val(val_tmp, feat_tmp, old_cos, type, m); + if ( ret < 0 ) + return ret; + + val_tmp += ret; + if ( val_tmp - val > array_len) + return -EINVAL; + } + return 0; }
Only can one COS ID be used by one domain at one time. That means all enabled features' COS registers at this COS ID are valid for this domain at that time. When user updates a feature's value, we need make sure all other features' values are not affected. So, we firstly need assemble an array which contains all features current values and replace the setting feature's value in array to new value. Then, we can try to find if there is a COS ID on which all features' COS registers values are same as the array. If we can find, we just use this COS ID. If fail to find, we need allocate a new COS ID. This patch implements value array assembling flow. Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com> --- xen/arch/x86/psr.c | 135 ++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 134 insertions(+), 1 deletion(-)