| Message ID | 1491054836-30488-6-git-send-email-yi.y.sun@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 01.04.17 at 15:53, <yi.y.sun@linux.intel.com> wrote: > @@ -76,7 +79,7 @@ struct feat_node { > * > * Feature independent HW info and common values are also defined in it. > */ > - const struct feat_props { > + struct feat_props { As said before, the const here should stay. The init-time writing to the structure can be done without going through this pointer. > +static void free_socket_resources(unsigned int socket) > +{ > + unsigned int i; > + struct psr_socket_info *info = socket_info + socket; > + > + if ( !info ) > + return; > + > + /* > + * Free resources of features. The global feature object, e.g. feat_l3_cat, > + * may not be freed here if it is not added into array. It is simply being > + * kept until the next CPU online attempt. > + */ > + for ( i = 0; i < PSR_SOCKET_MAX_FEAT; i++ ) > + { > + if ( !info->features[i] ) > + continue; > + > + xfree(info->features[i]); > + info->features[i] = NULL; There's no need for the if() here. And I'm sure this was pointed out already (perhaps in a different context). > +static bool feat_init_done(const struct psr_socket_info *info) > +{ > + unsigned int i; > + > + for ( i = 0; i < PSR_SOCKET_MAX_FEAT; i++ ) > + { > + if ( !info->features[i] ) > + continue; > + > + return true; > + } > + > + return false; > +} At the first glance this is a very strange function. > +/* CAT common functions implementation. */ > +static void cat_init_feature(const struct cpuid_leaf *regs, > + struct feat_node *feat, > + struct psr_socket_info *info, > + enum psr_feat_type type) > +{ > + unsigned int socket, i; > + > + /* No valid value so do not enable feature. */ > + if ( !regs->a || !regs->d ) > + return; > + > + feat->props->cbm_len = (regs->a & CAT_CBM_LEN_MASK) + 1; > + feat->props->cos_max = min(opt_cos_max, regs->d & CAT_COS_MAX_MASK); > + > + switch ( type ) > + { > + case PSR_SOCKET_L3_CAT: > + /* cos=0 is reserved as default cbm(all bits within cbm_len are 1). */ > + feat->cos_reg_val[0] = cat_default_val(feat->props->cbm_len); > + > + /* > + * To handle cpu offline and then online case, we need restore MSRs to > + * default values. > + */ > + for ( i = 1; i <= feat->props->cos_max; i++ ) > + { > + wrmsrl(MSR_IA32_PSR_L3_MASK(i), feat->cos_reg_val[0]); > + feat->cos_reg_val[i] = feat->cos_reg_val[0]; > + } I continue to have difficulty with this: Why is offline-then-online any different from first-time-online? Why wouldn't setting the registers to their intended values not be taken care of by context switch code, once vCPU-s get scheduled onto the newly onlined CPU? > + break; > + > + default: > + return; > + } > + > + /* Add this feature into array. */ > + info->features[type] = feat; > + > + socket = cpu_to_socket(smp_processor_id()); No need for this variable, and definitely no need to do the assignment ahead of ... > + if ( !opt_cpu_info ) > + return; ... this. > static void psr_cpu_init(void) > { > + struct psr_socket_info *info; > + unsigned int socket; > + unsigned int cpu = smp_processor_id(); > + struct feat_node *feat; > + struct cpuid_leaf regs; > + > + if ( !psr_alloc_feat_enabled() || !boot_cpu_has(X86_FEATURE_PQE) ) > + goto assoc_init; > + > + if ( boot_cpu_data.cpuid_level < PSR_CPUID_LEVEL_CAT ) > + { > + setup_clear_cpu_cap(X86_FEATURE_PQE); > + goto assoc_init; > + } > + > + socket = cpu_to_socket(cpu); > + info = socket_info + socket; > + if ( feat_init_done(info) ) > + goto assoc_init; Hmm, so you bail here if any of the features was already set up. But you don't bail if none of the features were available as the reason for the setup not having been done before. I think this can be solved in a better way once we have the static props array: You need to do anything here only if the props slot is not NULL, but the feature slot is NULL. In any event you intentions are likely easier to understand for a reader if this single-use function was inlined here. Jan
On 17-04-05 09:10:58, Jan Beulich wrote: > >>> On 01.04.17 at 15:53, <yi.y.sun@linux.intel.com> wrote: > > @@ -76,7 +79,7 @@ struct feat_node { > > * > > * Feature independent HW info and common values are also defined in it. > > */ > > - const struct feat_props { > > + struct feat_props { > > As said before, the const here should stay. The init-time writing > to the structure can be done without going through this pointer. > 'feat_props' contains 'cos_max' and 'cbm_len' in this version. I have to assign values to them in 'cat_init_feature'. So, I removed the 'const'. Anyway, this structure will be moved out as a standalone struct to define a static pointer array of 'props'. The 'cos_max' and 'cbm_len' will be moved to 'struct feat_node'. > > +static void free_socket_resources(unsigned int socket) > > +{ > > + unsigned int i; > > + struct psr_socket_info *info = socket_info + socket; > > + > > + if ( !info ) > > + return; > > + > > + /* > > + * Free resources of features. The global feature object, e.g. feat_l3_cat, > > + * may not be freed here if it is not added into array. It is simply being > > + * kept until the next CPU online attempt. > > + */ > > + for ( i = 0; i < PSR_SOCKET_MAX_FEAT; i++ ) > > + { > > + if ( !info->features[i] ) > > + continue; > > + > > + xfree(info->features[i]); > > + info->features[i] = NULL; > > There's no need for the if() here. And I'm sure this was pointed out > already (perhaps in a different context). > There may be NULL member in features array. Features array contains all features, including L3 CAT, CDP and L2 CAT. But on some machines, they may only support L3 CAT but do not support CDP and L2 CAT. So, the features array only has L3 CAT member in it and all other members are all NULL. That is the reason we must check if the member is NULL or not. > > +static bool feat_init_done(const struct psr_socket_info *info) > > +{ > > + unsigned int i; > > + > > + for ( i = 0; i < PSR_SOCKET_MAX_FEAT; i++ ) > > + { > > + if ( !info->features[i] ) > > + continue; > > + > > + return true; > > + } > > + > > + return false; > > +} > > At the first glance this is a very strange function. > I used 'feat_mask' before to check if any feature has been initialized. Per your comment in later patch, I want to define a flag to represent it. Is that acceptable to you? > > +/* CAT common functions implementation. */ > > +static void cat_init_feature(const struct cpuid_leaf *regs, > > + struct feat_node *feat, > > + struct psr_socket_info *info, > > + enum psr_feat_type type) > > +{ > > + unsigned int socket, i; > > + > > + /* No valid value so do not enable feature. */ > > + if ( !regs->a || !regs->d ) > > + return; > > + > > + feat->props->cbm_len = (regs->a & CAT_CBM_LEN_MASK) + 1; > > + feat->props->cos_max = min(opt_cos_max, regs->d & CAT_COS_MAX_MASK); > > + > > + switch ( type ) > > + { > > + case PSR_SOCKET_L3_CAT: > > + /* cos=0 is reserved as default cbm(all bits within cbm_len are 1). */ > > + feat->cos_reg_val[0] = cat_default_val(feat->props->cbm_len); > > + > > + /* > > + * To handle cpu offline and then online case, we need restore MSRs to > > + * default values. > > + */ > > + for ( i = 1; i <= feat->props->cos_max; i++ ) > > + { > > + wrmsrl(MSR_IA32_PSR_L3_MASK(i), feat->cos_reg_val[0]); > > + feat->cos_reg_val[i] = feat->cos_reg_val[0]; > > + } > > I continue to have difficulty with this: Why is offline-then-online > any different from first-time-online? Why wouldn't setting the May remove this comment. Per current codes, the MSRs are written to default values no matter first time or not. > registers to their intended values not be taken care of by > context switch code, once vCPU-s get scheduled onto the newly > onlined CPU? > cat_init_feature is only called when the first CPU on a socket is online. The MSRs to set are per socket. So, we only need set it once when socket is online. > > + break; > > + > > + default: > > + return; > > + } > > + > > + /* Add this feature into array. */ > > + info->features[type] = feat; > > + > > + socket = cpu_to_socket(smp_processor_id()); > > No need for this variable, and definitely no need to do the > assignment ahead of ... > > + if ( !opt_cpu_info ) > > + return; > > ... this. Ok, will remove socket to directly use 'cpu_to_socket(smp_processor_id())'. > > > static void psr_cpu_init(void) > > { > > + struct psr_socket_info *info; > > + unsigned int socket; > > + unsigned int cpu = smp_processor_id(); > > + struct feat_node *feat; > > + struct cpuid_leaf regs; > > + > > + if ( !psr_alloc_feat_enabled() || !boot_cpu_has(X86_FEATURE_PQE) ) > > + goto assoc_init; > > + > > + if ( boot_cpu_data.cpuid_level < PSR_CPUID_LEVEL_CAT ) > > + { > > + setup_clear_cpu_cap(X86_FEATURE_PQE); > > + goto assoc_init; > > + } > > + > > + socket = cpu_to_socket(cpu); > > + info = socket_info + socket; > > + if ( feat_init_done(info) ) > > + goto assoc_init; > > Hmm, so you bail here if any of the features was already set up. > But you don't bail if none of the features were available as the > reason for the setup not having been done before. I think this > can be solved in a better way once we have the static props > array: You need to do anything here only if the props slot is not > NULL, but the feature slot is NULL. > As above comment, this check will be changed to check a flag if you have no opinion. But, what is your conern here? Do you mind the 'goto'? > In any event you intentions are likely easier to understand for > a reader if this single-use function was inlined here. > As you have observed, this 'feat_init_done' is used many times in later patches. > Jan
>>> On 06.04.17 at 07:49, <yi.y.sun@linux.intel.com> wrote: > On 17-04-05 09:10:58, Jan Beulich wrote: >> >>> On 01.04.17 at 15:53, <yi.y.sun@linux.intel.com> wrote: >> > +static void free_socket_resources(unsigned int socket) >> > +{ >> > + unsigned int i; >> > + struct psr_socket_info *info = socket_info + socket; >> > + >> > + if ( !info ) >> > + return; >> > + >> > + /* >> > + * Free resources of features. The global feature object, e.g. feat_l3_cat, >> > + * may not be freed here if it is not added into array. It is simply being >> > + * kept until the next CPU online attempt. >> > + */ >> > + for ( i = 0; i < PSR_SOCKET_MAX_FEAT; i++ ) >> > + { >> > + if ( !info->features[i] ) >> > + continue; >> > + >> > + xfree(info->features[i]); >> > + info->features[i] = NULL; >> >> There's no need for the if() here. And I'm sure this was pointed out >> already (perhaps in a different context). >> > There may be NULL member in features array. Features array contains all > features, including L3 CAT, CDP and L2 CAT. But on some machines, they > may only support L3 CAT but do not support CDP and L2 CAT. So, the features > array only has L3 CAT member in it and all other members are all NULL. That > is the reason we must check if the member is NULL or not. No, and this has been explained before: xfree() happily accepts NULL pointers. >> > +static bool feat_init_done(const struct psr_socket_info *info) >> > +{ >> > + unsigned int i; >> > + >> > + for ( i = 0; i < PSR_SOCKET_MAX_FEAT; i++ ) >> > + { >> > + if ( !info->features[i] ) >> > + continue; >> > + >> > + return true; >> > + } >> > + >> > + return false; >> > +} >> >> At the first glance this is a very strange function. >> > I used 'feat_mask' before to check if any feature has been initialized. > Per your comment in later patch, I want to define a flag to represent it. > Is that acceptable to you? Excuse me, but if I suggested it there, how can it not be acceptable to me? >> > +/* CAT common functions implementation. */ >> > +static void cat_init_feature(const struct cpuid_leaf *regs, >> > + struct feat_node *feat, >> > + struct psr_socket_info *info, >> > + enum psr_feat_type type) >> > +{ >> > + unsigned int socket, i; >> > + >> > + /* No valid value so do not enable feature. */ >> > + if ( !regs->a || !regs->d ) >> > + return; >> > + >> > + feat->props->cbm_len = (regs->a & CAT_CBM_LEN_MASK) + 1; >> > + feat->props->cos_max = min(opt_cos_max, regs->d & CAT_COS_MAX_MASK); >> > + >> > + switch ( type ) >> > + { >> > + case PSR_SOCKET_L3_CAT: >> > + /* cos=0 is reserved as default cbm(all bits within cbm_len are 1). */ >> > + feat->cos_reg_val[0] = cat_default_val(feat->props->cbm_len); >> > + >> > + /* >> > + * To handle cpu offline and then online case, we need restore MSRs to >> > + * default values. >> > + */ >> > + for ( i = 1; i <= feat->props->cos_max; i++ ) >> > + { >> > + wrmsrl(MSR_IA32_PSR_L3_MASK(i), feat->cos_reg_val[0]); >> > + feat->cos_reg_val[i] = feat->cos_reg_val[0]; >> > + } >> >> I continue to have difficulty with this: Why is offline-then-online >> any different from first-time-online? Why wouldn't setting the > > May remove this comment. Per current codes, the MSRs are written to default > values no matter first time or not. > >> registers to their intended values not be taken care of by >> context switch code, once vCPU-s get scheduled onto the newly >> onlined CPU? >> > cat_init_feature is only called when the first CPU on a socket is online. > The MSRs to set are per socket. So, we only need set it once when socket > is online. This does not answer my question. Once again - why does this need doing here explicitly, rather than relying on the needed values being loaded when the first vCPU gets scheduled onto one of the pCPU-s of this socket? >> > static void psr_cpu_init(void) >> > { >> > + struct psr_socket_info *info; >> > + unsigned int socket; >> > + unsigned int cpu = smp_processor_id(); >> > + struct feat_node *feat; >> > + struct cpuid_leaf regs; >> > + >> > + if ( !psr_alloc_feat_enabled() || !boot_cpu_has(X86_FEATURE_PQE) ) >> > + goto assoc_init; >> > + >> > + if ( boot_cpu_data.cpuid_level < PSR_CPUID_LEVEL_CAT ) >> > + { >> > + setup_clear_cpu_cap(X86_FEATURE_PQE); >> > + goto assoc_init; >> > + } >> > + >> > + socket = cpu_to_socket(cpu); >> > + info = socket_info + socket; >> > + if ( feat_init_done(info) ) >> > + goto assoc_init; >> >> Hmm, so you bail here if any of the features was already set up. >> But you don't bail if none of the features were available as the >> reason for the setup not having been done before. I think this >> can be solved in a better way once we have the static props >> array: You need to do anything here only if the props slot is not >> NULL, but the feature slot is NULL. >> > As above comment, this check will be changed to check a flag if you have > no opinion. But, what is your conern here? Do you mind the 'goto'? Well, while with the intended flag introduction this discussion is mostly moot now - no, it's not the goto, it's the way of checking done. >> In any event you intentions are likely easier to understand for >> a reader if this single-use function was inlined here. >> > As you have observed, this 'feat_init_done' is used many times in later > patches. And (again mostly moot now) as expressed there, the further uses appear to want checks different from the one here. Jan
On 17-04-06 02:32:04, Jan Beulich wrote: > >>> On 06.04.17 at 07:49, <yi.y.sun@linux.intel.com> wrote: > > On 17-04-05 09:10:58, Jan Beulich wrote: > >> >>> On 01.04.17 at 15:53, <yi.y.sun@linux.intel.com> wrote: > >> > +static void free_socket_resources(unsigned int socket) > >> > +{ > >> > + unsigned int i; > >> > + struct psr_socket_info *info = socket_info + socket; > >> > + > >> > + if ( !info ) > >> > + return; > >> > + > >> > + /* > >> > + * Free resources of features. The global feature object, e.g. feat_l3_cat, > >> > + * may not be freed here if it is not added into array. It is simply being > >> > + * kept until the next CPU online attempt. > >> > + */ > >> > + for ( i = 0; i < PSR_SOCKET_MAX_FEAT; i++ ) > >> > + { > >> > + if ( !info->features[i] ) > >> > + continue; > >> > + > >> > + xfree(info->features[i]); > >> > + info->features[i] = NULL; > >> > >> There's no need for the if() here. And I'm sure this was pointed out > >> already (perhaps in a different context). > >> > > There may be NULL member in features array. Features array contains all > > features, including L3 CAT, CDP and L2 CAT. But on some machines, they > > may only support L3 CAT but do not support CDP and L2 CAT. So, the features > > array only has L3 CAT member in it and all other members are all NULL. That > > is the reason we must check if the member is NULL or not. > > No, and this has been explained before: xfree() happily accepts > NULL pointers. > Ok, got it. [...] > >> > +/* CAT common functions implementation. */ > >> > +static void cat_init_feature(const struct cpuid_leaf *regs, > >> > + struct feat_node *feat, > >> > + struct psr_socket_info *info, > >> > + enum psr_feat_type type) > >> > +{ > >> > + unsigned int socket, i; > >> > + > >> > + /* No valid value so do not enable feature. */ > >> > + if ( !regs->a || !regs->d ) > >> > + return; > >> > + > >> > + feat->props->cbm_len = (regs->a & CAT_CBM_LEN_MASK) + 1; > >> > + feat->props->cos_max = min(opt_cos_max, regs->d & CAT_COS_MAX_MASK); > >> > + > >> > + switch ( type ) > >> > + { > >> > + case PSR_SOCKET_L3_CAT: > >> > + /* cos=0 is reserved as default cbm(all bits within cbm_len are 1). */ > >> > + feat->cos_reg_val[0] = cat_default_val(feat->props->cbm_len); > >> > + > >> > + /* > >> > + * To handle cpu offline and then online case, we need restore MSRs to > >> > + * default values. > >> > + */ > >> > + for ( i = 1; i <= feat->props->cos_max; i++ ) > >> > + { > >> > + wrmsrl(MSR_IA32_PSR_L3_MASK(i), feat->cos_reg_val[0]); > >> > + feat->cos_reg_val[i] = feat->cos_reg_val[0]; > >> > + } > >> > >> I continue to have difficulty with this: Why is offline-then-online > >> any different from first-time-online? Why wouldn't setting the > > > > May remove this comment. Per current codes, the MSRs are written to default > > values no matter first time or not. > > > >> registers to their intended values not be taken care of by > >> context switch code, once vCPU-s get scheduled onto the newly > >> onlined CPU? > >> > > cat_init_feature is only called when the first CPU on a socket is online. > > The MSRs to set are per socket. So, we only need set it once when socket > > is online. > > This does not answer my question. Once again - why does this need > doing here explicitly, rather than relying on the needed values being > loaded when the first vCPU gets scheduled onto one of the pCPU-s > of this socket? > I do not know if I understand your question correctly. Let me try to explain again. As we discussed in v9, the MSRs values may be wrong values when socket is online. That is the reason we have to restore them. The MSRs are per socket. That means only one group of MSRs on one socket. So the setting on one CPU can make it valid on whole socket. The 'cat_init_feature' is executed when the first CPU on a socket is online so we restore them here. [...]
>>> On 06.04.17 at 11:22, <yi.y.sun@linux.intel.com> wrote: > On 17-04-06 02:32:04, Jan Beulich wrote: >> >>> On 06.04.17 at 07:49, <yi.y.sun@linux.intel.com> wrote: >> > On 17-04-05 09:10:58, Jan Beulich wrote: >> >> >>> On 01.04.17 at 15:53, <yi.y.sun@linux.intel.com> wrote: >> >> > +static void cat_init_feature(const struct cpuid_leaf *regs, >> >> > + struct feat_node *feat, >> >> > + struct psr_socket_info *info, >> >> > + enum psr_feat_type type) >> >> > +{ >> >> > + unsigned int socket, i; >> >> > + >> >> > + /* No valid value so do not enable feature. */ >> >> > + if ( !regs->a || !regs->d ) >> >> > + return; >> >> > + >> >> > + feat->props->cbm_len = (regs->a & CAT_CBM_LEN_MASK) + 1; >> >> > + feat->props->cos_max = min(opt_cos_max, regs->d & CAT_COS_MAX_MASK); >> >> > + >> >> > + switch ( type ) >> >> > + { >> >> > + case PSR_SOCKET_L3_CAT: >> >> > + /* cos=0 is reserved as default cbm(all bits within cbm_len are 1). */ >> >> > + feat->cos_reg_val[0] = cat_default_val(feat->props->cbm_len); >> >> > + >> >> > + /* >> >> > + * To handle cpu offline and then online case, we need restore MSRs to >> >> > + * default values. >> >> > + */ >> >> > + for ( i = 1; i <= feat->props->cos_max; i++ ) >> >> > + { >> >> > + wrmsrl(MSR_IA32_PSR_L3_MASK(i), feat->cos_reg_val[0]); >> >> > + feat->cos_reg_val[i] = feat->cos_reg_val[0]; >> >> > + } >> >> >> >> I continue to have difficulty with this: Why is offline-then-online >> >> any different from first-time-online? Why wouldn't setting the >> > >> > May remove this comment. Per current codes, the MSRs are written to default >> > values no matter first time or not. >> > >> >> registers to their intended values not be taken care of by >> >> context switch code, once vCPU-s get scheduled onto the newly >> >> onlined CPU? >> >> >> > cat_init_feature is only called when the first CPU on a socket is online. >> > The MSRs to set are per socket. So, we only need set it once when socket >> > is online. >> >> This does not answer my question. Once again - why does this need >> doing here explicitly, rather than relying on the needed values being >> loaded when the first vCPU gets scheduled onto one of the pCPU-s >> of this socket? >> > I do not know if I understand your question correctly. Let me try to explain > again. As we discussed in v9, the MSRs values may be wrong values when socket > is online. That is the reason we have to restore them. > > The MSRs are per socket. That means only one group of MSRs on one socket. So > the setting on one CPU can make it valid on whole socket. The 'cat_init_feature' > is executed when the first CPU on a socket is online so we restore them here. All understood. But you write the MSRs with the needed values in the context switch path, don't you? Why is that writing not sufficient? Jan
On 17-04-06 03:34:27, Jan Beulich wrote: > >>> On 06.04.17 at 11:22, <yi.y.sun@linux.intel.com> wrote: > > On 17-04-06 02:32:04, Jan Beulich wrote: > >> >>> On 06.04.17 at 07:49, <yi.y.sun@linux.intel.com> wrote: > >> > On 17-04-05 09:10:58, Jan Beulich wrote: > >> >> >>> On 01.04.17 at 15:53, <yi.y.sun@linux.intel.com> wrote: > >> >> > +static void cat_init_feature(const struct cpuid_leaf *regs, > >> >> > + struct feat_node *feat, > >> >> > + struct psr_socket_info *info, > >> >> > + enum psr_feat_type type) > >> >> > +{ > >> >> > + unsigned int socket, i; > >> >> > + > >> >> > + /* No valid value so do not enable feature. */ > >> >> > + if ( !regs->a || !regs->d ) > >> >> > + return; > >> >> > + > >> >> > + feat->props->cbm_len = (regs->a & CAT_CBM_LEN_MASK) + 1; > >> >> > + feat->props->cos_max = min(opt_cos_max, regs->d & CAT_COS_MAX_MASK); > >> >> > + > >> >> > + switch ( type ) > >> >> > + { > >> >> > + case PSR_SOCKET_L3_CAT: > >> >> > + /* cos=0 is reserved as default cbm(all bits within cbm_len are 1). */ > >> >> > + feat->cos_reg_val[0] = cat_default_val(feat->props->cbm_len); > >> >> > + > >> >> > + /* > >> >> > + * To handle cpu offline and then online case, we need restore MSRs to > >> >> > + * default values. > >> >> > + */ > >> >> > + for ( i = 1; i <= feat->props->cos_max; i++ ) > >> >> > + { > >> >> > + wrmsrl(MSR_IA32_PSR_L3_MASK(i), feat->cos_reg_val[0]); > >> >> > + feat->cos_reg_val[i] = feat->cos_reg_val[0]; > >> >> > + } > >> >> > >> >> I continue to have difficulty with this: Why is offline-then-online > >> >> any different from first-time-online? Why wouldn't setting the > >> > > >> > May remove this comment. Per current codes, the MSRs are written to default > >> > values no matter first time or not. > >> > > >> >> registers to their intended values not be taken care of by > >> >> context switch code, once vCPU-s get scheduled onto the newly > >> >> onlined CPU? > >> >> > >> > cat_init_feature is only called when the first CPU on a socket is online. > >> > The MSRs to set are per socket. So, we only need set it once when socket > >> > is online. > >> > >> This does not answer my question. Once again - why does this need > >> doing here explicitly, rather than relying on the needed values being > >> loaded when the first vCPU gets scheduled onto one of the pCPU-s > >> of this socket? > >> > > I do not know if I understand your question correctly. Let me try to explain > > again. As we discussed in v9, the MSRs values may be wrong values when socket > > is online. That is the reason we have to restore them. > > > > The MSRs are per socket. That means only one group of MSRs on one socket. So > > the setting on one CPU can make it valid on whole socket. The 'cat_init_feature' > > is executed when the first CPU on a socket is online so we restore them here. > > All understood. But you write the MSRs with the needed values in > the context switch path, don't you? Why is that writing not > sufficient? > No, in context switch path, we only set ASSOC register to set COS ID into it so that the corresponding COS MSR value (CBM) can work. Here, we set default CBM value into COS MSRs. > Jan > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > https://lists.xen.org/xen-devel
>>> On 06.04.17 at 12:02, <yi.y.sun@linux.intel.com> wrote: > On 17-04-06 03:34:27, Jan Beulich wrote: >> >>> On 06.04.17 at 11:22, <yi.y.sun@linux.intel.com> wrote: >> > On 17-04-06 02:32:04, Jan Beulich wrote: >> >> >>> On 06.04.17 at 07:49, <yi.y.sun@linux.intel.com> wrote: >> >> > On 17-04-05 09:10:58, Jan Beulich wrote: >> >> >> >>> On 01.04.17 at 15:53, <yi.y.sun@linux.intel.com> wrote: >> >> >> > +static void cat_init_feature(const struct cpuid_leaf *regs, >> >> >> > + struct feat_node *feat, >> >> >> > + struct psr_socket_info *info, >> >> >> > + enum psr_feat_type type) >> >> >> > +{ >> >> >> > + unsigned int socket, i; >> >> >> > + >> >> >> > + /* No valid value so do not enable feature. */ >> >> >> > + if ( !regs->a || !regs->d ) >> >> >> > + return; >> >> >> > + >> >> >> > + feat->props->cbm_len = (regs->a & CAT_CBM_LEN_MASK) + 1; >> >> >> > + feat->props->cos_max = min(opt_cos_max, regs->d & CAT_COS_MAX_MASK); >> >> >> > + >> >> >> > + switch ( type ) >> >> >> > + { >> >> >> > + case PSR_SOCKET_L3_CAT: >> >> >> > + /* cos=0 is reserved as default cbm(all bits within cbm_len are 1). */ >> >> >> > + feat->cos_reg_val[0] = cat_default_val(feat->props->cbm_len); >> >> >> > + >> >> >> > + /* >> >> >> > + * To handle cpu offline and then online case, we need restore MSRs to >> >> >> > + * default values. >> >> >> > + */ >> >> >> > + for ( i = 1; i <= feat->props->cos_max; i++ ) >> >> >> > + { >> >> >> > + wrmsrl(MSR_IA32_PSR_L3_MASK(i), feat->cos_reg_val[0]); >> >> >> > + feat->cos_reg_val[i] = feat->cos_reg_val[0]; >> >> >> > + } >> >> >> >> >> >> I continue to have difficulty with this: Why is offline-then-online >> >> >> any different from first-time-online? Why wouldn't setting the >> >> > >> >> > May remove this comment. Per current codes, the MSRs are written to default >> >> > values no matter first time or not. >> >> > >> >> >> registers to their intended values not be taken care of by >> >> >> context switch code, once vCPU-s get scheduled onto the newly >> >> >> onlined CPU? >> >> >> >> >> > cat_init_feature is only called when the first CPU on a socket is online. >> >> > The MSRs to set are per socket. So, we only need set it once when socket >> >> > is online. >> >> >> >> This does not answer my question. Once again - why does this need >> >> doing here explicitly, rather than relying on the needed values being >> >> loaded when the first vCPU gets scheduled onto one of the pCPU-s >> >> of this socket? >> >> >> > I do not know if I understand your question correctly. Let me try to explain >> > again. As we discussed in v9, the MSRs values may be wrong values when socket >> > is online. That is the reason we have to restore them. >> > >> > The MSRs are per socket. That means only one group of MSRs on one socket. So >> > the setting on one CPU can make it valid on whole socket. The 'cat_init_feature' >> > is executed when the first CPU on a socket is online so we restore them here. >> >> All understood. But you write the MSRs with the needed values in >> the context switch path, don't you? Why is that writing not >> sufficient? >> > No, in context switch path, we only set ASSOC register to set COS ID into it so > that the corresponding COS MSR value (CBM) can work. Okay, so not the context switch path then, But you must be changing the MSRs _somewhere_, and the question is why this somewhere isn't sufficient. Jan
On 17-04-06 08:02:15, Jan Beulich wrote: > >>> On 06.04.17 at 12:02, <yi.y.sun@linux.intel.com> wrote: > > On 17-04-06 03:34:27, Jan Beulich wrote: > >> >>> On 06.04.17 at 11:22, <yi.y.sun@linux.intel.com> wrote: > >> > On 17-04-06 02:32:04, Jan Beulich wrote: > >> >> >>> On 06.04.17 at 07:49, <yi.y.sun@linux.intel.com> wrote: > >> >> > On 17-04-05 09:10:58, Jan Beulich wrote: > >> >> >> >>> On 01.04.17 at 15:53, <yi.y.sun@linux.intel.com> wrote: > >> >> >> > +static void cat_init_feature(const struct cpuid_leaf *regs, > >> >> >> > + struct feat_node *feat, > >> >> >> > + struct psr_socket_info *info, > >> >> >> > + enum psr_feat_type type) > >> >> >> > +{ > >> >> >> > + unsigned int socket, i; > >> >> >> > + > >> >> >> > + /* No valid value so do not enable feature. */ > >> >> >> > + if ( !regs->a || !regs->d ) > >> >> >> > + return; > >> >> >> > + > >> >> >> > + feat->props->cbm_len = (regs->a & CAT_CBM_LEN_MASK) + 1; > >> >> >> > + feat->props->cos_max = min(opt_cos_max, regs->d & CAT_COS_MAX_MASK); > >> >> >> > + > >> >> >> > + switch ( type ) > >> >> >> > + { > >> >> >> > + case PSR_SOCKET_L3_CAT: > >> >> >> > + /* cos=0 is reserved as default cbm(all bits within cbm_len are 1). */ > >> >> >> > + feat->cos_reg_val[0] = cat_default_val(feat->props->cbm_len); > >> >> >> > + > >> >> >> > + /* > >> >> >> > + * To handle cpu offline and then online case, we need restore MSRs to > >> >> >> > + * default values. > >> >> >> > + */ > >> >> >> > + for ( i = 1; i <= feat->props->cos_max; i++ ) > >> >> >> > + { > >> >> >> > + wrmsrl(MSR_IA32_PSR_L3_MASK(i), feat->cos_reg_val[0]); > >> >> >> > + feat->cos_reg_val[i] = feat->cos_reg_val[0]; > >> >> >> > + } > >> >> >> > >> >> >> I continue to have difficulty with this: Why is offline-then-online > >> >> >> any different from first-time-online? Why wouldn't setting the > >> >> > > >> >> > May remove this comment. Per current codes, the MSRs are written to default > >> >> > values no matter first time or not. > >> >> > > >> >> >> registers to their intended values not be taken care of by > >> >> >> context switch code, once vCPU-s get scheduled onto the newly > >> >> >> onlined CPU? > >> >> >> > >> >> > cat_init_feature is only called when the first CPU on a socket is online. > >> >> > The MSRs to set are per socket. So, we only need set it once when socket > >> >> > is online. > >> >> > >> >> This does not answer my question. Once again - why does this need > >> >> doing here explicitly, rather than relying on the needed values being > >> >> loaded when the first vCPU gets scheduled onto one of the pCPU-s > >> >> of this socket? > >> >> > >> > I do not know if I understand your question correctly. Let me try to explain > >> > again. As we discussed in v9, the MSRs values may be wrong values when socket > >> > is online. That is the reason we have to restore them. > >> > > >> > The MSRs are per socket. That means only one group of MSRs on one socket. So > >> > the setting on one CPU can make it valid on whole socket. The 'cat_init_feature' > >> > is executed when the first CPU on a socket is online so we restore them here. > >> > >> All understood. But you write the MSRs with the needed values in > >> the context switch path, don't you? Why is that writing not > >> sufficient? > >> > > No, in context switch path, we only set ASSOC register to set COS ID into it so > > that the corresponding COS MSR value (CBM) can work. > > Okay, so not the context switch path then, But you must be > changing the MSRs _somewhere_, and the question is why this > somewhere isn't sufficient. > Besides the restore behavior in init process, I restore the MSRs when ref[cos] is reduced to 0. This behavior happens in two scenarios: 1. In a value setting process, restore MSR if the ref[old_cos] is reduced to 0. 2. When a domain is destroyed, its ref[cos] is reduced too. Reason to restore is below: For features, e.g. CDP, which cos_num is more than 1, we have to restore the old_cos value back to default when ref[old_cos] is 0. Otherwise, user will see wrong values when this COS ID is reused. E.g. user wants to set DATA to 0x3ff for a new domain. He hopes to see the DATA is set to 0x3ff and CODE should be the default value, 0x7ff. But if the COS ID picked for this action is the one that has been used by other domain and the CODE has been set to 0x1ff. Then, user will see DATA: 0x3ff, CODE: 0x1ff. So, we have to restore COS values for features using multiple COSs. BRs, Sun Yi
>>> On 07.04.17 at 07:17, <yi.y.sun@linux.intel.com> wrote: > On 17-04-06 08:02:15, Jan Beulich wrote: >> Okay, so not the context switch path then, But you must be >> changing the MSRs _somewhere_, and the question is why this >> somewhere isn't sufficient. >> > Besides the restore behavior in init process, I restore the MSRs when > ref[cos] > is reduced to 0. This behavior happens in two scenarios: > 1. In a value setting process, restore MSR if the ref[old_cos] is reduced to 0. > 2. When a domain is destroyed, its ref[cos] is reduced too. > > Reason to restore is below: > For features, e.g. CDP, which cos_num is more than 1, we have to > restore the old_cos value back to default when ref[old_cos] is 0. > Otherwise, user will see wrong values when this COS ID is reused. E.g. > user wants to set DATA to 0x3ff for a new domain. He hopes to see the > DATA is set to 0x3ff and CODE should be the default value, 0x7ff. But > if the COS ID picked for this action is the one that has been used by > other domain and the CODE has been set to 0x1ff. Then, user will see > DATA: 0x3ff, CODE: 0x1ff. So, we have to restore COS values for features > using multiple COSs. I still don't feel my question being answered. Without a COS ever allocated on a socket, how can values in MSRs other than the first (index 0) one be used by anyone? I ask because if they can't be used, their values don't matter (all you need to make sure is to write them regardless of their currently cached value). If syncing MSR and cached values at init time is to make sure those writes won#t be bypassed, that would be a legitimate explanation (if the alternatives, like introducing a separate flag, would be overall more expensive or uglier). But the reason(s) need(s) to be properly explained (and by "properly" I don't mean a _long_ code comment, but a _precise_ one). Jan
On 17-04-07 02:48:40, Jan Beulich wrote: > >>> On 07.04.17 at 07:17, <yi.y.sun@linux.intel.com> wrote: > > On 17-04-06 08:02:15, Jan Beulich wrote: > >> Okay, so not the context switch path then, But you must be > >> changing the MSRs _somewhere_, and the question is why this > >> somewhere isn't sufficient. > >> > > Besides the restore behavior in init process, I restore the MSRs when > > ref[cos] > > is reduced to 0. This behavior happens in two scenarios: > > 1. In a value setting process, restore MSR if the ref[old_cos] is reduced to 0. > > 2. When a domain is destroyed, its ref[cos] is reduced too. > > > > Reason to restore is below: > > For features, e.g. CDP, which cos_num is more than 1, we have to > > restore the old_cos value back to default when ref[old_cos] is 0. > > Otherwise, user will see wrong values when this COS ID is reused. E.g. > > user wants to set DATA to 0x3ff for a new domain. He hopes to see the > > DATA is set to 0x3ff and CODE should be the default value, 0x7ff. But > > if the COS ID picked for this action is the one that has been used by > > other domain and the CODE has been set to 0x1ff. Then, user will see > > DATA: 0x3ff, CODE: 0x1ff. So, we have to restore COS values for features > > using multiple COSs. > > I still don't feel my question being answered. Without a COS > ever allocated on a socket, how can values in MSRs other than > the first (index 0) one be used by anyone? I ask because if they > can't be used, their values don't matter (all you need to make > sure is to write them regardless of their currently cached value). The COS ID using is managed by domain (d->arch.psr_cos_ids[socket]). Even if a socket is offline, the COS ID saved in domain is still valid (domain is alive). When this socket is online again, the domain may be scheduled onto it to run. Then, the COS ID (e.g 2) will be used to get/set value for this domain. If we don't restore MSRs on the socket, we may get an unknown value. This the reason we have to restore MSRs in 'cat_init_feature' which is called when socket is online. Per explanation above (in previous mail), we have to restore MSRs when ref[cos] is reduced to 0. Those are all sencarios and reasons to restore MSRs. I don't know if the explanations are precise enough. Any unclear, please let me know. Thanks! > If syncing MSR and cached values at init time is to make sure > those writes won#t be bypassed, that would be a legitimate > explanation (if the alternatives, like introducing a separate flag, > would be overall more expensive or uglier). But the reason(s) > need(s) to be properly explained (and by "properly" I don't > mean a _long_ code comment, but a _precise_ one). > > Jan > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > https://lists.xen.org/xen-devel
>>> On 07.04.17 at 11:08, <yi.y.sun@linux.intel.com> wrote: > On 17-04-07 02:48:40, Jan Beulich wrote: >> >>> On 07.04.17 at 07:17, <yi.y.sun@linux.intel.com> wrote: >> > On 17-04-06 08:02:15, Jan Beulich wrote: >> >> Okay, so not the context switch path then, But you must be >> >> changing the MSRs _somewhere_, and the question is why this >> >> somewhere isn't sufficient. >> >> >> > Besides the restore behavior in init process, I restore the MSRs when >> > ref[cos] >> > is reduced to 0. This behavior happens in two scenarios: >> > 1. In a value setting process, restore MSR if the ref[old_cos] is reduced to 0. >> > 2. When a domain is destroyed, its ref[cos] is reduced too. >> > >> > Reason to restore is below: >> > For features, e.g. CDP, which cos_num is more than 1, we have to >> > restore the old_cos value back to default when ref[old_cos] is 0. >> > Otherwise, user will see wrong values when this COS ID is reused. E.g. >> > user wants to set DATA to 0x3ff for a new domain. He hopes to see the >> > DATA is set to 0x3ff and CODE should be the default value, 0x7ff. But >> > if the COS ID picked for this action is the one that has been used by >> > other domain and the CODE has been set to 0x1ff. Then, user will see >> > DATA: 0x3ff, CODE: 0x1ff. So, we have to restore COS values for features >> > using multiple COSs. >> >> I still don't feel my question being answered. Without a COS >> ever allocated on a socket, how can values in MSRs other than >> the first (index 0) one be used by anyone? I ask because if they >> can't be used, their values don't matter (all you need to make >> sure is to write them regardless of their currently cached value). > > The COS ID using is managed by domain (d->arch.psr_cos_ids[socket]). Even if a > socket is offline, the COS ID saved in domain is still valid (domain is alive). > When this socket is online again, the domain may be scheduled onto it to run. > Then, the COS ID (e.g 2) will be used to get/set value for this domain. If we > don't restore MSRs on the socket, we may get an unknown value. This the reason > we have to restore MSRs in 'cat_init_feature' which is called when socket is > online. No, it's not. At least that's not the only way to solve the problem: As said, you could equally well make sure you always write the MSR the first time a vCPU using a particular COS is being scheduled onto the newly onlined pCPU. In fact most of the time it is better if generic code can also deal with special cases than to introduce special purpose code. Hence my insisting on you properly explaining why generic code can't deal with the post-online situation here. Jan
On 17-04-07 03:46:42, Jan Beulich wrote: > >>> On 07.04.17 at 11:08, <yi.y.sun@linux.intel.com> wrote: > > On 17-04-07 02:48:40, Jan Beulich wrote: > >> >>> On 07.04.17 at 07:17, <yi.y.sun@linux.intel.com> wrote: > >> > On 17-04-06 08:02:15, Jan Beulich wrote: > >> >> Okay, so not the context switch path then, But you must be > >> >> changing the MSRs _somewhere_, and the question is why this > >> >> somewhere isn't sufficient. > >> >> > >> > Besides the restore behavior in init process, I restore the MSRs when > >> > ref[cos] > >> > is reduced to 0. This behavior happens in two scenarios: > >> > 1. In a value setting process, restore MSR if the ref[old_cos] is reduced to 0. > >> > 2. When a domain is destroyed, its ref[cos] is reduced too. > >> > > >> > Reason to restore is below: > >> > For features, e.g. CDP, which cos_num is more than 1, we have to > >> > restore the old_cos value back to default when ref[old_cos] is 0. > >> > Otherwise, user will see wrong values when this COS ID is reused. E.g. > >> > user wants to set DATA to 0x3ff for a new domain. He hopes to see the > >> > DATA is set to 0x3ff and CODE should be the default value, 0x7ff. But > >> > if the COS ID picked for this action is the one that has been used by > >> > other domain and the CODE has been set to 0x1ff. Then, user will see > >> > DATA: 0x3ff, CODE: 0x1ff. So, we have to restore COS values for features > >> > using multiple COSs. > >> > >> I still don't feel my question being answered. Without a COS > >> ever allocated on a socket, how can values in MSRs other than > >> the first (index 0) one be used by anyone? I ask because if they > >> can't be used, their values don't matter (all you need to make > >> sure is to write them regardless of their currently cached value). > > > > The COS ID using is managed by domain (d->arch.psr_cos_ids[socket]). Even if a > > socket is offline, the COS ID saved in domain is still valid (domain is alive). > > When this socket is online again, the domain may be scheduled onto it to run. > > Then, the COS ID (e.g 2) will be used to get/set value for this domain. If we > > don't restore MSRs on the socket, we may get an unknown value. This the reason > > we have to restore MSRs in 'cat_init_feature' which is called when socket is > > online. > > No, it's not. At least that's not the only way to solve the problem: > As said, you could equally well make sure you always write the MSR > the first time a vCPU using a particular COS is being scheduled onto > the newly onlined pCPU. In fact most of the time it is better if > generic code can also deal with special cases than to introduce > special purpose code. Hence my insisting on you properly explaining > why generic code can't deal with the post-online situation here. > Then, I propose another simple solution to handle this case. When writing MSRs, check if all values in val_array are same as old values (kept in 'cos_reg_val[]'). If a value is different, the MSR will be written and the corresponding member in 'cos_reg_val[]' will be updated. Still use above sample, user wants to set DATA to 0x3ff for a new domain. After value array assembling, the val_array will be val[DATA]=0x3ff, val[CODE]=0x7ff. The cos_reg_val[DATA]=0x7ff and cos_reg_val[DATA]=0x1ff. So, both of them will be updated. > Jan
On 17-04-10 11:27:04, Yi Sun wrote: > On 17-04-07 03:46:42, Jan Beulich wrote: > > >>> On 07.04.17 at 11:08, <yi.y.sun@linux.intel.com> wrote: > > > On 17-04-07 02:48:40, Jan Beulich wrote: > > >> >>> On 07.04.17 at 07:17, <yi.y.sun@linux.intel.com> wrote: > > >> > On 17-04-06 08:02:15, Jan Beulich wrote: > > >> >> Okay, so not the context switch path then, But you must be > > >> >> changing the MSRs _somewhere_, and the question is why this > > >> >> somewhere isn't sufficient. > > >> >> > > >> > Besides the restore behavior in init process, I restore the MSRs when > > >> > ref[cos] > > >> > is reduced to 0. This behavior happens in two scenarios: > > >> > 1. In a value setting process, restore MSR if the ref[old_cos] is reduced to 0. > > >> > 2. When a domain is destroyed, its ref[cos] is reduced too. > > >> > > > >> > Reason to restore is below: > > >> > For features, e.g. CDP, which cos_num is more than 1, we have to > > >> > restore the old_cos value back to default when ref[old_cos] is 0. > > >> > Otherwise, user will see wrong values when this COS ID is reused. E.g. > > >> > user wants to set DATA to 0x3ff for a new domain. He hopes to see the > > >> > DATA is set to 0x3ff and CODE should be the default value, 0x7ff. But > > >> > if the COS ID picked for this action is the one that has been used by > > >> > other domain and the CODE has been set to 0x1ff. Then, user will see > > >> > DATA: 0x3ff, CODE: 0x1ff. So, we have to restore COS values for features > > >> > using multiple COSs. > > >> > > >> I still don't feel my question being answered. Without a COS > > >> ever allocated on a socket, how can values in MSRs other than > > >> the first (index 0) one be used by anyone? I ask because if they > > >> can't be used, their values don't matter (all you need to make > > >> sure is to write them regardless of their currently cached value). > > > > > > The COS ID using is managed by domain (d->arch.psr_cos_ids[socket]). Even if a > > > socket is offline, the COS ID saved in domain is still valid (domain is alive). > > > When this socket is online again, the domain may be scheduled onto it to run. > > > Then, the COS ID (e.g 2) will be used to get/set value for this domain. If we > > > don't restore MSRs on the socket, we may get an unknown value. This the reason > > > we have to restore MSRs in 'cat_init_feature' which is called when socket is > > > online. > > > > No, it's not. At least that's not the only way to solve the problem: > > As said, you could equally well make sure you always write the MSR > > the first time a vCPU using a particular COS is being scheduled onto > > the newly onlined pCPU. In fact most of the time it is better if > > generic code can also deal with special cases than to introduce > > special purpose code. Hence my insisting on you properly explaining > > why generic code can't deal with the post-online situation here. > > > Then, I propose another simple solution to handle this case. When writing > MSRs, check if all values in val_array are same as old values (kept in > 'cos_reg_val[]'). If a value is different, the MSR will be written and the > corresponding member in 'cos_reg_val[]' will be updated. Still use above > sample, user wants to set DATA to 0x3ff for a new domain. After value array > assembling, the val_array will be val[DATA]=0x3ff, val[CODE]=0x7ff. The > cos_reg_val[DATA]=0x7ff and cos_reg_val[DATA]=0x1ff. So, both of them will > be updated. > I want to explain more to make it clearer. The original codes are to solve the issues when a COS ID is free and reused by other domain. With the newly proposed method above, we can solve this issue too. E.g: 1. COS ID 1 is used by domain 1. Its CODE=0x1ff, DATA=0x7ff. They are kept in cos_reg_val[1 - CODE] and cos_reg_val[1 - DATA]. 2. For some reason, COS ID 1 is free. 3. We want to set DATA of a new domain (8) to 0x3ff. Its old_cos is 0 so that val_array assembled is CODE=0x7ff, DATA=0x3ff. Then COS ID 1 is picked. 4. In write flow, iterate the feature's type array according to cos_num to set both CODE and DATA if val_array member is different with cos_reg_val member. Below are codes to explain it clearly: static void l3_cdp_write_msr(unsigned int cos, uint32_t val, ...) { /* Data. For above case, get_cdp_data(..., cos) returns 0x7ff */ if ( type == PSR_CBM_TYPE_L3_DATA && get_cdp_data(feat, cos) != val ) { get_cdp_data(feat, cos) = val; wrmsrl(MSR_IA32_PSR_L3_MASK_DATA(cos), val); } /* Code. For above case, get_cdp_code(..., cos) returns 0x1ff */ if ( type == PSR_CBM_TYPE_L3_CODE && get_cdp_code(feat, cos) != val ) { get_cdp_code(feat, cos) = val; wrmsrl(MSR_IA32_PSR_L3_MASK_CODE(cos), val); } } struct cos_write_info { unsigned int cos; struct feat_node *feature; /* All COS MSRs values of the feature. */ uint32_t *val; unsigned int array_len; enum psr_feat_type feat_type; }; static void do_write_psr_msr(void *data) { ... /* Write all COS MSRs of the feature. */ for ( i = 0; i < props[info->feat_type]->cos_num; i++ ) props[info->feat_type]->write_msr(cos, info->val[i], props[info->feat_type]->type[i], feat); } static int write_psr_msr(unsigned int socket, unsigned int cos, ...) { ... /* Skip to the feature's value head. */ for ( i = 0; i < feat_type; i++ ) { if ( !info->features[i] ) continue; if ( array_len <= props[i].cos_num ) return -ENOSPC; array_len -= props[i].cos_num; val += props[i].cos_num; } data.val = val; data.array_len = array_len; ... } int psr_set_val(struct domain *d, unsigned int socket, ...) { ... /* Gather and insert value array. Array should be val[DATA]=0x3ff, val[CODE]=0x7ff. */ gather_val_array(val_array,...); insert_val_to_array(val_array,...); /* Input val_array which has all COS MSRs values of all features. */ ret = write_psr_msr(socket, cos, val_array, array_len, feat_type); ... } > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > https://lists.xen.org/xen-devel
diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c index cf352d2..e422a23 100644 --- a/xen/arch/x86/psr.c +++ b/xen/arch/x86/psr.c @@ -34,6 +34,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 @@ -76,7 +79,7 @@ struct feat_node { * * Feature independent HW info and common values are also defined in it. */ - const struct feat_props { + struct feat_props { /* * cos_num, cos_max and cbm_len are common values for all features * so far. @@ -114,11 +117,124 @@ 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 node for every feature to facilitate the feature + * array creation. It is used to transiently store a spare node. + */ +static struct feat_node *feat_l3_cat; + +/* Common functions */ +#define cat_default_val(len) (0xffffffff >> (32 - (len))) + +/* + * Use this function to check if any allocation feature has been enabled + * in cmdline. + */ +static bool psr_alloc_feat_enabled(void) +{ + return !!socket_info; +} + +static void free_socket_resources(unsigned int socket) +{ + unsigned int i; + struct psr_socket_info *info = socket_info + socket; + + if ( !info ) + return; + + /* + * Free resources of features. The global feature object, e.g. feat_l3_cat, + * may not be freed here if it is not added into array. It is simply being + * kept until the next CPU online attempt. + */ + for ( i = 0; i < PSR_SOCKET_MAX_FEAT; i++ ) + { + if ( !info->features[i] ) + continue; + + xfree(info->features[i]); + info->features[i] = NULL; + } +} + +static bool feat_init_done(const struct psr_socket_info *info) +{ + unsigned int i; + + for ( i = 0; i < PSR_SOCKET_MAX_FEAT; i++ ) + { + if ( !info->features[i] ) + continue; + + return true; + } + + return false; +} + +/* CAT common functions implementation. */ +static void cat_init_feature(const struct cpuid_leaf *regs, + struct feat_node *feat, + struct psr_socket_info *info, + enum psr_feat_type type) +{ + unsigned int socket, i; + + /* No valid value so do not enable feature. */ + if ( !regs->a || !regs->d ) + return; + + feat->props->cbm_len = (regs->a & CAT_CBM_LEN_MASK) + 1; + feat->props->cos_max = min(opt_cos_max, regs->d & CAT_COS_MAX_MASK); + + switch ( type ) + { + case PSR_SOCKET_L3_CAT: + /* cos=0 is reserved as default cbm(all bits within cbm_len are 1). */ + feat->cos_reg_val[0] = cat_default_val(feat->props->cbm_len); + + /* + * To handle cpu offline and then online case, we need restore MSRs to + * default values. + */ + for ( i = 1; i <= feat->props->cos_max; i++ ) + { + wrmsrl(MSR_IA32_PSR_L3_MASK(i), feat->cos_reg_val[0]); + feat->cos_reg_val[i] = feat->cos_reg_val[0]; + } + + break; + + default: + return; + } + + /* Add this feature into array. */ + info->features[type] = feat; + + socket = cpu_to_socket(smp_processor_id()); + 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"), + socket, feat->props->cos_max, feat->props->cbm_len); +} + +/* L3 CAT ops */ +static struct feat_props l3_cat_props = { + .cos_num = 1, +}; + static void __init parse_psr_bool(char *s, char *value, char *feature, unsigned int mask) { @@ -158,6 +274,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 ); } @@ -313,19 +432,95 @@ void psr_domain_free(struct domain *d) psr_free_rmid(d); } -static int psr_cpu_prepare(unsigned int cpu) +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 ( !psr_alloc_feat_enabled() ) + return 0; + + /* Malloc memory for the global feature node here. */ + if ( feat_l3_cat == NULL && + (feat_l3_cat = xzalloc(struct feat_node)) == NULL ) + return -ENOMEM; + return 0; } static void psr_cpu_init(void) { + struct psr_socket_info *info; + unsigned int socket; + unsigned int cpu = smp_processor_id(); + struct feat_node *feat; + struct cpuid_leaf regs; + + if ( !psr_alloc_feat_enabled() || !boot_cpu_has(X86_FEATURE_PQE) ) + goto assoc_init; + + if ( boot_cpu_data.cpuid_level < PSR_CPUID_LEVEL_CAT ) + { + setup_clear_cpu_cap(X86_FEATURE_PQE); + goto assoc_init; + } + + socket = cpu_to_socket(cpu); + info = socket_info + socket; + if ( feat_init_done(info) ) + goto assoc_init; + + 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; + feat_l3_cat = NULL; + feat->props = &l3_cat_props; + + cat_init_feature(®s, feat, info, PSR_SOCKET_L3_CAT); + } + + assoc_init: psr_assoc_init(); } static void psr_cpu_fini(unsigned int cpu) { - return; + unsigned int socket = cpu_to_socket(cpu); + + if ( !psr_alloc_feat_enabled() ) + return; + + /* + * 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_socket_resources(socket); } static int cpu_callback( @@ -337,7 +532,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(); @@ -366,10 +561,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() || psr_alloc_feat_enabled() ) register_cpu_notifier(&cpu_nfb); return 0;
This patch implements the CPU init and free flow including L3 CAT initialization and some resources free. It includes below flows: 1. presmp init: - parse command line parameter. - allocate socket info for every socket. - allocate feature resource. - initialize socket info, get feature info and add feature into feature array per cpuid result. - free resources allocated if error happens. - register cpu notifier to handle cpu events. 2. cpu notifier: - handle cpu online events, if initialization work has been done before, do nothing. - handle cpu offline events, if it is the last cpu offline, free some socket resources. Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com> --- v10: - remove 'asm/x86_emulate.h' inclusion as it has been indirectly included. (suggested by Jan Beulich) - remove 'CAT_COS_NUM' as it is only used once. (suggested by Jan Beulich) - remove 'feat_mask'. (suggested by Jan Beulich) - changes about 'feat_props'. (suggested by Jan Beulich) - remove 'get_cos_max' hook declaration. (suggested by Jan Beulich) - modify 'cat_default_val' implementation. (suggested by Jan Beulich) - modify 'psr_alloc_feat_enabled' implementation to make it simple. (suggested by Jan Beulich) - rename 'free_feature' to 'free_socket_resources' because it is executed when socket is offline. It needs free resources related to the socket. (suggested by Jan Beulich) - define 'feat_init_done' to iterate feature array to check if any feature has been initialized. (suggested by Jan Beulich) - input 'struct cpuid_leaf' pointer into 'cat_init_feature' to avoid memory copy. (suggested by Jan Beulich) - modify 'cat_init_feature' to use switch and things related to above changes. (suggested by Jan Beulich) - add an indentation for label. (suggested by Jan Beulich) v9: - add commit message to explain the flows. - 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. - create a new patch about moving 'cpuid_count_leaf'. (suggested by Wei Liu) - modify comment to explain why not free some resource in 'free_feature'. (suggested by Wei Liu) - implement 'psr_alloc_feat_enabled' to check if allocation feature is enabled in cmdline and some initialization work done. (suggested by Wei Liu) - implement 'cat_default_val' to set default value for CAT features. (suggested by Wei Liu) - replace feature list handling to feature array handling. (suggested by Roger Pau) - implement a common 'cat_init_feature' to replace L3 CAT/L2 CAT specific init functions. (suggested by Roger Pau) - modify comments for global feature node. (suggested by Jan Beulich) - remove unnecessary comments. (suggested by Jan Beulich) - remove unnecessary 'else'. (suggested by Jan Beulich) - remove 'nr_feat'. (suggested by Jan Beulich) - modify patch title to indicate 'L3 CAT'. (suggested by Jan Beulich) - check global flag with boot cpu operations. (suggested by Jan Beulich) - remove 'cpu_init_work' and move codes into 'psr_cpu_init'. (suggested by Jan Beulich) - remove 'cpu_fini_work' and move codes into 'psr_cpu_fini'. (suggested by Jan Beulich) - assign value for 'cos_num'. (suggested by Jan Beulich) - change about 'uint64_t' to 'uint32_t'. (suggested by Jan Beulich) v8: - fix format issue. (suggested by Konrad Rzeszutek Wilk) - add comments to explain why we care about cpumask_empty when the last cpu on socket is offline. (suggested by Konrad Rzeszutek Wilk) v7: - initialize structure objects for avoiding surprise. (suggested by Konrad Rzeszutek Wilk) - fix typo. (suggested by Konrad Rzeszutek Wilk) - fix a logical mistake when handling the last cpu offline event. (suggested by Konrad Rzeszutek Wilk) v6: - use 'struct cpuid_leaf' introduced in Andrew's patch. (suggested by Konrad Rzeszutek Wilk) - add comments about cpu_add_remove_lock. (suggested by Konrad Rzeszutek Wilk) - change 'clear_bit' to '__clear_bit'. (suggested by Konrad Rzeszutek Wilk) - add 'ASSERT' check when setting 'feat_mask'. (suggested by Konrad Rzeszutek Wilk) - adjust 'printk' position to avoid odd spacing. (suggested by Konrad Rzeszutek Wilk) - add comment to explain usage of 'feat_l3_cat'. (suggested by Konrad Rzeszutek Wilk) - fix wording. (suggested by Konrad Rzeszutek Wilk) - move 'cpuid_count_leaf' helper function to 'asm-x86/processor.h'. It cannot be moved to 'cpuid.h' which causes compilation error because of header file loop reference. (suggested by Andrew Cooper) v5: - add comment to explain the reason to define 'feat_l3_cat'. (suggested by Jan Beulich) - use 'list_for_each_entry_safe'. (suggested by Jan Beulich) - remove codes to free 'feat_l3_cat' in 'free_feature' to avoid the need for an allocation the next time a CPU comes online. (suggested by Jan Beulich) - define 'struct cpuid_leaf_regs' to encapsulate eax~edx. (suggested by Jan Beulich) - print feature info on a socket only when 'opt_cpu_info' is true. (suggested by Jan Beulich) - declare global variable 'l3_cat_ops' to 'static const'. (suggested by Jan Beulich) - use 'current_cpu_data'. (suggested by Jan Beulich) - rename 'feat_tmp' to 'feat'. (suggested by Jan Beulich) - clear PQE feature bit when the maximum CPUID level is too low. (suggested by Jan Beulich) - directly call 'l3_cat_init_feature'. No need to make it a callback function. (suggested by Jan Beulich) - remove local variable 'info'. (suggested by Jan Beulich) - move 'INIT_LIST_HEAD' into 'cpu_init_work' to be together with spin_lock_init(). (suggested by Jan Beulich) - remove 'cpu_prepare_work' and move its content into 'psr_cpu_prepare'. (suggested by Jan Beulich) v4: - create this patch because of removing all CAT/CDP old codes to make implementation be more easily understood. (suggested by Jan Beulich) --- xen/arch/x86/psr.c | 211 +++++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 205 insertions(+), 6 deletions(-)