On 17-04-05 09:37:44, Jan Beulich wrote:
> >>> On 01.04.17 at 15:53, <yi.y....@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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
