| Message ID | 1487148579-7243-8-git-send-email-yi.y.sun@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Feb 15, 2017 at 04:49:22PM +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 but not for > L3 code/data which is implemented in patch "x86: refactor psr: > implement get value flow for CDP.". > > Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com> > Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > --- > xen/arch/x86/domctl.c | 18 +++++++++--------- > xen/arch/x86/psr.c | 43 ++++++++++++++++++++++++++++++++++++++----- > xen/include/asm-x86/psr.h | 4 ++-- > 3 files changed, 49 insertions(+), 16 deletions(-) [...] > diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c > index 8af59d9..c1afd36 100644 > --- a/xen/arch/x86/psr.c > +++ b/xen/arch/x86/psr.c > @@ -116,6 +116,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); > }; > > /* > @@ -260,9 +263,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; I don't know much, but shouldn't this return false instead of silently defaulting to 0? This doesn't seem to be what the caller expects. > + > + *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, > @@ -482,12 +498,14 @@ static struct psr_socket_info *get_socket_info(unsigned int socket) > return socket_info + socket; > } > > -int psr_get_info(unsigned int socket, enum cbm_type type, > - uint32_t data[], unsigned int array_len) > +static int psr_get(unsigned int socket, enum cbm_type type, > + uint32_t data[], unsigned int array_len, > + struct domain *d, uint64_t *val) > { > const struct psr_socket_info *info = get_socket_info(socket); > const struct feat_node *feat; > enum psr_feat_type feat_type; > + unsigned int cos; > > if ( IS_ERR(info) ) > return PTR_ERR(info); > @@ -498,6 +516,15 @@ int psr_get_info(unsigned int socket, enum cbm_type type, > if ( feat->feature != feat_type ) > continue; > > + if ( d ) > + { > + cos = d->arch.psr_cos_ids[socket]; > + if ( feat->ops.get_val(feat, cos, type, val) ) > + return 0; > + else No need for the "else" branch here. Roger.
On 17-02-28 12:44:34, Roger Pau Monn� wrote: > On Wed, Feb 15, 2017 at 04:49:22PM +0800, Yi Sun wrote: > > +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; > > I don't know much, but shouldn't this return false instead of silently > defaulting to 0? This doesn't seem to be what the caller expects. > If cos exceeds the cos_max, we should return default value saved in cos_reg_val[0]. Let me explain more, different features have different cos_max, e.g. L3 CAT cos_max=16, L2 CAT cos_max=8, user can set L3 CAT COS[9] for a domain. When COS ID 9 is set to ASSOC register, it works for all features. HW automatically works as default value for L2 CAT because the COS ID exceeds the max. > > @@ -498,6 +516,15 @@ int psr_get_info(unsigned int socket, enum cbm_type type, > > if ( feat->feature != feat_type ) > > continue; > > > > + if ( d ) > > + { > > + cos = d->arch.psr_cos_ids[socket]; > > + if ( feat->ops.get_val(feat, cos, type, val) ) > > + return 0; > > + else > > No need for the "else" branch here. > Thanks! > Roger.
>>> On 15.02.17 at 09:49, <yi.y.sun@linux.intel.com> wrote: > @@ -260,9 +263,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; This one never failing I wonder whether the same will apply to the later ones. If so, there's little point in returning a boolean here, but instead you could return the value instead of using indirection. > static void __init parse_psr_bool(char *s, char *value, char *feature, > @@ -482,12 +498,14 @@ static struct psr_socket_info *get_socket_info(unsigned int socket) > return socket_info + socket; > } > > -int psr_get_info(unsigned int socket, enum cbm_type type, > - uint32_t data[], unsigned int array_len) > +static int psr_get(unsigned int socket, enum cbm_type type, The immediately preceding patch introduced thus function, and now you're changing its name. Please give it the intended final name right away. > + uint32_t data[], unsigned int array_len, > + struct domain *d, uint64_t *val) const struct domain *, but I'm not even sure that's an appropriate parameter here: > @@ -498,6 +516,15 @@ int psr_get_info(unsigned int socket, enum cbm_type type, > if ( feat->feature != feat_type ) > continue; > > + if ( d ) > + { > + cos = d->arch.psr_cos_ids[socket]; You could equally well pass a more constrained pointer, like psr_cos_ids[] on its own. But of course much depends on whether you'll need d for other things in this function in later patches. > + if ( feat->ops.get_val(feat, cos, type, val) ) > + return 0; > + else > + break; > + } > + > if ( feat->ops.get_feat_info(feat, data, array_len) ) > return 0; > else Looking at the context here - is it really a good idea to overload the function in this way, rather than creating a second one? Your only complicating the live of the callers, as can be seen e.g. ... > @@ -507,10 +534,16 @@ 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_info(unsigned int socket, enum cbm_type type, > + uint32_t data[], unsigned int array_len) > +{ > + return psr_get(socket, type, data, array_len, NULL, NULL); ... here and ... > +} > + > +int psr_get_val(struct domain *d, unsigned int socket, > + uint64_t *val, enum cbm_type type) > { > - return 0; > + return psr_get(socket, type, NULL, 0, d, val); > } ... here (it is a bad sign that both pass NULL on either side). Jan
On 17-03-08 08:35:53, Jan Beulich wrote: > >>> On 15.02.17 at 09:49, <yi.y.sun@linux.intel.com> wrote: > > @@ -260,9 +263,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; > > This one never failing I wonder whether the same will apply to the > later ones. If so, there's little point in returning a boolean here, but > instead you could return the value instead of using indirection. > I have modified this function to 'void' in next version. > > static void __init parse_psr_bool(char *s, char *value, char *feature, > > @@ -482,12 +498,14 @@ static struct psr_socket_info *get_socket_info(unsigned int socket) > > return socket_info + socket; > > } > > > > -int psr_get_info(unsigned int socket, enum cbm_type type, > > - uint32_t data[], unsigned int array_len) > > +static int psr_get(unsigned int socket, enum cbm_type type, > > The immediately preceding patch introduced thus function, and > now you're changing its name. Please give it the intended final > name right away. > The name is not changed. You can see below lines. Here is just to implement a helper function which is called by 'psr_get_info'. > > + uint32_t data[], unsigned int array_len, > > + struct domain *d, uint64_t *val) > > const struct domain *, but I'm not even sure that's an appropriate > parameter here: > > > @@ -498,6 +516,15 @@ int psr_get_info(unsigned int socket, enum cbm_type type, > > if ( feat->feature != feat_type ) > > continue; > > > > + if ( d ) > > + { > > + cos = d->arch.psr_cos_ids[socket]; > > You could equally well pass a more constrained pointer, like > psr_cos_ids[] on its own. But of course much depends on whether > you'll need d for other things in this function in later patches. > Ok, thanks! Will consider the parameter. > > + if ( feat->ops.get_val(feat, cos, type, val) ) > > + return 0; > > + else > > + break; > > + } > > + > > if ( feat->ops.get_feat_info(feat, data, array_len) ) > > return 0; > > else > > Looking at the context here - is it really a good idea to overload the > function in this way, rather than creating a second one? Your > only complicating the live of the callers, as can be seen e.g. ... > These codes were separated into two functions before, 'psr_get_info' and 'psr_get_val'. But there are some common codes. So, Konrad suggested me to create a helper function to reduce redundant codes. > > @@ -507,10 +534,16 @@ 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_info(unsigned int socket, enum cbm_type type, > > + uint32_t data[], unsigned int array_len) > > +{ > > + return psr_get(socket, type, data, array_len, NULL, NULL); > > ... here and ... > > > +} > > + > > +int psr_get_val(struct domain *d, unsigned int socket, > > + uint64_t *val, enum cbm_type type) > > { > > - return 0; > > + return psr_get(socket, type, NULL, 0, d, val); > > } > > ... here (it is a bad sign that both pass NULL on either side). > Yes, these things look not good. But to keep a common helper I have to pass all necessary parameters into it. What is your suggestion? > Jan > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > https://lists.xen.org/xen-devel
>>> On 10.03.17 at 02:50, <yi.y.sun@linux.intel.com> wrote: > On 17-03-08 08:35:53, Jan Beulich wrote: >> >>> On 15.02.17 at 09:49, <yi.y.sun@linux.intel.com> wrote: >> > @@ -498,6 +516,15 @@ int psr_get_info(unsigned int socket, enum cbm_type type, >> > if ( feat->feature != feat_type ) >> > continue; >> > >> > + if ( d ) >> > + { >> > + cos = d->arch.psr_cos_ids[socket]; >> > + if ( feat->ops.get_val(feat, cos, type, val) ) >> > + return 0; >> > + else >> > + break; >> > + } >> > + >> > if ( feat->ops.get_feat_info(feat, data, array_len) ) >> > return 0; >> > else >> >> Looking at the context here - is it really a good idea to overload the >> function in this way, rather than creating a second one? Your >> only complicating the live of the callers, as can be seen e.g. ... >> > These codes were separated into two functions before, 'psr_get_info' and > 'psr_get_val'. But there are some common codes. So, Konrad suggested me > to create a helper function to reduce redundant codes. I think you went too far - breaking out common code is nice, but if the two callers now pass NULL/0 each as two of the last four arguments, this is a clear indication that you've folded more code than was actually common. >> > @@ -507,10 +534,16 @@ 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_info(unsigned int socket, enum cbm_type type, >> > + uint32_t data[], unsigned int array_len) >> > +{ >> > + return psr_get(socket, type, data, array_len, NULL, NULL); >> >> ... here and ... >> >> > +} >> > + >> > +int psr_get_val(struct domain *d, unsigned int socket, >> > + uint64_t *val, enum cbm_type type) >> > { >> > - return 0; >> > + return psr_get(socket, type, NULL, 0, d, val); >> > } >> >> ... here (it is a bad sign that both pass NULL on either side). >> > Yes, these things look not good. But to keep a common helper I have to pass > all necessary parameters into it. What is your suggestion? If there's really that much code to be shared (which I continue not to be convinced of), use of a function pointer may make this look a little better. But I think when having tried to carry out the earlier suggestion, you should have responded back right away indicating this would result in other ugliness. Jan
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c index 8e5259f..098e399 100644 --- a/xen/arch/x86/domctl.c +++ b/xen/arch/x86/domctl.c @@ -1440,23 +1440,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 8af59d9..c1afd36 100644 --- a/xen/arch/x86/psr.c +++ b/xen/arch/x86/psr.c @@ -116,6 +116,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); }; /* @@ -260,9 +263,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, @@ -482,12 +498,14 @@ static struct psr_socket_info *get_socket_info(unsigned int socket) return socket_info + socket; } -int psr_get_info(unsigned int socket, enum cbm_type type, - uint32_t data[], unsigned int array_len) +static int psr_get(unsigned int socket, enum cbm_type type, + uint32_t data[], unsigned int array_len, + struct domain *d, uint64_t *val) { const struct psr_socket_info *info = get_socket_info(socket); const struct feat_node *feat; enum psr_feat_type feat_type; + unsigned int cos; if ( IS_ERR(info) ) return PTR_ERR(info); @@ -498,6 +516,15 @@ int psr_get_info(unsigned int socket, enum cbm_type type, if ( feat->feature != feat_type ) continue; + if ( d ) + { + cos = d->arch.psr_cos_ids[socket]; + if ( feat->ops.get_val(feat, cos, type, val) ) + return 0; + else + break; + } + if ( feat->ops.get_feat_info(feat, data, array_len) ) return 0; else @@ -507,10 +534,16 @@ 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_info(unsigned int socket, enum cbm_type type, + uint32_t data[], unsigned int array_len) +{ + return psr_get(socket, type, data, array_len, NULL, NULL); +} + +int psr_get_val(struct domain *d, unsigned int socket, + uint64_t *val, enum cbm_type type) { - return 0; + return psr_get(socket, type, NULL, 0, d, val); } 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 0342a80..569e7df 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);