Re: [PATCH v8 10/24] x86: refactor psr: set value: implement cos finding flow.

[Yi Sun <yi.y.sun@linux.intel.com>]

On 17-03-08 10:03:10, Jan Beulich wrote:
> >>> On 15.02.17 at 09:49, <yi.y.sun@linux.intel.com> wrote:
> > Continue with patch:
> 
> ???
> 
> > 'x86: refactor psr: set value: assemble features value array'
> 
> Or did you perhaps mean "continue from"?
> 
Should be 'from'. Thanks!

> > --- a/xen/arch/x86/psr.c
> > +++ b/xen/arch/x86/psr.c
> > @@ -145,6 +145,19 @@ struct feat_ops {
> >                         const struct feat_node *feat,
> >                         enum cbm_type type,
> >                         uint64_t m);
> > +    /*
> > +     * compare_val is used in set value process to compare if the
> > +     * input value array can match all the features' COS registers values
> > +     * according to input cos id.
> > +     *
> > +     * The return value is the amount of entries to skip in the value array
> > +     * or error.
> > +     * 1 - one entry in value array.
> > +     * 2 - two entries in value array, e.g. CDP uses two entries.
> 
> Isn't this get_cos_num()'s result again? Or, looking at the function
> below, is the comment simply stale?
> 
Sorry, forgot to update it.

> > @@ -356,6 +369,34 @@ static int l3_cat_set_new_val(uint64_t val[],
> >      return 0;
> >  }
> >  
> > +static int l3_cat_compare_val(const uint64_t val[],
> > +                              const struct feat_node *feat,
> > +                              unsigned int cos, bool *found)
> > +{
> > +    uint64_t l3_def_cbm;
> > +
> > +    l3_def_cbm = (1ull << feat->info.l3_cat_info.cbm_len) - 1;
> 
> Please only calculate the value on the path you need it. Also this
> will again degenerate of cbm_len == 64.
> 
Sorry, what do you mean? I need get the default value of L3 CAT here
to check if input val equals the default one if cos exceeds cos_max.

As explanation in previous patch, cbm_len is not 64. It means how many bits
the CBM value has. E.g. L3 CAT may have 11 bits. L2 CAT may have 8 bits.

> > +    /*
> > +     * Different features' cos_max are different. If cos id of the feature
> > +     * being set exceeds other feature's cos_max, the val of other feature
> > +     * must be default value. HW supports such case.
> > +     */
> > +    if ( cos > feat->info.l3_cat_info.cos_max )
> > +    {
> > +        if ( val[0] != l3_def_cbm )
> > +        {
> > +            *found = false;
> > +            return -ENOENT;
> 
> What is the difference between this "not found" and ...
> 
This is an error case. If input cos exceeds the cos_max and the input val is
not default value, that means the input parameters exceed HW ability. We should
report error back.

> > +        }
> > +        *found = true;
> > +    }
> > +    else
> > +        *found = (val[0] == feat->cos_reg_val[cos]);
> > +
> > +    return 0;
> 
> ... the possible one here? I.e. why once -ENOENT and once 0 as
> return value?
> 
0 means success. '*found' means if the val is found or not. Per Roger's
suggestion, I will refine this function to use return value to check
if it is found or not.

> > @@ -715,6 +757,57 @@ static int find_cos(const uint64_t *val, uint32_t array_len,
> >                      enum psr_feat_type feat_type,
> >                      const struct psr_socket_info *info)
> >  {
> > +    unsigned int cos;
> > +    const unsigned int *ref = info->cos_ref;
> > +    const struct feat_node *feat;
> > +    const uint64_t *val_tmp = val;
> > +    int ret;
> > +    bool found = false;
> > +    unsigned int cos_max = 0;
> > +
> > +    /* cos_max is the one of the feature which is being set. */
> > +    list_for_each_entry(feat, &info->feat_list, list)
> > +    {
> > +        if ( feat->feature != feat_type )
> > +            continue;
> > +
> > +        cos_max = feat->ops.get_cos_max(feat);
> > +        if ( cos_max > 0 )
> 
> What's the purpose of this check? I.e. why do you continue the loop
> in case you find zero? You won't find another node with the same
> feat_type, will you?
> 
This is corrected in next version. Thanks!

> > +            break;
> > +    }
> > +
> > +    for ( cos = 0; cos <= cos_max; cos++ )
> > +    {
> > +        if ( cos && !ref[cos] )
> > +            continue;
> > +
> > +        /* Not found, need find again from beginning. */
> 
> You may not even have looked yet, so how can you say "not found"?
> 
Sorry, it should be "If not found, need find again...".

> Jan

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