Re: [Xen-devel] [PATCH v4 09/24] x86: refactor psr: set value: assemble features value array.

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

On 17-01-10 07:34:00, Jan Beulich wrote:
> >>> On 14.12.16 at 05:07, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
> > --- a/xen/arch/x86/psr.c
> > +++ b/xen/arch/x86/psr.c
> > @@ -121,6 +121,35 @@ struct feat_ops {
> >      /* get_val is used to get feature COS register value. */
> >      bool (*get_val)(const struct feat_node *feat, unsigned int cos,
> >                     enum cbm_type type, uint64_t *val);
> > +    /*
> > +     * get_cos_num is used to get the COS registers amount used by the
> > +     * feature for one setting, e.g. CDP uses 2 COSs but CAT uses 1.
> > +     */
> > +    unsigned int (*get_cos_num)(const struct feat_node *feat);
> > +    /*
> > +     * get_old_val and set_new_val are a pair of functions called together.
> > +     * 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.
> > +     *
> > +     * All the values are set into value array according the traversal 
> > order,
> > +     * meaning the same order of feature list members.
> > +     *
> > +     * The return value is the amount of entries to skip in the value array
> > +     * or error.
> > +     * 1 - one entry in value array.
> > +     * 2 - two entries in value array, e.g. CDP uses two entries.
> 
> Doesn't this match the get_cos_num() return value? Would be nice to
> be spelled out (and, where possible, ASSERT()ed).
> 
Yes, thanks!

> > @@ -186,6 +215,29 @@ static void free_feature(struct psr_socket_info *info)
> >      }
> >  }
> >  
> > +static bool_t psr_check_cbm(unsigned int cbm_len, uint64_t cbm)
> 
> bool (and then true/false in the function body)
> 
Ok, thanks!

> > +static int l3_cat_set_new_val(uint64_t val[],
> > +                              const struct feat_node *feat,
> > +                              unsigned int old_cos,
> > +                              enum cbm_type type,
> > +                              uint64_t m)
> > +{
> > +    if ( type != PSR_CBM_TYPE_L3 )
> > +        /* L3 CAT uses one COS. Skip it. */
> > +        return 1;
> 
> If this is the wrong type, how can you return 1 (or any positive
> value) here? This basically suggests that, as recommended for an
> earlier operation already, that you want the type check done in
> the caller. Or wait - isn't type not matching an error here, as you
> iterate the same list twice in the same order? Which then gets us
> back to me mentioning that the set-new-value should really be
> done in common code, not in the callbacks; it would really only be
> the check (psr_check_cbm() in the L3 case above) which would
> require a callback.
> 
Your understanding is right. The value array will be iterated twice. First,
assemble all features old value into it. Second, set the target value into
array according to the target feature's position in the array. Because
different features may have different behaviors, e.g. CDP uses two entries
of the array, I think it would be better to make 'set_new_val' be a callback
function.

> Jan

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