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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 15.02.17 at 09:49, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
> Continue with patch:

???

> 'x86: refactor psr: set value: assemble features value array'

Or did you perhaps mean "continue from"?

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

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

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

> +        }
> +        *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?

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

> +            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"?

Jan


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