Re: [Xen-devel] [PATCH v9 10/25] x86: refactor psr: L3 CAT: set value: assemble features value array.

["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
> @@ -101,6 +101,28 @@ struct feat_node {
>          /* get_val is used to get feature COS register value. */
>          void (*get_val)(const struct feat_node *feat, unsigned int cos,
>                          enum cbm_type type, uint32_t *val);
> +
> +        /*
> +         * get_old_val and set_new_val are a pair of functions called in 
> order.
> +         * The caller will traverse all features in the array and call
> +         * 'get_old_val' to get old_cos register value of all supported
> +         * features. Then, call 'set_new_val' to set the new value for the
> +         * designated feature.
> +         *
> +         * All the values are set into value array according to the traversal
> +         * order, meaning the same order of feature array members.
> +         *
> +         * The return value meaning of set_new_val:
> +         * 0 - success.
> +         * negative - error.
> +         */
> +        void (*get_old_val)(uint32_t val[],
> +                            const struct feat_node *feat,
> +                            unsigned int old_cos);
> +        int (*set_new_val)(uint32_t val[],
> +                           const struct feat_node *feat,
> +                           enum cbm_type type,
> +                           uint32_t new_val);

Along the lines of an earlier comment - are "old" and "new" really
meaningful here?

> @@ -212,6 +234,29 @@ static enum psr_feat_type psr_cbm_type_to_feat_type(enum 
> cbm_type type)
>  }
>  
>  /* CAT common functions implementation. */
> +static bool psr_check_cbm(unsigned int cbm_len, uint32_t cbm)
> +{
> +    unsigned int first_bit, zero_bit;
> +
> +    /* Set bits should only in the range of [0, cbm_len]. */
> +    if ( cbm & (~0ul << cbm_len) )

Same question as elsewhere about the use of the ul suffix here:
Can cbm_len really be any value in [0,32]? If not, I don't see
why the calculation needs to be done as unsigned long. Otoh ...

> +        return false;
> +
> +    /* At least one bit need to be set. */
> +    if ( cbm == 0 )
> +        return false;
> +
> +    first_bit = find_first_bit((uint64_t *)&cbm, cbm_len);
> +    zero_bit = find_next_zero_bit((uint64_t *)&cbm, cbm_len, first_bit);

... these bogus casts suggest that the function would best have
an "unsigned long" parameter.

> @@ -285,11 +330,35 @@ static void cat_get_val(const struct feat_node *feat, 
> unsigned int cos,
>      *val = feat->cos_reg_val[cos];
>  }
>  
> +/* val[] len checking is done by caller. */
> +static void cat_get_old_val(uint32_t val[],
> +                            const struct feat_node *feat,
> +                            unsigned int old_cos)
> +{
> +    cat_get_val(feat, old_cos, 0, &val[0]);
> +}
> +
> +/* val[] len checking is done by caller. */
> +static int cat_set_new_val(uint32_t val[],
> +                           const struct feat_node *feat,
> +                           enum cbm_type type,
> +                           uint32_t new_val)
> +{
> +    if ( !psr_check_cbm(feat->info.cat_info.cbm_len, new_val) )
> +        return -EINVAL;
> +
> +    val[0] = new_val;
> +
> +    return 0;
> +}
> +
>  /* L3 CAT ops */
>  static const struct feat_ops l3_cat_ops = {
>      .get_cos_max = cat_get_cos_max,
>      .get_feat_info = cat_get_feat_info,
>      .get_val = cat_get_val,
> +    .get_old_val = cat_get_old_val,
> +    .set_new_val = cat_set_new_val,
>  };
>  
>  static void __init parse_psr_bool(char *s, char *value, char *feature,
> @@ -581,7 +650,21 @@ int psr_get_val(struct domain *d, unsigned int socket,
>  /* Set value functions */
>  static unsigned int get_cos_num(const struct psr_socket_info *info)
>  {
> -    return 0;
> +    const struct feat_node *feat;
> +    unsigned int num = 0, i;
> +
> +    /* Get all features total amount. */
> +    for ( i = 0; i < PSR_SOCKET_MAX_FEAT; i++ )
> +    {
> +        if ( !info->features[i] )
> +            continue;
> +
> +        feat = info->features[i];
> +
> +        num += feat->cos_num;
> +    }
> +
> +    return num;
>  }
>  
>  static int gather_val_array(uint32_t val[],
> @@ -589,7 +672,34 @@ static int gather_val_array(uint32_t val[],
>                              const struct psr_socket_info *info,
>                              unsigned int old_cos)
>  {
> -    return -EINVAL;
> +    const struct feat_node *feat;
> +    unsigned int i;
> +
> +    if ( !val )
> +        return -EINVAL;
> +
> +    /* Get all features current values according to old_cos. */
> +    for ( i = 0; i < PSR_SOCKET_MAX_FEAT; i++ )
> +    {
> +        if ( !info->features[i] )
> +            continue;
> +
> +        feat = info->features[i];
> +
> +        if ( old_cos > feat->ops.get_cos_max(feat) )
> +            old_cos = 0;
> +
> +        /* value getting order is same as feature array */
> +        feat->ops.get_old_val(val, feat, old_cos);
> +
> +        array_len -= feat->cos_num;

So this I should really have asked about on a much earlier patch,
but I've recognize the oddity only now: Why is cos_num
per-feature-node instead of per-feature? This should really be a
field in struct feat_ops (albeit the name "ops" then will be slightly
misleading, but I think that's tolerable if you can't think of a better
name).

> +        if ( array_len < 0 )
> +            return -ENOSPC;

This check needs doing earlier - you need to make sure array_len
>= ops.cos_num prior to calling ops.get_old_val(). (Doing the
check after the subtraction even causes wrapping issues, which
are even more visible in similar code further down.)

> @@ -599,7 +709,43 @@ static int insert_new_val_to_array(uint32_t val[],
>                                     enum cbm_type type,
>                                     uint32_t new_val)
>  {
> -    return -EINVAL;
> +    const struct feat_node *feat;
> +    int ret;
> +    unsigned int i;
> +
> +    ASSERT(feat_type < PSR_SOCKET_MAX_FEAT);
> +
> +    /* Set new value into array according to feature's position in array. */
> +    for ( i = 0; i < feat_type; i++ )
> +    {
> +        if ( !info->features[i] )
> +            continue;
> +
> +        feat = info->features[i];
> +
> +        array_len -= feat->cos_num;
> +        if ( array_len <= 0 )
> +            return -ENOSPC;
> +
> +        val += feat->cos_num;
> +    }
> +
> +    feat = info->features[feat_type];
> +
> +    array_len -= feat->cos_num;
> +    if ( array_len < 0 )
> +        return -ENOSPC;
> +
> +    /*
> +     * Value setting position is same as feature array.
> +     * 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, feat, type, new_val);
> +
> +    return ret;

Again a case of a pointless intermediate variable.

Jan

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