| Message ID | 1487148579-7243-10-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:24PM +0800, Yi Sun wrote: [...] > > +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[], And the length of val is? How can you bound-check the access? But I *think* this is just a pointer to uint64_t, you can just use uint64_t *val here and *val = x; in code? Same comment applies to the set_new_val handler as well. > + 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]; > + > + return 0; > +} > + > +static int l3_cat_set_new_val(uint64_t val[], > + const struct feat_node *feat, > + enum cbm_type type, > + uint64_t m) > +{ > + if ( !psr_check_cbm(feat->info.l3_cat_info.cbm_len, m) ) > + return -EINVAL; > + > + val[0] = m; > + > + 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, > .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, > @@ -549,15 +633,42 @@ 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 assemble_val_array(uint64_t *val, > +static int combine_val_array(uint64_t *val, > uint32_t array_len, > const struct psr_socket_info *info, > unsigned int old_cos) Please just name this function combine_val_array in your previous patch instead of trying to rename it here. Or just don't change the name at all -- I don't see why changing name is necessary. Wei.
On 17-02-26 17:43:04, Wei Liu wrote: > On Wed, Feb 15, 2017 at 04:49:24PM +0800, Yi Sun wrote: > [...] > > > > +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[], > > And the length of val is? How can you bound-check the access? > > But I *think* this is just a pointer to uint64_t, you can just use > uint64_t *val here and *val = x; in code? > > Same comment applies to the set_new_val handler as well. > Such implementation is to simplify these functions. The bound-check will be done in caller functions, such as combine_val_array/set_new_val_to_array/etc. > > + 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]; > > + > > + return 0; > > +} > > + [...] > > -static int assemble_val_array(uint64_t *val, > > +static int combine_val_array(uint64_t *val, > > uint32_t array_len, > > const struct psr_socket_info *info, > > unsigned int old_cos) > > Please just name this function combine_val_array in your previous patch > instead of trying to rename it here. Or just don't change the name at > all -- I don't see why changing name is necessary. > Per Konrad's suggestion to change the name. Because he thought the 'assemble' is not accurate here. > Wei.
On Mon, Feb 27, 2017 at 03:11:35PM +0800, Yi Sun wrote: > On 17-02-26 17:43:04, Wei Liu wrote: > > On Wed, Feb 15, 2017 at 04:49:24PM +0800, Yi Sun wrote: > > [...] > > > > > > +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[], > > > > And the length of val is? How can you bound-check the access? > > > > But I *think* this is just a pointer to uint64_t, you can just use > > uint64_t *val here and *val = x; in code? > > > > Same comment applies to the set_new_val handler as well. > > > Such implementation is to simplify these functions. The bound-check will be > done in caller functions, such as combine_val_array/set_new_val_to_array/etc. > Please consider adding a comment to say bound-check is don't by the caller. > > > + 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]; > > > + > > > + return 0; > > > +} > > > + > [...] > > > -static int assemble_val_array(uint64_t *val, > > > +static int combine_val_array(uint64_t *val, > > > uint32_t array_len, > > > const struct psr_socket_info *info, > > > unsigned int old_cos) > > > > Please just name this function combine_val_array in your previous patch > > instead of trying to rename it here. Or just don't change the name at > > all -- I don't see why changing name is necessary. > > > Per Konrad's suggestion to change the name. Because he thought the 'assemble' > is not accurate here. > The such change should be inside the patch to introduce the framework, not in this patch. Wei. > > Wei.
>>> On 15.02.17 at 09:49, <yi.y.sun@linux.intel.com> wrote: > --- a/xen/arch/x86/psr.c > +++ b/xen/arch/x86/psr.c > @@ -119,6 +119,32 @@ 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); Please have blank lines ahead of every separating comment. (Of course this then may need to start already in earlier patches.) > + /* > + * get_old_val and set_new_val are a pair of functions called in order. > + * 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. This is misleading, I think: I don't think a new value is being set for every feature. This should be worded less ambiguously. > @@ -207,6 +233,29 @@ static enum psr_feat_type psr_cbm_type_to_feat_type(enum cbm_type type) > return feat_type; > } > > +static bool 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) ) This will not do what you intend for cbm_len == 64. > +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 ) Afaics this condition is the only L3 CAT specific thing in this function. Should more of it be moved into common code? Same below for set_new_val. > -static int assemble_val_array(uint64_t *val, > +static int combine_val_array(uint64_t *val, Same comment as earlier on - please decide for a final name right when introducing a function. In fact I'd prefer it to remain "assemble". > { > - return -EINVAL; > + const struct feat_node *feat; > + int ret; > + uint64_t *val_tmp = val; I don't really see the need for this helper variable. Simply ... > + if ( !val ) > + return -EINVAL; > + > + /* Get all features current values according to old_cos. */ > + list_for_each_entry(feat, &info->feat_list, list) > + { > + /* value getting order is same as feature list */ > + ret = feat->ops.get_old_val(val_tmp, feat, old_cos); > + if ( ret ) > + return ret; > + > + val_tmp += feat->ops.get_cos_num(feat); ... use val here, after checking the return value against array_len, and the also subtract from array_len. (I am averse to _tmp suffixes, I'm sorry.) Btw - for any of the later features, does their get_cos_num() ever return other than a constant value? If not, there's no point in making this a function call - you could simply have a numeric member in the structure. > @@ -567,7 +678,37 @@ static int set_new_val_to_array(uint64_t *val, > enum cbm_type type, > uint64_t m) > { > - return -EINVAL; > + const struct feat_node *feat; > + int ret; > + uint64_t *val_tmp = val; > + > + /* Set new value into array according to feature's position in array. */ > + list_for_each_entry(feat, &info->feat_list, list) > + { > + if ( feat->feature != feat_type ) > + { > + val_tmp += feat->ops.get_cos_num(feat); > + if ( val_tmp - val > array_len) > + return -ENOSPC; > + > + continue; > + } > + > + /* > + * Value setting position is same as feature list. > + * 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_tmp, feat, type, m); > + if ( ret ) > + return ret; > + else Pointless "else" again. I think I'll try to avoid pointing this out, should more of these appear - please simply go through yourself any remove any such uses. Jan
On 17-03-08 09:54:04, Jan Beulich wrote: > >>> On 15.02.17 at 09:49, <yi.y.sun@linux.intel.com> wrote: [...] > > + /* > > + * get_old_val and set_new_val are a pair of functions called in order. > > + * 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. > > This is misleading, I think: I don't think a new value is being set for > every feature. This should be worded less ambiguously. > The expression is not accurate. I will correct this. Thanks! > > @@ -207,6 +233,29 @@ static enum psr_feat_type psr_cbm_type_to_feat_type(enum cbm_type type) > > return feat_type; > > } > > > > +static bool 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) ) > > This will not do what you intend for cbm_len == 64. > cbm_len is not 64. cbm_len means the CBM value length, how many bits. For L3 CAT, it may be 11 bits. For L2 CAT, it may be 8 bits. > > +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 ) > > Afaics this condition is the only L3 CAT specific thing in this function. > Should more of it be moved into common code? Same below for > set_new_val. > Sorry, I may not understand your intention. For different features, they have different cos_max. Do you mean I should abstract a callback function for all features to handle this cos_max check? Thanks! > > -static int assemble_val_array(uint64_t *val, > > +static int combine_val_array(uint64_t *val, > > Same comment as earlier on - please decide for a final name right > when introducing a function. In fact I'd prefer it to remain > "assemble". > This is corrected in next version. I will change name back to assemble if you like it. > > { > > - return -EINVAL; > > + const struct feat_node *feat; > > + int ret; > > + uint64_t *val_tmp = val; > > I don't really see the need for this helper variable. Simply ... > > > + if ( !val ) > > + return -EINVAL; > > + > > + /* Get all features current values according to old_cos. */ > > + list_for_each_entry(feat, &info->feat_list, list) > > + { > > + /* value getting order is same as feature list */ > > + ret = feat->ops.get_old_val(val_tmp, feat, old_cos); > > + if ( ret ) > > + return ret; > > + > > + val_tmp += feat->ops.get_cos_num(feat); > > ... use val here, after checking the return value against > array_len, and the also subtract from array_len. (I am averse > to _tmp suffixes, I'm sorry.) > Ok, no problem. > Btw - for any of the later features, does their get_cos_num() ever > return other than a constant value? If not, there's no point in making > this a function call - you could simply have a numeric member in the > structure. > Good idea. Thanks! [...]
>>> On 10.03.17 at 04:21, <yi.y.sun@linux.intel.com> wrote: > On 17-03-08 09:54:04, Jan Beulich wrote: >> >>> On 15.02.17 at 09:49, <yi.y.sun@linux.intel.com> wrote: >> > @@ -207,6 +233,29 @@ static enum psr_feat_type psr_cbm_type_to_feat_type(enum cbm_type type) >> > return feat_type; >> > } >> > >> > +static bool 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) ) >> >> This will not do what you intend for cbm_len == 64. >> > cbm_len is not 64. cbm_len means the CBM value length, how many bits. For L3 > CAT, it may be 11 bits. For L2 CAT, it may be 8 bits. And there is an _architectural_ guarantee for them to never reach 64 bits? >> > +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 ) >> >> Afaics this condition is the only L3 CAT specific thing in this function. >> Should more of it be moved into common code? Same below for >> set_new_val. >> > Sorry, I may not understand your intention. For different features, they have > different cos_max. Do you mean I should abstract a callback function for all > features to handle this cos_max check? Thanks! Yes. All features have a cos_max (even if the precise values may differ), so common code can do the check if the values is stored suitably in a field outside of any feature dependent data structures. >> > -static int assemble_val_array(uint64_t *val, >> > +static int combine_val_array(uint64_t *val, >> >> Same comment as earlier on - please decide for a final name right >> when introducing a function. In fact I'd prefer it to remain >> "assemble". >> > This is corrected in next version. I will change name back to assemble if you > like it. I'm fine with it. Personally I'd like "gather" even better, but the naming choice in the end is up to you as long as it reflects the purpose of the function (which imo "combine" doesn't). Jan
On 17-03-10 02:15:20, Jan Beulich wrote: > >>> On 10.03.17 at 04:21, <yi.y.sun@linux.intel.com> wrote: > > On 17-03-08 09:54:04, Jan Beulich wrote: > >> >>> On 15.02.17 at 09:49, <yi.y.sun@linux.intel.com> wrote: > >> > @@ -207,6 +233,29 @@ static enum psr_feat_type psr_cbm_type_to_feat_type(enum cbm_type type) > >> > return feat_type; > >> > } > >> > > >> > +static bool 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) ) > >> > >> This will not do what you intend for cbm_len == 64. > >> > > cbm_len is not 64. cbm_len means the CBM value length, how many bits. For L3 > > CAT, it may be 11 bits. For L2 CAT, it may be 8 bits. > > And there is an _architectural_ guarantee for them to never > reach 64 bits? > Per SDM, 'EAX[4:0] reports the length of the capacity bitmask length'. So, we get cbm_len in '*_init_feature' function as below. #define CAT_CBM_LEN_MASK 0x1f cat.cbm_len = (regs.a & CAT_CBM_LEN_MASK) + 1; So, the max cbm_len is '32'. BRs, Sun Yi
>>> On 13.03.17 at 03:43, <yi.y.sun@linux.intel.com> wrote: > On 17-03-10 02:15:20, Jan Beulich wrote: >> >>> On 10.03.17 at 04:21, <yi.y.sun@linux.intel.com> wrote: >> > On 17-03-08 09:54:04, Jan Beulich wrote: >> >> >>> On 15.02.17 at 09:49, <yi.y.sun@linux.intel.com> wrote: >> >> > @@ -207,6 +233,29 @@ static enum psr_feat_type > psr_cbm_type_to_feat_type(enum cbm_type type) >> >> > return feat_type; >> >> > } >> >> > >> >> > +static bool 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) ) >> >> >> >> This will not do what you intend for cbm_len == 64. >> >> >> > cbm_len is not 64. cbm_len means the CBM value length, how many bits. For L3 >> > CAT, it may be 11 bits. For L2 CAT, it may be 8 bits. >> >> And there is an _architectural_ guarantee for them to never >> reach 64 bits? >> > Per SDM, 'EAX[4:0] reports the length of the capacity bitmask length'. So, we > get cbm_len in '*_init_feature' function as below. > > #define CAT_CBM_LEN_MASK 0x1f > cat.cbm_len = (regs.a & CAT_CBM_LEN_MASK) + 1; > > So, the max cbm_len is '32'. Great, yet in that case why do you use 64-bit calculations at all? Avoiding the undefinedness when cbm_len == 32 can be achieved without 64-bit arithmetic. Jan
On 17-03-13 06:37:41, Jan Beulich wrote: > >>> On 13.03.17 at 03:43, <yi.y.sun@linux.intel.com> wrote: > > On 17-03-10 02:15:20, Jan Beulich wrote: > >> >>> On 10.03.17 at 04:21, <yi.y.sun@linux.intel.com> wrote: > >> > On 17-03-08 09:54:04, Jan Beulich wrote: > >> >> >>> On 15.02.17 at 09:49, <yi.y.sun@linux.intel.com> wrote: > >> >> > @@ -207,6 +233,29 @@ static enum psr_feat_type > > psr_cbm_type_to_feat_type(enum cbm_type type) > >> >> > return feat_type; > >> >> > } > >> >> > > >> >> > +static bool 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) ) > >> >> > >> >> This will not do what you intend for cbm_len == 64. > >> >> > >> > cbm_len is not 64. cbm_len means the CBM value length, how many bits. For L3 > >> > CAT, it may be 11 bits. For L2 CAT, it may be 8 bits. > >> > >> And there is an _architectural_ guarantee for them to never > >> reach 64 bits? > >> > > Per SDM, 'EAX[4:0] reports the length of the capacity bitmask length'. So, we > > get cbm_len in '*_init_feature' function as below. > > > > #define CAT_CBM_LEN_MASK 0x1f > > cat.cbm_len = (regs.a & CAT_CBM_LEN_MASK) + 1; > > > > So, the max cbm_len is '32'. > > Great, yet in that case why do you use 64-bit calculations at all? > Avoiding the undefinedness when cbm_len == 32 can be > achieved without 64-bit arithmetic. > Considering to support future feature, we use 64bit as input parameter type for psr_get_val/psr_set_val. So, we handle all values as 64bit. For psr_check_cbm case, I think we can add an ASSERT as below. Is that good for you? Thanks! ASSERT(cbm_len <= 32); > Jan > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > https://lists.xen.org/xen-devel
>>> Yi Sun <yi.y.sun@linux.intel.com> 03/14/17 3:20 AM >>> >On 17-03-13 06:37:41, Jan Beulich wrote: >> >>> On 13.03.17 at 03:43, <yi.y.sun@linux.intel.com> wrote: >> > On 17-03-10 02:15:20, Jan Beulich wrote: >> >> >>> On 10.03.17 at 04:21, <yi.y.sun@linux.intel.com> wrote: >> >> > On 17-03-08 09:54:04, Jan Beulich wrote: >> >> >> >>> On 15.02.17 at 09:49, <yi.y.sun@linux.intel.com> wrote: >> >> >> > @@ -207,6 +233,29 @@ static enum psr_feat_type >> > psr_cbm_type_to_feat_type(enum cbm_type type) >> >> >> > return feat_type; >> >> >> > } >> >> >> > >> >> >> > +static bool 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) ) >> >> >> >> >> >> This will not do what you intend for cbm_len == 64. >> >> >> >> >> > cbm_len is not 64. cbm_len means the CBM value length, how many bits. For L3 >> >> > CAT, it may be 11 bits. For L2 CAT, it may be 8 bits. >> >> >> >> And there is an _architectural_ guarantee for them to never >> >> reach 64 bits? >> >> >> > Per SDM, 'EAX[4:0] reports the length of the capacity bitmask length'. So, we >> > get cbm_len in '*_init_feature' function as below. >> > >> > #define CAT_CBM_LEN_MASK 0x1f >> > cat.cbm_len = (regs.a & CAT_CBM_LEN_MASK) + 1; >> > >> > So, the max cbm_len is '32'. >> >> Great, yet in that case why do you use 64-bit calculations at all? >> Avoiding the undefinedness when cbm_len == 32 can be >> achieved without 64-bit arithmetic. >> >Considering to support future feature, we use 64bit as input parameter type for >psr_get_val/psr_set_val. So, we handle all values as 64bit. But you quote the SDM above as limiting values to 32 bits. Plus I can't see why making the code ready for 64-bit values is any help when you assume the architecture might change - if it does, values may as well become wider. Allowing for some slack in the public interface is certainly reasonable, but let's not make internal code deal with needlessly wide numbers. After all dealing with 32-bit quantities is more efficient. Jan
diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c index d414b5e..ae108b9 100644 --- a/xen/arch/x86/psr.c +++ b/xen/arch/x86/psr.c @@ -119,6 +119,32 @@ 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 in order. + * 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 meaning: + * 0 - success. + * 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, + enum cbm_type type, + uint64_t m); }; /* @@ -207,6 +233,29 @@ static enum psr_feat_type psr_cbm_type_to_feat_type(enum cbm_type type) return feat_type; } +static bool 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 false; + + /* At least one bit need to be set. */ + if ( cbm == 0 ) + return false; + + 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 false; + + return true; +} + /* L3 CAT functions implementation. */ static void l3_cat_init_feature(struct cpuid_leaf regs, struct feat_node *feat, @@ -275,10 +324,45 @@ 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]; + + return 0; +} + +static int l3_cat_set_new_val(uint64_t val[], + const struct feat_node *feat, + enum cbm_type type, + uint64_t m) +{ + if ( !psr_check_cbm(feat->info.l3_cat_info.cbm_len, m) ) + return -EINVAL; + + val[0] = m; + + 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, .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, @@ -549,15 +633,42 @@ 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 assemble_val_array(uint64_t *val, +static int combine_val_array(uint64_t *val, uint32_t array_len, const struct psr_socket_info *info, unsigned int old_cos) { - return -EINVAL; + const struct feat_node *feat; + 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, &info->feat_list, list) + { + /* value getting order is same as feature list */ + ret = feat->ops.get_old_val(val_tmp, feat, old_cos); + if ( ret ) + return ret; + + val_tmp += feat->ops.get_cos_num(feat); + if ( val_tmp - val > array_len) + return -ENOSPC; + } + + return 0; } static int set_new_val_to_array(uint64_t *val, @@ -567,7 +678,37 @@ static int set_new_val_to_array(uint64_t *val, enum cbm_type type, uint64_t m) { - return -EINVAL; + const struct feat_node *feat; + int ret; + uint64_t *val_tmp = val; + + /* Set new value into array according to feature's position in array. */ + list_for_each_entry(feat, &info->feat_list, list) + { + if ( feat->feature != feat_type ) + { + val_tmp += feat->ops.get_cos_num(feat); + if ( val_tmp - val > array_len) + return -ENOSPC; + + continue; + } + + /* + * Value setting position is same as feature list. + * 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_tmp, feat, type, m); + if ( ret ) + return ret; + else + break; + } + + return 0; } static int find_cos(const uint64_t *val, uint32_t array_len, @@ -638,7 +779,7 @@ int psr_set_val(struct domain *d, unsigned int socket, if ( !val_array ) return -ENOMEM; - if ( (ret = assemble_val_array(val_array, array_len, info, old_cos)) != 0 ) + if ( (ret = combine_val_array(val_array, array_len, info, old_cos)) != 0 ) { xfree(val_array); return ret;
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 | 151 +++++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 146 insertions(+), 5 deletions(-)