| Message ID | 1506160104-5890-5-git-send-email-yi.y.sun@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, Sep 23, 2017 at 09:48:13AM +0000, 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 delay between L2 cache and next > cache level. > > This patch also implements init flow for MBA and register stub > callback functions. > > Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> With two nits below. > @@ -332,20 +355,58 @@ static int cat_init_feature(const struct cpuid_leaf *regs, > } > > default: > - return -ENOENT; > + return false; > } > > /* Add this feature into array. */ > info->features[type] = feat; > > if ( !opt_cpu_info ) > - return 0; > + return true; > > 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); I would rather do: if ( opt_cpu_info ) printk(...); return true; So that the function has a single return path for the success case. > > - return 0; > + return true; > +} > + > +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", ^ newline. Thanks, Roger.
>>> On 26.09.17 at 10:38, <roger.pau@citrix.com> wrote: > On Sat, Sep 23, 2017 at 09:48:13AM +0000, Yi Sun wrote: >> @@ -332,20 +355,58 @@ static int cat_init_feature(const struct cpuid_leaf *regs, >> } >> >> default: >> - return -ENOENT; >> + return false; >> } >> >> /* Add this feature into array. */ >> info->features[type] = feat; >> >> if ( !opt_cpu_info ) >> - return 0; >> + return true; >> >> 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); > > I would rather do: > > if ( opt_cpu_info ) > printk(...); > > return true; > > So that the function has a single return path for the success case. But not in this patch, which only partly changes what's already there. Yet fundamentally I agree. >> - return 0; >> + return true; >> +} >> + >> +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", > ^ newline. And no full stop please. Jan
>>> On 23.09.17 at 11:48, <yi.y.sun@linux.intel.com> 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
On 17-09-28 05:00:09, Jan Beulich wrote: > >>> On 23.09.17 at 11:48, <yi.y.sun@linux.intel.com> 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? > Sure. > > 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? > There is a "programmable rate controller" between them to indirectly control the bandwidth. > > @@ -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. > Sure. > > +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. > Ok, thanks! > > @@ -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. > IMHO, it can avoid the 'cpuid_count_leaf()' being repeatedly called. Without it, we have to call 'cpuid_count_leaf()' for 2 more times. I can move it to another patch to make it clear if you like it. > Jan > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > https://lists.xen.org/xen-devel
>>> Yi Sun <yi.y.sun@linux.intel.com> 09/29/17 3:55 AM >>> >On 17-09-28 05:00:09, Jan Beulich wrote: >> >>> On 23.09.17 at 11:48, <yi.y.sun@linux.intel.com> wrote: >> > 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? >> >There is a "programmable rate controller" between them to indirectly control >the bandwidth. I'm afraid I don't feel like this answers my question. Anyway - the sentence needs some re-phrasing so it becomes clear what you're talking about. >> > @@ -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. >> >IMHO, it can avoid the 'cpuid_count_leaf()' being repeatedly called. Without it, >we have to call 'cpuid_count_leaf()' for 2 more times. Hmm, didn't you simply replace regs.b uses with ebx? Or did I overlook a place where regs is being overwritten before the last of these regs.b uses (in which case I think your change is fine)? Jan
On 17-10-03 23:52:09, Jan Beulich wrote: > >>> Yi Sun <yi.y.sun@linux.intel.com> 09/29/17 3:55 AM >>> > >On 17-09-28 05:00:09, Jan Beulich wrote: > >> >>> On 23.09.17 at 11:48, <yi.y.sun@linux.intel.com> wrote: > >> > 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? > >> > >There is a "programmable rate controller" between them to indirectly control > >the bandwidth. > > I'm afraid I don't feel like this answers my question. Anyway - the sentence > needs some re-phrasing so it becomes clear what you're talking about. > I may draw the figure shown in SDM to make things clear. > >> > @@ -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. > >> > >IMHO, it can avoid the 'cpuid_count_leaf()' being repeatedly called. Without it, > >we have to call 'cpuid_count_leaf()' for 2 more times. > > Hmm, didn't you simply replace regs.b uses with ebx? Or did I overlook a place > where regs is being overwritten before the last of these regs.b uses (in which case > I think your change is fine)? > The regs is overwritten when a feature presents. The old codes are below cpuid_count_leaf(PSR_CPUID_LEVEL_CAT, 0, ®s); if ( regs.b & PSR_RESOURCE_TYPE_L3 ) { cpuid_count_leaf(PSR_CPUID_LEVEL_CAT, 1, ®s); //It is overwritten here. ...... } cpuid_count_leaf(PSR_CPUID_LEVEL_CAT, 0, ®s); //So, we have to call cpuid to get regs again. if ( regs.b & PSR_RESOURCE_TYPE_L2 ) { cpuid_count_leaf(PSR_CPUID_LEVEL_CAT, 2, ®s); ...... Because above reason, I defined this ebx local variable to avoid calling cpuid again for next feature. > Jan
>>> On 05.10.17 at 06:42, <yi.y.sun@linux.intel.com> wrote: > On 17-10-03 23:52:09, Jan Beulich wrote: >> >>> Yi Sun <yi.y.sun@linux.intel.com> 09/29/17 3:55 AM >>> >> >On 17-09-28 05:00:09, Jan Beulich wrote: >> >> >>> On 23.09.17 at 11:48, <yi.y.sun@linux.intel.com> wrote: >> >> > @@ -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. >> >> >> >IMHO, it can avoid the 'cpuid_count_leaf()' being repeatedly called. Without it, >> >we have to call 'cpuid_count_leaf()' for 2 more times. >> >> Hmm, didn't you simply replace regs.b uses with ebx? Or did I overlook a place >> where regs is being overwritten before the last of these regs.b uses (in which case >> I think your change is fine)? >> > The regs is overwritten when a feature presents. The old codes are below > > cpuid_count_leaf(PSR_CPUID_LEVEL_CAT, 0, ®s); > if ( regs.b & PSR_RESOURCE_TYPE_L3 ) > { > cpuid_count_leaf(PSR_CPUID_LEVEL_CAT, 1, ®s); //It is overwritten here. > ...... > } > > cpuid_count_leaf(PSR_CPUID_LEVEL_CAT, 0, ®s); //So, we have to call cpuid to get regs again. > if ( regs.b & PSR_RESOURCE_TYPE_L2 ) > { > cpuid_count_leaf(PSR_CPUID_LEVEL_CAT, 2, ®s); > ...... > > Because above reason, I defined this ebx local variable to avoid calling cpuid > again for next feature. I see. But then please give the variable a better name, reflecting the data it holds. Jan
On 17-10-05 02:49:59, Jan Beulich wrote: > >>> On 05.10.17 at 06:42, <yi.y.sun@linux.intel.com> wrote: > > On 17-10-03 23:52:09, Jan Beulich wrote: > >> >>> Yi Sun <yi.y.sun@linux.intel.com> 09/29/17 3:55 AM >>> > >> >On 17-09-28 05:00:09, Jan Beulich wrote: > >> >> >>> On 23.09.17 at 11:48, <yi.y.sun@linux.intel.com> wrote: > >> >> > @@ -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. > >> >> > >> >IMHO, it can avoid the 'cpuid_count_leaf()' being repeatedly called. Without it, > >> >we have to call 'cpuid_count_leaf()' for 2 more times. > >> > >> Hmm, didn't you simply replace regs.b uses with ebx? Or did I overlook a place > >> where regs is being overwritten before the last of these regs.b uses (in which case > >> I think your change is fine)? > >> > > The regs is overwritten when a feature presents. The old codes are below > > > > cpuid_count_leaf(PSR_CPUID_LEVEL_CAT, 0, ®s); > > if ( regs.b & PSR_RESOURCE_TYPE_L3 ) > > { > > cpuid_count_leaf(PSR_CPUID_LEVEL_CAT, 1, ®s); //It is overwritten here. > > ...... > > } > > > > cpuid_count_leaf(PSR_CPUID_LEVEL_CAT, 0, ®s); //So, we have to call cpuid to get regs again. > > if ( regs.b & PSR_RESOURCE_TYPE_L2 ) > > { > > cpuid_count_leaf(PSR_CPUID_LEVEL_CAT, 2, ®s); > > ...... > > > > Because above reason, I defined this ebx local variable to avoid calling cpuid > > again for next feature. > > I see. But then please give the variable a better name, reflecting > the data it holds. > Then, how about 'feat_mask'? > Jan
diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c index aa5a60f..e2f8fd0 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 among 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; + + 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,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, struct feat_node *feat, struct psr_socket_info *info, enum psr_feat_type type) @@ -286,25 +307,25 @@ static int cat_init_feature(const struct cpuid_leaf *regs, /* No valid value so do not enable feature. */ if ( !regs->a || !regs->d ) - return -ENOENT; + return false; - 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 ) { case FEAT_TYPE_L3_CAT: case FEAT_TYPE_L2_CAT: if ( feat->cos_max < 1 ) - return -ENOENT; + return false; /* 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; @@ -313,17 +334,19 @@ static int cat_init_feature(const struct cpuid_leaf *regs, uint64_t val; if ( feat->cos_max < 3 ) - return -ENOENT; + return false; /* Cut half of cos_max when CDP is enabled. */ 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)); @@ -332,20 +355,58 @@ static int cat_init_feature(const struct cpuid_leaf *regs, } default: - return -ENOENT; + return false; } /* Add this feature into array. */ info->features[type] = feat; if ( !opt_cpu_info ) - return 0; + return true; 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; + return true; +} + +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", + cpu_to_socket(smp_processor_id()), + feat->cos_max, feat->mba.thrtl_max, feat->mba.linear); + + return true; } static bool cat_get_feat_info(const struct feat_node *feat, @@ -355,7 +416,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 +482,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 bool __init parse_psr_bool(const char *s, const char *delim, const char *ss, const char *feature, unsigned int mask) @@ -479,7 +560,8 @@ static int __init parse_psr_param(const char *s) } else if ( !parse_psr_bool(s, val_delim, ss, "cmt", PSR_CMT) && !parse_psr_bool(s, val_delim, ss, "cat", PSR_CAT) && - !parse_psr_bool(s, val_delim, ss, "cdp", PSR_CDP) ) + !parse_psr_bool(s, val_delim, ss, "cdp", PSR_CDP) && + !parse_psr_bool(s, val_delim, ss, "mba", PSR_MBA) ) rc = -EINVAL; s = ss + 1; @@ -883,7 +965,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; /* @@ -1401,6 +1483,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; } @@ -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; if ( !psr_alloc_feat_enabled() || !boot_cpu_has(X86_FEATURE_PQE) ) goto assoc_init; @@ -1428,7 +1515,8 @@ static void psr_cpu_init(void) spin_lock_init(&info->ref_lock); cpuid_count_leaf(PSR_CPUID_LEVEL_CAT, 0, ®s); - if ( regs.b & PSR_RESOURCE_TYPE_L3 ) + ebx = regs.b; + if ( ebx & PSR_RESOURCE_TYPE_L3 ) { cpuid_count_leaf(PSR_CPUID_LEVEL_CAT, 1, ®s); @@ -1436,32 +1524,43 @@ static void psr_cpu_init(void) feat_l3 = NULL; if ( (regs.c & PSR_CAT_CDP_CAPABILITY) && (opt_psr & PSR_CDP) && - !cat_init_feature(®s, feat, info, FEAT_TYPE_L3_CDP) ) + cat_init_feature(®s, feat, info, FEAT_TYPE_L3_CDP) ) feat_props[FEAT_TYPE_L3_CDP] = &l3_cdp_props; /* If CDP init fails, try to work as L3 CAT. */ if ( !feat_props[FEAT_TYPE_L3_CDP] ) { - if ( !cat_init_feature(®s, feat, info, FEAT_TYPE_L3_CAT) ) + if ( cat_init_feature(®s, feat, info, FEAT_TYPE_L3_CAT) ) feat_props[FEAT_TYPE_L3_CAT] = &l3_cat_props; else feat_l3 = feat; } } - cpuid_count_leaf(PSR_CPUID_LEVEL_CAT, 0, ®s); - if ( regs.b & PSR_RESOURCE_TYPE_L2 ) + if ( ebx & PSR_RESOURCE_TYPE_L2 ) { cpuid_count_leaf(PSR_CPUID_LEVEL_CAT, 2, ®s); feat = feat_l2_cat; feat_l2_cat = NULL; - if ( !cat_init_feature(®s, feat, info, FEAT_TYPE_L2_CAT) ) + if ( cat_init_feature(®s, feat, info, FEAT_TYPE_L2_CAT) ) feat_props[FEAT_TYPE_L2_CAT] = &l2_cat_props; else feat_l2_cat = feat; } + if ( ebx & PSR_RESOURCE_TYPE_MBA ) + { + cpuid_count_leaf(PSR_CPUID_LEVEL_CAT, 3, ®s); + + feat = feat_mba; + feat_mba = NULL; + if ( mba_init_feature(®s, feat, info, FEAT_TYPE_MBA) ) + feat_props[FEAT_TYPE_MBA] = &mba_props; + else + feat_mba = feat; + } + info->feat_init = true; assoc_init: @@ -1521,7 +1620,7 @@ static int __init psr_presmp_init(void) if ( (opt_psr & PSR_CMT) && opt_rmid_max ) init_psr_cmt(opt_rmid_max); - if ( opt_psr & (PSR_CAT | PSR_CDP) ) + if ( opt_psr & (PSR_CAT | PSR_CDP | PSR_MBA) ) init_psr(); if ( psr_cpu_prepare() ) diff --git a/xen/include/asm-x86/msr-index.h b/xen/include/asm-x86/msr-index.h index 4e08de6..41f1677 100644 --- a/xen/include/asm-x86/msr-index.h +++ b/xen/include/asm-x86/msr-index.h @@ -348,6 +348,7 @@ #define MSR_IA32_PSR_L3_MASK_CODE(n) (0x00000c90 + (n) * 2 + 1) #define MSR_IA32_PSR_L3_MASK_DATA(n) (0x00000c90 + (n) * 2) #define MSR_IA32_PSR_L2_MASK(n) (0x00000d10 + (n)) +#define MSR_IA32_PSR_MBA_MASK(n) (0x00000d50 + (n)) /* Intel Model 6 */ #define MSR_P6_PERFCTR(n) (0x000000c1 + (n)) diff --git a/xen/include/asm-x86/psr.h b/xen/include/asm-x86/psr.h index cb3f067..9d14264 100644 --- a/xen/include/asm-x86/psr.h +++ b/xen/include/asm-x86/psr.h @@ -24,6 +24,7 @@ /* Resource Type Enumeration */ #define PSR_RESOURCE_TYPE_L3 0x2 #define PSR_RESOURCE_TYPE_L2 0x4 +#define PSR_RESOURCE_TYPE_MBA 0x8 /* L3 Monitoring Features */ #define PSR_CMT_L3_OCCUPANCY 0x1 @@ -58,6 +59,7 @@ enum psr_type { PSR_TYPE_L3_CODE, PSR_TYPE_L3_DATA, PSR_TYPE_L2_CBM, + PSR_TYPE_MBA_THRTL, PSR_TYPE_UNKNOWN, };
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 delay between L2 cache and next cache level. This patch also implements init flow for MBA and register stub callback functions. Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com> --- CC: Jan Beulich <jbeulich@suse.com> CC: Andrew Cooper <andrew.cooper3@citrix.com> CC: Wei Liu <wei.liu2@citrix.com> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Chao Peng <chao.p.peng@linux.intel.com> v4: - modify commit message. (suggested by Roger Pau Monné) - fix a comment. (suggested by Roger Pau Monné) - join two checks in a single if. (suggested by Roger Pau Monné) - remove redundant initialization of 'feat->cos_reg_val[0]'. (suggested by Roger Pau Monné) - change 'reg_b' to 'ebx'. (suggested by Jan Beulich) - change type of 'mba_init_feature' from 'int' to 'bool'. (suggested by Roger Pau Monné) - change type of 'cat_init_feature' from 'int' to 'bool'. 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 | 157 ++++++++++++++++++++++++++++++++-------- xen/include/asm-x86/msr-index.h | 1 + xen/include/asm-x86/psr.h | 2 + 3 files changed, 131 insertions(+), 29 deletions(-)