* Re: [PATCH v1] x86: psr: support co-exist features' values setting
2017-10-06 9:13 [PATCH v1] x86: psr: support co-exist features' values setting Yi Sun
@ 2017-10-06 14:38 ` Roger Pau Monné
2017-10-08 2:14 ` Yi Sun
2017-10-08 4:22 ` [PATCH v2] " Yi Sun
` (3 subsequent siblings)
4 siblings, 1 reply; 16+ messages in thread
From: Roger Pau Monné @ 2017-10-06 14:38 UTC (permalink / raw)
To: Yi Sun; +Cc: xen-devel, Julien Grall, Wei Liu, Jan Beulich, Andrew Cooper
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.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v1] x86: psr: support co-exist features' values setting
2017-10-06 14:38 ` Roger Pau Monné
@ 2017-10-08 2:14 ` Yi Sun
0 siblings, 0 replies; 16+ messages in thread
From: Yi Sun @ 2017-10-08 2:14 UTC (permalink / raw)
To: Roger Pau Monn�
Cc: xen-devel, Julien Grall, Wei Liu, Jan Beulich, Andrew Cooper
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.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2] x86: psr: support co-exist features' values setting
2017-10-06 9:13 [PATCH v1] x86: psr: support co-exist features' values setting Yi Sun
2017-10-06 14:38 ` Roger Pau Monné
@ 2017-10-08 4:22 ` Yi Sun
2017-10-09 14:03 ` Roger Pau Monné
2017-10-10 9:19 ` [PATCH v3] " Yi Sun
` (2 subsequent siblings)
4 siblings, 1 reply; 16+ messages in thread
From: Yi Sun @ 2017-10-08 4:22 UTC (permalink / raw)
To: xen-devel
Cc: Yi Sun, Andrew Cooper, Julien Grall, Jan Beulich, Wei Liu,
Roger Pau Monné
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 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>
---
xen/arch/x86/psr.c | 54 ++++++++++++++++++++++++++++++------------------------
1 file changed, 30 insertions(+), 24 deletions(-)
diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c
index daa2aeb..dbf7a4c 100644
--- a/xen/arch/x86/psr.c
+++ b/xen/arch/x86/psr.c
@@ -1111,25 +1111,43 @@ 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;
- 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 = 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] != 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 +1155,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
--
1.9.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH v2] x86: psr: support co-exist features' values setting
2017-10-08 4:22 ` [PATCH v2] " Yi Sun
@ 2017-10-09 14:03 ` Roger Pau Monné
2017-10-10 0:41 ` Yi Sun
0 siblings, 1 reply; 16+ messages in thread
From: Roger Pau Monné @ 2017-10-09 14:03 UTC (permalink / raw)
To: Yi Sun; +Cc: xen-devel, Julien Grall, Wei Liu, Jan Beulich, Andrew Cooper
On Sun, Oct 08, 2017 at 04:22: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 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>
> ---
> xen/arch/x86/psr.c | 54 ++++++++++++++++++++++++++++++------------------------
> 1 file changed, 30 insertions(+), 24 deletions(-)
>
> diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c
> index daa2aeb..dbf7a4c 100644
> --- a/xen/arch/x86/psr.c
> +++ b/xen/arch/x86/psr.c
> @@ -1111,25 +1111,43 @@ 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)
Should this be "static int do_write_psr_msrs"...
> {
> 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;
>
> - 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 = 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;
So that you can return -ENOSPC here (inline with what was previously
done in write_psr_msrs)?
> +
> + 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 +1155,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 )
When moved inside of do_write_psr_msrs this becomes:
info->array_len < index + cos_num
Where cos_num == feat_props[feat_type]->cos_num. Is this correct?
I'm asking because the check used to be array_len < cos_num.
Thanks, Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v2] x86: psr: support co-exist features' values setting
2017-10-09 14:03 ` Roger Pau Monné
@ 2017-10-10 0:41 ` Yi Sun
2017-10-10 8:22 ` Roger Pau Monné
0 siblings, 1 reply; 16+ messages in thread
From: Yi Sun @ 2017-10-10 0:41 UTC (permalink / raw)
To: Roger Pau Monn�
Cc: xen-devel, Julien Grall, Wei Liu, Jan Beulich, Andrew Cooper
On 17-10-09 15:03:25, Roger Pau Monn� wrote:
> On Sun, Oct 08, 2017 at 04:22:00AM +0000, Yi Sun wrote:
[...]
> > static void do_write_psr_msrs(void *data)
>
> Should this be "static int do_write_psr_msrs"...
>
This function is a parameter of 'on_selected_cpus()' which requires it to be
'void'.
> > {
> > 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;
> >
> > - 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 = 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;
>
> So that you can return -ENOSPC here (inline with what was previously
> done in write_psr_msrs)?
>
> > +
> > + 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 +1155,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 )
>
> When moved inside of do_write_psr_msrs this becomes:
>
> info->array_len < index + cos_num
>
> Where cos_num == feat_props[feat_type]->cos_num. Is this correct?
>
> I'm asking because the check used to be array_len < cos_num.
>
The removed old codes here only check one feature cos_num. Because the old
codes can only handle one feature. That is the reason I submit this patch
to support multiple co-exist features' values setting.
> Thanks, Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v2] x86: psr: support co-exist features' values setting
2017-10-10 0:41 ` Yi Sun
@ 2017-10-10 8:22 ` Roger Pau Monné
0 siblings, 0 replies; 16+ messages in thread
From: Roger Pau Monné @ 2017-10-10 8:22 UTC (permalink / raw)
To: Yi Sun; +Cc: xen-devel, Julien Grall, Wei Liu, Jan Beulich, Andrew Cooper
On Tue, Oct 10, 2017 at 12:41:40AM +0000, Yi Sun wrote:
> On 17-10-09 15:03:25, Roger Pau Monn� wrote:
> > On Sun, Oct 08, 2017 at 04:22:00AM +0000, Yi Sun wrote:
> [...]
>
> > > static void do_write_psr_msrs(void *data)
> >
> > Should this be "static int do_write_psr_msrs"...
> >
> This function is a parameter of 'on_selected_cpus()' which requires it to be
> 'void'.
IMHO loosing the error code is wrong, so I would suggest storing it
inside of cos_write_info.
Thanks, Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v3] x86: psr: support co-exist features' values setting
2017-10-06 9:13 [PATCH v1] x86: psr: support co-exist features' values setting Yi Sun
2017-10-06 14:38 ` Roger Pau Monné
2017-10-08 4:22 ` [PATCH v2] " Yi Sun
@ 2017-10-10 9:19 ` Yi Sun
2017-10-10 9:49 ` Roger Pau Monné
2017-10-10 14:44 ` Jan Beulich
2017-10-11 1:55 ` [PATCH v4] " Yi Sun
2017-10-11 7:20 ` [PATCH v5] " Yi Sun
4 siblings, 2 replies; 16+ messages in thread
From: Yi Sun @ 2017-10-10 9:19 UTC (permalink / raw)
To: xen-devel
Cc: Yi Sun, Andrew Cooper, Julien Grall, Jan Beulich, Wei Liu,
Roger Pau Monné
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 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>
v2:
- add 'result' in 'cos_write_info' to return error code.
(suggested by Roger Pau Monné)
v1:
- 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 | 63 ++++++++++++++++++++++++++++++++----------------------
1 file changed, 37 insertions(+), 26 deletions(-)
diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c
index daa2aeb..4f47a0b 100644
--- a/xen/arch/x86/psr.c
+++ b/xen/arch/x86/psr.c
@@ -1111,25 +1111,47 @@ 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;
+ 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;
- 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 = 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 +1159,19 @@ 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,
+ .result = 0,
};
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 +1183,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,
--
1.9.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH v3] x86: psr: support co-exist features' values setting
2017-10-10 9:19 ` [PATCH v3] " Yi Sun
@ 2017-10-10 9:49 ` Roger Pau Monné
2017-10-10 14:44 ` Jan Beulich
1 sibling, 0 replies; 16+ messages in thread
From: Roger Pau Monné @ 2017-10-10 9:49 UTC (permalink / raw)
To: Yi Sun; +Cc: xen-devel, Julien Grall, Wei Liu, Jan Beulich, Andrew Cooper
On Tue, Oct 10, 2017 at 09:19:10AM +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 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>
LGTM, just one nit.
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
> @@ -1137,30 +1159,19 @@ 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,
> + .result = 0,
This last line is not needed (result will be set to 0 already).
Thanks, Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v3] x86: psr: support co-exist features' values setting
2017-10-10 9:19 ` [PATCH v3] " Yi Sun
2017-10-10 9:49 ` Roger Pau Monné
@ 2017-10-10 14:44 ` Jan Beulich
1 sibling, 0 replies; 16+ messages in thread
From: Jan Beulich @ 2017-10-10 14:44 UTC (permalink / raw)
To: Yi Sun
Cc: Andrew Cooper, Julien Grall, Wei Liu, xen-devel, Roger Pau Monné
>>> On 10.10.17 at 11:19, <yi.y.sun@linux.intel.com> wrote:
> --- a/xen/arch/x86/psr.c
> +++ b/xen/arch/x86/psr.c
> @@ -1111,25 +1111,47 @@ static unsigned int get_socket_cpu(unsigned int socket)
> struct cos_write_info
> {
> unsigned int cos;
> - struct feat_node *feature;
> + struct feat_node **features;
Why do you need to pass this? Can't do_write_psr_msrs() grab it
via get_socket_info()? I ask not the least because if you pass it,
you want to add const between the two stars to document the
consumer won't alter the array.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v4] x86: psr: support co-exist features' values setting
2017-10-06 9:13 [PATCH v1] x86: psr: support co-exist features' values setting Yi Sun
` (2 preceding siblings ...)
2017-10-10 9:19 ` [PATCH v3] " Yi Sun
@ 2017-10-11 1:55 ` Yi Sun
2017-10-11 6:59 ` Chao Peng
2017-10-11 7:20 ` [PATCH v5] " Yi Sun
4 siblings, 1 reply; 16+ messages in thread
From: Yi Sun @ 2017-10-11 1:55 UTC (permalink / raw)
To: xen-devel
Cc: Yi Sun, Andrew Cooper, Julien Grall, Jan Beulich, Wei Liu,
Roger Pau Monné
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_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,
--
1.9.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH v4] x86: psr: support co-exist features' values setting
2017-10-11 1:55 ` [PATCH v4] " Yi Sun
@ 2017-10-11 6:59 ` Chao Peng
2017-10-11 7:14 ` Yi Sun
0 siblings, 1 reply; 16+ messages in thread
From: Chao Peng @ 2017-10-11 6:59 UTC (permalink / raw)
To: Yi Sun, xen-devel
Cc: Andrew Cooper, Julien Grall, Wei Liu, Jan Beulich, Roger Pau Monné
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,
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v4] x86: psr: support co-exist features' values setting
2017-10-11 6:59 ` Chao Peng
@ 2017-10-11 7:14 ` Yi Sun
0 siblings, 0 replies; 16+ messages in thread
From: Yi Sun @ 2017-10-11 7:14 UTC (permalink / raw)
To: Chao Peng
Cc: Wei Liu, Andrew Cooper, Julien Grall, Jan Beulich, xen-devel,
Roger Pau Monn�
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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v5] x86: psr: support co-exist features' values setting
2017-10-06 9:13 [PATCH v1] x86: psr: support co-exist features' values setting Yi Sun
` (3 preceding siblings ...)
2017-10-11 1:55 ` [PATCH v4] " Yi Sun
@ 2017-10-11 7:20 ` Yi Sun
2017-10-11 12:06 ` Jan Beulich
4 siblings, 1 reply; 16+ messages in thread
From: Yi Sun @ 2017-10-11 7:20 UTC (permalink / raw)
To: xen-devel
Cc: Yi Sun, Andrew Cooper, Julien Grall, Jan Beulich, Wei Liu,
Roger Pau Monné
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>
v5:
- remove 'result' and use an ASSERT to handle error case.
(suggested by Chao Peng)
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 | 55 +++++++++++++++++++++++++++++-------------------------
1 file changed, 30 insertions(+), 25 deletions(-)
diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c
index daa2aeb..8936cf7 100644
--- a/xen/arch/x86/psr.c
+++ b/xen/arch/x86/psr.c
@@ -1111,25 +1111,43 @@ 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;
};
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;
+ ASSERT(info->array_len >= index + cos_num);
+
+ 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 +1155,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
--
1.9.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH v5] x86: psr: support co-exist features' values setting
2017-10-11 7:20 ` [PATCH v5] " Yi Sun
@ 2017-10-11 12:06 ` Jan Beulich
2017-10-12 2:52 ` Yi Sun
0 siblings, 1 reply; 16+ messages in thread
From: Jan Beulich @ 2017-10-11 12:06 UTC (permalink / raw)
To: Yi Sun
Cc: Andrew Cooper, Julien Grall, Wei Liu, xen-devel, Roger Pau Monné
>>> On 11.10.17 at 09:20, <yi.y.sun@linux.intel.com> wrote:
> --- a/xen/arch/x86/psr.c
> +++ b/xen/arch/x86/psr.c
> @@ -1111,25 +1111,43 @@ 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;
> };
The addition wants to go into the hole after "cos".
> 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;
const
> + unsigned int i, index = 0, cos = info->cos;
> + struct psr_socket_info *socket_info =
const
> + 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;
> + ASSERT(info->array_len >= index + cos_num);
While this transformation from the original -ENOSPC return looks to
be correct, but not obviously so, it would have been a good idea
to mention this in the commit message. After all the above can be
correct only if the original -ENOSPC return path could have been
an ASSERT() as well.
> + 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;
Looks like I only meant to comment on the uses of index above:
If you incremented it alongside j, you could use just index in the
respective array accesses, and you'd avoid the last statement
above altogether.
In the interest of getting the patch in I'll see to make the
adjustments myself. Please double check the result in case I end
up committing what I've come up with.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v5] x86: psr: support co-exist features' values setting
2017-10-11 12:06 ` Jan Beulich
@ 2017-10-12 2:52 ` Yi Sun
0 siblings, 0 replies; 16+ messages in thread
From: Yi Sun @ 2017-10-12 2:52 UTC (permalink / raw)
To: Jan Beulich
Cc: Andrew Cooper, Julien Grall, Wei Liu, xen-devel, Roger Pau Monné
Many thanks for the changes! The changes look good to me and pass the test.
On 17-10-11 06:06:49, Jan Beulich wrote:
> >>> On 11.10.17 at 09:20, <yi.y.sun@linux.intel.com> wrote:
> > --- a/xen/arch/x86/psr.c
> > +++ b/xen/arch/x86/psr.c
> > @@ -1111,25 +1111,43 @@ 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;
> > };
>
> The addition wants to go into the hole after "cos".
>
> > 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;
>
> const
>
> > + unsigned int i, index = 0, cos = info->cos;
> > + struct psr_socket_info *socket_info =
>
> const
>
> > + 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;
> > + ASSERT(info->array_len >= index + cos_num);
>
> While this transformation from the original -ENOSPC return looks to
> be correct, but not obviously so, it would have been a good idea
> to mention this in the commit message. After all the above can be
> correct only if the original -ENOSPC return path could have been
> an ASSERT() as well.
>
> > + 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;
>
> Looks like I only meant to comment on the uses of index above:
> If you incremented it alongside j, you could use just index in the
> respective array accesses, and you'd avoid the last statement
> above altogether.
>
> In the interest of getting the patch in I'll see to make the
> adjustments myself. Please double check the result in case I end
> up committing what I've come up with.
>
> Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 16+ messages in thread