Re: [Xen-devel] [PATCH v4 09/24] x86: refactor psr: set value: assemble features value array.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 14.12.16 at 05:07, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
> --- a/xen/arch/x86/psr.c
> +++ b/xen/arch/x86/psr.c
> @@ -121,6 +121,35 @@ struct feat_ops {
>      /* get_val is used to get feature COS register value. */
>      bool (*get_val)(const struct feat_node *feat, unsigned int cos,
>                     enum cbm_type type, uint64_t *val);
> +    /*
> +     * get_cos_num is used to get the COS registers amount used by the
> +     * feature for one setting, e.g. CDP uses 2 COSs but CAT uses 1.
> +     */
> +    unsigned int (*get_cos_num)(const struct feat_node *feat);
> +    /*
> +     * get_old_val and set_new_val are a pair of functions called together.
> +     * The caller will traverse all features in the list and call both
> +     * functions for every feature to do below two things:
> +     * 1. get old_cos register value of all supported features and
> +     * 2. set the new value for the feature.
> +     *
> +     * All the values are set into value array according the traversal order,
> +     * meaning the same order of feature list members.
> +     *
> +     * 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.

Doesn't this match the get_cos_num() return value? Would be nice to
be spelled out (and, where possible, ASSERT()ed).

> @@ -186,6 +215,29 @@ static void free_feature(struct psr_socket_info *info)
>      }
>  }
>  
> +static bool_t psr_check_cbm(unsigned int cbm_len, uint64_t cbm)

bool (and then true/false in the function body)

> +static int l3_cat_set_new_val(uint64_t val[],
> +                              const struct feat_node *feat,
> +                              unsigned int old_cos,
> +                              enum cbm_type type,
> +                              uint64_t m)
> +{
> +    if ( type != PSR_CBM_TYPE_L3 )
> +        /* L3 CAT uses one COS. Skip it. */
> +        return 1;

If this is the wrong type, how can you return 1 (or any positive
value) here? This basically suggests that, as recommended for an
earlier operation already, that you want the type check done in
the caller. Or wait - isn't type not matching an error here, as you
iterate the same list twice in the same order? Which then gets us
back to me mentioning that the set-new-value should really be
done in common code, not in the callbacks; it would really only be
the check (psr_check_cbm() in the L3 case above) which would
require a callback.

Jan


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