| Message ID | 1487148579-7243-21-git-send-email-yi.y.sun@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Feb 15, 2017 at 04:49:35PM +0800, Yi Sun wrote: > This patch implements L2 CAT set value related callback functions > and domctl interface. > > Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com> > --- > v8: > - modify 'l2_cat_write_msr' to 'void'. > --- > xen/arch/x86/domctl.c | 6 +++ > xen/arch/x86/psr.c | 95 +++++++++++++++++++++++++++++++++++++++++ > xen/include/asm-x86/msr-index.h | 1 + > xen/include/public/domctl.h | 1 + > 4 files changed, 103 insertions(+) > > diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c > index 68c2d60..38dc087 100644 > --- a/xen/arch/x86/domctl.c > +++ b/xen/arch/x86/domctl.c > @@ -1439,6 +1439,12 @@ long arch_do_domctl( > PSR_CBM_TYPE_L3_DATA); > break; > > + case XEN_DOMCTL_PSR_CAT_OP_SET_L2_CBM: > + ret = psr_set_val(d, domctl->u.psr_cat_op.target, > + domctl->u.psr_cat_op.data, > + PSR_CBM_TYPE_L2); > + break; > + > case XEN_DOMCTL_PSR_CAT_OP_GET_L3_CBM: > ret = psr_get_val(d, domctl->u.psr_cat_op.target, > &domctl->u.psr_cat_op.data, > diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c > index 2adf62c..5604e03 100644 > --- a/xen/arch/x86/psr.c > +++ b/xen/arch/x86/psr.c > @@ -755,10 +755,105 @@ static bool l2_cat_get_val(const struct feat_node *feat, unsigned int cos, > return true; > } > > +static unsigned int l2_cat_get_cos_num(const struct feat_node *feat) > +{ > + /* L2 CAT uses one COS. */ > + return 1; > +} > + > +static int l2_cat_get_old_val(uint64_t val[], > + const struct feat_node *feat, > + unsigned int old_cos) > +{ > + if ( old_cos > feat->info.l2_cat_info.cos_max ) > + /* Use default value. */ > + old_cos = 0; > + > + val[0] = feat->cos_reg_val[old_cos]; > + > + return 0; > +} > + > +static int l2_cat_set_new_val(uint64_t val[], > + const struct feat_node *feat, > + enum cbm_type type, > + uint64_t m) > +{ > + if ( !psr_check_cbm(feat->info.l2_cat_info.cbm_len, m) ) > + return -EINVAL; > + > + val[0] = m; > + > + return 0; > +} > + > +static int l2_cat_compare_val(const uint64_t val[], > + const struct feat_node *feat, > + unsigned int cos, bool *found) > +{ > + uint64_t l2_def_cbm; > + > + l2_def_cbm = (1ull << feat->info.l2_cat_info.cbm_len) - 1; > + > + if ( cos > feat->info.l2_cat_info.cos_max ) > + { > + if ( val[0] != l2_def_cbm ) > + { > + *found = false; > + return -ENOENT; > + } > + *found = true; > + } > + else > + *found = (val[0] == feat->cos_reg_val[cos]); > + > + return 0; The logic of this function is kind of weird IMHO, you seem to be able to return an error, and also a parameter that indicates success ("found"). Can't this be simplified to simply return an error code, and the found parameter removed? Roger.
On 17-02-28 15:25:39, Roger Pau Monn� wrote: > On Wed, Feb 15, 2017 at 04:49:35PM +0800, Yi Sun wrote: > > +static int l2_cat_compare_val(const uint64_t val[], > > + const struct feat_node *feat, > > + unsigned int cos, bool *found) > > +{ > > + uint64_t l2_def_cbm; > > + > > + l2_def_cbm = (1ull << feat->info.l2_cat_info.cbm_len) - 1; > > + > > + if ( cos > feat->info.l2_cat_info.cos_max ) > > + { > > + if ( val[0] != l2_def_cbm ) > > + { > > + *found = false; > > + return -ENOENT; > > + } > > + *found = true; > > + } > > + else > > + *found = (val[0] == feat->cos_reg_val[cos]); > > + > > + return 0; > > The logic of this function is kind of weird IMHO, you seem to be able to return > an error, and also a parameter that indicates success ("found"). Can't this be > simplified to simply return an error code, and the found parameter removed? > > Roger. As I explained in previous patch, the value must be default value if the COS ID exceeds the max ID. If not, we have to return error to exit the whole flow. That is the reason we return '-ENOENT'. In find_cos(), compare_val() is called to check if there is already a COS ID that all features values are same as input. All features in list should be checked. The 'found' is used record the final result, if all features values are same as input value array. You can see, after traversal of feature list, we return the cos if found is true. Of course, I can change the function to remove 'found' from the parameter. But is it so necessary? Thanks! static int find_cos(const uint64_t *val, uint32_t array_len, ... { ... for ( cos = 0; cos <= cos_max; cos++ ) { if ( cos && !ref[cos] ) continue; /* Not found, need find again from beginning. */ val_tmp = val; list_for_each_entry(feat, &info->feat_list, list) { /* * Compare value according to feature list order. * We must follow this order because value array is assembled * as this order in get_old_set_new(). */ ret = feat->ops.compare_val(val_tmp, feat, cos, &found); if ( ret < 0 ) return ret; /* If fail to match, go to next cos to compare. */ if ( !found ) break; val_tmp += feat->ops.get_cos_num(feat); if ( val_tmp - val > array_len ) return -EINVAL; } /* For this COS ID all entries in the values array did match. Use it. */ if ( found ) return cos; } ... }
On Wed, 2017-03-01 at 14:59 +0800, Yi Sun wrote: > On 17-02-28 15:25:39, Roger Pau Monn� wrote: > > The logic of this function is kind of weird IMHO, you seem to be > > able to return > > an error, and also a parameter that indicates success ("found"). > > Can't this be > > simplified to simply return an error code, and the found parameter > > removed? > > > In find_cos(), compare_val() is called to check if there is already a > COS ID > that all features values are same as input. All features in list > should be > checked. The 'found' is used record the final result, if all features > values are > same as input value array. You can see, after traversal of feature > list, we > return the cos if found is true. > So, you can, for instance, use different error values, and check for which one has been returned in the caller. Like, use ENOENT for 'invalid ID' as you're doing right now, and, say, ESRCH, for 'feature not found'. Is that right? > Of course, I can change the function to remove 'found' from the > parameter. > But is it so necessary? Thanks! > It's hard to tell whether it is "so necessary", but I agree with Roger that it would improve the code. :-) Regards, Dario
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c index 68c2d60..38dc087 100644 --- a/xen/arch/x86/domctl.c +++ b/xen/arch/x86/domctl.c @@ -1439,6 +1439,12 @@ long arch_do_domctl( PSR_CBM_TYPE_L3_DATA); break; + case XEN_DOMCTL_PSR_CAT_OP_SET_L2_CBM: + ret = psr_set_val(d, domctl->u.psr_cat_op.target, + domctl->u.psr_cat_op.data, + PSR_CBM_TYPE_L2); + break; + case XEN_DOMCTL_PSR_CAT_OP_GET_L3_CBM: ret = psr_get_val(d, domctl->u.psr_cat_op.target, &domctl->u.psr_cat_op.data, diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c index 2adf62c..5604e03 100644 --- a/xen/arch/x86/psr.c +++ b/xen/arch/x86/psr.c @@ -755,10 +755,105 @@ static bool l2_cat_get_val(const struct feat_node *feat, unsigned int cos, return true; } +static unsigned int l2_cat_get_cos_num(const struct feat_node *feat) +{ + /* L2 CAT uses one COS. */ + return 1; +} + +static int l2_cat_get_old_val(uint64_t val[], + const struct feat_node *feat, + unsigned int old_cos) +{ + if ( old_cos > feat->info.l2_cat_info.cos_max ) + /* Use default value. */ + old_cos = 0; + + val[0] = feat->cos_reg_val[old_cos]; + + return 0; +} + +static int l2_cat_set_new_val(uint64_t val[], + const struct feat_node *feat, + enum cbm_type type, + uint64_t m) +{ + if ( !psr_check_cbm(feat->info.l2_cat_info.cbm_len, m) ) + return -EINVAL; + + val[0] = m; + + return 0; +} + +static int l2_cat_compare_val(const uint64_t val[], + const struct feat_node *feat, + unsigned int cos, bool *found) +{ + uint64_t l2_def_cbm; + + l2_def_cbm = (1ull << feat->info.l2_cat_info.cbm_len) - 1; + + if ( cos > feat->info.l2_cat_info.cos_max ) + { + if ( val[0] != l2_def_cbm ) + { + *found = false; + return -ENOENT; + } + *found = true; + } + else + *found = (val[0] == feat->cos_reg_val[cos]); + + return 0; +} + +static bool l2_cat_fits_cos_max(const uint64_t val[], + const struct feat_node *feat, + unsigned int cos) +{ + uint64_t l2_def_cbm; + + l2_def_cbm = (1ull << feat->info.l2_cat_info.cbm_len) - 1; + + if ( cos > feat->info.l2_cat_info.cos_max && + val[0] != l2_def_cbm ) + /* + * Exceed cos_max and value to set is not default, + * return error. + */ + return false; + + return true; +} + +static void l2_cat_write_msr(unsigned int cos, const uint64_t val[], + struct feat_node *feat) +{ + if ( cos > feat->info.l2_cat_info.cos_max ) + return; + + if ( feat->cos_reg_val[cos] != val[0] ) + { + feat->cos_reg_val[cos] = val[0]; + wrmsrl(MSR_IA32_PSR_L2_MASK(cos), val[0]); + } + + return; +} + struct feat_ops l2_cat_ops = { .get_cos_max = l2_cat_get_cos_max, .get_feat_info = l2_cat_get_feat_info, .get_val = l2_cat_get_val, + .get_cos_num = l2_cat_get_cos_num, + .get_old_val = l2_cat_get_old_val, + .set_new_val = l2_cat_set_new_val, + .compare_val = l2_cat_compare_val, + .fits_cos_max = l2_cat_fits_cos_max, + .write_msr = l2_cat_write_msr, }; static void __init parse_psr_bool(char *s, char *value, char *feature, diff --git a/xen/include/asm-x86/msr-index.h b/xen/include/asm-x86/msr-index.h index 98dbff1..a41e63a 100644 --- a/xen/include/asm-x86/msr-index.h +++ b/xen/include/asm-x86/msr-index.h @@ -343,6 +343,7 @@ #define MSR_IA32_PSR_L3_MASK(n) (0x00000c90 + (n)) #define MSR_IA32_PSR_L3_MASK_CODE(n) (0x00000c90 + (n) * 2 + 1) #define MSR_IA32_PSR_L3_MASK_DATA(n) (0x00000c90 + (n) * 2) +#define MSR_IA32_PSR_L2_MASK(n) (0x00000d10 + (n)) /* Intel Model 6 */ #define MSR_P6_PERFCTR(n) (0x000000c1 + (n)) diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h index 8c183ba..523a2cd 100644 --- a/xen/include/public/domctl.h +++ b/xen/include/public/domctl.h @@ -1138,6 +1138,7 @@ struct xen_domctl_psr_cat_op { #define XEN_DOMCTL_PSR_CAT_OP_SET_L3_DATA 3 #define XEN_DOMCTL_PSR_CAT_OP_GET_L3_CODE 4 #define XEN_DOMCTL_PSR_CAT_OP_GET_L3_DATA 5 +#define XEN_DOMCTL_PSR_CAT_OP_SET_L2_CBM 6 #define XEN_DOMCTL_PSR_CAT_OP_GET_L2_CBM 7 uint32_t cmd; /* IN: XEN_DOMCTL_PSR_CAT_OP_* */ uint32_t target; /* IN */
This patch implements L2 CAT set value related callback functions and domctl interface. Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com> --- v8: - modify 'l2_cat_write_msr' to 'void'. --- xen/arch/x86/domctl.c | 6 +++ xen/arch/x86/psr.c | 95 +++++++++++++++++++++++++++++++++++++++++ xen/include/asm-x86/msr-index.h | 1 + xen/include/public/domctl.h | 1 + 4 files changed, 103 insertions(+)