| Message ID | 1507281180-5101-1-git-send-email-yi.y.sun@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Oct 06, 2017 at 09:13:00AM +0000, Yi Sun wrote: > It changes the memebers in 'cos_write_info' to transfer the feature array, > feature properties array and value array. 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 a same COS ID (2). The L3 CAT CBM of Dom1 is 0x1ff, ^ the > 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 | > --------- > > So, we should write all features values into their MSRs. That requires the > feature array, feature properties array and value array are input. ^ as? I'm not sure the last sentence is helpful. > > 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> > --- > xen/arch/x86/psr.c | 51 +++++++++++++++++++++++++++------------------------ > 1 file changed, 27 insertions(+), 24 deletions(-) > > diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c > index daa2aeb..596b0ca 100644 > --- a/xen/arch/x86/psr.c > +++ b/xen/arch/x86/psr.c > @@ -1111,25 +1111,40 @@ static unsigned int get_socket_cpu(unsigned int socket) > struct cos_write_info > { > unsigned int cos; > - struct feat_node *feature; > + struct feat_node **features; > const uint32_t *val; > - const struct feat_props *props; > + unsigned int array_len; > }; > > 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; > + unsigned int i, index = 0, cos = info->cos; > + const uint32_t *val_array = info->val; > > - for ( i = 0; i < cos_num; i++ ) > + 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 = 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 ) Shouldn't this be '<='? index + cos_num is an index position with base 0 AFAICT. > + 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] != val_array[index + j] ) I'm afraid this code could benefit from a comment (or comments) explaining what all this arrays are supposed to contain. IMHO it's not trivial to follow what you are trying to do here. Also names like val_array are not specially helpful, it's quite clear that 'val_array' is an array just by looking at it's usage. Thanks, Roger.
On 17-10-06 15:38:35, Roger Pau Monn� wrote: > On Fri, Oct 06, 2017 at 09:13:00AM +0000, Yi Sun wrote: > > It changes the memebers in 'cos_write_info' to transfer the feature array, > > feature properties array and value array. 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 a same COS ID (2). The L3 CAT CBM of Dom1 is 0x1ff, > ^ the > > 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 | > > --------- > > > > So, we should write all features values into their MSRs. That requires the > > feature array, feature properties array and value array are input. > ^ as? > > I'm not sure the last sentence is helpful. > Ok, will remove it. > > > > 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> > > --- > > xen/arch/x86/psr.c | 51 +++++++++++++++++++++++++++------------------------ > > 1 file changed, 27 insertions(+), 24 deletions(-) > > > > diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c > > index daa2aeb..596b0ca 100644 > > --- a/xen/arch/x86/psr.c > > +++ b/xen/arch/x86/psr.c > > @@ -1111,25 +1111,40 @@ static unsigned int get_socket_cpu(unsigned int socket) > > struct cos_write_info > > { > > unsigned int cos; > > - struct feat_node *feature; > > + struct feat_node **features; > > const uint32_t *val; > > - const struct feat_props *props; > > + unsigned int array_len; > > }; > > > > 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; > > + unsigned int i, index = 0, cos = info->cos; > > + const uint32_t *val_array = info->val; > > > > - for ( i = 0; i < cos_num; i++ ) > > + 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 = 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 ) > > Shouldn't this be '<='? index + cos_num is an index position with base > 0 AFAICT. > Nope. E.g. there are L2 CAT and CDP co-exist. cos_num of L2 is 1, cos_num of CDP is 2. CDP is the first element in feature array. array_len is 3. 1. First loop to handle CDP: index is changed from 0 to 2. 2. Second loop to handle L2: cos_num = 1; index + cos_num = 3; array_len = 3; So, we must use '<' here to check if overflow happens. > > + 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] != val_array[index + j] ) > > I'm afraid this code could benefit from a comment (or comments) > explaining what all this arrays are supposed to contain. IMHO it's not > trivial to follow what you are trying to do here. > Will add comment. > Also names like val_array are not specially helpful, it's quite clear > that 'val_array' is an array just by looking at it's usage. > Ok, may consider to remove 'val_array'. > Thanks, Roger.
diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c index daa2aeb..596b0ca 100644 --- a/xen/arch/x86/psr.c +++ b/xen/arch/x86/psr.c @@ -1111,25 +1111,40 @@ static unsigned int get_socket_cpu(unsigned int socket) struct cos_write_info { unsigned int cos; - struct feat_node *feature; + struct feat_node **features; const uint32_t *val; - const struct feat_props *props; + unsigned int array_len; }; 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; + unsigned int i, index = 0, cos = info->cos; + const uint32_t *val_array = info->val; - for ( i = 0; i < cos_num; i++ ) + 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 = 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 ) + 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] != val_array[index + j] ) + { + feat->cos_reg_val[cos * cos_num + j] = val_array[index + j]; + props->write_msr(cos, val_array[index + j], props->type[j]); + } } + + index += cos_num; } } @@ -1137,30 +1152,18 @@ 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], + .features = info->features, + .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
It changes the memebers in 'cos_write_info' to transfer the feature array, feature properties array and value array. 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 a 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 | --------- So, we should write all features values into their MSRs. That requires the feature array, feature properties array and value array are input. 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> --- xen/arch/x86/psr.c | 51 +++++++++++++++++++++++++++------------------------ 1 file changed, 27 insertions(+), 24 deletions(-)