| Message ID | 1493801063-38513-6-git-send-email-yi.y.sun@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 03.05.17 at 10:44, <yi.y.sun@linux.intel.com> wrote: > +static unsigned int get_max_cos_max(const struct psr_socket_info *info) > +{ > + unsigned int cos_max = 0, i; > + > + for ( i = 0; i < PSR_SOCKET_FEAT_NUM; i++ ) > + { > + const struct feat_node *feat = info->features[i]; > + if ( !feat ) Blank line between declaration(s) and statement(s) please. > + continue; > + > + cos_max = max(feat->cos_max, cos_max); And you're likely better off inverting the condition and dropping the "continue". > +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); > +} Indirection is normally only needed if a function needs to return more than one value. Is there a reason you can't have this one return the computed result? > @@ -376,6 +412,14 @@ void psr_ctxt_switch_to(struct domain *d) > if ( psr_cmt_enabled() ) > psr_assoc_rmid(®, d->arch.psr_rmid); > > + /* IDLE domain's 'psr_cos_ids' is NULL so we set default value for it. */ > + if ( psra->cos_mask ) > + psr_assoc_cos(®, > + d->arch.psr_cos_ids ? > + d->arch.psr_cos_ids[cpu_to_socket(smp_processor_id())] : > + 0, While this doesn't really conflict with our coding style, it makes reading harder than necessary. Please use either if ( psra->cos_mask ) psr_assoc_cos(®, (d->arch.psr_cos_ids ? d->arch.psr_cos_ids[cpu_to_socket(smp_processor_id())] : 0), or if ( psra->cos_mask ) psr_assoc_cos(®, d->arch.psr_cos_ids ? d->arch.psr_cos_ids[cpu_to_socket(smp_processor_id())] : 0, to allow immediate recognition that it is a single argument's expression that spans three lines. As to the idle domain aspect - is there a strict need to write a new value for the idle domain at all? I.e. can't you just skip the write in that case, knowing you'll write a proper value anyway once the next non-idle vCPU gets scheduled here? Which then raises the question on d->arch.psr_cos_ids being NULL - is that strictly only possible for the idle domain, or are there also other cases? This determines how the if() condition should be re-written ... > @@ -401,14 +445,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; > +} Along the lines of the above - do we really need to fail domain creation if we can't alloc psr_cos_ids? Granted there'll be other allocation failures, but from an abstract pov this is an optional feature, and hence the domain could do fine without. Jan
On 17-05-30 07:26:55, Jan Beulich wrote: > >>> On 03.05.17 at 10:44, <yi.y.sun@linux.intel.com> wrote: > > +static unsigned int get_max_cos_max(const struct psr_socket_info *info) > > +{ > > + unsigned int cos_max = 0, i; > > + > > + for ( i = 0; i < PSR_SOCKET_FEAT_NUM; i++ ) > > + { > > + const struct feat_node *feat = info->features[i]; > > + if ( !feat ) > > Blank line between declaration(s) and statement(s) please. > > > + continue; > > + > > + cos_max = max(feat->cos_max, cos_max); > > And you're likely better off inverting the condition and dropping > the "continue". > Will modify the above points. > > +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); > > +} > > Indirection is normally only needed if a function needs to return > more than one value. Is there a reason you can't have this one > return the computed result? > This is inherited from old code. I think it is to avoid indentation issue in caller below. Anyway, I will modify it according to your comment. > > @@ -376,6 +412,14 @@ void psr_ctxt_switch_to(struct domain *d) > > if ( psr_cmt_enabled() ) > > psr_assoc_rmid(®, d->arch.psr_rmid); > > > > + /* IDLE domain's 'psr_cos_ids' is NULL so we set default value for it. */ > > + if ( psra->cos_mask ) > > + psr_assoc_cos(®, > > + d->arch.psr_cos_ids ? > > + d->arch.psr_cos_ids[cpu_to_socket(smp_processor_id())] : > > + 0, > > While this doesn't really conflict with our coding style, it makes > reading harder than necessary. Please use either > > if ( psra->cos_mask ) > psr_assoc_cos(®, > (d->arch.psr_cos_ids ? > d->arch.psr_cos_ids[cpu_to_socket(smp_processor_id())] : > 0), > > or > > if ( psra->cos_mask ) > psr_assoc_cos(®, > d->arch.psr_cos_ids > ? d->arch.psr_cos_ids[cpu_to_socket(smp_processor_id())] > : 0, > > to allow immediate recognition that it is a single argument's expression > that spans three lines. > Thank you! > As to the idle domain aspect - is there a strict need to write a new > value for the idle domain at all? I.e. can't you just skip the write in > that case, knowing you'll write a proper value anyway once the > next non-idle vCPU gets scheduled here? Which then raises the > question on d->arch.psr_cos_ids being NULL - is that strictly only > possible for the idle domain, or are there also other cases? This > determines how the if() condition should be re-written ... > I agree with you that IDLE domain can be skipped. But considering that some domains' 'psr_cos_ids' may fail to be allocated, we have to set default value for these domains. So, I think we should keep this but skip IDLE domain in 'psr_ctxt_switch_to' caller. > > @@ -401,14 +445,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; > > +} > > Along the lines of the above - do we really need to fail domain > creation if we can't alloc psr_cos_ids? Granted there'll be other > allocation failures, but from an abstract pov this is an optional > feature, and hence the domain could do fine without. > Ok, will modiy related codes. > Jan
>>> On 31.05.17 at 08:37, <yi.y.sun@linux.intel.com> wrote: > On 17-05-30 07:26:55, Jan Beulich wrote: >> >>> On 03.05.17 at 10:44, <yi.y.sun@linux.intel.com> wrote: >> > @@ -376,6 +412,14 @@ void psr_ctxt_switch_to(struct domain *d) >> > if ( psr_cmt_enabled() ) >> > psr_assoc_rmid(®, d->arch.psr_rmid); >> > >> > + /* IDLE domain's 'psr_cos_ids' is NULL so we set default value for it. */ >> > + if ( psra->cos_mask ) >> > + psr_assoc_cos(®, >> > + d->arch.psr_cos_ids ? >> > + d->arch.psr_cos_ids[cpu_to_socket(smp_processor_id())] : >> > + 0, >> >> As to the idle domain aspect - is there a strict need to write a new >> value for the idle domain at all? I.e. can't you just skip the write in >> that case, knowing you'll write a proper value anyway once the >> next non-idle vCPU gets scheduled here? Which then raises the >> question on d->arch.psr_cos_ids being NULL - is that strictly only >> possible for the idle domain, or are there also other cases? This >> determines how the if() condition should be re-written ... >> > I agree with you that IDLE domain can be skipped. But considering that some > domains' 'psr_cos_ids' may fail to be allocated, we have to set default value > for these domains. So, I think we should keep this but skip IDLE domain in > 'psr_ctxt_switch_to' caller. There don't need to be two conditionals, i.e. using the one here is fine. But the comment then shouldn't give the impression the idle domain is the only case to consider. Jan
diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c index b73856e..bda325d 100644 --- a/xen/arch/x86/psr.c +++ b/xen/arch/x86/psr.c @@ -50,6 +50,8 @@ */ #define MAX_COS_REG_CNT 128 +#define PSR_ASSOC_REG_SHIFT 32 + /* * Every PSR feature uses some COS registers for each COS ID, e.g. CDP uses 2 * COS registers (DATA and CODE) for one COS ID, but CAT uses 1 COS register. @@ -355,11 +357,38 @@ 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) +{ + unsigned int cos_max = 0, i; + + for ( i = 0; i < PSR_SOCKET_FEAT_NUM; i++ ) + { + const struct feat_node *feat = info->features[i]; + if ( !feat ) + continue; + + cos_max = max(feat->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 ( info->feat_init ) + 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); } @@ -368,6 +397,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); @@ -376,6 +412,14 @@ void psr_ctxt_switch_to(struct domain *d) if ( psr_cmt_enabled() ) psr_assoc_rmid(®, d->arch.psr_rmid); + /* IDLE domain's 'psr_cos_ids' is NULL so we set default value for it. */ + 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); @@ -401,14 +445,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> --- v11: - replace 'feat_init_done()' to 'feat_init' flag. (suggested by Jan Beulich) - adjust parameters positions when calling 'psr_assoc_cos'. (suggested by Jan Beulich) - add comment to explain why to check 'psr_cos_ids'. 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 | 73 +++++++++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 70 insertions(+), 3 deletions(-)