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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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().

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

> +/* Used by psr_get_info() */
> +#define PSR_INFO_IDX_CBM_LEN            0
> +#define PSR_INFO_IDX_COS_MAX            1
> +#define PSR_INFO_IDX_FLAG               2
> +#define PSR_INFO_CAT_SIZE               3

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

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

Jan


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