| Message ID | 1491054836-30488-8-git-send-email-yi.y.sun@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 01.04.17 at 15:53, <yi.y.sun@linux.intel.com> wrote: > --- a/xen/arch/x86/psr.c > +++ b/xen/arch/x86/psr.c > @@ -93,6 +93,10 @@ struct feat_node { > unsigned int cos_num; > unsigned int cos_max; > unsigned int cbm_len; > + > + /* 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); The comment isn't very helpful in its current shape. You really want to make clear that this is being used to return HW info via sysctl. Without this (and without seeing the rest of this patch), despite having seen previous versions my first thought was that this retrieves data from MSRs at initialization time. > @@ -183,6 +187,22 @@ static bool feat_init_done(const struct psr_socket_info *info) > return false; > } > > +static enum psr_feat_type psr_cbm_type_to_feat_type(enum cbm_type type) > +{ > + enum psr_feat_type feat_type; > + > + switch ( type ) > + { > + case PSR_CBM_TYPE_L3: > + feat_type = PSR_SOCKET_L3_CAT; > + break; > + default: > + ASSERT_UNREACHABLE(); > + } > + > + return feat_type; I'm pretty certain this will (validly) produce an uninitialized variable warning at least in a non-debug build. Not how I did say "add ASSERT_UNREACHABLE()" in the v9 review. > +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; > + > + if ( IS_ERR(info) ) > + return PTR_ERR(info); > + > + if ( !data ) > + return -EINVAL; I think I've asked this before - what does this check guard against? A bad caller? This could be an ASSERT() then. Returning an error to the sysctl caller because of some implementation bug seems pretty odd to me. > + feat_type = psr_cbm_type_to_feat_type(type); > + if ( feat_type > ARRAY_SIZE(info->features) ) > + return -ENOENT; > + > + feat = info->features[feat_type]; Please can you be more careful when adding checks like the above one? You still allow overrunning the array, when feat_type == ARRAY_SIZE(info->features). Jan
On 17-04-05 09:37:44, Jan Beulich wrote: > >>> On 01.04.17 at 15:53, <yi.y.sun@linux.intel.com> wrote: > > --- a/xen/arch/x86/psr.c > > +++ b/xen/arch/x86/psr.c > > @@ -93,6 +93,10 @@ struct feat_node { > > unsigned int cos_num; > > unsigned int cos_max; > > unsigned int cbm_len; > > + > > + /* 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); > > The comment isn't very helpful in its current shape. You really want > to make clear that this is being used to return HW info via sysctl. > Without this (and without seeing the rest of this patch), despite > having seen previous versions my first thought was that this > retrieves data from MSRs at initialization time. > Will modify the comment to make it clearer. > > @@ -183,6 +187,22 @@ static bool feat_init_done(const struct psr_socket_info *info) > > return false; > > } > > > > +static enum psr_feat_type psr_cbm_type_to_feat_type(enum cbm_type type) > > +{ > > + enum psr_feat_type feat_type; > > + > > + switch ( type ) > > + { > > + case PSR_CBM_TYPE_L3: > > + feat_type = PSR_SOCKET_L3_CAT; > > + break; > > + default: > > + ASSERT_UNREACHABLE(); > > + } > > + > > + return feat_type; > > I'm pretty certain this will (validly) produce an uninitialized variable > warning at least in a non-debug build. Not how I did say "add > ASSERT_UNREACHABLE()" in the v9 review. > Do you mean to init feat_type to 'PSR_SOCKET_MAX_FEAT' and then check it at the end of function using ASSERT? > > +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; > > + > > + if ( IS_ERR(info) ) > > + return PTR_ERR(info); > > + > > + if ( !data ) > > + return -EINVAL; > > I think I've asked this before - what does this check guard against? > A bad caller? This could be an ASSERT() then. Returning an error to > the sysctl caller because of some implementation bug seems pretty > odd to me. > Sorry, will use ASSERT for such cases. > > + feat_type = psr_cbm_type_to_feat_type(type); > > + if ( feat_type > ARRAY_SIZE(info->features) ) > > + return -ENOENT; > > + > > + feat = info->features[feat_type]; > > Please can you be more careful when adding checks like the above > one? You still allow overrunning the array, when feat_type > == ARRAY_SIZE(info->features). > Oh, sorry. > Jan
>>> On 06.04.17 at 08:05, <yi.y.sun@linux.intel.com> wrote: > On 17-04-05 09:37:44, Jan Beulich wrote: >> >>> On 01.04.17 at 15:53, <yi.y.sun@linux.intel.com> wrote: >> > @@ -183,6 +187,22 @@ static bool feat_init_done(const struct psr_socket_info *info) >> > return false; >> > } >> > >> > +static enum psr_feat_type psr_cbm_type_to_feat_type(enum cbm_type type) >> > +{ >> > + enum psr_feat_type feat_type; >> > + >> > + switch ( type ) >> > + { >> > + case PSR_CBM_TYPE_L3: >> > + feat_type = PSR_SOCKET_L3_CAT; >> > + break; >> > + default: >> > + ASSERT_UNREACHABLE(); >> > + } >> > + >> > + return feat_type; >> >> I'm pretty certain this will (validly) produce an uninitialized variable >> warning at least in a non-debug build. Not how I did say "add >> ASSERT_UNREACHABLE()" in the v9 review. >> > Do you mean to init feat_type to 'PSR_SOCKET_MAX_FEAT' and then check it > at the end of function using ASSERT? That's a (less desirable) option, but what I really mean is take v9 code and _add_ ASSERT_UNREACHABLE() first thing in the default case. Jan
On 17-04-06 02:36:19, Jan Beulich wrote: > >>> On 06.04.17 at 08:05, <yi.y.sun@linux.intel.com> wrote: > > On 17-04-05 09:37:44, Jan Beulich wrote: > >> >>> On 01.04.17 at 15:53, <yi.y.sun@linux.intel.com> wrote: > >> > @@ -183,6 +187,22 @@ static bool feat_init_done(const struct psr_socket_info *info) > >> > return false; > >> > } > >> > > >> > +static enum psr_feat_type psr_cbm_type_to_feat_type(enum cbm_type type) > >> > +{ > >> > + enum psr_feat_type feat_type; > >> > + > >> > + switch ( type ) > >> > + { > >> > + case PSR_CBM_TYPE_L3: > >> > + feat_type = PSR_SOCKET_L3_CAT; > >> > + break; > >> > + default: > >> > + ASSERT_UNREACHABLE(); > >> > + } > >> > + > >> > + return feat_type; > >> > >> I'm pretty certain this will (validly) produce an uninitialized variable > >> warning at least in a non-debug build. Not how I did say "add > >> ASSERT_UNREACHABLE()" in the v9 review. > >> > > Do you mean to init feat_type to 'PSR_SOCKET_MAX_FEAT' and then check it > > at the end of function using ASSERT? > > That's a (less desirable) option, but what I really mean is take v9 > code and _add_ ASSERT_UNREACHABLE() first thing in the default > case. > DYM we should initialize 'feat_type' to a valid value, e.g. PSR_SOCKET_L3_CAT and keep ASSERT_UNREACHABLE() in default case?
>>> On 06.04.17 at 13:16, <yi.y.sun@linux.intel.com> wrote: > On 17-04-06 02:36:19, Jan Beulich wrote: >> >>> On 06.04.17 at 08:05, <yi.y.sun@linux.intel.com> wrote: >> > On 17-04-05 09:37:44, Jan Beulich wrote: >> >> >>> On 01.04.17 at 15:53, <yi.y.sun@linux.intel.com> wrote: >> >> > @@ -183,6 +187,22 @@ static bool feat_init_done(const struct > psr_socket_info *info) >> >> > return false; >> >> > } >> >> > >> >> > +static enum psr_feat_type psr_cbm_type_to_feat_type(enum cbm_type type) >> >> > +{ >> >> > + enum psr_feat_type feat_type; >> >> > + >> >> > + switch ( type ) >> >> > + { >> >> > + case PSR_CBM_TYPE_L3: >> >> > + feat_type = PSR_SOCKET_L3_CAT; >> >> > + break; >> >> > + default: >> >> > + ASSERT_UNREACHABLE(); >> >> > + } >> >> > + >> >> > + return feat_type; >> >> >> >> I'm pretty certain this will (validly) produce an uninitialized variable >> >> warning at least in a non-debug build. Not how I did say "add >> >> ASSERT_UNREACHABLE()" in the v9 review. >> >> >> > Do you mean to init feat_type to 'PSR_SOCKET_MAX_FEAT' and then check it >> > at the end of function using ASSERT? >> >> That's a (less desirable) option, but what I really mean is take v9 >> code and _add_ ASSERT_UNREACHABLE() first thing in the default >> case. > > DYM we should initialize 'feat_type' to a valid value, e.g. PSR_SOCKET_L3_CAT > and keep ASSERT_UNREACHABLE() in default case? Yi, please. Did you read my previous reply, where I think I did say very precisely what I mean? Why would you pick some random valid type here? Jan
On 17-04-06 08:04:16, Jan Beulich wrote: > >>> On 06.04.17 at 13:16, <yi.y.sun@linux.intel.com> wrote: > > On 17-04-06 02:36:19, Jan Beulich wrote: > >> >>> On 06.04.17 at 08:05, <yi.y.sun@linux.intel.com> wrote: > >> > On 17-04-05 09:37:44, Jan Beulich wrote: > >> >> >>> On 01.04.17 at 15:53, <yi.y.sun@linux.intel.com> wrote: > >> >> > @@ -183,6 +187,22 @@ static bool feat_init_done(const struct > > psr_socket_info *info) > >> >> > return false; > >> >> > } > >> >> > > >> >> > +static enum psr_feat_type psr_cbm_type_to_feat_type(enum cbm_type type) > >> >> > +{ > >> >> > + enum psr_feat_type feat_type; > >> >> > + > >> >> > + switch ( type ) > >> >> > + { > >> >> > + case PSR_CBM_TYPE_L3: > >> >> > + feat_type = PSR_SOCKET_L3_CAT; > >> >> > + break; > >> >> > + default: > >> >> > + ASSERT_UNREACHABLE(); > >> >> > + } > >> >> > + > >> >> > + return feat_type; > >> >> > >> >> I'm pretty certain this will (validly) produce an uninitialized variable > >> >> warning at least in a non-debug build. Not how I did say "add > >> >> ASSERT_UNREACHABLE()" in the v9 review. > >> >> > >> > Do you mean to init feat_type to 'PSR_SOCKET_MAX_FEAT' and then check it > >> > at the end of function using ASSERT? > >> > >> That's a (less desirable) option, but what I really mean is take v9 > >> code and _add_ ASSERT_UNREACHABLE() first thing in the default > >> case. > > > > DYM we should initialize 'feat_type' to a valid value, e.g. PSR_SOCKET_L3_CAT > > and keep ASSERT_UNREACHABLE() in default case? > > Yi, please. Did you read my previous reply, where I think I did say > very precisely what I mean? Why would you pick some random > valid type here? > Sorry, I mis-understood your intention before. In v9 codes, I defined 'PSR_SOCKET_UNKNOWN' which is returned in default case. I thought you wanted me to remove it. I will add ASSERT_UNREACHABLE() and keep 'feat_type = PSR_SOCKET_UNKNOWN;' in default case. BRs, Sun Yi
diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c index 3421219..36ade48 100644 --- a/xen/arch/x86/psr.c +++ b/xen/arch/x86/psr.c @@ -93,6 +93,10 @@ struct feat_node { unsigned int cos_num; unsigned int cos_max; unsigned int cbm_len; + + /* 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); } *props; uint32_t cos_reg_val[MAX_COS_REG_CNT]; @@ -183,6 +187,22 @@ static bool feat_init_done(const struct psr_socket_info *info) return false; } +static enum psr_feat_type psr_cbm_type_to_feat_type(enum cbm_type type) +{ + enum psr_feat_type feat_type; + + switch ( type ) + { + case PSR_CBM_TYPE_L3: + feat_type = PSR_SOCKET_L3_CAT; + break; + default: + ASSERT_UNREACHABLE(); + } + + return feat_type; +} + /* CAT common functions implementation. */ static void cat_init_feature(const struct cpuid_leaf *regs, struct feat_node *feat, @@ -232,9 +252,23 @@ static void cat_init_feature(const struct cpuid_leaf *regs, socket, feat->props->cos_max, feat->props->cbm_len); } +static bool cat_get_feat_info(const struct feat_node *feat, + uint32_t data[], unsigned int array_len) +{ + if ( array_len != PSR_INFO_ARRAY_SIZE ) + return false; + + data[PSR_INFO_IDX_COS_MAX] = feat->props->cos_max; + data[PSR_INFO_IDX_CAT_CBM_LEN] = feat->props->cbm_len; + data[PSR_INFO_IDX_CAT_FLAG] = 0; + + return true; +} + /* L3 CAT ops */ static struct feat_props l3_cat_props = { .cos_num = 1, + .get_feat_info = cat_get_feat_info, }; static void __init parse_psr_bool(char *s, char *value, char *feature, @@ -446,10 +480,45 @@ void psr_ctxt_switch_to(struct domain *d) } } -int psr_get_cat_l3_info(unsigned int socket, uint32_t *cbm_len, - uint32_t *cos_max, uint32_t *flags) +static struct psr_socket_info *get_socket_info(unsigned int socket) { - return 0; + if ( !socket_info ) + return ERR_PTR(-ENODEV); + + if ( socket >= nr_sockets ) + return ERR_PTR(-ERANGE); + + if ( !feat_init_done(socket_info + socket) ) + return ERR_PTR(-ENOENT); + + return socket_info + socket; +} + +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; + + if ( IS_ERR(info) ) + return PTR_ERR(info); + + if ( !data ) + return -EINVAL; + + feat_type = psr_cbm_type_to_feat_type(type); + if ( feat_type > ARRAY_SIZE(info->features) ) + return -ENOENT; + + feat = info->features[feat_type]; + if ( !feat ) + return -ENOENT; + + if ( feat->props->get_feat_info(feat, data, array_len) ) + return 0; + + return -EINVAL; } int psr_get_l3_cbm(struct domain *d, unsigned int socket, diff --git a/xen/arch/x86/sysctl.c b/xen/arch/x86/sysctl.c index 2f7056e..c23270d 100644 --- a/xen/arch/x86/sysctl.c +++ b/xen/arch/x86/sysctl.c @@ -175,14 +175,25 @@ long arch_do_sysctl( switch ( sysctl->u.psr_cat_op.cmd ) { case XEN_SYSCTL_PSR_CAT_get_l3_info: - ret = psr_get_cat_l3_info(sysctl->u.psr_cat_op.target, - &sysctl->u.psr_cat_op.u.l3_info.cbm_len, - &sysctl->u.psr_cat_op.u.l3_info.cos_max, - &sysctl->u.psr_cat_op.u.l3_info.flags); + { + uint32_t data[PSR_INFO_ARRAY_SIZE]; + + ret = psr_get_info(sysctl->u.psr_cat_op.target, + PSR_CBM_TYPE_L3, data, ARRAY_SIZE(data)); + if ( ret ) + break; + + sysctl->u.psr_cat_op.u.l3_info.cos_max = + data[PSR_INFO_IDX_COS_MAX]; + sysctl->u.psr_cat_op.u.l3_info.cbm_len = + data[PSR_INFO_IDX_CAT_CBM_LEN]; + sysctl->u.psr_cat_op.u.l3_info.flags = + data[PSR_INFO_IDX_CAT_FLAG]; if ( !ret && __copy_field_to_guest(u_sysctl, sysctl, u.psr_cat_op) ) ret = -EFAULT; break; + } default: ret = -EOPNOTSUPP; diff --git a/xen/include/asm-x86/psr.h b/xen/include/asm-x86/psr.h index 57f47e9..af3a465 100644 --- a/xen/include/asm-x86/psr.h +++ b/xen/include/asm-x86/psr.h @@ -19,20 +19,26 @@ #include <xen/types.h> /* CAT cpuid level */ -#define PSR_CPUID_LEVEL_CAT 0x10 +#define PSR_CPUID_LEVEL_CAT 0x10 /* Resource Type Enumeration */ #define PSR_RESOURCE_TYPE_L3 0x2 /* L3 Monitoring Features */ -#define PSR_CMT_L3_OCCUPANCY 0x1 +#define PSR_CMT_L3_OCCUPANCY 0x1 /* CDP Capability */ -#define PSR_CAT_CDP_CAPABILITY (1u << 2) +#define PSR_CAT_CDP_CAPABILITY (1u << 2) /* L3 CDP Enable bit*/ #define PSR_L3_QOS_CDP_ENABLE_BIT 0x0 +/* Used by psr_get_info() */ +#define PSR_INFO_IDX_COS_MAX 0 +#define PSR_INFO_IDX_CAT_CBM_LEN 1 +#define PSR_INFO_IDX_CAT_FLAG 2 +#define PSR_INFO_ARRAY_SIZE 3 + struct psr_cmt_l3 { unsigned int features; unsigned int upscaling_factor; @@ -63,8 +69,8 @@ int psr_alloc_rmid(struct domain *d); void psr_free_rmid(struct domain *d); void psr_ctxt_switch_to(struct domain *d); -int psr_get_cat_l3_info(unsigned int socket, uint32_t *cbm_len, - uint32_t *cos_max, uint32_t *flags); +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_set_l3_cbm(struct domain *d, unsigned int socket,
This patch implements get HW info flow including L3 CAT callback function. It also changes sysctl interface to make it more general. With this patch, 'psr-hwinfo' can work for L3 CAT. Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com> --- v10: - remove 'PSR_SOCKET_UNKNOWN' and use 'ASSERT_UNREACHABLE()' to handle this case. (suggested by Jan Beulich) - check 'feat_type'. (suggested by Jan Beulich) - adjust macros names and values to make them more appropriate. (suggested by Jan Beulich) - use 'feat_init_done'. (suggested by Jan Beulich) - changes about 'cbm_len'. (suggested by Jan Beulich) v9: - replace feature list handling to feature array handling. (suggested by Roger Pau) - define 'PSR_INFO_SIZE'. (suggested by Roger Pau) - fix coding style issue. (suggested by Roger Pau and Jan Beulich) - use 'ARRAY_SIZE'. (suggested by Roger Pau) - rename 'l3_cat_get_feat_info' to 'cat_get_feat_info' to make it a common function for both L3/L2 CAT. (suggested by Roger Pau) - move constant to the right of comparison. (suggested by Wei Liu) - remove wrong comment. (suggested by Jan Beulich) - rename macros used by psr_get_info to make them meaningful. (suggested by Jan Beulich) - remove assignment for 'PSR_SOCKET_UNKNOWN'. (suggested by Jan Beulich) - retain blank line after 'case XEN_SYSCTL_PSR_CAT_get_l3_info'. (suggested by Jan Beulich) - modify patch title to indicate 'L3 CAT'. (suggested by Jan Beulich) - move common data check into common function. (suggested by Jan Beulich) v6: - fix coding style issue. (suggested by Konrad Rzeszutek Wilk) - define 'PSR_SOCKET_UNKNOWN' in 'psr_feat_type'. (suggested by Konrad Rzeszutek Wilk) - change '-ENOTSOCK' to 'ERANGE'. (suggested by Konrad Rzeszutek Wilk) - modify position of macros to remove odd spacing in psr.h. (suggested by Konrad Rzeszutek Wilk) v5: - change 'dat[]' to 'data[]'. (suggested by Jan Beulich) - modify parameter type to avoid fixed width type when there is no such intention. (suggested by Jan Beulich) - use 'const' when it is possible. (suggested by Jan Beulich) - check feature type outside callback function. (suggested by Jan Beulich) - modify macros names to add prefix 'PSR_' and change 'CDP_FLAG' to 'PSR_FLAG'. (suggested by Jan Beulich) v4: - create this patch to make codes easier to understand. (suggested by Jan Beulich) --- xen/arch/x86/psr.c | 75 +++++++++++++++++++++++++++++++++++++++++++++-- xen/arch/x86/sysctl.c | 19 +++++++++--- xen/include/asm-x86/psr.h | 16 ++++++---- 3 files changed, 98 insertions(+), 12 deletions(-)