Re: [Xen-devel] [PATCH v10 17/25] x86: refactor psr: CDP: implement set value callback functions.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 01.04.17 at 15:53, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
> @@ -94,6 +102,13 @@ struct feat_node {
>          unsigned int cos_max;
>          unsigned int cbm_len;
>  
> +        /*
> +         * An array to save all 'enum cbm_type' values of the feature. It is
> +         * used with cos_num together to get/write a feature's COS registers
> +         * values one by one.
> +         */
> +        enum cbm_type type[PSR_MAX_COS_NUM];

So here it finally comes. But it's needed the latest in the first CDP
patch, quite possibly even earlier.

> +static int compare_val(const uint32_t val[],
> +                       const struct feat_node *feat,
> +                       unsigned int cos)
> +{
> +    int rc = 0;

The variable deserves a better name and type bool, according to its
usage. But I'm unconvinced the variable is needed at all - I think
without it the intention of the function would be more clear. See
remarks further down.

> +    unsigned int i;
> +
> +    for ( i = 0; i < feat->props->cos_num; i++ )
> +    {
> +        uint32_t feat_val = 0;

Pointless initializer again.

> +        rc = 0;
> +
> +        /* If cos is bigger than cos_max, we need compare default value. */
> +        if ( cos > feat->props->cos_max )
> +        {
> +            /*
> +             * COS ID 0 always stores the default value so input 0 to get
> +             * default value.
> +             */
> +            feat->props->get_val(feat, 0, feat->props->type[i], &feat_val);
> +
> +            /*
> +             * If cos is bigger than feature's cos_max, the val should be
> +             * default value. Otherwise, it fails to find a COS ID. So we
> +             * have to exit find flow.
> +             */
> +            if ( val[i] != feat_val )
> +                return -EINVAL;
> +
> +            rc = 1;
> +            continue;

Drop these two.

> +        }

else
{

> +
> +        /* For CDP, DATA is the first item in val[], CODE is the second. */
> +        feat->props->get_val(feat, cos, feat->props->type[i], &feat_val);
> +        if ( val[i] != feat_val )
> +            break;

return 0;
}

> +
> +        rc = 1;

Drop.

> +    }
> +
> +    return rc;

return 1;

> @@ -851,43 +948,21 @@ static int find_cos(const uint32_t val[], unsigned int 
> array_len,
>           */
>          for ( i = 0; i < PSR_SOCKET_MAX_FEAT; i++ )
>          {
> -            uint32_t default_val = 0;
> -
>              feat = info->features[i];
>              if ( !feat )
>                  continue;
>  
>              /*
> -             * COS ID 0 always stores the default value so input 0 to get
> -             * default value.
> -             */
> -            feat->props->get_val(feat, 0, 0, &default_val);
> -
> -            /*
>               * Compare value according to feature array order.
>               * We must follow this order because value array is assembled
>               * as this order.
>               */
> -            if ( cos > feat->props->cos_max )
> -            {
> -                /*
> -                 * If cos is bigger than feature's cos_max, the val should be
> -                 * default value. Otherwise, it fails to find a COS ID. So we
> -                 * have to exit find flow.
> -                 */
> -                if ( val[0] != default_val )
> -                    return -EINVAL;
> -
> -                found = true;
> -            }
> -            else
> -            {
> -                if ( val[0] == feat->cos_reg_val[cos] )
> -                    found = true;
> -            }
> +            rc = compare_val(val, feat, cos);

Why is this being moved into a function here rather than being
introduced as a function right away?

> @@ -922,9 +997,14 @@ static bool fits_cos_max(const uint32_t val[],
>  
>          if ( cos > feat->props->cos_max )
>          {
> -            feat->props->get_val(feat, 0, 0, &default_val);
> -            if ( val[0] != default_val )
> -                return false;
> +            /* For CDP, DATA is the first item in val[], CODE is the second. 
> */

This CDP specific comment doesn't belong into a generic function.

> @@ -1033,6 +1116,40 @@ static int write_psr_msr(unsigned int socket, unsigned 
> int cos,
>      return 0;
>  }
>  
> +static void restore_default_val(unsigned int socket, unsigned int cos,
> +                                enum psr_feat_type feat_type)
> +{
> +    unsigned int i, j;
> +    uint32_t default_val;
> +    const struct psr_socket_info *info = get_socket_info(socket);
> +
> +    for ( i = 0; i < PSR_SOCKET_MAX_FEAT; i++ )
> +    {
> +        const struct feat_node *feat = info->features[i];

Blank line here.

> +        /*
> +         * There are four judgements:
> +         * 1. Input 'feat_type' is valid so we have to get feature according 
> to
> +         *    it. If current feature type (i) does not match 'feat_type', we
> +         *    need skip it, so continue to check next feature.
> +         * 2. Input 'feat_type' is 'PSR_SOCKET_MAX_FEAT' which means we 
> should
> +         *    handle all features in this case. So, go to next loop.
> +         * 3. Do not need restore the COS value back to default if cos_num 
> is 1,
> +         *    e.g. L3 CAT. Because next value setting will overwrite it.
> +         * 4. 'feat' we got is NULL, continue.
> +         */
> +        if ( ( feat_type != PSR_SOCKET_MAX_FEAT && feat_type != i ) ||

Stray blanks inside inner parentheses.

Jan

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