| Message ID | 1491054836-30488-7-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: > @@ -376,11 +378,39 @@ void psr_free_rmid(struct domain *d) > d->arch.psr_rmid = 0; > } > > -static inline void psr_assoc_init(void) > +static unsigned int get_max_cos_max(const struct psr_socket_info *info) > +{ > + const struct feat_node *feat; > + unsigned int cos_max = 0, i; > + > + for ( i = 0; i < PSR_SOCKET_MAX_FEAT; i++ ) > + { > + feat = info->features[i]; > + if ( !feat ) > + continue; > + > + cos_max = max(feat->props->cos_max, cos_max); > + } > + > + return cos_max; > +} > + > +static void psr_assoc_init(void) > { > struct psr_assoc *psra = &this_cpu(psr_assoc); > > - if ( psr_cmt_enabled() ) > + if ( psr_alloc_feat_enabled() ) > + { > + unsigned int socket = cpu_to_socket(smp_processor_id()); > + const struct psr_socket_info *info = socket_info + socket; > + unsigned int cos_max = get_max_cos_max(info); > + > + if ( feat_init_done(info) ) I think the use here is different from the one in the earlier patch: While there looking at props[] appears to be desirable, I think here you indeed only want to look at features[]. And btw, I wouldn't mind a simple flag or counter in info telling whether any (or how many) features have been enabled, to avoid such iterations. It's just that the original feature mask was fully redundant with features[]. > @@ -397,6 +434,11 @@ void psr_ctxt_switch_to(struct domain *d) > if ( psr_cmt_enabled() ) > psr_assoc_rmid(®, d->arch.psr_rmid); > > + if ( psra->cos_mask ) > + psr_assoc_cos(®, d->arch.psr_cos_ids ? > + d->arch.psr_cos_ids[cpu_to_socket(smp_processor_id())] : > + 0, psra->cos_mask); I may have asked this question before, but if so you can see that the code above continues puzzling me: Under what conditions would psra->cos_mask be non-zero, but d->arch.psr_cos_ids be NULL? And why is zero the right value in that case? Also you need to deal with alignment issues here: Part of an expression at equal rank should align with one another. This implies that you want to move the 2nd argument on a new line (and the 3rd one would then better be moved to its own line too). Jan
On 17-04-05 09:23:50, Jan Beulich wrote: > >>> On 01.04.17 at 15:53, <yi.y.sun@linux.intel.com> wrote: > > +static void psr_assoc_init(void) > > { > > struct psr_assoc *psra = &this_cpu(psr_assoc); > > > > - if ( psr_cmt_enabled() ) > > + if ( psr_alloc_feat_enabled() ) > > + { > > + unsigned int socket = cpu_to_socket(smp_processor_id()); > > + const struct psr_socket_info *info = socket_info + socket; > > + unsigned int cos_max = get_max_cos_max(info); > > + > > + if ( feat_init_done(info) ) > > I think the use here is different from the one in the earlier patch: > While there looking at props[] appears to be desirable, I think here > you indeed only want to look at features[]. And btw, I wouldn't > mind a simple flag or counter in info telling whether any (or how > many) features have been enabled, to avoid such iterations. It's > just that the original feature mask was fully redundant with > features[]. > Per comment in previous patch, will add a 'flag' to indicate if any feature has been initialized. > > @@ -397,6 +434,11 @@ void psr_ctxt_switch_to(struct domain *d) > > if ( psr_cmt_enabled() ) > > psr_assoc_rmid(®, d->arch.psr_rmid); > > > > + if ( psra->cos_mask ) > > + psr_assoc_cos(®, d->arch.psr_cos_ids ? > > + d->arch.psr_cos_ids[cpu_to_socket(smp_processor_id())] : > > + 0, psra->cos_mask); > > I may have asked this question before, but if so you can see that > the code above continues puzzling me: Under what conditions > would psra->cos_mask be non-zero, but d->arch.psr_cos_ids be > NULL? And why is zero the right value in that case? > 'cos_mask' is initialized in 'psr_assoc_init' during cpu starting. The 'psr_cos_ids' is allocated during domain init. Here is soft a protection to handle abnormal case. Of course, we can use ASSERT to check it. > Also you need to deal with alignment issues here: Part of an > expression at equal rank should align with one another. This > implies that you want to move the 2nd argument on a new line > (and the 3rd one would then better be moved to its own line > too). > Ok, thanks! > Jan
>>> On 06.04.17 at 08:01, <yi.y.sun@linux.intel.com> wrote: > On 17-04-05 09:23:50, Jan Beulich wrote: >> >>> On 01.04.17 at 15:53, <yi.y.sun@linux.intel.com> wrote: >> > @@ -397,6 +434,11 @@ void psr_ctxt_switch_to(struct domain *d) >> > if ( psr_cmt_enabled() ) >> > psr_assoc_rmid(®, d->arch.psr_rmid); >> > >> > + if ( psra->cos_mask ) >> > + psr_assoc_cos(®, d->arch.psr_cos_ids ? >> > + d->arch.psr_cos_ids[cpu_to_socket(smp_processor_id())] : >> > + 0, psra->cos_mask); >> >> I may have asked this question before, but if so you can see that >> the code above continues puzzling me: Under what conditions >> would psra->cos_mask be non-zero, but d->arch.psr_cos_ids be >> NULL? And why is zero the right value in that case? >> > 'cos_mask' is initialized in 'psr_assoc_init' during cpu starting. The > 'psr_cos_ids' is allocated during domain init. Here is soft a protection > to handle abnormal case. Of course, we can use ASSERT to check it. Obviously a domain failing initialization won't ever make it here, so an ASSERT() is the maximum I'd consider reasonable here. We should try to not go overboard with assertions - I appreciate useful ones, but there is a line beyond which they end up being clutter. Jan
diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c index e422a23..3421219 100644 --- a/xen/arch/x86/psr.c +++ b/xen/arch/x86/psr.c @@ -49,6 +49,8 @@ */ #define MAX_COS_REG_CNT 128 +#define PSR_ASSOC_REG_SHIFT 32 + enum psr_feat_type { PSR_SOCKET_L3_CAT, PSR_SOCKET_L3_CDP, @@ -376,11 +378,39 @@ void psr_free_rmid(struct domain *d) d->arch.psr_rmid = 0; } -static inline void psr_assoc_init(void) +static unsigned int get_max_cos_max(const struct psr_socket_info *info) +{ + const struct feat_node *feat; + unsigned int cos_max = 0, i; + + for ( i = 0; i < PSR_SOCKET_MAX_FEAT; i++ ) + { + feat = info->features[i]; + if ( !feat ) + continue; + + cos_max = max(feat->props->cos_max, cos_max); + } + + return cos_max; +} + +static void psr_assoc_init(void) { struct psr_assoc *psra = &this_cpu(psr_assoc); - if ( psr_cmt_enabled() ) + if ( psr_alloc_feat_enabled() ) + { + unsigned int socket = cpu_to_socket(smp_processor_id()); + const struct psr_socket_info *info = socket_info + socket; + unsigned int cos_max = get_max_cos_max(info); + + if ( feat_init_done(info) ) + psra->cos_mask = ((1ull << get_count_order(cos_max)) - 1) << + PSR_ASSOC_REG_SHIFT; + } + + if ( psr_cmt_enabled() || psra->cos_mask ) rdmsrl(MSR_IA32_PSR_ASSOC, psra->val); } @@ -389,6 +419,13 @@ static inline void psr_assoc_rmid(uint64_t *reg, unsigned int rmid) *reg = (*reg & ~rmid_mask) | (rmid & rmid_mask); } +static void psr_assoc_cos(uint64_t *reg, unsigned int cos, + uint64_t cos_mask) +{ + *reg = (*reg & ~cos_mask) | + (((uint64_t)cos << PSR_ASSOC_REG_SHIFT) & cos_mask); +} + void psr_ctxt_switch_to(struct domain *d) { struct psr_assoc *psra = &this_cpu(psr_assoc); @@ -397,6 +434,11 @@ void psr_ctxt_switch_to(struct domain *d) if ( psr_cmt_enabled() ) psr_assoc_rmid(®, d->arch.psr_rmid); + if ( psra->cos_mask ) + psr_assoc_cos(®, d->arch.psr_cos_ids ? + d->arch.psr_cos_ids[cpu_to_socket(smp_processor_id())] : + 0, psra->cos_mask); + if ( reg != psra->val ) { wrmsrl(MSR_IA32_PSR_ASSOC, reg); @@ -422,14 +464,37 @@ int psr_set_l3_cbm(struct domain *d, unsigned int socket, return 0; } -int psr_domain_init(struct domain *d) +/* Called with domain lock held, no extra lock needed for 'psr_cos_ids' */ +static void psr_free_cos(struct domain *d) +{ + xfree(d->arch.psr_cos_ids); + d->arch.psr_cos_ids = NULL; +} + +static int psr_alloc_cos(struct domain *d) { + d->arch.psr_cos_ids = xzalloc_array(unsigned int, nr_sockets); + if ( !d->arch.psr_cos_ids ) + return -ENOMEM; + return 0; } +int psr_domain_init(struct domain *d) +{ + /* Init to success value */ + int ret = 0; + + if ( psr_alloc_feat_enabled() ) + ret = psr_alloc_cos(d); + + return ret; +} + void psr_domain_free(struct domain *d) { psr_free_rmid(d); + psr_free_cos(d); } static void __init init_psr(void)
This patch implements the Domain init/free and schedule flows. - When domain init, its psr resource should be allocated. - When domain free, its psr resource should be freed too. - When domain is scheduled, its COS ID on the socket should be set into ASSOC register to make corresponding COS MSR value work. Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com> --- v10: - remove 'cat_get_cos_max' as 'cos_max' is a feature property now which can be directly used. (suggested by Jan Beulich) - replace 'info->feat_mask' check to 'feat_init_done'. (suggested by Jan Beulich) v9: - rename 'l3_cat_get_cos_max' to 'cat_get_cos_max' to cover all CAT/CDP features. (suggested by Roger Pau) - replace feature list handling to feature array handling. (suggested by Roger Pau) - implement 'psr_alloc_cos' to match 'psr_free_cos'. (suggested by Wei Liu) - use 'psr_alloc_feat_enabled'. (suggested by Wei Liu) - fix coding style issue. (suggested by Wei Liu) - remove 'inline'. (suggested by Jan Beulich) - modify patch title to indicate 'L3 CAT'. (suggested by Jan Beulich) - remove 'psr_cos_ids' check in 'psr_free_cos'. (suggested by Jan Beulich) v6: - change 'PSR_ASSOC_REG_POS' to 'PSR_ASSOC_REG_SHIFT'. (suggested by Konrad Rzeszutek Wilk) v5: - rename 'feat_tmp' to 'feat'. (suggested by Jan Beulich) - define 'PSR_ASSOC_REG_POS'. (suggested by Jan Beulich) v4: - create this patch to make codes easier to understand. (suggested by Jan Beulich) --- xen/arch/x86/psr.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 68 insertions(+), 3 deletions(-)