| Message ID | 1493801063-38513-8-git-send-email-yi.y.sun@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 03.05.17 at 10:44, <yi.y.sun@linux.intel.com> wrote: > --- a/xen/arch/x86/psr.c > +++ b/xen/arch/x86/psr.c > @@ -476,23 +476,34 @@ static struct psr_socket_info *get_socket_info(unsigned int socket) > return socket_info + socket; > } > > +static struct feat_node *psr_get_feat_and_type(unsigned int socket, > + enum cbm_type type, > + enum psr_feat_type *feat_type) > +{ > + const struct psr_socket_info *info = get_socket_info(socket); > + > + if ( IS_ERR(info) ) > + return ERR_PTR(PTR_ERR(info)); > + > + *feat_type = psr_cbm_type_to_feat_type(type); > + if ( *feat_type >= ARRAY_SIZE(info->features) ) > + return NULL; Note how this return is not being taken care of by ... > + return info->features[*feat_type]; > +} > + > int psr_get_info(unsigned int socket, enum cbm_type type, > uint32_t data[], unsigned int array_len) > { > - const struct psr_socket_info *info = get_socket_info(socket); > const struct feat_node *feat; > enum psr_feat_type feat_type; > > ASSERT(data); > > - if ( IS_ERR(info) ) > - return PTR_ERR(info); > - > - feat_type = psr_cbm_type_to_feat_type(type); > - if ( feat_type >= ARRAY_SIZE(info->features) ) > - return -ENOENT; > + feat = psr_get_feat_and_type(socket, type, &feat_type); > + if ( IS_ERR(feat) ) > + return PTR_ERR(feat); ... the check here. I think you want to alter the return above. And of course I wonder why you replace code here that was only introduced one or two patches earlier. Perhaps that earlier patch should do things this way right away? > - feat = info->features[feat_type]; > if ( !feat || !feat_props[feat_type] ) > return -ENOENT; Afaics you need feat_type here only to get at the right feat_props[] entry. If that's the case also for future callers of psr_get_feat_and_type(), perhaps it would be better for it to provide those two instead of the intermediate type? Of course that would imply renaming the function. (This change would clearly benefit the readability of psr_get_val() below.) > @@ -502,9 +513,38 @@ int psr_get_info(unsigned int socket, enum cbm_type type, > return -EINVAL; > } > > -int psr_get_l3_cbm(struct domain *d, unsigned int socket, > - uint64_t *cbm, enum cbm_type type) > +int psr_get_val(struct domain *d, unsigned int socket, > + uint32_t *val, enum cbm_type type) > { > + const struct feat_node *feat; > + enum psr_feat_type feat_type; > + unsigned int cos, i; > + > + ASSERT(val); > + > + feat = psr_get_feat_and_type(socket, type, &feat_type); > + if ( IS_ERR(feat) ) > + return PTR_ERR(feat); > + > + if ( !feat || !feat_props[feat_type] ) > + return -ENOENT; > + > + cos = d->arch.psr_cos_ids[socket]; > + /* > + * If input cos exceeds current feature's cos_max, we should return its > + * default value which is stored in cos 0. This case only happens > + * when more than two features enabled concurrently and at least one > + * features's cos_max is bigger than others. When a domain's working cos > + * id is bigger than some features' cos_max, HW automatically works as > + * default value for those features which cos_max is smaller. > + */ > + if ( cos > feat->cos_max ) > + cos = 0; > + > + for ( i = 0; i < feat_props[feat_type]->cos_num; i++ ) > + if ( type == feat_props[feat_type]->type[i] ) > + *val = feat->cos_reg_val[cos * feat_props[feat_type]->cos_num + i]; > + > return 0; > } Do you really want to return success here even if you didn't write to *val? With the way the callers are coded, this is an (at least latent) information leak at present. Jan
On 17-05-30 08:05:02, Jan Beulich wrote: > >>> On 03.05.17 at 10:44, <yi.y.sun@linux.intel.com> wrote: > > > --- a/xen/arch/x86/psr.c > > +++ b/xen/arch/x86/psr.c > > @@ -476,23 +476,34 @@ static struct psr_socket_info *get_socket_info(unsigned int socket) > > return socket_info + socket; > > } > > > > +static struct feat_node *psr_get_feat_and_type(unsigned int socket, > > + enum cbm_type type, > > + enum psr_feat_type *feat_type) > > +{ > > + const struct psr_socket_info *info = get_socket_info(socket); > > + > > + if ( IS_ERR(info) ) > > + return ERR_PTR(PTR_ERR(info)); > > + > > + *feat_type = psr_cbm_type_to_feat_type(type); > > + if ( *feat_type >= ARRAY_SIZE(info->features) ) > > + return NULL; > > Note how this return is not being taken care of by ... > > > + return info->features[*feat_type]; > > +} > > + > > int psr_get_info(unsigned int socket, enum cbm_type type, > > uint32_t data[], unsigned int array_len) > > { > > - const struct psr_socket_info *info = get_socket_info(socket); > > const struct feat_node *feat; > > enum psr_feat_type feat_type; > > > > ASSERT(data); > > > > - if ( IS_ERR(info) ) > > - return PTR_ERR(info); > > - > > - feat_type = psr_cbm_type_to_feat_type(type); > > - if ( feat_type >= ARRAY_SIZE(info->features) ) > > - return -ENOENT; > > + feat = psr_get_feat_and_type(socket, type, &feat_type); > > + if ( IS_ERR(feat) ) > > + return PTR_ERR(feat); > > ... the check here. I think you want to alter the return above. > This NULL is taken care by below code: if ( !feat || !feat_props[feat_type] ) The returned errors are handled separately. For PTR_ERR, it is handled above. For NULL, it is handled below. I checked IS_ERR, I think it can handle the NULL case. The NULL will not be treated as an error. > And of course I wonder why you replace code here that was > only introduced one or two patches earlier. Perhaps that earlier > patch should do things this way right away? > Because the helper function 'psr_get_feat_and_type' is only used by 'psr_get_info' if we implement it in previous patch. This seems unnecessary. So, I introduce this helper function in this patch. Shall I move it to previous patch? > > - feat = info->features[feat_type]; > > if ( !feat || !feat_props[feat_type] ) > > return -ENOENT; > > Afaics you need feat_type here only to get at the right feat_props[] > entry. If that's the case also for future callers of > psr_get_feat_and_type(), perhaps it would be better for it to > provide those two instead of the intermediate type? Of course > that would imply renaming the function. (This change would > clearly benefit the readability of psr_get_val() below.) > Ok, got it. > > @@ -502,9 +513,38 @@ int psr_get_info(unsigned int socket, enum cbm_type type, > > return -EINVAL; > > } > > > > -int psr_get_l3_cbm(struct domain *d, unsigned int socket, > > - uint64_t *cbm, enum cbm_type type) > > +int psr_get_val(struct domain *d, unsigned int socket, > > + uint32_t *val, enum cbm_type type) > > { > > + const struct feat_node *feat; > > + enum psr_feat_type feat_type; > > + unsigned int cos, i; > > + > > + ASSERT(val); > > + > > + feat = psr_get_feat_and_type(socket, type, &feat_type); > > + if ( IS_ERR(feat) ) > > + return PTR_ERR(feat); > > + > > + if ( !feat || !feat_props[feat_type] ) > > + return -ENOENT; > > + > > + cos = d->arch.psr_cos_ids[socket]; > > + /* > > + * If input cos exceeds current feature's cos_max, we should return its > > + * default value which is stored in cos 0. This case only happens > > + * when more than two features enabled concurrently and at least one > > + * features's cos_max is bigger than others. When a domain's working cos > > + * id is bigger than some features' cos_max, HW automatically works as > > + * default value for those features which cos_max is smaller. > > + */ > > + if ( cos > feat->cos_max ) > > + cos = 0; > > + > > + for ( i = 0; i < feat_props[feat_type]->cos_num; i++ ) > > + if ( type == feat_props[feat_type]->type[i] ) > > + *val = feat->cos_reg_val[cos * feat_props[feat_type]->cos_num + i]; > > + > > return 0; > > } > > Do you really want to return success here even if you didn't write > to *val? With the way the callers are coded, this is an (at least > latent) information leak at present. > Will fix it. Thanks! > Jan
>>> On 31.05.17 at 09:30, <yi.y.sun@linux.intel.com> wrote: > On 17-05-30 08:05:02, Jan Beulich wrote: >> >>> On 03.05.17 at 10:44, <yi.y.sun@linux.intel.com> wrote: >> >> > --- a/xen/arch/x86/psr.c >> > +++ b/xen/arch/x86/psr.c >> > @@ -476,23 +476,34 @@ static struct psr_socket_info *get_socket_info(unsigned int socket) >> > return socket_info + socket; >> > } >> > >> > +static struct feat_node *psr_get_feat_and_type(unsigned int socket, >> > + enum cbm_type type, >> > + enum psr_feat_type *feat_type) >> > +{ >> > + const struct psr_socket_info *info = get_socket_info(socket); >> > + >> > + if ( IS_ERR(info) ) >> > + return ERR_PTR(PTR_ERR(info)); >> > + >> > + *feat_type = psr_cbm_type_to_feat_type(type); >> > + if ( *feat_type >= ARRAY_SIZE(info->features) ) >> > + return NULL; >> >> Note how this return is not being taken care of by ... >> >> > + return info->features[*feat_type]; >> > +} >> > + >> > int psr_get_info(unsigned int socket, enum cbm_type type, >> > uint32_t data[], unsigned int array_len) >> > { >> > - const struct psr_socket_info *info = get_socket_info(socket); >> > const struct feat_node *feat; >> > enum psr_feat_type feat_type; >> > >> > ASSERT(data); >> > >> > - if ( IS_ERR(info) ) >> > - return PTR_ERR(info); >> > - >> > - feat_type = psr_cbm_type_to_feat_type(type); >> > - if ( feat_type >= ARRAY_SIZE(info->features) ) >> > - return -ENOENT; >> > + feat = psr_get_feat_and_type(socket, type, &feat_type); >> > + if ( IS_ERR(feat) ) >> > + return PTR_ERR(feat); >> >> ... the check here. I think you want to alter the return above. >> > This NULL is taken care by below code: > if ( !feat || !feat_props[feat_type] ) Oh, I see. > The returned errors are handled separately. For PTR_ERR, it is handled > above. For NULL, it is handled below. > > I checked IS_ERR, I think it can handle the NULL case. The NULL will not > be treated as an error. "it can handle the NULL case" is rather ambiguous: The NULL case normally (including the case here) also is an error, and IS_ERR() _does not_ detect this error. Hence using it one the result of a function that may return NULL is at least questionable (IS_ERR_OR_NULL() is intended to be used in such cases). So while indeed the code is correct as is, I'd still like to ask you to make the suggested change so that the code also ends up being visibly correct at the first glance. >> And of course I wonder why you replace code here that was >> only introduced one or two patches earlier. Perhaps that earlier >> patch should do things this way right away? >> > Because the helper function 'psr_get_feat_and_type' is only used by > 'psr_get_info' if we implement it in previous patch. This seems > unnecessary. So, I introduce this helper function in this patch. > Shall I move it to previous patch? I'd prefer if you did. When taking such decisions, please also consider the amount of churn you cause as well as reviewability. Jan
On 17-05-31 01:45:45, Jan Beulich wrote: > >>> On 31.05.17 at 09:30, <yi.y.sun@linux.intel.com> wrote: > > On 17-05-30 08:05:02, Jan Beulich wrote: > >> >>> On 03.05.17 at 10:44, <yi.y.sun@linux.intel.com> wrote: > >> > >> > --- a/xen/arch/x86/psr.c > >> > +++ b/xen/arch/x86/psr.c > >> > @@ -476,23 +476,34 @@ static struct psr_socket_info *get_socket_info(unsigned int socket) > >> > return socket_info + socket; > >> > } > >> > > >> > +static struct feat_node *psr_get_feat_and_type(unsigned int socket, > >> > + enum cbm_type type, > >> > + enum psr_feat_type *feat_type) > >> > +{ > >> > + const struct psr_socket_info *info = get_socket_info(socket); > >> > + > >> > + if ( IS_ERR(info) ) > >> > + return ERR_PTR(PTR_ERR(info)); > >> > + > >> > + *feat_type = psr_cbm_type_to_feat_type(type); > >> > + if ( *feat_type >= ARRAY_SIZE(info->features) ) > >> > + return NULL; > >> > >> Note how this return is not being taken care of by ... > >> > >> > + return info->features[*feat_type]; > >> > +} > >> > + > >> > int psr_get_info(unsigned int socket, enum cbm_type type, > >> > uint32_t data[], unsigned int array_len) > >> > { > >> > - const struct psr_socket_info *info = get_socket_info(socket); > >> > const struct feat_node *feat; > >> > enum psr_feat_type feat_type; > >> > > >> > ASSERT(data); > >> > > >> > - if ( IS_ERR(info) ) > >> > - return PTR_ERR(info); > >> > - > >> > - feat_type = psr_cbm_type_to_feat_type(type); > >> > - if ( feat_type >= ARRAY_SIZE(info->features) ) > >> > - return -ENOENT; > >> > + feat = psr_get_feat_and_type(socket, type, &feat_type); > >> > + if ( IS_ERR(feat) ) > >> > + return PTR_ERR(feat); > >> > >> ... the check here. I think you want to alter the return above. > >> > > This NULL is taken care by below code: > > if ( !feat || !feat_props[feat_type] ) > > Oh, I see. > > > The returned errors are handled separately. For PTR_ERR, it is handled > > above. For NULL, it is handled below. > > > > I checked IS_ERR, I think it can handle the NULL case. The NULL will not > > be treated as an error. > > "it can handle the NULL case" is rather ambiguous: The NULL > case normally (including the case here) also is an error, and > IS_ERR() _does not_ detect this error. Hence using it one the > result of a function that may return NULL is at least > questionable (IS_ERR_OR_NULL() is intended to be used in such > cases). > > So while indeed the code is correct as is, I'd still like to ask you > to make the suggested change so that the code also ends up > being visibly correct at the first glance. > Thank you! Will use 'IS_ERR_OR_NULL()' to check result. > >> And of course I wonder why you replace code here that was > >> only introduced one or two patches earlier. Perhaps that earlier > >> patch should do things this way right away? > >> > > Because the helper function 'psr_get_feat_and_type' is only used by > > 'psr_get_info' if we implement it in previous patch. This seems > > unnecessary. So, I introduce this helper function in this patch. > > Shall I move it to previous patch? > > I'd prefer if you did. When taking such decisions, please also consider > the amount of churn you cause as well as reviewability. > Got it, thanks! > Jan
>>> On 31.05.17 at 10:05, <yi.y.sun@linux.intel.com> wrote: > On 17-05-31 01:45:45, Jan Beulich wrote: >> >>> On 31.05.17 at 09:30, <yi.y.sun@linux.intel.com> wrote: >> > On 17-05-30 08:05:02, Jan Beulich wrote: >> >> >>> On 03.05.17 at 10:44, <yi.y.sun@linux.intel.com> wrote: >> >> >> >> > --- a/xen/arch/x86/psr.c >> >> > +++ b/xen/arch/x86/psr.c >> >> > @@ -476,23 +476,34 @@ static struct psr_socket_info > *get_socket_info(unsigned int socket) >> >> > return socket_info + socket; >> >> > } >> >> > >> >> > +static struct feat_node *psr_get_feat_and_type(unsigned int socket, >> >> > + enum cbm_type type, >> >> > + enum psr_feat_type > *feat_type) >> >> > +{ >> >> > + const struct psr_socket_info *info = get_socket_info(socket); >> >> > + >> >> > + if ( IS_ERR(info) ) >> >> > + return ERR_PTR(PTR_ERR(info)); >> >> > + >> >> > + *feat_type = psr_cbm_type_to_feat_type(type); >> >> > + if ( *feat_type >= ARRAY_SIZE(info->features) ) >> >> > + return NULL; >> >> >> >> Note how this return is not being taken care of by ... >> >> >> >> > + return info->features[*feat_type]; >> >> > +} >> >> > + >> >> > int psr_get_info(unsigned int socket, enum cbm_type type, >> >> > uint32_t data[], unsigned int array_len) >> >> > { >> >> > - const struct psr_socket_info *info = get_socket_info(socket); >> >> > const struct feat_node *feat; >> >> > enum psr_feat_type feat_type; >> >> > >> >> > ASSERT(data); >> >> > >> >> > - if ( IS_ERR(info) ) >> >> > - return PTR_ERR(info); >> >> > - >> >> > - feat_type = psr_cbm_type_to_feat_type(type); >> >> > - if ( feat_type >= ARRAY_SIZE(info->features) ) >> >> > - return -ENOENT; >> >> > + feat = psr_get_feat_and_type(socket, type, &feat_type); >> >> > + if ( IS_ERR(feat) ) >> >> > + return PTR_ERR(feat); >> >> >> >> ... the check here. I think you want to alter the return above. >> >> >> > This NULL is taken care by below code: >> > if ( !feat || !feat_props[feat_type] ) >> >> Oh, I see. >> >> > The returned errors are handled separately. For PTR_ERR, it is handled >> > above. For NULL, it is handled below. >> > >> > I checked IS_ERR, I think it can handle the NULL case. The NULL will not >> > be treated as an error. >> >> "it can handle the NULL case" is rather ambiguous: The NULL >> case normally (including the case here) also is an error, and >> IS_ERR() _does not_ detect this error. Hence using it one the >> result of a function that may return NULL is at least >> questionable (IS_ERR_OR_NULL() is intended to be used in such >> cases). >> >> So while indeed the code is correct as is, I'd still like to ask you >> to make the suggested change so that the code also ends up >> being visibly correct at the first glance. >> > Thank you! Will use 'IS_ERR_OR_NULL()' to check result. But that's not what I did suggest, and doing so will make the handling at the checking site more clumsy. Jan
On 17-05-31 02:10:27, Jan Beulich wrote: > >>> On 31.05.17 at 10:05, <yi.y.sun@linux.intel.com> wrote: > > On 17-05-31 01:45:45, Jan Beulich wrote: > >> >>> On 31.05.17 at 09:30, <yi.y.sun@linux.intel.com> wrote: > >> > On 17-05-30 08:05:02, Jan Beulich wrote: > >> >> >>> On 03.05.17 at 10:44, <yi.y.sun@linux.intel.com> wrote: > >> >> > >> >> > --- a/xen/arch/x86/psr.c > >> >> > +++ b/xen/arch/x86/psr.c > >> >> > @@ -476,23 +476,34 @@ static struct psr_socket_info > > *get_socket_info(unsigned int socket) > >> >> > return socket_info + socket; > >> >> > } > >> >> > > >> >> > +static struct feat_node *psr_get_feat_and_type(unsigned int socket, > >> >> > + enum cbm_type type, > >> >> > + enum psr_feat_type > > *feat_type) > >> >> > +{ > >> >> > + const struct psr_socket_info *info = get_socket_info(socket); > >> >> > + > >> >> > + if ( IS_ERR(info) ) > >> >> > + return ERR_PTR(PTR_ERR(info)); > >> >> > + > >> >> > + *feat_type = psr_cbm_type_to_feat_type(type); > >> >> > + if ( *feat_type >= ARRAY_SIZE(info->features) ) > >> >> > + return NULL; > >> >> > >> >> Note how this return is not being taken care of by ... > >> >> > >> >> > + return info->features[*feat_type]; > >> >> > +} > >> >> > + > >> >> > int psr_get_info(unsigned int socket, enum cbm_type type, > >> >> > uint32_t data[], unsigned int array_len) > >> >> > { > >> >> > - const struct psr_socket_info *info = get_socket_info(socket); > >> >> > const struct feat_node *feat; > >> >> > enum psr_feat_type feat_type; > >> >> > > >> >> > ASSERT(data); > >> >> > > >> >> > - if ( IS_ERR(info) ) > >> >> > - return PTR_ERR(info); > >> >> > - > >> >> > - feat_type = psr_cbm_type_to_feat_type(type); > >> >> > - if ( feat_type >= ARRAY_SIZE(info->features) ) > >> >> > - return -ENOENT; > >> >> > + feat = psr_get_feat_and_type(socket, type, &feat_type); > >> >> > + if ( IS_ERR(feat) ) > >> >> > + return PTR_ERR(feat); > >> >> > >> >> ... the check here. I think you want to alter the return above. > >> >> > >> > This NULL is taken care by below code: > >> > if ( !feat || !feat_props[feat_type] ) > >> > >> Oh, I see. > >> > >> > The returned errors are handled separately. For PTR_ERR, it is handled > >> > above. For NULL, it is handled below. > >> > > >> > I checked IS_ERR, I think it can handle the NULL case. The NULL will not > >> > be treated as an error. > >> > >> "it can handle the NULL case" is rather ambiguous: The NULL > >> case normally (including the case here) also is an error, and > >> IS_ERR() _does not_ detect this error. Hence using it one the > >> result of a function that may return NULL is at least > >> questionable (IS_ERR_OR_NULL() is intended to be used in such > >> cases). > >> > >> So while indeed the code is correct as is, I'd still like to ask you > >> to make the suggested change so that the code also ends up > >> being visibly correct at the first glance. > >> > > Thank you! Will use 'IS_ERR_OR_NULL()' to check result. > > But that's not what I did suggest, and doing so will make the > handling at the checking site more clumsy. > The 'psr_get_feat_and_type' make things complex here. I'd like to discard it and directly implement the functionality of it in 'psr_get_info' and 'psr_get_val' to make codes clear. > Jan
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c index 1220224..5b62c5c 100644 --- a/xen/arch/x86/domctl.c +++ b/xen/arch/x86/domctl.c @@ -1408,6 +1408,8 @@ long arch_do_domctl( case XEN_DOMCTL_psr_cat_op: switch ( domctl->u.psr_cat_op.cmd ) { + uint32_t val32; + case XEN_DOMCTL_PSR_CAT_OP_SET_L3_CBM: ret = psr_set_l3_cbm(d, domctl->u.psr_cat_op.target, domctl->u.psr_cat_op.data, @@ -1427,23 +1429,23 @@ long arch_do_domctl( break; case XEN_DOMCTL_PSR_CAT_OP_GET_L3_CBM: - ret = psr_get_l3_cbm(d, domctl->u.psr_cat_op.target, - &domctl->u.psr_cat_op.data, - PSR_CBM_TYPE_L3); + ret = psr_get_val(d, domctl->u.psr_cat_op.target, + &val32, PSR_CBM_TYPE_L3); + domctl->u.psr_cat_op.data = val32; copyback = 1; break; case XEN_DOMCTL_PSR_CAT_OP_GET_L3_CODE: - ret = psr_get_l3_cbm(d, domctl->u.psr_cat_op.target, - &domctl->u.psr_cat_op.data, - PSR_CBM_TYPE_L3_CODE); + ret = psr_get_val(d, domctl->u.psr_cat_op.target, + &val32, PSR_CBM_TYPE_L3_CODE); + domctl->u.psr_cat_op.data = val32; copyback = 1; break; case XEN_DOMCTL_PSR_CAT_OP_GET_L3_DATA: - ret = psr_get_l3_cbm(d, domctl->u.psr_cat_op.target, - &domctl->u.psr_cat_op.data, - PSR_CBM_TYPE_L3_DATA); + ret = psr_get_val(d, domctl->u.psr_cat_op.target, + &val32, PSR_CBM_TYPE_L3_DATA); + domctl->u.psr_cat_op.data = val32; copyback = 1; break; diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c index 2e6595d..1b781e1 100644 --- a/xen/arch/x86/psr.c +++ b/xen/arch/x86/psr.c @@ -476,23 +476,34 @@ static struct psr_socket_info *get_socket_info(unsigned int socket) return socket_info + socket; } +static struct feat_node *psr_get_feat_and_type(unsigned int socket, + enum cbm_type type, + enum psr_feat_type *feat_type) +{ + const struct psr_socket_info *info = get_socket_info(socket); + + if ( IS_ERR(info) ) + return ERR_PTR(PTR_ERR(info)); + + *feat_type = psr_cbm_type_to_feat_type(type); + if ( *feat_type >= ARRAY_SIZE(info->features) ) + return NULL; + + return info->features[*feat_type]; +} + int psr_get_info(unsigned int socket, enum cbm_type type, uint32_t data[], unsigned int array_len) { - const struct psr_socket_info *info = get_socket_info(socket); const struct feat_node *feat; enum psr_feat_type feat_type; ASSERT(data); - if ( IS_ERR(info) ) - return PTR_ERR(info); - - feat_type = psr_cbm_type_to_feat_type(type); - if ( feat_type >= ARRAY_SIZE(info->features) ) - return -ENOENT; + feat = psr_get_feat_and_type(socket, type, &feat_type); + if ( IS_ERR(feat) ) + return PTR_ERR(feat); - feat = info->features[feat_type]; if ( !feat || !feat_props[feat_type] ) return -ENOENT; @@ -502,9 +513,38 @@ int psr_get_info(unsigned int socket, enum cbm_type type, return -EINVAL; } -int psr_get_l3_cbm(struct domain *d, unsigned int socket, - uint64_t *cbm, enum cbm_type type) +int psr_get_val(struct domain *d, unsigned int socket, + uint32_t *val, enum cbm_type type) { + const struct feat_node *feat; + enum psr_feat_type feat_type; + unsigned int cos, i; + + ASSERT(val); + + feat = psr_get_feat_and_type(socket, type, &feat_type); + if ( IS_ERR(feat) ) + return PTR_ERR(feat); + + if ( !feat || !feat_props[feat_type] ) + return -ENOENT; + + cos = d->arch.psr_cos_ids[socket]; + /* + * If input cos exceeds current feature's cos_max, we should return its + * default value which is stored in cos 0. This case only happens + * when more than two features enabled concurrently and at least one + * features's cos_max is bigger than others. When a domain's working cos + * id is bigger than some features' cos_max, HW automatically works as + * default value for those features which cos_max is smaller. + */ + if ( cos > feat->cos_max ) + cos = 0; + + for ( i = 0; i < feat_props[feat_type]->cos_num; i++ ) + if ( type == feat_props[feat_type]->type[i] ) + *val = feat->cos_reg_val[cos * feat_props[feat_type]->cos_num + i]; + return 0; } diff --git a/xen/include/asm-x86/psr.h b/xen/include/asm-x86/psr.h index af3a465..7c6d38a 100644 --- a/xen/include/asm-x86/psr.h +++ b/xen/include/asm-x86/psr.h @@ -71,8 +71,8 @@ void psr_ctxt_switch_to(struct domain *d); int psr_get_info(unsigned int socket, enum cbm_type type, uint32_t data[], unsigned int array_len); -int psr_get_l3_cbm(struct domain *d, unsigned int socket, - uint64_t *cbm, enum cbm_type type); +int psr_get_val(struct domain *d, unsigned int socket, + uint32_t *val, enum cbm_type type); int psr_set_l3_cbm(struct domain *d, unsigned int socket, uint64_t cbm, enum cbm_type type);
There is an interface in user space to show feature value of domains. This patch implements get value flow in hypervisor. It also changes domctl interface to make it more general. With this patch, 'psr-cat-show' can work for L3 CAT but not for L3 code/data which is implemented in CDP related patches. Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com> --- v11: - declare a 'switch()' wide variable 'val32' in domctl. (suggested by Jan Beulich) - remove 'get_val' callback function which is replaced by generic codes. (suggested by Jan Beulich) - fix coding style issue. (suggested by Jan Beulich) - do not 'ASSERT' domain pointer. (suggested by Jan Beulich) - modify commit message. v10: - use an intermediate variable to get value and avoid cast in domctl. (suggested by Jan Beulich) - remove 'type' in 'get_val' parameters and will add it back when implementing CDP. (suggested by Jan Beulich) - remove unnecessary variable and return error about 'info' in 'psr_get_feat'. (suggested by Jan Beulich) - use 'ASSERT' to check input parameter in 'psr_get_val'. (suggested by Jan Beulich) - changes about 'feat_props'. (suggested by Jan Beulich) v9: - add commit message to explain there is an user space interface. - rename 'l3_cat_get_val' to 'cat_get_val' to cover all L3/L2 CAT features. (suggested by Roger Pau) - replace feature list handling to feature array handling. (suggested by Roger Pau) - change parameter of 'psr_get'. Use 'psr_cos_ids' directly to replace domain. Also declare it to 'const'. (suggested by Jan Beulich) - change code flow to remove 'psr_get' but add 'psr_get_feat' to make codes more reasonable. (suggested by Jan Beulich) - modify patch title to indicate 'L3 CAT'. (suggested by Jan Beulich) - move cos check into common function because this check is required by all features. (suggested by Jan Beulich) - fix coding style issue. (suggested by Jan Beulich) - changes about 'uint64_t' to 'uint32_t'. (suggested by Jan Beulich) v7: - rename '__psr_get' to 'psr_get'. (suggested by Wei Liu) v6: - modify commit message to make it clearer. (suggested by Konrad Rzeszutek Wilk) - remove one extra space in code. (suggested by Konrad Rzeszutek Wilk) - remove unnecessary comment. (suggested by Konrad Rzeszutek Wilk) - write a helper function to move get info and get val functions into it. Because most codes of 'get_info' and 'get_val' are same. (suggested by Konrad Rzeszutek Wilk) v5: - rename 'dat[]' to 'data[]' (suggested by Jan Beulich) - modify variables names to make them better, e.g. 'feat_tmp' to 'feat'. (suggested by Jan Beulich) - check if feature type match in caller of feature callback function. (suggested by Jan Beulich) v4: - create this patch to make codes easier to understand. (suggested by Jan Beulich) --- xen/arch/x86/domctl.c | 20 +++++++++------- xen/arch/x86/psr.c | 60 +++++++++++++++++++++++++++++++++++++++-------- xen/include/asm-x86/psr.h | 4 ++-- 3 files changed, 63 insertions(+), 21 deletions(-)