Re: [PATCH v12 16/23] x86: L2 CAT: implement CPU init flow.

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

On 17-06-30 00:58:47, Jan Beulich wrote:
> >>> Yi Sun <yi.y.sun@linux.intel.com> 06/14/17 3:26 AM >>>
> > @@ -279,10 +281,14 @@ static void cat_init_feature(const struct cpuid_leaf *regs,
> >      switch ( type )
> >      {
> >      case PSR_SOCKET_L3_CAT:
> > +    case PSR_SOCKET_L2_CAT:
> >          /* cos=0 is reserved as default cbm(all bits within cbm_len are 1). */
> >          feat->cos_reg_val[0] = cat_default_val(feat->cbm_len);
> >  
> > -        wrmsrl(MSR_IA32_PSR_L3_MASK(0), cat_default_val(feat->cbm_len));
> > +        if ( type == PSR_SOCKET_L3_CAT )
> > +            wrmsrl(MSR_IA32_PSR_L3_MASK(0), cat_default_val(feat->cbm_len));
> > +        else
> > +            wrmsrl(MSR_IA32_PSR_L2_MASK(0), cat_default_val(feat->cbm_len));
> 
> Once again I think a conditional expression would yield in easier to read code,
> as that would make even more obvious that the second argument is the same for
> both cases.
> 
Ok, will use conditional expression here to make codes clearer.

> > @@ -317,7 +323,8 @@ static void cat_init_feature(const struct cpuid_leaf *regs,
> >          return;
> >  
> >      printk(XENLOG_INFO "%s: enabled on socket %u, cos_max:%u, cbm_len:%u\n",
> > -           ((type == PSR_SOCKET_L3_CDP) ? "CDP" : "L3 CAT"),
> > +           ((type == PSR_SOCKET_L3_CDP) ? "CDP" :
> > +            ((type == PSR_SOCKET_L3_CAT) ? "L3 CAT": "L2 CAT")),
> 
> At this point it would probably be better to have a static const lookup array
> for the types, or for this descriptive string to be passed into the function.
> 
Got it.

> > @@ -375,6 +382,12 @@ static const struct feat_props l3_cdp_props = {
> >      .write_msr = l3_cdp_write_msr,
> >  };
> >  
> > +/* L2 CAT props */
> > +static const struct feat_props l2_cat_props = {
> > +    .cos_num = 1,
> > +    .type[0] = PSR_CBM_TYPE_L2,
> > +};
> 
> Same remark as for CDP regarding the NULL function pointers left around here
> until the later patches populate them.
> 
Will highlight it in comments.

> > @@ -1407,6 +1424,19 @@ static void psr_cpu_init(void)
> >          info->feat_init = true;
> >      }
> >  
> > +    cpuid_count_leaf(PSR_CPUID_LEVEL_CAT, 0, ®s);
> > +    if ( regs.b & PSR_RESOURCE_TYPE_L2 )
> > +    {
> > +        cpuid_count_leaf(PSR_CPUID_LEVEL_CAT, 2, ®s);
> > +
> > +        feat = feat_l2_cat;
> > +        feat_l2_cat = NULL;
> > +        feat_props[PSR_SOCKET_L2_CAT] = &l2_cat_props;
> > +        cat_init_feature(®s, feat, info, PSR_SOCKET_L2_CAT);
> > +
> > +        info->feat_init = true;
> 
> This recurring setting of feat_init starts looking suspicious here. Why can't
> this be done once at the end of the function, outside of any if()-s?
> 
I am afraid there is no any feature found through CPUID so I set feat_init in
every statement that a feature is found.

> > --- a/xen/include/asm-x86/psr.h
> > +++ b/xen/include/asm-x86/psr.h
> > @@ -23,6 +23,7 @@
> >  
> >  /* Resource Type Enumeration */
> >  #define PSR_RESOURCE_TYPE_L3            0x2
> > +#define PSR_RESOURCE_TYPE_L2            0x4
> 
> These are used in psr.c only afaics, so shouldn't be put in a header.
> 
PSR_RESOURCE_TYPE_L3 is used in sysctl.c too. For L2, I just want to keep it
same place as L3.

> Jan

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