Re: [Xen-devel] [PATCH v9 11/25] x86: refactor psr: L3 CAT: set value: implement cos finding flow.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 16.03.17 at 12:08, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
> --- a/xen/arch/x86/psr.c
> +++ b/xen/arch/x86/psr.c
> @@ -123,6 +123,19 @@ struct feat_node {
>                             const struct feat_node *feat,
>                             enum cbm_type type,
>                             uint32_t new_val);
> +
> +        /*
> +         * 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:
> +         * 1 - find the entry in value array.

found ...

> +         * 0 - not find the entry in value array.

didn't find ...

> +static int cat_compare_val(const uint32_t val[],
> +                           const struct feat_node *feat,
> +                           unsigned int cos)
> +{
> +    /*
> +     * 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.cat_info.cos_max )
> +    {
> +        /* cos_reg_val[0] is the default value. */
> +        if ( val[0] != feat->cos_reg_val[0] )
> +            return -EINVAL;

As you can see, with cos_max moved into the generic portion of the
feature node, this entire check can move into the caller.

> +        /* Find */

Found (also below)

> +        return 1;
> +    }
> +
> +    if ( val[0] == feat->cos_reg_val[cos] )
> +        /* Find */
> +        return 1;
> +
> +    /* Not find */
> +    return 0;
> +}

Or actually, the entire function then becomes feature independent,
as it seems. And I think I did suggest that already during review of
an earlier version.

> @@ -752,7 +793,61 @@ static int find_cos(const uint32_t val[], uint32_t 
> array_len,
>                      enum psr_feat_type feat_type,
>                      const struct psr_socket_info *info)
>  {
> +    unsigned int cos, i;
> +    const unsigned int *ref = info->cos_ref;
> +    const struct feat_node *feat;
> +    const uint32_t *val_array = val;

The name doesn't match the purpose - as you increment the pointer,
its name should rather be "val_ptr" or some such.

> +    int find = 0;

"found" again, or even simply "rc"? Also I think this would better
move into the outer for() scope.

> +    unsigned int cos_max;
> +
>      ASSERT(spin_is_locked((spinlock_t *)(&info->ref_lock)));
> +
> +    /* cos_max is the one of the feature which is being set. */
> +    feat = info->features[feat_type];
> +    if ( !feat )
> +        return -ENOENT;
> +
> +    cos_max = feat->ops.get_cos_max(feat);
> +
> +    for ( cos = 0; cos <= cos_max; cos++ )
> +    {
> +        if ( cos && !ref[cos] )
> +            continue;
> +
> +        /*
> +         * If fail to find cos in below loop, need find whole feature array
> +         * again from beginning.
> +         */
> +        val_array = val;

You wouldn't need to re-do this here if you moved the variable
declaration (with initializer) into this scope. This then also
eliminates the need for the comment, which otherwise would
need its wording corrected.

> +        for ( i = 0; i < PSR_SOCKET_MAX_FEAT; i++ )
> +        {
> +            if ( !info->features[i] )
> +                continue;
> +
> +            feat = info->features[i];

Please swap if() and assignment, utilizing the local variable in the
if().

Jan


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