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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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_.

>> > +/* 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).

>> > +
>> > +
>> >  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?

No, no-one so far had the time to put one together.

Jan


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