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

[Yi Sun <yi.y.sun@xxxxxxxxxxxxxxx>]

On 17-04-11 10:03:21, Jan Beulich wrote:
> >>> 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.
> 
Hmm, ok, as the previous patches make you have some confusions. I will move
this to the first CDP patch.

> > +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;
> 
Thanks! My original intention is to avoid the 'else' so that no indentations.
But it seems 'else' is good to you so I will change it to above codes.

> > @@ -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?
> 
In L3 CAT patch, this part looks simple so I did not encapulate them into a
function. If you prefer a function here, I will do it at the beginning.

> > @@ -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.
> 
Ok, will remove it.

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

> > +        /*
> > +         * 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.
> 
Ok, will remove them.

> Jan

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