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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 15.02.17 at 09:49, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
> --- a/xen/arch/x86/psr.c
> +++ b/xen/arch/x86/psr.c
> @@ -119,6 +119,32 @@ 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);

Please have blank lines ahead of every separating comment. (Of
course this then may need to start already in earlier patches.)

> +    /*
> +     * get_old_val and set_new_val are a pair of functions called in order.
> +     * 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.

This is misleading, I think: I don't think a new value is being set for
every feature. This should be worded less ambiguously.

> @@ -207,6 +233,29 @@ static enum psr_feat_type psr_cbm_type_to_feat_type(enum 
> cbm_type type)
>      return feat_type;
>  }
>  
> +static bool psr_check_cbm(unsigned int cbm_len, uint64_t cbm)
> +{
> +    unsigned int first_bit, zero_bit;
> +
> +    /* Set bits should only in the range of [0, cbm_len). */
> +    if ( cbm & (~0ull << cbm_len) )

This will not do what you intend for cbm_len == 64.

> +static int l3_cat_get_old_val(uint64_t val[],
> +                              const struct feat_node *feat,
> +                              unsigned int old_cos)
> +{
> +    if ( old_cos > feat->info.l3_cat_info.cos_max )

Afaics this condition is the only L3 CAT specific thing in this function.
Should more of it be moved into common code? Same below for
set_new_val.

> -static int assemble_val_array(uint64_t *val,
> +static int combine_val_array(uint64_t *val,

Same comment as earlier on - please decide for a final name right
when introducing a function. In fact I'd prefer it to remain
"assemble".

>  {
> -    return -EINVAL;
> +    const struct feat_node *feat;
> +    int ret;
> +    uint64_t *val_tmp = val;

I don't really see the need for this helper variable. Simply ...

> +    if ( !val )
> +        return -EINVAL;
> +
> +    /* Get all features current values according to old_cos. */
> +    list_for_each_entry(feat, &info->feat_list, list)
> +    {
> +        /* value getting order is same as feature list */
> +        ret = feat->ops.get_old_val(val_tmp, feat, old_cos);
> +        if ( ret )
> +            return ret;
> +
> +        val_tmp += feat->ops.get_cos_num(feat);

... use val here, after checking the return value against
array_len, and the also subtract from array_len. (I am averse
to _tmp suffixes, I'm sorry.)

Btw - for any of the later features, does their get_cos_num() ever
return other than a constant value? If not, there's no point in making
this a function call - you could simply have a numeric member in the
structure.

> @@ -567,7 +678,37 @@ static int set_new_val_to_array(uint64_t *val,
>                                  enum cbm_type type,
>                                  uint64_t m)
>  {
> -    return -EINVAL;
> +    const struct feat_node *feat;
> +    int ret;
> +    uint64_t *val_tmp = val;
> +
> +    /* Set new value into array according to feature's position in array. */
> +    list_for_each_entry(feat, &info->feat_list, list)
> +    {
> +        if ( feat->feature != feat_type )
> +        {
> +            val_tmp += feat->ops.get_cos_num(feat);
> +            if ( val_tmp - val > array_len)
> +                return -ENOSPC;
> +
> +            continue;
> +        }
> +
> +        /*
> +         * Value setting position is same as feature list.
> +         * Different features may have different setting behaviors, e.g. CDP
> +         * has two values (DATA/CODE) which need us to save input value to
> +         * different position in the array according to type, so we have to
> +         * maintain a callback function.
> +         */
> +        ret = feat->ops.set_new_val(val_tmp, feat, type, m);
> +        if ( ret )
> +            return ret;
> +        else

Pointless "else" again. I think I'll try to avoid pointing this out, should
more of these appear - please simply go through yourself any remove
any such uses.

Jan

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