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

[Yi Sun <yi.y.sun@linux.intel.com>]

On 17-03-08 09:54:04, Jan Beulich wrote:
> >>> On 15.02.17 at 09:49, <yi.y.sun@linux.intel.com> wrote:
[...]
> > +    /*
> > +     * 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.
> 
The expression is not accurate. I will correct this. Thanks!

> > @@ -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.
> 
cbm_len is not 64. cbm_len means the CBM value length, how many bits. For L3
CAT, it may be 11 bits. For L2 CAT, it may be 8 bits.

> > +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.
> 
Sorry, I may not understand your intention. For different features, they have
different cos_max. Do you mean I should abstract a callback function for all
features to handle this cos_max check? Thanks!

> > -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".
> 
This is corrected in next version. I will change name back to assemble if you
like it.

> >  {
> > -    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.)
> 
Ok, no problem.

> 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.
>
Good idea. Thanks!

[...] 

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