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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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

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

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

Jan


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