[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v9 10/25] x86: refactor psr: L3 CAT: set value: assemble features value array.
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, 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@xxxxxxxxxxxxx https://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |