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

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

On 17-03-27 03:07:37, Jan Beulich wrote:
> >>> On 16.03.17 at 12:07, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
> > +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:
> > +        feat_type = PSR_SOCKET_UNKNOWN;
> > +        break;
> 
> Is this actually reachable, if there are no bugs in the code? If not,
> you will want to add ASSERT_UNREACHABLE().
> 
If there is no bug, we should not reach here. Will use ASSERT_UNREACHABLE().

> > +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);
> > +    feat = info->features[feat_type];
> 
> You can't blindly use the return value here as array index, as (at
> least in theory, see above) the function may return
> PSR_SOCKET_UNKNOWN. IOW you need to check against
> ARRAY_SIZE(info->features) first.
> 
If I use 'ASSERT_UNREACHABLE()' above, I don't need check against
ARRAY_SIZE(info->features) here, right?

> > +/* Used by psr_get_info() */
> > +#define PSR_INFO_IDX_CBM_LEN            0
CAT/CDP specific.

> > +#define PSR_INFO_IDX_COS_MAX            1
Common so far.

> > +#define PSR_INFO_IDX_FLAG               2
CAT/CDP specific so far.

> > +#define PSR_INFO_CAT_SIZE               3
Array size which can be used for all features but may not be appropriate for
future features. So I defined it as CAT specific.

> 
> So I need some explanation on the naming here: Are the first three
> CAT-independent, but the last one is CAT-dependent? It doesn't
> look so (or else it would be odd coincidence for the last one to be
> one higher than the biggest of the _IDX ones). And if they're all
> in either of the two categories, their names should reflect that
> (i.e. either all have _CAT in their names, or none does).
> 
Please check above comments. Maybe below definitions are better?
PSR_INFO_IDX_CAT_CBM_LEN
PSR_INFO_IDX_COS_MAX
PSR_INFO_IDX_CAT_FLAG
PSR_INFO_ARRAY_SIZE

> > +
> > +
> >  struct psr_cmt_l3 {
> 
> No double blank lines please (and just in case: comments of this kind
> apply to the entire series, i.e. you shouldn't expect them to be
> repeated in other patches).
> 
Sorry for this. Do we have code style check tool in xen?

> Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel