| Message ID | 1487148579-7243-5-git-send-email-yi.y.sun@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Feb 15, 2017 at 04:49:19PM +0800, Yi Sun wrote: > This patch implements the CPU init and free flow including L3 CAT > initialization and feature list free. > > Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com> Either you need to use a separate patch to move cpuid_count_leaf or you should state it is moved in the commit message. > + > +/* Common functions. */ > +static void free_feature(struct psr_socket_info *info) > +{ > + struct feat_node *feat, *next; > + > + if ( !info ) > + return; > + > + /* > + * Free resources of features. But we do not free global feature list > + * entry, like feat_l3_cat. Although it may cause a few memory leak, > + * it is OK simplify things. I don't think it is OK to leak memory in the hypervisor in general. Please specify why it is OK in this particular case in the comment. > + */ > + list_for_each_entry_safe(feat, next, &info->feat_list, list) > + { > + if ( !feat ) > + return; > + > + __clear_bit(feat->feature, &info->feat_mask); > + list_del(&feat->list); > + xfree(feat); > + } > +} > + > -static int psr_cpu_prepare(unsigned int cpu) > +static void cpu_init_work(void) > +{ > + struct psr_socket_info *info; > + unsigned int socket; > + unsigned int cpu = smp_processor_id(); > + struct feat_node *feat; > + struct cpuid_leaf regs = { .a = 0, .b = 0, .c = 0, .d = 0 }; > + > + if ( !cpu_has(¤t_cpu_data, X86_FEATURE_PQE) ) > + return; > + else if ( current_cpu_data.cpuid_level < PSR_CPUID_LEVEL_CAT ) > + { > + __clear_bit(X86_FEATURE_PQE, current_cpu_data.x86_capability); > + return; > + } > + > + socket = cpu_to_socket(cpu); > + info = socket_info + socket; > + if ( info->feat_mask ) > + return; > + > + INIT_LIST_HEAD(&info->feat_list); > + spin_lock_init(&info->ref_lock); > + > + cpuid_count_leaf(PSR_CPUID_LEVEL_CAT, 0, ®s); > + if ( regs.b & PSR_RESOURCE_TYPE_L3 ) > + { You can move struct feat_node *feat here. > + cpuid_count_leaf(PSR_CPUID_LEVEL_CAT, 1, ®s); > + > + feat = feat_l3_cat; > + /* psr_cpu_prepare will allocate it on subsequent CPU onlining. */ > + feat_l3_cat = NULL; > + feat->ops = l3_cat_ops; > + > + l3_cat_init_feature(regs, feat, info); > + } > +} > + [...] > > @@ -359,7 +528,7 @@ static int cpu_callback( > switch ( action ) > { > case CPU_UP_PREPARE: > - rc = psr_cpu_prepare(cpu); > + rc = psr_cpu_prepare(); > break; > case CPU_STARTING: > psr_cpu_init(); > @@ -388,10 +557,14 @@ static int __init psr_presmp_init(void) > if ( (opt_psr & PSR_CMT) && opt_rmid_max ) > init_psr_cmt(opt_rmid_max); > > - psr_cpu_prepare(0); > + if ( opt_psr & PSR_CAT ) > + init_psr(); > + > + if ( psr_cpu_prepare() ) > + psr_free(); > > psr_cpu_init(); > - if ( psr_cmt_enabled() ) > + if ( psr_cmt_enabled() || socket_info ) Why not have psr_cat_enabled() here? Wei.
On 17-02-26 17:41:08, Wei Liu wrote: > On Wed, Feb 15, 2017 at 04:49:19PM +0800, Yi Sun wrote: > > This patch implements the CPU init and free flow including L3 CAT > > initialization and feature list free. > > > > Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com> > > Either you need to use a separate patch to move cpuid_count_leaf or you > should state it is moved in the commit message. > Thanks! I will add description in commit message. > > + > > +/* Common functions. */ > > +static void free_feature(struct psr_socket_info *info) > > +{ > > + struct feat_node *feat, *next; > > + > > + if ( !info ) > > + return; > > + > > + /* > > + * Free resources of features. But we do not free global feature list > > + * entry, like feat_l3_cat. Although it may cause a few memory leak, > > + * it is OK simplify things. > > I don't think it is OK to leak memory in the hypervisor in general. > Please specify why it is OK in this particular case in the comment. > In most cases, such global feature list entry will be added into feature list so that it can be freed here. In extreme case, e.g. socket 1 does not support L3 CAT. The feat_l3_cat allocated in psr_cpu_prepare will not be released. But this is rare case. Jan, Konrad and me disucssed this before. Per Jan's suggestion, we do not free it. > > + */ > > + list_for_each_entry_safe(feat, next, &info->feat_list, list) > > + { > > + if ( !feat ) > > + return; > > + > > + __clear_bit(feat->feature, &info->feat_mask); > > + list_del(&feat->list); > > + xfree(feat); > > + } > > +} > > + > > -static int psr_cpu_prepare(unsigned int cpu) > > +static void cpu_init_work(void) > > +{ > > + struct psr_socket_info *info; > > + unsigned int socket; > > + unsigned int cpu = smp_processor_id(); > > + struct feat_node *feat; > > + struct cpuid_leaf regs = { .a = 0, .b = 0, .c = 0, .d = 0 }; > > + > > + if ( !cpu_has(¤t_cpu_data, X86_FEATURE_PQE) ) > > + return; > > + else if ( current_cpu_data.cpuid_level < PSR_CPUID_LEVEL_CAT ) > > + { > > + __clear_bit(X86_FEATURE_PQE, current_cpu_data.x86_capability); > > + return; > > + } > > + > > + socket = cpu_to_socket(cpu); > > + info = socket_info + socket; > > + if ( info->feat_mask ) > > + return; > > + > > + INIT_LIST_HEAD(&info->feat_list); > > + spin_lock_init(&info->ref_lock); > > + > > + cpuid_count_leaf(PSR_CPUID_LEVEL_CAT, 0, ®s); > > + if ( regs.b & PSR_RESOURCE_TYPE_L3 ) > > + { > > You can move > > struct feat_node *feat > > here. > This variable will also be used by L2 CAT which codes exist in a different branch. So, I declare it at the top of the function. Please refer below patch: [PATCH v8 17/24] x86: L2 CAT: implement CPU init and free flow. > > + cpuid_count_leaf(PSR_CPUID_LEVEL_CAT, 1, ®s); > > + > > + feat = feat_l3_cat; > > + /* psr_cpu_prepare will allocate it on subsequent CPU onlining. */ > > + feat_l3_cat = NULL; > > + feat->ops = l3_cat_ops; > > + > > + l3_cat_init_feature(regs, feat, info); > > + } > > +} > > + > [...] > > > > @@ -359,7 +528,7 @@ static int cpu_callback( > > switch ( action ) > > { > > case CPU_UP_PREPARE: > > - rc = psr_cpu_prepare(cpu); > > + rc = psr_cpu_prepare(); > > break; > > case CPU_STARTING: > > psr_cpu_init(); > > @@ -388,10 +557,14 @@ static int __init psr_presmp_init(void) > > if ( (opt_psr & PSR_CMT) && opt_rmid_max ) > > init_psr_cmt(opt_rmid_max); > > > > - psr_cpu_prepare(0); > > + if ( opt_psr & PSR_CAT ) > > + init_psr(); > > + > > + if ( psr_cpu_prepare() ) > > + psr_free(); > > > > psr_cpu_init(); > > - if ( psr_cmt_enabled() ) > > + if ( psr_cmt_enabled() || socket_info ) > > Why not have psr_cat_enabled() here? > psr_cmt_enabled() returns true of false by checking if the global pointer 'psr_cmt' has been allocated or not. The 'psr_cmt' is also used in sysctl.c. For allocation features, we have a similar global pointer 'socket_info'. But it is only used in psr.c and all allocation features(CAT/CDP/MBA) use it. So we directly use it in psr.c to check if related initialization has been done. > Wei.
>>> Wei Liu <wei.liu2@citrix.com> 02/26/17 6:41 PM >>> >On Wed, Feb 15, 2017 at 04:49:19PM +0800, Yi Sun wrote: >> +static void free_feature(struct psr_socket_info *info) >> +{ >> + struct feat_node *feat, *next; >> + >> + if ( !info ) >> + return; >> + >> + /* >> + * Free resources of features. But we do not free global feature list >> + * entry, like feat_l3_cat. Although it may cause a few memory leak, >> + * it is OK simplify things. > >I don't think it is OK to leak memory in the hypervisor in general. >Please specify why it is OK in this particular case in the comment. The problem is to call this a leak in the comment. There's no leak here, the allocation is simply being kept until the next CPU online attempt. Jan
On Mon, Feb 27, 2017 at 02:42:31PM +0800, Yi Sun wrote: > On 17-02-26 17:41:08, Wei Liu wrote: > > On Wed, Feb 15, 2017 at 04:49:19PM +0800, Yi Sun wrote: > > > This patch implements the CPU init and free flow including L3 CAT > > > initialization and feature list free. > > > > > > Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com> > > > > Either you need to use a separate patch to move cpuid_count_leaf or you > > should state it is moved in the commit message. > > > Thanks! I will add description in commit message. > > > > + > > > +/* Common functions. */ > > > +static void free_feature(struct psr_socket_info *info) > > > +{ > > > + struct feat_node *feat, *next; > > > + > > > + if ( !info ) > > > + return; > > > + > > > + /* > > > + * Free resources of features. But we do not free global feature list > > > + * entry, like feat_l3_cat. Although it may cause a few memory leak, > > > + * it is OK simplify things. > > > > I don't think it is OK to leak memory in the hypervisor in general. > > Please specify why it is OK in this particular case in the comment. > > > In most cases, such global feature list entry will be added into feature list > so that it can be freed here. > > In extreme case, e.g. socket 1 does not support L3 CAT. The feat_l3_cat > allocated in psr_cpu_prepare will not be released. But this is rare case. > > Jan, Konrad and me disucssed this before. Per Jan's suggestion, we do not free > it. Then I would suggest you to not use "leak" in the comment. And put the relevant bits from the discussion in the comment. Otherwise a drive-by reviewer like me will call this out again. :-) > > > > + */ > > > + list_for_each_entry_safe(feat, next, &info->feat_list, list) > > > + { > > > + if ( !feat ) > > > + return; > > > + > > > + __clear_bit(feat->feature, &info->feat_mask); > > > + list_del(&feat->list); > > > + xfree(feat); > > > + } > > > +} > > > + > > > -static int psr_cpu_prepare(unsigned int cpu) > > > +static void cpu_init_work(void) > > > +{ > > > + struct psr_socket_info *info; > > > + unsigned int socket; > > > + unsigned int cpu = smp_processor_id(); > > > + struct feat_node *feat; > > > + struct cpuid_leaf regs = { .a = 0, .b = 0, .c = 0, .d = 0 }; > > > + > > > + if ( !cpu_has(¤t_cpu_data, X86_FEATURE_PQE) ) > > > + return; > > > + else if ( current_cpu_data.cpuid_level < PSR_CPUID_LEVEL_CAT ) > > > + { > > > + __clear_bit(X86_FEATURE_PQE, current_cpu_data.x86_capability); > > > + return; > > > + } > > > + > > > + socket = cpu_to_socket(cpu); > > > + info = socket_info + socket; > > > + if ( info->feat_mask ) > > > + return; > > > + > > > + INIT_LIST_HEAD(&info->feat_list); > > > + spin_lock_init(&info->ref_lock); > > > + > > > + cpuid_count_leaf(PSR_CPUID_LEVEL_CAT, 0, ®s); > > > + if ( regs.b & PSR_RESOURCE_TYPE_L3 ) > > > + { > > > > You can move > > > > struct feat_node *feat > > > > here. > > > This variable will also be used by L2 CAT which codes exist in a different > branch. So, I declare it at the top of the function. Please refer below > patch: > [PATCH v8 17/24] x86: L2 CAT: implement CPU init and free flow. OK. > > > > + cpuid_count_leaf(PSR_CPUID_LEVEL_CAT, 1, ®s); > > > + > > > + feat = feat_l3_cat; > > > + /* psr_cpu_prepare will allocate it on subsequent CPU onlining. */ > > > + feat_l3_cat = NULL; > > > + feat->ops = l3_cat_ops; > > > + > > > + l3_cat_init_feature(regs, feat, info); > > > + } > > > +} > > > + > > [...] > > > > > > @@ -359,7 +528,7 @@ static int cpu_callback( > > > switch ( action ) > > > { > > > case CPU_UP_PREPARE: > > > - rc = psr_cpu_prepare(cpu); > > > + rc = psr_cpu_prepare(); > > > break; > > > case CPU_STARTING: > > > psr_cpu_init(); > > > @@ -388,10 +557,14 @@ static int __init psr_presmp_init(void) > > > if ( (opt_psr & PSR_CMT) && opt_rmid_max ) > > > init_psr_cmt(opt_rmid_max); > > > > > > - psr_cpu_prepare(0); > > > + if ( opt_psr & PSR_CAT ) > > > + init_psr(); > > > + > > > + if ( psr_cpu_prepare() ) > > > + psr_free(); > > > > > > psr_cpu_init(); > > > - if ( psr_cmt_enabled() ) > > > + if ( psr_cmt_enabled() || socket_info ) > > > > Why not have psr_cat_enabled() here? > > > psr_cmt_enabled() returns true of false by checking if the global pointer > 'psr_cmt' has been allocated or not. The 'psr_cmt' is also used in sysctl.c. > For allocation features, we have a similar global pointer 'socket_info'. But > it is only used in psr.c and all allocation features(CAT/CDP/MBA) use it. So > we directly use it in psr.c to check if related initialization has been done. The problem with using socket_info directly is that the name doesn't tell you much. It doesn't carry specific semantics by itself. Wrapping it inside an inline function with a proper name is much nicer. Also there is the possibility that in the future you change the code to use socket_info for a slightly different purpose or you want to expose it outside of psr.c, you then need to retrospectively inspect all sites to make sure you don't screw things up. IMHO using a psr_XXX_enabled like psr_cmt_enabled is a small change with big benefit. This is just a general suggestion. I don't feel too strongly about this. If the maintainers are happy with the code as-is, you don't need to change. Wei. > > > Wei.
>>> On 15.02.17 at 09:49, <yi.y.sun@linux.intel.com> wrote: > This patch implements the CPU init and free flow including L3 CAT > initialization and feature list free. I think from the final-effect-of-the-patch point of view the L3 CAT aspect is more relevant, so that's what should appear in the title. > @@ -136,11 +140,87 @@ struct psr_assoc { > > struct psr_cmt *__read_mostly psr_cmt; > > +static struct psr_socket_info *__read_mostly socket_info; > + > static unsigned int opt_psr; > static unsigned int __initdata opt_rmid_max = 255; > +static unsigned int __read_mostly opt_cos_max = MAX_COS_REG_CNT; > static uint64_t rmid_mask; > static DEFINE_PER_CPU(struct psr_assoc, psr_assoc); > > +/* > + * Declare global feature list entry for every feature to facilitate the > + * feature list creation. It will be allocated in psr_cpu_prepare() and > + * inserted into feature list in cpu_init_work(). It is protected by > + * cpu_add_remove_lock spinlock. > + */ > +static struct feat_node *feat_l3_cat; The comment is misleading imo: The only relevant aspect here is that of when the allocation would need to happen vs. when it actually happens (because putting it where we'd like it to be is not easily possible). There's nothing list related here. I'd recommend simply saying that we need a place to transiently store a spare node. > +/* Common functions. */ > +static void free_feature(struct psr_socket_info *info) > +{ > + struct feat_node *feat, *next; > + > + if ( !info ) > + return; > + > + /* > + * Free resources of features. But we do not free global feature list > + * entry, like feat_l3_cat. Although it may cause a few memory leak, > + * it is OK simplify things. > + */ > + list_for_each_entry_safe(feat, next, &info->feat_list, list) > + { > + if ( !feat ) > + return; How would that happen? But well, this is going to go away anyway with the move from list to array. > @@ -335,18 +418,104 @@ void psr_domain_free(struct domain *d) > psr_free_rmid(d); > } > > -static int psr_cpu_prepare(unsigned int cpu) > +static void cpu_init_work(void) > +{ > + struct psr_socket_info *info; > + unsigned int socket; > + unsigned int cpu = smp_processor_id(); > + struct feat_node *feat; > + struct cpuid_leaf regs = { .a = 0, .b = 0, .c = 0, .d = 0 }; I don't see you needing an initializer here at all, but if you really want one for some reason, the same effect can be had with just {}. > + if ( !cpu_has(¤t_cpu_data, X86_FEATURE_PQE) ) Do you really mean to not universally check the global (boot CPU) flag? I.e. possibly differing behavior on different CPUs? > + return; > + else if ( current_cpu_data.cpuid_level < PSR_CPUID_LEVEL_CAT ) Pointless "else". > + { > + __clear_bit(X86_FEATURE_PQE, current_cpu_data.x86_capability); setup_clear_cpu_cap() if you use boot_cpu_has() above. > + return; > + } > + > + socket = cpu_to_socket(cpu); > + info = socket_info + socket; > + if ( info->feat_mask ) > + return; > + > + INIT_LIST_HEAD(&info->feat_list); > + spin_lock_init(&info->ref_lock); > + > + 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); > + > + feat = feat_l3_cat; > + /* psr_cpu_prepare will allocate it on subsequent CPU onlining. */ > + feat_l3_cat = NULL; I don't think the comment is very useful: You've consumed the object, so you simply should not leave a dangling pointer (or else you'd risk multiple use). > static void psr_cpu_init(void) > { > + if ( socket_info ) > + cpu_init_work(); > + > psr_assoc_init(); > } > > static void psr_cpu_fini(unsigned int cpu) > { > + if ( socket_info ) > + cpu_fini_work(cpu); > return; > } Is it really useful to use another layer of helper functions here? Jan
On 17-03-08 07:56:54, Jan Beulich wrote: > >>> On 15.02.17 at 09:49, <yi.y.sun@linux.intel.com> wrote: [...] > > -static int psr_cpu_prepare(unsigned int cpu) > > +static void cpu_init_work(void) > > +{ > > + struct psr_socket_info *info; > > + unsigned int socket; > > + unsigned int cpu = smp_processor_id(); > > + struct feat_node *feat; > > + struct cpuid_leaf regs = { .a = 0, .b = 0, .c = 0, .d = 0 }; > > I don't see you needing an initializer here at all, but if you really > want one for some reason, the same effect can be had with just > {}. > Konrad suggested me to initialize it like this in v5 patch review comments. I think he has experienced some strange issues when he forgot to set _all_ the entries a structure allocated on the stack. > > + if ( !cpu_has(¤t_cpu_data, X86_FEATURE_PQE) ) > > Do you really mean to not universally check the global (boot CPU) > flag? I.e. possibly differing behavior on different CPUs? > Yes, different sockets may have different configurations. E.g. sockt 0 has PQE support but socket 1 does not. Per my info, there is only one boot cpu no matter how many sockets there are. > > + return; > > + else if ( current_cpu_data.cpuid_level < PSR_CPUID_LEVEL_CAT ) > > Pointless "else". > Thanks, will remove it. > > + { > > + __clear_bit(X86_FEATURE_PQE, current_cpu_data.x86_capability); > > setup_clear_cpu_cap() if you use boot_cpu_has() above. > > > + return; > > + } > > + > > + socket = cpu_to_socket(cpu); > > + info = socket_info + socket; > > + if ( info->feat_mask ) > > + return; > > + > > + INIT_LIST_HEAD(&info->feat_list); > > + spin_lock_init(&info->ref_lock); > > + > > + 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); > > + > > + feat = feat_l3_cat; > > + /* psr_cpu_prepare will allocate it on subsequent CPU onlining. */ > > + feat_l3_cat = NULL; > > I don't think the comment is very useful: You've consumed the object, > so you simply should not leave a dangling pointer (or else you'd risk > multiple use). > Will remove it. > > static void psr_cpu_init(void) > > { > > + if ( socket_info ) > > + cpu_init_work(); > > + > > psr_assoc_init(); > > } > > > > static void psr_cpu_fini(unsigned int cpu) > > { > > + if ( socket_info ) > > + cpu_fini_work(cpu); > > return; > > } > > Is it really useful to use another layer of helper functions here? > The reason we define 'cpu_fini_work' is to match 'cpu_init_work'. If we move codes of 'cpu_init_work' into 'psr_cpu_init', the codes look messy. That is the reason we define 'cpu_init_work'. Do you think if it is acceptable to you? > Jan
>>> On 10.03.17 at 02:32, <yi.y.sun@linux.intel.com> wrote: > On 17-03-08 07:56:54, Jan Beulich wrote: >> >>> On 15.02.17 at 09:49, <yi.y.sun@linux.intel.com> wrote: >> > -static int psr_cpu_prepare(unsigned int cpu) >> > +static void cpu_init_work(void) >> > +{ >> > + struct psr_socket_info *info; >> > + unsigned int socket; >> > + unsigned int cpu = smp_processor_id(); >> > + struct feat_node *feat; >> > + struct cpuid_leaf regs = { .a = 0, .b = 0, .c = 0, .d = 0 }; >> >> I don't see you needing an initializer here at all, but if you really >> want one for some reason, the same effect can be had with just >> {}. >> > Konrad suggested me to initialize it like this in v5 patch review comments. > I think he has experienced some strange issues when he forgot to set _all_ > the entries a structure allocated on the stack. I can see there being cases where this is desirable; I don't think this is one of them. (See also a related comment I had made on a later patch.) >> > + if ( !cpu_has(¤t_cpu_data, X86_FEATURE_PQE) ) >> >> Do you really mean to not universally check the global (boot CPU) >> flag? I.e. possibly differing behavior on different CPUs? >> > Yes, different sockets may have different configurations. E.g. sockt 0 has > PQE support but socket 1 does not. For all other CPU features we assume symmetry. Why would we not do so here? And how would things even work in that case, when a vCPU gets moved (by the scheduler) from a more capable to a less capable pCPU? >> > static void psr_cpu_init(void) >> > { >> > + if ( socket_info ) >> > + cpu_init_work(); >> > + >> > psr_assoc_init(); >> > } >> > >> > static void psr_cpu_fini(unsigned int cpu) >> > { >> > + if ( socket_info ) >> > + cpu_fini_work(cpu); >> > return; >> > } >> >> Is it really useful to use another layer of helper functions here? >> > The reason we define 'cpu_fini_work' is to match 'cpu_init_work'. And the question was for both of them. > If we move > codes of 'cpu_init_work' into 'psr_cpu_init', the codes look messy. I don't think that's the case; I could see this as a valid argument if the calling functions above were already quite complex. > That is > the reason we define 'cpu_init_work'. Do you think if it is acceptable to > you? Well, I won't NAK the patch if you keep them, but I'd prefer the functions to be folded into their callers. Jan
On 17-03-10 01:56:04, Jan Beulich wrote: > >>> On 10.03.17 at 02:32, <yi.y.sun@linux.intel.com> wrote: > > On 17-03-08 07:56:54, Jan Beulich wrote: > >> >>> On 15.02.17 at 09:49, <yi.y.sun@linux.intel.com> wrote: [...] > >> > + if ( !cpu_has(¤t_cpu_data, X86_FEATURE_PQE) ) > >> > >> Do you really mean to not universally check the global (boot CPU) > >> flag? I.e. possibly differing behavior on different CPUs? > >> > > Yes, different sockets may have different configurations. E.g. sockt 0 has > > PQE support but socket 1 does not. > > For all other CPU features we assume symmetry. Why would we > not do so here? And how would things even work in that case, > when a vCPU gets moved (by the scheduler) from a more capable > to a less capable pCPU? > Hmm, ok, I will use boot cpu to check PQE here. Thanks! > >> > static void psr_cpu_init(void) > >> > { > >> > + if ( socket_info ) > >> > + cpu_init_work(); > >> > + > >> > psr_assoc_init(); > >> > } > >> > > >> > static void psr_cpu_fini(unsigned int cpu) > >> > { > >> > + if ( socket_info ) > >> > + cpu_fini_work(cpu); > >> > return; > >> > } > >> > >> Is it really useful to use another layer of helper functions here? > >> > > The reason we define 'cpu_fini_work' is to match 'cpu_init_work'. > > And the question was for both of them. > > > If we move > > codes of 'cpu_init_work' into 'psr_cpu_init', the codes look messy. > > I don't think that's the case; I could see this as a valid argument if > the calling functions above were already quite complex. > > > That is > > the reason we define 'cpu_init_work'. Do you think if it is acceptable to > > you? > > Well, I won't NAK the patch if you keep them, but I'd prefer the > functions to be folded into their callers. > I will move the codes. > Jan
diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c index e0a387e..e3e92dd 100644 --- a/xen/arch/x86/cpuid.c +++ b/xen/arch/x86/cpuid.c @@ -34,12 +34,6 @@ static void cpuid_leaf(uint32_t leaf, struct cpuid_leaf *data) cpuid(leaf, &data->a, &data->b, &data->c, &data->d); } -static void cpuid_count_leaf(uint32_t leaf, uint32_t subleaf, - struct cpuid_leaf *data) -{ - cpuid_count(leaf, subleaf, &data->a, &data->b, &data->c, &data->d); -} - static void sanitise_featureset(uint32_t *fs) { /* for_each_set_bit() uses unsigned longs. Extend with zeroes. */ diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c index 5acd9ca..10d268a 100644 --- a/xen/arch/x86/psr.c +++ b/xen/arch/x86/psr.c @@ -19,6 +19,7 @@ #include <xen/list.h> #include <xen/sched.h> #include <asm/psr.h> +#include <asm/x86_emulate.h> /* * Terminology: @@ -35,6 +36,9 @@ #define PSR_CAT (1<<1) #define PSR_CDP (1<<2) +#define CAT_CBM_LEN_MASK 0x1f +#define CAT_COS_MAX_MASK 0xffff + /* * Per SDM chapter 'Cache Allocation Technology: Cache Mask Configuration', * the MSRs ranging from 0C90H through 0D0FH (inclusive), enables support for @@ -136,11 +140,87 @@ struct psr_assoc { struct psr_cmt *__read_mostly psr_cmt; +static struct psr_socket_info *__read_mostly socket_info; + static unsigned int opt_psr; static unsigned int __initdata opt_rmid_max = 255; +static unsigned int __read_mostly opt_cos_max = MAX_COS_REG_CNT; static uint64_t rmid_mask; static DEFINE_PER_CPU(struct psr_assoc, psr_assoc); +/* + * Declare global feature list entry for every feature to facilitate the + * feature list creation. It will be allocated in psr_cpu_prepare() and + * inserted into feature list in cpu_init_work(). It is protected by + * cpu_add_remove_lock spinlock. + */ +static struct feat_node *feat_l3_cat; + +/* Common functions. */ +static void free_feature(struct psr_socket_info *info) +{ + struct feat_node *feat, *next; + + if ( !info ) + return; + + /* + * Free resources of features. But we do not free global feature list + * entry, like feat_l3_cat. Although it may cause a few memory leak, + * it is OK simplify things. + */ + list_for_each_entry_safe(feat, next, &info->feat_list, list) + { + if ( !feat ) + return; + + __clear_bit(feat->feature, &info->feat_mask); + list_del(&feat->list); + xfree(feat); + } +} + +/* L3 CAT functions implementation. */ +static void l3_cat_init_feature(struct cpuid_leaf regs, + struct feat_node *feat, + struct psr_socket_info *info) +{ + struct psr_cat_hw_info l3_cat = { }; + unsigned int socket; + + /* No valid value so do not enable feature. */ + if ( !regs.a || !regs.d ) + return; + + l3_cat.cbm_len = (regs.a & CAT_CBM_LEN_MASK) + 1; + l3_cat.cos_max = min(opt_cos_max, regs.d & CAT_COS_MAX_MASK); + + /* cos=0 is reserved as default cbm(all bits within cbm_len are 1). */ + feat->cos_reg_val[0] = (1ull << l3_cat.cbm_len) - 1; + + feat->feature = PSR_SOCKET_L3_CAT; + ASSERT(!test_bit(PSR_SOCKET_L3_CAT, &info->feat_mask)); + __set_bit(PSR_SOCKET_L3_CAT, &info->feat_mask); + + feat->info.l3_cat_info = l3_cat; + + info->nr_feat++; + + /* Add this feature into list. */ + list_add_tail(&feat->list, &info->feat_list); + + socket = cpu_to_socket(smp_processor_id()); + if ( !opt_cpu_info ) + return; + + printk(XENLOG_INFO "L3 CAT: enabled on socket %u, cos_max:%u, cbm_len:%u\n", + socket, feat->info.l3_cat_info.cos_max, + feat->info.l3_cat_info.cbm_len); +} + +static const struct feat_ops l3_cat_ops = { +}; + static void __init parse_psr_bool(char *s, char *value, char *feature, unsigned int mask) { @@ -180,6 +260,9 @@ static void __init parse_psr_param(char *s) if ( val_str && !strcmp(s, "rmid_max") ) opt_rmid_max = simple_strtoul(val_str, NULL, 0); + if ( val_str && !strcmp(s, "cos_max") ) + opt_cos_max = simple_strtoul(val_str, NULL, 0); + s = ss + 1; } while ( ss ); } @@ -335,18 +418,104 @@ void psr_domain_free(struct domain *d) psr_free_rmid(d); } -static int psr_cpu_prepare(unsigned int cpu) +static void cpu_init_work(void) +{ + struct psr_socket_info *info; + unsigned int socket; + unsigned int cpu = smp_processor_id(); + struct feat_node *feat; + struct cpuid_leaf regs = { .a = 0, .b = 0, .c = 0, .d = 0 }; + + if ( !cpu_has(¤t_cpu_data, X86_FEATURE_PQE) ) + return; + else if ( current_cpu_data.cpuid_level < PSR_CPUID_LEVEL_CAT ) + { + __clear_bit(X86_FEATURE_PQE, current_cpu_data.x86_capability); + return; + } + + socket = cpu_to_socket(cpu); + info = socket_info + socket; + if ( info->feat_mask ) + return; + + INIT_LIST_HEAD(&info->feat_list); + spin_lock_init(&info->ref_lock); + + 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); + + feat = feat_l3_cat; + /* psr_cpu_prepare will allocate it on subsequent CPU onlining. */ + feat_l3_cat = NULL; + feat->ops = l3_cat_ops; + + l3_cat_init_feature(regs, feat, info); + } +} + +static void cpu_fini_work(unsigned int cpu) { + unsigned int socket = cpu_to_socket(cpu); + + /* + * We only free when we are the last CPU in the socket. The socket_cpumask + * is cleared prior to this notification code by remove_siblinginfo(). + */ + if ( socket_cpumask[socket] && cpumask_empty(socket_cpumask[socket]) ) + free_feature(socket_info + socket); +} + +static void __init init_psr(void) +{ + if ( opt_cos_max < 1 ) + { + printk(XENLOG_INFO "CAT: disabled, cos_max is too small\n"); + return; + } + + socket_info = xzalloc_array(struct psr_socket_info, nr_sockets); + + if ( !socket_info ) + { + printk(XENLOG_INFO "Failed to alloc socket_info!\n"); + return; + } +} + +static void __init psr_free(void) +{ + xfree(socket_info); + socket_info = NULL; +} + +static int psr_cpu_prepare(void) +{ + if ( !socket_info ) + return 0; + + /* Malloc memory for the global feature head here. */ + if ( feat_l3_cat == NULL && + (feat_l3_cat = xzalloc(struct feat_node)) == NULL ) + return -ENOMEM; + return 0; } static void psr_cpu_init(void) { + if ( socket_info ) + cpu_init_work(); + psr_assoc_init(); } static void psr_cpu_fini(unsigned int cpu) { + if ( socket_info ) + cpu_fini_work(cpu); return; } @@ -359,7 +528,7 @@ static int cpu_callback( switch ( action ) { case CPU_UP_PREPARE: - rc = psr_cpu_prepare(cpu); + rc = psr_cpu_prepare(); break; case CPU_STARTING: psr_cpu_init(); @@ -388,10 +557,14 @@ static int __init psr_presmp_init(void) if ( (opt_psr & PSR_CMT) && opt_rmid_max ) init_psr_cmt(opt_rmid_max); - psr_cpu_prepare(0); + if ( opt_psr & PSR_CAT ) + init_psr(); + + if ( psr_cpu_prepare() ) + psr_free(); psr_cpu_init(); - if ( psr_cmt_enabled() ) + if ( psr_cmt_enabled() || socket_info ) register_cpu_notifier(&cpu_nfb); return 0; diff --git a/xen/include/asm-x86/processor.h b/xen/include/asm-x86/processor.h index 7735bc2..eb0a1e9 100644 --- a/xen/include/asm-x86/processor.h +++ b/xen/include/asm-x86/processor.h @@ -14,6 +14,7 @@ #include <asm/types.h> #include <asm/cpufeature.h> #include <asm/desc.h> +#include <asm/x86_emulate.h> #endif #include <asm/x86-defns.h> @@ -259,6 +260,12 @@ static always_inline unsigned int cpuid_count_ebx( return ebx; } +static always_inline void cpuid_count_leaf(uint32_t leaf, uint32_t subleaf, + struct cpuid_leaf *data) +{ + cpuid_count(leaf, subleaf, &data->a, &data->b, &data->c, &data->d); +} + static inline unsigned long read_cr0(void) { unsigned long cr0;
This patch implements the CPU init and free flow including L3 CAT initialization and feature list free. Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com> --- v8: - remove unused parameter of psr_cpu_prepare. - add comments in cpu_fini_work. - coding style fix. --- xen/arch/x86/cpuid.c | 6 -- xen/arch/x86/psr.c | 181 +++++++++++++++++++++++++++++++++++++++- xen/include/asm-x86/processor.h | 7 ++ 3 files changed, 184 insertions(+), 10 deletions(-)