[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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.

> 
> This patch also implements init flow for MBA and register stub
> callback functions.
> 
> Signed-off-by: Yi Sun <yi.y.sun@xxxxxxxxxxxxxxx>
> ---
> v3:
>     - replace 'psr_val_type' to 'psr_type'. Also, change 'PSR_VAL_TYPE_MBA' to
>       'PSR_TYPE_MBA_THRTL'.
>       (suggested by Roger Pau Monné)
>     - replace 'MBA_LINEAR' to 'MBA_LINEAR_MASK' to make the name more clear.
>       (suggested by Roger Pau Monné)
>     - replase 'cat_info'/'mba_info' to 'cat'/'mba' to make the names shorter.
>       (suggested by Roger Pau Monné)
>     - change type of 'linear' to 'bool'.
>       (suggested by Roger Pau Monné)
>     - make format string of printf in one line.
>       (suggested by Roger Pau Monné)
> 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              | 140 
> ++++++++++++++++++++++++++++++++++------
>  xen/include/asm-x86/msr-index.h |   1 +
>  xen/include/asm-x86/psr.h       |   2 +
>  3 files changed, 125 insertions(+), 18 deletions(-)
> 
> diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c
> index 4166a1c..10776d2 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_MASK    (1u << 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. */

...common among all features...

>      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;
> +
> +        struct {
> +            /* The max throttling value got through CPUID. */
> +            unsigned int thrtl_max;
> +            bool linear;
> +        } mba;
> +    };
> +
>      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)))
> @@ -272,7 +293,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,
> @@ -288,8 +309,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.cbm_len = (regs->a & CAT_CBM_LEN_MASK) + 1;
>  
>      switch ( type )
>      {
> @@ -299,12 +320,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.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.cbm_len));
>  
>          break;
>  
> @@ -319,11 +340,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.cbm_len);
> +        get_cdp_data(feat, 0) = cat_default_val(feat->cat.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.cbm_len));
> +        wrmsrl(MSR_IA32_PSR_L3_MASK(1),
> +               cat_default_val(feat->cat.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));
> @@ -343,7 +366,50 @@ 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.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;

You can join the two checks above in a single if.

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

> +    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",
> +           cpu_to_socket(smp_processor_id()),
> +           feat->cos_max, feat->mba.thrtl_max, feat->mba.linear);
>  
>      return 0;
>  }
> @@ -355,7 +421,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.cbm_len;
>      data[PSR_INFO_IDX_CAT_FLAG] = 0;
>  
>      return true;
> @@ -421,6 +487,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;
> +}
> +
> +static void mba_write_msr(unsigned int cos, uint32_t val,
> +                          enum psr_type type)
> +{
> +}
> +
> +static const struct feat_props mba_props = {
> +    .cos_num = 1,
> +    .type[0] = PSR_TYPE_MBA_THRTL,
> +    .alt_type = PSR_TYPE_UNKNOWN,
> +    .get_feat_info = mba_get_feat_info,
> +    .write_msr = mba_write_msr,
> +};
> +
>  static void __init parse_psr_bool(char *s, char *value, char *feature,
>                                    unsigned int mask)
>  {
> @@ -456,6 +542,7 @@ static void __init parse_psr_param(char *s)
>          parse_psr_bool(s, val_str, "cmt", PSR_CMT);
>          parse_psr_bool(s, val_str, "cat", PSR_CAT);
>          parse_psr_bool(s, val_str, "cdp", PSR_CDP);
> +        parse_psr_bool(s, val_str, "mba", PSR_MBA);
>  
>          if ( val_str && !strcmp(s, "rmid_max") )
>              opt_rmid_max = simple_strtoul(val_str, NULL, 0);
> @@ -862,7 +949,7 @@ static int insert_val_into_array(uint32_t val[],
>      if ( array_len < props->cos_num )
>          return -ENOSPC;
>  
> -    if ( !psr_check_cbm(feat->cbm_len, new_val) )
> +    if ( !psr_check_cbm(feat->cat.cbm_len, new_val) )
>          return -EINVAL;
>  
>      /*
> @@ -1380,6 +1467,10 @@ static int psr_cpu_prepare(void)
>           (feat_l2_cat = xzalloc(struct feat_node)) == NULL )
>          return -ENOMEM;
>  
> +    if ( feat_mba == NULL &&
> +         (feat_mba = xzalloc(struct feat_node)) == NULL )
> +        return -ENOMEM;
> +
>      return 0;
>  }
>  
> @@ -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).

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

Thanks, Roger.

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.