| Message ID | 1484805686-7249-8-git-send-email-yi.y.sun@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jan 19, 2017 at 02:01:09PM +0800, Yi Sun wrote: > This patch implements get value flow including L3 CAT callback > function. > > It also changes domctl interface to make it more general. > > With this patch, 'psr-cat-show' can work for L3 CAT. Could you add: "but not for L3 code/data which is implemented in patch titled XYZ" ? > > Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com> > --- > v5: > - rename 'dat[]' to 'data[]' > - modify variables names to make them better, e.g. 'feat_tmp' to 'feat'. > - check if feature type match in caller of feature callback function. > --- > xen/arch/x86/domctl.c | 18 +++++++++--------- > xen/arch/x86/psr.c | 41 ++++++++++++++++++++++++++++++++++++++--- > xen/include/asm-x86/psr.h | 4 ++-- > 3 files changed, 49 insertions(+), 14 deletions(-) > > diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c > index ab141b1..11d2127 100644 > --- a/xen/arch/x86/domctl.c > +++ b/xen/arch/x86/domctl.c > @@ -1383,23 +1383,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, > + &domctl->u.psr_cat_op.data, > + PSR_CBM_TYPE_L3); > 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, > + &domctl->u.psr_cat_op.data, > + PSR_CBM_TYPE_L3_CODE); > 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, > + &domctl->u.psr_cat_op.data, > + PSR_CBM_TYPE_L3_DATA); > copyback = 1; > break; > > diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c > index 319bfcc..3cbb60c 100644 > --- a/xen/arch/x86/psr.c > +++ b/xen/arch/x86/psr.c > @@ -112,6 +112,9 @@ struct feat_ops { > /* get_feat_info is used to get feature HW info. */ > bool (*get_feat_info)(const struct feat_node *feat, > uint32_t data[], unsigned int array_len); > + /* get_val is used to get feature COS register value. */ > + bool (*get_val)(const struct feat_node *feat, unsigned int cos, > + enum cbm_type type, uint64_t *val); > }; > > /* > @@ -251,9 +254,22 @@ static bool l3_cat_get_feat_info(const struct feat_node *feat, > return true; > } > > +static bool l3_cat_get_val(const struct feat_node *feat, unsigned int cos, > + enum cbm_type type, uint64_t *val) > +{ > + if ( cos > feat->info.l3_cat_info.cos_max ) > + /* Use default value. */ > + cos = 0; > + > + *val = feat->cos_reg_val[cos]; Extra space there. No need for it. > + > + return true; > +} > + > static const struct feat_ops l3_cat_ops = { > .get_cos_max = l3_cat_get_cos_max, > .get_feat_info = l3_cat_get_feat_info, > + .get_val = l3_cat_get_val, > }; > > static void __init parse_psr_bool(char *s, char *value, char *feature, > @@ -498,10 +514,29 @@ int psr_get_info(unsigned int socket, enum cbm_type type, > return -ENOENT; > } > > -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, > + uint64_t *val, enum cbm_type type) > { > - return 0; > + const struct psr_socket_info *info = get_socket_info(socket); > + unsigned int cos = d->arch.psr_cos_ids[socket]; > + const struct feat_node *feat; > + enum psr_feat_type feat_type; > + > + if ( IS_ERR(info) ) > + return PTR_ERR(info); > + > + feat_type = psr_cbm_type_to_feat_type(type); > + list_for_each_entry(feat, &info->feat_list, list) > + { > + if ( feat->feature != feat_type ) > + continue; > + > + if ( feat->ops.get_val(feat, cos, type, val) ) > + /* Found */ No need. The 'psr_get_info' does not have this. > + return 0; > + } > + > + return -ENOENT; This function looks quite similar to 'psr_get_info'. Perhaps it may make sense to have an common one that has an extra argument (whether to call get_val or get_feat_info)? And then psr_get_val and psr_get_info can call in this common code with this extra argument attached? > } > > int psr_set_l3_cbm(struct domain *d, unsigned int socket, > diff --git a/xen/include/asm-x86/psr.h b/xen/include/asm-x86/psr.h > index e3b18bc..d50e359 100644 > --- a/xen/include/asm-x86/psr.h > +++ b/xen/include/asm-x86/psr.h > @@ -70,8 +70,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, > + uint64_t *val, enum cbm_type type); > int psr_set_l3_cbm(struct domain *d, unsigned int socket, > uint64_t cbm, enum cbm_type type); > > -- > 1.9.1 >
Hi, Thanks for reviewing! I agree with your comments except below one. Could you please check my response? On 17-01-31 15:29:34, Konrad Rzeszutek Wilk wrote: > On Thu, Jan 19, 2017 at 02:01:09PM +0800, Yi Sun wrote: > > +int psr_get_val(struct domain *d, unsigned int socket, > > + uint64_t *val, enum cbm_type type) > > { > > - return 0; > > + const struct psr_socket_info *info = get_socket_info(socket); > > + unsigned int cos = d->arch.psr_cos_ids[socket]; > > + const struct feat_node *feat; > > + enum psr_feat_type feat_type; > > + > > + if ( IS_ERR(info) ) > > + return PTR_ERR(info); > > + > > + feat_type = psr_cbm_type_to_feat_type(type); > > + list_for_each_entry(feat, &info->feat_list, list) > > + { > > + if ( feat->feature != feat_type ) > > + continue; > > + > > + if ( feat->ops.get_val(feat, cos, type, val) ) > > + /* Found */ > > No need. The 'psr_get_info' does not have this. > > > + return 0; > > + } > > + > > + return -ENOENT; > > This function looks quite similar to 'psr_get_info'. > > Perhaps it may make sense to have an common one that has an > extra argument (whether to call get_val or get_feat_info)? > > And then psr_get_val and psr_get_info can call in this common > code with this extra argument attached? > Yes, the both functions are almost same. But I feel not good to combine them to one function. I think it makes the interface not explicit. As there are only 3 interfaces exposed by psr.c, I want to keep current implementation. Is that acceptable to you? Thanks, Sun Yi
On Tue, Feb 07, 2017 at 10:47:01AM +0800, Yi Sun wrote: > Hi, > > Thanks for reviewing! I agree with your comments except below one. Could you > please check my response? > > On 17-01-31 15:29:34, Konrad Rzeszutek Wilk wrote: > > On Thu, Jan 19, 2017 at 02:01:09PM +0800, Yi Sun wrote: > > > +int psr_get_val(struct domain *d, unsigned int socket, > > > + uint64_t *val, enum cbm_type type) > > > { > > > - return 0; > > > + const struct psr_socket_info *info = get_socket_info(socket); > > > + unsigned int cos = d->arch.psr_cos_ids[socket]; > > > + const struct feat_node *feat; > > > + enum psr_feat_type feat_type; > > > + > > > + if ( IS_ERR(info) ) > > > + return PTR_ERR(info); > > > + > > > + feat_type = psr_cbm_type_to_feat_type(type); > > > + list_for_each_entry(feat, &info->feat_list, list) > > > + { > > > + if ( feat->feature != feat_type ) > > > + continue; > > > + > > > + if ( feat->ops.get_val(feat, cos, type, val) ) > > > + /* Found */ > > > > No need. The 'psr_get_info' does not have this. > > > > > + return 0; > > > + } > > > + > > > + return -ENOENT; > > > > This function looks quite similar to 'psr_get_info'. > > > > Perhaps it may make sense to have an common one that has an > > extra argument (whether to call get_val or get_feat_info)? > > > > And then psr_get_val and psr_get_info can call in this common > > code with this extra argument attached? > > > Yes, the both functions are almost same. But I feel not good to combine them to > one function. I think it makes the interface not explicit. As there are only 3 > interfaces exposed by psr.c, I want to keep current implementation. Is that > acceptable to you? Keep the interface as is. You would have _three_ functions: psr_get_val psr_get_info and the one both of them would call which would be called: __psr_get which would do the heavy lifting. > > Thanks, > Sun Yi
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c index ab141b1..11d2127 100644 --- a/xen/arch/x86/domctl.c +++ b/xen/arch/x86/domctl.c @@ -1383,23 +1383,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, + &domctl->u.psr_cat_op.data, + PSR_CBM_TYPE_L3); 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, + &domctl->u.psr_cat_op.data, + PSR_CBM_TYPE_L3_CODE); 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, + &domctl->u.psr_cat_op.data, + PSR_CBM_TYPE_L3_DATA); copyback = 1; break; diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c index 319bfcc..3cbb60c 100644 --- a/xen/arch/x86/psr.c +++ b/xen/arch/x86/psr.c @@ -112,6 +112,9 @@ struct feat_ops { /* get_feat_info is used to get feature HW info. */ bool (*get_feat_info)(const struct feat_node *feat, uint32_t data[], unsigned int array_len); + /* get_val is used to get feature COS register value. */ + bool (*get_val)(const struct feat_node *feat, unsigned int cos, + enum cbm_type type, uint64_t *val); }; /* @@ -251,9 +254,22 @@ static bool l3_cat_get_feat_info(const struct feat_node *feat, return true; } +static bool l3_cat_get_val(const struct feat_node *feat, unsigned int cos, + enum cbm_type type, uint64_t *val) +{ + if ( cos > feat->info.l3_cat_info.cos_max ) + /* Use default value. */ + cos = 0; + + *val = feat->cos_reg_val[cos]; + + return true; +} + static const struct feat_ops l3_cat_ops = { .get_cos_max = l3_cat_get_cos_max, .get_feat_info = l3_cat_get_feat_info, + .get_val = l3_cat_get_val, }; static void __init parse_psr_bool(char *s, char *value, char *feature, @@ -498,10 +514,29 @@ int psr_get_info(unsigned int socket, enum cbm_type type, return -ENOENT; } -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, + uint64_t *val, enum cbm_type type) { - return 0; + const struct psr_socket_info *info = get_socket_info(socket); + unsigned int cos = d->arch.psr_cos_ids[socket]; + const struct feat_node *feat; + enum psr_feat_type feat_type; + + if ( IS_ERR(info) ) + return PTR_ERR(info); + + feat_type = psr_cbm_type_to_feat_type(type); + list_for_each_entry(feat, &info->feat_list, list) + { + if ( feat->feature != feat_type ) + continue; + + if ( feat->ops.get_val(feat, cos, type, val) ) + /* Found */ + return 0; + } + + return -ENOENT; } int psr_set_l3_cbm(struct domain *d, unsigned int socket, diff --git a/xen/include/asm-x86/psr.h b/xen/include/asm-x86/psr.h index e3b18bc..d50e359 100644 --- a/xen/include/asm-x86/psr.h +++ b/xen/include/asm-x86/psr.h @@ -70,8 +70,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, + uint64_t *val, enum cbm_type type); int psr_set_l3_cbm(struct domain *d, unsigned int socket, uint64_t cbm, enum cbm_type type);
This patch implements get value flow including L3 CAT callback function. It also changes domctl interface to make it more general. With this patch, 'psr-cat-show' can work for L3 CAT. Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com> --- v5: - rename 'dat[]' to 'data[]' - modify variables names to make them better, e.g. 'feat_tmp' to 'feat'. - check if feature type match in caller of feature callback function. --- xen/arch/x86/domctl.c | 18 +++++++++--------- xen/arch/x86/psr.c | 41 ++++++++++++++++++++++++++++++++++++++--- xen/include/asm-x86/psr.h | 4 ++-- 3 files changed, 49 insertions(+), 14 deletions(-)