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 06:51:07, Jan Beulich wrote:
> >>> On 27.03.17 at 14:24, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
> > 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?
> 
> That's a slightly difficult call: The assertion will expand to nothing
> in production builds, so to be on the safe side I think you better
> check function return values _everywhere_.
> 
Got it, will check feat_type.

> >> > +/* 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
> 
> But why would the array size be 3 for some feature only having
> COS_MAX (for example)? I think you should
> - put common indexes first
> - have PSR_INFO_CAT_ARRAY_SIZE (or PSR_INFO_NUM_IDX_CAT
> or whatever, but with CAT in it).
> 
Per current known features, all of them (CAT/MBA) have three members in
HW info. So, I think we can define ARRAY_SIZE to 3 and define its name to
a common name so far.

BRs,
Sun Yi

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