Re: [PATCH v9 12/25] x86: refactor psr: L3 CAT: set value: implement cos id picking flow.

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

On 17-03-28 02:45:13, Jan Beulich wrote:
> >>> On 28.03.17 at 06:58, <yi.y.sun@linux.intel.com> wrote:
> > On 17-03-27 04:37:37, Jan Beulich wrote:
> >> >>> On 16.03.17 at 12:08, <yi.y.sun@linux.intel.com> wrote:
> >> > +static bool cat_fits_cos_max(const uint32_t val[],
> >> > +                             const struct feat_node *feat,
> >> > +                             unsigned int cos)
> >> > +{
> >> > +    if ( cos > feat->info.cat_info.cos_max &&
> >> > +         val[0] != feat->cos_reg_val[0] )
> >> > +            /*
> >> > +             * Exceed cos_max and value to set is not default,
> >> > +             * return error.
> >> > +             */
> >> > +            return false;
> >> > +
> >> > +    return true;
> >> > +}
> >> 
> >> Same here - with cos_max moved out, the hook would seem to
> >> become unnecessary.
> >> 
> > As explanation in previous patch, CDP has different behavior.
> > static bool l3_cdp_fits_cos_max(...)
> > {
> >     if ( cos > feat->info.cat_info.cos_max &&
> >          (val[0] != get_cdp_data(feat, 0) || val[1] != get_cdp_code(feat, 0)) )
> >             /*
> >              * Exceed cos_max and value to set is not default,
> >              * return error.
> >              */
> >             return false;
> > 
> >     return true;
> > 
> > }
> 
> As said in reply, by making get_val() flexible enough you should
> be able to avoid this.

Sorry, I am confused here. 'fits_cos_max' is called during set value process.
Why "making get_val() flexible enough" can avoid this?

> The caller knows how many values to compare.
> 
My purpose to implement such hook is to avoid caller knows feature details.
Then, we can create a general flow to cover all features. So, I do not
understand your mearing here. Sorry.

> >> >  static int pick_avail_cos(const struct psr_socket_info *info,
> >> >                            const uint32_t val[], uint32_t array_len,
> >> >                            unsigned int old_cos,
> >> >                            enum psr_feat_type feat_type)
> >> >  {
> >> > +    unsigned int cos;
> >> > +    unsigned int cos_max = 0;
> >> > +    const struct feat_node *feat;
> >> > +    const unsigned int *ref = info->cos_ref;
> >> > +
> >> >      ASSERT(spin_is_locked((spinlock_t *)(&info->ref_lock)));
> >> > -    return -ENOENT;
> >> > +
> >> > +    /* cos_max is the one of the feature which is being set. */
> >> > +    feat = info->features[feat_type];
> >> > +    if ( !feat )
> >> > +        return -ENOENT;
> >> > +
> >> > +    cos_max = feat->ops.get_cos_max(feat);
> >> > +    if ( !cos_max )
> >> > +        return -ENOENT;
> >> > +
> >> > +    /*
> >> > +     * If old cos is referred only by the domain, then use it. And, we cannot
> >> 
> >> "the domain" here is lacking context - there's no domain involved
> > 
> > How about "the domain input through 'psr_set_val'"?
> 
> If you assume this going to remain a helper function for just this
> one purpose, then I could live with that. Note however that if
> ever a 2nd caller would appear, such a comment likely would
> become stale. Therefore it is generally better to write comments
> based on what the specific function does or assumes, without
> regard to its caller(s) assumptions/restrictions.
> 
Ok, I should explain this in caller and I think there already are
comments to explain this. So, I think I may remove this comment here.

> Jan

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