Re: [PATCH v4 03/24] x86: refactor psr: implement main data structures.

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

On 17-01-03 02:12:53, Jan Beulich wrote:
> >>> On 03.01.17 at 09:49, <yi.y.sun@linux.intel.com> wrote:
> > On 17-01-03 01:00:37, Jan Beulich wrote:
> >> >>> On 26.12.16 at 07:56, <yi.y.sun@linux.intel.com> wrote:
> >> > On 16-12-22 09:13:43, Jan Beulich wrote:
> >> >> >>> On 14.12.16 at 05:07, <yi.y.sun@linux.intel.com> wrote:
> >> >> > +struct feat_node;
> >> >> > +
> >> >> > +/*
> >> >> > + * This structure defines feature operation callback functions. Every feature
> >> >> > + * enabled MUST implement such callback functions and register them to ops.
> >> >> > + *
> >> >> > + * Feature specific behaviors will be encapsulated into these callback
> >> >> > + * functions. Then, the main flows will not be changed when introducing a new
> >> >> > + * feature.
> >> >> > + */
> >> >> > +struct feat_ops {
> >> >> > +    /*
> >> >> > +     * init_feature is used in cpu initialization process to do feature
> >> >> > +     * specific initialization works.
> >> >> > +     */
> >> >> > +    void (*init_feature)(unsigned int eax, unsigned int ebx,
> >> >> > +                         unsigned int ecx, unsigned int edx,
> >> >> > +                         struct feat_node *feat,
> >> >> > +                         struct psr_socket_info *info);
> >> >> > +};
> >> >> 
> >> >> What is the reason to have a separate structure for this, when you
> >> >> don't store a pointer in struct feat_node? If this was inlined there,
> >> >> the odd forward declaration of struct feat_node wouldn't be needed
> >> >> either. (The same question may apply to struct feat_hw_info.) 
> >> >> 
> >> > I just want to make codes be clear. If you prefer inline declaration, I 
> >> > think I
> >> > should change it as below, right?
> >> > 
> >> > struct feat_node {
> >> > ......
> >> >     struct feat_ops {
> >> >         ......
> >> >     } ops;
> >> >     struct feat_hw_info {
> >> >         ......
> >> >     } info;
> >> > ......
> >> > };
> >> 
> >> Well, not exactly: The struct <tag> { ... } <name>; wrappers
> >> are unnecessary then too. With them kept there indeed would be
> >> no big difference between both variants.
> >> 
> > To facilitate the callback functions assignment for a feature, I defined
> > feature specific callback function ops like below.
> > 
> > struct feat_ops l3_cat_ops = {
> >     .init_feature = l3_cat_init_feature,
> >     .get_max_cos_max = l3_cat_get_max_cos_max,
> >     ......
> > };
> > 
> > And directly assign it to global feature node in initialization process like
> > below.
> > 
> > static void cpu_init_work(void)
> > {
> > ......
> >             feat_tmp = feat_l3_cat;
> >             feat_l3_cat = NULL;
> >             feat_tmp->ops = l3_cat_ops;
> > ......
> > }
> > 
> > I think this can make codes be clear. How do you think? Is this way 
> > acceptable?
> 
> Yes.
> 
Thanks a lot! Do you have any comment to other patches?

> Jan

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