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

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

On 17-03-28 02:34:51, Jan Beulich wrote:
> >>> On 28.03.17 at 05:12, <yi.y.sun@linux.intel.com> wrote:
> > On 17-03-27 04:17:28, Jan Beulich wrote:
> >> >>> On 16.03.17 at 12:08, <yi.y.sun@linux.intel.com> 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'm fine with "current", but the name collision - would "current" be
> omitted still bothers me. The fact that cat_get_old_val() calls
> cat_get_val(), however, strongly suggests that the hook here is
> redundant anyway. Even in the CDP case I think you can get
> away without it, but if this turns out really impossible or clumsy,
> then the hook could be introduced there (with a better name)
> and be an optional one (with the caller using ->get_val() if the
> one here is NULL).
> 
I am afraid we have to keep this hook. CDP uses this hook to get both CODE and
DATA at same time. But CDP uses get_val() hook to get either CODE or DATA.
So, they have different functionalitiy. I prefer to rename it to
'get_current_val'.

I can make it optional hook. But the codes in caller look a little strange. E.g.
static int gather_val_array()
{
...
    if ( feat->ops.get_old_val )
        feat->ops.get_old_val(val, feat, old_cos);
    else
        feat->ops.get_val(feat, old_cos, 0, &val[0]);
...
}

So, I think a wrapper like cat_get_old_val() may be a better choice. What is
your opinion?

> > I think 'new' is meaningful to express we are setting the newly input value.
> 
> Well, this is the meaning to its caller. The function itself doesn't
> care whether the value is a new one, or just some other value
> coming from an unnamed source.
> 
Ok, will remove 'new'.

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