| Message ID | 1491054836-30488-11-git-send-email-yi.y.sun@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 01.04.17 at 15:53, <yi.y.sun@linux.intel.com> wrote: > @@ -593,7 +616,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; > + unsigned int num = 0, i; > + > + /* Get all features total amount. */ > + for ( i = 0; i < PSR_SOCKET_MAX_FEAT; i++ ) > + { > + const struct feat_node *feat = info->features[i]; > + if ( !feat ) Blank line between ... (and I likely won't repeat this any further) > + continue; > + > + feat = info->features[i]; ??? > @@ -611,7 +679,40 @@ static int insert_val_to_array(uint32_t val[], > enum cbm_type type, > uint32_t new_val) > { > - return -EINVAL; > + const struct feat_node *feat; > + unsigned int i; > + > + ASSERT(feat_type < PSR_SOCKET_MAX_FEAT); > + > + /* Insert new value into array according to feature's position in array. */ > + for ( i = 0; i < feat_type; i++ ) > + { > + feat = info->features[i]; > + if ( !feat ) > + continue; > + > + if ( array_len <= feat->props->cos_num ) > + return -ENOSPC; > + > + array_len -= feat->props->cos_num; > + > + val += feat->props->cos_num; > + } > + > + feat = info->features[feat_type]; > + if ( !feat ) > + return -ENOENT; > + > + if ( array_len < feat->props->cos_num ) > + return -ENOSPC; > + > + if ( !psr_check_cbm(feat->props->cbm_len, new_val) ) > + return -EINVAL; > + > + /* Value setting position is same as feature array. */ > + val[0] = new_val; How come this is array index 0 unconditionally, when cos_num may be greater than 1? Jan
On 17-04-11 09:11:20, Jan Beulich wrote: > >>> On 01.04.17 at 15:53, <yi.y.sun@linux.intel.com> wrote: > > @@ -593,7 +616,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; > > + unsigned int num = 0, i; > > + > > + /* Get all features total amount. */ > > + for ( i = 0; i < PSR_SOCKET_MAX_FEAT; i++ ) > > + { > > + const struct feat_node *feat = info->features[i]; > > + if ( !feat ) > > Blank line between ... (and I likely won't repeat this any further) > Got it, will fix all of them. > > + continue; > > + > > + feat = info->features[i]; > > ??? > Sorry, missed it. > > @@ -611,7 +679,40 @@ static int insert_val_to_array(uint32_t val[], > > enum cbm_type type, > > uint32_t new_val) > > { > > - return -EINVAL; > > + const struct feat_node *feat; > > + unsigned int i; > > + > > + ASSERT(feat_type < PSR_SOCKET_MAX_FEAT); > > + > > + /* Insert new value into array according to feature's position in array. */ > > + for ( i = 0; i < feat_type; i++ ) > > + { > > + feat = info->features[i]; > > + if ( !feat ) > > + continue; > > + > > + if ( array_len <= feat->props->cos_num ) > > + return -ENOSPC; > > + > > + array_len -= feat->props->cos_num; > > + > > + val += feat->props->cos_num; > > + } > > + > > + feat = info->features[feat_type]; > > + if ( !feat ) > > + return -ENOENT; > > + > > + if ( array_len < feat->props->cos_num ) > > + return -ENOSPC; > > + > > + if ( !psr_check_cbm(feat->props->cbm_len, new_val) ) > > + return -EINVAL; > > + > > + /* Value setting position is same as feature array. */ > > + val[0] = new_val; > > How come this is array index 0 unconditionally, when cos_num > may be greater than 1? > This patch is to implement L3 CAT so I do not introduce 'type[]' yet. The mechanism will be introduced in CDP patch. > Jan
>>> On 12.04.17 at 07:55, <yi.y.sun@linux.intel.com> wrote: > On 17-04-11 09:11:20, Jan Beulich wrote: >> >>> On 01.04.17 at 15:53, <yi.y.sun@linux.intel.com> wrote: >> > @@ -611,7 +679,40 @@ static int insert_val_to_array(uint32_t val[], >> > enum cbm_type type, >> > uint32_t new_val) >> > { >> > - return -EINVAL; >> > + const struct feat_node *feat; >> > + unsigned int i; >> > + >> > + ASSERT(feat_type < PSR_SOCKET_MAX_FEAT); >> > + >> > + /* Insert new value into array according to feature's position in array. */ >> > + for ( i = 0; i < feat_type; i++ ) >> > + { >> > + feat = info->features[i]; >> > + if ( !feat ) >> > + continue; >> > + >> > + if ( array_len <= feat->props->cos_num ) >> > + return -ENOSPC; >> > + >> > + array_len -= feat->props->cos_num; >> > + >> > + val += feat->props->cos_num; >> > + } >> > + >> > + feat = info->features[feat_type]; >> > + if ( !feat ) >> > + return -ENOENT; >> > + >> > + if ( array_len < feat->props->cos_num ) >> > + return -ENOSPC; >> > + >> > + if ( !psr_check_cbm(feat->props->cbm_len, new_val) ) >> > + return -EINVAL; >> > + >> > + /* Value setting position is same as feature array. */ >> > + val[0] = new_val; >> >> How come this is array index 0 unconditionally, when cos_num >> may be greater than 1? >> > This patch is to implement L3 CAT so I do not introduce 'type[]' yet. The > mechanism will be introduced in CDP patch. Which imo is wrong, as I've pointed out in a later patch. The moment you have the "cos_num" field (instead of implying 1 everywhere) you ought to be dealing with the field having a value other than 1. Otherwise, as that later patch shows, you'd then re-write _part_ of the logic without in fact _newly_ introducing "cos_num" being a variable. Jan
On 17-04-12 03:13:12, Jan Beulich wrote: > >>> On 12.04.17 at 07:55, <yi.y.sun@linux.intel.com> wrote: > > On 17-04-11 09:11:20, Jan Beulich wrote: > >> >>> On 01.04.17 at 15:53, <yi.y.sun@linux.intel.com> wrote: > >> > @@ -611,7 +679,40 @@ static int insert_val_to_array(uint32_t val[], > >> > enum cbm_type type, > >> > uint32_t new_val) > >> > { > >> > - return -EINVAL; > >> > + const struct feat_node *feat; > >> > + unsigned int i; > >> > + > >> > + ASSERT(feat_type < PSR_SOCKET_MAX_FEAT); > >> > + > >> > + /* Insert new value into array according to feature's position in array. */ > >> > + for ( i = 0; i < feat_type; i++ ) > >> > + { > >> > + feat = info->features[i]; > >> > + if ( !feat ) > >> > + continue; > >> > + > >> > + if ( array_len <= feat->props->cos_num ) > >> > + return -ENOSPC; > >> > + > >> > + array_len -= feat->props->cos_num; > >> > + > >> > + val += feat->props->cos_num; > >> > + } > >> > + > >> > + feat = info->features[feat_type]; > >> > + if ( !feat ) > >> > + return -ENOENT; > >> > + > >> > + if ( array_len < feat->props->cos_num ) > >> > + return -ENOSPC; > >> > + > >> > + if ( !psr_check_cbm(feat->props->cbm_len, new_val) ) > >> > + return -EINVAL; > >> > + > >> > + /* Value setting position is same as feature array. */ > >> > + val[0] = new_val; > >> > >> How come this is array index 0 unconditionally, when cos_num > >> may be greater than 1? > >> > > This patch is to implement L3 CAT so I do not introduce 'type[]' yet. The > > mechanism will be introduced in CDP patch. > > Which imo is wrong, as I've pointed out in a later patch. The > moment you have the "cos_num" field (instead of implying 1 > everywhere) you ought to be dealing with the field having a > value other than 1. Otherwise, as that later patch shows, you'd > then re-write _part_ of the logic without in fact _newly_ > introducing "cos_num" being a variable. > Ok, I will introduce the 'type[]' with 'cos_num' together from the beginning. > Jan
diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c index 9d805d6..c912478 100644 --- a/xen/arch/x86/psr.c +++ b/xen/arch/x86/psr.c @@ -224,6 +224,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, unsigned long 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(&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; +} + static void cat_init_feature(const struct cpuid_leaf *regs, struct feat_node *feat, struct psr_socket_info *info, @@ -593,7 +616,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; + unsigned int num = 0, i; + + /* Get all features total amount. */ + for ( i = 0; i < PSR_SOCKET_MAX_FEAT; i++ ) + { + const struct feat_node *feat = info->features[i]; + if ( !feat ) + continue; + + feat = info->features[i]; + + num += feat->props->cos_num; + } + + return num; } static int gather_val_array(uint32_t val[], @@ -601,7 +638,38 @@ static int gather_val_array(uint32_t val[], const struct psr_socket_info *info, unsigned int old_cos) { - return -EINVAL; + 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++ ) + { + unsigned int cos = old_cos; + const struct feat_node *feat = info->features[i]; + if ( !feat ) + continue; + + if ( array_len < feat->props->cos_num ) + return -ENOSPC; + + /* + * If old_cos exceeds current feature's cos_max, we should get + * default value. So assign cos to 0 which stores default value. + */ + if ( cos > feat->props->cos_max ) + cos = 0; + + /* Value getting order is same as feature array. */ + feat->props->get_val(feat, cos, &val[0]); + + array_len -= feat->props->cos_num; + + val += feat->props->cos_num; + } + + return 0; } static int insert_val_to_array(uint32_t val[], @@ -611,7 +679,40 @@ static int insert_val_to_array(uint32_t val[], enum cbm_type type, uint32_t new_val) { - return -EINVAL; + const struct feat_node *feat; + unsigned int i; + + ASSERT(feat_type < PSR_SOCKET_MAX_FEAT); + + /* Insert new value into array according to feature's position in array. */ + for ( i = 0; i < feat_type; i++ ) + { + feat = info->features[i]; + if ( !feat ) + continue; + + if ( array_len <= feat->props->cos_num ) + return -ENOSPC; + + array_len -= feat->props->cos_num; + + val += feat->props->cos_num; + } + + feat = info->features[feat_type]; + if ( !feat ) + return -ENOENT; + + if ( array_len < feat->props->cos_num ) + return -ENOSPC; + + if ( !psr_check_cbm(feat->props->cbm_len, new_val) ) + return -EINVAL; + + /* Value setting position is same as feature array. */ + val[0] = new_val; + + return 0; } static int find_cos(const uint32_t val[], unsigned int 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> --- v10: - remove 'get_old_val' to directly call 'get_val' to get needed val. (suggested by Jan Beulich) - move 'psr_check_cbm' into 'insert_val_to_array'. (suggested by Jan Beulich) - change type of 'cbm' in 'psr_check_cbm' to 'unsigned long'. (suggested by Jan Beulich) - remove 'set_new_val' as it can be handled in generic process. - changes related to 'feat_props'. (suggested by Jan Beulich) - adjust flow in 'gather_val_array' to avoid array cross. (suggested by Jan Beulich) - adjust flow in 'insert_val_to_array' to avoid array cross. (suggested by Jan Beulich) 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 | 107 +++++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 104 insertions(+), 3 deletions(-)