| Message ID | 1507686913-5106-1-git-send-email-yi.y.sun@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 2017-10-11 at 09:55 +0800, Yi Sun wrote: > The whole value array is transferred into 'do_write_psr_msrs'. Then, > we can > write all features values on the cos id into MSRs. > > Because multiple features may co-exist, we need handle all features to > write > values of them into a COS register with new COS ID. E.g: > 1. L3 CAT and L2 CAT co-exist. > 2. Dom1 and Dom2 share the same COS ID (2). The L3 CAT CBM of Dom1 is > 0x1ff, > the L2 CAT CBM of Dom1 is 0x1f. > 3. User wants to change L2 CBM of Dom1 to be 0xf. Because COS ID 2 is > used by Dom2 too, we have to pick a new COS ID 3. The values of > Dom1 on > COS ID 3 are all default values as below: > --------- > | COS 3 | > --------- > L3 CAT | 0x7ff | > --------- > L2 CAT | 0xff | > --------- > 4. After setting, the L3 CAT CBM value of Dom1 should be kept and the > new L2 > CAT CBM is set. So, the values on COS ID 3 should be below. > --------- > | COS 3 | > --------- > L3 CAT | 0x1ff | > --------- > L2 CAT | 0xf | > --------- > > Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com> > --- > CC: Jan Beulich <jbeulich@suse.com> > CC: Andrew Cooper <andrew.cooper3@citrix.com> > CC: Wei Liu <wei.liu2@citrix.com> > CC: Roger Pau Monné <roger.pau@citrix.com> > CC: Julien Grall <julien.grall@arm.com> > > v4: > - remove init of 'result'. > (suggested by Roger Pau Monné) > - remove 'features' in 'cos_write_info' and get socket info in > 'do_write_psr_msrs' to get features array. > (suggested by Jan Beulich) > - fix a typo in commit message. > (suggested by Kent R. Spillner) > v3: > - add 'result' in 'cos_write_info' to return error code. > (suggested by Roger Pau Monné) > v2: > - fix issues in commit message. > (suggested by Roger Pau Monné) > - remove unnecessary local variable 'val_array'. > (suggested by Roger Pau Monné) > --- > xen/arch/x86/psr.c | 62 +++++++++++++++++++++++++++++++------------ > ----------- > 1 file changed, 36 insertions(+), 26 deletions(-) > > diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c > index daa2aeb..a812124 100644 > --- a/xen/arch/x86/psr.c > +++ b/xen/arch/x86/psr.c > @@ -1111,25 +1111,48 @@ static unsigned int get_socket_cpu(unsigned > int socket) > struct cos_write_info > { > unsigned int cos; > - struct feat_node *feature; > const uint32_t *val; > - const struct feat_props *props; > + unsigned int array_len; > + int result; > }; > > static void do_write_psr_msrs(void *data) > { > - const struct cos_write_info *info = data; > - struct feat_node *feat = info->feature; > - const struct feat_props *props = info->props; > - unsigned int i, cos = info->cos, cos_num = props->cos_num; > + struct cos_write_info *info = data; > + unsigned int i, index = 0, cos = info->cos; > + struct psr_socket_info *socket_info = > + get_socket_info(cpu_to_socket(smp_process > or_id())); > > - for ( i = 0; i < cos_num; i++ ) > + /* > + * Iterate all featuers to write different value (not same as > MSR) for > + * each feature. > + */ > + for ( i = 0; i < ARRAY_SIZE(feat_props); i++ ) > { > - if ( feat->cos_reg_val[cos * cos_num + i] != info->val[i] ) > + struct feat_node *feat = socket_info->features[i]; > + const struct feat_props *props = feat_props[i]; > + unsigned int cos_num, j; > + > + if ( !feat || !props ) > + continue; > + > + cos_num = props->cos_num; > + if ( info->array_len < index + cos_num ) > + { > + info->result = -ENOSPC; > + return; This will have side effect (You may have run write_msr for some features already) when you return the error. It probably will not cause logic error here, there is performance penalty however (writing MSR and sending IPI). Another thing is this error is a real error that we want to propagate to user? E.g, I don't quite understand in which case the condition can happen? If this is only a program error then ASSERT can be used. Chao > + } > + > + for ( j = 0; j < cos_num; j++ ) > { > - feat->cos_reg_val[cos * cos_num + i] = info->val[i]; > - props->write_msr(cos, info->val[i], props->type[i]); > + if ( feat->cos_reg_val[cos * cos_num + j] != info- > >val[index + j] ) > + { > + feat->cos_reg_val[cos * cos_num + j] = info- > >val[index + j]; > + props->write_msr(cos, info->val[index + j], props- > >type[j]); > + } > } > + > + index += cos_num; > } > } > > @@ -1137,30 +1160,17 @@ static int write_psr_msrs(unsigned int socket, > unsigned int cos, > const uint32_t val[], unsigned int > array_len, > enum psr_feat_type feat_type) > { > - int ret; > struct psr_socket_info *info = get_socket_info(socket); > struct cos_write_info data = > { > .cos = cos, > - .feature = info->features[feat_type], > - .props = feat_props[feat_type], > + .val = val, > + .array_len = array_len, > }; > > if ( cos > info->features[feat_type]->cos_max ) > return -EINVAL; > > - /* Skip to the feature's value head. */ > - ret = skip_prior_features(&array_len, feat_type); > - if ( ret < 0 ) > - return ret; > - > - val += ret; > - > - 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 > @@ -1172,7 +1182,7 @@ static int write_psr_msrs(unsigned int socket, > unsigned int cos, > on_selected_cpus(cpumask_of(cpu), do_write_psr_msrs, &data, > 1); > } > > - return 0; > + return data.result; > } > > int psr_set_val(struct domain *d, unsigned int socket,
On 17-10-11 14:59:23, Chao Peng wrote: > On Wed, 2017-10-11 at 09:55 +0800, Yi Sun wrote: > > static void do_write_psr_msrs(void *data) > > { > > - const struct cos_write_info *info = data; > > - struct feat_node *feat = info->feature; > > - const struct feat_props *props = info->props; > > - unsigned int i, cos = info->cos, cos_num = props->cos_num; > > + struct cos_write_info *info = data; > > + unsigned int i, index = 0, cos = info->cos; > > + struct psr_socket_info *socket_info = > > + get_socket_info(cpu_to_socket(smp_process > > or_id())); > > > > - for ( i = 0; i < cos_num; i++ ) > > + /* > > + * Iterate all featuers to write different value (not same as > > MSR) for > > + * each feature. > > + */ > > + for ( i = 0; i < ARRAY_SIZE(feat_props); i++ ) > > { > > - if ( feat->cos_reg_val[cos * cos_num + i] != info->val[i] ) > > + struct feat_node *feat = socket_info->features[i]; > > + const struct feat_props *props = feat_props[i]; > > + unsigned int cos_num, j; > > + > > + if ( !feat || !props ) > > + continue; > > + > > + cos_num = props->cos_num; > > + if ( info->array_len < index + cos_num ) > > + { > > + info->result = -ENOSPC; > > + return; > > This will have side effect (You may have run write_msr for some features > already) when you return the error. It probably will not cause logic > error here, there is performance penalty however (writing MSR and > sending IPI). > > Another thing is this error is a real error that we want to propagate to > user? E.g, I don't quite understand in which case the condition can > happen? If this is only a program error then ASSERT can be used. > Thanks! If error happens, this error is a program error. So, an ASSERT here is better. > Chao
diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c index daa2aeb..a812124 100644 --- a/xen/arch/x86/psr.c +++ b/xen/arch/x86/psr.c @@ -1111,25 +1111,48 @@ static unsigned int get_socket_cpu(unsigned int socket) struct cos_write_info { unsigned int cos; - struct feat_node *feature; const uint32_t *val; - const struct feat_props *props; + unsigned int array_len; + int result; }; static void do_write_psr_msrs(void *data) { - const struct cos_write_info *info = data; - struct feat_node *feat = info->feature; - const struct feat_props *props = info->props; - unsigned int i, cos = info->cos, cos_num = props->cos_num; + struct cos_write_info *info = data; + unsigned int i, index = 0, cos = info->cos; + struct psr_socket_info *socket_info = + get_socket_info(cpu_to_socket(smp_processor_id())); - for ( i = 0; i < cos_num; i++ ) + /* + * Iterate all featuers to write different value (not same as MSR) for + * each feature. + */ + for ( i = 0; i < ARRAY_SIZE(feat_props); i++ ) { - if ( feat->cos_reg_val[cos * cos_num + i] != info->val[i] ) + struct feat_node *feat = socket_info->features[i]; + const struct feat_props *props = feat_props[i]; + unsigned int cos_num, j; + + if ( !feat || !props ) + continue; + + cos_num = props->cos_num; + if ( info->array_len < index + cos_num ) + { + info->result = -ENOSPC; + return; + } + + for ( j = 0; j < cos_num; j++ ) { - feat->cos_reg_val[cos * cos_num + i] = info->val[i]; - props->write_msr(cos, info->val[i], props->type[i]); + if ( feat->cos_reg_val[cos * cos_num + j] != info->val[index + j] ) + { + feat->cos_reg_val[cos * cos_num + j] = info->val[index + j]; + props->write_msr(cos, info->val[index + j], props->type[j]); + } } + + index += cos_num; } } @@ -1137,30 +1160,17 @@ static int write_psr_msrs(unsigned int socket, unsigned int cos, const uint32_t val[], unsigned int array_len, enum psr_feat_type feat_type) { - int ret; struct psr_socket_info *info = get_socket_info(socket); struct cos_write_info data = { .cos = cos, - .feature = info->features[feat_type], - .props = feat_props[feat_type], + .val = val, + .array_len = array_len, }; if ( cos > info->features[feat_type]->cos_max ) return -EINVAL; - /* Skip to the feature's value head. */ - ret = skip_prior_features(&array_len, feat_type); - if ( ret < 0 ) - return ret; - - val += ret; - - 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 @@ -1172,7 +1182,7 @@ static int write_psr_msrs(unsigned int socket, unsigned int cos, on_selected_cpus(cpumask_of(cpu), do_write_psr_msrs, &data, 1); } - return 0; + return data.result; } int psr_set_val(struct domain *d, unsigned int socket,
The whole value array is transferred into 'do_write_psr_msrs'. Then, we can write all features values on the cos id into MSRs. Because multiple features may co-exist, we need handle all features to write values of them into a COS register with new COS ID. E.g: 1. L3 CAT and L2 CAT co-exist. 2. Dom1 and Dom2 share the same COS ID (2). The L3 CAT CBM of Dom1 is 0x1ff, the L2 CAT CBM of Dom1 is 0x1f. 3. User wants to change L2 CBM of Dom1 to be 0xf. Because COS ID 2 is used by Dom2 too, we have to pick a new COS ID 3. The values of Dom1 on COS ID 3 are all default values as below: --------- | COS 3 | --------- L3 CAT | 0x7ff | --------- L2 CAT | 0xff | --------- 4. After setting, the L3 CAT CBM value of Dom1 should be kept and the new L2 CAT CBM is set. So, the values on COS ID 3 should be below. --------- | COS 3 | --------- L3 CAT | 0x1ff | --------- L2 CAT | 0xf | --------- Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com> --- CC: Jan Beulich <jbeulich@suse.com> CC: Andrew Cooper <andrew.cooper3@citrix.com> CC: Wei Liu <wei.liu2@citrix.com> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Julien Grall <julien.grall@arm.com> v4: - remove init of 'result'. (suggested by Roger Pau Monné) - remove 'features' in 'cos_write_info' and get socket info in 'do_write_psr_msrs' to get features array. (suggested by Jan Beulich) - fix a typo in commit message. (suggested by Kent R. Spillner) v3: - add 'result' in 'cos_write_info' to return error code. (suggested by Roger Pau Monné) v2: - fix issues in commit message. (suggested by Roger Pau Monné) - remove unnecessary local variable 'val_array'. (suggested by Roger Pau Monné) --- xen/arch/x86/psr.c | 62 +++++++++++++++++++++++++++++++----------------------- 1 file changed, 36 insertions(+), 26 deletions(-)