Re: [Xen-devel] [PATCH v10 07/25] x86: refactor psr: L3 CAT: implement get hw info flow.

[Yi Sun <yi.y.sun@xxxxxxxxxxxxxxx>]

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