Re: [Xen-devel] [RFC v2 04/12] x86: implement data structure and CPU init flow for MBA.

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

On 17-08-09 09:09:10, Chao Peng wrote:
> 
> > @@ -71,7 +78,6 @@ enum psr_feat_type {
> >  /*
> >   * This structure represents one feature.
> >   * cos_max     - The max COS registers number got through CPUID.
> > - * cbm_len     - The length of CBM got through CPUID.
> 
> As you are moving instead of removing the code, the comment can also
> move together with the code (but not get deleted). But if the remove is
> on your purpose (which sounds acceptable to me) then it's another thing.
> 
Ok, will move the comment.

> >   * cos_reg_val - Array to store the values of COS registers. One
> > entry stores
> >   *               the value of one COS register.
> >   *               For L3 CAT and L2 CAT, one entry corresponds to one
> > COS_ID.
> > @@ -80,9 +86,21 @@ enum psr_feat_type {
> >   *               cos_reg_val[1] (Code).
> >   */
> >  struct feat_node {
> > -    /* cos_max and cbm_len are common values for all features so far.
> > */
> > +    /* cos_max is common values for all features so far. */
> >      unsigned int cos_max;
> > -    unsigned int cbm_len;
> > +
> > +    /* Feature specific HW info. */
> > +    union {
> > +        struct {
> > +            unsigned int cbm_len;
> > +        } cat_info;
> > +
> > +        struct {
> > +            unsigned int thrtl_max;
> > +            unsigned int linear;
> > +        } mba_info;
> > +    };
> > +
> >      uint32_t cos_reg_val[MAX_COS_REG_CNT];
> >  };
> >  
> >  /* CAT common functions implementation. */
> > -static int cat_init_feature(const struct cpuid_leaf *regs,
> > -                            struct feat_node *feat,
> > -                            struct psr_socket_info *info,
> > -                            enum psr_feat_type type)
> > +static int init_alloc_features(const struct cpuid_leaf *regs,
> 
> You still initialize the feature one by one, right? In that case
> 'features' should keep as 'feature'. Also I'm not sure which degree we
> can share the code between CAT and MBA. If not much but just bring many
> switch-cases and ifs then I tend to introduce a totally new
> mba_init_feature().
> 
Ok, a new 'mba_init_feature()' seems good.

> 
> > @@ -1439,12 +1508,25 @@ static void psr_cpu_init(void)
> >  
> >          feat = feat_l2_cat;
> >          feat_l2_cat = NULL;
> > -        if ( !cat_init_feature(&regs, feat, info, FEAT_TYPE_L2_CAT) )
> > +        if ( !init_alloc_features(&regs, feat, info,
> > FEAT_TYPE_L2_CAT) )
> >              feat_props[FEAT_TYPE_L2_CAT] = &l2_cat_props;
> >          else
> >              feat_l2_cat = feat;
> >      }
> >  
> > +    cpuid_count_leaf(PSR_CPUID_LEVEL_CAT, 0, &regs);
> 
> Can we cache this sub leaf 0? Currently we call this for every
> allocation feature which in my mind is unnecessary.
> 
Good suggestion to optimize codes. Will do it.

> > +    if ( regs.b & PSR_RESOURCE_TYPE_MBA )
> > +    {
> > +        cpuid_count_leaf(PSR_CPUID_LEVEL_CAT, 3, &regs);
> > +
> > +        feat = feat_mba;
> > +        feat_mba = NULL;
> > +        if ( !init_alloc_features(&regs, feat, info, FEAT_TYPE_MBA) )
> > +            feat_props[FEAT_TYPE_MBA] = &mba_props;
> > +        else
> > +            feat_mba = feat;
> > +    }
> > +
> >      info->feat_init = true;
> 
> Chao

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