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

[Chao Peng <chao.p.peng@xxxxxxxxxxxxxxx>]

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

>   * 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];
>  };
>  
> @@ -161,6 +179,7 @@ static DEFINE_PER_CPU(struct psr_assoc,
> psr_assoc);
>   */
>  static struct feat_node *feat_l3;
>  static struct feat_node *feat_l2_cat;
> +static struct feat_node *feat_mba;
>  
>  /* Common functions */
>  #define cat_default_val(len) (0xffffffff >> (32 - (len)))
> @@ -274,22 +293,22 @@ static bool psr_check_cbm(unsigned int cbm_len,
> unsigned long cbm)
>  }
>  
>  /* 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().


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

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