| Message ID | 1489662495-5375-10-git-send-email-yi.y.sun@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 16.03.17 at 12:07, <yi.y.sun@linux.intel.com> wrote: > --- a/xen/arch/x86/domctl.c > +++ b/xen/arch/x86/domctl.c > @@ -1437,21 +1437,21 @@ long arch_do_domctl( > switch ( domctl->u.psr_cat_op.cmd ) > { > case XEN_DOMCTL_PSR_CAT_OP_SET_L3_CBM: > - ret = psr_set_l3_cbm(d, domctl->u.psr_cat_op.target, > - domctl->u.psr_cat_op.data, > - PSR_CBM_TYPE_L3); > + ret = psr_set_val(d, domctl->u.psr_cat_op.target, > + (uint32_t)domctl->u.psr_cat_op.data, The cast here is pointless, but - along the lines of the comment on the earlier patch - indicates a problem: You silently ignore the upper 32-bit the caller handed you. I think for forward compatibility it would be better if you checked they're zero. And in such a check you could then use a cast which I would not grumble about: if ( domctl->u.psr_cat_op.data != (uint32_t)domctl->u.psr_cat_op.data ) return -E...; > --- a/xen/arch/x86/psr.c > +++ b/xen/arch/x86/psr.c > @@ -578,15 +578,203 @@ int psr_get_val(struct domain *d, unsigned int socket, > return 0; > } > > -int psr_set_l3_cbm(struct domain *d, unsigned int socket, > - uint64_t cbm, enum cbm_type type) > +/* Set value functions */ > +static unsigned int get_cos_num(const struct psr_socket_info *info) > { > return 0; > } > > +static int gather_val_array(uint32_t val[], > + uint32_t array_len, I think I've mentioned this before - please avoid fixed width types where they're no needed. In the case here the array wants it, but the length can easily be unsigned int. > + const struct psr_socket_info *info, > + unsigned int old_cos) > +{ > + return -EINVAL; > +} > + > +static int insert_new_val_to_array(uint32_t val[], insert_new_val_into_array()? (and whether "new" needs to be part of the name is then even questionable) > +static int find_cos(const uint32_t val[], uint32_t array_len, > + enum psr_feat_type feat_type, > + const struct psr_socket_info *info) > +{ > + ASSERT(spin_is_locked((spinlock_t *)(&info->ref_lock))); Excuse me, but no, this is another of those really bad casts. I guess you've added it to deal with info being pointer-to-const. In such a case you should instead drop the const, unless the consuming function can be made take a pointer-to-const (which isn't the case here, as check_lock() wants to write into the structure). > +int psr_set_val(struct domain *d, unsigned int socket, > + uint32_t val, enum cbm_type type) > +{ > + unsigned int old_cos; > + int cos, ret; > + unsigned int *ref; > + uint32_t *val_array; > + struct psr_socket_info *info = get_socket_info(socket); > + uint32_t array_len; > + enum psr_feat_type feat_type; > + > + if ( IS_ERR(info) ) > + return PTR_ERR(info); > + > + feat_type = psr_cbm_type_to_feat_type(type); > + if ( !test_bit(feat_type, &info->feat_mask) ) > + return -ENOENT; > + > + /* > + * Step 0: > + * old_cos means the COS ID current domain is using. By default, it is 0. > + * > + * For every COS ID, there is a reference count to record how many domains > + * are using the COS register corresponding to this COS ID. > + * - If ref[old_cos] is 0, that means this COS is not used by any domain. > + * - If ref[old_cos] is 1, that means this COS is only used by current > + * domain. > + * - If ref[old_cos] is more than 1, that mean multiple domains are using > + * this COS. > + */ > + old_cos = d->arch.psr_cos_ids[socket]; > + ASSERT(old_cos < MAX_COS_REG_CNT && old_cos >= 0); The right side is always true and should hence be dropped. > + ref = info->cos_ref; > + > + /* > + * Step 1: > + * Gather a value array to store all features cos_reg_val[old_cos]. > + * And, set the input new val into array according to the feature's > + * position in array. > + */ > + array_len = get_cos_num(info); > + val_array = xzalloc_array(uint32_t, array_len); > + if ( !val_array ) > + return -ENOMEM; > + > + if ( (ret = gather_val_array(val_array, array_len, info, old_cos)) != 0 ) > + goto free_array; > + > + if ( (ret = insert_new_val_to_array(val_array, array_len, info, > + feat_type, type, val)) != 0 ) > + goto free_array; > + > + spin_lock(&info->ref_lock); Am I right in understanding that up to here operations are not risking to race merely because they're inside the domctl lock? If so, this needs to be spelled out, so we have a chance of noticing the dependency when we break up that lock (which we have plans for doing). > + /* > + * Step 2: > + * Try to find if there is already a COS ID on which all features' values Btw., I think this spelling ("features' values") is the right one - please go through and make it consistent everywhere, including in the patch description(s). > + * are same as the array. Then, we can reuse this COS ID. > + */ > + cos = find_cos(val_array, array_len, feat_type, info); > + if ( cos == old_cos ) > + { > + ret = 0; > + goto unlock_free_array; > + } > + else if ( cos >= 0 ) Pointless "else". > + goto cos_found; I think it would be better not to use goto here, other than for the error handling parts (where I don't really like it either, but I do accept it since others think that's the least ugly way). > + /* > + * Step 3: > + * If fail to find, we need pick an available COS ID. I think you've corrected this somewhat strange wording at the start of the comment here elsewhere already. I can only repeat that respective comments given for one location should always be extended to the entire series. > + * In fact, only COS ID which ref is 1 or 0 can be picked for current > + * domain. If old_cos is not 0 and its ref==1, that means only current > + * domain is using this old_cos ID. So, this old_cos ID certainly can > + * be reused by current domain. Ref==0 means there is no any domain > + * using this COS ID. So it can be used for current domain too. > + */ > + cos = pick_avail_cos(info, val_array, array_len, old_cos, feat_type); > + if ( cos < 0 ) > + { > + ret = cos; > + goto unlock_free_array; > + } > + > + /* > + * Step 4: > + * Write all features MSRs according to the COS ID. > + */ > + ret = write_psr_msr(socket, cos, val, type, feat_type); > + if ( ret ) > + goto unlock_free_array; > + > +cos_found: Style (also the other labels further down). > + /* > + * Step 5: > + * Update ref according to COS ID. > + */ > + ref[cos]++; > + ASSERT(!cos || ref[cos]); > + ASSERT(!old_cos || ref[old_cos]); > + ref[old_cos]--; > + spin_unlock(&info->ref_lock); > + > + /* > + * Step 6: > + * Save the COS ID into current domain's psr_cos_ids[] so that we can know > + * which COS the domain is using on the socket. One domain can only use > + * one COS ID at same time on each socket. > + */ > + d->arch.psr_cos_ids[socket] = cos; > + goto free_array; > + > +unlock_free_array: > + spin_unlock(&info->ref_lock); > +free_array: > + xfree(val_array); > + return ret; > +} I think overall things would look better if the successful path was the straight (goto-free) one. > /* Called with domain lock held, no extra lock needed for 'psr_cos_ids' */ > static void psr_free_cos(struct domain *d) > { > + unsigned int socket, cos; > + > + if ( !socket_info || !d->arch.psr_cos_ids ) > + return; Can d->arch.psr_cos_ids be non-NULL when !socket_info? If not, check only the former with an if(), and ASSERT() the latter. > + /* Domain is destroied so its cos_ref should be decreased. */ > + for ( socket = 0; socket < nr_sockets; socket++ ) > + { > + struct psr_socket_info *info; > + > + /* cos 0 is default one which does not need be handled. */ > + cos = d->arch.psr_cos_ids[socket]; > + if ( cos == 0 ) > + continue; > + > + /* > + * If domain uses other cos ids, all corresponding refs must have been > + * increased 1 for this domain. So, we need decrease them. ... by 1 ... need to ... I also question the presence of the word "must" in here. Jan
On 17-03-27 03:59:32, Jan Beulich wrote: > >>> On 16.03.17 at 12:07, <yi.y.sun@linux.intel.com> wrote: > > --- a/xen/arch/x86/domctl.c > > +++ b/xen/arch/x86/domctl.c > > @@ -1437,21 +1437,21 @@ long arch_do_domctl( > > switch ( domctl->u.psr_cat_op.cmd ) > > { > > case XEN_DOMCTL_PSR_CAT_OP_SET_L3_CBM: > > - ret = psr_set_l3_cbm(d, domctl->u.psr_cat_op.target, > > - domctl->u.psr_cat_op.data, > > - PSR_CBM_TYPE_L3); > > + ret = psr_set_val(d, domctl->u.psr_cat_op.target, > > + (uint32_t)domctl->u.psr_cat_op.data, > > The cast here is pointless, but - along the lines of the comment > on the earlier patch - indicates a problem: You silently ignore the > upper 32-bit the caller handed you. I think for forward > compatibility it would be better if you checked they're zero. And > in such a check you could then use a cast which I would not > grumble about: > > if ( domctl->u.psr_cat_op.data != (uint32_t)domctl->u.psr_cat_op.data ) > return -E...; > Thanks for the suggestion! I will check the 'data' for both get_val/set_val. > > --- a/xen/arch/x86/psr.c > > +++ b/xen/arch/x86/psr.c [...] > > +static int insert_new_val_to_array(uint32_t val[], > > insert_new_val_into_array()? > > (and whether "new" needs to be part of the name is then even > questionable) > Hmm, will remove 'new'. > > +static int find_cos(const uint32_t val[], uint32_t array_len, > > + enum psr_feat_type feat_type, > > + const struct psr_socket_info *info) > > +{ > > + ASSERT(spin_is_locked((spinlock_t *)(&info->ref_lock))); > > Excuse me, but no, this is another of those really bad casts. I > guess you've added it to deal with info being pointer-to-const. > In such a case you should instead drop the const, unless the > consuming function can be made take a pointer-to-const (which > isn't the case here, as check_lock() wants to write into the > structure). > Sorry for this. Will pass '&info->ref_lock' as a parameter into the function to check it. [...] > > + ref = info->cos_ref; > > + > > + /* > > + * Step 1: > > + * Gather a value array to store all features cos_reg_val[old_cos]. > > + * And, set the input new val into array according to the feature's > > + * position in array. > > + */ > > + array_len = get_cos_num(info); > > + val_array = xzalloc_array(uint32_t, array_len); > > + if ( !val_array ) > > + return -ENOMEM; > > + > > + if ( (ret = gather_val_array(val_array, array_len, info, old_cos)) != 0 ) > > + goto free_array; > > + > > + if ( (ret = insert_new_val_to_array(val_array, array_len, info, > > + feat_type, type, val)) != 0 ) > > + goto free_array; > > + > > + spin_lock(&info->ref_lock); > > Am I right in understanding that up to here operations are not > risking to race merely because they're inside the domctl lock? If so, > this needs to be spelled out, so we have a chance of noticing the > dependency when we break up that lock (which we have plans for > doing). > Yes, you are right. I will add comments. > > + /* > > + * Step 2: > > + * Try to find if there is already a COS ID on which all features' values > > Btw., I think this spelling ("features' values") is the right one - please > go through and make it consistent everywhere, including in the patch > description(s). > Sure, I will do it. > > + * are same as the array. Then, we can reuse this COS ID. > > + */ > > + cos = find_cos(val_array, array_len, feat_type, info); > > + if ( cos == old_cos ) > > + { > > + ret = 0; > > + goto unlock_free_array; > > + } > > + else if ( cos >= 0 ) > > Pointless "else". > Ok. > > + goto cos_found; > > I think it would be better not to use goto here, other than for the > error handling parts (where I don't really like it either, but I do > accept it since others think that's the least ugly way). > Sorry, I don't understand your meaning here. DYM I should goto error handling route? 'cos >= 0' means a valid cos id has been found. Then, we should do the 'cos_found' process to handle ref and set cos id into domain. > > + /* > > + * Step 3: > > + * If fail to find, we need pick an available COS ID. > > I think you've corrected this somewhat strange wording at the start > of the comment here elsewhere already. I can only repeat that > respective comments given for one location should always be > extended to the entire series. > I will try best to check entire series to keep it consistent. > > + * In fact, only COS ID which ref is 1 or 0 can be picked for current > > + * domain. If old_cos is not 0 and its ref==1, that means only current > > + * domain is using this old_cos ID. So, this old_cos ID certainly can > > + * be reused by current domain. Ref==0 means there is no any domain > > + * using this COS ID. So it can be used for current domain too. > > + */ > > + cos = pick_avail_cos(info, val_array, array_len, old_cos, feat_type); > > + if ( cos < 0 ) > > + { > > + ret = cos; > > + goto unlock_free_array; > > + } > > + > > + /* > > + * Step 4: > > + * Write all features MSRs according to the COS ID. > > + */ > > + ret = write_psr_msr(socket, cos, val, type, feat_type); > > + if ( ret ) > > + goto unlock_free_array; > > + > > +cos_found: > > Style (also the other labels further down). > Will correct all of them. > > + /* > > + * Step 5: > > + * Update ref according to COS ID. > > + */ > > + ref[cos]++; > > + ASSERT(!cos || ref[cos]); > > + ASSERT(!old_cos || ref[old_cos]); > > + ref[old_cos]--; > > + spin_unlock(&info->ref_lock); > > + > > + /* > > + * Step 6: > > + * Save the COS ID into current domain's psr_cos_ids[] so that we can know > > + * which COS the domain is using on the socket. One domain can only use > > + * one COS ID at same time on each socket. > > + */ > > + d->arch.psr_cos_ids[socket] = cos; > > + goto free_array; > > + > > +unlock_free_array: > > + spin_unlock(&info->ref_lock); > > +free_array: > > + xfree(val_array); > > + return ret; > > +} > > I think overall things would look better if the successful path was the > straight (goto-free) one. > DYM change 'free_array' to 'free'? > > /* Called with domain lock held, no extra lock needed for 'psr_cos_ids' */ > > static void psr_free_cos(struct domain *d) > > { > > + unsigned int socket, cos; > > + > > + if ( !socket_info || !d->arch.psr_cos_ids ) > > + return; > > Can d->arch.psr_cos_ids be non-NULL when !socket_info? If not, > check only the former with an if(), and ASSERT() the latter. > In normal case, 'psr_cos_ids' should not be NULL when entering this function. So, I think ASSERT() is a good option. Thanks! > > + /* Domain is destroied so its cos_ref should be decreased. */ > > + for ( socket = 0; socket < nr_sockets; socket++ ) > > + { > > + struct psr_socket_info *info; > > + > > + /* cos 0 is default one which does not need be handled. */ > > + cos = d->arch.psr_cos_ids[socket]; > > + if ( cos == 0 ) > > + continue; > > + > > + /* > > + * If domain uses other cos ids, all corresponding refs must have been > > + * increased 1 for this domain. So, we need decrease them. > > ... by 1 ... need to ... I also question the presence of the word > "must" in here. > After considering this comments again, I think the purpose of this piece of codes has been explained outer the circle. So, I think this comment can be removed. Is that OK for you? > Jan
>>> On 28.03.17 at 03:21, <yi.y.sun@linux.intel.com> wrote: > On 17-03-27 03:59:32, Jan Beulich wrote: >> >>> On 16.03.17 at 12:07, <yi.y.sun@linux.intel.com> wrote: >> > --- a/xen/arch/x86/domctl.c >> > +++ b/xen/arch/x86/domctl.c >> > @@ -1437,21 +1437,21 @@ long arch_do_domctl( >> > switch ( domctl->u.psr_cat_op.cmd ) >> > { >> > case XEN_DOMCTL_PSR_CAT_OP_SET_L3_CBM: >> > - ret = psr_set_l3_cbm(d, domctl->u.psr_cat_op.target, >> > - domctl->u.psr_cat_op.data, >> > - PSR_CBM_TYPE_L3); >> > + ret = psr_set_val(d, domctl->u.psr_cat_op.target, >> > + (uint32_t)domctl->u.psr_cat_op.data, >> >> The cast here is pointless, but - along the lines of the comment >> on the earlier patch - indicates a problem: You silently ignore the >> upper 32-bit the caller handed you. I think for forward >> compatibility it would be better if you checked they're zero. And >> in such a check you could then use a cast which I would not >> grumble about: >> >> if ( domctl->u.psr_cat_op.data != (uint32_t)domctl->u.psr_cat_op.data ) >> return -E...; >> > Thanks for the suggestion! I will check the 'data' for both get_val/set_val. There's no data to check on the "get" path - you return data there. >> > +static int find_cos(const uint32_t val[], uint32_t array_len, >> > + enum psr_feat_type feat_type, >> > + const struct psr_socket_info *info) >> > +{ >> > + ASSERT(spin_is_locked((spinlock_t *)(&info->ref_lock))); >> >> Excuse me, but no, this is another of those really bad casts. I >> guess you've added it to deal with info being pointer-to-const. >> In such a case you should instead drop the const, unless the >> consuming function can be made take a pointer-to-const (which >> isn't the case here, as check_lock() wants to write into the >> structure). >> > Sorry for this. Will pass '&info->ref_lock' as a parameter into the function > to check it. That's suitable if you don't need "info" for anything else. Otherwise - as said - you should drop const from info. >> > + * are same as the array. Then, we can reuse this COS ID. >> > + */ >> > + cos = find_cos(val_array, array_len, feat_type, info); >> > + if ( cos == old_cos ) >> > + { >> > + ret = 0; >> > + goto unlock_free_array; >> > + } >> > + else if ( cos >= 0 ) >> >> Pointless "else". >> > Ok. > >> > + goto cos_found; >> >> I think it would be better not to use goto here, other than for the >> error handling parts (where I don't really like it either, but I do >> accept it since others think that's the least ugly way). >> > Sorry, I don't understand your meaning here. DYM I should goto error handling > route? 'cos >= 0' means a valid cos id has been found. Then, we should do the > 'cos_found' process to handle ref and set cos id into domain. Using goto for error handling is acceptable, as said. But please use if ( cos < 0 ) { } followed by the code that's now after the cos_found label. >> > + /* >> > + * Step 5: >> > + * Update ref according to COS ID. >> > + */ >> > + ref[cos]++; >> > + ASSERT(!cos || ref[cos]); >> > + ASSERT(!old_cos || ref[old_cos]); >> > + ref[old_cos]--; >> > + spin_unlock(&info->ref_lock); >> > + >> > + /* >> > + * Step 6: >> > + * Save the COS ID into current domain's psr_cos_ids[] so that we can know >> > + * which COS the domain is using on the socket. One domain can only use >> > + * one COS ID at same time on each socket. >> > + */ >> > + d->arch.psr_cos_ids[socket] = cos; >> > + goto free_array; >> > + >> > +unlock_free_array: >> > + spin_unlock(&info->ref_lock); >> > +free_array: >> > + xfree(val_array); >> > + return ret; >> > +} >> >> I think overall things would look better if the successful path was the >> straight (goto-free) one. >> > DYM change 'free_array' to 'free'? My remark was not about label names, but about code flow: Please use goto only for error paths, unless not using it on normal paths means unacceptable resulting code (use your own judgment). >> > /* Called with domain lock held, no extra lock needed for 'psr_cos_ids' */ >> > static void psr_free_cos(struct domain *d) >> > { >> > + unsigned int socket, cos; >> > + >> > + if ( !socket_info || !d->arch.psr_cos_ids ) >> > + return; >> >> Can d->arch.psr_cos_ids be non-NULL when !socket_info? If not, >> check only the former with an if(), and ASSERT() the latter. >> > In normal case, 'psr_cos_ids' should not be NULL when entering this function. > So, I think ASSERT() is a good option. Thanks! What does "normal" mean to you here? If, e.g. due to an error during domain creation, we can come here with the pointer being NULL, you must not ASSERT() but use if() instead. Please note that my earlier comment did _not_ suggest to replace the checking of d->arch.psr_cos_ids, but that of socket_info (as long as the answer to the question is "no"). >> > + /* Domain is destroied so its cos_ref should be decreased. */ >> > + for ( socket = 0; socket < nr_sockets; socket++ ) >> > + { >> > + struct psr_socket_info *info; >> > + >> > + /* cos 0 is default one which does not need be handled. */ >> > + cos = d->arch.psr_cos_ids[socket]; >> > + if ( cos == 0 ) >> > + continue; >> > + >> > + /* >> > + * If domain uses other cos ids, all corresponding refs must have been >> > + * increased 1 for this domain. So, we need decrease them. >> >> ... by 1 ... need to ... I also question the presence of the word >> "must" in here. >> > After considering this comments again, I think the purpose of this piece of > codes has been explained outer the circle. So, I think this comment can be > removed. Is that OK for you? If "outer the circle" means "outside of the loop", then yes, that earlier comment should be sufficient on its own. Jan
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c index e714d1d..76923c8 100644 --- a/xen/arch/x86/domctl.c +++ b/xen/arch/x86/domctl.c @@ -1437,21 +1437,21 @@ long arch_do_domctl( switch ( domctl->u.psr_cat_op.cmd ) { case XEN_DOMCTL_PSR_CAT_OP_SET_L3_CBM: - ret = psr_set_l3_cbm(d, domctl->u.psr_cat_op.target, - domctl->u.psr_cat_op.data, - PSR_CBM_TYPE_L3); + ret = psr_set_val(d, domctl->u.psr_cat_op.target, + (uint32_t)domctl->u.psr_cat_op.data, + PSR_CBM_TYPE_L3); break; case XEN_DOMCTL_PSR_CAT_OP_SET_L3_CODE: - ret = psr_set_l3_cbm(d, domctl->u.psr_cat_op.target, - domctl->u.psr_cat_op.data, - PSR_CBM_TYPE_L3_CODE); + ret = psr_set_val(d, domctl->u.psr_cat_op.target, + (uint32_t)domctl->u.psr_cat_op.data, + PSR_CBM_TYPE_L3_CODE); break; case XEN_DOMCTL_PSR_CAT_OP_SET_L3_DATA: - ret = psr_set_l3_cbm(d, domctl->u.psr_cat_op.target, - domctl->u.psr_cat_op.data, - PSR_CBM_TYPE_L3_DATA); + ret = psr_set_val(d, domctl->u.psr_cat_op.target, + (uint32_t)domctl->u.psr_cat_op.data, + PSR_CBM_TYPE_L3_DATA); break; case XEN_DOMCTL_PSR_CAT_OP_GET_L3_CBM: diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c index 53105f5..f8d4be4 100644 --- a/xen/arch/x86/psr.c +++ b/xen/arch/x86/psr.c @@ -578,15 +578,203 @@ int psr_get_val(struct domain *d, unsigned int socket, return 0; } -int psr_set_l3_cbm(struct domain *d, unsigned int socket, - uint64_t cbm, enum cbm_type type) +/* Set value functions */ +static unsigned int get_cos_num(const struct psr_socket_info *info) { return 0; } +static int gather_val_array(uint32_t val[], + uint32_t array_len, + const struct psr_socket_info *info, + unsigned int old_cos) +{ + return -EINVAL; +} + +static int insert_new_val_to_array(uint32_t val[], + uint32_t array_len, + const struct psr_socket_info *info, + enum psr_feat_type feat_type, + enum cbm_type type, + uint32_t new_val) +{ + return -EINVAL; +} + +static int find_cos(const uint32_t val[], uint32_t array_len, + enum psr_feat_type feat_type, + const struct psr_socket_info *info) +{ + ASSERT(spin_is_locked((spinlock_t *)(&info->ref_lock))); + return -ENOENT; +} + +static int pick_avail_cos(const struct psr_socket_info *info, + const uint32_t val[], uint32_t array_len, + unsigned int old_cos, + enum psr_feat_type feat_type) +{ + ASSERT(spin_is_locked((spinlock_t *)(&info->ref_lock))); + return -ENOENT; +} + +static int write_psr_msr(unsigned int socket, unsigned int cos, + uint32_t val, enum cbm_type type, + enum psr_feat_type feat_type) +{ + return -ENOENT; +} + +int psr_set_val(struct domain *d, unsigned int socket, + uint32_t val, enum cbm_type type) +{ + unsigned int old_cos; + int cos, ret; + unsigned int *ref; + uint32_t *val_array; + struct psr_socket_info *info = get_socket_info(socket); + uint32_t array_len; + enum psr_feat_type feat_type; + + if ( IS_ERR(info) ) + return PTR_ERR(info); + + feat_type = psr_cbm_type_to_feat_type(type); + if ( !test_bit(feat_type, &info->feat_mask) ) + return -ENOENT; + + /* + * Step 0: + * old_cos means the COS ID current domain is using. By default, it is 0. + * + * For every COS ID, there is a reference count to record how many domains + * are using the COS register corresponding to this COS ID. + * - If ref[old_cos] is 0, that means this COS is not used by any domain. + * - If ref[old_cos] is 1, that means this COS is only used by current + * domain. + * - If ref[old_cos] is more than 1, that mean multiple domains are using + * this COS. + */ + old_cos = d->arch.psr_cos_ids[socket]; + ASSERT(old_cos < MAX_COS_REG_CNT && old_cos >= 0); + + ref = info->cos_ref; + + /* + * Step 1: + * Gather a value array to store all features cos_reg_val[old_cos]. + * And, set the input new val into array according to the feature's + * position in array. + */ + array_len = get_cos_num(info); + val_array = xzalloc_array(uint32_t, array_len); + if ( !val_array ) + return -ENOMEM; + + if ( (ret = gather_val_array(val_array, array_len, info, old_cos)) != 0 ) + goto free_array; + + if ( (ret = insert_new_val_to_array(val_array, array_len, info, + feat_type, type, val)) != 0 ) + goto free_array; + + spin_lock(&info->ref_lock); + + /* + * Step 2: + * Try to find if there is already a COS ID on which all features' values + * are same as the array. Then, we can reuse this COS ID. + */ + cos = find_cos(val_array, array_len, feat_type, info); + if ( cos == old_cos ) + { + ret = 0; + goto unlock_free_array; + } + else if ( cos >= 0 ) + goto cos_found; + + /* + * Step 3: + * If fail to find, we need pick an available COS ID. + * In fact, only COS ID which ref is 1 or 0 can be picked for current + * domain. If old_cos is not 0 and its ref==1, that means only current + * domain is using this old_cos ID. So, this old_cos ID certainly can + * be reused by current domain. Ref==0 means there is no any domain + * using this COS ID. So it can be used for current domain too. + */ + cos = pick_avail_cos(info, val_array, array_len, old_cos, feat_type); + if ( cos < 0 ) + { + ret = cos; + goto unlock_free_array; + } + + /* + * Step 4: + * Write all features MSRs according to the COS ID. + */ + ret = write_psr_msr(socket, cos, val, type, feat_type); + if ( ret ) + goto unlock_free_array; + +cos_found: + /* + * Step 5: + * Update ref according to COS ID. + */ + ref[cos]++; + ASSERT(!cos || ref[cos]); + ASSERT(!old_cos || ref[old_cos]); + ref[old_cos]--; + spin_unlock(&info->ref_lock); + + /* + * Step 6: + * Save the COS ID into current domain's psr_cos_ids[] so that we can know + * which COS the domain is using on the socket. One domain can only use + * one COS ID at same time on each socket. + */ + d->arch.psr_cos_ids[socket] = cos; + goto free_array; + +unlock_free_array: + spin_unlock(&info->ref_lock); +free_array: + xfree(val_array); + return ret; +} + /* Called with domain lock held, no extra lock needed for 'psr_cos_ids' */ static void psr_free_cos(struct domain *d) { + unsigned int socket, cos; + + if ( !socket_info || !d->arch.psr_cos_ids ) + return; + + /* Domain is destroied so its cos_ref should be decreased. */ + for ( socket = 0; socket < nr_sockets; socket++ ) + { + struct psr_socket_info *info; + + /* cos 0 is default one which does not need be handled. */ + cos = d->arch.psr_cos_ids[socket]; + if ( cos == 0 ) + continue; + + /* + * If domain uses other cos ids, all corresponding refs must have been + * increased 1 for this domain. So, we need decrease them. + */ + info = socket_info + socket; + spin_lock(&info->ref_lock); + ASSERT(!cos || info->cos_ref[cos]); + info->cos_ref[cos]--; + spin_unlock(&info->ref_lock); + } + xfree(d->arch.psr_cos_ids); d->arch.psr_cos_ids = NULL; } diff --git a/xen/include/asm-x86/psr.h b/xen/include/asm-x86/psr.h index cd00733..d2262d9 100644 --- a/xen/include/asm-x86/psr.h +++ b/xen/include/asm-x86/psr.h @@ -74,8 +74,8 @@ int psr_get_info(unsigned int socket, enum cbm_type type, uint32_t data[], unsigned int array_len); int psr_get_val(struct domain *d, unsigned int socket, uint32_t *val, enum cbm_type type); -int psr_set_l3_cbm(struct domain *d, unsigned int socket, - uint64_t cbm, enum cbm_type type); +int psr_set_val(struct domain *d, unsigned int socket, + uint32_t val, enum cbm_type type); int psr_domain_init(struct domain *d); void psr_domain_free(struct domain *d);
As set value flow is the most complicated one in psr, it will be divided to some patches to make things clearer. This patch implements the set value framework to show a whole picture firstly. It also changes domctl interface to make it more general. To make the set value flow be general and can support multiple features at same time, it includes below steps: 1. Get COS ID that current domain is using. 2. Gather a value array to store all features current value into it and replace the current value of the feature which is being set to the new input value. 3. Find if there is already a COS ID on which all features' values are same as the array. Then, we can reuse this COS ID. 4. If fail to find, we need pick an available COS ID. Only COS ID which ref is 0 or 1 can be picked. 5. Write all features MSRs according to the COS ID. 6. Update ref according to COS ID. 7. Save the COS ID into current domain's psr_cos_ids[socket] so that we can know which COS the domain is using on the socket. So, some functions are abstracted and the callback functions will be implemented in next patches. Here is an example to understand the process. The CPU supports two featuers, e.g. L3 CAT and L2 CAT. User wants to set L3 CAT of Dom1 to 0x1ff. 1. Get the old_cos of Dom1 which is 0. L3 CAT is the first element of feature list. The COS registers values are below at this time. ------------------------------- | COS 0 | COS 1 | COS 2 | ... | ------------------------------- L3 CAT | 0x7ff | ... | ... | ... | ------------------------------- L2 CAT | 0xff | ... | ... | ... | ------------------------------- 2. Gather the value array and set new value into it: val[0]: 0x1ff val[1]: 0xff 3. It cannot find a matching COS. 4. Pick COS 1 to store the value set. 5. Write the COS 1 registers. The COS registers values are changed to below now. ------------------------------- | COS 0 | COS 1 | COS 2 | ... | ------------------------------- L3 CAT | 0x7ff | 0x1ff | ... | ... | ------------------------------- L2 CAT | 0xff | 0xff | ... | ... | ------------------------------- 6. The ref[1] is increased to 1 because Dom1 is using it now. 7. Save 1 to Dom1's psr_cos_ids[socket]. Then, user wants to set L3 CAT of Dom2 to 0x1ff too. The old_cos of Dom2 is 0 too. Repeat above flow. The val array assembled is: val[0]: 0x1ff val[1]: 0xff So, it can find a matching COS, COS 1. Then, it can reuse COS 1 for Dom2. The ref[1] is increased to 2 now because both Dom1 and Dom2 are using this COS ID. Set 1 to Dom2's psr_cos_ids[socket]. Another thing need to emphasize is the context switch. When context switch happens, 'psr_ctxt_switch_to' is called by system to get domain's COS ID from 'psr_cos_ids[socket]'. But 'psr_cos_ids[socket]' is set at step 7 above. So, there are three scenarios, e.g.: 1. User calls domctl interface on Dom0 to set a COS ID 1 for Dom1 into its psr_cos_ids[]. Then, Dom1 is scheduled so that 'psr_ctxt_switch_to()' is called which makes COS ID 1 work. For this case, we do not any action. 2. Dom1 runs on CPU 1 and COS ID 1 is working. At same time, user calls domctl interface on Dom0 to set a new COS ID 2 for Dom1 into psr_cos_ids[]. After time slice ends, the Dom1 is scheduled again, the new COS ID 2 will work. 3. When a new COS ID is being set to psr_cos_ids[], 'psr_ctxt_switch_to()' is called to access the same psr_cos_ids[] member through 'psr_assoc_cos'. The COS ID is constrained by cos_mask so that it cannot exceeds the cos_max. So even the COS ID got here is wrong, it is still a workable ID (within cos_max). The functionality is still workable, only another valid CBM be effective for a short time. In next schedule, the correct CBM will take effect. All these cases will not cause race condition and no harm to system. The PSR features are to set cache capacity for a domain. The setting to cache is progressively effective. When the cache setting becomes really effective, the time slice to schedule a domain may have passed. So, even if a wrong COS ID is used to set ASSOC, only another valid CBM be effective for a short time during cache preparation time. The correct COS ID will take effect in a short time. This does not affect cache capacity setting much. Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com> --- v9: - use goto style error handling in 'psr_set_val'. (suggested by Wei Liu) - use ASSERT for checking old_cos. (suggested by Wei Liu and Jan Beulich) - fix coding style issue. (suggested by Wei Liu) - rename 'assemble_val_array' to 'combine_val_array' in pervious patch. (suggested by Wei Liu) - use 'spin_is_locked' to check ref_lock. (suggested by Roger Pau) - add an input parameter 'array_len' for 'write_psr_msr'. - check 'socket_info' and 'psr_cos_ids' in this patch. (suggested by Jan Beulich) - modify patch title to indicate 'L3 CAT'. (suggested by Jan Beulich) - fix commit message words. (suggested by Jan Beulich) - change 'assemble_val_array' to 'gather_val_array'. (suggested by Jan Beulich) - change 'set_new_val_to_array' to 'insert_new_val_to_array'. (suggested by Jan Beulich) - change parameter 'm' of 'insert_new_val_to_array' to 'new_val'. (suggested by Jan Beulich) - change 'write_psr_msr' to 'write_psr_msrs'. (suggested by Jan Beulich) - correct comments. (suggested by Jan Beulich) - remove unnecessary comments. (suggested by Jan Beulich) - adjust conditions after 'find_cos' to save a level of indentation. (suggested by Jan Beulich) - add 'ASSERT(!old_cos || ref[old_cos])'. (suggested by Jan Beulich) - move ASSERT() check into locked region. (suggested by Jan Beulich) - replace parameter '*val' to 'val[]' in some functions. (suggested by Jan Beulich) - change 'write_psr_msr' parameters to prepare to only set one new value for one feature. (suggested by Jan Beulich) - changes about 'uint64_t' to 'uint32_t'. (suggested by Jan Beulich) - add explanation about context switch. (suggested by Jan Beulich) v5: - modify commit message. (suggested by Jan Beulich) - return an error for all helper functions in set flow. (suggested by Jan Beulich) - remove unnecessary cast. (suggested by Jan Beulich) - divide 'get_old_set_new' to two functions, 'assemble_val_array' and 'set_new_val_to_array'. (suggested by Jan Beulich) - modify comments. (suggested by Jan Beulich) - adjust code format. (suggested by Jan Beulich) - change 'alloc_new_cos' to 'pick_avail_cos' to make name accurate. (suggested by Jan Beulich) - check feature type when entering 'psr_set_val'. (suggested by Jan Beulich) - use ASSERT to check ref. (suggested by Jan Beulich) - rename 'dat[]' to 'data[]'. (suggested by Jan Beulich) v4: - create this patch to make codes easier to understand. (suggested by Jan Beulich) --- xen/arch/x86/domctl.c | 18 ++--- xen/arch/x86/psr.c | 192 +++++++++++++++++++++++++++++++++++++++++++++- xen/include/asm-x86/psr.h | 4 +- 3 files changed, 201 insertions(+), 13 deletions(-)