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

["Roger Pau Monné" <roger.pau@citrix.com>]

On Thu, Aug 24, 2017 at 09:14:38AM +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.
> 
> This patch also implements init flow for MBA and register stub
> callback functions.
> 
> Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com>
> ---
> v2:
>     - modify commit message to replace 'cos register' to 'thrtl register' to
>       make it accurate.
>       (suggested by Chao Peng)
>     - restore the place of the sentence to assign value to 'feat->cbm_len'
>       because the MBA init flow is splitted out as a separate function in v1.
>       (suggested by Chao Peng)
>     - add comment to explain what the MBA thrtl defaul value '0' stands for.
>       (suggested by Chao Peng)
>     - check 'thrtl_max' under linear mode. It could not be euqal or larger than
>       100.
>       (suggested by Chao Peng)
> v1:
>     - rebase codes onto L2 CAT v15.
>     - move comment to appropriate place.
>       (suggested by Chao Peng)
>     - implement 'mba_init_feature' and keep 'cat_init_feature'.
>       (suggested by Chao Peng)
>     - keep 'regs.b' into a local variable to avoid reading CPUID every time.
>       (suggested by Chao Peng)
> ---
>  xen/arch/x86/psr.c              | 141 +++++++++++++++++++++++++++++++++++-----
>  xen/include/asm-x86/msr-index.h |   1 +
>  xen/include/asm-x86/psr.h       |   2 +
>  3 files changed, 126 insertions(+), 18 deletions(-)
> 
> diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c
> index da62f81..f5e99ce 100644
> --- a/xen/arch/x86/psr.c
> +++ b/xen/arch/x86/psr.c
> @@ -27,13 +27,16 @@
>   * - CMT         Cache Monitoring Technology
>   * - COS/CLOS    Class of Service. Also mean COS registers.
>   * - COS_MAX     Max number of COS for the feature (minus 1)
> + * - MBA         Memory Bandwidth Allocation
>   * - MSRs        Machine Specific Registers
>   * - PSR         Intel Platform Shared Resource
> + * - THRTL_MAX   Max throttle value (delay value) of MBA
>   */
>  
>  #define PSR_CMT        (1u << 0)
>  #define PSR_CAT        (1u << 1)
>  #define PSR_CDP        (1u << 2)
> +#define PSR_MBA        (1u << 3)
>  
>  #define CAT_CBM_LEN_MASK 0x1f
>  #define CAT_COS_MAX_MASK 0xffff
> @@ -60,10 +63,14 @@
>   */
>  #define MAX_COS_NUM 2
>  
> +#define MBA_LINEAR         (1u << 2)

Why is this shifted by 2?

> +#define MBA_THRTL_MAX_MASK 0xfff
> +
>  enum psr_feat_type {
>      FEAT_TYPE_L3_CAT,
>      FEAT_TYPE_L3_CDP,
>      FEAT_TYPE_L2_CAT,
> +    FEAT_TYPE_MBA,
>      FEAT_TYPE_NUM,
>      FEAT_TYPE_UNKNOWN,
>  };
> @@ -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.
>   * 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,23 @@ 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 {
> +            /* The length of CBM got through CPUID. */
> +            unsigned int cbm_len;
> +        } cat_info;
> +
> +        struct {
> +            /* The max throttling value got through CPUID. */
> +            unsigned int thrtl_max;
> +            unsigned int linear;

This seems like it wants to be a boolean?

> +        } mba_info;

Just naming the fields 'cat' and 'mba' would probably be enough IMHO,
but that's just taste I think, and I won't argue if you prefer to
leave them with the _info suffix.

