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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 23.09.17 at 11:48, <yi.y.sun@xxxxxxxxxxxxxxx> 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?

> 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?

> @@ -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.

> +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.

> @@ -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.

Jan


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