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

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

On 17-03-27 04:37:37, Jan Beulich wrote:
> >>> On 16.03.17 at 12:08, <yi.y.sun@xxxxxxxxxxxxxxx> 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;

}

> >  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'"?

> in the function - "... the domain currently using it, then re-use it"?
> 
Ok, 're-use it' is better.

> > +     * use id 0 because it stores the default values.
> > +     */
> > +    if ( old_cos && ref[old_cos] == 1 &&
> > +         fits_cos_max(val, array_len, info, old_cos) )
> > +            return old_cos;
> > +
> > +    /* Find an unused one other than cos0. */
> > +    for ( cos = 1; cos <= cos_max; cos++ )
> > +    {
> > +        /*
> > +         * ref is 0 means this COS is not used by other domain and
> > +         * can be used for current setting.
> > +         */
> > +        if ( !ref[cos] )
> > +        {
> > +            if ( !fits_cos_max(val, array_len, info, cos) )
> > +                return -EOVERFLOW;
> 
> Perhaps better use "break;" here.
> 
Oh yes, thanks!

> > +            return cos;
> > +        }
> > +    }
> > +
> > +    return -EOVERFLOW;
> >  }
> 
> Jan

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