> +    };
> +
>      uint32_t cos_reg_val[MAX_COS_REG_CNT];
>  };
>  
> @@ -161,6 +181,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)))
> @@ -273,7 +294,7 @@ static bool psr_check_cbm(unsigned int cbm_len, unsigned long cbm)
>      return true;
>  }
>  
> -/* CAT common functions implementation. */
> +/* Implementation of allocation features' functions. */
>  static int cat_init_feature(const struct cpuid_leaf *regs,
>                              struct feat_node *feat,
>                              struct psr_socket_info *info,
> @@ -289,8 +310,8 @@ static int cat_init_feature(const struct cpuid_leaf *regs,
>      if ( !regs->a || !regs->d )
>          return -ENOENT;
>  
> -    feat->cbm_len = (regs->a & CAT_CBM_LEN_MASK) + 1;
>      feat->cos_max = min(opt_cos_max, regs->d & CAT_COS_MAX_MASK);
> +    feat->cat_info.cbm_len = (regs->a & CAT_CBM_LEN_MASK) + 1;
>  
>      switch ( type )
>      {
> @@ -300,12 +321,12 @@ static int cat_init_feature(const struct cpuid_leaf *regs,
>              return -ENOENT;
>  
>          /* We reserve cos=0 as default cbm (all bits within cbm_len are 1). */
> -        feat->cos_reg_val[0] = cat_default_val(feat->cbm_len);
> +        feat->cos_reg_val[0] = cat_default_val(feat->cat_info.cbm_len);
>  
>          wrmsrl((type == FEAT_TYPE_L3_CAT ?
>                  MSR_IA32_PSR_L3_MASK(0) :
>                  MSR_IA32_PSR_L2_MASK(0)),
> -               cat_default_val(feat->cbm_len));
> +               cat_default_val(feat->cat_info.cbm_len));
>  
>          break;
>  
> @@ -320,11 +341,13 @@ static int cat_init_feature(const struct cpuid_leaf *regs,
>          feat->cos_max = (feat->cos_max - 1) >> 1;
>  
>          /* We reserve cos=0 as default cbm (all bits within cbm_len are 1). */
> -        get_cdp_code(feat, 0) = cat_default_val(feat->cbm_len);
> -        get_cdp_data(feat, 0) = cat_default_val(feat->cbm_len);
> +        get_cdp_code(feat, 0) = cat_default_val(feat->cat_info.cbm_len);
> +        get_cdp_data(feat, 0) = cat_default_val(feat->cat_info.cbm_len);
>  
> -        wrmsrl(MSR_IA32_PSR_L3_MASK(0), cat_default_val(feat->cbm_len));
> -        wrmsrl(MSR_IA32_PSR_L3_MASK(1), cat_default_val(feat->cbm_len));
> +        wrmsrl(MSR_IA32_PSR_L3_MASK(0),
> +               cat_default_val(feat->cat_info.cbm_len));
> +        wrmsrl(MSR_IA32_PSR_L3_MASK(1),
> +               cat_default_val(feat->cat_info.cbm_len));
>          rdmsrl(MSR_IA32_PSR_L3_QOS_CFG, val);
>          wrmsrl(MSR_IA32_PSR_L3_QOS_CFG,
>                 val | (1ull << PSR_L3_QOS_CDP_ENABLE_BIT));
> @@ -344,7 +367,51 @@ static int cat_init_feature(const struct cpuid_leaf *regs,
>  
>      printk(XENLOG_INFO "%s: enabled on socket %u, cos_max:%u, cbm_len:%u\n",
>             cat_feat_name[type], cpu_to_socket(smp_processor_id()),
> -           feat->cos_max, feat->cbm_len);
> +           feat->cos_max, feat->cat_info.cbm_len);
> +
> +    return 0;
> +}
> +
> +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;
> +
> +    feat->cos_max = min(opt_cos_max, regs->d & CAT_COS_MAX_MASK);
> +    if ( feat->cos_max < 1 )
> +        return -ENOENT;
> +
> +    feat->mba_info.thrtl_max = (regs->a & MBA_THRTL_MAX_MASK) + 1;
> +
> +    if ( regs->c & MBA_LINEAR )
> +    {
> +        feat->mba_info.linear = 1;
> +
> +        if ( feat->mba_info.thrtl_max >= 100 )
> +            return -ENOENT;
> +    }
> +
> +    /* We reserve cos=0 as default thrtl (0) which means no delay. */
> +    feat->cos_reg_val[0] = 0;
> +    wrmsrl(MSR_IA32_PSR_MBA_MASK(0), 0);
> +
> +    /* Add this feature into array. */
> +    info->features[type] = feat;
> +
> +    if ( !opt_cpu_info )
> +        return 0;
> +
> +    printk(XENLOG_INFO "MBA: enabled on socket %u, cos_max:%u,"
> +           "thrtl_max:%u, linear:%u.\n",

Please try to avoid splitting messages, it makes it hard to grep for
them afterward.

> +           cpu_to_socket(smp_processor_id()),
> +           feat->cos_max, feat->mba_info.thrtl_max, feat->mba_info.linear);
>  
>      return 0;
>  }
> @@ -356,7 +423,7 @@ static bool cat_get_feat_info(const struct feat_node *feat,
>          return false;
>  
>      data[PSR_INFO_IDX_COS_MAX] = feat->cos_max;
> -    data[PSR_INFO_IDX_CAT_CBM_LEN] = feat->cbm_len;
> +    data[PSR_INFO_IDX_CAT_CBM_LEN] = feat->cat_info.cbm_len;
>      data[PSR_INFO_IDX_CAT_FLAG] = 0;
>  
>      return true;
> @@ -422,6 +489,26 @@ static const struct feat_props l2_cat_props = {
>      .write_msr = l2_cat_write_msr,
>  };
>  
> +/* MBA props */
> +static bool mba_get_feat_info(const struct feat_node *feat,
> +                              uint32_t data[], unsigned int array_len)
> +{
> +    return false;
> +}

Shouldn't this return thrtl_max and whether it's linear?

> +
> +static void mba_write_msr(unsigned int cos, uint32_t val,
> +                          enum psr_val_type type)
> +{

And this perform the MSR write? (MSR_IA32_PSR_MBA_MASK...)

Roger.

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