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

[Yi Sun <yi.y.sun@xxxxxxxxxxxxxxx>]

On 17-03-27 04:17:28, Jan Beulich wrote:
> >>> 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?
> 
Maybe 'old' is not accurate. How about 'current'? In fact, we use this
function to get domain's current CBM value. Furthermore, this is to distinguish
'get_val' which is declared above.

I think 'new' is meaningful to express we are setting the newly input value.

How do you think? Thanks!

> > @@ -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 ...
> 
cbm_len is got as below:
#define CAT_CBM_LEN_MASK 0x1f
cat.cbm_len = (regs.a & CAT_CBM_LEN_MASK) + 1;

So its max value is 32.

> > +        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.
> 
I would like to modify 'cbm' type to 'uint64_t'. Use a local variable in caller
to do the type conversion. 

> > @@ -285,11 +330,35 @@ static void cat_get_val(const struct feat_node *feat, 
> > unsigned int cos,
> >      *val = feat->cos_reg_val[cos];
> >  }
> >  

[...]
> >  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).
> 
Ok, I got your meaning. How about 'feat_props'? No matter operations or
variables are all properties of the feature.

> > +        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.)
> 
Thanks for the suggestion! Will move 'array_len >= cos_num' check prior to
call 'get_old_val'.

> > @@ -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.
> 
Will remove it.

> Jan

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