| Message ID | 1497402776-22348-16-git-send-email-yi.y.sun@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> Yi Sun <yi.y.sun@linux.intel.com> 06/14/17 3:26 AM >>> > This patch implements L3 CDP set value related callback function. > > With this patch, 'psr-cat-cbm-set' command can work for L3 CDP. > > Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com> > --- > v12: > - add comment to explain how to deal with the case that user set new val > for both DATA and CODE at same time. > - add parameter for 'psr_cbm_type_to_feat_type' to return the feature type > according to it. > - use the feature type returned by 'psr_cbm_type_to_feat_type' to check > if we need insert the new value into all items of the feature value array. > - use conditional expression for wrmsrl. > (suggested by Jan Beulich) > --- > xen/arch/x86/psr.c | 36 +++++++++++++++++++++++++++--------- > 1 file changed, 27 insertions(+), 9 deletions(-) > > diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c > index aee6e3e..91b2122 100644 > --- a/xen/arch/x86/psr.c > +++ b/xen/arch/x86/psr.c > @@ -209,7 +209,8 @@ static void free_socket_resources(unsigned int socket) > bitmap_zero(info->dom_set, DOMID_IDLE + 1); > } > > -static enum psr_feat_type psr_cbm_type_to_feat_type(enum cbm_type type) > +static enum psr_feat_type psr_cbm_type_to_feat_type(enum cbm_type type, > + bool strict) > { > enum psr_feat_type feat_type = PSR_SOCKET_FEAT_UNKNOWN; > > @@ -222,7 +223,7 @@ static enum psr_feat_type psr_cbm_type_to_feat_type(enum cbm_type type) > * If type is L3 CAT but we cannot find it in feat_props array, > * try CDP. > */ > - if ( !feat_props[feat_type] ) > + if ( !feat_props[feat_type] && !strict ) > feat_type = PSR_SOCKET_L3_CDP; > > break; > @@ -358,11 +359,20 @@ static bool l3_cdp_get_feat_info(const struct feat_node *feat, > return true; > } > > +static void l3_cdp_write_msr(unsigned int cos, uint32_t val, enum cbm_type type) > +{ > + wrmsrl(((type == PSR_CBM_TYPE_L3_DATA) ? > + MSR_IA32_PSR_L3_MASK_DATA(cos) : > + MSR_IA32_PSR_L3_MASK_CODE(cos)), > + val); > +} > + > static const struct feat_props l3_cdp_props = { > .cos_num = 2, > .type[0] = PSR_CBM_TYPE_L3_DATA, > .type[1] = PSR_CBM_TYPE_L3_CODE, > .get_feat_info = l3_cdp_get_feat_info, > + .write_msr = l3_cdp_write_msr, Adding this hook only now means the earlier CDP patches must not be applied on their own. You should state this prominently (in the patch introducing l3_cdp_props) for whoever is going to eventually commit (parts of) this series. > @@ -805,17 +816,24 @@ static int insert_val_into_array(uint32_t val[], > if ( !psr_check_cbm(feat->cbm_len, new_val) ) > return -EINVAL; > > - /* Value setting position is same as feature array. */ > + /* > + * Value setting position is same as feature array. > + * For CDP, user may set both DATA and CODE to same value. For such case, > + * user input 'PSR_CBM_TYPE_L3' as type. The strict feature type of > + * 'PSR_CBM_TYPE_L3' is L3 CAT. So, we should set new_val to both of DATA > + * and CODE under such case. > + */ > for ( i = 0; i < props->cos_num; i++ ) > { > - if ( type == props->type[i] ) > + if ( type == props->type[i] || > + feat_type != psr_cbm_type_to_feat_type(type, true) ) While I think it is correct (at least up to the L2 CAT additions), it still seems fragile to me to use != here (effectively allowing any other type to come back). Couldn't props gain a field indicating the permitted alternative type? > { > val[i] = new_val; > - return 0; > + ret = 0; > } Wouldn't it be better to return -EINVAL in a to be added else branch here and ... > } > > - return -EINVAL; > + return ret; > } ... to return zero here? Jan
On 17-06-30 00:42:22, Jan Beulich wrote: > >>> Yi Sun <yi.y.sun@linux.intel.com> 06/14/17 3:26 AM >>> > > +static void l3_cdp_write_msr(unsigned int cos, uint32_t val, enum cbm_type type) > > +{ > > + wrmsrl(((type == PSR_CBM_TYPE_L3_DATA) ? > > + MSR_IA32_PSR_L3_MASK_DATA(cos) : > > + MSR_IA32_PSR_L3_MASK_CODE(cos)), > > + val); > > +} > > + > > static const struct feat_props l3_cdp_props = { > > .cos_num = 2, > > .type[0] = PSR_CBM_TYPE_L3_DATA, > > .type[1] = PSR_CBM_TYPE_L3_CODE, > > .get_feat_info = l3_cdp_get_feat_info, > > + .write_msr = l3_cdp_write_msr, > > Adding this hook only now means the earlier CDP patches must not be applied on > their own. You should state this prominently (in the patch introducing > l3_cdp_props) for whoever is going to eventually commit (parts of) this series. > Ok, I will highlight it in CDP init patch. Also apply it to L2 CAT patches. > > @@ -805,17 +816,24 @@ static int insert_val_into_array(uint32_t val[], > > if ( !psr_check_cbm(feat->cbm_len, new_val) ) > > return -EINVAL; > > > > - /* Value setting position is same as feature array. */ > > + /* > > + * Value setting position is same as feature array. > > + * For CDP, user may set both DATA and CODE to same value. For such case, > > + * user input 'PSR_CBM_TYPE_L3' as type. The strict feature type of > > + * 'PSR_CBM_TYPE_L3' is L3 CAT. So, we should set new_val to both of DATA > > + * and CODE under such case. > > + */ > > for ( i = 0; i < props->cos_num; i++ ) > > { > > - if ( type == props->type[i] ) > > + if ( type == props->type[i] || > > + feat_type != psr_cbm_type_to_feat_type(type, true) ) > > While I think it is correct (at least up to the L2 CAT additions), it still > seems fragile to me to use != here (effectively allowing any other type to > come back). Couldn't props gain a field indicating the permitted alternative > type? > Thanks for the good idea. Will add 'enum psr_feat_type alt_type;' in props to handle such case. > > { > > val[i] = new_val; > > - return 0; > > + ret = 0; > > } > > Wouldn't it be better to return -EINVAL in a to be added else branch here > and ... > > > } > > > > - return -EINVAL; > > + return ret; > > } > > ... to return zero here? > Sure. > Jan > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > https://lists.xen.org/xen-devel
On 17-06-30 15:22:56, Yi Sun wrote: > On 17-06-30 00:42:22, Jan Beulich wrote: > > > > @@ -805,17 +816,24 @@ static int insert_val_into_array(uint32_t val[], > > > if ( !psr_check_cbm(feat->cbm_len, new_val) ) > > > return -EINVAL; > > > > > > - /* Value setting position is same as feature array. */ > > > + /* > > > + * Value setting position is same as feature array. > > > + * For CDP, user may set both DATA and CODE to same value. For such case, > > > + * user input 'PSR_CBM_TYPE_L3' as type. The strict feature type of > > > + * 'PSR_CBM_TYPE_L3' is L3 CAT. So, we should set new_val to both of DATA > > > + * and CODE under such case. > > > + */ > > > for ( i = 0; i < props->cos_num; i++ ) > > > { > > > - if ( type == props->type[i] ) > > > + if ( type == props->type[i] || > > > + feat_type != psr_cbm_type_to_feat_type(type, true) ) > > > > While I think it is correct (at least up to the L2 CAT additions), it still > > seems fragile to me to use != here (effectively allowing any other type to > > come back). Couldn't props gain a field indicating the permitted alternative > > type? > > > Thanks for the good idea. Will add 'enum psr_feat_type alt_type;' in props > to handle such case. > > > > { > > > val[i] = new_val; > > > - return 0; > > > + ret = 0; > > > } > > > > Wouldn't it be better to return -EINVAL in a to be added else branch here > > and ... > > After reading codes again, I think we cannot return -EINVAL in else branch here. E.g. for CDP, user wants to set CODE. Then, the 'type' is CODE. At the first iteration, the props->type[0] is DATA which does not match 'type'. But we cannot return error here. We should iterate next 'type[]'. After iterating all type[], if we still do not find matching one, return the error back. So, I use 'ret' here. > > > } > > > > > > - return -EINVAL; > > > + return ret; > > > } > > > > ... to return zero here? > > > Sure. > > > Jan > > > > _______________________________________________ > > Xen-devel mailing list > > Xen-devel@lists.xen.org > > https://lists.xen.org/xen-devel > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > https://lists.xen.org/xen-devel
>>> Yi Sun <yi.y.sun@linux.intel.com> 06/30/17 10:56 AM >>> >On 17-06-30 15:22:56, Yi Sun wrote: >> On 17-06-30 00:42:22, Jan Beulich wrote: >> > > { >> > > val[i] = new_val; >> > > - return 0; >> > > + ret = 0; >> > > } >> > >> > Wouldn't it be better to return -EINVAL in a to be added else branch here >> > and ... >> > >After reading codes again, I think we cannot return -EINVAL in else branch here. >E.g. for CDP, user wants to set CODE. Then, the 'type' is CODE. At the first >iteration, the props->type[0] is DATA which does not match 'type'. But we cannot >return error here. We should iterate next 'type[]'. But that's why you're adding the second check in the if(). If neither of the two conditions are true, this is an error, isn't it? On the converse, why would you return 0 if the first loop iteration is fine but the second isn't. Jan
On 17-06-30 03:33:17, Jan Beulich wrote: > >>> Yi Sun <yi.y.sun@linux.intel.com> 06/30/17 10:56 AM >>> > >On 17-06-30 15:22:56, Yi Sun wrote: > >> On 17-06-30 00:42:22, Jan Beulich wrote: > >> > > { > >> > > val[i] = new_val; > >> > > - return 0; > >> > > + ret = 0; > >> > > } > >> > > >> > Wouldn't it be better to return -EINVAL in a to be added else branch here > >> > and ... > >> > > >After reading codes again, I think we cannot return -EINVAL in else branch here. > >E.g. for CDP, user wants to set CODE. Then, the 'type' is CODE. At the first > >iteration, the props->type[0] is DATA which does not match 'type'. But we cannot > >return error here. We should iterate next 'type[]'. > > But that's why you're adding the second check in the if(). If neither of the two > conditions are true, this is an error, isn't it? On the converse, why would you > return 0 if the first loop iteration is fine but the second isn't. > The second check in the if() is for the case to set both DATA and CODE at same time. The above example I wrote is for the case that user just wants to set CODE but not DATA. Under such case, the input 'feat_type' is CDP so that the second check is always false. The input 'type' is CODE. The props->type[0] is DATA and props->type[1] is CODE. In the first iteration, the props->type[0] is DATA so that it does not match 'type' and the second check is false too. If we use else branch here, it will enter the branch and return -EVINVAL. But this is not we want, right? We hope the second iteration should be executed to set CODE. > Jan
>>> Yi Sun <yi.y.sun@linux.intel.com> 06/30/17 1:30 PM >>> >The input 'type' is CODE. The props->type[0] is DATA and props->type[1] is CODE. >In the first iteration, the props->type[0] is DATA so that it does not match >'type' and the second check is false too. If we use else branch here, it will >enter the branch and return -EVINVAL. But this is not we want, right? We hope >the second iteration should be executed to set CODE. I see. That'll then call for yet another solution; I don't think the code should stay as is. Jan
On 17-06-30 06:02:32, Jan Beulich wrote: > >>> Yi Sun <yi.y.sun@linux.intel.com> 06/30/17 1:30 PM >>> > >The input 'type' is CODE. The props->type[0] is DATA and props->type[1] is CODE. > >In the first iteration, the props->type[0] is DATA so that it does not match > >'type' and the second check is false too. If we use else branch here, it will > >enter the branch and return -EVINVAL. But this is not we want, right? We hope > >the second iteration should be executed to set CODE. > > I see. That'll then call for yet another solution; I don't think the code should > stay as is. > Then, how about ASSERT() at the beginning to check if input 'type' is correct? enum cbm_type { PSR_CBM_TYPE_L3, PSR_CBM_TYPE_L3_DATA, PSR_CBM_TYPE_L3_CODE, PSR_CBM_TYPE_L2, }; ASSERT((type >= props->type[0] && type <= props->type[props->cos_num - 1]) || type == props->alt_type); We don't need 'ret' anymore with above check.
>>> On 03.07.17 at 08:33, <yi.y.sun@linux.intel.com> wrote: > On 17-06-30 06:02:32, Jan Beulich wrote: >> >>> Yi Sun <yi.y.sun@linux.intel.com> 06/30/17 1:30 PM >>> >> >The input 'type' is CODE. The props->type[0] is DATA and props->type[1] is CODE. >> >In the first iteration, the props->type[0] is DATA so that it does not match >> >'type' and the second check is false too. If we use else branch here, it will >> >enter the branch and return -EVINVAL. But this is not we want, right? We hope >> >the second iteration should be executed to set CODE. >> >> I see. That'll then call for yet another solution; I don't think the code should >> stay as is. >> > Then, how about ASSERT() at the beginning to check if input 'type' is > correct? > enum cbm_type { > PSR_CBM_TYPE_L3, > PSR_CBM_TYPE_L3_DATA, > PSR_CBM_TYPE_L3_CODE, > PSR_CBM_TYPE_L2, > }; > > ASSERT((type >= props->type[0] && type <= props->type[props->cos_num - 1]) || > type == props->alt_type); Baking in ordering assumptions? No, please don't. > We don't need 'ret' anymore with above check. So in a release build you'd then do what in case of a bad type finding its way in? Jan
On 17-07-03 01:01:09, Jan Beulich wrote: > >>> On 03.07.17 at 08:33, <yi.y.sun@linux.intel.com> wrote: > > On 17-06-30 06:02:32, Jan Beulich wrote: > >> >>> Yi Sun <yi.y.sun@linux.intel.com> 06/30/17 1:30 PM >>> > >> >The input 'type' is CODE. The props->type[0] is DATA and props->type[1] is CODE. > >> >In the first iteration, the props->type[0] is DATA so that it does not match > >> >'type' and the second check is false too. If we use else branch here, it will > >> >enter the branch and return -EVINVAL. But this is not we want, right? We hope > >> >the second iteration should be executed to set CODE. > >> > >> I see. That'll then call for yet another solution; I don't think the code should > >> stay as is. > >> > > Then, how about ASSERT() at the beginning to check if input 'type' is > > correct? > > enum cbm_type { > > PSR_CBM_TYPE_L3, > > PSR_CBM_TYPE_L3_DATA, > > PSR_CBM_TYPE_L3_CODE, > > PSR_CBM_TYPE_L2, > > }; > > > > ASSERT((type >= props->type[0] && type <= props->type[props->cos_num - 1]) || > > type == props->alt_type); > > Baking in ordering assumptions? No, please don't. > > > We don't need 'ret' anymore with above check. > > So in a release build you'd then do what in case of a bad type finding > its way in? > > Jan To decide the return value, we have to know if input 'type' is correct or not. There are two ways: 1. Check if input 'type' without iteration, like the above codes. Becaue you don't agree the ordering assumptions, this way is not good. 2. Use iteration, like the original codes. Record if the statement is hit. If yes, return 0. Otherwise, return -EINVAL. The original codes are below: for ( i = 0; i < props->cos_num; i++ ) { if ( type == props->type[i] || type == props->alt_type ) { val[i] = new_val; ret = 0; } } I think the main issue you don't like in the original codes is that the 'ret = 0' may happen for several times. How about below change? for ( i = 0; i < props->cos_num; i++ ) { if ( type == props->type[i] || type == props->alt_type ) { val[i] = new_val; if ( ret ) ret = 0; } }
>>> On 03.07.17 at 10:40, <yi.y.sun@linux.intel.com> wrote: > On 17-07-03 01:01:09, Jan Beulich wrote: >> >>> On 03.07.17 at 08:33, <yi.y.sun@linux.intel.com> wrote: >> > On 17-06-30 06:02:32, Jan Beulich wrote: >> >> >>> Yi Sun <yi.y.sun@linux.intel.com> 06/30/17 1:30 PM >>> >> >> >The input 'type' is CODE. The props->type[0] is DATA and props->type[1] is > CODE. >> >> >In the first iteration, the props->type[0] is DATA so that it does not match >> >> >'type' and the second check is false too. If we use else branch here, it > will >> >> >enter the branch and return -EVINVAL. But this is not we want, right? We > hope >> >> >the second iteration should be executed to set CODE. >> >> >> >> I see. That'll then call for yet another solution; I don't think the code > should >> >> stay as is. >> >> >> > Then, how about ASSERT() at the beginning to check if input 'type' is >> > correct? >> > enum cbm_type { >> > PSR_CBM_TYPE_L3, >> > PSR_CBM_TYPE_L3_DATA, >> > PSR_CBM_TYPE_L3_CODE, >> > PSR_CBM_TYPE_L2, >> > }; >> > >> > ASSERT((type >= props->type[0] && type <= props->type[props->cos_num - 1]) || >> > type == props->alt_type); >> >> Baking in ordering assumptions? No, please don't. >> >> > We don't need 'ret' anymore with above check. >> >> So in a release build you'd then do what in case of a bad type finding >> its way in? >> >> Jan > > To decide the return value, we have to know if input 'type' is correct or > not. > There are two ways: > 1. Check if input 'type' without iteration, like the above codes. Becaue you > don't agree the ordering assumptions, this way is not good. > 2. Use iteration, like the original codes. Record if the statement is hit. > If yes, return 0. Otherwise, return -EINVAL. The original codes are below: > for ( i = 0; i < props->cos_num; i++ ) > { > if ( type == props->type[i] || type == props->alt_type ) > { > val[i] = new_val; > ret = 0; > } > } > > I think the main issue you don't like in the original codes is that > the 'ret = 0' may happen for several times. How about below change? > for ( i = 0; i < props->cos_num; i++ ) > { > if ( type == props->type[i] || type == props->alt_type ) > { > val[i] = new_val; > if ( ret ) > ret = 0; > } > } No, the multiple assignments would be no issue at all. As said before, what I dislike is the wrongness of the return value if the first iteration sets ret to zero, but a subsequent one wouldn't. In that case, an error should be signaled. Jan
On 17-07-03 03:18:05, Jan Beulich wrote: > >>> On 03.07.17 at 10:40, <yi.y.sun@linux.intel.com> wrote: > > On 17-07-03 01:01:09, Jan Beulich wrote: > >> >>> On 03.07.17 at 08:33, <yi.y.sun@linux.intel.com> wrote: > >> > On 17-06-30 06:02:32, Jan Beulich wrote: > >> >> >>> Yi Sun <yi.y.sun@linux.intel.com> 06/30/17 1:30 PM >>> > > > > To decide the return value, we have to know if input 'type' is correct or > > not. > > There are two ways: > > 1. Check if input 'type' without iteration, like the above codes. Becaue you > > don't agree the ordering assumptions, this way is not good. > > 2. Use iteration, like the original codes. Record if the statement is hit. > > If yes, return 0. Otherwise, return -EINVAL. The original codes are below: > > for ( i = 0; i < props->cos_num; i++ ) > > { > > if ( type == props->type[i] || type == props->alt_type ) > > { > > val[i] = new_val; > > ret = 0; > > } > > } > > > > I think the main issue you don't like in the original codes is that > > the 'ret = 0' may happen for several times. How about below change? > > for ( i = 0; i < props->cos_num; i++ ) > > { > > if ( type == props->type[i] || type == props->alt_type ) > > { > > val[i] = new_val; > > if ( ret ) > > ret = 0; > > } > > } > > No, the multiple assignments would be no issue at all. As said > before, what I dislike is the wrongness of the return value if > the first iteration sets ret to zero, but a subsequent one > wouldn't. In that case, an error should be signaled. > Ok. Then, how about below change? Thanks! int ret = 0; for ( i = 0; i < props->cos_num; i++ ) { if ( type == props->type[i] ) { val[i] = new_val; ret = 0; break; } else if ( type == props->alt_type ) val[i] = new_val; else ret = -EINVAL; }
>>> On 03.07.17 at 14:52, <yi.y.sun@linux.intel.com> wrote: > Ok. Then, how about below change? Thanks! > int ret = 0; > for ( i = 0; i < props->cos_num; i++ ) > { > if ( type == props->type[i] ) > { > val[i] = new_val; > ret = 0; > break; > } > else if ( type == props->alt_type ) > val[i] = new_val; > else > ret = -EINVAL; > } This looks okay to me. Jan
diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c index aee6e3e..91b2122 100644 --- a/xen/arch/x86/psr.c +++ b/xen/arch/x86/psr.c @@ -209,7 +209,8 @@ static void free_socket_resources(unsigned int socket) bitmap_zero(info->dom_set, DOMID_IDLE + 1); } -static enum psr_feat_type psr_cbm_type_to_feat_type(enum cbm_type type) +static enum psr_feat_type psr_cbm_type_to_feat_type(enum cbm_type type, + bool strict) { enum psr_feat_type feat_type = PSR_SOCKET_FEAT_UNKNOWN; @@ -222,7 +223,7 @@ static enum psr_feat_type psr_cbm_type_to_feat_type(enum cbm_type type) * If type is L3 CAT but we cannot find it in feat_props array, * try CDP. */ - if ( !feat_props[feat_type] ) + if ( !feat_props[feat_type] && !strict ) feat_type = PSR_SOCKET_L3_CDP; break; @@ -358,11 +359,20 @@ static bool l3_cdp_get_feat_info(const struct feat_node *feat, return true; } +static void l3_cdp_write_msr(unsigned int cos, uint32_t val, enum cbm_type type) +{ + wrmsrl(((type == PSR_CBM_TYPE_L3_DATA) ? + MSR_IA32_PSR_L3_MASK_DATA(cos) : + MSR_IA32_PSR_L3_MASK_CODE(cos)), + val); +} + static const struct feat_props l3_cdp_props = { .cos_num = 2, .type[0] = PSR_CBM_TYPE_L3_DATA, .type[1] = PSR_CBM_TYPE_L3_CODE, .get_feat_info = l3_cdp_get_feat_info, + .write_msr = l3_cdp_write_msr, }; static void __init parse_psr_bool(char *s, char *value, char *feature, @@ -613,7 +623,7 @@ int psr_get_info(unsigned int socket, enum cbm_type type, if ( IS_ERR(info) ) return PTR_ERR(info); - feat_type = psr_cbm_type_to_feat_type(type); + feat_type = psr_cbm_type_to_feat_type(type, false); if ( feat_type >= ARRAY_SIZE(info->features) ) return -ENOENT; @@ -646,7 +656,7 @@ int psr_get_val(struct domain *d, unsigned int socket, if ( IS_ERR(info) ) return PTR_ERR(info); - feat_type = psr_cbm_type_to_feat_type(type); + feat_type = psr_cbm_type_to_feat_type(type, false); if ( feat_type >= ARRAY_SIZE(info->features) ) return -ENOENT; @@ -764,6 +774,7 @@ static int insert_val_into_array(uint32_t val[], const struct feat_node *feat; const struct feat_props *props; unsigned int i; + int ret = -EINVAL; ASSERT(feat_type < PSR_SOCKET_FEAT_NUM); @@ -805,17 +816,24 @@ static int insert_val_into_array(uint32_t val[], if ( !psr_check_cbm(feat->cbm_len, new_val) ) return -EINVAL; - /* Value setting position is same as feature array. */ + /* + * Value setting position is same as feature array. + * For CDP, user may set both DATA and CODE to same value. For such case, + * user input 'PSR_CBM_TYPE_L3' as type. The strict feature type of + * 'PSR_CBM_TYPE_L3' is L3 CAT. So, we should set new_val to both of DATA + * and CODE under such case. + */ for ( i = 0; i < props->cos_num; i++ ) { - if ( type == props->type[i] ) + if ( type == props->type[i] || + feat_type != psr_cbm_type_to_feat_type(type, true) ) { val[i] = new_val; - return 0; + ret = 0; } } - return -EINVAL; + return ret; } static int compare_val(const uint32_t val[], @@ -1124,7 +1142,7 @@ int psr_set_val(struct domain *d, unsigned int socket, if ( new_val != val ) return -EINVAL; - feat_type = psr_cbm_type_to_feat_type(type); + feat_type = psr_cbm_type_to_feat_type(type, false); if ( feat_type >= ARRAY_SIZE(info->features) || !info->features[feat_type] ) return -ENOENT;
This patch implements L3 CDP set value related callback function. With this patch, 'psr-cat-cbm-set' command can work for L3 CDP. Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com> --- v12: - add comment to explain how to deal with the case that user set new val for both DATA and CODE at same time. - add parameter for 'psr_cbm_type_to_feat_type' to return the feature type according to it. - use the feature type returned by 'psr_cbm_type_to_feat_type' to check if we need insert the new value into all items of the feature value array. - use conditional expression for wrmsrl. (suggested by Jan Beulich) v11: - move 'feat->cos_reg_val' assignment and value comparison in 'write_msr' callback function out as generic codes. (suggested by Jan Beulich) - changes about setting both CDP DATA and CODE at same time. - move 'type[]' declaration into previous patch which introduced 'cos_num'. (suggested by Jan Beulich) - changes about 'type[]'. (suggested by Jan Beulich) - move 'compare_val' to previous patch. (suggested by Jan Beulich) - changes about 'get_val' which has been replace by generic codes. (suggested by Jan Beulich) - remove 'restore_default_val' which is unnecessary now. (suggested by Jan Beulich) v10: - remove 'l3_cdp_get_old_val' and use 'l3_cdp_get_val' to replace it. (suggested by Jan Beulich) - remove 'l3_cdp_set_new_val'. - modify 'insert_val_to_array' flow to handle multiple COSs case. (suggested by Jan Beulich) - remove 'l3_cdp_compare_val' and implement a generic function 'comapre_val'. (suggested by Jan Beulich) - remove 'l3_cdp_fits_cos_max'. (suggested by Jan Beulich) - introduce macro 'PSR_MAX_COS_NUM'. - introduce a new member in 'feat_props', 'type[PSR_MAX_COS_NUM]' to record all 'cbm_type' the feature has. (suggested by Jan Beulich) - modify 'gather_val_array' flow to handle multiple COSs case. (suggested by Jan Beulich) - modify 'find_cos' flow and implement 'compare_val' to handle multiple COSs case. (suggested by Jan Beulich) - modify 'fits_cos_max' flow to handle multiple COSs case. (suggested by Jan Beulich) - changes about 'props'. (suggested by Jan Beulich) - remove cast in 'l3_cdp_write_msr'. (suggested by Jan Beulich) - implement 'compare_val' function to compare if feature values are what we expect in finding flow. - implement 'restore_default_val' function to restore feature's COS values to default if the feature has multiple COSs. It is called when the COS ID is reduced to 0. v9: - add comment to explain why CDP uses 2 COSs. (suggested by Wei Liu) - use 'cat_default_val'. (suggested by Wei Liu) - remove 'l3_cdp_get_cos_num' because we can directly get cos_num from feat_node now. (suggested by Jan Beulich) - remove cos checking because it has been moved to common function. (suggested by Jan Beulich) - l3_cdp_set_new_val parameter 'm' is changed to 'new_val'. (suggested by Jan Beulich) - directly use get_cdp_data(feat, 0) and get_cdp_code(feat, 0) to get default value. (suggested by Jan Beulich) - modify 'l3_cdp_write_msr' flow to write value into register according to input type. - changes about 'uint64_t' to 'uint32_t'. (suggested by Jan Beulich) v8: - modify 'l3_cdp_write_msr' type to 'void'. v5: - remove type check in callback function. (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) - remove 'l3_cdp_get_cos_max_from_type'. - rename 'l3_cdp_exceeds_cos_max' to 'l3_cdp_fits_cos_max'. (suggested by Jan Beulich) v4: - create this patch to make codes easier to understand. (suggested by Jan Beulich) --- --- xen/arch/x86/psr.c | 36 +++++++++++++++++++++++++++--------- 1 file changed, 27 insertions(+), 9 deletions(-)