| Message ID | 1489662495-5375-11-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 > @@ -101,6 +101,28 @@ struct feat_node { > /* get_val is used to get feature COS register value. */ > void (*get_val)(const struct feat_node *feat, unsigned int cos, > enum cbm_type type, uint32_t *val); > + > + /* > + * get_old_val and set_new_val are a pair of functions called in order. > + * The caller will traverse all features in the array and call > + * 'get_old_val' to get old_cos register value of all supported > + * features. Then, call 'set_new_val' to set the new value for the > + * designated feature. > + * > + * All the values are set into value array according to the traversal > + * order, meaning the same order of feature array members. > + * > + * The return value meaning of set_new_val: > + * 0 - success. > + * negative - error. > + */ > + void (*get_old_val)(uint32_t val[], > + const struct feat_node *feat, > + unsigned int old_cos); > + int (*set_new_val)(uint32_t val[], > + const struct feat_node *feat, > + enum cbm_type type, > + uint32_t new_val); Along the lines of an earlier comment - are "old" and "new" really meaningful here? > @@ -212,6 +234,29 @@ static enum psr_feat_type psr_cbm_type_to_feat_type(enum cbm_type type) > } > > /* CAT common functions implementation. */ > +static bool psr_check_cbm(unsigned int cbm_len, uint32_t cbm) > +{ > + unsigned int first_bit, zero_bit; > + > + /* Set bits should only in the range of [0, cbm_len]. */ > + if ( cbm & (~0ul << cbm_len) ) Same question as elsewhere about the use of the ul suffix here: Can cbm_len really be any value in [0,32]? If not, I don't see why the calculation needs to be done as unsigned long. Otoh ... > + return false; > + > + /* At least one bit need to be set. */ > + if ( cbm == 0 ) > + return false; > + > + first_bit = find_first_bit((uint64_t *)&cbm, cbm_len); > + zero_bit = find_next_zero_bit((uint64_t *)&cbm, cbm_len, first_bit); ... these bogus casts suggest that the function would best have an "unsigned long" parameter. > @@ -285,11 +330,35 @@ static void cat_get_val(const struct feat_node *feat, unsigned int cos, > *val = feat->cos_reg_val[cos]; > } > > +/* val[] len checking is done by caller. */ > +static void cat_get_old_val(uint32_t val[], > + const struct feat_node *feat, > + unsigned int old_cos) > +{ > + cat_get_val(feat, old_cos, 0, &val[0]); > +} > + > +/* val[] len checking is done by caller. */ > +static int cat_set_new_val(uint32_t val[], > + const struct feat_node *feat, > + enum cbm_type type, > + uint32_t new_val) > +{ > + if ( !psr_check_cbm(feat->info.cat_info.cbm_len, new_val) ) > + return -EINVAL; > + > + val[0] = new_val; > + > + return 0; > +} > + > /* L3 CAT ops */ > static const struct feat_ops l3_cat_ops = { > .get_cos_max = cat_get_cos_max, > .get_feat_info = cat_get_feat_info, > .get_val = cat_get_val, > + .get_old_val = cat_get_old_val, > + .set_new_val = cat_set_new_val, > }; > > static void __init parse_psr_bool(char *s, char *value, char *feature, > @@ -581,7 +650,21 @@ 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; > + unsigned int num = 0, i; > + > + /* Get all features total amount. */ > + for ( i = 0; i < PSR_SOCKET_MAX_FEAT; i++ ) > + { > + if ( !info->features[i] ) > + continue; > + > + feat = info->features[i]; > + > + num += feat->cos_num; > + } > + > + return num; > } > > static int gather_val_array(uint32_t val[], > @@ -589,7 +672,34 @@ static int gather_val_array(uint32_t val[], > const struct psr_socket_info *info, > unsigned int old_cos) > { > - return -EINVAL; > + const struct feat_node *feat; > + unsigned int i; > + > + if ( !val ) > + return -EINVAL; > + > + /* Get all features current values according to old_cos. */ > + for ( i = 0; i < PSR_SOCKET_MAX_FEAT; i++ ) > + { > + if ( !info->features[i] ) > + continue; > + > + feat = info->features[i]; > + > + if ( old_cos > feat->ops.get_cos_max(feat) ) > + old_cos = 0; > + > + /* value getting order is same as feature array */ > + feat->ops.get_old_val(val, feat, old_cos); > + > + array_len -= feat->cos_num; So this I should really have asked about on a much earlier patch, but I've recognize the oddity only now: Why is cos_num per-feature-node instead of per-feature? This should really be a field in struct feat_ops (albeit the name "ops" then will be slightly misleading, but I think that's tolerable if you can't think of a better name). > + if ( array_len < 0 ) > + return -ENOSPC; This check needs doing earlier - you need to make sure array_len >= ops.cos_num prior to calling ops.get_old_val(). (Doing the check after the subtraction even causes wrapping issues, which are even more visible in similar code further down.) > @@ -599,7 +709,43 @@ static int insert_new_val_to_array(uint32_t val[], > enum cbm_type type, > uint32_t new_val) > { > - return -EINVAL; > + const struct feat_node *feat; > + int ret; > + unsigned int i; > + > + ASSERT(feat_type < PSR_SOCKET_MAX_FEAT); > + > + /* Set new value into array according to feature's position in array. */ > + for ( i = 0; i < feat_type; i++ ) > + { > + if ( !info->features[i] ) > + continue; > + > + feat = info->features[i]; > + > + array_len -= feat->cos_num; > + if ( array_len <= 0 ) > + return -ENOSPC; > + > + val += feat->cos_num; > + } > + > + feat = info->features[feat_type]; > + > + array_len -= feat->cos_num; > + if ( array_len < 0 ) > + return -ENOSPC; > + > + /* > + * Value setting position is same as feature array. > + * Different features may have different setting behaviors, e.g. CDP > + * has two values (DATA/CODE) which need us to save input value to > + * different position in the array according to type, so we have to > + * maintain a callback function. > + */ > + ret = feat->ops.set_new_val(val, feat, type, new_val); > + > + return ret; Again a case of a pointless intermediate variable. Jan
On 17-03-27 04:17:28, 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 > > @@ -101,6 +101,28 @@ struct feat_node { > > /* get_val is used to get feature COS register value. */ > > void (*get_val)(const struct feat_node *feat, unsigned int cos, > > enum cbm_type type, uint32_t *val); > > + > > + /* > > + * get_old_val and set_new_val are a pair of functions called in order. > > + * The caller will traverse all features in the array and call > > + * 'get_old_val' to get old_cos register value of all supported > > + * features. Then, call 'set_new_val' to set the new value for the > > + * designated feature. > > + * > > + * All the values are set into value array according to the traversal > > + * order, meaning the same order of feature array members. > > + * > > + * The return value meaning of set_new_val: > > + * 0 - success. > > + * negative - error. > > + */ > > + void (*get_old_val)(uint32_t val[], > > + const struct feat_node *feat, > > + unsigned int old_cos); > > + int (*set_new_val)(uint32_t val[], > > + const struct feat_node *feat, > > + enum cbm_type type, > > + uint32_t new_val); > > Along the lines of an earlier comment - are "old" and "new" really > meaningful here? > Maybe 'old' is not accurate. How about 'current'? In fact, we use this function to get domain's current CBM value. Furthermore, this is to distinguish 'get_val' which is declared above. I think 'new' is meaningful to express we are setting the newly input value. How do you think? Thanks! > > @@ -212,6 +234,29 @@ static enum psr_feat_type psr_cbm_type_to_feat_type(enum cbm_type type) > > } > > > > /* CAT common functions implementation. */ > > +static bool psr_check_cbm(unsigned int cbm_len, uint32_t cbm) > > +{ > > + unsigned int first_bit, zero_bit; > > + > > + /* Set bits should only in the range of [0, cbm_len]. */ > > + if ( cbm & (~0ul << cbm_len) ) > > Same question as elsewhere about the use of the ul suffix here: > Can cbm_len really be any value in [0,32]? If not, I don't see > why the calculation needs to be done as unsigned long. Otoh ... > cbm_len is got as below: #define CAT_CBM_LEN_MASK 0x1f cat.cbm_len = (regs.a & CAT_CBM_LEN_MASK) + 1; So its max value is 32. > > + return false; > > + > > + /* At least one bit need to be set. */ > > + if ( cbm == 0 ) > > + return false; > > + > > + first_bit = find_first_bit((uint64_t *)&cbm, cbm_len); > > + zero_bit = find_next_zero_bit((uint64_t *)&cbm, cbm_len, first_bit); > > ... these bogus casts suggest that the function would best have > an "unsigned long" parameter. > I would like to modify 'cbm' type to 'uint64_t'. Use a local variable in caller to do the type conversion. > > @@ -285,11 +330,35 @@ static void cat_get_val(const struct feat_node *feat, unsigned int cos, > > *val = feat->cos_reg_val[cos]; > > } > > [...] > > static int gather_val_array(uint32_t val[], > > @@ -589,7 +672,34 @@ static int gather_val_array(uint32_t val[], > > const struct psr_socket_info *info, > > unsigned int old_cos) > > { > > - return -EINVAL; > > + const struct feat_node *feat; > > + unsigned int i; > > + > > + if ( !val ) > > + return -EINVAL; > > + > > + /* Get all features current values according to old_cos. */ > > + for ( i = 0; i < PSR_SOCKET_MAX_FEAT; i++ ) > > + { > > + if ( !info->features[i] ) > > + continue; > > + > > + feat = info->features[i]; > > + > > + if ( old_cos > feat->ops.get_cos_max(feat) ) > > + old_cos = 0; > > + > > + /* value getting order is same as feature array */ > > + feat->ops.get_old_val(val, feat, old_cos); > > + > > + array_len -= feat->cos_num; > > So this I should really have asked about on a much earlier patch, > but I've recognize the oddity only now: Why is cos_num > per-feature-node instead of per-feature? This should really be a > field in struct feat_ops (albeit the name "ops" then will be slightly > misleading, but I think that's tolerable if you can't think of a better > name). > Ok, I got your meaning. How about 'feat_props'? No matter operations or variables are all properties of the feature. > > + if ( array_len < 0 ) > > + return -ENOSPC; > > This check needs doing earlier - you need to make sure array_len > >= ops.cos_num prior to calling ops.get_old_val(). (Doing the > check after the subtraction even causes wrapping issues, which > are even more visible in similar code further down.) > Thanks for the suggestion! Will move 'array_len >= cos_num' check prior to call 'get_old_val'. > > @@ -599,7 +709,43 @@ static int insert_new_val_to_array(uint32_t val[], > > enum cbm_type type, > > uint32_t new_val) > > { > > - return -EINVAL; > > + const struct feat_node *feat; > > + int ret; > > + unsigned int i; > > + > > + ASSERT(feat_type < PSR_SOCKET_MAX_FEAT); > > + > > + /* Set new value into array according to feature's position in array. */ > > + for ( i = 0; i < feat_type; i++ ) > > + { > > + if ( !info->features[i] ) > > + continue; > > + > > + feat = info->features[i]; > > + > > + array_len -= feat->cos_num; > > + if ( array_len <= 0 ) > > + return -ENOSPC; > > + > > + val += feat->cos_num; > > + } > > + > > + feat = info->features[feat_type]; > > + > > + array_len -= feat->cos_num; > > + if ( array_len < 0 ) > > + return -ENOSPC; > > + > > + /* > > + * Value setting position is same as feature array. > > + * Different features may have different setting behaviors, e.g. CDP > > + * has two values (DATA/CODE) which need us to save input value to > > + * different position in the array according to type, so we have to > > + * maintain a callback function. > > + */ > > + ret = feat->ops.set_new_val(val, feat, type, new_val); > > + > > + return ret; > > Again a case of a pointless intermediate variable. > Will remove it. > Jan
On 17-03-28 11:12:43, Yi Sun wrote: > On 17-03-27 04:17:28, 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 gather_val_array(uint32_t val[], > > > @@ -589,7 +672,34 @@ static int gather_val_array(uint32_t val[], > > > const struct psr_socket_info *info, > > > unsigned int old_cos) > > > { > > > - return -EINVAL; > > > + const struct feat_node *feat; > > > + unsigned int i; > > > + > > > + if ( !val ) > > > + return -EINVAL; > > > + > > > + /* Get all features current values according to old_cos. */ > > > + for ( i = 0; i < PSR_SOCKET_MAX_FEAT; i++ ) > > > + { > > > + if ( !info->features[i] ) > > > + continue; > > > + > > > + feat = info->features[i]; > > > + > > > + if ( old_cos > feat->ops.get_cos_max(feat) ) > > > + old_cos = 0; > > > + > > > + /* value getting order is same as feature array */ > > > + feat->ops.get_old_val(val, feat, old_cos); > > > + > > > + array_len -= feat->cos_num; > > > > So this I should really have asked about on a much earlier patch, > > but I've recognize the oddity only now: Why is cos_num > > per-feature-node instead of per-feature? This should really be a > > field in struct feat_ops (albeit the name "ops" then will be slightly > > misleading, but I think that's tolerable if you can't think of a better > > name). > > > Ok, I got your meaning. How about 'feat_props'? No matter operations or > variables are all properties of the feature. > One more thing here. If we move 'cos_max' into 'feat_ops', we cannot declare 'feat_ops' as const. Because we have to assign value to 'cos_max' in cat_init_feature().
>>> On 28.03.17 at 05:12, <yi.y.sun@linux.intel.com> wrote: > On 17-03-27 04:17:28, 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 >> > @@ -101,6 +101,28 @@ struct feat_node { >> > /* get_val is used to get feature COS register value. */ >> > void (*get_val)(const struct feat_node *feat, unsigned int cos, >> > enum cbm_type type, uint32_t *val); >> > + >> > + /* >> > + * get_old_val and set_new_val are a pair of functions called in order. >> > + * The caller will traverse all features in the array and call >> > + * 'get_old_val' to get old_cos register value of all supported >> > + * features. Then, call 'set_new_val' to set the new value for the >> > + * designated feature. >> > + * >> > + * All the values are set into value array according to the traversal >> > + * order, meaning the same order of feature array members. >> > + * >> > + * The return value meaning of set_new_val: >> > + * 0 - success. >> > + * negative - error. >> > + */ >> > + void (*get_old_val)(uint32_t val[], >> > + const struct feat_node *feat, >> > + unsigned int old_cos); >> > + int (*set_new_val)(uint32_t val[], >> > + const struct feat_node *feat, >> > + enum cbm_type type, >> > + uint32_t new_val); >> >> Along the lines of an earlier comment - are "old" and "new" really >> meaningful here? >> > Maybe 'old' is not accurate. How about 'current'? In fact, we use this > function to get domain's current CBM value. Furthermore, this is to distinguish > 'get_val' which is declared above. I'm fine with "current", but the name collision - would "current" be omitted still bothers me. The fact that cat_get_old_val() calls cat_get_val(), however, strongly suggests that the hook here is redundant anyway. Even in the CDP case I think you can get away without it, but if this turns out really impossible or clumsy, then the hook could be introduced there (with a better name) and be an optional one (with the caller using ->get_val() if the one here is NULL). > I think 'new' is meaningful to express we are setting the newly input value. Well, this is the meaning to its caller. The function itself doesn't care whether the value is a new one, or just some other value coming from an unnamed source. >> > @@ -212,6 +234,29 @@ static enum psr_feat_type psr_cbm_type_to_feat_type(enum cbm_type type) >> > } >> > >> > /* CAT common functions implementation. */ >> > +static bool psr_check_cbm(unsigned int cbm_len, uint32_t cbm) >> > +{ >> > + unsigned int first_bit, zero_bit; >> > + >> > + /* Set bits should only in the range of [0, cbm_len]. */ >> > + if ( cbm & (~0ul << cbm_len) ) >> >> Same question as elsewhere about the use of the ul suffix here: >> Can cbm_len really be any value in [0,32]? If not, I don't see >> why the calculation needs to be done as unsigned long. Otoh ... >> > cbm_len is got as below: > #define CAT_CBM_LEN_MASK 0x1f > cat.cbm_len = (regs.a & CAT_CBM_LEN_MASK) + 1; > > So its max value is 32. And its min one is 1. I.e. no need for an unsigned long calculation. >> > + return false; >> > + >> > + /* At least one bit need to be set. */ >> > + if ( cbm == 0 ) >> > + return false; >> > + >> > + first_bit = find_first_bit((uint64_t *)&cbm, cbm_len); >> > + zero_bit = find_next_zero_bit((uint64_t *)&cbm, cbm_len, first_bit); >> >> ... these bogus casts suggest that the function would best have >> an "unsigned long" parameter. >> > I would like to modify 'cbm' type to 'uint64_t'. Use a local variable in caller > to do the type conversion. Why uint64_t? As said elsewhere, please stay away from fixed width types if you don't really need them. >> > @@ -589,7 +672,34 @@ static int gather_val_array(uint32_t val[], >> > const struct psr_socket_info *info, >> > unsigned int old_cos) >> > { >> > - return -EINVAL; >> > + const struct feat_node *feat; >> > + unsigned int i; >> > + >> > + if ( !val ) >> > + return -EINVAL; >> > + >> > + /* Get all features current values according to old_cos. */ >> > + for ( i = 0; i < PSR_SOCKET_MAX_FEAT; i++ ) >> > + { >> > + if ( !info->features[i] ) >> > + continue; >> > + >> > + feat = info->features[i]; >> > + >> > + if ( old_cos > feat->ops.get_cos_max(feat) ) >> > + old_cos = 0; >> > + >> > + /* value getting order is same as feature array */ >> > + feat->ops.get_old_val(val, feat, old_cos); >> > + >> > + array_len -= feat->cos_num; >> >> So this I should really have asked about on a much earlier patch, >> but I've recognize the oddity only now: Why is cos_num >> per-feature-node instead of per-feature? This should really be a >> field in struct feat_ops (albeit the name "ops" then will be slightly >> misleading, but I think that's tolerable if you can't think of a better >> name). >> > Ok, I got your meaning. How about 'feat_props'? No matter operations or > variables are all properties of the feature. "props" sounds fine to me. >> > + if ( array_len < 0 ) >> > + return -ENOSPC; >> >> This check needs doing earlier - you need to make sure array_len >> >= ops.cos_num prior to calling ops.get_old_val(). (Doing the >> check after the subtraction even causes wrapping issues, which >> are even more visible in similar code further down.) >> > Thanks for the suggestion! Will move 'array_len >= cos_num' check prior to > call 'get_old_val'. And don't forget to do this everywhere in the series - there were quite a few similar constructs in later patches, iirc. Jan
>>> On 28.03.17 at 10:05, <yi.y.sun@linux.intel.com> wrote: > On 17-03-28 11:12:43, Yi Sun wrote: >> On 17-03-27 04:17:28, 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 gather_val_array(uint32_t val[], >> > > @@ -589,7 +672,34 @@ static int gather_val_array(uint32_t val[], >> > > const struct psr_socket_info *info, >> > > unsigned int old_cos) >> > > { >> > > - return -EINVAL; >> > > + const struct feat_node *feat; >> > > + unsigned int i; >> > > + >> > > + if ( !val ) >> > > + return -EINVAL; >> > > + >> > > + /* Get all features current values according to old_cos. */ >> > > + for ( i = 0; i < PSR_SOCKET_MAX_FEAT; i++ ) >> > > + { >> > > + if ( !info->features[i] ) >> > > + continue; >> > > + >> > > + feat = info->features[i]; >> > > + >> > > + if ( old_cos > feat->ops.get_cos_max(feat) ) >> > > + old_cos = 0; >> > > + >> > > + /* value getting order is same as feature array */ >> > > + feat->ops.get_old_val(val, feat, old_cos); >> > > + >> > > + array_len -= feat->cos_num; >> > >> > So this I should really have asked about on a much earlier patch, >> > but I've recognize the oddity only now: Why is cos_num >> > per-feature-node instead of per-feature? This should really be a >> > field in struct feat_ops (albeit the name "ops" then will be slightly >> > misleading, but I think that's tolerable if you can't think of a better >> > name). >> > >> Ok, I got your meaning. How about 'feat_props'? No matter operations or >> variables are all properties of the feature. >> > One more thing here. If we move 'cos_max' into 'feat_ops', we cannot declare > 'feat_ops' as const. Because we have to assign value to 'cos_max' in > cat_init_feature(). I don't see a problem with this. It's only the static variable which can't be const then anymore. The pointer used everywhere else easily can be, afaict. Jan
On 17-03-28 02:36:05, Jan Beulich wrote: > >>> On 28.03.17 at 10:05, <yi.y.sun@linux.intel.com> wrote: > > On 17-03-28 11:12:43, Yi Sun wrote: > >> On 17-03-27 04:17:28, 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 gather_val_array(uint32_t val[], > >> > > @@ -589,7 +672,34 @@ static int gather_val_array(uint32_t val[], > >> > > const struct psr_socket_info *info, > >> > > unsigned int old_cos) > >> > > { > >> > > - return -EINVAL; > >> > > + const struct feat_node *feat; > >> > > + unsigned int i; > >> > > + > >> > > + if ( !val ) > >> > > + return -EINVAL; > >> > > + > >> > > + /* Get all features current values according to old_cos. */ > >> > > + for ( i = 0; i < PSR_SOCKET_MAX_FEAT; i++ ) > >> > > + { > >> > > + if ( !info->features[i] ) > >> > > + continue; > >> > > + > >> > > + feat = info->features[i]; > >> > > + > >> > > + if ( old_cos > feat->ops.get_cos_max(feat) ) > >> > > + old_cos = 0; > >> > > + > >> > > + /* value getting order is same as feature array */ > >> > > + feat->ops.get_old_val(val, feat, old_cos); > >> > > + > >> > > + array_len -= feat->cos_num; > >> > > >> > So this I should really have asked about on a much earlier patch, > >> > but I've recognize the oddity only now: Why is cos_num > >> > per-feature-node instead of per-feature? This should really be a > >> > field in struct feat_ops (albeit the name "ops" then will be slightly > >> > misleading, but I think that's tolerable if you can't think of a better > >> > name). > >> > > >> Ok, I got your meaning. How about 'feat_props'? No matter operations or > >> variables are all properties of the feature. > >> > > One more thing here. If we move 'cos_max' into 'feat_ops', we cannot declare > > 'feat_ops' as const. Because we have to assign value to 'cos_max' in > > cat_init_feature(). > > I don't see a problem with this. It's only the static variable which > can't be const then anymore. The pointer used everywhere else > easily can be, afaict. > Because I want to assign the l3_cat_props to feat->props before executing cat_init_feature(). The codes sequence is below. Then, in cat_init_feature(), I can use 'feat' but not 'l3_cat_props' which is feature specific. static void cat_init_feature(...) { ...... feat->info.cbm_len = (regs->a & CAT_CBM_LEN_MASK) + 1; feat->props->cos_max = min(opt_cos_max, regs->d & CAT_COS_MAX_MASK); ...... } static struct feat_props l3_cat_props = { .cos_num = 1, }; static void psr_cpu_init(void) { ...... feat->props = &l3_cat_props; cat_init_feature(®s, feat, info, PSR_SOCKET_L3_CAT); ...... } Then, back to the origin of this. I think feature-node is feature itself. Everything in it is feature specific thing. Is it necessary to move values into a sub-structure, 'feat_props'? If not doing this, we can keep 'feat_ops' to only handle callback functions. Sorry that I output my thought until now.
>>> On 28.03.17 at 11:11, <yi.y.sun@linux.intel.com> wrote: > On 17-03-28 02:36:05, Jan Beulich wrote: >> >>> On 28.03.17 at 10:05, <yi.y.sun@linux.intel.com> wrote: >> > On 17-03-28 11:12:43, Yi Sun wrote: >> >> On 17-03-27 04:17:28, 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 gather_val_array(uint32_t val[], >> >> > > @@ -589,7 +672,34 @@ static int gather_val_array(uint32_t val[], >> >> > > const struct psr_socket_info *info, >> >> > > unsigned int old_cos) >> >> > > { >> >> > > - return -EINVAL; >> >> > > + const struct feat_node *feat; >> >> > > + unsigned int i; >> >> > > + >> >> > > + if ( !val ) >> >> > > + return -EINVAL; >> >> > > + >> >> > > + /* Get all features current values according to old_cos. */ >> >> > > + for ( i = 0; i < PSR_SOCKET_MAX_FEAT; i++ ) >> >> > > + { >> >> > > + if ( !info->features[i] ) >> >> > > + continue; >> >> > > + >> >> > > + feat = info->features[i]; >> >> > > + >> >> > > + if ( old_cos > feat->ops.get_cos_max(feat) ) >> >> > > + old_cos = 0; >> >> > > + >> >> > > + /* value getting order is same as feature array */ >> >> > > + feat->ops.get_old_val(val, feat, old_cos); >> >> > > + >> >> > > + array_len -= feat->cos_num; >> >> > >> >> > So this I should really have asked about on a much earlier patch, >> >> > but I've recognize the oddity only now: Why is cos_num >> >> > per-feature-node instead of per-feature? This should really be a >> >> > field in struct feat_ops (albeit the name "ops" then will be slightly >> >> > misleading, but I think that's tolerable if you can't think of a better >> >> > name). >> >> > >> >> Ok, I got your meaning. How about 'feat_props'? No matter operations or >> >> variables are all properties of the feature. >> >> >> > One more thing here. If we move 'cos_max' into 'feat_ops', we cannot > declare >> > 'feat_ops' as const. Because we have to assign value to 'cos_max' in >> > cat_init_feature(). >> >> I don't see a problem with this. It's only the static variable which >> can't be const then anymore. The pointer used everywhere else >> easily can be, afaict. >> > Because I want to assign the l3_cat_props to feat->props before executing > cat_init_feature(). The codes sequence is below. Then, in > cat_init_feature(), > I can use 'feat' but not 'l3_cat_props' which is feature specific. > > static void cat_init_feature(...) > { > ...... > feat->info.cbm_len = (regs->a & CAT_CBM_LEN_MASK) + 1; > feat->props->cos_max = min(opt_cos_max, regs->d & CAT_COS_MAX_MASK); > ...... > } > > static struct feat_props l3_cat_props = { > .cos_num = 1, > }; > > static void psr_cpu_init(void) > { > ...... > feat->props = &l3_cat_props; > cat_init_feature(®s, feat, info, PSR_SOCKET_L3_CAT); > ...... > } static void psr_cpu_init(void) { ...... cat_init_feature(®s, &l3_cat_props, feat, info, PSR_SOCKET_L3_CAT); feat->props = &l3_cat_props; ...... } > Then, back to the origin of this. I think feature-node is feature itself. > Everything in it is feature specific thing. Is it necessary to move values > into a sub-structure, 'feat_props'? If not doing this, we can keep > 'feat_ops' to only handle callback functions. I'm not sure I understand what you're trying to tell me. I can only repeat what I've said before: The amount of feature specific callbacks should be reduced to the minimum necessary - the more generic code, the less code overall to maintain. Jan
On 17-03-28 02:34:51, Jan Beulich wrote: > >>> On 28.03.17 at 05:12, <yi.y.sun@linux.intel.com> wrote: > > On 17-03-27 04:17:28, 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 > >> > @@ -101,6 +101,28 @@ struct feat_node { > >> > /* get_val is used to get feature COS register value. */ > >> > void (*get_val)(const struct feat_node *feat, unsigned int cos, > >> > enum cbm_type type, uint32_t *val); > >> > + > >> > + /* > >> > + * get_old_val and set_new_val are a pair of functions called in order. > >> > + * The caller will traverse all features in the array and call > >> > + * 'get_old_val' to get old_cos register value of all supported > >> > + * features. Then, call 'set_new_val' to set the new value for the > >> > + * designated feature. > >> > + * > >> > + * All the values are set into value array according to the traversal > >> > + * order, meaning the same order of feature array members. > >> > + * > >> > + * The return value meaning of set_new_val: > >> > + * 0 - success. > >> > + * negative - error. > >> > + */ > >> > + void (*get_old_val)(uint32_t val[], > >> > + const struct feat_node *feat, > >> > + unsigned int old_cos); > >> > + int (*set_new_val)(uint32_t val[], > >> > + const struct feat_node *feat, > >> > + enum cbm_type type, > >> > + uint32_t new_val); > >> > >> Along the lines of an earlier comment - are "old" and "new" really > >> meaningful here? > >> > > Maybe 'old' is not accurate. How about 'current'? In fact, we use this > > function to get domain's current CBM value. Furthermore, this is to distinguish > > 'get_val' which is declared above. > > I'm fine with "current", but the name collision - would "current" be > omitted still bothers me. The fact that cat_get_old_val() calls > cat_get_val(), however, strongly suggests that the hook here is > redundant anyway. Even in the CDP case I think you can get > away without it, but if this turns out really impossible or clumsy, > then the hook could be introduced there (with a better name) > and be an optional one (with the caller using ->get_val() if the > one here is NULL). > I am afraid we have to keep this hook. CDP uses this hook to get both CODE and DATA at same time. But CDP uses get_val() hook to get either CODE or DATA. So, they have different functionalitiy. I prefer to rename it to 'get_current_val'. I can make it optional hook. But the codes in caller look a little strange. E.g. static int gather_val_array() { ... if ( feat->ops.get_old_val ) feat->ops.get_old_val(val, feat, old_cos); else feat->ops.get_val(feat, old_cos, 0, &val[0]); ... } So, I think a wrapper like cat_get_old_val() may be a better choice. What is your opinion? > > I think 'new' is meaningful to express we are setting the newly input value. > > Well, this is the meaning to its caller. The function itself doesn't > care whether the value is a new one, or just some other value > coming from an unnamed source. > Ok, will remove 'new'.
On 17-03-28 03:20:05, Jan Beulich wrote: > >>> On 28.03.17 at 11:11, <yi.y.sun@linux.intel.com> wrote: > > On 17-03-28 02:36:05, Jan Beulich wrote: > >> >>> On 28.03.17 at 10:05, <yi.y.sun@linux.intel.com> wrote: > >> > On 17-03-28 11:12:43, Yi Sun wrote: > >> >> On 17-03-27 04:17:28, 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 gather_val_array(uint32_t val[], > >> >> > > @@ -589,7 +672,34 @@ static int gather_val_array(uint32_t val[], > >> >> > > const struct psr_socket_info *info, > >> >> > > unsigned int old_cos) > >> >> > > { > >> >> > > - return -EINVAL; > >> >> > > + const struct feat_node *feat; > >> >> > > + unsigned int i; > >> >> > > + > >> >> > > + if ( !val ) > >> >> > > + return -EINVAL; > >> >> > > + > >> >> > > + /* Get all features current values according to old_cos. */ > >> >> > > + for ( i = 0; i < PSR_SOCKET_MAX_FEAT; i++ ) > >> >> > > + { > >> >> > > + if ( !info->features[i] ) > >> >> > > + continue; > >> >> > > + > >> >> > > + feat = info->features[i]; > >> >> > > + > >> >> > > + if ( old_cos > feat->ops.get_cos_max(feat) ) > >> >> > > + old_cos = 0; > >> >> > > + > >> >> > > + /* value getting order is same as feature array */ > >> >> > > + feat->ops.get_old_val(val, feat, old_cos); > >> >> > > + > >> >> > > + array_len -= feat->cos_num; > >> >> > > >> >> > So this I should really have asked about on a much earlier patch, > >> >> > but I've recognize the oddity only now: Why is cos_num > >> >> > per-feature-node instead of per-feature? This should really be a > >> >> > field in struct feat_ops (albeit the name "ops" then will be slightly > >> >> > misleading, but I think that's tolerable if you can't think of a better > >> >> > name). > >> >> > > >> >> Ok, I got your meaning. How about 'feat_props'? No matter operations or > >> >> variables are all properties of the feature. > >> >> > >> > One more thing here. If we move 'cos_max' into 'feat_ops', we cannot > > declare > >> > 'feat_ops' as const. Because we have to assign value to 'cos_max' in > >> > cat_init_feature(). > >> > >> I don't see a problem with this. It's only the static variable which > >> can't be const then anymore. The pointer used everywhere else > >> easily can be, afaict. > >> > > Because I want to assign the l3_cat_props to feat->props before executing > > cat_init_feature(). The codes sequence is below. Then, in > > cat_init_feature(), > > I can use 'feat' but not 'l3_cat_props' which is feature specific. > > > > static void cat_init_feature(...) > > { > > ...... > > feat->info.cbm_len = (regs->a & CAT_CBM_LEN_MASK) + 1; > > feat->props->cos_max = min(opt_cos_max, regs->d & CAT_COS_MAX_MASK); > > ...... > > } > > > > static struct feat_props l3_cat_props = { > > .cos_num = 1, > > }; > > > > static void psr_cpu_init(void) > > { > > ...... > > feat->props = &l3_cat_props; > > cat_init_feature(®s, feat, info, PSR_SOCKET_L3_CAT); > > ...... > > } > > static void psr_cpu_init(void) > { > ...... > cat_init_feature(®s, &l3_cat_props, feat, info, PSR_SOCKET_L3_CAT); > feat->props = &l3_cat_props; > ...... > } > > > Then, back to the origin of this. I think feature-node is feature itself. > > Everything in it is feature specific thing. Is it necessary to move values > > into a sub-structure, 'feat_props'? If not doing this, we can keep > > 'feat_ops' to only handle callback functions. > > I'm not sure I understand what you're trying to tell me. I can only > repeat what I've said before: The amount of feature specific > callbacks should be reduced to the minimum necessary - the more > generic code, the less code overall to maintain. > My key point is: can we keep 'cos_num' and 'cos_max' into 'feat_node' but not 'feat_ops'? Because I think 'feat_node' represents a feature. It can keep all feature specific things. If you still think it is not good, can we define 'struct feat_props' without const? Then, I can keep above code sequence.
>>> On 28.03.17 at 12:12, <yi.y.sun@linux.intel.com> wrote: > On 17-03-28 02:34:51, Jan Beulich wrote: >> >>> On 28.03.17 at 05:12, <yi.y.sun@linux.intel.com> wrote: >> > On 17-03-27 04:17:28, 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 >> >> > @@ -101,6 +101,28 @@ struct feat_node { >> >> > /* get_val is used to get feature COS register value. */ >> >> > void (*get_val)(const struct feat_node *feat, unsigned int cos, >> >> > enum cbm_type type, uint32_t *val); >> >> > + >> >> > + /* >> >> > + * get_old_val and set_new_val are a pair of functions called in > order. >> >> > + * The caller will traverse all features in the array and call >> >> > + * 'get_old_val' to get old_cos register value of all supported >> >> > + * features. Then, call 'set_new_val' to set the new value for > the >> >> > + * designated feature. >> >> > + * >> >> > + * All the values are set into value array according to the > traversal >> >> > + * order, meaning the same order of feature array members. >> >> > + * >> >> > + * The return value meaning of set_new_val: >> >> > + * 0 - success. >> >> > + * negative - error. >> >> > + */ >> >> > + void (*get_old_val)(uint32_t val[], >> >> > + const struct feat_node *feat, >> >> > + unsigned int old_cos); >> >> > + int (*set_new_val)(uint32_t val[], >> >> > + const struct feat_node *feat, >> >> > + enum cbm_type type, >> >> > + uint32_t new_val); >> >> >> >> Along the lines of an earlier comment - are "old" and "new" really >> >> meaningful here? >> >> >> > Maybe 'old' is not accurate. How about 'current'? In fact, we use this >> > function to get domain's current CBM value. Furthermore, this is to > distinguish >> > 'get_val' which is declared above. >> >> I'm fine with "current", but the name collision - would "current" be >> omitted still bothers me. The fact that cat_get_old_val() calls >> cat_get_val(), however, strongly suggests that the hook here is >> redundant anyway. Even in the CDP case I think you can get >> away without it, but if this turns out really impossible or clumsy, >> then the hook could be introduced there (with a better name) >> and be an optional one (with the caller using ->get_val() if the >> one here is NULL). >> > I am afraid we have to keep this hook. CDP uses this hook to get both CODE and > DATA at same time. But CDP uses get_val() hook to get either CODE or DATA. > So, they have different functionalitiy. I prefer to rename it to > 'get_current_val'. > > I can make it optional hook. But the codes in caller look a little strange. > E.g. > static int gather_val_array() > { > ... > if ( feat->ops.get_old_val ) > feat->ops.get_old_val(val, feat, old_cos); > else > feat->ops.get_val(feat, old_cos, 0, &val[0]); > ... > } So it looks like I have to repeat myself here: The caller knows the number of slots. Make the get_val() hook flexible enough to allow it to replace get_old_val() here (the caller would then invoke it cos_num times). Jan
>>> On 28.03.17 at 12:18, <yi.y.sun@linux.intel.com> wrote: > On 17-03-28 03:20:05, Jan Beulich wrote: >> >>> On 28.03.17 at 11:11, <yi.y.sun@linux.intel.com> wrote: >> > On 17-03-28 02:36:05, Jan Beulich wrote: >> >> >>> On 28.03.17 at 10:05, <yi.y.sun@linux.intel.com> wrote: >> >> > On 17-03-28 11:12:43, Yi Sun wrote: >> >> >> On 17-03-27 04:17:28, 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 gather_val_array(uint32_t val[], >> >> >> > > @@ -589,7 +672,34 @@ static int gather_val_array(uint32_t val[], >> >> >> > > const struct psr_socket_info *info, >> >> >> > > unsigned int old_cos) >> >> >> > > { >> >> >> > > - return -EINVAL; >> >> >> > > + const struct feat_node *feat; >> >> >> > > + unsigned int i; >> >> >> > > + >> >> >> > > + if ( !val ) >> >> >> > > + return -EINVAL; >> >> >> > > + >> >> >> > > + /* Get all features current values according to old_cos. */ >> >> >> > > + for ( i = 0; i < PSR_SOCKET_MAX_FEAT; i++ ) >> >> >> > > + { >> >> >> > > + if ( !info->features[i] ) >> >> >> > > + continue; >> >> >> > > + >> >> >> > > + feat = info->features[i]; >> >> >> > > + >> >> >> > > + if ( old_cos > feat->ops.get_cos_max(feat) ) >> >> >> > > + old_cos = 0; >> >> >> > > + >> >> >> > > + /* value getting order is same as feature array */ >> >> >> > > + feat->ops.get_old_val(val, feat, old_cos); >> >> >> > > + >> >> >> > > + array_len -= feat->cos_num; >> >> >> > >> >> >> > So this I should really have asked about on a much earlier patch, >> >> >> > but I've recognize the oddity only now: Why is cos_num >> >> >> > per-feature-node instead of per-feature? This should really be a >> >> >> > field in struct feat_ops (albeit the name "ops" then will be slightly >> >> >> > misleading, but I think that's tolerable if you can't think of a better >> >> >> > name). >> >> >> > >> >> >> Ok, I got your meaning. How about 'feat_props'? No matter operations or >> >> >> variables are all properties of the feature. >> >> >> >> >> > One more thing here. If we move 'cos_max' into 'feat_ops', we cannot >> > declare >> >> > 'feat_ops' as const. Because we have to assign value to 'cos_max' in >> >> > cat_init_feature(). >> >> >> >> I don't see a problem with this. It's only the static variable which >> >> can't be const then anymore. The pointer used everywhere else >> >> easily can be, afaict. >> >> >> > Because I want to assign the l3_cat_props to feat->props before executing >> > cat_init_feature(). The codes sequence is below. Then, in >> > cat_init_feature(), >> > I can use 'feat' but not 'l3_cat_props' which is feature specific. >> > >> > static void cat_init_feature(...) >> > { >> > ...... >> > feat->info.cbm_len = (regs->a & CAT_CBM_LEN_MASK) + 1; >> > feat->props->cos_max = min(opt_cos_max, regs->d & CAT_COS_MAX_MASK); >> > ...... >> > } >> > >> > static struct feat_props l3_cat_props = { >> > .cos_num = 1, >> > }; >> > >> > static void psr_cpu_init(void) >> > { >> > ...... >> > feat->props = &l3_cat_props; >> > cat_init_feature(®s, feat, info, PSR_SOCKET_L3_CAT); >> > ...... >> > } >> >> static void psr_cpu_init(void) >> { >> ...... >> cat_init_feature(®s, &l3_cat_props, feat, info, PSR_SOCKET_L3_CAT); >> feat->props = &l3_cat_props; >> ...... >> } >> >> > Then, back to the origin of this. I think feature-node is feature itself. >> > Everything in it is feature specific thing. Is it necessary to move values >> > into a sub-structure, 'feat_props'? If not doing this, we can keep >> > 'feat_ops' to only handle callback functions. >> >> I'm not sure I understand what you're trying to tell me. I can only >> repeat what I've said before: The amount of feature specific >> callbacks should be reduced to the minimum necessary - the more >> generic code, the less code overall to maintain. >> > My key point is: can we keep 'cos_num' and 'cos_max' into 'feat_node' but not > 'feat_ops'? Because I think 'feat_node' represents a feature. It can keep > all feature specific things. Let me ask the question this way - for a given feature, can cos_max and cos_num vary between individual nodes created for this feature? If they can, the values need to be per-node. If they can't, there's no point in replicating them in every node. > If you still think it is not good, can we define 'struct feat_props' without > const? Then, I can keep above code sequence. Sure - I've already outlined how that would work with keeping const in most places. Jan
diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c index f8d4be4..18aad8e 100644 --- a/xen/arch/x86/psr.c +++ b/xen/arch/x86/psr.c @@ -101,6 +101,28 @@ struct feat_node { /* get_val is used to get feature COS register value. */ void (*get_val)(const struct feat_node *feat, unsigned int cos, enum cbm_type type, uint32_t *val); + + /* + * get_old_val and set_new_val are a pair of functions called in order. + * The caller will traverse all features in the array and call + * 'get_old_val' to get old_cos register value of all supported + * features. Then, call 'set_new_val' to set the new value for the + * designated feature. + * + * All the values are set into value array according to the traversal + * order, meaning the same order of feature array members. + * + * The return value meaning of set_new_val: + * 0 - success. + * negative - error. + */ + void (*get_old_val)(uint32_t val[], + const struct feat_node *feat, + unsigned int old_cos); + int (*set_new_val)(uint32_t val[], + const struct feat_node *feat, + enum cbm_type type, + uint32_t new_val); } ops; /* Encapsulate feature specific HW info here. */ @@ -212,6 +234,29 @@ static enum psr_feat_type psr_cbm_type_to_feat_type(enum cbm_type type) } /* CAT common functions implementation. */ +static bool psr_check_cbm(unsigned int cbm_len, uint32_t cbm) +{ + unsigned int first_bit, zero_bit; + + /* Set bits should only in the range of [0, cbm_len]. */ + if ( cbm & (~0ul << cbm_len) ) + return false; + + /* At least one bit need to be set. */ + if ( cbm == 0 ) + return false; + + first_bit = find_first_bit((uint64_t *)&cbm, cbm_len); + zero_bit = find_next_zero_bit((uint64_t *)&cbm, cbm_len, first_bit); + + /* Set bits should be contiguous. */ + if ( zero_bit < cbm_len && + find_next_bit((uint64_t *)&cbm, cbm_len, zero_bit) < cbm_len ) + return false; + + return true; +} + static void cat_init_feature(struct cpuid_leaf regs, struct feat_node *feat, struct psr_socket_info *info, @@ -285,11 +330,35 @@ static void cat_get_val(const struct feat_node *feat, unsigned int cos, *val = feat->cos_reg_val[cos]; } +/* val[] len checking is done by caller. */ +static void cat_get_old_val(uint32_t val[], + const struct feat_node *feat, + unsigned int old_cos) +{ + cat_get_val(feat, old_cos, 0, &val[0]); +} + +/* val[] len checking is done by caller. */ +static int cat_set_new_val(uint32_t val[], + const struct feat_node *feat, + enum cbm_type type, + uint32_t new_val) +{ + if ( !psr_check_cbm(feat->info.cat_info.cbm_len, new_val) ) + return -EINVAL; + + val[0] = new_val; + + return 0; +} + /* L3 CAT ops */ static const struct feat_ops l3_cat_ops = { .get_cos_max = cat_get_cos_max, .get_feat_info = cat_get_feat_info, .get_val = cat_get_val, + .get_old_val = cat_get_old_val, + .set_new_val = cat_set_new_val, }; static void __init parse_psr_bool(char *s, char *value, char *feature, @@ -581,7 +650,21 @@ 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; + unsigned int num = 0, i; + + /* Get all features total amount. */ + for ( i = 0; i < PSR_SOCKET_MAX_FEAT; i++ ) + { + if ( !info->features[i] ) + continue; + + feat = info->features[i]; + + num += feat->cos_num; + } + + return num; } static int gather_val_array(uint32_t val[], @@ -589,7 +672,34 @@ static int gather_val_array(uint32_t val[], const struct psr_socket_info *info, unsigned int old_cos) { - return -EINVAL; + const struct feat_node *feat; + unsigned int i; + + if ( !val ) + return -EINVAL; + + /* Get all features current values according to old_cos. */ + for ( i = 0; i < PSR_SOCKET_MAX_FEAT; i++ ) + { + if ( !info->features[i] ) + continue; + + feat = info->features[i]; + + if ( old_cos > feat->ops.get_cos_max(feat) ) + old_cos = 0; + + /* value getting order is same as feature array */ + feat->ops.get_old_val(val, feat, old_cos); + + array_len -= feat->cos_num; + if ( array_len < 0 ) + return -ENOSPC; + + val += feat->cos_num; + } + + return 0; } static int insert_new_val_to_array(uint32_t val[], @@ -599,7 +709,43 @@ static int insert_new_val_to_array(uint32_t val[], enum cbm_type type, uint32_t new_val) { - return -EINVAL; + const struct feat_node *feat; + int ret; + unsigned int i; + + ASSERT(feat_type < PSR_SOCKET_MAX_FEAT); + + /* Set new value into array according to feature's position in array. */ + for ( i = 0; i < feat_type; i++ ) + { + if ( !info->features[i] ) + continue; + + feat = info->features[i]; + + array_len -= feat->cos_num; + if ( array_len <= 0 ) + return -ENOSPC; + + val += feat->cos_num; + } + + feat = info->features[feat_type]; + + array_len -= feat->cos_num; + if ( array_len < 0 ) + return -ENOSPC; + + /* + * Value setting position is same as feature array. + * Different features may have different setting behaviors, e.g. CDP + * has two values (DATA/CODE) which need us to save input value to + * different position in the array according to type, so we have to + * maintain a callback function. + */ + ret = feat->ops.set_new_val(val, feat, type, new_val); + + return ret; } static int find_cos(const uint32_t val[], uint32_t array_len,
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 gather 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 pick a new COS ID. This patch implements value array assembling flow. Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com> --- v9: - add comments about boundary checking. (suggested by Wei Liu) - rename 'assemble_val_array' to 'combine_val_array' in pervious patch. (suggested by Wei Liu) - rename 'l3_cat_get_cos_num' to 'cat_get_cos_num' to cover all L3/L2 CAT features. (suggested by Roger Pau) - rename 'l3_cat_get_old_val' to 'cat_get_old_val' to cover all L3/L2 CAT features and reuse cat_get_val in it. (suggested by Roger Pau) - replace feature list handling to feature array handling. (suggested by Roger Pau) - modify patch title to indicate 'L3 CAT'. (suggested by Jan Beulich) - replace 'm' to 'new_val'. (suggested by Jan Beulich) - move cos check outside callback function. (suggested by Jan Beulich) - remove 'get_cos_num' callback function. (suggested by Jan Beulich) - changes about 'uint64_t' to 'uint32_t'. (suggested by Jan Beulich) v6: - change 'assemble_val_array' to 'combine_val_array'. (suggested by Konrad Rzeszutek Wilk) - check return value of 'get_old_val'. (suggested by Konrad Rzeszutek Wilk) - replace some 'EINVAL' to 'ENOSPC'. (suggested by Konrad Rzeszutek Wilk) v5: - modify comments according to changes of codes. (suggested by Jan Beulich) - change 'bool_t' to 'bool'. (suggested by Jan Beulich) - modify return value of callback functions because we do not need them to return number of entries the feature uses. In caller, we call 'get_cos_num' to get the number of entries the feature uses. (suggested by Jan Beulich) - modify variables names to make them better, e.g. 'feat_tmp' to 'feat'. (suggested by Jan Beulich) v4: - create this patch to make codes easier to understand. (suggested by Jan Beulich) --- xen/arch/x86/psr.c | 152 +++++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 149 insertions(+), 3 deletions(-)