Re: [Xen-devel] [PATCH v3 04/15] x86: implement data structure and CPU init flow for MBA

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

On 17-09-19 09:55:28, Roger Pau Monn� wrote:
> On Tue, Sep 05, 2017 at 05:32:26PM +0800, Yi Sun 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.
> > 
> > One thrtl register of MBA stores a throttle value for one or more
> > domains. The throttle value means the transaction time between L2
> > cache and next level memory to be delayed.
> 
> "The throttle value contains the delay between L2 cache and the next
> cache level."
> 
> Seems better, but I'm not a native speaker anyway.
> 
Or:
"The throttle value means the delay between L2 cache and the next cache level."

[...]

> >  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. */
> 
> ...common among all features...
> 
Ok, thanks!

[...]

> > +static int 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 )
> > +        return -ENOENT;
> > +
> > +    if ( type != FEAT_TYPE_MBA )
> > +        return -ENOENT;
> 
> You can join the two checks above in a single if.
> 
Sure.

> > +
> > +    feat->cos_max = min(opt_cos_max, regs->d & CAT_COS_MAX_MASK);
> > +    if ( feat->cos_max < 1 )
> > +        return -ENOENT;
> > +
> > +    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 -ENOENT;
> > +    }
> > +
> > +    /* We reserve cos=0 as default thrtl (0) which means no delay. */
> > +    feat->cos_reg_val[0] = 0;
> 
> AFAICT feat is allocated using xzalloc, so this will already be 0.
> 
Yes, you are right. My original purpose is to explicitly let reader know that
'cos=0' is reserved. But the code is redundant that I will remove it.

> > @@ -1389,6 +1480,7 @@ static void psr_cpu_init(void)
> >      unsigned int socket, cpu = smp_processor_id();
> >      struct feat_node *feat;
> >      struct cpuid_leaf regs;
> > +    uint32_t reg_b;
> 
> Not sure of the benefit between using regs.b or reg_b (it's only 1
> char shorter).
> 
You can see the 'regs' is overwritten in below codes so that the 'regs.b' is not
kept. To add a new local variable 'reg_b' here, we can avoid calling
'cpuid_count_leaf' for L2 CAT and MBA.

> >  
> >      if ( !psr_alloc_feat_enabled() || !boot_cpu_has(X86_FEATURE_PQE) )
> >          goto assoc_init;
> > @@ -1407,7 +1499,8 @@ static void psr_cpu_init(void)
> >      spin_lock_init(&info->ref_lock);
> >  
> >      cpuid_count_leaf(PSR_CPUID_LEVEL_CAT, 0, &regs);
> > -    if ( regs.b & PSR_RESOURCE_TYPE_L3 )
> > +    reg_b = regs.b;
> > +    if ( reg_b & PSR_RESOURCE_TYPE_L3 )
> >      {
> >          cpuid_count_leaf(PSR_CPUID_LEVEL_CAT, 1, &regs);
> >  
> > @@ -1428,8 +1521,7 @@ static void psr_cpu_init(void)
> >          }
> >      }
> >  
> > -    cpuid_count_leaf(PSR_CPUID_LEVEL_CAT, 0, &regs);
> > -    if ( regs.b & PSR_RESOURCE_TYPE_L2 )
> > +    if ( reg_b & PSR_RESOURCE_TYPE_L2 )
> >      {
> >          cpuid_count_leaf(PSR_CPUID_LEVEL_CAT, 2, &regs);
> >  
> > @@ -1441,6 +1533,18 @@ static void psr_cpu_init(void)
> >              feat_l2_cat = feat;
> >      }
> >  
> > +    if ( reg_b & PSR_RESOURCE_TYPE_MBA )
> > +    {
> > +        cpuid_count_leaf(PSR_CPUID_LEVEL_CAT, 3, &regs);
> > +
> > +        feat = feat_mba;
> > +        feat_mba = NULL;
> > +        if ( !mba_init_feature(&regs, feat, info, FEAT_TYPE_MBA) )
> 
> Seems kind of pointless that mba_init_feature returns an error code
> when it's ignored by it's callers. You could switch it to bool if you
> are going to use it like that.
> 
Hmm, bool type seems better. Thanks!

> Thanks, Roger.

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