| Message ID | 1493801063-38513-14-git-send-email-yi.y.sun@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 03.05.17 at 10:44, <yi.y.sun@linux.intel.com> wrote: > @@ -150,11 +151,28 @@ static DEFINE_PER_CPU(struct psr_assoc, psr_assoc); > * array creation. It is used to transiently store a spare node. > */ > static struct feat_node *feat_l3_cat; > +static struct feat_node *feat_l3_cdp; > > /* Common functions */ > #define cat_default_val(len) (0xffffffff >> (32 - (len))) > > /* > + * get_cdp_data - get DATA COS register value from input COS ID. > + * @feat: the feature node. > + * @cos: the COS ID. > + */ > +#define get_cdp_data(feat, cos) \ > + ( (feat)->cos_reg_val[(cos) * 2] ) Stray blanks inside parentheses. > +/* > + * get_cdp_code - get CODE COS register value from input COS ID. > + * @feat: the feature node. > + * @cos: the COS ID. > + */ > +#define get_cdp_code(feat, cos) \ > + ( (feat)->cos_reg_val[(cos) * 2 + 1] ) Same here. In fact, while I'm not going to ask to remove the parentheses, this is the one case where an expression a macro expands to does not strictly need an outer pair of parentheses, as suffix expressions have highest precedence anyway. > @@ -249,6 +267,25 @@ static void cat_init_feature(const struct cpuid_leaf *regs, > > break; > > + case PSR_SOCKET_L3_CDP: > + { > + unsigned long val; As MSR values are specifically 64-bit ones, I think uint64_t would be more appropriate here. Depending on intended later additions to this function it may also be worthwhile making this a switch-wide variable. > + /* Cut half of cos_max when CDP is enabled. */ > + feat->cos_max >>= 1; > + > + /* We only write mask1 since mask0 is always all ones by default. */ Is this, btw, just reset state or even guaranteed after offlining and re-onlining a CPU? > + wrmsrl(MSR_IA32_PSR_L3_MASK(1), cat_default_val(feat->cbm_len)); > + rdmsrl(MSR_IA32_PSR_L3_QOS_CFG, val); > + wrmsrl(MSR_IA32_PSR_L3_QOS_CFG, val | (1 << PSR_L3_QOS_CDP_ENABLE_BIT)); 1u at the very least, perhaps even 1ull. Jan
On 17-05-31 03:37:48, Jan Beulich wrote: > >>> On 03.05.17 at 10:44, <yi.y.sun@linux.intel.com> wrote: > > @@ -150,11 +151,28 @@ static DEFINE_PER_CPU(struct psr_assoc, psr_assoc); > > @@ -249,6 +267,25 @@ static void cat_init_feature(const struct cpuid_leaf *regs, > > > > break; > > > > + case PSR_SOCKET_L3_CDP: > > + { > > + unsigned long val; > > As MSR values are specifically 64-bit ones, I think uint64_t would > be more appropriate here. Depending on intended later additions to > this function it may also be worthwhile making this a switch-wide > variable. > This variable is only used in this case. Will change it to uint64_t. > > + /* Cut half of cos_max when CDP is enabled. */ > > + feat->cos_max >>= 1; > > + > > + /* We only write mask1 since mask0 is always all ones by default. */ > > Is this, btw, just reset state or even guaranteed after offlining > and re-onlining a CPU? > Below MSRs are all per socket. So, we just need reset them when socket is online. > > + wrmsrl(MSR_IA32_PSR_L3_MASK(1), cat_default_val(feat->cbm_len)); > > + rdmsrl(MSR_IA32_PSR_L3_QOS_CFG, val); > > + wrmsrl(MSR_IA32_PSR_L3_QOS_CFG, val | (1 << PSR_L3_QOS_CDP_ENABLE_BIT)); > > 1u at the very least, perhaps even 1ull. > Thanks! > Jan
>>> On 02.06.17 at 09:26, <yi.y.sun@linux.intel.com> wrote: > On 17-05-31 03:37:48, Jan Beulich wrote: >> >>> On 03.05.17 at 10:44, <yi.y.sun@linux.intel.com> wrote: >> > + /* Cut half of cos_max when CDP is enabled. */ >> > + feat->cos_max >>= 1; >> > + >> > + /* We only write mask1 since mask0 is always all ones by default. */ >> >> Is this, btw, just reset state or even guaranteed after offlining >> and re-onlining a CPU? >> > Below MSRs are all per socket. So, we just need reset them when socket is > online. Which I hope you've understood then means the comment and presumably also the code here need further refinement. Jan
On 17-06-06 01:45:11, Jan Beulich wrote: > >>> On 02.06.17 at 09:26, <yi.y.sun@linux.intel.com> wrote: > > On 17-05-31 03:37:48, Jan Beulich wrote: > >> >>> On 03.05.17 at 10:44, <yi.y.sun@linux.intel.com> wrote: > >> > + /* Cut half of cos_max when CDP is enabled. */ > >> > + feat->cos_max >>= 1; > >> > + > >> > + /* We only write mask1 since mask0 is always all ones by default. */ > >> > >> Is this, btw, just reset state or even guaranteed after offlining > >> and re-onlining a CPU? > >> > > Below MSRs are all per socket. So, we just need reset them when socket is > > online. > > Which I hope you've understood then means the comment and > presumably also the code here need further refinement. > Spec states it below. So, the mask0 is guranteed. "the default mask in IA32_L3_MASK_0 - which is all “1”s (on reset)" > Jan
>>> On 06.06.17 at 10:13, <yi.y.sun@linux.intel.com> wrote: > On 17-06-06 01:45:11, Jan Beulich wrote: >> >>> On 02.06.17 at 09:26, <yi.y.sun@linux.intel.com> wrote: >> > On 17-05-31 03:37:48, Jan Beulich wrote: >> >> >>> On 03.05.17 at 10:44, <yi.y.sun@linux.intel.com> wrote: >> >> > + /* Cut half of cos_max when CDP is enabled. */ >> >> > + feat->cos_max >>= 1; >> >> > + >> >> > + /* We only write mask1 since mask0 is always all ones by default. */ >> >> >> >> Is this, btw, just reset state or even guaranteed after offlining >> >> and re-onlining a CPU? >> >> >> > Below MSRs are all per socket. So, we just need reset them when socket is >> > online. >> >> Which I hope you've understood then means the comment and >> presumably also the code here need further refinement. >> > Spec states it below. So, the mask0 is guranteed. > "the default mask in IA32_L3_MASK_0 - which is all “1”s (on reset)" Sigh. I did ask very clearly (and this is still visible above) about the case where the CPU did _not_ undergo a reset cycle. Jan
On 17-06-06 02:38:27, Jan Beulich wrote: > >>> On 06.06.17 at 10:13, <yi.y.sun@linux.intel.com> wrote: > > On 17-06-06 01:45:11, Jan Beulich wrote: > >> >>> On 02.06.17 at 09:26, <yi.y.sun@linux.intel.com> wrote: > >> > On 17-05-31 03:37:48, Jan Beulich wrote: > >> >> >>> On 03.05.17 at 10:44, <yi.y.sun@linux.intel.com> wrote: > >> >> > + /* Cut half of cos_max when CDP is enabled. */ > >> >> > + feat->cos_max >>= 1; > >> >> > + > >> >> > + /* We only write mask1 since mask0 is always all ones by default. */ > >> >> > >> >> Is this, btw, just reset state or even guaranteed after offlining > >> >> and re-onlining a CPU? > >> >> > >> > Below MSRs are all per socket. So, we just need reset them when socket is > >> > online. > >> > >> Which I hope you've understood then means the comment and > >> presumably also the code here need further refinement. > >> > > Spec states it below. So, the mask0 is guranteed. > > "the default mask in IA32_L3_MASK_0 - which is all “1”s (on reset)" > > Sigh. I did ask very clearly (and this is still visible above) about > the case where the CPU did _not_ undergo a reset cycle. > Sorry, I think I must mis-understand your question. SW never modifies value of COS ID 0 mask register. So, the value in mask0 will not be changed after socket first online (mask0 is set to default by HW this time). So, we think it is not necessary to restore mask0 value to default here.
On 17-06-07 09:31:02, Yi Sun wrote: > On 17-06-06 02:38:27, Jan Beulich wrote: > > >>> On 06.06.17 at 10:13, <yi.y.sun@linux.intel.com> wrote: > > > On 17-06-06 01:45:11, Jan Beulich wrote: > > >> >>> On 02.06.17 at 09:26, <yi.y.sun@linux.intel.com> wrote: > > >> > On 17-05-31 03:37:48, Jan Beulich wrote: > > >> >> >>> On 03.05.17 at 10:44, <yi.y.sun@linux.intel.com> wrote: > > >> >> > + /* Cut half of cos_max when CDP is enabled. */ > > >> >> > + feat->cos_max >>= 1; > > >> >> > + > > >> >> > + /* We only write mask1 since mask0 is always all ones by default. */ > > >> >> > > >> >> Is this, btw, just reset state or even guaranteed after offlining > > >> >> and re-onlining a CPU? > > >> >> > > >> > Below MSRs are all per socket. So, we just need reset them when socket is > > >> > online. > > >> > > >> Which I hope you've understood then means the comment and > > >> presumably also the code here need further refinement. > > >> > > > Spec states it below. So, the mask0 is guranteed. > > > "the default mask in IA32_L3_MASK_0 - which is all “1”s (on reset)" > > > > Sigh. I did ask very clearly (and this is still visible above) about > > the case where the CPU did _not_ undergo a reset cycle. > > > Sorry, I think I must mis-understand your question. SW never modifies value > of COS ID 0 mask register. So, the value in mask0 will not be changed after > socket first online (mask0 is set to default by HW this time). So, we think > it is not necessary to restore mask0 value to default here. > But considering something else (e.g. FW) may change this MSR value during socket offline-online cycle, we may need to restore mask0 here too. Is that your intention? Thanks! > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > https://lists.xen.org/xen-devel
>>> On 07.06.17 at 03:31, <yi.y.sun@linux.intel.com> wrote: > On 17-06-06 02:38:27, Jan Beulich wrote: >> >>> On 06.06.17 at 10:13, <yi.y.sun@linux.intel.com> wrote: >> > On 17-06-06 01:45:11, Jan Beulich wrote: >> >> >>> On 02.06.17 at 09:26, <yi.y.sun@linux.intel.com> wrote: >> >> > On 17-05-31 03:37:48, Jan Beulich wrote: >> >> >> >>> On 03.05.17 at 10:44, <yi.y.sun@linux.intel.com> wrote: >> >> >> > + /* Cut half of cos_max when CDP is enabled. */ >> >> >> > + feat->cos_max >>= 1; >> >> >> > + >> >> >> > + /* We only write mask1 since mask0 is always all ones by default. */ >> >> >> >> >> >> Is this, btw, just reset state or even guaranteed after offlining >> >> >> and re-onlining a CPU? >> >> >> >> >> > Below MSRs are all per socket. So, we just need reset them when socket is >> >> > online. >> >> >> >> Which I hope you've understood then means the comment and >> >> presumably also the code here need further refinement. >> >> >> > Spec states it below. So, the mask0 is guranteed. >> > "the default mask in IA32_L3_MASK_0 - which is all “1”s (on reset)" >> >> Sigh. I did ask very clearly (and this is still visible above) about >> the case where the CPU did _not_ undergo a reset cycle. >> > Sorry, I think I must mis-understand your question. SW never modifies value > of COS ID 0 mask register. So, the value in mask0 will not be changed after > socket first online (mask0 is set to default by HW this time). So, we think > it is not necessary to restore mask0 value to default here. Is the register read-only? If so, writing it would indeed be unnecessary (and likely wrong, as it might raise #GP then instead of being ignored). If not, firmware may have written it, so you'll want to write it even during boot time CPU bringup. Jan
On 17-06-07 02:14:26, Jan Beulich wrote: > >>> On 07.06.17 at 03:31, <yi.y.sun@linux.intel.com> wrote: > > On 17-06-06 02:38:27, Jan Beulich wrote: > >> >>> On 06.06.17 at 10:13, <yi.y.sun@linux.intel.com> wrote: > >> > On 17-06-06 01:45:11, Jan Beulich wrote: > >> >> >>> On 02.06.17 at 09:26, <yi.y.sun@linux.intel.com> wrote: > >> >> > On 17-05-31 03:37:48, Jan Beulich wrote: > >> >> >> >>> On 03.05.17 at 10:44, <yi.y.sun@linux.intel.com> wrote: > >> >> >> > + /* Cut half of cos_max when CDP is enabled. */ > >> >> >> > + feat->cos_max >>= 1; > >> >> >> > + > >> >> >> > + /* We only write mask1 since mask0 is always all ones by default. */ > >> >> >> > >> >> >> Is this, btw, just reset state or even guaranteed after offlining > >> >> >> and re-onlining a CPU? > >> >> >> > >> >> > Below MSRs are all per socket. So, we just need reset them when socket is > >> >> > online. > >> >> > >> >> Which I hope you've understood then means the comment and > >> >> presumably also the code here need further refinement. > >> >> > >> > Spec states it below. So, the mask0 is guranteed. > >> > "the default mask in IA32_L3_MASK_0 - which is all “1”s (on reset)" > >> > >> Sigh. I did ask very clearly (and this is still visible above) about > >> the case where the CPU did _not_ undergo a reset cycle. > >> > > Sorry, I think I must mis-understand your question. SW never modifies value > > of COS ID 0 mask register. So, the value in mask0 will not be changed after > > socket first online (mask0 is set to default by HW this time). So, we think > > it is not necessary to restore mask0 value to default here. > > Is the register read-only? If so, writing it would indeed be > unnecessary (and likely wrong, as it might raise #GP then > instead of being ignored). If not, firmware may have written it, > so you'll want to write it even during boot time CPU bringup. > No, it is writable. I will restore mask0 to default value here. Furthermore, for L3/L2 CAT mask0, we may need to restore mask0 too. > Jan
diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c index 9b8428d..7000d95 100644 --- a/xen/arch/x86/psr.c +++ b/xen/arch/x86/psr.c @@ -62,6 +62,7 @@ enum psr_feat_type { PSR_SOCKET_L3_CAT, + PSR_SOCKET_L3_CDP, PSR_SOCKET_FEAT_NUM, PSR_SOCKET_FEAT_UNKNOWN, }; @@ -150,11 +151,28 @@ static DEFINE_PER_CPU(struct psr_assoc, psr_assoc); * array creation. It is used to transiently store a spare node. */ static struct feat_node *feat_l3_cat; +static struct feat_node *feat_l3_cdp; /* Common functions */ #define cat_default_val(len) (0xffffffff >> (32 - (len))) /* + * get_cdp_data - get DATA COS register value from input COS ID. + * @feat: the feature node. + * @cos: the COS ID. + */ +#define get_cdp_data(feat, cos) \ + ( (feat)->cos_reg_val[(cos) * 2] ) + +/* + * get_cdp_code - get CODE COS register value from input COS ID. + * @feat: the feature node. + * @cos: the COS ID. + */ +#define get_cdp_code(feat, cos) \ + ( (feat)->cos_reg_val[(cos) * 2 + 1] ) + +/* * Use this function to check if any allocation feature has been enabled * in cmdline. */ @@ -249,6 +267,25 @@ static void cat_init_feature(const struct cpuid_leaf *regs, break; + case PSR_SOCKET_L3_CDP: + { + unsigned long val; + + /* Cut half of cos_max when CDP is enabled. */ + feat->cos_max >>= 1; + + /* We only write mask1 since mask0 is always all ones by default. */ + wrmsrl(MSR_IA32_PSR_L3_MASK(1), cat_default_val(feat->cbm_len)); + rdmsrl(MSR_IA32_PSR_L3_QOS_CFG, val); + wrmsrl(MSR_IA32_PSR_L3_QOS_CFG, val | (1 << PSR_L3_QOS_CDP_ENABLE_BIT)); + + /* cos=0 is reserved 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); + + break; + } + default: return; } @@ -259,8 +296,9 @@ static void cat_init_feature(const struct cpuid_leaf *regs, if ( !opt_cpu_info ) return; - printk(XENLOG_INFO "%s CAT: enabled on socket %u, cos_max:%u, cbm_len:%u\n", - ((type == PSR_SOCKET_L3_CAT) ? "L3" : "L2"), + printk(XENLOG_INFO "%s: enabled on socket %u, cos_max:%u, cbm_len:%u\n", + ((type == PSR_SOCKET_L3_CDP) ? "CDP" : + ((type == PSR_SOCKET_L3_CAT) ? "L3 CAT": "L2 CAT")), cpu_to_socket(smp_processor_id()), feat->cos_max, feat->cbm_len); } @@ -289,6 +327,11 @@ static struct feat_props l3_cat_props = { .write_msr = l3_cat_write_msr, }; +/* L3 CDP props */ +static struct feat_props l3_cdp_props = { + .cos_num = 2, +}; + static void __init parse_psr_bool(char *s, char *value, char *feature, unsigned int mask) { @@ -1206,6 +1249,10 @@ static int psr_cpu_prepare(void) (feat_l3_cat = xzalloc(struct feat_node)) == NULL ) return -ENOMEM; + if ( feat_l3_cdp == NULL && + (feat_l3_cdp = xzalloc(struct feat_node)) == NULL ) + return -ENOMEM; + return 0; } @@ -1238,12 +1285,24 @@ static void psr_cpu_init(void) { cpuid_count_leaf(PSR_CPUID_LEVEL_CAT, 1, ®s); - feat = feat_l3_cat; - feat_l3_cat = NULL; - l3_cat_props.type[0] = PSR_CBM_TYPE_L3; - feat_props[PSR_SOCKET_L3_CAT] = &l3_cat_props; - - cat_init_feature(®s, feat, info, PSR_SOCKET_L3_CAT); + if ( (regs.c & PSR_CAT_CDP_CAPABILITY) && (opt_psr & PSR_CDP) && + !info->features[PSR_SOCKET_L3_CDP] ) + { + feat = feat_l3_cdp; + feat_l3_cdp = NULL; + l3_cdp_props.type[0] = PSR_CBM_TYPE_L3_DATA; + l3_cdp_props.type[1] = PSR_CBM_TYPE_L3_CODE; + feat_props[PSR_SOCKET_L3_CDP] = &l3_cdp_props; + cat_init_feature(®s, feat, info, PSR_SOCKET_L3_CDP); + } + else + { + feat = feat_l3_cat; + feat_l3_cat = NULL; + l3_cat_props.type[0] = PSR_CBM_TYPE_L3; + feat_props[PSR_SOCKET_L3_CAT] = &l3_cat_props; + cat_init_feature(®s, feat, info, PSR_SOCKET_L3_CAT); + } info->feat_init = true; } @@ -1305,7 +1364,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 ) + if ( opt_psr & (PSR_CAT | PSR_CDP) ) init_psr(); if ( psr_cpu_prepare() )
This patch implements the CPU init flow for CDP. The flow is almost same as L3 CAT. Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com> --- v11: - changes about 'feat_props'. (suggested by Jan Beulich) - remove MSR restore action which is unnecessary. (suggested by Jan Beulich) - modify commit message. v10: - fix comment. (suggested by Jan Beulich) - use swith in 'cat_init_feature' to handle different feature types. (suggested by Jan Beulich) - changes about 'props'. (suggested by Jan Beulich) - restore MSRs to default value when cpu online. (suggested by Jan Beulich) - remove feat_mask. (suggested by Jan Beulich) v9: - modify commit message to describe flow clearer. - handle cpu offline and online again case to read MSRs registers values back and save them into cos array to make user can get real data. - modify error handling process in 'psr_cpu_prepare' to reduce redundant codes. - modify 'get_cdp_data' and 'get_cdp_code' to make them standard. (suggested by Roger Pau and Jan Beulich) - encapsulate CDP operations into 'cat_init_feature' to reduce redundant codes. (suggested by Roger Pau) - reuse 'cat_get_cos_max' for CDP. (suggested by Roger Pau) - handle 'PSR_CDP' in psr_presmp_init to make init work can be done when there is only 'psr=cdp' in cmdline. - remove unnecessary comment. (suggested by Jan Beulich) - move CDP related codes in 'cpu_init_work' into 'psr_cpu_init'. (suggested by Jan Beulich) - add codes to handle CDP's 'cos_num'. (suggested by Jan Beulich) - fix coding style issue. (suggested by Jan Beulich) - do not free resources when allocation fails in 'psr_cpu_prepare'. (suggested by Jan Beulich) - changes about 'uint64_t' to 'uint32_t'. (suggested by Jan Beulich) v7: - initialize 'l3_cdp'. (suggested by Konrad Rzeszutek Wilk) v6: - use 'cpuid_leaf'. (suggested by Konrad Rzeszutek Wilk and Jan Beulich) v5: - remove codes to free 'feat_l3_cdp' in 'free_feature'. (suggested by Jan Beulich) - encapsulate cpuid registers into 'struct cpuid_leaf_regs'. (suggested by Jan Beulich) - print socket info when 'opt_cpu_info' is true. (suggested by Jan Beulich) - rename 'l3_cdp_get_max_cos_max' to 'l3_cdp_get_cos_max'. (suggested by Jan Beulich) - rename 'dat[]' to 'data[]'. (suggested by Jan Beulich) - move 'cpu_prepare_work' contents into 'psr_cpu_prepare'. (suggested by Jan Beulich) v4: - create this patch to make codes easier to understand. (suggested by Jan Beulich) --- xen/arch/x86/psr.c | 77 +++++++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 68 insertions(+), 9 deletions(-)