Re: [PATCH v4 04/15] x86: implement data structure and CPU init flow for MBA

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

On 17-09-28 05:00:09, Jan Beulich wrote:
> >>> On 23.09.17 at 11:48, <yi.y.sun@linux.intel.com> wrote:
> > This patch implements main data structures of MBA.
> > 
> > Like CAT features, MBA HW info has cos_max which means the max thrtl
> > register number, and thrtl_max which means the max throttle value
> > (delay value). It also has a flag to represent if the throttle
> > value is linear or not.
> 
> Could you replace "or not" with what the alternative actually is,
> as "non-linear" can mean all sorts of things?
> 
Sure.

> > One thrtl register of MBA stores a throttle value for one or more
> > domains. The throttle value means the delay between L2 cache and next
> > cache level.
> 
> What is a delay between two cache levels?
> 
There is a "programmable rate controller" between them to indirectly control
the bandwidth.

> > @@ -272,8 +293,8 @@ static bool psr_check_cbm(unsigned int cbm_len, unsigned long cbm)
> >      return true;
> >  }
> >  
> > -/* CAT common functions implementation. */
> > -static int cat_init_feature(const struct cpuid_leaf *regs,
> > +/* Implementation of allocation features' functions. */
> > +static bool cat_init_feature(const struct cpuid_leaf *regs,
> 
> Such a type change should happen in a separate patch, as this
> isn't specific to MBA. That way you can also make clear why you
> want this to change - the current description doesn't mention
> this at all.
> 
Sure.

> > +static bool mba_init_feature(const struct cpuid_leaf *regs,
> > +                            struct feat_node *feat,
> > +                            struct psr_socket_info *info,
> > +                            enum psr_feat_type type)
> > +{
> > +    /* No valid value so do not enable feature. */
> > +    if ( !regs->a || !regs->d || type != FEAT_TYPE_MBA )
> > +        return false;
> > +
> > +    feat->cos_max = min(opt_cos_max, regs->d & CAT_COS_MAX_MASK);
> > +    if ( feat->cos_max < 1 )
> > +        return false;
> > +
> > +    feat->mba.thrtl_max = (regs->a & MBA_THRTL_MAX_MASK) + 1;
> > +
> > +    if ( regs->c & MBA_LINEAR_MASK )
> > +    {
> > +        feat->mba.linear = true;
> > +
> > +        if ( feat->mba.thrtl_max >= 100 )
> > +            return false;
> > +    }
> > +
> > +    wrmsrl(MSR_IA32_PSR_MBA_MASK(0), 0);
> > +
> > +    /* Add this feature into array. */
> > +    info->features[type] = feat;
> > +
> > +    if ( !opt_cpu_info )
> > +        return true;
> > +
> > +    printk(XENLOG_INFO "MBA: enabled on socket %u, cos_max:%u, thrtl_max:%u, linear:%u.\n",
> 
> The last one wants to be %d.
> 
Ok, thanks!

> > @@ -1410,6 +1496,7 @@ static void psr_cpu_init(void)
> >      unsigned int socket, cpu = smp_processor_id();
> >      struct feat_node *feat;
> >      struct cpuid_leaf regs;
> > +    uint32_t ebx;
> 
> Is this local variable really a big help? To me it looks like it only
> makes the patch larger without actually improving anything,
> and without being related to the subject of the patch.
> 
IMHO, it can avoid the 'cpuid_count_leaf()' being repeatedly called. Without it,
we have to call 'cpuid_count_leaf()' for 2 more times.

I can move it to another patch to make it clear if you like it.

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

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