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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 28.03.17 at 12:12, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
> On 17-03-28 02:34:51, Jan Beulich wrote:
>> >>> On 28.03.17 at 05:12, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
>> > On 17-03-27 04:17:28, Jan Beulich wrote:
>> >> >>> On 16.03.17 at 12:08, <yi.y.sun@xxxxxxxxxxxxxxx> 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 it looks like I have to repeat myself here: The caller knows
the number of slots. Make the get_val() hook flexible enough to
allow it to replace get_old_val() here (the caller would then
invoke it cos_num times).

Jan


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