| Message ID | 1493801063-38513-13-git-send-email-yi.y.sun@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 03.05.17 at 10:44, <yi.y.sun@linux.intel.com> wrote: > +struct cos_write_info > +{ > + unsigned int cos; > + struct feat_node *feature; > + uint32_t *val; > + enum psr_feat_type feat_type; > +}; > + > +static void do_write_psr_msrs(void *data) > +{ > + struct cos_write_info *info = data; > + unsigned int cos = info->cos; > + struct feat_node *feat = info->feature; > + const struct feat_props *props = feat_props[info->feat_type]; > + unsigned int i; > + > + for ( i = 0; i < props->cos_num; i++ ) > + { > + if ( feat->cos_reg_val[cos * props->cos_num + i] != info->val[i] ) > + { > + feat->cos_reg_val[cos * props->cos_num + i] = info->val[i]; > + props->write_msr(cos, info->val[i], props->type[i]); > + } > + } > +} Again you're passing feat_type here only to get at props. Why not pass props right away? Also I think it would make sense to pull props->cos_num into a local variable. > static int write_psr_msrs(unsigned int socket, unsigned int cos, > uint32_t val[], unsigned int array_len, > enum psr_feat_type feat_type) > { > - return -ENOENT; > + unsigned int i; > + struct psr_socket_info *info = get_socket_info(socket); > + struct cos_write_info data = > + { > + .cos = cos, > + .feature = info->features[feat_type], > + .feat_type = feat_type, > + }; > + > + if ( cos > info->features[feat_type]->cos_max ) > + return -EINVAL; > + > + /* Skip to the feature's value head. */ > + for ( i = 0; i < feat_type; i++ ) > + { > + if ( !info->features[i] ) > + continue; This is inconsistent with checks done elsewhere, where you also check feat_props[feat_type] against NULL. I've made a comment regarding whether both checks are wanted in a uniform or non- uniform way pretty early in the series. Whatever is selected should then be used consistently. Jan
On 17-05-30 09:35:53, Jan Beulich wrote: > >>> On 03.05.17 at 10:44, <yi.y.sun@linux.intel.com> wrote: > > +struct cos_write_info > > +{ > > + unsigned int cos; > > + struct feat_node *feature; > > + uint32_t *val; > > + enum psr_feat_type feat_type; > > +}; > > + > > +static void do_write_psr_msrs(void *data) > > +{ > > + struct cos_write_info *info = data; > > + unsigned int cos = info->cos; > > + struct feat_node *feat = info->feature; > > + const struct feat_props *props = feat_props[info->feat_type]; > > + unsigned int i; > > + > > + for ( i = 0; i < props->cos_num; i++ ) > > + { > > + if ( feat->cos_reg_val[cos * props->cos_num + i] != info->val[i] ) > > + { > > + feat->cos_reg_val[cos * props->cos_num + i] = info->val[i]; > > + props->write_msr(cos, info->val[i], props->type[i]); > > + } > > + } > > +} > > Again you're passing feat_type here only to get at props. Why > not pass props right away? Also I think it would make sense to > pull props->cos_num into a local variable. > Have modified these according to your comments. Thanks! > > static int write_psr_msrs(unsigned int socket, unsigned int cos, > > uint32_t val[], unsigned int array_len, > > enum psr_feat_type feat_type) > > { > > - return -ENOENT; > > + unsigned int i; > > + struct psr_socket_info *info = get_socket_info(socket); > > + struct cos_write_info data = > > + { > > + .cos = cos, > > + .feature = info->features[feat_type], > > + .feat_type = feat_type, > > + }; > > + > > + if ( cos > info->features[feat_type]->cos_max ) > > + return -EINVAL; > > + > > + /* Skip to the feature's value head. */ > > + for ( i = 0; i < feat_type; i++ ) > > + { > > + if ( !info->features[i] ) > > + continue; > > This is inconsistent with checks done elsewhere, where you also > check feat_props[feat_type] against NULL. I've made a comment > regarding whether both checks are wanted in a uniform or non- > uniform way pretty early in the series. Whatever is selected > should then be used consistently. > Have changed it. Thanks! > Jan
diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c index 9693506..9b8428d 100644 --- a/xen/arch/x86/psr.c +++ b/xen/arch/x86/psr.c @@ -107,6 +107,9 @@ static const struct feat_props { /* get_feat_info is used to return feature HW info through sysctl. */ bool (*get_feat_info)(const struct feat_node *feat, uint32_t data[], unsigned int array_len); + + /* write_msr is used to write out feature MSR register. */ + void (*write_msr)(unsigned int cos, uint32_t val, enum cbm_type type); } *feat_props[PSR_SOCKET_FEAT_NUM]; /* @@ -275,9 +278,15 @@ static bool cat_get_feat_info(const struct feat_node *feat, } /* L3 CAT props */ +static void l3_cat_write_msr(unsigned int cos, uint32_t val, enum cbm_type type) +{ + wrmsrl(MSR_IA32_PSR_L3_MASK(cos), val); +} + static struct feat_props l3_cat_props = { .cos_num = 1, .get_feat_info = cat_get_feat_info, + .write_msr = l3_cat_write_msr, }; static void __init parse_psr_bool(char *s, char *value, char *feature, @@ -885,11 +894,87 @@ static int pick_avail_cos(const struct psr_socket_info *info, return -EOVERFLOW; } +static unsigned int get_socket_cpu(unsigned int socket) +{ + if ( likely(socket < nr_sockets) ) + return cpumask_any(socket_cpumask[socket]); + + return nr_cpu_ids; +} + +struct cos_write_info +{ + unsigned int cos; + struct feat_node *feature; + uint32_t *val; + enum psr_feat_type feat_type; +}; + +static void do_write_psr_msrs(void *data) +{ + struct cos_write_info *info = data; + unsigned int cos = info->cos; + struct feat_node *feat = info->feature; + const struct feat_props *props = feat_props[info->feat_type]; + unsigned int i; + + for ( i = 0; i < props->cos_num; i++ ) + { + if ( feat->cos_reg_val[cos * props->cos_num + i] != info->val[i] ) + { + feat->cos_reg_val[cos * props->cos_num + i] = info->val[i]; + props->write_msr(cos, info->val[i], props->type[i]); + } + } +} + static int write_psr_msrs(unsigned int socket, unsigned int cos, uint32_t val[], unsigned int array_len, enum psr_feat_type feat_type) { - return -ENOENT; + unsigned int i; + struct psr_socket_info *info = get_socket_info(socket); + struct cos_write_info data = + { + .cos = cos, + .feature = info->features[feat_type], + .feat_type = feat_type, + }; + + if ( cos > info->features[feat_type]->cos_max ) + return -EINVAL; + + /* Skip to the feature's value head. */ + for ( i = 0; i < feat_type; i++ ) + { + if ( !info->features[i] ) + continue; + + if ( array_len <= feat_props[feat_type]->cos_num ) + return -ENOSPC; + + array_len -= feat_props[feat_type]->cos_num; + + val += feat_props[feat_type]->cos_num; + } + + if ( array_len < feat_props[feat_type]->cos_num ) + return -ENOSPC; + + data.val = val; + + if ( socket == cpu_to_socket(smp_processor_id()) ) + do_write_psr_msrs(&data); + else + { + unsigned int cpu = get_socket_cpu(socket); + + if ( cpu >= nr_cpu_ids ) + return -ENOTSOCK; + on_selected_cpus(cpumask_of(cpu), do_write_psr_msrs, &data, 1); + } + + return 0; } /* The whole set process is protected by domctl_lock. */
Continue from previous patch: 'x86: refactor psr: L3 CAT: set value: implement cos id picking flow.' We have got the feature value and COS ID to set. Then, we write MSRs of the designated feature. Till now, set value process is completed. Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com> --- v11: - rename 'write_psr_msr' to 'write_psr_msrs'. - rename 'do_write_psr_msr' to 'do_write_psr_msrs'. - change parameters and codes of 'write_psr_msrs' to handle value array. - add 'feat_type' in 'struct cos_write_info' to handle props array. - in 'do_write_psr_msrs', write value array into msrs according to 'props->type[i]'. - move 'feat->cos_reg_val' assignment and value comparison in 'write_msr' callback function out as generic codes. (suggested by Jan Beulich) - move check from 'do_write_psr_msrs' to 'write_psr_msrs'. (suggested by Jan Beulich) - change about 'cos_max'. (suggested by Jan Beulich) - change about 'feat_props'. (suggested by Jan Beulich) v10: - remove 'type' from 'write_msr' parameter list. Will add it back when implementing CDP. (suggested by Jan Beulich) - remove unnecessary casts. (suggested by Jan Beulich) - changes about 'props'. (suggested by Jan Beulich) v9: - replace feature list handling to feature array handling. (suggested by Roger Pau) - add 'array_len' in 'struct cos_write_info' and check if val array exceeds it. - modify 'write_psr_msr' flow only to set one value a time. No need to set whole feature array values. - modify patch title to indicate 'L3 CAT'. (suggested by Jan Beulich) - changes about 'uint64_t' to 'uint32_t'. (suggested by Jan Beulich) v8: - modify 'write_msr' callback function to 'void' because we have to set all features' cbm. When input cos exceeds some features' cos_max, just skip them but not break the iteration. v5: - modify commit message to provide exact patch name to continue from. (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) - move type check out from callback functions to caller. (suggested by Jan Beulich) - modify variables names to make them better, e.g. 'feat_tmp' to 'feat'. (suggested by Jan Beulich) - correct code format. (suggested by Jan Beulich) v4: - create this patch to make codes easier understand. (suggested by Jan Beulich) --- xen/arch/x86/psr.c | 87 +++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 86 insertions(+), 1 deletion(-)